[cifs srcaddr-v4] cifs: Allow binding to local IP address.

[cifs srcaddr-v4] cifs: Allow binding to local IP address.

[Ben Greear]

When using multi-homed machines, it's nice to be able to specify
the local IP to use for outbound connections.  This patch gives
cifs the ability to bind to a particular IP address.

   Usage:  mount -t cifs -o srcaddr=192.168.1.50,user=foo, ...
   Usage:  mount -t cifs -o srcaddr=2002::100:1,user=foo, ...

Signed-off-by: Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
---
:100644 100644 b7431af... 25590d2... M	fs/cifs/cifsfs.c
:100644 100644 c9d0cfc... 784fd4a... M	fs/cifs/cifsglob.h
:100644 100644 ec0ea4a... d3a36b4... M	fs/cifs/connect.c
 fs/cifs/cifsfs.c   |   16 +++++++++
 fs/cifs/cifsglob.h |    1 +
 fs/cifs/connect.c  |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index b7431af..25590d2 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -36,6 +36,7 @@
 #include <linux/kthread.h>
 #include <linux/freezer.h>
 #include <linux/smp_lock.h>
+#include <net/ipv6.h>
 #include "cifsfs.h"
 #include "cifspdu.h"
 #define DECLARE_GLOBALS_HERE
@@ -367,6 +368,8 @@ cifs_show_options(struct seq_file *s, struct vfsmount *m)
 {
 	struct cifs_sb_info *cifs_sb = CIFS_SB(m->mnt_sb);
 	struct cifsTconInfo *tcon = cifs_sb->tcon;
+	struct sockaddr *srcaddr;
+	srcaddr = (struct sockaddr *)(&tcon->ses->server->srcaddr);
 
 	seq_printf(s, ",unc=%s", tcon->treeName);
 	if (tcon->ses->userName)
@@ -374,6 +377,19 @@ cifs_show_options(struct seq_file *s, struct vfsmount *m)
 	if (tcon->ses->domainName)
 		seq_printf(s, ",domain=%s", tcon->ses->domainName);
 
+	if (srcaddr->sa_family != AF_UNSPEC) {
+		struct sockaddr_in *saddr4;
+		struct sockaddr_in6 *saddr6;
+		saddr4 = (struct sockaddr_in *)srcaddr;
+		saddr6 = (struct sockaddr_in6 *)srcaddr;
+		if (saddr6->sin6_family == AF_INET6)
+			seq_printf(s, ",srcaddr=%pI6c",
+				   &saddr6->sin6_addr);
+		else
+			seq_printf(s, ",srcaddr=%pI4",
+				   &saddr4->sin_addr.s_addr);
+	}
+
 	seq_printf(s, ",uid=%d", cifs_sb->mnt_uid);
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_UID)
 		seq_printf(s, ",forceuid");
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index c9d0cfc..784fd4a 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -157,6 +157,7 @@ struct TCP_Server_Info {
 		struct sockaddr_in sockAddr;
 		struct sockaddr_in6 sockAddr6;
 	} addr;
+	struct sockaddr_storage srcaddr; /* locally bind to this IP */
 	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;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index ec0ea4a..d3a36b4 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -105,6 +105,7 @@ struct smb_vol {
 	bool sockopt_tcp_nodelay:1;
 	unsigned short int port;
 	char *prepath;
+	struct sockaddr_storage srcaddr; /* allow binding to a local IP */
 	struct nls_table *local_nls;
 };
 
@@ -1064,6 +1065,22 @@ cifs_parse_mount_options(char *options, const char *devname,
 						    "long\n");
 				return 1;
 			}
+		} else if (strnicmp(data, "srcaddr", 7) == 0) {
+			vol->srcaddr.ss_family = AF_UNSPEC;
+
+			if (!value || !*value) {
+				printk(KERN_WARNING "CIFS: srcaddr value"
+				       " not specified.\n");
+				return 1;	/* needs_arg; */
+			}
+			i = cifs_convert_address((struct sockaddr *)&vol->srcaddr,
+						 value, strlen(value));
+			if (i < 0) {
+				printk(KERN_WARNING "CIFS:  Could not parse"
+				       " srcaddr: %s\n",
+				       value);
+				return 1;
+			}
 		} else if (strnicmp(data, "prefixpath", 10) == 0) {
 			if (!value || !*value) {
 				printk(KERN_WARNING
@@ -1392,8 +1409,36 @@ cifs_parse_mount_options(char *options, const char *devname,
 	return 0;
 }
 
+/** Returns true if srcaddr isn't specified and rhs isn't
+ * specified, or if srcaddr is specified and
+ * matches the IP address of the rhs argument.
+ */
+static bool
+srcip_matches(struct sockaddr *srcaddr, struct sockaddr *rhs)
+{
+	switch (srcaddr->sa_family) {
+	case AF_UNSPEC:
+		return (rhs->sa_family == AF_UNSPEC);
+	case AF_INET: {
+		struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
+		struct sockaddr_in *vaddr4 = (struct sockaddr_in *)rhs;
+		return (saddr4->sin_addr.s_addr == vaddr4->sin_addr.s_addr);
+	}
+	case AF_INET6: {
+		struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
+		struct sockaddr_in6 *vaddr6 = (struct sockaddr_in6 *)&rhs;
+		return ipv6_addr_equal(&saddr6->sin6_addr, &vaddr6->sin6_addr);
+	}
+	default:
+		WARN_ON(1);
+		return false; /* don't expect to be here */
+	}
+}
+
+
 static bool
-match_address(struct TCP_Server_Info *server, struct sockaddr *addr)
+match_address(struct TCP_Server_Info *server, struct sockaddr *addr,
+	      struct sockaddr *srcaddr)
 {
 	struct sockaddr_in *addr4 = (struct sockaddr_in *)addr;
 	struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)addr;
@@ -1420,6 +1465,9 @@ match_address(struct TCP_Server_Info *server, struct sockaddr *addr)
 		break;
 	}
 
+	if (!srcip_matches(srcaddr, (struct sockaddr *)&server->srcaddr))
+		return false;
+
 	return true;
 }
 
@@ -1487,7 +1535,8 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
 		if (server->tcpStatus == CifsNew)
 			continue;
 
-		if (!match_address(server, addr))
+		if (!match_address(server, addr,
+				   (struct sockaddr *)&vol->srcaddr))
 			continue;
 
 		if (!match_security(server, vol))
@@ -1602,6 +1651,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	 * no need to spinlock this init of tcpStatus or srv_count
 	 */
 	tcp_ses->tcpStatus = CifsNew;
+	memcpy(&tcp_ses->srcaddr, &volume_info->srcaddr,
+	       sizeof(tcp_ses->srcaddr));
 	++tcp_ses->srv_count;
 
 	if (addr.ss_family == AF_INET6) {
@@ -2026,6 +2077,33 @@ static void rfc1002mangle(char *target, char *source, unsigned int length)
 
 }
 
+static int
+bind_socket(struct TCP_Server_Info *server)
+{
+	int rc = 0;
+	if (server->srcaddr.ss_family != AF_UNSPEC) {
+		/* Bind to the specified local IP address */
+		struct socket *socket = server->ssocket;
+		rc = socket->ops->bind(socket,
+				       (struct sockaddr *) &server->srcaddr,
+				       sizeof(server->srcaddr));
+		if (rc < 0) {
+			struct sockaddr_in *saddr4;
+			struct sockaddr_in6 *saddr6;
+			saddr4 = (struct sockaddr_in *)&server->srcaddr;
+			saddr6 = (struct sockaddr_in6 *)&server->srcaddr;
+			if (saddr6->sin6_family == AF_INET6)
+				cERROR(1, "cifs: "
+				       "Failed to bind to: %pI6c, error: %d\n",
+				       &saddr6->sin6_addr, rc);
+			else
+				cERROR(1, "cifs: "
+				       "Failed to bind to: %pI4, error: %d\n",
+				       &saddr4->sin_addr.s_addr, rc);
+		}
+	}
+	return rc;
+}
 
 static int
 ipv4_connect(struct TCP_Server_Info *server)
@@ -2051,6 +2129,10 @@ ipv4_connect(struct TCP_Server_Info *server)
 		cifs_reclassify_socket4(socket);
 	}
 
+	rc = bind_socket(server);
+	if (rc < 0)
+		return rc;
+
 	/* user overrode default port */
 	if (server->addr.sockAddr.sin_port) {
 		rc = socket->ops->connect(socket, (struct sockaddr *)
@@ -2213,6 +2295,10 @@ ipv6_connect(struct TCP_Server_Info *server)
 		cifs_reclassify_socket6(socket);
 	}
 
+	rc = bind_socket(server);
+	if (rc < 0)
+		return rc;
+
 	/* user overrode default port */
 	if (server->addr.sockAddr6.sin6_port) {
 		rc = socket->ops->connect(socket,
-- 
1.6.2.5

Re: [cifs srcaddr-v4] cifs: Allow binding to local IP address.

[Jeff Layton]

On Wed,  1 Sep 2010 12:00:06 -0700
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org> wrote:

> When using multi-homed machines, it's nice to be able to specify
> the local IP to use for outbound connections.  This patch gives
> cifs the ability to bind to a particular IP address.
> 
>    Usage:  mount -t cifs -o srcaddr=192.168.1.50,user=foo, ...
>    Usage:  mount -t cifs -o srcaddr=2002::100:1,user=foo, ...
> 
> Signed-off-by: Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
> ---
> :100644 100644 b7431af... 25590d2... M	fs/cifs/cifsfs.c
> :100644 100644 c9d0cfc... 784fd4a... M	fs/cifs/cifsglob.h
> :100644 100644 ec0ea4a... d3a36b4... M	fs/cifs/connect.c
>  fs/cifs/cifsfs.c   |   16 +++++++++
>  fs/cifs/cifsglob.h |    1 +
>  fs/cifs/connect.c  |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index b7431af..25590d2 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -36,6 +36,7 @@
>  #include <linux/kthread.h>
>  #include <linux/freezer.h>
>  #include <linux/smp_lock.h>
> +#include <net/ipv6.h>
>  #include "cifsfs.h"
>  #include "cifspdu.h"
>  #define DECLARE_GLOBALS_HERE
> @@ -367,6 +368,8 @@ cifs_show_options(struct seq_file *s, struct vfsmount *m)
>  {
>  	struct cifs_sb_info *cifs_sb = CIFS_SB(m->mnt_sb);
>  	struct cifsTconInfo *tcon = cifs_sb->tcon;
> +	struct sockaddr *srcaddr;
> +	srcaddr = (struct sockaddr *)(&tcon->ses->server->srcaddr);
					^^^ nit: parens not needed here
>  
>  	seq_printf(s, ",unc=%s", tcon->treeName);
>  	if (tcon->ses->userName)
> @@ -374,6 +377,19 @@ cifs_show_options(struct seq_file *s, struct vfsmount *m)
>  	if (tcon->ses->domainName)
>  		seq_printf(s, ",domain=%s", tcon->ses->domainName);
>  
> +	if (srcaddr->sa_family != AF_UNSPEC) {
> +		struct sockaddr_in *saddr4;
> +		struct sockaddr_in6 *saddr6;
> +		saddr4 = (struct sockaddr_in *)srcaddr;
> +		saddr6 = (struct sockaddr_in6 *)srcaddr;
> +		if (saddr6->sin6_family == AF_INET6)
> +			seq_printf(s, ",srcaddr=%pI6c",
> +				   &saddr6->sin6_addr);
> +		else
> +			seq_printf(s, ",srcaddr=%pI4",
> +				   &saddr4->sin_addr.s_addr);
		^^^ It's unlikely to occur, but maybe better to make
		this a switch() and have a default: case that doesn't
		prints the address as "(unknown)" or something? It's
		usually better to code defensively for this sort of
		stuff and printing a garbage address may be confusing
		for users.

> +	}
> +
>  	seq_printf(s, ",uid=%d", cifs_sb->mnt_uid);
>  	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_UID)
>  		seq_printf(s, ",forceuid");
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index c9d0cfc..784fd4a 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -157,6 +157,7 @@ struct TCP_Server_Info {
>  		struct sockaddr_in sockAddr;
>  		struct sockaddr_in6 sockAddr6;
>  	} addr;
> +	struct sockaddr_storage srcaddr; /* locally bind to this IP */
>  	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;
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index ec0ea4a..d3a36b4 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -105,6 +105,7 @@ struct smb_vol {
>  	bool sockopt_tcp_nodelay:1;
>  	unsigned short int port;
>  	char *prepath;
> +	struct sockaddr_storage srcaddr; /* allow binding to a local IP */
>  	struct nls_table *local_nls;
>  };
>  
> @@ -1064,6 +1065,22 @@ cifs_parse_mount_options(char *options, const char *devname,
>  						    "long\n");
>  				return 1;
>  			}
> +		} else if (strnicmp(data, "srcaddr", 7) == 0) {
> +			vol->srcaddr.ss_family = AF_UNSPEC;
> +
> +			if (!value || !*value) {
> +				printk(KERN_WARNING "CIFS: srcaddr value"
> +				       " not specified.\n");
> +				return 1;	/* needs_arg; */
> +			}
> +			i = cifs_convert_address((struct sockaddr *)&vol->srcaddr,
> +						 value, strlen(value));
> +			if (i < 0) {
> +				printk(KERN_WARNING "CIFS:  Could not parse"
> +				       " srcaddr: %s\n",
> +				       value);
> +				return 1;
> +			}
>  		} else if (strnicmp(data, "prefixpath", 10) == 0) {
>  			if (!value || !*value) {
>  				printk(KERN_WARNING
> @@ -1392,8 +1409,36 @@ cifs_parse_mount_options(char *options, const char *devname,
>  	return 0;
>  }
>  
> +/** Returns true if srcaddr isn't specified and rhs isn't
> + * specified, or if srcaddr is specified and
> + * matches the IP address of the rhs argument.
> + */
> +static bool
> +srcip_matches(struct sockaddr *srcaddr, struct sockaddr *rhs)
> +{
> +	switch (srcaddr->sa_family) {
> +	case AF_UNSPEC:
> +		return (rhs->sa_family == AF_UNSPEC);
> +	case AF_INET: {
> +		struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
> +		struct sockaddr_in *vaddr4 = (struct sockaddr_in *)rhs;
> +		return (saddr4->sin_addr.s_addr == vaddr4->sin_addr.s_addr);
> +	}
> +	case AF_INET6: {
> +		struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
> +		struct sockaddr_in6 *vaddr6 = (struct sockaddr_in6 *)&rhs;
> +		return ipv6_addr_equal(&saddr6->sin6_addr, &vaddr6->sin6_addr);
> +	}
	^^^^^
	These curly braces aren't needed.

> +	default:
> +		WARN_ON(1);

Again, I'm not a huge fan of the cERROR and cFYI macros, but they are
our "standard". This would probably be best as a cERROR macro. You
should probably also have it print the value of srcaddr->sa_family as
that may be useful for debugging.

> +		return false; /* don't expect to be here */
> +	}
> +}

Does the above generate a compiler warning about reaching end of a
non-void function? Either way, it's less clear. I'd change the default
to just fall through and move the "return false" outside the switch.

> +
> +
>  static bool
> -match_address(struct TCP_Server_Info *server, struct sockaddr *addr)
> +match_address(struct TCP_Server_Info *server, struct sockaddr *addr,
> +	      struct sockaddr *srcaddr)
>  {
>  	struct sockaddr_in *addr4 = (struct sockaddr_in *)addr;
>  	struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)addr;
> @@ -1420,6 +1465,9 @@ match_address(struct TCP_Server_Info *server, struct sockaddr *addr)
>  		break;
>  	}
>  
> +	if (!srcip_matches(srcaddr, (struct sockaddr *)&server->srcaddr))
> +		return false;
> +
>  	return true;
>  }
>  
> @@ -1487,7 +1535,8 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
>  		if (server->tcpStatus == CifsNew)
>  			continue;
>  
> -		if (!match_address(server, addr))
> +		if (!match_address(server, addr,
> +				   (struct sockaddr *)&vol->srcaddr))
>  			continue;
>  
>  		if (!match_security(server, vol))
> @@ -1602,6 +1651,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
>  	 * no need to spinlock this init of tcpStatus or srv_count
>  	 */
>  	tcp_ses->tcpStatus = CifsNew;
> +	memcpy(&tcp_ses->srcaddr, &volume_info->srcaddr,
> +	       sizeof(tcp_ses->srcaddr));
>  	++tcp_ses->srv_count;
>  
>  	if (addr.ss_family == AF_INET6) {
> @@ -2026,6 +2077,33 @@ static void rfc1002mangle(char *target, char *source, unsigned int length)
>  
>  }
>  
> +static int
> +bind_socket(struct TCP_Server_Info *server)
> +{
> +	int rc = 0;
> +	if (server->srcaddr.ss_family != AF_UNSPEC) {
> +		/* Bind to the specified local IP address */
> +		struct socket *socket = server->ssocket;
> +		rc = socket->ops->bind(socket,
> +				       (struct sockaddr *) &server->srcaddr,
> +				       sizeof(server->srcaddr));
> +		if (rc < 0) {
> +			struct sockaddr_in *saddr4;
> +			struct sockaddr_in6 *saddr6;
> +			saddr4 = (struct sockaddr_in *)&server->srcaddr;
> +			saddr6 = (struct sockaddr_in6 *)&server->srcaddr;
> +			if (saddr6->sin6_family == AF_INET6)
> +				cERROR(1, "cifs: "
> +				       "Failed to bind to: %pI6c, error: %d\n",
> +				       &saddr6->sin6_addr, rc);
> +			else
> +				cERROR(1, "cifs: "
> +				       "Failed to bind to: %pI4, error: %d\n",
> +				       &saddr4->sin_addr.s_addr, rc);
> +		}
> +	}
> +	return rc;
> +}
>  
>  static int
>  ipv4_connect(struct TCP_Server_Info *server)
> @@ -2051,6 +2129,10 @@ ipv4_connect(struct TCP_Server_Info *server)
>  		cifs_reclassify_socket4(socket);
>  	}
>  
> +	rc = bind_socket(server);
> +	if (rc < 0)
> +		return rc;
> +
>  	/* user overrode default port */
>  	if (server->addr.sockAddr.sin_port) {
>  		rc = socket->ops->connect(socket, (struct sockaddr *)
> @@ -2213,6 +2295,10 @@ ipv6_connect(struct TCP_Server_Info *server)
>  		cifs_reclassify_socket6(socket);
>  	}
>  
> +	rc = bind_socket(server);
> +	if (rc < 0)
> +		return rc;
> +
>  	/* user overrode default port */
>  	if (server->addr.sockAddr6.sin6_port) {
>  		rc = socket->ops->connect(socket,

Nice work so far.
-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [cifs srcaddr-v4] cifs: Allow binding to local IP address.

[Ben Greear]

On 09/01/2010 02:14 PM, Jeff Layton wrote:
> On Wed,  1 Sep 2010 12:00:06 -0700
> Ben Greear<greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>  wrote:

>> +	struct sockaddr *srcaddr;
>> +	srcaddr = (struct sockaddr *)(&tcon->ses->server->srcaddr);
> 					^^^ nit: parens not needed here

Ok, will fix in next patch.

>>
>>   	seq_printf(s, ",unc=%s", tcon->treeName);
>>   	if (tcon->ses->userName)
>> @@ -374,6 +377,19 @@ cifs_show_options(struct seq_file *s, struct vfsmount *m)
>>   	if (tcon->ses->domainName)
>>   		seq_printf(s, ",domain=%s", tcon->ses->domainName);
>>
>> +	if (srcaddr->sa_family != AF_UNSPEC) {
>> +		struct sockaddr_in *saddr4;
>> +		struct sockaddr_in6 *saddr6;
>> +		saddr4 = (struct sockaddr_in *)srcaddr;
>> +		saddr6 = (struct sockaddr_in6 *)srcaddr;
>> +		if (saddr6->sin6_family == AF_INET6)
>> +			seq_printf(s, ",srcaddr=%pI6c",
>> +				&saddr6->sin6_addr);
>> +		else
>> +			seq_printf(s, ",srcaddr=%pI4",
>> +				&saddr4->sin_addr.s_addr);
> 		^^^ It's unlikely to occur, but maybe better to make
> 		this a switch() and have a default: case that doesn't
> 		prints the address as "(unknown)" or something? It's
> 		usually better to code defensively for this sort of
> 		stuff and printing a garbage address may be confusing
> 		for users.

I'll work on that.

>> +/** Returns true if srcaddr isn't specified and rhs isn't
>> + * specified, or if srcaddr is specified and
>> + * matches the IP address of the rhs argument.
>> + */
>> +static bool
>> +srcip_matches(struct sockaddr *srcaddr, struct sockaddr *rhs)
>> +{
>> +	switch (srcaddr->sa_family) {
>> +	case AF_UNSPEC:
>> +		return (rhs->sa_family == AF_UNSPEC);
>> +	case AF_INET: {
>> +		struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
>> +		struct sockaddr_in *vaddr4 = (struct sockaddr_in *)rhs;
>> +		return (saddr4->sin_addr.s_addr == vaddr4->sin_addr.s_addr);
>> +	}
>> +	case AF_INET6: {
>> +		struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
>> +		struct sockaddr_in6 *vaddr6 = (struct sockaddr_in6 *)&rhs;
>> +		return ipv6_addr_equal(&saddr6->sin6_addr,&vaddr6->sin6_addr);
>> +	}
> 	^^^^^
> 	These curly braces aren't needed.

It won't compile without them.  I'd have to declare those variables before the
switch to do away with the parens, and I prefer it as I wrote it.  I'll change
it if you all prefer it otherwise, however.

>
>> +	default:
>> +		WARN_ON(1);
>
> Again, I'm not a huge fan of the cERROR and cFYI macros, but they are
> our "standard". This would probably be best as a cERROR macro. You
> should probably also have it print the value of srcaddr->sa_family as
> that may be useful for debugging.
>
>> +		return false; /* don't expect to be here */
>> +	}
>> +}
>
> Does the above generate a compiler warning about reaching end of a
> non-void function? Either way, it's less clear. I'd change the default
> to just fall through and move the "return false" outside the switch.

No warning, it always returns something since the default case catches
all others.  If I did put the return at the end, then the compiler wouldn't
catch a case where I forgot to return from one of the case statements,
but it's not overly complex code, so I don't care so much either way.
Plz let me know if you still want it at the end.

I also think the WARN_ON is valid, because it can only be a coding bug
that hits that state, and I'd like it to be as loud as possible while
still allowing the user to continue.  There are automated tools that
catch WARN_ON output and post to kernel bug trackers, for instance.

If you still want a cERROR, I can do that..but I prefer to not waste
the space.

Thanks,
Ben


-- 
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
Candela Technologies Inc  http://www.candelatech.com

Re: [cifs srcaddr-v4] cifs: Allow binding to local IP address.

[Jeff Layton]

On Wed, 01 Sep 2010 14:31:00 -0700
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org> wrote:

> On 09/01/2010 02:14 PM, Jeff Layton wrote:
> > On Wed,  1 Sep 2010 12:00:06 -0700
> > Ben Greear<greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>  wrote:
> 
> >> +	struct sockaddr *srcaddr;
> >> +	srcaddr = (struct sockaddr *)(&tcon->ses->server->srcaddr);
> > 					^^^ nit: parens not needed here
> 
> Ok, will fix in next patch.
> 
> >>
> >>   	seq_printf(s, ",unc=%s", tcon->treeName);
> >>   	if (tcon->ses->userName)
> >> @@ -374,6 +377,19 @@ cifs_show_options(struct seq_file *s, struct vfsmount *m)
> >>   	if (tcon->ses->domainName)
> >>   		seq_printf(s, ",domain=%s", tcon->ses->domainName);
> >>
> >> +	if (srcaddr->sa_family != AF_UNSPEC) {
> >> +		struct sockaddr_in *saddr4;
> >> +		struct sockaddr_in6 *saddr6;
> >> +		saddr4 = (struct sockaddr_in *)srcaddr;
> >> +		saddr6 = (struct sockaddr_in6 *)srcaddr;
> >> +		if (saddr6->sin6_family == AF_INET6)
> >> +			seq_printf(s, ",srcaddr=%pI6c",
> >> +				&saddr6->sin6_addr);
> >> +		else
> >> +			seq_printf(s, ",srcaddr=%pI4",
> >> +				&saddr4->sin_addr.s_addr);
> > 		^^^ It's unlikely to occur, but maybe better to make
> > 		this a switch() and have a default: case that doesn't
> > 		prints the address as "(unknown)" or something? It's
> > 		usually better to code defensively for this sort of
> > 		stuff and printing a garbage address may be confusing
> > 		for users.
> 
> I'll work on that.
> 
> >> +/** Returns true if srcaddr isn't specified and rhs isn't
> >> + * specified, or if srcaddr is specified and
> >> + * matches the IP address of the rhs argument.
> >> + */
> >> +static bool
> >> +srcip_matches(struct sockaddr *srcaddr, struct sockaddr *rhs)
> >> +{
> >> +	switch (srcaddr->sa_family) {
> >> +	case AF_UNSPEC:
> >> +		return (rhs->sa_family == AF_UNSPEC);
> >> +	case AF_INET: {
> >> +		struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
> >> +		struct sockaddr_in *vaddr4 = (struct sockaddr_in *)rhs;
> >> +		return (saddr4->sin_addr.s_addr == vaddr4->sin_addr.s_addr);
> >> +	}
> >> +	case AF_INET6: {
> >> +		struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
> >> +		struct sockaddr_in6 *vaddr6 = (struct sockaddr_in6 *)&rhs;
> >> +		return ipv6_addr_equal(&saddr6->sin6_addr,&vaddr6->sin6_addr);
> >> +	}
> > 	^^^^^
> > 	These curly braces aren't needed.
> 
> It won't compile without them.  I'd have to declare those variables before the
> switch to do away with the parens, and I prefer it as I wrote it.  I'll change
> it if you all prefer it otherwise, however.
> 
> >
> >> +	default:
> >> +		WARN_ON(1);
> >
> > Again, I'm not a huge fan of the cERROR and cFYI macros, but they are
> > our "standard". This would probably be best as a cERROR macro. You
> > should probably also have it print the value of srcaddr->sa_family as
> > that may be useful for debugging.
> >
> >> +		return false; /* don't expect to be here */
> >> +	}
> >> +}
> >
> > Does the above generate a compiler warning about reaching end of a
> > non-void function? Either way, it's less clear. I'd change the default
> > to just fall through and move the "return false" outside the switch.
> 
> No warning, it always returns something since the default case catches
> all others.  If I did put the return at the end, then the compiler wouldn't
> catch a case where I forgot to return from one of the case statements,
> but it's not overly complex code, so I don't care so much either way.
> Plz let me know if you still want it at the end.
> 
> I also think the WARN_ON is valid, because it can only be a coding bug
> that hits that state, and I'd like it to be as loud as possible while
> still allowing the user to continue.  There are automated tools that
> catch WARN_ON output and post to kernel bug trackers, for instance.
> 
> If you still want a cERROR, I can do that..but I prefer to not waste
> the space.
> 

It's definitely a coding bug if that fires, but a WARN_ON will mean
nothing to users. It looks scary and is virtually indistinguishable
from an oops. We'll get a stack trace, but it's unlikely to tell us
much.

At that point, you might as well make it a BUG(). At least that way,
we might get a core dump if it fires.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [cifs srcaddr-v4] cifs: Allow binding to local IP address.

[Ben Greear]

On 09/01/2010 05:38 PM, Jeff Layton wrote:

>> No warning, it always returns something since the default case catches
>> all others.  If I did put the return at the end, then the compiler wouldn't
>> catch a case where I forgot to return from one of the case statements,
>> but it's not overly complex code, so I don't care so much either way.
>> Plz let me know if you still want it at the end.
>>
>> I also think the WARN_ON is valid, because it can only be a coding bug
>> that hits that state, and I'd like it to be as loud as possible while
>> still allowing the user to continue.  There are automated tools that
>> catch WARN_ON output and post to kernel bug trackers, for instance.
>>
>> If you still want a cERROR, I can do that..but I prefer to not waste
>> the space.
>>
>
> It's definitely a coding bug if that fires, but a WARN_ON will mean
> nothing to users. It looks scary and is virtually indistinguishable
> from an oops. We'll get a stack trace, but it's unlikely to tell us
> much.
>
> At that point, you might as well make it a BUG(). At least that way,
> we might get a core dump if it fires.

I'd like to wrap this up.  Please let me know exactly what you want there
and I'll make it so.

Thanks,
Ben

-- 
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
Candela Technologies Inc  http://www.candelatech.com

Re: [cifs srcaddr-v4] cifs: Allow binding to local IP address.

[Jeff Layton]

On Wed, 01 Sep 2010 22:07:29 -0700
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org> wrote:

> On 09/01/2010 05:38 PM, Jeff Layton wrote:
> 
> >> No warning, it always returns something since the default case catches
> >> all others.  If I did put the return at the end, then the compiler wouldn't
> >> catch a case where I forgot to return from one of the case statements,
> >> but it's not overly complex code, so I don't care so much either way.
> >> Plz let me know if you still want it at the end.
> >>
> >> I also think the WARN_ON is valid, because it can only be a coding bug
> >> that hits that state, and I'd like it to be as loud as possible while
> >> still allowing the user to continue.  There are automated tools that
> >> catch WARN_ON output and post to kernel bug trackers, for instance.
> >>
> >> If you still want a cERROR, I can do that..but I prefer to not waste
> >> the space.
> >>
> >
> > It's definitely a coding bug if that fires, but a WARN_ON will mean
> > nothing to users. It looks scary and is virtually indistinguishable
> > from an oops. We'll get a stack trace, but it's unlikely to tell us
> > much.
> >
> > At that point, you might as well make it a BUG(). At least that way,
> > we might get a core dump if it fires.
> 
> I'd like to wrap this up.  Please let me know exactly what you want there
> and I'll make it so.
> 

Fair enough. We're splitting hairs at this point. Let's just take this
as-is. For v5 of the patch:

Acked-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [cifs srcaddr-v4] cifs: Allow binding to local IP address.

[Steve French]

On Thu, Sep 2, 2010 at 5:56 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Wed, 01 Sep 2010 22:07:29 -0700
> Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org> wrote:
>
>> On 09/01/2010 05:38 PM, Jeff Layton wrote:
>>
>> >> No warning, it always returns something since the default case catches
>> >> all others.  If I did put the return at the end, then the compiler wouldn't
>> >> catch a case where I forgot to return from one of the case statements,
>> >> but it's not overly complex code, so I don't care so much either way.
>> >> Plz let me know if you still want it at the end.
>> >>
>> >> I also think the WARN_ON is valid, because it can only be a coding bug
>> >> that hits that state, and I'd like it to be as loud as possible while
>> >> still allowing the user to continue.  There are automated tools that
>> >> catch WARN_ON output and post to kernel bug trackers, for instance.
>> >>
>> >> If you still want a cERROR, I can do that..but I prefer to not waste
>> >> the space.
>> >>
>> >
>> > It's definitely a coding bug if that fires, but a WARN_ON will mean
>> > nothing to users. It looks scary and is virtually indistinguishable
>> > from an oops. We'll get a stack trace, but it's unlikely to tell us
>> > much.
>> >
>> > At that point, you might as well make it a BUG(). At least that way,
>> > we might get a core dump if it fires.
>>
>> I'd like to wrap this up.  Please let me know exactly what you want there
>> and I'll make it so.
>>
>
> Fair enough. We're splitting hairs at this point. Let's just take this
> as-is. For v5 of the patch:
>
> Acked-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

I do like this patch as well - although not for rc4 but see no problem
with it in the next merge window - unless there is some other feedback
on IPv6 related syntax that I missed.

-- 
Thanks,

Steve

Re: [cifs srcaddr-v4] cifs: Allow binding to local IP address.

[Ben Greear]

On 09/02/2010 09:02 PM, Steve French wrote:
> On Thu, Sep 2, 2010 at 5:56 AM, Jeff Layton<jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>  wrote:
>> On Wed, 01 Sep 2010 22:07:29 -0700
>> Fair enough. We're splitting hairs at this point. Let's just take this
>> as-is. For v5 of the patch:
>>
>> Acked-by: Jeff Layton<jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>
> I do like this patch as well - although not for rc4 but see no problem
> with it in the next merge window - unless there is some other feedback
> on IPv6 related syntax that I missed.

Next merge window is fine with me.

Thanks for all the reviews and suggestions!

I should have the nfs version posted (to the nfs
list) tomorrow.

Ben

-- 
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
Candela Technologies Inc  http://www.candelatech.com

Re: [cifs srcaddr-v4] cifs: Allow binding to local IP address.

[Steve French]

On Fri, Sep 3, 2010 at 5:21 PM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> Good point - I need to update the smb2 howto ...
>
> If you have Windows 2008, Windows Vista or Windows 7 - smb2 just works (it is the default for current Windows) but for Samba (version 3.5 or later, but current Samba 3.6 server is better and passes Microsoft's (and our) functional tests as a server - the client side in the kernel is farther behind) there is an smb.conf entry to add.   A recent blog entry summarized the server side for Samba:
>
> With Windows 7 Microsoft have introduce a new variant of the CIFS protocol, SMB2. Samba is keeping pace, with a sample implementation of SMB2 which will be released for early adopters to test in Samba 3.5.0. A fully finished production version is expected to be available in Samba 3.6.0. Testing SMB2 in Samba 3.5.0 is as simple as setting :
>
> max protocol = SMB2
>
> in the [global] section of your smb.conf
>
> On the client side, you need smb2.ko from my smb2.git tree (which currently has some changes I need to put in to address some sideeffects of the Google Summer of Code smb2 async write performance improvements) - and if you don't want to use ip addresses (mount -t smb2 //127.0.0.1/someshare /mnt .... etc) then you need to recompile mount.cifs.c as mount.smb2 and put mount.smb2 in /sbin
> The smb2 development tree for the kernel client is at:
> http://git.kernel.org/?p=linux/kernel/git/sfrench/smb2.git;a=summary
> I have 3 patches from Pavel to integrate still and a few other fixes (one for unmount in particular) that need to be posted.  I expect it to be more stable usable by next week (I have been out much of this week)
> On Fri, Sep 3, 2010 at 5:10 PM, Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org> wrote:
>
> >> since SMB2 is more common with newer systems, and in particular
> >> systems more likely to be running IPv6 and thus seem to export more
> >> interfaces...am also interested in an SMB2 version of this (we will be
> >> testing SMB2 extensively in three weeks at the annual Storage
> >> Developer Conference) - smb2.git on kernel.org
> >
> > Is there a howto for configuring an smb2 server and client?
> >
> > I'm very slowly bisecting an O_DIRECT bug for NFS, but might
> > can start working on smb2 while waiting for compiles.
>
>
> --
> Thanks,
>
> Steve
>



--
Thanks,

Steve

Re: [cifs srcaddr-v4] cifs: Allow binding to local IP address.

[Ben Greear]

On 09/03/2010 03:21 PM, Steve French wrote:
> On Fri, Sep 3, 2010 at 5:21 PM, Steve French<smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>  wrote:
>>
>> Good point - I need to update the smb2 howto ...
>>
>> If you have Windows 2008, Windows Vista or Windows 7 - smb2 just works (it is the default for current Windows) but for Samba (version 3.5 or later, but current Samba 3.6 server is better and passes Microsoft's (and our) functional tests as a server - the client side in the kernel is farther behind) there is an smb.conf entry to add.   A recent blog entry summarized the server side for Samba:
>>
>> With Windows 7 Microsoft have introduce a new variant of the CIFS protocol, SMB2. Samba is keeping pace, with a sample implementation of SMB2 which will be released for early adopters to test in Samba 3.5.0. A fully finished production version is expected to be available in Samba 3.6.0. Testing SMB2 in Samba 3.5.0 is as simple as setting :
>>
>> max protocol = SMB2
>>
>> in the [global] section of your smb.conf
>>
>> On the client side, you need smb2.ko from my smb2.git tree (which currently has some changes I need to put in to address some sideeffects of the Google Summer of Code smb2 async write performance improvements) - and if you don't want to use ip addresses (mount -t smb2 //127.0.0.1/someshare /mnt .... etc) then you need to recompile mount.cifs.c as mount.smb2 and put mount.smb2 in /sbin
>> The smb2 development tree for the kernel client is at:
>> http://git.kernel.org/?p=linux/kernel/git/sfrench/smb2.git;a=summary
>> I have 3 patches from Pavel to integrate still and a few other fixes (one for unmount in particular) that need to be posted.  I expect it to be more stable usable by next week (I have been out much of this week)
>> On Fri, Sep 3, 2010 at 5:10 PM, Ben Greear<greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>  wrote:

Ok.  Fedora 13 has smbd version 3.5.4, so maybe that will do the trick.
If not, I should be able to dig up a vista system.

I should be able to get hacking on this next week.

Thanks,
Ben

-- 
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
Candela Technologies Inc  http://www.candelatech.com

smb2 configuration

[Ben Greear]

On 09/03/2010 03:21 PM, Steve French wrote:
> Good point - I need to update the smb2 howto ...
>
>
> If you have Windows 2008, Windows Vista or Windows 7 - smb2 just works
> (it is the default for current Windows) but for Samba (version 3.5 or

Ok, I found a vista machine to test against for now.

> On the client side, you need smb2.ko from my smb2.git tree (which
> currently has some changes I need to put in to address some sideeffects
> of the Google Summer of Code smb2 async write performance improvements)
> - and if you don't want to use ip addresses (mount -t smb2
> //127.0.0.1/someshare <http://127.0.0.1/someshare> /mnt .... etc) then
> you need to recompile mount.cifs.c as mount.smb2 and put mount.smb2 in /sbin

Do I understand correctly that I should be able to use standard mount
so long as I use IP addresses?

What do you mean about recompile mount.cifs.c?  Do you mean rename mount.cifs to
mount.smb2?  Or do I need to hack on mount.cifs.c code?  If I need to do any
re-compiling at all, please point me to the proper upstream repository for
these changes.

It doesn't seem to be working on Fedora 13.  I checked that smb2 module is loaded.

[root@ct503-10G-09 ]# mount -t smb2 -o srcaddr=192.168.100.106,user=lf,passwd=lf //192.168.100.125/pub /mnt/lf/smb2
mount: unknown filesystem type 'smb2'

Thanks,
Ben

-- 
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
Candela Technologies Inc  http://www.candelatech.com

Re: smb2 configuration

[Ben Greear]

On 09/08/2010 10:27 AM, Ben Greear wrote:
> On 09/03/2010 03:21 PM, Steve French wrote:
>> Good point - I need to update the smb2 howto ...
>>
>>
>> If you have Windows 2008, Windows Vista or Windows 7 - smb2 just works
>> (it is the default for current Windows) but for Samba (version 3.5 or
>
> Ok, I found a vista machine to test against for now.
>
>> On the client side, you need smb2.ko from my smb2.git tree (which
>> currently has some changes I need to put in to address some sideeffects
>> of the Google Summer of Code smb2 async write performance improvements)
>> - and if you don't want to use ip addresses (mount -t smb2
>> //127.0.0.1/someshare <http://127.0.0.1/someshare> /mnt .... etc) then
>> you need to recompile mount.cifs.c as mount.smb2 and put mount.smb2 in
>> /sbin
>
> Do I understand correctly that I should be able to use standard mount
> so long as I use IP addresses?
>
> What do you mean about recompile mount.cifs.c? Do you mean rename
> mount.cifs to
> mount.smb2? Or do I need to hack on mount.cifs.c code? If I need to do any
> re-compiling at all, please point me to the proper upstream repository for
> these changes.
>
> It doesn't seem to be working on Fedora 13. I checked that smb2 module
> is loaded.
>
> [root@ct503-10G-09 ]# mount -t smb2 -o
> srcaddr=192.168.100.106,user=lf,passwd=lf //192.168.100.125/pub
> /mnt/lf/smb2
> mount: unknown filesystem type 'smb2'

I tried installing the latest util-linux-ng and cifs-utils,
but still no luck:

[root@ct503-10G-09 mount]# mount -v -t smb2 -o user=lf,pass=lf //192.168.100.125/pub /mnt/lf/smb2
mount.smb2 kernel mount options: ip=192.168.100.125,unc=\\192.168.100.125\pub,,ver=1,user=lf,pass=********
mount error: smb2 filesystem not supported by the system
mount error(19): No such device
Refer to the mount.smb2(8) manual page (e.g. man mount.smb2)

I'm out of ideas on this.

Thanks,
Ben

-- 
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
Candela Technologies Inc  http://www.candelatech.com