[PATCH] Teach cifs about network namespaces.

[PATCH] Teach cifs about network namespaces.

[Rob Landley]

From: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>

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 <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
---

 fs/cifs/cifsglob.h |   32 ++++++++++++++++++++++++++++++++
 fs/cifs/connect.c  |   22 +++++++++++++++++-----
 2 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 7136c0c..86f31bb 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -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 {
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index cc1a860..b4faef0 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -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;
 		}

Re: [PATCH] Teach cifs about network namespaces.

[Matt Helsley]

On Mon, Jan 10, 2011 at 10:35:19PM -0600, Rob Landley wrote:
> From: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> 
> 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 <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> ---
> 
>  fs/cifs/cifsglob.h |   32 ++++++++++++++++++++++++++++++++
>  fs/cifs/connect.c  |   22 +++++++++++++++++-----
>  2 files changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 7136c0c..86f31bb 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -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...

>  	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
> +}

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 */

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

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.

> +
> +/*
>   * Session structure.  One of these for each uid session with a particular host
>   */
>  struct cifsSesInfo {
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index cc1a860..b4faef0 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -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:

> +
>  		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));

>  	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

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

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

Re: [PATCH] Teach cifs about network namespaces.

[Rob Landley]

On 01/11/2011 01:12 AM, Matt Helsley wrote:
> On Mon, Jan 10, 2011 at 10:35:19PM -0600, Rob Landley wrote:
>> From: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>>
>> 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 <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>> ---
>>
>>  fs/cifs/cifsglob.h |   32 ++++++++++++++++++++++++++++++++
>>  fs/cifs/connect.c  |   22 +++++++++++++++++-----
>>  2 files changed, 49 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index 7136c0c..86f31bb 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -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...
> 
>>  	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
>> +}
> 
> 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 */

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

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

>> +
>> +/*
>>   * Session structure.  One of these for each uid session with a particular host
>>   */
>>  struct cifsSesInfo {
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index cc1a860..b4faef0 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -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()):

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.

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

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

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

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

>>  	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.
> 
>> +
>>  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;
>>  		}

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

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

[Rob Landley]

From: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>

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

Signed-off-by: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
---

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

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 606ca8b..8175d31 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -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 {
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index a65d311..7dab1d3 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -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;

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

[Jeff Layton]

On Tue, 11 Jan 2011 12:04:54 -0600
Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:

> From: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> 
> Teach cifs about network namespaces, so mounting uses adresses/routing
> visible from the container rather than from init context.
> 
> Signed-off-by: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> ---
> 
> 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(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 606ca8b..8175d31 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -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 {
>  };
>  

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

>  /*
> + * 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 {
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index a65d311..7dab1d3 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -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.

>  		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));
> +

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

>  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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

[Rob Landley]

On 01/11/2011 03:30 PM, Jeff Layton wrote:
> 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 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...

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

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

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

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

[Jeff Layton]

On Wed, 12 Jan 2011 07:57:36 -0600
Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:

> On 01/11/2011 03:30 PM, Jeff Layton wrote:
> > 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.

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

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

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.

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

That seems like the simplest fix.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

[Rob Landley]

From: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>

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

Signed-off-by: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
---

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

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 606ca8b..54cd4ab 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -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 {
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index a65d311..53679f6 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -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;

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

[Jeff Layton]

On Thu, 13 Jan 2011 12:55:04 -0600
Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:

> From: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> 
> Teach cifs about network namespaces, so mounting uses adresses/routing
> visible from the container rather than from init context.
> 
> Signed-off-by: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> ---
> 
> 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 <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

[Rob Landley]

On 01/11/2011 03:30 PM, Jeff Layton wrote:
> On Tue, 11 Jan 2011 12:04:54 -0600
> Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:
> 
>> From: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>>
>> Teach cifs about network namespaces, so mounting uses adresses/routing
>> visible from the container rather than from init context.
>>
>> Signed-off-by: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>> ---
>>
>> 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(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index 606ca8b..8175d31 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -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 {
>>  };
>>  
> 
> 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

Re: [PATCH] Teach cifs about network namespaces.

[Matt Helsley]

On Tue, Jan 11, 2011 at 08:05:38AM -0600, Rob Landley wrote:
> On 01/11/2011 01:12 AM, Matt Helsley wrote:
> > On Mon, Jan 10, 2011 at 10:35:19PM -0600, Rob Landley wrote:
> >> From: Rob Landley <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> >>
> >> 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 <rlandley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> >> ---
> >>
> >>  fs/cifs/cifsglob.h |   32 ++++++++++++++++++++++++++++++++
> >>  fs/cifs/connect.c  |   22 +++++++++++++++++-----
> >>  2 files changed, 49 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >> index 7136c0c..86f31bb 100644
> >> --- a/fs/cifs/cifsglob.h
> >> +++ b/fs/cifs/cifsglob.h

<snip>

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

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

> 
> >> +
> >> +/*
> >>   * Session structure.  One of these for each uid session with a particular host
> >>   */
> >>  struct cifsSesInfo {
> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> index cc1a860..b4faef0 100644
> >> --- a/fs/cifs/connect.c
> >> +++ b/fs/cifs/connect.c
> >> @@ -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()):
> 
> 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.

> 
> > 	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:
> > 
> >> +
> >>  		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));
> 
> 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...)
> 
> >>  	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
> 
> 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>

> >> @@ -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;
> >>  		}
> 
> 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

Re: [PATCH] Teach cifs about network namespaces.

[Pavel Emelyanov]

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