At 12:26 PM 4/23/2003, Arnaldo Carvalho de Melo wrote:
>Em Wed, Apr 23, 2003 at 12:13:37PM -0700, Max Krasnyansky escreveu:
>> Hi Folks,
>> Can somebody (DaveM perhaps) please explain to me what the hell the
>> changset is doing in 2.5.68
>> I've spent quite a bit of time looking into what's needed to fix socket
>> module refcounting
>> issues and convicting DaveM and Alex that we need to fix them.
>> Here is the original thread
>I was not aware of this thread, will read it
>> My patch was rejected without giving any technical explanation. I was
>> waiting for Rusty's
>> __module_get() patch to get in, to release a new patch which addresses some
>> issues that
>> came up during discussion.
>> Next thing you know, incomplete patch (changeset mentioned above) shows up
>> in the BK and
>> 2.5.68. Without even a simple note on the lkm. And completely ignoring
>> discussion that
>> we had about this very issue just a few weeks ago.
>This is just the first part, DaveM already merged the second part, that deals
>with struct sock
That's exactly what surprised me. He rejected complete patch and accepted
>> I don't mind of course if some other (better) patch is accepted instead of
>> mine. But patch
>> that went in is incomplete and buggy. It doesn't handle accept case properly
>> and doesn't
>> address the issue of the 'struct sock' ownership (read original thread for
>> more details).
>This is dealt with the following patch, and these patches were discussed on
>mailing list. Considerations ? I'll be sending today patches converting the
>that I maintain and then for some others that are ummaintained, etc.
I thought changes like that are always discussed on the lkml. They are directly
related to the
new module interface changes. It least CC lkml.
(I've subscribed to netdev list)
The latest patches that I've seen from you on netdev list and in main BK tree
handle accept() properly
if (!(newsock = sock_alloc()))
newsock->type = sock->type;
newsock->ops = sock->ops;
newsock is not accounted for (sock_alloc() doesn't bump module refcount).
> struct sock *sk_alloc(int family, int priority, int zero_it, kmem_cache_t
>- struct sock *sk;
>+ struct sock *sk = NULL;
>+ if (!net_family_get(family))
>+ goto out;
Ok. This is wrong. Which should be clear from reading the thread that I
Owner of the net_proto_family is not necessarily the owner of the 'struct sock'.
Example: af_inet module registers net_proto_family but udp module owns the
(not that IPv4 code is modular but just an example). Another example would be
We have Bluetooth core that registers Bluetooth proto_family and several modules
that handle specific protocols (l2cap, rfcomm, etc).
Also net_proto_family has pretty much nothing to do with the struct sock. The
we would want to hold reference to the module is if the module has replaced
callbacks (i.e. sk->data_ready(), etc).
So my point is we need sk->owner field.
I'd also prefer to see sock->owner which gives more flexibility (i.e.
be unregistered while struct socket still exist, etc).
net_family_get/put() makes no sense net_proto_family has only one function
which is referenced only from net/socket.c. struct socket should inherit owner
struct net_proto_family by default but protocol should be able to assign
ownership to a
different module if it needs to.
So I'd suggest reverting the patches that went in and applying my patch
instead. Not because
its mine but because IMO it's a better starting point. Dave, comments ?