Skip to contentSkip to conversation

Teach cifs about network namespaces.

PATCH In review Rob Landley 4 participants 23 messages Jan 11, 2011 – Jan 13, 2011 7 messages not in the archive

Subsystems (2)
SMB client (CIFS) Kernel SMB3 server (ksmbd)
lore
Share & tools
lore

Rob Landley

author
#0 lore raw

PATCH 1/1 Teach cifs about network namespaces.

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses and routing visible from a container rather than from init context.

For a long drawn out test reproduction sequence, see:

http://landley.livejournal.com/47024.html
http://landley.livejournal.com/47205.html
http://landley.livejournal.com/47476.html

Signed-off-by Rob Landley <[email protected]>

---

 fs/cifs/cifsglob.h |   32 ++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   22 +++++++++++++++++-----
 2 files changed, 49 insertions(+), 5 deletions(-)
fs/cifs/cifsglob.h +32 -0
index 7136c0c..86f31bb 100644
@@ -168,6 +168,9 @@ struct TCP_Server_Info {
 		struct sockaddr_in6 sockAddr6;
 	} addr;
 	struct sockaddr_storage srcaddr; /* locally bind to this IP */
+#ifdef CONFIG_NET_NS
+	struct net *net;
+#endif
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
 	struct list_head pending_mid_q;

@@ -227,6 +230,35 @@ struct TCP_Server_Info {
 };
 
 /*
+ * Macros to allow the TCP_Server_Info->net field and related code to drop out
+ * when CONFIG_NET_NS isn't set.
+ */
+
+static inline struct net *
+cifs_net_ns(struct TCP_Server_Info *srv)
+{
+#ifdef CONFIG_NET_NS
+	return srv->net;
+#else
+	return &init_net;
+#endif
+}
+
+static inline void
+cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+#ifdef CONFIG_NET_NS
+	srv->net = net;
+#endif
+}
+
+#ifdef CONFIG_NET_NS
+#define cifs_use_net_ns() (1)
+#else
+#define cifs_use_net_ns() (0)
+#endif
+
+/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
fs/cifs/connect.c +17 -5
index cc1a860..b4faef0 100644
@@ -1545,6 +1545,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (cifs_use_net_ns()
+		    && cifs_net_ns(server) == current->nsproxy->net_ns)
+			continue;
+
 		if (!match_address(server, addr,
 				   (struct sockaddr *)&vol->srcaddr))
 			continue;

@@ -1572,6 +1576,9 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 		return;
 	}
 
+	if (cifs_use_net_ns())
+		put_net(cifs_net_ns(server));
+
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 

@@ -1677,6 +1684,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	       sizeof(tcp_ses->srcaddr));
 	++tcp_ses->srv_count;
 
+	if (cifs_use_net_ns())
+		cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
+
 	if (addr.ss_family == AF_INET6) {
 		cFYI(1, "attempting ipv6 connect");
 		/* BB should we allow ipv6 on port 139? */

@@ -1720,6 +1730,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
 
+	if (cifs_use_net_ns())
+		put_net(cifs_net_ns(tcp_ses));
+
 out_err:
 	if (tcp_ses) {
 		if (!IS_ERR(tcp_ses->hostname))

@@ -2145,8 +2158,8 @@ ipv4_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
 
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating socket", rc);
 			return rc;

@@ -2310,11 +2323,10 @@ ipv6_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
 
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET6, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET6,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating ipv6 socket", rc);
-			socket = NULL;
 			return rc;
 		}

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw
Quoted 23 lines · Rob Landley

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses and routing visible from a container rather than from init context.

For a long drawn out test reproduction sequence, see:

http://landley.livejournal.com/47024.html
http://landley.livejournal.com/47205.html
http://landley.livejournal.com/47476.html

Signed-off-by Rob Landley <[email protected]>

---

 fs/cifs/cifsglob.h |   32 ++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   22 +++++++++++++++++-----
 2 files changed, 49 insertions(+), 5 deletions(-)
fs/cifs/cifsglob.h +0 -0
index 7136c0c..86f31bb 100644

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 173

Quoted 6 lines
fs/cifs/cifsglob.h +3 -0
@@ -168,6 +168,9 @@ struct TCP_Server_Info {
 		struct sockaddr_in6 sockAddr6;
 	} addr;
 	struct sockaddr_storage srcaddr; /* locally bind to this IP */
+#ifdef CONFIG_NET_NS
+	struct net *net;
+#endif

I'm assuming this bit is correct -- don't know enough about CIFS to be sure...

Quoted 10 lines
fs/cifs/cifsglob.h +3 -0
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
 	struct list_head pending_mid_q;

@@ -227,6 +230,35 @@ struct TCP_Server_Info {
 };
 
 /*
+ * Macros to allow the TCP_Server_Info->net field and related code to drop out
+ * when CONFIG_NET_NS isn't set.
+ */

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 245

Quoted 10 lines
fs/cifs/cifsglob.h +10 -0
+
+static inline struct net *
+cifs_net_ns(struct TCP_Server_Info *srv)
+{
+#ifdef CONFIG_NET_NS
+	return srv->net;
+#else
+	return &init_net;
+#endif
+}

I thought style dictated we do this a different way:

#ifdef CONFIG_NET_NS
static inline struct net * cifs_net_ns(struct TCP_Server_Info *srv)

{
	return srv->net;
}
<other CONFIG_NET_NS cases>
#else
static inline struct net * cifs_net_ns(struct TCP_Server_Info *srv)
{
	return &init_net;
}

<other no-ops>
#endif /* CONFIG_NET_NS */

Quoted 4 lines
fs/cifs/cifsglob.h +4 -0
+
+static inline void
+cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 259

Quoted 10 lines
fs/cifs/cifsglob.h +10 -0
+#ifdef CONFIG_NET_NS
+	srv->net = net;
+#endif
+}
+
+#ifdef CONFIG_NET_NS
+#define cifs_use_net_ns() (1)
+#else
+#define cifs_use_net_ns() (0)
+#endif

This looks wrong -- we shouldn't need this at all. The #ifdef bits in your patch already make all the cases below become empty/no-ops when CONFIG_NET_NS=n.

Quoted 10 lines
fs/cifs/cifsglob.h +2 -0
+
+/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
fs/cifs/connect.c +0 -0
index cc1a860..b4faef0 100644

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1550

Quoted 6 lines
fs/cifs/connect.c +3 -0
@@ -1545,6 +1545,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (cifs_use_net_ns()
+		    && cifs_net_ns(server) == current->nsproxy->net_ns)
+			continue;

This looks wrong -- you want to invert part of this I think (and drop the unnecessary cifs_use_net_ns()):

	if (cifs_net_ns(server) != current->nsproxy->net_ns)
		continue;

This is obvious when you note that the context below shows that we 'continue' to the next entry when the addresses don't match:

fs/cifs/connect.c cifs_put_tcp_session(struct TCP_Server_Info *server) · line 1581

Quoted 11 lines
fs/cifs/connect.c +4 -0
+
 		if (!match_address(server, addr,
 				   (struct sockaddr *)&vol->srcaddr))
 			continue;

@@ -1572,6 +1576,9 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 		return;
 	}
 
+	if (cifs_use_net_ns())
+		put_net(cifs_net_ns(server));
+

I think this should just be:

	put_net(cifs_net_ns(server));

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1689

Quoted 10 lines
fs/cifs/connect.c +3 -0
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 

@@ -1677,6 +1684,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	       sizeof(tcp_ses->srcaddr));
 	++tcp_ses->srv_count;
 
+	if (cifs_use_net_ns())
+		cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
+

Just use cifs_set_net_ns() because it already turns into a no-op when CONFIG_NET_NS=n

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1734

Quoted 9 lines
fs/cifs/connect.c +2 -0
 	if (addr.ss_family == AF_INET6) {
 		cFYI(1, "attempting ipv6 connect");
 		/* BB should we allow ipv6 on port 139? */

@@ -1720,6 +1730,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
 
+	if (cifs_use_net_ns())
+		put_net(cifs_net_ns(tcp_ses));

etc.

Quoted 33 lines
fs/cifs/connect.c +5 -5
+
 out_err:
 	if (tcp_ses) {
 		if (!IS_ERR(tcp_ses->hostname))

@@ -2145,8 +2158,8 @@ ipv4_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
 
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating socket", rc);
 			return rc;

@@ -2310,11 +2323,10 @@ ipv6_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
 
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET6, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET6,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating ipv6 socket", rc);
-			socket = NULL;
 			return rc;
 		}

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

A message that is not in the archive (sent off-list or not yet received): 20110111071239.GL29064-52DBMbEzqgQ/wnmkkaCWp/[email protected]

author
#4 lore raw
Quoted 68 lines · Matt Helsley
Quoted 23 lines · Rob Landley

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses and routing visible from a container rather than from init context.

For a long drawn out test reproduction sequence, see:

http://landley.livejournal.com/47024.html
http://landley.livejournal.com/47205.html
http://landley.livejournal.com/47476.html

Signed-off-by Rob Landley <[email protected]>

---

 fs/cifs/cifsglob.h |   32 ++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   22 +++++++++++++++++-----
 2 files changed, 49 insertions(+), 5 deletions(-)
fs/cifs/cifsglob.h +0 -0
index 7136c0c..86f31bb 100644

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 173

Quoted 6 lines
fs/cifs/cifsglob.h +3 -0
@@ -168,6 +168,9 @@ struct TCP_Server_Info {
 		struct sockaddr_in6 sockAddr6;
 	} addr;
 	struct sockaddr_storage srcaddr; /* locally bind to this IP */
+#ifdef CONFIG_NET_NS
+	struct net *net;
+#endif

I'm assuming this bit is correct -- don't know enough about CIFS to be sure...

Quoted 10 lines
fs/cifs/cifsglob.h +3 -0
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
 	struct list_head pending_mid_q;

@@ -227,6 +230,35 @@ struct TCP_Server_Info {
 };
 
 /*
+ * Macros to allow the TCP_Server_Info->net field and related code to drop out
+ * when CONFIG_NET_NS isn't set.
+ */

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 245

Quoted 10 lines
fs/cifs/cifsglob.h +10 -0
+
+static inline struct net *
+cifs_net_ns(struct TCP_Server_Info *srv)
+{
+#ifdef CONFIG_NET_NS
+	return srv->net;
+#else
+	return &init_net;
+#endif
+}

I thought style dictated we do this a different way:

#ifdef CONFIG_NET_NS
static inline struct net * cifs_net_ns(struct TCP_Server_Info *srv)

{
	return srv->net;
}
<other CONFIG_NET_NS cases>
#else
static inline struct net * cifs_net_ns(struct TCP_Server_Info *srv)
{
	return &init_net;
}
Quoted 2 lines

<other no-ops>
#endif /* CONFIG_NET_NS */

If you want to duplicate more code and open the possibility of the declarations mismatching if the config changes. *shrug*

Quoted 10 lines
Quoted 10 lines
fs/cifs/cifsglob.h +10 -0
+
+static inline void
+cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+#ifdef CONFIG_NET_NS
+	srv->net = net;
+#endif
+}
+
+#ifdef CONFIG_NET_NS
Quoted 8 lines

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 259

Quoted 4 lines
fs/cifs/cifsglob.h +4 -0
+#define cifs_use_net_ns() (1)
+#else
+#define cifs_use_net_ns() (0)
+#endif

This looks wrong -- we shouldn't need this at all. The #ifdef bits in your patch already make all the cases below become empty/no-ops when CONFIG_NET_NS=n.

Except that bloat-o-meter said they were still generating code and making the kernel bigger. (Things like calling get() and put() on init_net to twiddle its reference count.)

I'm a long-time embedded programmer, I try not to make the code bigger than necessary, especially when we have a config symbol to remove stuff.
I did ponder turning those into HAVE_NET_HS so the if was more
obviously against a constant.

Quoted 11 lines
Quoted 11 lines
fs/cifs/cifsglob.h +2 -0
+
+/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
fs/cifs/connect.c +0 -0
index cc1a860..b4faef0 100644
@@ -1545,6 +1545,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
Quoted 8 lines

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1550

Quoted 5 lines
fs/cifs/connect.c +3 -0
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (cifs_use_net_ns()
+		    && cifs_net_ns(server) == current->nsproxy->net_ns)
+			continue;

This looks wrong -- you want to invert part of this I think (and drop the unnecessary cifs_use_net_ns()):

You're right, I got the test backwards. Thanks.

The reason for the guards is that compiler couldn't tell that current->nsproxy->net_ns always contains the same value, so without a test against a constant allowing it to do dead code elimination, it will generate code to perform the useless test.

Quoted 14 lines
	if (cifs_net_ns(server) != current->nsproxy->net_ns)
		continue;

This is obvious when you note that the context below shows that we 'continue' to the next entry when the addresses don't match:

Quoted 8 lines
fs/cifs/connect.c +1 -0
+
 		if (!match_address(server, addr,
 				   (struct sockaddr *)&vol->srcaddr))
 			continue;

@@ -1572,6 +1576,9 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 		return;
 	}
Quoted 7 lines

fs/cifs/connect.c cifs_put_tcp_session(struct TCP_Server_Info *server) · line 1581

Quoted 3 lines
fs/cifs/connect.c +3 -0
+	if (cifs_use_net_ns())
+		put_net(cifs_net_ns(server));
+

I think this should just be:

	put_net(cifs_net_ns(server));

Hmmm... that one you're probably right, because include/net/net_namespace.h makes put_net() an empty inline, so as long as cifs_net_ns() has no side effects the compiler should be able to drop the whole thing out. (Whether or not it will, I have to test...)

Quoted 7 lines
Quoted 7 lines
fs/cifs/connect.c +0 -0
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 

@@ -1677,6 +1684,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	       sizeof(tcp_ses->srcaddr));
 	++tcp_ses->srv_count;
Quoted 6 lines

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1689

Quoted 3 lines
fs/cifs/connect.c +3 -0
+	if (cifs_use_net_ns())
+		cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
+

Just use cifs_set_net_ns() because it already turns into a no-op when CONFIG_NET_NS=n

no-op yes. no-code, I want to make sure. (I tried it with just the macros the first time through, and scripts/bloat-o-meter kept saying code was being generated. I'll fire up objdump and see if the disassembly tells me anything...)

Quoted 30 lines

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1734

Quoted 9 lines
fs/cifs/connect.c +2 -0
 	if (addr.ss_family == AF_INET6) {
 		cFYI(1, "attempting ipv6 connect");
 		/* BB should we allow ipv6 on port 139? */

@@ -1720,6 +1730,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
 
+	if (cifs_use_net_ns())
+		put_net(cifs_net_ns(tcp_ses));

etc.

Quoted 18 lines
fs/cifs/connect.c +3 -2
+
 out_err:
 	if (tcp_ses) {
 		if (!IS_ERR(tcp_ses->hostname))

@@ -2145,8 +2158,8 @@ ipv4_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
 
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating socket", rc);
 			return rc;

@@ -2310,11 +2323,10 @@ ipv6_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
Quoted 10 lines

fs/cifs/connect.c ipv6_connect(struct TCP_Server_Info *server) · line 2331

Quoted 10 lines
fs/cifs/connect.c +2 -3
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET6, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET6,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating ipv6 socket", rc);
-			socket = NULL;
 			return rc;
 		}

Note, those two add 16 bytes each on x86-64 (two extra 8-byte arguments), but that I couldn't easily stop. I might try to merge the ipv4/ipv6 functions, but that's a separate patch.

Rob

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#6 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 2)

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses/routing visible from the container rather than from init context.

Signed-off-by Rob Landley <[email protected]>

---

Updated with Matt's feedback and to apply to current linus-git.

 fs/cifs/cifsglob.h |   37 +++++++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   14 ++++++++++++--
 2 files changed, 49 insertions(+), 2 deletions(-)
fs/cifs/cifsglob.h +37 -0
index 606ca8b..8175d31 100644
@@ -165,6 +165,9 @@ struct TCP_Server_Info {
 	struct socket *ssocket;
 	struct sockaddr_storage dstaddr;
 	struct sockaddr_storage srcaddr; /* locally bind to this IP */
+#ifdef CONFIG_NET_NS
+	struct net *net;
+#endif
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
 	struct list_head pending_mid_q;

@@ -224,6 +227,40 @@ struct TCP_Server_Info {
 };
 
 /*
+ * Macros to allow the TCP_Server_Info->net field and related code to drop out
+ * when CONFIG_NET_NS isn't set.
+ */
+
+#ifdef CONFIG_NET_NS
+
+#define HAVE_NET_NS 1
+
+static inline struct net *cifs_net_ns(struct TCP_Server_Info *srv)
+{
+	return srv->net;
+}
+
+static inline void cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+	srv->net = net;
+}
+
+#else
+
+#define HAVE_NET_NS 0
+
+static inline struct net *cifs_net_ns(struct TCP_Server_Info *srv)
+{
+	return &init_net;
+}
+
+static inline void cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+}
+
+#endif
+
+/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
fs/cifs/connect.c +12 -2
index a65d311..7dab1d3 100644
@@ -1577,6 +1577,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (HAVE_NET_NS &&
+		    cifs_net_ns(server) != current->nsproxy->net_ns)
+			continue;
+
 		if (!match_address(server, addr,
 				   (struct sockaddr *)&vol->srcaddr))
 			continue;

@@ -1607,6 +1611,8 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 		return;
 	}
 
+	put_net(cifs_net_ns(server));
+
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 

@@ -1712,6 +1718,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	       sizeof(tcp_ses->srcaddr));
 	++tcp_ses->srv_count;
 
+	cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
+
 	if (addr.ss_family == AF_INET6) {
 		cFYI(1, "attempting ipv6 connect");
 		/* BB should we allow ipv6 on port 139? */

@@ -1754,6 +1762,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
 
+	put_net(cifs_net_ns(tcp_ses));
+
 out_err:
 	if (tcp_ses) {
 		if (!IS_ERR(tcp_ses->hostname))

@@ -2265,8 +2275,8 @@ generic_ip_connect(struct TCP_Server_Info *server)
 	}
 
 	if (socket == NULL) {
-		rc = sock_create_kern(sfamily, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), sfamily, SOCK_STREAM,
+				   IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating socket", rc);
 			server->ssocket = NULL;

A message that is not in the archive (sent off-list or not yet received): [email protected]

#8 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 2)

Quoted 29 lines · On Tue, 11 Jan 2011 12:04:54 -0600 Rob Landley…

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses/routing visible from the container rather than from init context.

Signed-off-by Rob Landley <[email protected]>

---

Updated with Matt's feedback and to apply to current linus-git.

 fs/cifs/cifsglob.h |   37 +++++++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   14 ++++++++++++--
 2 files changed, 49 insertions(+), 2 deletions(-)
fs/cifs/cifsglob.h +3 -0
index 606ca8b..8175d31 100644
@@ -165,6 +165,9 @@ struct TCP_Server_Info {
 	struct socket *ssocket;
 	struct sockaddr_storage dstaddr;
 	struct sockaddr_storage srcaddr; /* locally bind to this IP */
+#ifdef CONFIG_NET_NS
+	struct net *net;
+#endif
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
 	struct list_head pending_mid_q;

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 228

Quoted 2 lines
fs/cifs/cifsglob.h +0 -0
@@ -224,6 +227,40 @@ struct TCP_Server_Info {
 };

I've got a patch queued that rearranges some fields in TCP_Server_Info according to pahole's recommendations. You may want to base this patch on that.

It might also be good to run pahole on this after your patch to see if it might be better placed...

Quoted 43 lines
fs/cifs/cifsglob.h +34 -0
 /*
+ * Macros to allow the TCP_Server_Info->net field and related code to drop out
+ * when CONFIG_NET_NS isn't set.
+ */
+
+#ifdef CONFIG_NET_NS
+
+#define HAVE_NET_NS 1
+
+static inline struct net *cifs_net_ns(struct TCP_Server_Info *srv)
+{
+	return srv->net;
+}
+
+static inline void cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+	srv->net = net;
+}
+
+#else
+
+#define HAVE_NET_NS 0
+
+static inline struct net *cifs_net_ns(struct TCP_Server_Info *srv)
+{
+	return &init_net;
+}
+
+static inline void cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+}
+
+#endif
+
+/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
fs/cifs/connect.c +0 -0
index a65d311..7dab1d3 100644

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1583

Quoted 7 lines
fs/cifs/connect.c +4 -0
@@ -1577,6 +1577,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (HAVE_NET_NS &&
+		    cifs_net_ns(server) != current->nsproxy->net_ns)
+			continue;
+

This HAVE_NET_NS thing is pretty ugly. This is not a high-performance codepath. My vote would be to get rid of this and just have the useless test when CONFIG_NET_NS isn't set.

Alternately, you could turn the comparison into a static inline or macro, and simply have that compile down to nothing when CONFIG_NET_NS isn't set.

Quoted 22 lines
fs/cifs/connect.c +4 -0
 		if (!match_address(server, addr,
 				   (struct sockaddr *)&vol->srcaddr))
 			continue;

@@ -1607,6 +1611,8 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 		return;
 	}
 
+	put_net(cifs_net_ns(server));
+
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 

@@ -1712,6 +1718,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	       sizeof(tcp_ses->srcaddr));
 	++tcp_ses->srv_count;
 
+	cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
+
 	if (addr.ss_family == AF_INET6) {
 		cFYI(1, "attempting ipv6 connect");
 		/* BB should we allow ipv6 on port 139? */

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1766

Quoted 5 lines
fs/cifs/connect.c +2 -0
@@ -1754,6 +1762,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
 
+	put_net(cifs_net_ns(tcp_ses));
+

	^^^^

This looks like it will oops if you end up doing the "goto out_err_crypto_release" after the extract_hostname call.

Quoted 18 lines
fs/cifs/connect.c +2 -3
 out_err:
 	if (tcp_ses) {
 		if (!IS_ERR(tcp_ses->hostname))

@@ -2265,8 +2275,8 @@ generic_ip_connect(struct TCP_Server_Info *server)
 	}
 
 	if (socket == NULL) {
-		rc = sock_create_kern(sfamily, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), sfamily, SOCK_STREAM,
+				   IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating socket", rc);
 			server->ssocket = NULL;
--

To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line
Jeff Layton <[email protected]>

A message that is not in the archive (sent off-list or not yet received): 20110111163000.04d02a7f-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org

author
#10 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 2)

Quoted 3 lines · Jeff Layton

I've got a patch queued that rearranges some fields in TCP_Server_Info according to pahole's recommendations. You may want to base this patch on that.

Queued where?

Quoted 2 lines

It might also be good to run pahole on this after your patch to see if it might be better placed...

I put it right after the network address fields affected by it. I can stick it between:

char *hostname; /* hostname portion of UNC string */ struct socket *ssocket;

So that two consecutive pointers become three consecutive pointers. But again, I haven't seen what you did to the structure yet...

Quoted 8 lines

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1583

Quoted 6 lines
fs/cifs/connect.c +4 -0
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (HAVE_NET_NS &&
+		    cifs_net_ns(server) != current->nsproxy->net_ns)
+			continue;
+

This HAVE_NET_NS thing is pretty ugly.

It's a compile time constant optimized away by the compiler, and cleanly taking out the entire dependent clause with it when it's false (because && only evaluates the right side when the left side is true, meaning the if can never trigger, thus dead code elimination).

How is that ugly? It does exactly what it says. I'm not sure what the criteria are here. (Was it better when it looked like a function rather than a constant? I'd use the CONFIG_NET_NS symbol directly if it was 0 or 1 rather than undefined or 1, but it isn't. It's designed for use with #ifdefs only, not from C code.)

Quoted 7 lines

This is not a high-performance
codepath. My vote would be to get rid of this and just have the useless test when CONFIG_NET_NS isn't set.

Alternately, you could turn the comparison into a static inline or macro, and simply have that compile down to nothing when CONFIG_NET_NS isn't set.

I tried returning a constant from a static inline, it doesn't propogate the constant far enough to do compile time dead code elimination based on the return value under gcc 4.4.3.

As for a macro, the constant already is a macro. So adding more #ifdefs to the headers to make the code in the function look like it's doing something other than it actually is somehow improves matters? (I'm not understanding the aesthetic criteria here at all.)

I think I'll just ignore the bloat. Make allnoconfig is already 800k compressed, what's a few more bytes...

Quoted 9 lines

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1766

Quoted 6 lines
fs/cifs/connect.c +2 -0
@@ -1754,6 +1762,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
 
+	put_net(cifs_net_ns(tcp_ses));
+

	^^^^

This looks like it will oops if you end up doing the "goto out_err_crypto_release" after the extract_hostname call.

Hmmm... Yup, failure case when CONFIG_NET_NS enabled will dereference the null pointer. I need to move the initialization up a few lines. Thanks.

Rob

A message that is not in the archive (sent off-list or not yet received): [email protected]

#12 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 2)

Quoted 7 lines · On Wed, 12 Jan 2011 07:57:36 -0600 Rob Landley…
Quoted 3 lines · Jeff Layton

I've got a patch queued that rearranges some fields in TCP_Server_Info according to pahole's recommendations. You may want to base this patch on that.

Queued where?

It was sent to the list a week or so ago:

http://article.gmane.org/gmane.linux.kernel.cifs/2205

...not sure if/when Steve is going to merge it though.

Quoted 12 lines
Quoted 2 lines

It might also be good to run pahole on this after your patch to see if it might be better placed...

I put it right after the network address fields affected by it. I can stick it between:

char *hostname; /* hostname portion of UNC string */ struct socket *ssocket;

So that two consecutive pointers become three consecutive pointers. But again, I haven't seen what you did to the structure yet...

Your placement may be fine, but you might consider using pahole afterward to see whether it might be better placed elsewhere.

Quoted 14 lines
Quoted 8 lines

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1583

Quoted 6 lines
fs/cifs/connect.c +4 -0
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (HAVE_NET_NS &&
+		    cifs_net_ns(server) != current->nsproxy->net_ns)
+			continue;
+

This HAVE_NET_NS thing is pretty ugly.

It's a compile time constant optimized away by the compiler, and cleanly taking out the entire dependent clause with it when it's false (because && only evaluates the right side when the left side is true, meaning the if can never trigger, thus dead code elimination).

Quoted 6 lines

How is that ugly? It does exactly what it says. I'm not sure what the criteria are here. (Was it better when it looked like a function rather than a constant? I'd use the CONFIG_NET_NS symbol directly if it was 0 or 1 rather than undefined or 1, but it isn't. It's designed for use with #ifdefs only, not from C code.)

It's ugly because it makes the code difficult to read, and readability trumps these sorts of micro-optimizations. If this were a high traffic codepath, I might feel differently but this is generally only touched once per mount.

Quoted 24 lines
Quoted 7 lines

This is not a high-performance
codepath. My vote would be to get rid of this and just have the useless test when CONFIG_NET_NS isn't set.

Alternately, you could turn the comparison into a static inline or macro, and simply have that compile down to nothing when CONFIG_NET_NS isn't set.

I tried returning a constant from a static inline, it doesn't propogate the constant far enough to do compile time dead code elimination based on the return value under gcc 4.4.3.

As for a macro, the constant already is a macro. So adding more #ifdefs to the headers to make the code in the function look like it's doing something other than it actually is somehow improves matters? (I'm not understanding the aesthetic criteria here at all.)

I think I'll just ignore the bloat. Make allnoconfig is already 800k compressed, what's a few more bytes...

Quoted 4 lines
Quoted 4 lines
fs/cifs/connect.c +0 -0
@@ -1754,6 +1762,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
Quoted 8 lines
Quoted 5 lines

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1766

Quoted 2 lines
fs/cifs/connect.c +2 -0
+	put_net(cifs_net_ns(tcp_ses));
+

	^^^^

This looks like it will oops if you end up doing the "goto out_err_crypto_release" after the extract_hostname call.

Hmmm... Yup, failure case when CONFIG_NET_NS enabled will dereference the null pointer. I need to move the initialization up a few lines.

That seems like the simplest fix.

Signature · 1 line
Jeff Layton <[email protected]>
#13 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 2)

In reply to a message not in the archive
Quoted 7 lines · On Wed, 12 Jan 2011 07:57:36 -0600 Rob Landley…
Quoted 3 lines · Jeff Layton

I've got a patch queued that rearranges some fields in TCP_Server_Info according to pahole's recommendations. You may want to base this patch on that.

Queued where?

It was sent to the list a week or so ago:

http://article.gmane.org/gmane.linux.kernel.cifs/2205

...not sure if/when Steve is going to merge it though.

Quoted 12 lines
Quoted 2 lines

It might also be good to run pahole on this after your patch to see if it might be better placed...

I put it right after the network address fields affected by it. I can stick it between:

char *hostname; /* hostname portion of UNC string */ struct socket *ssocket;

So that two consecutive pointers become three consecutive pointers. But again, I haven't seen what you did to the structure yet...

Your placement may be fine, but you might consider using pahole afterward to see whether it might be better placed elsewhere.

Quoted 14 lines
Quoted 8 lines

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1583

Quoted 6 lines
fs/cifs/connect.c +4 -0
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (HAVE_NET_NS &&
+		    cifs_net_ns(server) != current->nsproxy->net_ns)
+			continue;
+

This HAVE_NET_NS thing is pretty ugly.

It's a compile time constant optimized away by the compiler, and cleanly taking out the entire dependent clause with it when it's false (because && only evaluates the right side when the left side is true, meaning the if can never trigger, thus dead code elimination).

Quoted 6 lines

How is that ugly? It does exactly what it says. I'm not sure what the criteria are here. (Was it better when it looked like a function rather than a constant? I'd use the CONFIG_NET_NS symbol directly if it was 0 or 1 rather than undefined or 1, but it isn't. It's designed for use with #ifdefs only, not from C code.)

It's ugly because it makes the code difficult to read, and readability trumps these sorts of micro-optimizations. If this were a high traffic codepath, I might feel differently but this is generally only touched once per mount.

Quoted 24 lines
Quoted 7 lines

This is not a high-performance
codepath. My vote would be to get rid of this and just have the useless test when CONFIG_NET_NS isn't set.

Alternately, you could turn the comparison into a static inline or macro, and simply have that compile down to nothing when CONFIG_NET_NS isn't set.

I tried returning a constant from a static inline, it doesn't propogate the constant far enough to do compile time dead code elimination based on the return value under gcc 4.4.3.

As for a macro, the constant already is a macro. So adding more #ifdefs to the headers to make the code in the function look like it's doing something other than it actually is somehow improves matters? (I'm not understanding the aesthetic criteria here at all.)

I think I'll just ignore the bloat. Make allnoconfig is already 800k compressed, what's a few more bytes...

Quoted 4 lines
Quoted 4 lines
fs/cifs/connect.c +0 -0
@@ -1754,6 +1762,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
Quoted 8 lines
Quoted 5 lines

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1766

Quoted 2 lines
fs/cifs/connect.c +2 -0
+	put_net(cifs_net_ns(tcp_ses));
+

	^^^^

This looks like it will oops if you end up doing the "goto out_err_crypto_release" after the extract_hostname call.

Hmmm... Yup, failure case when CONFIG_NET_NS enabled will dereference the null pointer. I need to move the initialization up a few lines.

That seems like the simplest fix.

Signature · 1 line
Jeff Layton <[email protected]>
author
#14 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 3)

In reply to a message not in the archive

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses/routing visible from the container rather than from init context.

Signed-off-by Rob Landley <[email protected]>

---

Now using net_eq(), with the initialization moved up so the error path doesn't dereference a null on the put.

 fs/cifs/cifsglob.h |   33 +++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   12 ++++++++++--
 2 files changed, 43 insertions(+), 2 deletions(-)
fs/cifs/cifsglob.h +33 -0
index 606ca8b..54cd4ab 100644
@@ -165,6 +165,9 @@ struct TCP_Server_Info {
 	struct socket *ssocket;
 	struct sockaddr_storage dstaddr;
 	struct sockaddr_storage srcaddr; /* locally bind to this IP */
+#ifdef CONFIG_NET_NS
+	struct net *net;
+#endif
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
 	struct list_head pending_mid_q;

@@ -224,6 +227,36 @@ struct TCP_Server_Info {
 };
 
 /*
+ * Macros to allow the TCP_Server_Info->net field and related code to drop out
+ * when CONFIG_NET_NS isn't set.
+ */
+
+#ifdef CONFIG_NET_NS
+
+static inline struct net *cifs_net_ns(struct TCP_Server_Info *srv)
+{
+	return srv->net;
+}
+
+static inline void cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+	srv->net = net;
+}
+
+#else
+
+static inline struct net *cifs_net_ns(struct TCP_Server_Info *srv)
+{
+	return &init_net;
+}
+
+static inline void cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+}
+
+#endif
+
+/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
fs/cifs/connect.c +10 -2
index a65d311..53679f6 100644
@@ -1577,6 +1577,9 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (!net_eq(cifs_net_ns(server), current->nsproxy->net_ns))
+			continue;
+
 		if (!match_address(server, addr,
 				   (struct sockaddr *)&vol->srcaddr))
 			continue;

@@ -1607,6 +1610,8 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 		return;
 	}
 
+	put_net(cifs_net_ns(server));
+
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 

@@ -1679,6 +1684,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 		goto out_err;
 	}
 
+	cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
 	tcp_ses->hostname = extract_hostname(volume_info->UNC);
 	if (IS_ERR(tcp_ses->hostname)) {
 		rc = PTR_ERR(tcp_ses->hostname);

@@ -1754,6 +1760,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
 
+	put_net(cifs_net_ns(tcp_ses));
+
 out_err:
 	if (tcp_ses) {
 		if (!IS_ERR(tcp_ses->hostname))

@@ -2265,8 +2273,8 @@ generic_ip_connect(struct TCP_Server_Info *server)
 	}
 
 	if (socket == NULL) {
-		rc = sock_create_kern(sfamily, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), sfamily, SOCK_STREAM,
+				   IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating socket", rc);
 			server->ssocket = NULL;

A message that is not in the archive (sent off-list or not yet received): [email protected]

Reviewed-by
#16 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 3)

Quoted 6 lines · On Thu, 13 Jan 2011 12:55:04 -0600 Rob Landley…

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses/routing visible from the container rather than from init context.

Signed-off-by Rob Landley <[email protected]>
Quoted 9 lines

---

Now using net_eq(), with the initialization moved up so the error path doesn't dereference a null on the put.

 fs/cifs/cifsglob.h |   33 +++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   12 ++++++++++--
 2 files changed, 43 insertions(+), 2 deletions(-)

Looks good to me:

Reviewed-by Jeff Layton <[email protected]>
Reviewed-by
#17 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 3)

In reply to a message not in the archive
Quoted 6 lines · On Thu, 13 Jan 2011 12:55:04 -0600 Rob Landley…

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses/routing visible from the container rather than from init context.

Signed-off-by Rob Landley <[email protected]>
Quoted 9 lines

---

Now using net_eq(), with the initialization moved up so the error path doesn't dereference a null on the put.

 fs/cifs/cifsglob.h |   33 +++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   12 ++++++++++--
 2 files changed, 43 insertions(+), 2 deletions(-)

Looks good to me:

Reviewed-by Jeff Layton <[email protected]>
author
#18 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 3)

In reply to a message not in the archive

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses/routing visible from the container rather than from init context.

Signed-off-by Rob Landley <[email protected]>

---

Now using net_eq(), with the initialization moved up so the error path doesn't dereference a null on the put.

 fs/cifs/cifsglob.h |   33 +++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   12 ++++++++++--
 2 files changed, 43 insertions(+), 2 deletions(-)
fs/cifs/cifsglob.h +33 -0
index 606ca8b..54cd4ab 100644
@@ -165,6 +165,9 @@ struct TCP_Server_Info {
 	struct socket *ssocket;
 	struct sockaddr_storage dstaddr;
 	struct sockaddr_storage srcaddr; /* locally bind to this IP */
+#ifdef CONFIG_NET_NS
+	struct net *net;
+#endif
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
 	struct list_head pending_mid_q;

@@ -224,6 +227,36 @@ struct TCP_Server_Info {
 };
 
 /*
+ * Macros to allow the TCP_Server_Info->net field and related code to drop out
+ * when CONFIG_NET_NS isn't set.
+ */
+
+#ifdef CONFIG_NET_NS
+
+static inline struct net *cifs_net_ns(struct TCP_Server_Info *srv)
+{
+	return srv->net;
+}
+
+static inline void cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+	srv->net = net;
+}
+
+#else
+
+static inline struct net *cifs_net_ns(struct TCP_Server_Info *srv)
+{
+	return &init_net;
+}
+
+static inline void cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+}
+
+#endif
+
+/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
fs/cifs/connect.c +10 -2
index a65d311..53679f6 100644
@@ -1577,6 +1577,9 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (!net_eq(cifs_net_ns(server), current->nsproxy->net_ns))
+			continue;
+
 		if (!match_address(server, addr,
 				   (struct sockaddr *)&vol->srcaddr))
 			continue;

@@ -1607,6 +1610,8 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 		return;
 	}
 
+	put_net(cifs_net_ns(server));
+
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 

@@ -1679,6 +1684,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 		goto out_err;
 	}
 
+	cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
 	tcp_ses->hostname = extract_hostname(volume_info->UNC);
 	if (IS_ERR(tcp_ses->hostname)) {
 		rc = PTR_ERR(tcp_ses->hostname);

@@ -1754,6 +1760,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
 
+	put_net(cifs_net_ns(tcp_ses));
+
 out_err:
 	if (tcp_ses) {
 		if (!IS_ERR(tcp_ses->hostname))

@@ -2265,8 +2273,8 @@ generic_ip_connect(struct TCP_Server_Info *server)
 	}
 
 	if (socket == NULL) {
-		rc = sock_create_kern(sfamily, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), sfamily, SOCK_STREAM,
+				   IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating socket", rc);
 			server->ssocket = NULL;
author
#19 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 2)

In reply to a message not in the archive
Quoted 3 lines · Jeff Layton

I've got a patch queued that rearranges some fields in TCP_Server_Info according to pahole's recommendations. You may want to base this patch on that.

Queued where?

Quoted 2 lines

It might also be good to run pahole on this after your patch to see if it might be better placed...

I put it right after the network address fields affected by it. I can stick it between:

char *hostname; /* hostname portion of UNC string */ struct socket *ssocket;

So that two consecutive pointers become three consecutive pointers. But again, I haven't seen what you did to the structure yet...

Quoted 8 lines

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1583

Quoted 6 lines
fs/cifs/connect.c +4 -0
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (HAVE_NET_NS &&
+		    cifs_net_ns(server) != current->nsproxy->net_ns)
+			continue;
+

This HAVE_NET_NS thing is pretty ugly.

It's a compile time constant optimized away by the compiler, and cleanly taking out the entire dependent clause with it when it's false (because && only evaluates the right side when the left side is true, meaning the if can never trigger, thus dead code elimination).

How is that ugly? It does exactly what it says. I'm not sure what the criteria are here. (Was it better when it looked like a function rather than a constant? I'd use the CONFIG_NET_NS symbol directly if it was 0 or 1 rather than undefined or 1, but it isn't. It's designed for use with #ifdefs only, not from C code.)

Quoted 7 lines

This is not a high-performance
codepath. My vote would be to get rid of this and just have the useless test when CONFIG_NET_NS isn't set.

Alternately, you could turn the comparison into a static inline or macro, and simply have that compile down to nothing when CONFIG_NET_NS isn't set.

I tried returning a constant from a static inline, it doesn't propogate the constant far enough to do compile time dead code elimination based on the return value under gcc 4.4.3.

As for a macro, the constant already is a macro. So adding more #ifdefs to the headers to make the code in the function look like it's doing something other than it actually is somehow improves matters? (I'm not understanding the aesthetic criteria here at all.)

I think I'll just ignore the bloat. Make allnoconfig is already 800k compressed, what's a few more bytes...

Quoted 9 lines

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1766

Quoted 6 lines
fs/cifs/connect.c +2 -0
@@ -1754,6 +1762,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
 
+	put_net(cifs_net_ns(tcp_ses));
+

	^^^^

This looks like it will oops if you end up doing the "goto out_err_crypto_release" after the extract_hostname call.

Hmmm... Yup, failure case when CONFIG_NET_NS enabled will dereference the null pointer. I need to move the initialization up a few lines. Thanks.

Rob

author
#20 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 2)

In reply to a message not in the archive
Quoted 30 lines · Jeff Layton
Quoted 27 lines · On Tue, 11 Jan 2011 12:04:54 -0600 Rob Landley…

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses/routing visible from the container rather than from init context.

Signed-off-by Rob Landley <[email protected]>

---

Updated with Matt's feedback and to apply to current linus-git.

 fs/cifs/cifsglob.h |   37 +++++++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   14 ++++++++++++--
 2 files changed, 49 insertions(+), 2 deletions(-)
fs/cifs/cifsglob.h +3 -0
index 606ca8b..8175d31 100644
@@ -165,6 +165,9 @@ struct TCP_Server_Info {
 	struct socket *ssocket;
 	struct sockaddr_storage dstaddr;
 	struct sockaddr_storage srcaddr; /* locally bind to this IP */
+#ifdef CONFIG_NET_NS
+	struct net *net;
+#endif
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
Quoted 8 lines

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 228

Quoted 4 lines
fs/cifs/cifsglob.h +0 -0
 	struct list_head pending_mid_q;

@@ -224,6 +227,40 @@ struct TCP_Server_Info {
 };

I've got a patch queued that rearranges some fields in TCP_Server_Info according to pahole's recommendations. You may want to base this patch on that.

I confirmed that where it is just misses being affected by your patch (offset but no fuzz), and it follows struct sockaddr_storage which include/linux/socket.h A) pads to 128 bytes, B) adds an alignment compiler directive to just to be sure.

So it seems reasonable to leave it where it is for the moment.

Rob

author
#21 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 2)

In reply to a message not in the archive
Quoted 30 lines · Jeff Layton
Quoted 27 lines · On Tue, 11 Jan 2011 12:04:54 -0600 Rob Landley…

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses/routing visible from the container rather than from init context.

Signed-off-by Rob Landley <[email protected]>

---

Updated with Matt's feedback and to apply to current linus-git.

 fs/cifs/cifsglob.h |   37 +++++++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   14 ++++++++++++--
 2 files changed, 49 insertions(+), 2 deletions(-)
fs/cifs/cifsglob.h +3 -0
index 606ca8b..8175d31 100644
@@ -165,6 +165,9 @@ struct TCP_Server_Info {
 	struct socket *ssocket;
 	struct sockaddr_storage dstaddr;
 	struct sockaddr_storage srcaddr; /* locally bind to this IP */
+#ifdef CONFIG_NET_NS
+	struct net *net;
+#endif
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
Quoted 8 lines

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 228

Quoted 4 lines
fs/cifs/cifsglob.h +0 -0
 	struct list_head pending_mid_q;

@@ -224,6 +227,40 @@ struct TCP_Server_Info {
 };

I've got a patch queued that rearranges some fields in TCP_Server_Info according to pahole's recommendations. You may want to base this patch on that.

I confirmed that where it is just misses being affected by your patch (offset but no fuzz), and it follows struct sockaddr_storage which include/linux/socket.h A) pads to 128 bytes, B) adds an alignment compiler directive to just to be sure.

So it seems reasonable to leave it where it is for the moment.

Rob

#22 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 2)

In reply to a message not in the archive
Quoted 29 lines · On Tue, 11 Jan 2011 12:04:54 -0600 Rob Landley…

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses/routing visible from the container rather than from init context.

Signed-off-by Rob Landley <[email protected]>

---

Updated with Matt's feedback and to apply to current linus-git.

 fs/cifs/cifsglob.h |   37 +++++++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   14 ++++++++++++--
 2 files changed, 49 insertions(+), 2 deletions(-)
fs/cifs/cifsglob.h +3 -0
index 606ca8b..8175d31 100644
@@ -165,6 +165,9 @@ struct TCP_Server_Info {
 	struct socket *ssocket;
 	struct sockaddr_storage dstaddr;
 	struct sockaddr_storage srcaddr; /* locally bind to this IP */
+#ifdef CONFIG_NET_NS
+	struct net *net;
+#endif
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
 	struct list_head pending_mid_q;

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 228

Quoted 2 lines
fs/cifs/cifsglob.h +0 -0
@@ -224,6 +227,40 @@ struct TCP_Server_Info {
 };

I've got a patch queued that rearranges some fields in TCP_Server_Info according to pahole's recommendations. You may want to base this patch on that.

It might also be good to run pahole on this after your patch to see if it might be better placed...

Quoted 43 lines
fs/cifs/cifsglob.h +34 -0
 /*
+ * Macros to allow the TCP_Server_Info->net field and related code to drop out
+ * when CONFIG_NET_NS isn't set.
+ */
+
+#ifdef CONFIG_NET_NS
+
+#define HAVE_NET_NS 1
+
+static inline struct net *cifs_net_ns(struct TCP_Server_Info *srv)
+{
+	return srv->net;
+}
+
+static inline void cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+	srv->net = net;
+}
+
+#else
+
+#define HAVE_NET_NS 0
+
+static inline struct net *cifs_net_ns(struct TCP_Server_Info *srv)
+{
+	return &init_net;
+}
+
+static inline void cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+}
+
+#endif
+
+/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
fs/cifs/connect.c +0 -0
index a65d311..7dab1d3 100644

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1583

Quoted 7 lines
fs/cifs/connect.c +4 -0
@@ -1577,6 +1577,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (HAVE_NET_NS &&
+		    cifs_net_ns(server) != current->nsproxy->net_ns)
+			continue;
+

This HAVE_NET_NS thing is pretty ugly. This is not a high-performance codepath. My vote would be to get rid of this and just have the useless test when CONFIG_NET_NS isn't set.

Alternately, you could turn the comparison into a static inline or macro, and simply have that compile down to nothing when CONFIG_NET_NS isn't set.

Quoted 22 lines
fs/cifs/connect.c +4 -0
 		if (!match_address(server, addr,
 				   (struct sockaddr *)&vol->srcaddr))
 			continue;

@@ -1607,6 +1611,8 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 		return;
 	}
 
+	put_net(cifs_net_ns(server));
+
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 

@@ -1712,6 +1718,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	       sizeof(tcp_ses->srcaddr));
 	++tcp_ses->srv_count;
 
+	cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
+
 	if (addr.ss_family == AF_INET6) {
 		cFYI(1, "attempting ipv6 connect");
 		/* BB should we allow ipv6 on port 139? */

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1766

Quoted 5 lines
fs/cifs/connect.c +2 -0
@@ -1754,6 +1762,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
 
+	put_net(cifs_net_ns(tcp_ses));
+

	^^^^

This looks like it will oops if you end up doing the "goto out_err_crypto_release" after the extract_hostname call.

Quoted 18 lines
fs/cifs/connect.c +2 -3
 out_err:
 	if (tcp_ses) {
 		if (!IS_ERR(tcp_ses->hostname))

@@ -2265,8 +2275,8 @@ generic_ip_connect(struct TCP_Server_Info *server)
 	}
 
 	if (socket == NULL) {
-		rc = sock_create_kern(sfamily, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), sfamily, SOCK_STREAM,
+				   IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating socket", rc);
 			server->ssocket = NULL;
--

To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line
Jeff Layton <[email protected]>
author
#23 lore raw

Re: [PATCH] Teach cifs about network namespaces (take 2)

In reply to a message not in the archive

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses/routing visible from the container rather than from init context.

Signed-off-by Rob Landley <[email protected]>

---

Updated with Matt's feedback and to apply to current linus-git.

 fs/cifs/cifsglob.h |   37 +++++++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   14 ++++++++++++--
 2 files changed, 49 insertions(+), 2 deletions(-)
fs/cifs/cifsglob.h +37 -0
index 606ca8b..8175d31 100644
@@ -165,6 +165,9 @@ struct TCP_Server_Info {
 	struct socket *ssocket;
 	struct sockaddr_storage dstaddr;
 	struct sockaddr_storage srcaddr; /* locally bind to this IP */
+#ifdef CONFIG_NET_NS
+	struct net *net;
+#endif
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
 	struct list_head pending_mid_q;

@@ -224,6 +227,40 @@ struct TCP_Server_Info {
 };
 
 /*
+ * Macros to allow the TCP_Server_Info->net field and related code to drop out
+ * when CONFIG_NET_NS isn't set.
+ */
+
+#ifdef CONFIG_NET_NS
+
+#define HAVE_NET_NS 1
+
+static inline struct net *cifs_net_ns(struct TCP_Server_Info *srv)
+{
+	return srv->net;
+}
+
+static inline void cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+	srv->net = net;
+}
+
+#else
+
+#define HAVE_NET_NS 0
+
+static inline struct net *cifs_net_ns(struct TCP_Server_Info *srv)
+{
+	return &init_net;
+}
+
+static inline void cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+}
+
+#endif
+
+/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
fs/cifs/connect.c +12 -2
index a65d311..7dab1d3 100644
@@ -1577,6 +1577,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (HAVE_NET_NS &&
+		    cifs_net_ns(server) != current->nsproxy->net_ns)
+			continue;
+
 		if (!match_address(server, addr,
 				   (struct sockaddr *)&vol->srcaddr))
 			continue;

@@ -1607,6 +1611,8 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 		return;
 	}
 
+	put_net(cifs_net_ns(server));
+
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 

@@ -1712,6 +1718,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	       sizeof(tcp_ses->srcaddr));
 	++tcp_ses->srv_count;
 
+	cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
+
 	if (addr.ss_family == AF_INET6) {
 		cFYI(1, "attempting ipv6 connect");
 		/* BB should we allow ipv6 on port 139? */

@@ -1754,6 +1762,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
 
+	put_net(cifs_net_ns(tcp_ses));
+
 out_err:
 	if (tcp_ses) {
 		if (!IS_ERR(tcp_ses->hostname))

@@ -2265,8 +2275,8 @@ generic_ip_connect(struct TCP_Server_Info *server)
 	}
 
 	if (socket == NULL) {
-		rc = sock_create_kern(sfamily, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), sfamily, SOCK_STREAM,
+				   IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating socket", rc);
 			server->ssocket = NULL;
#24 lore raw
In reply to a message not in the archive
Quoted 24 lines · Rob Landley
Quoted 23 lines · Matt Helsley
Quoted 22 lines · Rob Landley

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses and routing visible from a container rather than from init context.

For a long drawn out test reproduction sequence, see:

http://landley.livejournal.com/47024.html
http://landley.livejournal.com/47205.html
http://landley.livejournal.com/47476.html

Signed-off-by Rob Landley <[email protected]>

---

 fs/cifs/cifsglob.h |   32 ++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   22 +++++++++++++++++-----
 2 files changed, 49 insertions(+), 5 deletions(-)
fs/cifs/cifsglob.h +0 -0
index 7136c0c..86f31bb 100644

<snip>

Quoted 10 lines
Quoted 9 lines

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 259

Quoted 5 lines
fs/cifs/cifsglob.h +5 -0
+#ifdef CONFIG_NET_NS
+#define cifs_use_net_ns() (1)
+#else
+#define cifs_use_net_ns() (0)
+#endif

This looks wrong -- we shouldn't need this at all. The #ifdef bits in your patch already make all the cases below become empty/no-ops when CONFIG_NET_NS=n.

Quoted 3 lines

Except that bloat-o-meter said they were still generating code and making the kernel bigger. (Things like calling get() and put() on init_net to twiddle its reference count.)

I think the only code that would be generated is the test discussed below. And we're probably talking a few instructions -- an insignificant amount. There are probably much better places to make a substantial impact on kernel bloat.

Quoted 5 lines

I'm a long-time embedded programmer, I try not to make the code bigger than necessary, especially when we have a config symbol to remove stuff.
I did ponder turning those into HAVE_NET_HS so the if was more
obviously against a constant.

My recollection is that method of doing things is often frowned upon in kernel code. There are exceptions of course, but most use the CONFIG_FOO patterns you've already employed (or that I already suggested).

Quoted 21 lines
Quoted 12 lines
Quoted 12 lines
fs/cifs/cifsglob.h +2 -0
+
+/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
fs/cifs/connect.c +0 -0
index cc1a860..b4faef0 100644
@@ -1545,6 +1545,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
Quoted 8 lines

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1550

Quoted 5 lines
fs/cifs/connect.c +3 -0
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (cifs_use_net_ns()
+		    && cifs_net_ns(server) == current->nsproxy->net_ns)
+			continue;

This looks wrong -- you want to invert part of this I think (and drop the unnecessary cifs_use_net_ns()):

Quoted 6 lines

You're right, I got the test backwards. Thanks.

The reason for the guards is that compiler couldn't tell that current->nsproxy->net_ns always contains the same value, so without a test against a constant allowing it to do dead code elimination, it will generate code to perform the useless test.

I think we're better off with simpler source code than trying to eliminate a few instructions.

Quoted 42 lines
Quoted 15 lines
	if (cifs_net_ns(server) != current->nsproxy->net_ns)
		continue;

This is obvious when you note that the context below shows that we 'continue' to the next entry when the addresses don't match:

Quoted 8 lines
fs/cifs/connect.c +1 -0
+
 		if (!match_address(server, addr,
 				   (struct sockaddr *)&vol->srcaddr))
 			continue;

@@ -1572,6 +1576,9 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 		return;
 	}
Quoted 7 lines

fs/cifs/connect.c cifs_put_tcp_session(struct TCP_Server_Info *server) · line 1581

Quoted 3 lines
fs/cifs/connect.c +3 -0
+	if (cifs_use_net_ns())
+		put_net(cifs_net_ns(server));
+

I think this should just be:

	put_net(cifs_net_ns(server));

Hmmm... that one you're probably right, because include/net/net_namespace.h makes put_net() an empty inline, so as long as cifs_net_ns() has no side effects the compiler should be able to drop the whole thing out. (Whether or not it will, I have to test...)

Quoted 7 lines
Quoted 7 lines
fs/cifs/connect.c +0 -0
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 

@@ -1677,6 +1684,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	       sizeof(tcp_ses->srcaddr));
 	++tcp_ses->srv_count;
Quoted 6 lines

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1689

Quoted 3 lines
fs/cifs/connect.c +3 -0
+	if (cifs_use_net_ns())
+		cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
+

Just use cifs_set_net_ns() because it already turns into a no-op when CONFIG_NET_NS=n

Quoted 4 lines

no-op yes. no-code, I want to make sure. (I tried it with just the macros the first time through, and scripts/bloat-o-meter kept saying code was being generated. I'll fire up objdump and see if the disassembly tells me anything...)

Yup, thanks for making sure.

<snip>

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
fs/cifs/connect.c +2 -2
@@ -2145,8 +2158,8 @@ ipv4_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
 
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating socket", rc);
 			return rc;

@@ -2310,11 +2323,10 @@ ipv6_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
Quoted 14 lines
Quoted 10 lines

fs/cifs/connect.c ipv6_connect(struct TCP_Server_Info *server) · line 2331

Quoted 10 lines
fs/cifs/connect.c +2 -3
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET6, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET6,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating ipv6 socket", rc);
-			socket = NULL;
 			return rc;
 		}

Note, those two add 16 bytes each on x86-64 (two extra 8-byte arguments), but that I couldn't easily stop. I might try to merge the ipv4/ipv6 functions, but that's a separate patch.

If anything I think you're saving more space with your patched code:

int sock_create_kern(int family, int type, int protocol, struct socket **res)

{
        return __sock_create(&init_net, family, type, protocol, res, 1);
}

EXPORT_SYMBOL(sock_create_kern);

It's not an inline function so I expect using sock_create_kern() will take up more stack space than calling __sock_create() directly. On some archs the instructions to make the extra function call might also take up 'significant' space. And you're passing an 8-byte pointer *anyway* so no size difference there unless the compiler is more clever than I expect.

In my humble opinion you don't need to spend so much time counting bytes :).

Cheers,
-Matt

#25 lore raw
In reply to a message not in the archive
Quoted 24 lines · Rob Landley
Quoted 23 lines · Matt Helsley
Quoted 22 lines · Rob Landley

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses and routing visible from a container rather than from init context.

For a long drawn out test reproduction sequence, see:

http://landley.livejournal.com/47024.html
http://landley.livejournal.com/47205.html
http://landley.livejournal.com/47476.html

Signed-off-by Rob Landley <[email protected]>

---

 fs/cifs/cifsglob.h |   32 ++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   22 +++++++++++++++++-----
 2 files changed, 49 insertions(+), 5 deletions(-)
fs/cifs/cifsglob.h +0 -0
index 7136c0c..86f31bb 100644

<snip>

Quoted 10 lines
Quoted 9 lines

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 259

Quoted 5 lines
fs/cifs/cifsglob.h +5 -0
+#ifdef CONFIG_NET_NS
+#define cifs_use_net_ns() (1)
+#else
+#define cifs_use_net_ns() (0)
+#endif

This looks wrong -- we shouldn't need this at all. The #ifdef bits in your patch already make all the cases below become empty/no-ops when CONFIG_NET_NS=n.

Quoted 3 lines

Except that bloat-o-meter said they were still generating code and making the kernel bigger. (Things like calling get() and put() on init_net to twiddle its reference count.)

I think the only code that would be generated is the test discussed below. And we're probably talking a few instructions -- an insignificant amount. There are probably much better places to make a substantial impact on kernel bloat.

Quoted 5 lines

I'm a long-time embedded programmer, I try not to make the code bigger than necessary, especially when we have a config symbol to remove stuff.
I did ponder turning those into HAVE_NET_HS so the if was more
obviously against a constant.

My recollection is that method of doing things is often frowned upon in kernel code. There are exceptions of course, but most use the CONFIG_FOO patterns you've already employed (or that I already suggested).

Quoted 21 lines
Quoted 12 lines
Quoted 12 lines
fs/cifs/cifsglob.h +2 -0
+
+/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
fs/cifs/connect.c +0 -0
index cc1a860..b4faef0 100644
@@ -1545,6 +1545,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
Quoted 8 lines

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1550

Quoted 5 lines
fs/cifs/connect.c +3 -0
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (cifs_use_net_ns()
+		    && cifs_net_ns(server) == current->nsproxy->net_ns)
+			continue;

This looks wrong -- you want to invert part of this I think (and drop the unnecessary cifs_use_net_ns()):

Quoted 6 lines

You're right, I got the test backwards. Thanks.

The reason for the guards is that compiler couldn't tell that current->nsproxy->net_ns always contains the same value, so without a test against a constant allowing it to do dead code elimination, it will generate code to perform the useless test.

I think we're better off with simpler source code than trying to eliminate a few instructions.

Quoted 42 lines
Quoted 15 lines
	if (cifs_net_ns(server) != current->nsproxy->net_ns)
		continue;

This is obvious when you note that the context below shows that we 'continue' to the next entry when the addresses don't match:

Quoted 8 lines
fs/cifs/connect.c +1 -0
+
 		if (!match_address(server, addr,
 				   (struct sockaddr *)&vol->srcaddr))
 			continue;

@@ -1572,6 +1576,9 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 		return;
 	}
Quoted 7 lines

fs/cifs/connect.c cifs_put_tcp_session(struct TCP_Server_Info *server) · line 1581

Quoted 3 lines
fs/cifs/connect.c +3 -0
+	if (cifs_use_net_ns())
+		put_net(cifs_net_ns(server));
+

I think this should just be:

	put_net(cifs_net_ns(server));

Hmmm... that one you're probably right, because include/net/net_namespace.h makes put_net() an empty inline, so as long as cifs_net_ns() has no side effects the compiler should be able to drop the whole thing out. (Whether or not it will, I have to test...)

Quoted 7 lines
Quoted 7 lines
fs/cifs/connect.c +0 -0
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 

@@ -1677,6 +1684,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	       sizeof(tcp_ses->srcaddr));
 	++tcp_ses->srv_count;
Quoted 6 lines

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1689

Quoted 3 lines
fs/cifs/connect.c +3 -0
+	if (cifs_use_net_ns())
+		cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
+

Just use cifs_set_net_ns() because it already turns into a no-op when CONFIG_NET_NS=n

Quoted 4 lines

no-op yes. no-code, I want to make sure. (I tried it with just the macros the first time through, and scripts/bloat-o-meter kept saying code was being generated. I'll fire up objdump and see if the disassembly tells me anything...)

Yup, thanks for making sure.

<snip>

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
fs/cifs/connect.c +2 -2
@@ -2145,8 +2158,8 @@ ipv4_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
 
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating socket", rc);
 			return rc;

@@ -2310,11 +2323,10 @@ ipv6_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
Quoted 14 lines
Quoted 10 lines

fs/cifs/connect.c ipv6_connect(struct TCP_Server_Info *server) · line 2331

Quoted 10 lines
fs/cifs/connect.c +2 -3
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET6, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET6,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating ipv6 socket", rc);
-			socket = NULL;
 			return rc;
 		}

Note, those two add 16 bytes each on x86-64 (two extra 8-byte arguments), but that I couldn't easily stop. I might try to merge the ipv4/ipv6 functions, but that's a separate patch.

If anything I think you're saving more space with your patched code:

int sock_create_kern(int family, int type, int protocol, struct socket **res)

{
        return __sock_create(&init_net, family, type, protocol, res, 1);
}

EXPORT_SYMBOL(sock_create_kern);

It's not an inline function so I expect using sock_create_kern() will take up more stack space than calling __sock_create() directly. On some archs the instructions to make the extra function call might also take up 'significant' space. And you're passing an 8-byte pointer *anyway* so no size difference there unless the compiler is more clever than I expect.

In my humble opinion you don't need to spend so much time counting bytes :).

Cheers,
-Matt

author
#26 lore raw
In reply to a message not in the archive
Quoted 68 lines · Matt Helsley
Quoted 23 lines · Rob Landley

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses and routing visible from a container rather than from init context.

For a long drawn out test reproduction sequence, see:

http://landley.livejournal.com/47024.html
http://landley.livejournal.com/47205.html
http://landley.livejournal.com/47476.html

Signed-off-by Rob Landley <[email protected]>

---

 fs/cifs/cifsglob.h |   32 ++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   22 +++++++++++++++++-----
 2 files changed, 49 insertions(+), 5 deletions(-)
fs/cifs/cifsglob.h +0 -0
index 7136c0c..86f31bb 100644

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 173

Quoted 6 lines
fs/cifs/cifsglob.h +3 -0
@@ -168,6 +168,9 @@ struct TCP_Server_Info {
 		struct sockaddr_in6 sockAddr6;
 	} addr;
 	struct sockaddr_storage srcaddr; /* locally bind to this IP */
+#ifdef CONFIG_NET_NS
+	struct net *net;
+#endif

I'm assuming this bit is correct -- don't know enough about CIFS to be sure...

Quoted 10 lines
fs/cifs/cifsglob.h +3 -0
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
 	struct list_head pending_mid_q;

@@ -227,6 +230,35 @@ struct TCP_Server_Info {
 };
 
 /*
+ * Macros to allow the TCP_Server_Info->net field and related code to drop out
+ * when CONFIG_NET_NS isn't set.
+ */

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 245

Quoted 10 lines
fs/cifs/cifsglob.h +10 -0
+
+static inline struct net *
+cifs_net_ns(struct TCP_Server_Info *srv)
+{
+#ifdef CONFIG_NET_NS
+	return srv->net;
+#else
+	return &init_net;
+#endif
+}

I thought style dictated we do this a different way:

#ifdef CONFIG_NET_NS
static inline struct net * cifs_net_ns(struct TCP_Server_Info *srv)

{
	return srv->net;
}
<other CONFIG_NET_NS cases>
#else
static inline struct net * cifs_net_ns(struct TCP_Server_Info *srv)
{
	return &init_net;
}
Quoted 2 lines

<other no-ops>
#endif /* CONFIG_NET_NS */

If you want to duplicate more code and open the possibility of the declarations mismatching if the config changes. *shrug*

Quoted 10 lines
Quoted 10 lines
fs/cifs/cifsglob.h +10 -0
+
+static inline void
+cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{
+#ifdef CONFIG_NET_NS
+	srv->net = net;
+#endif
+}
+
+#ifdef CONFIG_NET_NS
Quoted 8 lines

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 259

Quoted 4 lines
fs/cifs/cifsglob.h +4 -0
+#define cifs_use_net_ns() (1)
+#else
+#define cifs_use_net_ns() (0)
+#endif

This looks wrong -- we shouldn't need this at all. The #ifdef bits in your patch already make all the cases below become empty/no-ops when CONFIG_NET_NS=n.

Except that bloat-o-meter said they were still generating code and making the kernel bigger. (Things like calling get() and put() on init_net to twiddle its reference count.)

I'm a long-time embedded programmer, I try not to make the code bigger than necessary, especially when we have a config symbol to remove stuff.
I did ponder turning those into HAVE_NET_HS so the if was more
obviously against a constant.

Quoted 11 lines
Quoted 11 lines
fs/cifs/cifsglob.h +2 -0
+
+/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
fs/cifs/connect.c +0 -0
index cc1a860..b4faef0 100644
@@ -1545,6 +1545,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
Quoted 8 lines

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1550

Quoted 5 lines
fs/cifs/connect.c +3 -0
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (cifs_use_net_ns()
+		    && cifs_net_ns(server) == current->nsproxy->net_ns)
+			continue;

This looks wrong -- you want to invert part of this I think (and drop the unnecessary cifs_use_net_ns()):

You're right, I got the test backwards. Thanks.

The reason for the guards is that compiler couldn't tell that current->nsproxy->net_ns always contains the same value, so without a test against a constant allowing it to do dead code elimination, it will generate code to perform the useless test.

Quoted 14 lines
	if (cifs_net_ns(server) != current->nsproxy->net_ns)
		continue;

This is obvious when you note that the context below shows that we 'continue' to the next entry when the addresses don't match:

Quoted 8 lines
fs/cifs/connect.c +1 -0
+
 		if (!match_address(server, addr,
 				   (struct sockaddr *)&vol->srcaddr))
 			continue;

@@ -1572,6 +1576,9 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 		return;
 	}
Quoted 7 lines

fs/cifs/connect.c cifs_put_tcp_session(struct TCP_Server_Info *server) · line 1581

Quoted 3 lines
fs/cifs/connect.c +3 -0
+	if (cifs_use_net_ns())
+		put_net(cifs_net_ns(server));
+

I think this should just be:

	put_net(cifs_net_ns(server));

Hmmm... that one you're probably right, because include/net/net_namespace.h makes put_net() an empty inline, so as long as cifs_net_ns() has no side effects the compiler should be able to drop the whole thing out. (Whether or not it will, I have to test...)

Quoted 7 lines
Quoted 7 lines
fs/cifs/connect.c +0 -0
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 

@@ -1677,6 +1684,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	       sizeof(tcp_ses->srcaddr));
 	++tcp_ses->srv_count;
Quoted 6 lines

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1689

Quoted 3 lines
fs/cifs/connect.c +3 -0
+	if (cifs_use_net_ns())
+		cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
+

Just use cifs_set_net_ns() because it already turns into a no-op when CONFIG_NET_NS=n

no-op yes. no-code, I want to make sure. (I tried it with just the macros the first time through, and scripts/bloat-o-meter kept saying code was being generated. I'll fire up objdump and see if the disassembly tells me anything...)

Quoted 30 lines

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1734

Quoted 9 lines
fs/cifs/connect.c +2 -0
 	if (addr.ss_family == AF_INET6) {
 		cFYI(1, "attempting ipv6 connect");
 		/* BB should we allow ipv6 on port 139? */

@@ -1720,6 +1730,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
 
+	if (cifs_use_net_ns())
+		put_net(cifs_net_ns(tcp_ses));

etc.

Quoted 18 lines
fs/cifs/connect.c +3 -2
+
 out_err:
 	if (tcp_ses) {
 		if (!IS_ERR(tcp_ses->hostname))

@@ -2145,8 +2158,8 @@ ipv4_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
 
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating socket", rc);
 			return rc;

@@ -2310,11 +2323,10 @@ ipv6_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
Quoted 10 lines

fs/cifs/connect.c ipv6_connect(struct TCP_Server_Info *server) · line 2331

Quoted 10 lines
fs/cifs/connect.c +2 -3
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET6, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET6,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating ipv6 socket", rc);
-			socket = NULL;
 			return rc;
 		}

Note, those two add 16 bytes each on x86-64 (two extra 8-byte arguments), but that I couldn't easily stop. I might try to merge the ipv4/ipv6 functions, but that's a separate patch.

Rob

#27 lore raw
In reply to a message not in the archive
Quoted 23 lines · Rob Landley

From: Rob Landley <[email protected]>

Teach cifs about network namespaces, so mounting uses adresses and routing visible from a container rather than from init context.

For a long drawn out test reproduction sequence, see:

http://landley.livejournal.com/47024.html
http://landley.livejournal.com/47205.html
http://landley.livejournal.com/47476.html

Signed-off-by Rob Landley <[email protected]>

---

 fs/cifs/cifsglob.h |   32 ++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   22 +++++++++++++++++-----
 2 files changed, 49 insertions(+), 5 deletions(-)
fs/cifs/cifsglob.h +0 -0
index 7136c0c..86f31bb 100644

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 173

Quoted 6 lines
fs/cifs/cifsglob.h +3 -0
@@ -168,6 +168,9 @@ struct TCP_Server_Info {
 		struct sockaddr_in6 sockAddr6;
 	} addr;
 	struct sockaddr_storage srcaddr; /* locally bind to this IP */
+#ifdef CONFIG_NET_NS
+	struct net *net;
+#endif

I'm assuming this bit is correct -- don't know enough about CIFS to be sure...

Quoted 10 lines
fs/cifs/cifsglob.h +3 -0
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
 	struct list_head pending_mid_q;

@@ -227,6 +230,35 @@ struct TCP_Server_Info {
 };
 
 /*
+ * Macros to allow the TCP_Server_Info->net field and related code to drop out
+ * when CONFIG_NET_NS isn't set.
+ */

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 245

Quoted 10 lines
fs/cifs/cifsglob.h +10 -0
+
+static inline struct net *
+cifs_net_ns(struct TCP_Server_Info *srv)
+{
+#ifdef CONFIG_NET_NS
+	return srv->net;
+#else
+	return &init_net;
+#endif
+}

I thought style dictated we do this a different way:

#ifdef CONFIG_NET_NS
static inline struct net * cifs_net_ns(struct TCP_Server_Info *srv)

{
	return srv->net;
}
<other CONFIG_NET_NS cases>
#else
static inline struct net * cifs_net_ns(struct TCP_Server_Info *srv)
{
	return &init_net;
}

<other no-ops>
#endif /* CONFIG_NET_NS */

Quoted 4 lines
fs/cifs/cifsglob.h +4 -0
+
+static inline void
+cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
+{

fs/cifs/cifsglob.h struct TCP_Server_Info { · line 259

Quoted 10 lines
fs/cifs/cifsglob.h +10 -0
+#ifdef CONFIG_NET_NS
+	srv->net = net;
+#endif
+}
+
+#ifdef CONFIG_NET_NS
+#define cifs_use_net_ns() (1)
+#else
+#define cifs_use_net_ns() (0)
+#endif

This looks wrong -- we shouldn't need this at all. The #ifdef bits in your patch already make all the cases below become empty/no-ops when CONFIG_NET_NS=n.

Quoted 10 lines
fs/cifs/cifsglob.h +2 -0
+
+/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
fs/cifs/connect.c +0 -0
index cc1a860..b4faef0 100644

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1550

Quoted 6 lines
fs/cifs/connect.c +3 -0
@@ -1545,6 +1545,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (cifs_use_net_ns()
+		    && cifs_net_ns(server) == current->nsproxy->net_ns)
+			continue;

This looks wrong -- you want to invert part of this I think (and drop the unnecessary cifs_use_net_ns()):

	if (cifs_net_ns(server) != current->nsproxy->net_ns)
		continue;

This is obvious when you note that the context below shows that we 'continue' to the next entry when the addresses don't match:

fs/cifs/connect.c cifs_put_tcp_session(struct TCP_Server_Info *server) · line 1581

Quoted 11 lines
fs/cifs/connect.c +4 -0
+
 		if (!match_address(server, addr,
 				   (struct sockaddr *)&vol->srcaddr))
 			continue;

@@ -1572,6 +1576,9 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 		return;
 	}
 
+	if (cifs_use_net_ns())
+		put_net(cifs_net_ns(server));
+

I think this should just be:

	put_net(cifs_net_ns(server));

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1689

Quoted 10 lines
fs/cifs/connect.c +3 -0
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 

@@ -1677,6 +1684,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	       sizeof(tcp_ses->srcaddr));
 	++tcp_ses->srv_count;
 
+	if (cifs_use_net_ns())
+		cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
+

Just use cifs_set_net_ns() because it already turns into a no-op when CONFIG_NET_NS=n

fs/cifs/connect.c cifs_get_tcp_session(struct smb_vol *volume_info) · line 1734

Quoted 9 lines
fs/cifs/connect.c +2 -0
 	if (addr.ss_family == AF_INET6) {
 		cFYI(1, "attempting ipv6 connect");
 		/* BB should we allow ipv6 on port 139? */

@@ -1720,6 +1730,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 out_err_crypto_release:
 	cifs_crypto_shash_release(tcp_ses);
 
+	if (cifs_use_net_ns())
+		put_net(cifs_net_ns(tcp_ses));

etc.

Quoted 33 lines
fs/cifs/connect.c +5 -5
+
 out_err:
 	if (tcp_ses) {
 		if (!IS_ERR(tcp_ses->hostname))

@@ -2145,8 +2158,8 @@ ipv4_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
 
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating socket", rc);
 			return rc;

@@ -2310,11 +2323,10 @@ ipv6_connect(struct TCP_Server_Info *server)
 	struct socket *socket = server->ssocket;
 
 	if (socket == NULL) {
-		rc = sock_create_kern(PF_INET6, SOCK_STREAM,
-				      IPPROTO_TCP, &socket);
+		rc = __sock_create(cifs_net_ns(server), PF_INET6,
+				   SOCK_STREAM, IPPROTO_TCP, &socket, 1);
 		if (rc < 0) {
 			cERROR(1, "Error %d creating ipv6 socket", rc);
-			socket = NULL;
 			return rc;
 		}

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

#28 lore raw
In reply to a message not in the archive

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1549

Quoted 6 lines
fs/cifs/connect.c +2 -0
@@ -1545,6 +1545,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (cifs_use_net_ns()
+		    && cifs_net_ns(server) == current->nsproxy->net_ns)

The netns_eq helper will handle that.

#29 lore raw
In reply to a message not in the archive

fs/cifs/connect.c cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol) · line 1549

Quoted 6 lines
fs/cifs/connect.c +2 -0
@@ -1545,6 +1545,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
+		if (cifs_use_net_ns()
+		    && cifs_net_ns(server) == current->nsproxy->net_ns)

The netns_eq helper will handle that.