[cifs] cifs: Allow binding to local IP address.

[cifs] 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 bindaddr=192.168.1.50,user=foo, ...

Signed-off-by: Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
---
:100644 100644 b7431af... 7466533... M	fs/cifs/cifsfs.c
:100644 100644 c9d0cfc... c0176d8... M	fs/cifs/cifsglob.h
:100644 100644 ec0ea4a... cc0a16a... M	fs/cifs/connect.c
 fs/cifs/cifsfs.c   |    3 +++
 fs/cifs/cifsglob.h |    1 +
 fs/cifs/connect.c  |   51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index b7431af..7466533 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -374,6 +374,9 @@ cifs_show_options(struct seq_file *s, struct vfsmount *m)
 	if (tcon->ses->domainName)
 		seq_printf(s, ",domain=%s", tcon->ses->domainName);
 
+	if (tcon->ses->server->ip4_local_ip)
+		seq_printf(s, ",bindaddr=%pI4", &tcon->ses->server->ip4_local_ip);
+	
 	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..c0176d8 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;
+	u32 ip4_local_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..cc0a16a 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;
+	u32 local_ip; /* allow binding to a local IP address if != 0 */
 	struct nls_table *local_nls;
 };
 
@@ -1064,6 +1065,31 @@ cifs_parse_mount_options(char *options, const char *devname,
 						    "long\n");
 				return 1;
 			}
+		} else if (strnicmp(data, "bindaddr", 8) == 0) {
+			struct sockaddr_storage tmp_laddr;
+			memset(&tmp_laddr, 0, sizeof(tmp_laddr));
+			
+			if (!value || !*value) {
+				printk(KERN_WARNING "CIFS: bindaddr value not specified.\n");
+				return 1;	/* needs_arg; */
+			}
+			i = cifs_convert_address((struct sockaddr*)(&tmp_laddr), value, strlen(value));
+			if (i < 0) {
+				vol->local_ip = 0;
+				printk(KERN_WARNING "CIFS:  Could not parse bindaddr: %s\n",
+				       value);
+				return 1;
+			}
+			else {
+				struct sockaddr_in *s4 = (struct sockaddr_in *) &tmp_laddr;
+				//struct sockaddr_in6 *s6 = (struct sockaddr_in6 *) &tmp_laddr;
+				if (s4->sin_family == AF_INET) {
+					vol->local_ip = s4->sin_addr.s_addr;
+				}
+				else {
+					printk("WARNING:  IPv6 bindaddr not supported yet.\n");
+				}
+			}
 		} else if (strnicmp(data, "prefixpath", 10) == 0) {
 			if (!value || !*value) {
 				printk(KERN_WARNING
@@ -1393,7 +1419,7 @@ cifs_parse_mount_options(char *options, const char *devname,
 }
 
 static bool
-match_address(struct TCP_Server_Info *server, struct sockaddr *addr)
+match_address(struct TCP_Server_Info *server, struct sockaddr *addr, u32 local_ip4)
 {
 	struct sockaddr_in *addr4 = (struct sockaddr_in *)addr;
 	struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)addr;
@@ -1406,6 +1432,8 @@ match_address(struct TCP_Server_Info *server, struct sockaddr *addr)
 		if (addr4->sin_port &&
 		    addr4->sin_port != server->addr.sockAddr.sin_port)
 			return false;
+		if (local_ip4 && (local_ip4 != server->ip4_local_ip))
+			return false;
 		break;
 	case AF_INET6:
 		if (!ipv6_addr_equal(&addr6->sin6_addr,
@@ -1487,7 +1515,7 @@ 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, vol->local_ip))
 			continue;
 
 		if (!match_security(server, vol))
@@ -1602,6 +1630,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	 * no need to spinlock this init of tcpStatus or srv_count
 	 */
 	tcp_ses->tcpStatus = CifsNew;
+	tcp_ses->ip4_local_ip = volume_info->local_ip;
 	++tcp_ses->srv_count;
 
 	if (addr.ss_family == AF_INET6) {
@@ -1678,6 +1707,10 @@ cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb_vol *vol)
 				    vol->password ? vol->password : "",
 				    MAX_PASSWORD_SIZE))
 				continue;
+			if (server->ip4_local_ip &&
+			    (server->ip4_local_ip != ses->server->ip4_local_ip))
+				continue;
+			/* TODO:  Deal with IPv6 local addr matching? --Ben */
 		}
 		++ses->ses_count;
 		write_unlock(&cifs_tcp_ses_lock);
@@ -2051,6 +2084,20 @@ ipv4_connect(struct TCP_Server_Info *server)
 		cifs_reclassify_socket4(socket);
 	}
 
+	/* Bind to the local IP address if specified */
+	if (server->ip4_local_ip) {
+		struct sockaddr_in myaddr = {
+			.sin_family = AF_INET,
+		};
+		myaddr.sin_addr.s_addr = server->ip4_local_ip;
+		myaddr.sin_port = 0; /* any */
+		rc = socket->ops->bind(socket, (struct sockaddr *) &myaddr,
+				       sizeof(myaddr));
+		if (rc < 0)
+			cERROR(1, "Failed to bind to: %pI4, error: %d\n",
+			       &server->ip4_local_ip, rc);
+	}
+
 	/* user overrode default port */
 	if (server->addr.sockAddr.sin_port) {
 		rc = socket->ops->connect(socket, (struct sockaddr *)
-- 
1.6.2.5

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

[Steve French]

On Wed, Aug 25, 2010 at 6:00 PM, 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 bindaddr=192.168.1.50,user=foo, ...

Seems like a reasonable idea, but I have a few questions.
Is there a similar named mount option for other network
file systems on other operating systems?

Also don't IPv6 address strings already allow you to specify the local
interface to use?

-- 
Thanks,

Steve

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

[Ben Greear]

On 08/25/2010 05:35 PM, Steve French wrote:
> On Wed, Aug 25, 2010 at 6:00 PM, 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 bindaddr=192.168.1.50,user=foo, ...
>
> Seems like a reasonable idea, but I have a few questions.
> Is there a similar named mount option for other network
> file systems on other operating systems?

I have patches for NFS as well, but they are a bit more complex,
so I was going to start with cifs.

It *seems* that bindaddr is sort of half way expected to work,
for nfs, (but does not on linux, last I checked, without my additional patches).

I've never tried this on any other platform.

> Also don't IPv6 address strings already allow you to specify the local
> interface to use?

I think you still have to do the bind() call somewhere, and I don't
think that is happening in the current cifs code.

If/when I can get the ipv4 stuff in acceptable shape for inclusion,
I'll work on ipv6 (it shouldn't be much additional work).

Thanks,
Ben

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

[cifs] 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... 4510f4d... M	fs/cifs/cifsfs.c
:100644 100644 d82f5fb... d6f0a7d... M	fs/cifs/cifsfs.h
:100644 100644 c9d0cfc... 784fd4a... M	fs/cifs/cifsglob.h
:100644 100644 ec0ea4a... 9a3175b... M	fs/cifs/connect.c
:100644 100644 ab70a3f... 735989a... M	net/ipv6/addrconf.c
 fs/cifs/cifsfs.c    |   32 ++++++++++++++++++
 fs/cifs/cifsfs.h    |    3 ++
 fs/cifs/cifsglob.h  |    1 +
 fs/cifs/connect.c   |   91 +++++++++++++++++++++++++++++++++++++++++++++++++-
 net/ipv6/addrconf.c |    1 +
 5 files changed, 126 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index b7431af..4510f4d 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -357,6 +357,23 @@ cifs_show_address(struct seq_file *s, struct TCP_Server_Info *server)
 	}
 }
 
+bool
+cifs_addr_is_specified(struct sockaddr *srcaddr) {
+	struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
+	struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
+	switch (srcaddr->sa_family) {
+	case AF_INET:
+		return saddr4->sin_addr.s_addr != 0;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+	case AF_INET6:
+		/* in6addr_any isn't available if IPv6 isn't compiled in */
+		return (memcmp(&in6addr_any, &saddr6->sin6_addr,
+			       sizeof(in6addr_any)) != 0);
+#endif
+	}
+	return false;
+}
+
 /*
  * cifs_show_options() is for displaying mount options in /proc/mounts.
  * Not all settable options are displayed but most of the important
@@ -367,6 +384,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 +393,19 @@ cifs_show_options(struct seq_file *s, struct vfsmount *m)
 	if (tcon->ses->domainName)
 		seq_printf(s, ",domain=%s", tcon->ses->domainName);
 
+	if (cifs_addr_is_specified(srcaddr)) {
+		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/cifsfs.h b/fs/cifs/cifsfs.h
index d82f5fb..d6f0a7d 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -110,6 +110,9 @@ extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
 extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
 extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
 
+struct sockaddr;
+extern bool cifs_addr_is_specified(struct sockaddr *srcaddr);
+
 #ifdef CONFIG_CIFS_EXPERIMENTAL
 extern const struct export_operations cifs_export_ops;
 #endif /* EXPERIMENTAL */
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..9a3175b 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -49,6 +49,7 @@
 #include "rfc1002pdu.h"
 #include "cn_cifs.h"
 #include "fscache.h"
+#include "cifsfs.h"
 
 #define CIFS_PORT 445
 #define RFC1001_PORT 139
@@ -105,6 +106,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 +1066,22 @@ cifs_parse_mount_options(char *options, const char *devname,
 						    "long\n");
 				return 1;
 			}
+		} else if (strnicmp(data, "srcaddr", 8) == 0) {
+			memset(&vol->srcaddr, 0, sizeof(vol->srcaddr));
+
+			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 +1410,37 @@ cifs_parse_mount_options(char *options, const char *devname,
 	return 0;
 }
 
+/** Returns true if srcaddr isn't specified or if it matches
+ * the IP address of the rhs argument.
+ */
 static bool
-match_address(struct TCP_Server_Info *server, struct sockaddr *addr)
+srcip_matches(struct sockaddr *srcaddr, struct sockaddr *rhs)
+{
+	if (cifs_addr_is_specified(srcaddr)) {
+		struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
+		struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
+		struct sockaddr_in *vaddr4 = (struct sockaddr_in *)rhs;
+		struct sockaddr_in6 *vaddr6 = (struct sockaddr_in6 *)&rhs;
+
+		switch (srcaddr->sa_family) {
+		case AF_INET:
+			if (saddr4->sin_addr.s_addr != vaddr4->sin_addr.s_addr)
+				return false;
+			break;
+		case AF_INET6:
+			if (memcmp(&saddr6->sin6_addr, &vaddr6->sin6_addr,
+				   sizeof(saddr6->sin6_addr)) != 0)
+				return false;
+			break;
+		}
+	}
+	return true;
+}
+
+
+static bool
+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 +1467,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 +1537,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 +1653,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) {
@@ -1678,6 +1731,9 @@ cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb_vol *vol)
 				    vol->password ? vol->password : "",
 				    MAX_PASSWORD_SIZE))
 				continue;
+			if (!srcip_matches((struct sockaddr *)&server->srcaddr,
+					   (struct sockaddr *)&ses->server->srcaddr))
+				continue;
 		}
 		++ses->ses_count;
 		write_unlock(&cifs_tcp_ses_lock);
@@ -2026,6 +2082,33 @@ static void rfc1002mangle(char *target, char *source, unsigned int length)
 
 }
 
+static int
+bind_socket(struct TCP_Server_Info *server)
+{
+	int rc = 0;
+	if (cifs_addr_is_specified((struct sockaddr *)&server->srcaddr)) {
+		/* Bind to the local IP address if specified */
+		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)
+				printk(KERN_WARNING "cifs: "
+				       "Failed to bind to: %pI6c, error: %d\n",
+				       &saddr6->sin6_addr, rc);
+			else
+				printk(KERN_WARNING "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 +2134,8 @@ ipv4_connect(struct TCP_Server_Info *server)
 		cifs_reclassify_socket4(socket);
 	}
 
+	bind_socket(server);
+
 	/* user overrode default port */
 	if (server->addr.sockAddr.sin_port) {
 		rc = socket->ops->connect(socket, (struct sockaddr *)
@@ -2213,6 +2298,8 @@ ipv6_connect(struct TCP_Server_Info *server)
 		cifs_reclassify_socket6(socket);
 	}
 
+	bind_socket(server);
+
 	/* user overrode default port */
 	if (server->addr.sockAddr6.sin6_port) {
 		rc = socket->ops->connect(socket,
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ab70a3f..735989a 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -230,6 +230,7 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
 
 /* IPv6 Wildcard Address and Loopback Address defined by RFC2553 */
 const struct in6_addr in6addr_any = IN6ADDR_ANY_INIT;
+EXPORT_SYMBOL(in6addr_any);
 const struct in6_addr in6addr_loopback = IN6ADDR_LOOPBACK_INIT;
 const struct in6_addr in6addr_linklocal_allnodes = IN6ADDR_LINKLOCAL_ALLNODES_INIT;
 const struct in6_addr in6addr_linklocal_allrouters = IN6ADDR_LINKLOCAL_ALLROUTERS_INIT;
-- 
1.6.2.5

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

[Ben Greear]

On 08/30/2010 10:40 PM, Ben Greear 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, ...

I tested IPv6 and IPv4 and it seems to work as designed.

checkpatch warns of a few long lines, but I saw no way to
break them up w/out adding even worse (IMO) cruft.  But,
I can change that if it's important.

Please let me know if any other changes are needed.

Thanks,
Ben

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

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

[Jeff Layton]

On Mon, 30 Aug 2010 22:40:22 -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... 4510f4d... M	fs/cifs/cifsfs.c
> :100644 100644 d82f5fb... d6f0a7d... M	fs/cifs/cifsfs.h
> :100644 100644 c9d0cfc... 784fd4a... M	fs/cifs/cifsglob.h
> :100644 100644 ec0ea4a... 9a3175b... M	fs/cifs/connect.c
> :100644 100644 ab70a3f... 735989a... M	net/ipv6/addrconf.c
>  fs/cifs/cifsfs.c    |   32 ++++++++++++++++++
>  fs/cifs/cifsfs.h    |    3 ++
>  fs/cifs/cifsglob.h  |    1 +
>  fs/cifs/connect.c   |   91 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  net/ipv6/addrconf.c |    1 +
>  5 files changed, 126 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index b7431af..4510f4d 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -357,6 +357,23 @@ cifs_show_address(struct seq_file *s, struct TCP_Server_Info *server)
>  	}
>  }
>  
> +bool
> +cifs_addr_is_specified(struct sockaddr *srcaddr) {
> +	struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
> +	struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
> +	switch (srcaddr->sa_family) {
> +	case AF_INET:
> +		return saddr4->sin_addr.s_addr != 0;
						^^^^
					nit: should probably be htonl(INADDR_ANY)
> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> +	case AF_INET6:
> +		/* in6addr_any isn't available if IPv6 isn't compiled in */
> +		return (memcmp(&in6addr_any, &saddr6->sin6_addr,
> +			       sizeof(in6addr_any)) != 0);
> +#endif
> +	}
> +	return false;
> +}
> +
>  /*
>   * cifs_show_options() is for displaying mount options in /proc/mounts.
>   * Not all settable options are displayed but most of the important
> @@ -367,6 +384,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 +393,19 @@ cifs_show_options(struct seq_file *s, struct vfsmount *m)
>  	if (tcon->ses->domainName)
>  		seq_printf(s, ",domain=%s", tcon->ses->domainName);
>  
> +	if (cifs_addr_is_specified(srcaddr)) {
> +		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/cifsfs.h b/fs/cifs/cifsfs.h
> index d82f5fb..d6f0a7d 100644
> --- a/fs/cifs/cifsfs.h
> +++ b/fs/cifs/cifsfs.h
> @@ -110,6 +110,9 @@ extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
>  extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
>  extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
>  
> +struct sockaddr;
> +extern bool cifs_addr_is_specified(struct sockaddr *srcaddr);
> +
>  #ifdef CONFIG_CIFS_EXPERIMENTAL
>  extern const struct export_operations cifs_export_ops;
>  #endif /* EXPERIMENTAL */
> 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..9a3175b 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -49,6 +49,7 @@
>  #include "rfc1002pdu.h"
>  #include "cn_cifs.h"
>  #include "fscache.h"
> +#include "cifsfs.h"
>  
>  #define CIFS_PORT 445
>  #define RFC1001_PORT 139
> @@ -105,6 +106,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 +1066,22 @@ cifs_parse_mount_options(char *options, const char *devname,
>  						    "long\n");
>  				return 1;
>  			}
> +		} else if (strnicmp(data, "srcaddr", 8) == 0) {
> +			memset(&vol->srcaddr, 0, sizeof(vol->srcaddr));
			^^^^^^^^^
			Is this necessary? vol is kzalloc'ed so it
			should be zeroed out already. Or are you
			worried about people specifying srcaddr= more
			than once?

> +
> +			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 +1410,37 @@ cifs_parse_mount_options(char *options, const char *devname,
>  	return 0;
>  }
>  
> +/** Returns true if srcaddr isn't specified or if it matches
> + * the IP address of the rhs argument.
> + */
>  static bool
> -match_address(struct TCP_Server_Info *server, struct sockaddr *addr)
> +srcip_matches(struct sockaddr *srcaddr, struct sockaddr *rhs)
> +{
> +	if (cifs_addr_is_specified(srcaddr)) {

	Just wondering what the expected behavior would be here...

	Suppose I have a clustered IP address set up, and I want to
	ensure that one of my CIFS mounts use that as the srcaddr. Now,
	suppose I make another mount to the same server, but don't
	specify the srcaddr there. That mount uses the same socket as
	the first one (that is bound to the srcaddr). Now, suppose I
	unmount the first mount and then fail over the IP address. What
	happens to the second one? (Nothing good, I'd wager...)

	Perhaps we should consider using a different socket when the
	srcaddr isn't specified?

> +		struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
> +		struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
> +		struct sockaddr_in *vaddr4 = (struct sockaddr_in *)rhs;
> +		struct sockaddr_in6 *vaddr6 = (struct sockaddr_in6 *)&rhs;
> +
> +		switch (srcaddr->sa_family) {
> +		case AF_INET:
> +			if (saddr4->sin_addr.s_addr != vaddr4->sin_addr.s_addr)
> +				return false;
> +			break;
> +		case AF_INET6:
> +			if (memcmp(&saddr6->sin6_addr, &vaddr6->sin6_addr,
> +				   sizeof(saddr6->sin6_addr)) != 0)

			^^^ should use ipv6_addr_equal, unless there's some reason not to do so...

> +				return false;
> +			break;
> +		}
> +	}
> +	return true;
> +}
> +
> +
> +static bool
> +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 +1467,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 +1537,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 +1653,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) {
> @@ -1678,6 +1731,9 @@ cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb_vol *vol)
>  				    vol->password ? vol->password : "",
>  				    MAX_PASSWORD_SIZE))
>  				continue;
> +			if (!srcip_matches((struct sockaddr *)&server->srcaddr,
> +					   (struct sockaddr *)&ses->server->srcaddr))
> +				continue;

			^^^ there should be no need for this. By the
			time you get here, you've already determined
			whether you can use the same socket as another
			one (via the check in match_address).

>  		}
>  		++ses->ses_count;
>  		write_unlock(&cifs_tcp_ses_lock);
> @@ -2026,6 +2082,33 @@ static void rfc1002mangle(char *target, char *source, unsigned int length)
>  
>  }
>  
> +static int
> +bind_socket(struct TCP_Server_Info *server)
> +{
> +	int rc = 0;
> +	if (cifs_addr_is_specified((struct sockaddr *)&server->srcaddr)) {
> +		/* Bind to the local IP address if specified */
> +		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)
> +				printk(KERN_WARNING "cifs: "
> +				       "Failed to bind to: %pI6c, error: %d\n",
> +				       &saddr6->sin6_addr, rc);
> +			else
> +				printk(KERN_WARNING "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 +2134,8 @@ ipv4_connect(struct TCP_Server_Info *server)
>  		cifs_reclassify_socket4(socket);
>  	}
>  
> +	bind_socket(server);
> +
>  	/* user overrode default port */
>  	if (server->addr.sockAddr.sin_port) {
>  		rc = socket->ops->connect(socket, (struct sockaddr *)
> @@ -2213,6 +2298,8 @@ ipv6_connect(struct TCP_Server_Info *server)
>  		cifs_reclassify_socket6(socket);
>  	}
>  
> +	bind_socket(server);
> +
>  	/* user overrode default port */
>  	if (server->addr.sockAddr6.sin6_port) {
>  		rc = socket->ops->connect(socket,
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index ab70a3f..735989a 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -230,6 +230,7 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
>  
>  /* IPv6 Wildcard Address and Loopback Address defined by RFC2553 */
>  const struct in6_addr in6addr_any = IN6ADDR_ANY_INIT;
> +EXPORT_SYMBOL(in6addr_any);

There should be no need at all to touch stuff outside of cifs.ko, at
least, not in this patch. If you want to do this, you need to do it as
a separate patch and float it in linux-netdev or someplace like that.

In the meantime, I'd suggest declaring a private in6_addr variable and
initializing it to IN6ADDR_ANY_INIT (similar to what the sunrpc code
does).

>  const struct in6_addr in6addr_loopback = IN6ADDR_LOOPBACK_INIT;
>  const struct in6_addr in6addr_linklocal_allnodes = IN6ADDR_LINKLOCAL_ALLNODES_INIT;
>  const struct in6_addr in6addr_linklocal_allrouters = IN6ADDR_LINKLOCAL_ALLROUTERS_INIT;


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

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

[Ben Greear]

On 08/31/2010 07:06 AM, Jeff Layton wrote:
> On Mon, 30 Aug 2010 22:40:22 -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, ...

>> +bool
>> +cifs_addr_is_specified(struct sockaddr *srcaddr) {
>> +	struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
>> +	struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
>> +	switch (srcaddr->sa_family) {
>> +	case AF_INET:
>> +		return saddr4->sin_addr.s_addr != 0;
> 						^^^^
> 					nit: should probably be htonl(INADDR_ANY)

We don't initialize it to INADDR_ANY, so I'd rather not check
against it, even though now and forever it's going to be zero.
But, will change if that's the consensus.

>> @@ -1064,6 +1066,22 @@ cifs_parse_mount_options(char *options, const char *devname,
>>   						    "long\n");
>>   				return 1;
>>   			}
>> +		} else if (strnicmp(data, "srcaddr", 8) == 0) {
>> +			memset(&vol->srcaddr, 0, sizeof(vol->srcaddr));
> 			^^^^^^^^^
> 			Is this necessary? vol is kzalloc'ed so it
> 			should be zeroed out already. Or are you
> 			worried about people specifying srcaddr= more
> 			than once?

It just seems cleaner in general to set it to known value in case errors
later in that method keep us from setting it to a proper new value.
And, certainly useful if it can be set twice somehow..but not sure if that's
currently the case or not.

>
>> +
>> +			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 +1410,37 @@ cifs_parse_mount_options(char *options, const char *devname,
>>   	return 0;
>>   }
>>
>> +/** Returns true if srcaddr isn't specified or if it matches
>> + * the IP address of the rhs argument.
>> + */
>>   static bool
>> -match_address(struct TCP_Server_Info *server, struct sockaddr *addr)
>> +srcip_matches(struct sockaddr *srcaddr, struct sockaddr *rhs)
>> +{
>> +	if (cifs_addr_is_specified(srcaddr)) {
>
> 	Just wondering what the expected behavior would be here...
>
> 	Suppose I have a clustered IP address set up, and I want to
> 	ensure that one of my CIFS mounts use that as the srcaddr. Now,
> 	suppose I make another mount to the same server, but don't
> 	specify the srcaddr there. That mount uses the same socket as
> 	the first one (that is bound to the srcaddr). Now, suppose I
> 	unmount the first mount and then fail over the IP address. What
> 	happens to the second one? (Nothing good, I'd wager...)
>
> 	Perhaps we should consider using a different socket when the
> 	srcaddr isn't specified?

The idea is that each mount with unique source addr gets it's own
cifs session.  They will not be shared even if the server is the
same.  If srcaddr isn't specified, then you get today's behaviour
(shared so long as server is the same).

In your example, the bound and the un-bound shouldn't share anything,
but it's possible my code doesn't fully do that.  If so, I would
consider that a bug.

>> +		case AF_INET6:
>> +			if (memcmp(&saddr6->sin6_addr,&vaddr6->sin6_addr,
>> +				   sizeof(saddr6->sin6_addr)) != 0)
>
> 			^^^ should use ipv6_addr_equal, unless there's some reason not to do so...

addr_equal should be fine, I will change that.

>
>> +				return false;
>> +			break;
>> +		}
>> +	}
>> +	return true;
>> +}
>> +
>> +
>> +static bool
>> +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 +1467,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 +1537,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 +1653,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) {
>> @@ -1678,6 +1731,9 @@ cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb_vol *vol)
>>   				    vol->password ? vol->password : "",
>>   				    MAX_PASSWORD_SIZE))
>>   				continue;
>> +			if (!srcip_matches((struct sockaddr *)&server->srcaddr,
>> +					   (struct sockaddr *)&ses->server->srcaddr))
>> +				continue;
>
> 			^^^ there should be no need for this. By the
> 			time you get here, you've already determined
> 			whether you can use the same socket as another
> 			one (via the check in match_address).

Ok, I'll remove that.

>> index ab70a3f..735989a 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -230,6 +230,7 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
>>
>>   /* IPv6 Wildcard Address and Loopback Address defined by RFC2553 */
>>   const struct in6_addr in6addr_any = IN6ADDR_ANY_INIT;
>> +EXPORT_SYMBOL(in6addr_any);
>
> There should be no need at all to touch stuff outside of cifs.ko, at
> least, not in this patch. If you want to do this, you need to do it as
> a separate patch and float it in linux-netdev or someplace like that.
>
> In the meantime, I'd suggest declaring a private in6_addr variable and
> initializing it to IN6ADDR_ANY_INIT (similar to what the sunrpc code
> does).

Sounds good.  I'll see if davem will accept such a patch, and will use
a private copy in cifs until such time as the ipv6 patch goes in.

Thanks for the review!

Thanks,
Ben


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

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

[Jeff Layton]

On Tue, 31 Aug 2010 09:44:39 -0700
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org> wrote:

> On 08/31/2010 07:06 AM, Jeff Layton wrote:
> > On Mon, 30 Aug 2010 22:40:22 -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, ...
> 
> >> +bool
> >> +cifs_addr_is_specified(struct sockaddr *srcaddr) {
> >> +	struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
> >> +	struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
> >> +	switch (srcaddr->sa_family) {
> >> +	case AF_INET:
> >> +		return saddr4->sin_addr.s_addr != 0;
> > 						^^^^
> > 					nit: should probably be htonl(INADDR_ANY)
> 
> We don't initialize it to INADDR_ANY, so I'd rather not check
> against it, even though now and forever it's going to be zero.
> But, will change if that's the consensus.
> 

What may be best instead is to just set sa_family to AF_UNSPEC when
it's not specified. An sa_family that was AF_INET or AF_INET6 would
imply that it had been specified. That way someone could reasonably
force a ->bind to occur to INADDR_ANY (as silly as that sounds).

> >> @@ -1064,6 +1066,22 @@ cifs_parse_mount_options(char *options, const char *devname,
> >>   						    "long\n");
> >>   				return 1;
> >>   			}
> >> +		} else if (strnicmp(data, "srcaddr", 8) == 0) {
> >> +			memset(&vol->srcaddr, 0, sizeof(vol->srcaddr));
> > 			^^^^^^^^^
> > 			Is this necessary? vol is kzalloc'ed so it
> > 			should be zeroed out already. Or are you
> > 			worried about people specifying srcaddr= more
> > 			than once?
> 
> It just seems cleaner in general to set it to known value in case errors
> later in that method keep us from setting it to a proper new value.
> And, certainly useful if it can be set twice somehow..but not sure if that's
> currently the case or not.
> 

Fair enough. There's probably no need to zero this out entirely though.
Setting the family to AF_UNSPEC should be sufficient.

> >
> >> +
> >> +			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 +1410,37 @@ cifs_parse_mount_options(char *options, const char *devname,
> >>   	return 0;
> >>   }
> >>
> >> +/** Returns true if srcaddr isn't specified or if it matches
> >> + * the IP address of the rhs argument.
> >> + */
> >>   static bool
> >> -match_address(struct TCP_Server_Info *server, struct sockaddr *addr)
> >> +srcip_matches(struct sockaddr *srcaddr, struct sockaddr *rhs)
> >> +{
> >> +	if (cifs_addr_is_specified(srcaddr)) {
> >
> > 	Just wondering what the expected behavior would be here...
> >
> > 	Suppose I have a clustered IP address set up, and I want to
> > 	ensure that one of my CIFS mounts use that as the srcaddr. Now,
> > 	suppose I make another mount to the same server, but don't
> > 	specify the srcaddr there. That mount uses the same socket as
> > 	the first one (that is bound to the srcaddr). Now, suppose I
> > 	unmount the first mount and then fail over the IP address. What
> > 	happens to the second one? (Nothing good, I'd wager...)
> >
> > 	Perhaps we should consider using a different socket when the
> > 	srcaddr isn't specified?
> 
> The idea is that each mount with unique source addr gets it's own
> cifs session.  They will not be shared even if the server is the
> same.  If srcaddr isn't specified, then you get today's behaviour
> (shared so long as server is the same).
> 
> In your example, the bound and the un-bound shouldn't share anything,
> but it's possible my code doesn't fully do that.  If so, I would
> consider that a bug.
> 

Ok, I think that is a bug here. srcip_matches returns true when
cifs_addr_is_specified returns false. So if you don't specify a
srcaddr= option, you'll match any session, right?

> >> +		case AF_INET6:
> >> +			if (memcmp(&saddr6->sin6_addr,&vaddr6->sin6_addr,
> >> +				   sizeof(saddr6->sin6_addr)) != 0)
> >
> > 			^^^ should use ipv6_addr_equal, unless there's some reason not to do so...
> 
> addr_equal should be fine, I will change that.
> 
> >
> >> +				return false;
> >> +			break;
> >> +		}
> >> +	}
> >> +	return true;
> >> +}
> >> +
> >> +
> >> +static bool
> >> +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 +1467,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 +1537,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 +1653,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) {
> >> @@ -1678,6 +1731,9 @@ cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb_vol *vol)
> >>   				    vol->password ? vol->password : "",
> >>   				    MAX_PASSWORD_SIZE))
> >>   				continue;
> >> +			if (!srcip_matches((struct sockaddr *)&server->srcaddr,
> >> +					   (struct sockaddr *)&ses->server->srcaddr))
> >> +				continue;
> >
> > 			^^^ there should be no need for this. By the
> > 			time you get here, you've already determined
> > 			whether you can use the same socket as another
> > 			one (via the check in match_address).
> 
> Ok, I'll remove that.
> 
> >> index ab70a3f..735989a 100644
> >> --- a/net/ipv6/addrconf.c
> >> +++ b/net/ipv6/addrconf.c
> >> @@ -230,6 +230,7 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
> >>
> >>   /* IPv6 Wildcard Address and Loopback Address defined by RFC2553 */
> >>   const struct in6_addr in6addr_any = IN6ADDR_ANY_INIT;
> >> +EXPORT_SYMBOL(in6addr_any);
> >
> > There should be no need at all to touch stuff outside of cifs.ko, at
> > least, not in this patch. If you want to do this, you need to do it as
> > a separate patch and float it in linux-netdev or someplace like that.
> >
> > In the meantime, I'd suggest declaring a private in6_addr variable and
> > initializing it to IN6ADDR_ANY_INIT (similar to what the sunrpc code
> > does).
> 
> Sounds good.  I'll see if davem will accept such a patch, and will use
> a private copy in cifs until such time as the ipv6 patch goes in.
> 
> Thanks for the review!
> 

No problem.

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

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

[Ben Greear]

On 08/31/2010 11:50 AM, Jeff Layton wrote:
> On Tue, 31 Aug 2010 09:44:39 -0700

> What may be best instead is to just set sa_family to AF_UNSPEC when
> it's not specified. An sa_family that was AF_INET or AF_INET6 would
> imply that it had been specified. That way someone could reasonably
> force a ->bind to occur to INADDR_ANY (as silly as that sounds).

Ok, I'll do that.

>> It just seems cleaner in general to set it to known value in case errors
>> later in that method keep us from setting it to a proper new value.
>> And, certainly useful if it can be set twice somehow..but not sure if that's
>> currently the case or not.
>>
>
> Fair enough. There's probably no need to zero this out entirely though.
> Setting the family to AF_UNSPEC should be sufficient.

Will do that in next patch.

>>>> -match_address(struct TCP_Server_Info *server, struct sockaddr *addr)
>>>> +srcip_matches(struct sockaddr *srcaddr, struct sockaddr *rhs)
>>>> +{
>>>> +	if (cifs_addr_is_specified(srcaddr)) {
>>>
>>> 	Just wondering what the expected behavior would be here...
>>>
>>> 	Suppose I have a clustered IP address set up, and I want to
>>> 	ensure that one of my CIFS mounts use that as the srcaddr. Now,
>>> 	suppose I make another mount to the same server, but don't
>>> 	specify the srcaddr there. That mount uses the same socket as
>>> 	the first one (that is bound to the srcaddr). Now, suppose I
>>> 	unmount the first mount and then fail over the IP address. What
>>> 	happens to the second one? (Nothing good, I'd wager...)
>>>
>>> 	Perhaps we should consider using a different socket when the
>>> 	srcaddr isn't specified?
>>
>> The idea is that each mount with unique source addr gets it's own
>> cifs session.  They will not be shared even if the server is the
>> same.  If srcaddr isn't specified, then you get today's behaviour
>> (shared so long as server is the same).
>>
>> In your example, the bound and the un-bound shouldn't share anything,
>> but it's possible my code doesn't fully do that.  If so, I would
>> consider that a bug.
>>
>
> Ok, I think that is a bug here. srcip_matches returns true when
> cifs_addr_is_specified returns false. So if you don't specify a
> srcaddr= option, you'll match any session, right?

Ok, I think you're right, I'll work on a fix and post a new
patch shortly.

Thanks,
Ben

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