Skip to contentSkip to conversation

New module refcounting for net_proto_family

PATCH/RFC Max Krasnyansky 3 participants 6 messages Dec 18, 2002 – Dec 21, 2002

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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
diff -Nru a/include/linux/net.h b/include/linux/net.h
@@ -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
diff -Nru a/net/socket.c b/net/socket.c
@@ -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
#1 lore raw

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 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. # -------------------------------------------- #
diff -Nru a/include/linux/net.h b/include/linux/net.h

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 

diff -Nru a/net/socket.c b/net/socket.c

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)

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
#2 lore raw
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

#3 lore raw

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

#4 lore raw
Quoted 10 lines · David S. Miller

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

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

#5 lore raw
In reply to Max Krasnyansky

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.