[BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Max Krasnyansky]

Hi Folks,

Can somebody (DaveM perhaps) please explain to me what the hell the following 
changset is doing in 2.5.68 
        http://linux.bkbits.net:8080/linux-2.5/cset@1.1118.1.1?nav=index.html|ChangeSet@-7d
??

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
        http://marc.theaimsgroup.com/?l=linux-kernel&m=104308300808557&w=2

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.

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).

Thanks

Max

http://bluez.sf.net
http://vtun.sf.net

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Arnaldo Carvalho de Melo]

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 
>         http://linux.bkbits.net:8080/linux-2.5/cset@1.1118.1.1?nav=index.html|ChangeSet@-7d
> ??
> 
> 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
>         http://marc.theaimsgroup.com/?l=linux-kernel&m=104308300808557&w=2

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
 
> 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.

- Arnaldo

You can import this changeset into BK by piping this whole message to:
'| bk receive [path to repository]' or apply the patch as usual.

===================================================================


ChangeSet@1.1180, 2003-04-23 02:53:59-03:00, acme@conectiva.com.br
  o net: module refcounting for sk_alloc/sk_free
  
  I had to move the rtnetlink_init and init_netlink calls to af_netlink init time, so that
  the sk_alloc called down the rtnetlink_init callchain is done after the PF_NETLINK
  net_proto_family is sock_registered, and because of that the af_netlink init function
  call had to be moved to earlier by means of subsys_initcall (DaveM's suggestion).


 include/linux/net.h      |    3 +++
 net/core/sock.c          |   13 +++++++++----
 net/netlink/af_netlink.c |    8 +++++++-
 net/socket.c             |   42 +++++++++++++++++++++++++++---------------
 4 files changed, 46 insertions(+), 20 deletions(-)


diff -Nru a/include/linux/net.h b/include/linux/net.h
--- a/include/linux/net.h	Wed Apr 23 02:57:58 2003
+++ b/include/linux/net.h	Wed Apr 23 02:57:58 2003
@@ -140,6 +140,9 @@
 	struct module	*owner;
 };
 
+extern int	     net_family_get(int family);
+extern void	     net_family_put(int family);
+
 struct iovec;
 
 extern int	     sock_wake_async(struct socket *sk, int how, int band);
diff -Nru a/net/core/sock.c b/net/core/sock.c
--- a/net/core/sock.c	Wed Apr 23 02:57:58 2003
+++ b/net/core/sock.c	Wed Apr 23 02:57:58 2003
@@ -589,8 +589,10 @@
  */
 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;
 	if (!slab)
 		slab = sk_cachep;
 	sk = kmem_cache_alloc(slab, priority);
@@ -602,14 +604,16 @@
 			sock_lock_init(sk);
 		}
 		sk->slab = slab;
-	}
-
+	} else
+		net_family_put(family);
+out:
 	return sk;
 }
 
 void sk_free(struct sock *sk)
 {
 	struct sk_filter *filter;
+	const int family = sk->family;
 
 	if (sk->destruct)
 		sk->destruct(sk);
@@ -624,6 +628,7 @@
 		printk(KERN_DEBUG "sk_free: optmem leakage (%d bytes) detected.\n", atomic_read(&sk->omem_alloc));
 
 	kmem_cache_free(sk->slab, sk);
+	net_family_put(family);
 }
 
 void __init sk_init(void)
diff -Nru a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
--- a/net/netlink/af_netlink.c	Wed Apr 23 02:57:58 2003
+++ b/net/netlink/af_netlink.c	Wed Apr 23 02:57:58 2003
@@ -1052,6 +1052,7 @@
 struct net_proto_family netlink_family_ops = {
 	.family = PF_NETLINK,
 	.create = netlink_create,
+	.owner	= THIS_MODULE,	/* for consistency 8) */
 };
 
 static int __init netlink_proto_init(void)
@@ -1066,6 +1067,11 @@
 #ifdef CONFIG_PROC_FS
 	create_proc_read_entry("net/netlink", 0, 0, netlink_read_proc, NULL);
 #endif
+	/* The netlink device handler may be needed early. */ 
+	rtnetlink_init();
+#ifdef CONFIG_NETLINK_DEV
+	init_netlink();
+#endif
 	return 0;
 }
 
@@ -1075,7 +1081,7 @@
        remove_proc_entry("net/netlink", NULL);
 }
 
-module_init(netlink_proto_init);
+subsys_initcall(netlink_proto_init);
 module_exit(netlink_proto_exit);
 
 MODULE_LICENSE("GPL");
diff -Nru a/net/socket.c b/net/socket.c
--- a/net/socket.c	Wed Apr 23 02:57:58 2003
+++ b/net/socket.c	Wed Apr 23 02:57:58 2003
@@ -69,8 +69,6 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/wanrouter.h>
-#include <linux/netlink.h>
-#include <linux/rtnetlink.h>
 #include <linux/if_bridge.h>
 #include <linux/init.h>
 #include <linux/poll.h>
@@ -143,6 +141,31 @@
 
 static struct net_proto_family *net_families[NPROTO];
 
+static __inline__ void net_family_bug(int family)
+{
+	printk(KERN_ERR "%d is not yet sock_registered!\n", family);
+	BUG();
+}
+
+int net_family_get(int family)
+{
+	int rc = 1;
+
+	if (likely(net_families[family] != NULL))
+		rc = try_module_get(net_families[family]->owner);
+	else
+		net_family_bug(family);
+	return rc;
+}
+
+void net_family_put(int family)
+{
+	if (likely(net_families[family] != NULL))
+		module_put(net_families[family]->owner);
+	else
+		net_family_bug(family);
+}
+
 #if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT)
 static atomic_t net_family_lockct = ATOMIC_INIT(0);
 static spinlock_t net_family_lock = SPIN_LOCK_UNLOCKED;
@@ -511,7 +534,7 @@
 
 		sock->ops->release(sock);
 		sock->ops = NULL;
-		module_put(net_families[family]->owner);
+		net_family_put(family);
 	}
 
 	if (sock->fasync_list)
@@ -1064,7 +1087,7 @@
 	sock->type  = type;
 
 	i = -EBUSY;
-	if (!try_module_get(net_families[family]->owner))
+	if (!net_family_get(family))
 		goto out_release;
 
 	if ((i = net_families[family]->create(sock, protocol)) < 0) 
@@ -1953,17 +1976,6 @@
 	 *  do_initcalls is run.  
 	 */
 
-
-	/*
-	 * The netlink device handler may be needed early.
-	 */
-
-#ifdef CONFIG_NET
-	rtnetlink_init();
-#endif
-#ifdef CONFIG_NETLINK_DEV
-	init_netlink();
-#endif
 #ifdef CONFIG_NETFILTER
 	netfilter_init();
 #endif

===================================================================


This BitKeeper patch contains the following changesets:
1.1180
## Wrapped with gzip_uu ##


M'XL( &8KICX  ^U8;7/:1A#^K/L5FV0ZQ:F!.[V+C#-N@I,P=AR/$_=+T]&(
MTPDT",DCG4AHR7_OGF0P5L OM-]L8$;2:7?UW-X^C_9X 1>%R'M:P*>"O( /
M62%[&L]2P64\"SH\FW:&.=XXSS*\T1UG4]%]<]Q-A6SK'8O@G;- \C',1%[T
M--8Q5B-R?BEZVOG1^XN3W\\).3B M^,@'8G/0L+! 9%9/@N2L#@,Y#C)TH[,
M@[28"ED]<[$R7>B4ZOBUF&-0RUXPFYK.@K.0L<!D(J2ZZ=HF22;3Y+ 8BZDH
MQIUA_/?-  8U=9W9AHFNS+8=C_2!=1AS*5"C2\VN;@#5>Y;1L[PV-7J4@LK'
M83,/\)L);4K>P/\+_BWAD &FM ?3+"P3 ;F(>%:F,DY'$&4Y%!,_2)*,=_$D
MRH5 !_P-8!R$B 6]9@+D&/TD1DGB=.+':2PA2$-0)_[5*'",4BB/(%J-598R
MGHI]*#*,$D@,K8(M'UIYB1#"[%NZZ2GJ-A\'<0IQ@4:IP.A2Y)7IV3O_].C+
MR>#T&(.BEW^99S+SHV :)W-E7V1\XN=B%!?H(L+]"O-0\* L!&11A:>*U$0<
ME2DN39:2&M\R%4-19:,Z%T&>Q AD.(>IP 52\8IR6,R+"GCEUNH',_'Q5P12
MCD:B4!'W.N08F.W8#CF[KEG2?N"'$!I0\OJ.6HE3GI2AZ.+$RN^*5IWQ>M5X
M)EWH'GX7MNE15<+,#6A@"[&Y0K?%4PS $C1=RUW8AFW<#0Q=NVIQ, *_@<AR
M$(OC6 M.(T>W@\BRG&BH>WP+HF:@=2C,9 :[%Y2KM>]>ET$3EK=@&,Q:6)QQ
M(S+,D(O 9=R^!=:VH.L0==<US'M!Y%DNJIDVD57S])#X^M!E@2N&'G<,9MP"
MK!'J1LKPPJSD=,-2WRVL.]<;>6B],4\W=,:<A66YAETKKGE#;TVW9[FWZZWQ
MI+>/0V]K4?H$[?Q;]4/]/-M4X3O(\("9.AA$?,<98\Y2J8'ZJ.S4>?%'0K9P
M'.K+O5=+VUD6AS\97Y8-XZ\5&QNTO9N)NT@&"3%[T\.TE$4GQ:4).AAEFV(P
M:ND>M1>4,48K_BG:/8Q_'K3-)_X] O[5KY4&_QJ%M0/W^I:G@TX&EF> 2;1"
MYB6751+@93&! SB].#E1%-+B"%K/&IR\HM@>T;01IA&R4KXB?9M:*J1-;62U
M]@-$4@BT:%!T14]TZJ$UTX$1M;,I)%S3%Q$4D_;K^N(5FNFV,ML6:TGT38W#
M_1B_>Q^SY05\1Q]##8:['M-"/;&86:N _E 5<*#-GE3@$:A W>QN4(%-%;;3
MJQB[6$6P#F95Y-H!?/DP^.Q__-2_.#G:U[HOJW57)%692?D<W#UXV55^M@L6
M419?,"W+G(1B%G.!64C#!*<\#>8J%:D0(>9")6+>07<@VLW%:R&57\11*")X
M^^GTW>#]<KG\_M$?*$5KQ5.9BC2,(])GU'$1_.#JV$AJ:_F$>K'5\)IB+'= 
M]U.)AVV\;E&&QL9KU9/;U+#KGMQZL!KH2@ZL)SUX!'I0[\\WZ,&RKG9I"9RJ
M(\#W$>@6*60@8PX^8L%9"=^OFN[U?GM8CM;[;?(/T2YS')BTCH_.3_VC\W-X
M_DNHLIAF$N9"-K/Y[&OZ?/^Z7=?>7+Q7K/Z!78>*NWT?H!ZE+G-D+;!5EY+$
M$Y',6RN_6!1_UAY_P;.ZHZE:ELI-YG._+N<J^B:G]NM*#!6TGUL9-?MKZ+F0
M)>Y+<E[#;Z:JL36I\#\ \!5.%>6_X538^MAN*+&L#]O;,U15VZE5M3K>W@CV
?F6=AA\:N_WCF8\$G13D]$'KHF=2FY%]2!X$WTQ8     
 

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Max Krasnyansky]

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 
>>         http://linux.bkbits.net:8080/linux-2.5/cset@1.1118.1.1?nav=index.html|ChangeSet@-7d
>> ??
>> 
>> 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
>>         http://marc.theaimsgroup.com/?l=linux-kernel&m=104308300808557&w=2
>
>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

net/socket.c:sys_accept()
        ...
        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 ?

Thanks
Max

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[David S. Miller]

   From: Max Krasnyansky <maxk@qualcomm.com>
   Date: Wed, 23 Apr 2003 15:51:11 -0700

   >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.

No, it was not broken, because he told me completely where he
was going with his changes.  He was building infrastructure piece by
piece, and that's always an acceptable way to do things as long
as it is explained where one is going with the changes.

Your stuff was unacceptable from the start because you didn't put
the ->owner into the protocol ops.
   
   I thought changes like that are always discussed on the lkml.

No, networking patches can go solely to netdev or linux-net.
   

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Max Krasnyansky]

At 04:30 PM 4/23/2003, David S. Miller wrote:
>   From: Max Krasnyansky <maxk@qualcomm.com>
>   Date: Wed, 23 Apr 2003 15:51:11 -0700
>
>   >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.
>
>No, it was not broken, because he told me completely where he
>was going with his changes. 
Of course it was and still is. New socket is allocated without incrementing 
modules ref count in sys_accept().

>He was building infrastructure piece by piece, and that's always an acceptable 
>way to do things as long as it is explained where one is going with the changes.
Oh, I see. And I just sent a patch without any explanation. Ok. 
(you might want to reread our original discussion again).

>Your stuff was unacceptable from the start because you didn't put
>the ->owner into the protocol ops.
But you didn't tell me that. You just said that it's "an ugly hack" without
giving any other feedback.
 
->owner field in protocol ops did come up during discussion (I think Rusty brought 
that up) and I explained why it shouldn't be there. But again there was no feed back 
from you. You just ignored that thread at some point. 

btw I still don't see ->owner in protocol ops. I read archives of netdev. You guys 
didn't even talk about that.

Anyway it's not important who said what now. You chose to ignore stuff that I did, fine.
What about this though

>>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.

Max

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[David S. Miller]

   From: Max Krasnyansky <maxk@qualcomm.com>
   Date: Wed, 23 Apr 2003 18:41:56 -0700

   At 04:30 PM 4/23/2003, David S. Miller wrote:
   >Your stuff was unacceptable from the start because you didn't put
   >the ->owner into the protocol ops.
   But you didn't tell me that. You just said that it's "an ugly hack" without
   giving any other feedback.
    
As you mention, Rusty said this.

   What about this though
   
I'm sure Arnaldo will deal with the sys_accept() issues.
But this is a minor issue, Arnaldo's stuff is architectually
solid.

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Arnaldo Carvalho de Melo]

Em Wed, Apr 23, 2003 at 03:51:11PM -0700, Max Krasnyansky escreveu:
> 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 
> >>         http://linux.bkbits.net:8080/linux-2.5/cset@1.1118.1.1?nav=index.html|ChangeSet@-7d
> >> ??
> >> 
> >> 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
> >>         http://marc.theaimsgroup.com/?l=linux-kernel&m=104308300808557&w=2
> >
> >I was not aware of this thread, will read it
> Please do.

I did, there are several valid points, I forgot the sys_accept case, I'm
studying the code to see how to properly fix it, but one thing I want covered
is keeping the net_families registered while there is still a struct sock
associated with the proto family, while not allowing the criation of new sockets
till the last struct sock is sk_freed.

Think about protecting the module with the top level network protocol family
->create code on sock_create level and the modules with specific protocols at
<net_proto_family>_sock_create level.
 
> >> 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
> 
> net/socket.c:sys_accept()
>         ...
>         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).

I'm think that this can be handled at this protocol family code level, i.e.  if
the net_proto_family wants to further modularise based on specific protocol
type this has to be refcounted at this level, example, at bt_sock_create.
 
> 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. 

That is a way to solve it, yes, but I don't think the only one,
net_proto_family has to do with the struct sock as those data structures are
related to the network protocol family.

> 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.

I'm studying how to signal that the net_proto_family was unregistered while
still having the entry in net_families available, that is a problem with the
current infrastructure in Linus tree.

Sorry, today I didn't had time to work on this at night at home, but tomorrow
I'll be working on this, thing is, as I'm working only on having the
infrastructure in place and no protocols have ->owner set there is no way to
trigger the problems discussed in this message in any way.

Ah, and thanks for raising these issues and pointing me to the previous
discussion that I had not read before, just too much e-mail on lkml, netdev
is more manageable :-)

- Arnaldo

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Max Krasnyansky]

At 08:29 PM 4/23/2003, David S. Miller wrote:
>   From: Max Krasnyansky <maxk@qualcomm.com>
>   Date: Wed, 23 Apr 2003 18:41:56 -0700
>
>   At 04:30 PM 4/23/2003, David S. Miller wrote:
>   >Your stuff was unacceptable from the start because you didn't put
>   >the ->owner into the protocol ops.
>   But you didn't tell me that. You just said that it's "an ugly hack" without
>   giving any other feedback.
>    
>As you mention, Rusty said this.
>
>   What about this though
>   
>I'm sure Arnaldo will deal with the sys_accept() issues.
>But this is a minor issue, Arnaldo's stuff is architectually
>solid.

:) 

Ok Dave. I'm not sure why you're _completely_ ignoring my arguments.
You should have just said from the beginning that you were going to
ignore what I have to say regarding that issue. At least I wouldn't 
have wasted my time.

Sorry for bugging you.
Max

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Max Krasnyansky]

Hi Arnaldo,

>I did, there are several valid points, I forgot the sys_accept case, I'm
>studying the code to see how to properly fix it,
It's pretty easy if we have sock->owner. Take a look how my patch does it 
(try_module_get should be replaced with __module_get()).
It also covers the case when net_family_owner transferred ownership to the
other module.

>but one thing I want covered
>is keeping the net_families registered while there is still a struct sock
>associated with the proto family, while not allowing the criation of new sockets
>till the last struct sock is sk_freed.
Why ?
Like I said all we care about is if non default callbacks in struct sock are valid.
Please read below.

>Think about protecting the module with the top level network protocol family
>->create code on sock_create level and the modules with specific protocols at
><net_proto_family>_sock_create level.
With the current infrastructure you'd need some kind of callback from sk_free() for
that to work.
I think it's simpler with sk->owner.

>> > 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).
>
>I'm think that this can be handled at this protocol family code level, i.e.  if
>the net_proto_family wants to further modularise based on specific protocol
>type this has to be refcounted at this level, example, at bt_sock_create.
Sure. Transfer of the ownership has to be done on that level. What I'm saying is
that everything else can be done in generic code. 

>> 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. 
>
>That is a way to solve it, yes, but I don't think the only one,
>net_proto_family has to do with the struct sock as those data structures are
>related to the network protocol family.
By the same logic all things are related in the kernel :).
struct socket uses net_proto_family ops and stuff and therefor is related.
But struct sock is just a callbacks.

>> 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.
>
>I'm studying how to signal that the net_proto_family was unregistered while
>still having the entry in net_families available, that is a problem with the
>current infrastructure in Linus tree.
Why do you need that ? 
Let's just get rid of the net_family_get/put thing. Each socket will have owner
field so we know who owns it and there is no need to access global net_family 
table.

>Sorry, today I didn't had time to work on this at night at home, but tomorrow
>I'll be working on this, thing is, as I'm working only on having the
>infrastructure in place and no protocols have ->owner set there is no way to
>trigger the problems discussed in this message in any way.
I actually had it all working with the patch that I mentioned. I can go ahead 
and add support to Bluetooth stuff, that's the reason I started that effort
in first place. However current code in main BK tree doesn't cover scenario  
when owner of net_proto_family != owner of the socket.

Here is how I think it should work.

sock_create()
        if (!try_module_get(npf->owner))
                goto fail;
        sock->owner = npf->owner;

        if (family_sock_create() < 0) {
                sock_release(sock);
                goto fail;
        }

 From this point on we don't care if net_proto_family is still registered or not, we know
who owns the socket.

sock_accept()
        __module_get(sock->owner);
        newsock->owner = sock->owner;

sock_release()
        module_put(sock->owner);

family_sock_create() (i.e. bt_sock_create(), etc)
        
#ifdef SPECIAL_FAMILY_THAT_HANDLES_PROTOCOLS_IN_SEPARATE_MODULES
        if (!try_module_get(proto_module))
                goto fail;

        prev_owner = sock->owner;
        sock->owner = proto_module;
        module_put(prev_owner); 
#else
        /* Other families do not have to do anything */
#endif

That's it for 'struct socket'.  And I mean that is _it_. Family can be unregistered 
and stuff, we don't care, and why should we.

Now 'struct sock'. It can exist independently from 'struct socket' and therefor
is a separate issue.
We only care about callbacks (sk->data_ready(), etc) and private sk slab caches. Some 
protocols simply use default callback which belong to non modular part of the kernel. 
Those don't have to do anything. They will just work.
Other protocols replace callbacks and therefor have to make sure that module is still 
loaded while sk exists. For those modules we need the following

void sk_set_owner(struct module *owner)
{
        /* Module must already hold reference when it call this function. */
        __module_get(owner);
        sk->owner = owner;
}
        
sk_free()
        ...
        module_put(sk->owner);
        ...

proto_set_sk_callbacks()

        sk_set_owner(THIS_MODULE);
        sk->destroy = proto_destroy;
        ...

That's it. There is no need to modify sk_alloc() or anything else. So it's not going
to introduce any overhead during socket creation from protocols that aren't modules or 
use default callback. And it gives us a flexibility of being able to pass ownership of 
the socket to another module or release module without having to access global family 
array.
For example if protocol wants to be unloaded by socket is still being used by net code
it could do something like
        write_lock(&sk->callback_lock);
        sk->data_ready = default_data_ready;
        ... other non default callbacks ...
        module_put(sk->owner);
        sk_set_owner(NULL);
        write_unlock(&sk->callback_lock);

Here is my original explanation.

>>>Here is the list of sk callbacks
>>>        void                    (*state_change)(struct sock *sk);
>>>        void                    (*data_ready)(struct sock *sk, int bytes);
>>>        void                    (*write_space)(struct sock *sk);
>>>        void                    (*error_report)(struct sock *sk);
>>>        int                     (*backlog_rcv) (struct sock *sk, struct sk_buff *skb);  
>>>        void                    (*destruct)(struct sock *sk);
>>>Net core doesn't make any other calls into the protocol module.
>>>
>>>In any case if protocol module wants, for some reason, to be around until sk is destroyed
>>>it can call sk_set_owner() right after sk_alloc(), even if it uses default callbacks.
>>>
>>>While looking at the net families that we support I realized that there is one more reason
>>>why protocol may have to call sk_set_owner(). Is if protocol uses private slab cache for its 
>>>sks. Otherwise cache will be destroyed during module unloading but sk may still be alive.
>>>
>>>Here is a little summary about families that I looked at:
>>>        Netlink - Global sk cache, replaces sk->destruct() callback.
>>>                  Must call sk_set_owner()
>>>
>>>        Unix -  Private sk cache, replaces sk->destruct() and sk->write_space() callbacks
>>>                Must call sk_set_owner()
>>>
>>>        IPv4/6 - Private cache, non-default callbacks.
>>>                Must call sk_set_owner() (cannot be a module, so we don't really care)
>>>
>>>        IPX - Global cache, default callbacks.
>>>
>>>        ATM - Global cache, default callbacks.
>>>                
>>>        IRDA - Global cache, default callbacks.
>>>
>>>        Bluetooth - Private cache (don't really need it, will change to global), non-default callbacks
>>>                Must call sk_set_owner()
>>>
>>>        AX25 - Global cache, non-default sk->destroy()
>>>                Must call sk_set_owner()
>>>
>>>        Rose - Global cache, default callbacks. 
>>>        
>>>        DecNet - Private cache, non-default callbacks.
>>>                Must call sk_set_owner()
>>>        
>>>        LLC - Global cache, default callbacks.

And here is the patch that implements it
        http://marc.theaimsgroup.com/?l=linux-kernel&m=104308300808557&w=2
(try_module_get() should be replaced with __module_get() in some places).

>Ah, and thanks for raising these issues and pointing me to the previous
>discussion that I had not read before,
>just too much e-mail on lkml, netdev is more manageable :-)
You're very welcome. I'm very interested to get this thing fixed.

(I wish Dave and Alexey were involved in this discussion and expressed their
thoughts/ideas/concerns instead of ignoring it.)

Max

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Arnaldo Carvalho de Melo]

Em Thu, Apr 24, 2003 at 12:33:35PM -0700, Max Krasnyansky escreveu:
> Hi Arnaldo,
> 
> >I did, there are several valid points, I forgot the sys_accept case, I'm
> >studying the code to see how to properly fix it,

> It's pretty easy if we have sock->owner. Take a look how my patch does it 
> (try_module_get should be replaced with __module_get()).
> It also covers the case when net_family_owner transferred ownership to the
> other module.

Yes, this gives us more flexibility, but as well some more overhead in the
common struct sock, I have worked in 2.5 to reduce its size, and want to
reduce it further, if possible. But read on, other reasons below.
 
> >but one thing I want covered
> >is keeping the net_families registered while there is still a struct sock
> >associated with the proto family, while not allowing the criation of new sockets
> >till the last struct sock is sk_freed.
> Why ?
> Like I said all we care about is if non default callbacks in struct sock are valid.
> Please read below.
> 
> >Think about protecting the module with the top level network protocol family
> >->create code on sock_create level and the modules with specific protocols at
> ><net_proto_family>_sock_create level.

> With the current infrastructure you'd need some kind of callback from
> sk_free() for that to work.  I think it's simpler with sk->owner.

not a callback, it'd be in sk_free itself (talking from the top of my head,
just arrived home and will be further studying this now), I pretty much want to
have protocol family maintainers not worrying about module refcounting at all
in the default case of no further modularisation per specific prococol, that
would save, for instance, Steven Whitehouse some work on DecNET :)
 
> >> > 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).

> >I'm think that this can be handled at this protocol family code level, i.e.
> >if the net_proto_family wants to further modularise based on specific
> >protocol type this has to be refcounted at this level, example, at
> >bt_sock_create.

> Sure. Transfer of the ownership has to be done on that level. What I'm saying
> is that everything else can be done in generic code. 

So we're in agreement on having most of this infrastructure in generic code :)
 
> >> 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. 

> >That is a way to solve it, yes, but I don't think the only one,
> >net_proto_family has to do with the struct sock as those data structures are
> >related to the network protocol family.

> By the same logic all things are related in the kernel :).

:-)

> struct socket uses net_proto_family ops and stuff and therefor is related.
> But struct sock is just a callbacks.

But I want to avoid having another thing to worry about for net protocol
family writers, i.e. not having to add a sk_set_owner if one decides it has
to change one of the default callbacks.
 
> >> 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.

> >I'm studying how to signal that the net_proto_family was unregistered while
> >still having the entry in net_families available, that is a problem with the
> >current infrastructure in Linus tree.

> Why do you need that ? 

> Let's just get rid of the net_family_get/put thing. Each socket will have
> owner field so we know who owns it and there is no need to access global
> net_family table.

I'm trying to avoid adding a pointer to struct sock, if it is not possible
at all to avoid, ok, we do it, but I think we can have this at just one
place, the net_family table.

> >Sorry, today I didn't had time to work on this at night at home, but
> >tomorrow I'll be working on this, thing is, as I'm working only on having
> >the infrastructure in place and no protocols have ->owner set there is no
> >way to trigger the problems discussed in this message in any way.

> I actually had it all working with the patch that I mentioned. I can go ahead 
> and add support to Bluetooth stuff, that's the reason I started that effort
> in first place. However current code in main BK tree doesn't cover scenario  
> when owner of net_proto_family != owner of the socket.

> Here is how I think it should work.
> 
> sock_create()
>         if (!try_module_get(npf->owner))
>                 goto fail;
>         sock->owner = npf->owner;
> 
>         if (family_sock_create() < 0) {
>                 sock_release(sock);
>                 goto fail;
>         }
 
>  From this point on we don't care if net_proto_family is still registered or
>  not, we know who owns the socket.
 
> sock_accept()
>         __module_get(sock->owner);
>         newsock->owner = sock->owner;
> 
> sock_release()
>         module_put(sock->owner);
> 
> family_sock_create() (i.e. bt_sock_create(), etc)
>         
> #ifdef SPECIAL_FAMILY_THAT_HANDLES_PROTOCOLS_IN_SEPARATE_MODULES
>         if (!try_module_get(proto_module))
>                 goto fail;
> 
>         prev_owner = sock->owner;
>         sock->owner = proto_module;
>         module_put(prev_owner); 
> #else
>         /* Other families do not have to do anything */
> #endif
 
> That's it for 'struct socket'.  And I mean that is _it_. Family can be
> unregistered and stuff, we don't care, and why should we.
 
> Now 'struct sock'. It can exist independently from 'struct socket' and
> therefor is a separate issue.

> We only care about callbacks (sk->data_ready(), etc) and private sk slab
> caches. Some protocols simply use default callback which belong to non
> modular part of the kernel.  Those don't have to do anything. They will just
> work.  Other protocols replace callbacks and therefor have to make sure that
> module is still loaded while sk exists. For those modules we need the
> following
 
> void sk_set_owner(struct module *owner)
> {
>         /* Module must already hold reference when it call this function. */
>         __module_get(owner);
>         sk->owner = owner;
> }

This is the additional rule I'm trying to avoid.
         
> sk_free()
>         ...
>         module_put(sk->owner);
>         ...

this is already in BK tree.
 
> >>>        IPv4/6 - Private cache, non-default callbacks.
> >>>                Must call sk_set_owner() (cannot be a module, so we don't really care)

Well, I care, I wish to have this as a module at some point, if allowed by
DaveM, of course 8)

- Arnaldo

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Max Krasnyansky]

At 04:02 PM 4/24/2003, Arnaldo Carvalho de Melo wrote:
>Em Thu, Apr 24, 2003 at 12:33:35PM -0700, Max Krasnyansky escreveu:
>> Hi Arnaldo,
>> 
>> >I did, there are several valid points, I forgot the sys_accept case, I'm
>> >studying the code to see how to properly fix it,
>
>> It's pretty easy if we have sock->owner. Take a look how my patch does it 
>> (try_module_get should be replaced with __module_get()).
>> It also covers the case when net_family_owner transferred ownership to the
>> other module.
>
>Yes, this gives us more flexibility, but as well some more overhead in the
>common struct sock, I have worked in 2.5 to reduce its size, and want to
>reduce it further, if possible. 
Easy.
struct sock {
        ...
        struct sock             *next;
        struct sock             **pprev;
        struct sock             *bind_next;
        struct sock             **bind_pprev;
        struct sock             *prev;

At least one of the prev can go.

>> >Think about protecting the module with the top level network protocol family
>> >->create code on sock_create level and the modules with specific protocols at
>> ><net_proto_family>_sock_create level.
>
>> With the current infrastructure you'd need some kind of callback from
>> sk_free() for that to work.  I think it's simpler with sk->owner.
>
>not a callback, it'd be in sk_free itself (talking from the top of my head,
>just arrived home and will be further studying this now), I pretty much want to
>have protocol family maintainers not worrying about module refcounting at all
>in the default case of no further modularisation per specific prococol, that
>would save, for instance, Steven Whitehouse some work on DecNET :)

sk_free() doesn't know which module owns the socket in 
"net_proto_family owner != socket owner" case. 

BTW Let's call that case "NOnSO" I'm referring to it way too often :)

>> Sure. Transfer of the ownership has to be done on that level. What I'm saying
>> is that everything else can be done in generic code. 
>
>So we're in agreement on having most of this infrastructure in generic code :)
I guess so :). But haven't seen a generic solution for the NOnSO from you yet ;-). 

>> struct socket uses net_proto_family ops and stuff and therefor is related.
>> But struct sock is just a callbacks.
>
>But I want to avoid having another thing to worry about for net protocol
>family writers, i.e. not having to add a sk_set_owner if one decides it has
>to change one of the default callbacks.
Well, that means that refcounting overhead will affect even the protocols that 
don't have to keep track of 'struct sock'. It also means that those protocols 
will not unloadable until all sk's are killed. Even though sk's have no references 
whatsoever to the module that created them. 

>> Let's just get rid of the net_family_get/put thing. Each socket will have
>> owner field so we know who owns it and there is no need to access global
>> net_family table.
>
>I'm trying to avoid adding a pointer to struct sock, if it is not possible
>at all to avoid, ok, we do it, but I think we can have this at just one
>place, the net_family table.
Killing one of the list pointers from 'struct sock' shouldn't be a problem (see above). 

One net_family table won't handle NOnSO. Even if it will, protocol writers will have to 
worry about proper initialization of the tables anyway. Especially if someone comes up 
with something like:
        Family X (module X.ko)
                Proto X1 (module X1.ko)
                        Subproto X1-A (module X1-A.ko)
not that it makes a lot of sense right now but why restrict it if we don't have to. 

>> void sk_set_owner(struct module *owner)
>> {
>>         /* Module must already hold reference when it call this function. */
>>         __module_get(owner);
>>         sk->owner = owner;
>> }
>
>This is the additional rule I'm trying to avoid.

It's not a big deal rule. All protocols do hold reference when they allocate sk.

>> sk_free()
>>         ...
>>         module_put(sk->owner);
>>         ...
>
>this is already in BK tree.
No. I mean NOnSO ;-). 
What's in BK is net_family_put(family) which is by no means the same.

>> >>>        IPv4/6 - Private cache, non-default callbacks.
>> >>>                Must call sk_set_owner() (cannot be a module, so we don't really care)
>
>Well, I care, I wish to have this as a module at some point, if allowed by
>DaveM, of course 8)
If I remember correctly DaveM was one of those are strongly against that.

Max