archive mirror
 help / color / mirror / Atom feed
From: Max Krasnyansky <>
To: Arnaldo Carvalho de Melo <>
Subject: Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family
Date: Wed, 23 Apr 2003 15:51:11 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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 following 
>> 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
Please do.

>> 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 something incomplete
and broken.

>> 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 the netdev
>mailing list. Considerations ? I'll be sending today patches converting the net protocols
>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 still don't 
handle accept() properly

        if (!(newsock = sock_alloc())) 
                goto out_put;

        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 *slab)
> {
>-       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 mentioned.
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 socket.
(not that IPv4 code is modular but just an example). Another example would be Bluetooth.
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 only reason 
we would want to hold reference to the module is if the module has replaced default 
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. net_proto_family can 
be unregistered while struct socket still exist, etc). 
net_family_get/put() makes no sense net_proto_family has only one function npf->create() 
which is referenced only from net/socket.c. struct socket should inherit owner field from
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 ?


  reply	other threads:[~2003-04-23 22:39 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2003-04-23 19:13 [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family Max Krasnyansky
2003-04-23 19:26 ` Arnaldo Carvalho de Melo
2003-04-23 22:51   ` Max Krasnyansky [this message]
2003-04-23 23:30     ` David S. Miller
2003-04-24  1:41       ` Max Krasnyansky
2003-04-24  3:29         ` David S. Miller
2003-04-24 16:43           ` Max Krasnyansky
2003-04-24  6:44     ` Arnaldo Carvalho de Melo
2003-04-24 19:33       ` Max Krasnyansky
2003-04-24 23:02         ` Arnaldo Carvalho de Melo
2003-04-25  0:40           ` Max Krasnyansky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).