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

Max Krasnyansky
2003-02-19T17:40:08Z

At 07:54 PM 2/18/2003, Rusty Russell wrote:
>In message <5.1.0.14.2.20030218101309.048d4288@mail1.qualcomm.com> 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