On Tue, Jul 26, 2005 at 04:37:19PM -0700, David S. Miller wrote: > From: Harald Welte <[EMAIL PROTECTED]> > Date: Sat, 23 Jul 2005 16:15:52 -0400 > > > The attached patch adds support for refcounting of modules implementing > > netlink protocols. The idea is that you prevent the module from > > disappearing as long as someone in userspace has still a socket talking > > to you. > > Ok, the changes look mostly fine. I've made a few slight > modifications before integrating, some of which I've mentioned > already: > > 1) I keep nl_table[] dynamically allocated > 2) I fixed up some white spacing, very minor stuff > 3) I fixed a socket leak in netlink_kernel_create(). If > netlink_lookup() returns non-NULL, you have a reference > to that socket thus have to release it.
thanks for taking care of this, and especially modifying the patch. I
would have done that based on your comments, but well, if you want to do
it, it's more convenient for me ;)
> I'm only including the af_netlink.c part of that patch I integrated
> since that's the only part I modified compared to your original
> patch.
ok, I read through it and it seems fine to me.
> I think there is a slight hole in this code though, which we can
> fixup as a followon patch. We probably need to grab the netlink
> table from the point at which we set p_ops all the way to where
> we full commit and netlink_insert() the kernel netlink socket.
mh, I have to think about that in more detail, will get back to you.
There's also a potential module refcounting leak when we have the
following order of events:
1) netlink_kernel_create()
2) userspace opens socket, increases refcount of kernel socket
3) sock_release(kernel_sock), resets p_ops to generic ones
4) userspace closes socket, but can no longer drop refcount on module
implementing the kernel socket.
I had a somewhat lengthy discussion with Thomas and Patrick about this,
and we didn't think it's worth fixing this up, esp. since all the
current users seem to sock_release only in the module unload path, which
can in turn only be invoked if the refcount is already zero. The only
real solution for this is to split netlink_kernel_create() in two parts,
let's say netlink_proto_register and netlink_kernel_sock_create(), and
the same for sock_release() / netlink_proto_unregister(). Let me know
whether you think it's worth the effort.
Thanks,
Harald
--
- Harald Welte <[EMAIL PROTECTED]> http://netfilter.org/
============================================================================
"Fragmentation is like classful addressing -- an interesting early
architectural error that shows how much experimentation was going
on while IP was being designed." -- Paul Vixie
pgpaKkcTCs0Uh.pgp
Description: PGP signature
