Re: [PATCH/RFC] New module refcounting for net_proto_family

[Max Krasnyansky]

At 07:54 PM 2/18/2003, Rusty Russell wrote:
>In message <[e-mail]> you write:
>> At 07:46 PM 2/17/2003, David S. Miller wrote:
>> 
>> >After talking to Alexey, I don't like this patch.
>> >
>> >The new module subsystem was supposed to deal with things
>> >like this cleanly, and this patch is merely a hack to cover
>> >up for it's shortcomings.
>
>I don't quite understand.  
>
>There are some issue with this patch, however.
>
>Firstly, the owner field should probably be in struct proto_ops not
>struct socket, where the function pointers are.
struct proto_ops doesn't exists on its own without struct socket.
I think it make sense to simply keep track of the sockets but I don't 
see any problem with putting it in proto_ops.

struct sock is different though. callbacks are inside.

>The sk thing looks reasonable at first glance.  Getting a reference to
>npf->owner, then holding it for the socket is a little confusing, but
>an obvious optimization over a naive "get, use, drop, get".
That was an optimization indeed. There is no point in dropping reference
there.

>In sys_accept:
>
>> @@ -1196,9 +1198,13 @@
>>       if (!(newsock = sock_alloc())) 
>>               goto out_put;
>>  
>> -     newsock->type = sock->type;
>> -     newsock->ops = sock->ops;
>> +     newsock->type  = sock->type;
>> +     newsock->ops   = sock->ops;
>> +     newsock->owner = sock->owner;
>>  
>> +     try_module_get(sock->owner);
>> +     newsock->owner = sock->owner;
>> +     
>>       err = sock->ops->accept(sock, newsock, sock->file->f_flags);
>>       if (err < 0)
>>               goto out_release;
>
>You still need to check the result of try_module_get, and fail if it
>fails.  The *only* time this will fail is when someone is doing an
>"rmmod --wait" on the module, which presumably means they really do
>not want you to increase the reference count furthur.
Ohh, I see. My assumption here was that we know for sure 
that module is alive at this point since we already hold a reference to the 
first socket. Actually I was going to send another email and ask for unconditional 
module_get() specifically for the cases like that. 

Even after your explanation I still think we need unconditional module_get() there.
Because in this case 'rmmod --wait' will simply brake accept() logic. I mean it'll 
keep waiting until listening socket is destroyed (i.e. until socket app is killed) 
but accept() will mysteriously fail for no good reason.
Comments ?

Max