New module refcounting for net_proto_family
Subsystems (2)
Hi Folks,
It seems that new module code is going to stay with us at least for a while :).
So it's probably time to start fixing old interfaces to use new refcounting scheme.
Here is a patch for sock_create() and stuff that uses net_proto_family interface. Tested with modified af_bluetooth.c and seems to work fine. Other families are unaffected for now because their owner field == NULL.
If people are ok with this aproach I will also fix af_unix and other families and push all fixes into my BK tree.
(URL in case if my mailer messed up spaces
http://bluez.sourceforge.net/patches/npf_refcnt_patch-2.5.52.gz)
# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.888 -> 1.889 # net/socket.c 1.39 -> 1.40 # include/linux/net.h 1.7 -> 1.8
#
# The following is the BitKeeper ChangeSet Log # --------------------------------------------
# 02/12/17 [email protected] 1.889
# Convert generic socket code to new module refcounting. # -------------------------------------------- #
include/linux/net.h +4 -0
@@ -76,6 +76,8 @@short type;unsigned char passcred; + +struct module *owner; };struct scm_cookie ;
@@ -124,6 +126,8 @@short authentication;short encryption;short encrypt_net; + +struct module *owner; };struct net_proto
net/socket.c +4 -0
@@ -470,6 +470,8 @@ sock =SOCKET_I (inode); + sock->owner =NULL ; + inode->i_mode =S_IFSOCK |S_IRWXUGO ; inode->i_sock =1 ; inode->i_uid = current->fsuid;
@@ -517,6 +519,8 @@return ; } sock->file=NULL ; + +module_put (sock->owner); } static int __sock_sendmsg(struct kiocb *iocb, struct socket *sock,
struct msghdr *msg, int size)
@@ -964,8 +968,9 @@
int sock_create(int family, int type, int protocol, struct socket
**res)
{
- int i;
+ struct net_proto_family *npf;
struct socket *sock;
+ int err;
/*
* Check protocol is in range
@@ -990,14 +995,8 @@
}
#if defined(CONFIG_KMOD) && defined(CONFIG_NET)
- /* Attempt to load a protocol module if the find failed.
- *
- * 12/09/1996 Marcin: But! this makes REALLY only sense, if the user
- * requested real, full-featured networking support upon
configuration.
- * Otherwise module support will break!
- */
- if (net_families[family]==NULL)
- {
+ /* Attempt to load a protocol module if the find failed. */
+ if (net_families[family]==NULL) {
char module_name[30]; sprintf(module_name,"net-pf-%d",family); request_module(module_name);
@@ -1005,29 +1004,31 @@
#endif
net_family_read_lock();
- if (net_families[family] == NULL) {
- i = -EAFNOSUPPORT;
+
+ npf = net_families[family];
+ if (!npf || !try_module_get(npf->owner)) {
+ err = -EAFNOSUPPORT;
goto out;
}
+
+ /*
+ * Allocate the socket and allow the family to set things up. if
+ * the protocol is 0, the family is instructed to select an
appropriate
+ * default.
+ */
-/*
- * Allocate the socket and allow the family to set things up. if
- * the protocol is 0, the family is instructed to select an appropriate
- * default.
- */
-
- if (!(sock = sock_alloc()))
- {
+ sock = sock_alloc();
+ if (!sock) {
printk(KERN_WARNING "socket: no more sockets\n");
- i = -ENFILE; /* Not exactly a match, but its the
+ err = -ENFILE; /* Not exactly a match, but its the
closest posix thing */
goto out;
}
sock->type = type;
+ sock->owner = npf->owner;
- if ((i = net_families[family]->create(sock, protocol)) < 0)
- {
+ if ((err = npf->create(sock, protocol)) < 0) {
sock_release(sock);
goto out;
}
@@ -1036,7 +1037,7 @@
out:
net_family_read_unlock();
- return i;
+ return err;
}
asmlinkage long sys_socket(int family, int type, int protocol)
@@ -1201,6 +1202,9 @@
newsock->type = sock->type;
newsock->ops = sock->ops;
+ 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;
Signature · 2 lines
Max Krasnyansky <[email protected]> Qualcomm
Hmm, no replies. Nobody is interested in this or what ? I want to get this fixed otherwise I can't fix Bluetooth module refcounting.
Max
Quoted 171 lines · Max Krasnyansky
Hi Folks,
It seems that new module code is going to stay with us at least for a while :).
So it's probably time to start fixing old interfaces to use new refcounting scheme.Here is a patch for
sock_create()and stuff that usesnet_proto_familyinterface. Tested with modifiedaf_bluetooth.cand seems to work fine. Other families are unaffected for now because their owner field == NULL.If people are ok with this aproach I will also fix
af_unixand other families and push all fixes into my BK tree.(URL in case if my mailer messed up spaces
http://bluez.sourceforge.net/patches/npf_refcnt_patch-2.5.52.gz)# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:# ChangeSet 1.888 -> 1.889 # net/socket.c 1.39 -> 1.40 # include/linux/net.h 1.7 -> 1.8#
# The following is the BitKeeper ChangeSet Log # --------------------------------------------
# 02/12/17 [email protected] 1.889
# Convert generic socket code to new module refcounting. # -------------------------------------------- #
diff -Nrua/include/linux/net.hb/include/linux/net.hinclude/linux/net.h +4
-0@@ -76,6 +76,8 @@short type;unsigned char passcred; + +struct module *owner; };struct scm_cookie ;@@ -124,6 +126,8 @@short authentication;short encryption;short encrypt_net; + +struct module *owner; };struct net_proto diff -Nru
a/net/socket.cb/net/socket.cnet/socket.c +4
-0@@ -470,6 +470,8 @@ sock =SOCKET_I (inode); + sock->owner =NULL ; + inode->i_mode =S_IFSOCK |S_IRWXUGO ; inode->i_sock =1 ; inode->i_uid = current->fsuid;@@ -517,6 +519,8 @@return ; } sock->file=NULL ; + +module_put (sock->owner); } static int __sock_sendmsg(struct kiocb *iocb, struct socket *sock,struct msghdr *msg, int size)
net/socket.c +2
-1@@ -964,8 +968,9 @@int sock_create (int family,int type,int protocol,struct socket **res) { -int i; +struct net_proto_family *npf;struct socket *sock; +int err;/* net/socket.c +0
-4* Check protocol is in range @@ -990,14 +995,8 @@ }#if defined(CONFIG_KMOD ) && defined(CONFIG_NET ) -/* Attempt to load a protocol module if the find failed. -* -* 12/09/1996 Marcin: But! this makes REALLY only sense, if the user -* requested real, full-featured networking support upon configuration.
net/socket.c +10
-6-* Otherwise module support will break! -*/ -if (net_families[family]==NULL ) - { +/* Attempt to load a protocol module if the find failed. */ +if (net_families[family]==NULL ) {char module_name[30 ];sprintf (module_name,"net-pf-%d" ,family);request_module (module_name);@@ -1005,29 +1004,31 @@#endif net_family_read_lock (); -if (net_families[family] ==NULL ) { - i = -EAFNOSUPPORT ; + + npf = net_families[family]; +if (!npf || !try_module_get (npf->owner)) { + err = -EAFNOSUPPORT ; goto out; } + +/* +* Allocate the socket and allow the family to set things up. if +* the protocol is 0, the family is instructed to select an appropriate
net/socket.c +11
-12+* default. +*/ -/* -* Allocate the socket and allow the family to set things up. if -* the protocol is 0, the family is instructed to select an appropriate -* default. -*/ - -if (!(sock =sock_alloc ())) - { + sock =sock_alloc (); +if (!sock) {printk (KERN_WARNING "socket: no more sockets\n" ); - i = -ENFILE ;/* Not exactly a match, but its the + err = -ENFILE ;/* Not exactly a match, but its the closest posix thing */ goto out; } sock->type = type; + sock->owner = npf->owner; -if ((i = net_families[family]->create (sock, protocol)) <0 ) - { +if ((err = npf->create (sock, protocol)) <0 ) {sock_release (sock); goto out; }@@ -1036,7 +1037,7 @@ out:net_family_read_unlock (); -return i; +return err; }asmlinkage long sys_socket (int family,int type,int protocol)@@ -1201,6 +1202,9 @@ newsock->type = sock->type; newsock->ops = sock->ops; +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;
Signature · 2 lines
Max Krasnyansky <[email protected]> Qualcomm
Quoted 3 lines · Max Krasnyansky
Hmm, no replies. Nobody is interested in this or what ? I want to get this fixed otherwise I can't fix Bluetooth module refcounting.
Looks good to me, but its christmas so I wouldnt expect much to happen till the new year
From: Alan Cox <[email protected]>
Date: 19 Dec 2002 19:28:35 +0000
Quoted 3 lines · Max Krasnyansky
Hmm, no replies. Nobody is interested in this or what ? I want to get this fixed otherwise I can't fix Bluetooth module refcounting.
Looks good to me, but its christmas so I wouldnt expect much to happen till the new year
I just got back from a long snowboarding trip, I'll look at this over the weekend before I disappear for another similar trip :)
Quoted 10 lines · David S. Miller
From: Alan Cox <[email protected]>
Date: 19 Dec 2002 19:28:35 +0000Quoted 3 lines · Max Krasnyansky
Hmm, no replies. Nobody is interested in this or what ? I want to get this fixed otherwise I can't fix Bluetooth module refcounting.
Looks good to me, but its christmas so I wouldnt expect much to happen till the new year
Ah, Christmas time :)
Quoted 2 lines
I just got back from a long snowboarding trip, I'll look at this over the weekend before I disappear for another similar trip :)
Ok. Drop me a note and I'll push this stuff to BK were you can pull from. In the mean time I'll go bug other folks :). I want to do same kinda changes for the TTY ldisc code.
Max
Bunch of problems with this patch:
1) Module leak. If try_module_get(npf->owner) works but sock_alloc() fails, we never put the module. It just branches to the "out" label in that case, which unlocks the net_family table and returns err.
2) Bigger issue, why not attach the owner to struct sock instead of socket? The sock can exist, and thus reference the protocol family code, long after the socket (ie. the user end) is killed off and closed.
For example, this could happen for just about any protocol family due to stray device sk_buff references to the sock and thus the protocol family.
Please address this stuff.