[PATCH] Bug 11061, NFS mounts dropped

[PATCH] Bug 11061, NFS mounts dropped

[Ian Dall]

The following patch is relative to 2.6.29-rc3. The crux of the matter is
that sock_addr structures can't be reliably compared using memcmp()
because there are padding bytes in the structure which can't be
guaranteed to be the same even when the sock_addr structures refer to
the same thing.

There have been significant changes to the way address comparison is
done in client.c since my original patch (Kernel Bug Tracker #11061,
comment 19). In particular ipv4 addresses are now mapped to ipv6
addresses for comparison (but only if the kernel is configured for
IPV6). The conditional compilation blocks were quite big and contained
duplication. Adding my changes in their original style would have only
made matters worse. So I have taken to opportunity to clean up. By
inverting the logic, I was able to reduce the depth of nested ifs,
reduce the number of trivial helper functions and reduce the size of the
conditional compilation blocks. This is a mater of opinion of course,
but I think the result is cleaner and easier to read and maintain.

This has been tested with kernels configured with and without IPV6.

Log:
======================
Clean up address comparison code, replace nfs_sockaddr_match_ipaddr()
with more generic nfs_sockaddr_match_addr(). Don't compare sock_addr
structures using memcmp().

Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
======================
diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 9b728f3..bc38e24 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -223,6 +223,8 @@ void nfs_put_client(struct nfs_client *clp)
 	}
 }
 
+enum nfs_sockaddr_match_flags { nfs_port_nomatch = 0, nfs_port_match };
+
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 static const struct in6_addr *nfs_map_ipv4_addr(const struct sockaddr *sa, struct in6_addr *addr_mapped)
 {
@@ -238,40 +240,56 @@ static const struct in6_addr *nfs_map_ipv4_addr(const struct sockaddr *sa, struc
 			return addr_mapped;
 	}
 }
+#endif
 
-static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
-		const struct sockaddr *sa2)
+static int nfs_sockaddr_match_addr(const struct sockaddr *sa1,
+				   const struct sockaddr *sa2,
+				   enum nfs_sockaddr_match_flags flags)
 {
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	const struct in6_addr *addr1;
 	const struct in6_addr *addr2;
 	struct in6_addr addr1_mapped;
 	struct in6_addr addr2_mapped;
-
+	
 	addr1 = nfs_map_ipv4_addr(sa1, &addr1_mapped);
-	if (likely(addr1 != NULL)) {
-		addr2 = nfs_map_ipv4_addr(sa2, &addr2_mapped);
-		if (likely(addr2 != NULL))
-			return ipv6_addr_equal(addr1, addr2);
-	}
-	return 0;
-}
+	if (unlikely(addr1 == NULL))
+		return 0;
+	addr2 = nfs_map_ipv4_addr(sa2, &addr2_mapped);
+	if (unlikely(addr2 == NULL))
+		return 0;
+	if (likely(!ipv6_addr_equal(addr1, addr2)))
+		return 0;
 #else
-static int nfs_sockaddr_match_ipaddr4(const struct sockaddr_in *sa1,
-				 const struct sockaddr_in *sa2)
-{
-	return sa1->sin_addr.s_addr == sa2->sin_addr.s_addr;
-}
-
-static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
-				 const struct sockaddr *sa2)
-{
 	if (unlikely(sa1->sa_family != AF_INET || sa2->sa_family != AF_INET))
 		return 0;
-	return nfs_sockaddr_match_ipaddr4((const struct sockaddr_in *)sa1,
-			(const struct sockaddr_in *)sa2);
-}
+	if (likely(((const struct sockaddr_in *)sa1)->sin_addr.s_addr !=
+		   ((const struct sockaddr_in *)sa2)->sin_addr.s_addr))
+		return 0;
 #endif
 
+	if (flags != nfs_port_match)
+		return 1;
+
+	/* match the port */
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+	switch (sa1->sa_family) {
+	default:
+		return 0;
+	case AF_INET:
+		return (((const struct sockaddr_in *)sa1)->sin_port ==
+			((const struct sockaddr_in *)sa2)->sin_port);
+	case AF_INET6:
+		return (((const struct sockaddr_in6 *)sa1)->sin6_port ==
+			((const struct sockaddr_in6 *)sa2)->sin6_port);
+	}
+#else
+	return (((const struct sockaddr_in *)sa1)->sin_port ==
+		((const struct sockaddr_in *)sa2)->sin_port);
+#endif
+
+}
+
 /*
  * Find a client by IP address and protocol version
  * - returns NULL if no such client
@@ -293,7 +311,7 @@ struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion)
 			continue;
 
 		/* Match only the IP address, not the port number */
-		if (!nfs_sockaddr_match_ipaddr(addr, clap))
+		if (!nfs_sockaddr_match_addr(addr, clap, nfs_port_nomatch))
 			continue;
 
 		atomic_inc(&clp->cl_count);
@@ -326,7 +344,7 @@ struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
 			continue;
 
 		/* Match only the IP address, not the port number */
-		if (!nfs_sockaddr_match_ipaddr(sap, clap))
+		if (!nfs_sockaddr_match_addr(sap, clap, nfs_port_nomatch))
 			continue;
 
 		atomic_inc(&clp->cl_count);
@@ -344,8 +362,10 @@ struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
 static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *data)
 {
 	struct nfs_client *clp;
+	const struct sockaddr *sap = data->addr;
 
 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
+	        struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;
 		/* Don't match clients that failed to initialise properly */
 		if (clp->cl_cons_state < 0)
 			continue;
@@ -358,9 +378,9 @@ static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *dat
 			continue;
 
 		/* Match the full socket address */
-		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
+		if (!nfs_sockaddr_match_addr(sap, clap, nfs_port_match))
 			continue;
-
+		
 		atomic_inc(&clp->cl_count);
 		return clp;
 	}



-- 
Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Re: [PATCH] Bug 11061, NFS mounts dropped

[Chuck Lever]

On Jan 31, 2009, at 10:45 PM, Ian Dall wrote:
> The following patch is relative to 2.6.29-rc3. The crux of the  
> matter is
> that sock_addr structures can't be reliably compared using memcmp()
> because there are padding bytes in the structure which can't be
> guaranteed to be the same even when the sock_addr structures refer to
> the same thing.
>
> There have been significant changes to the way address comparison is
> done in client.c since my original patch (Kernel Bug Tracker #11061,
> comment 19). In particular ipv4 addresses are now mapped to ipv6
> addresses for comparison (but only if the kernel is configured for
> IPV6). The conditional compilation blocks were quite big and contained
> duplication. Adding my changes in their original style would have only
> made matters worse. So I have taken to opportunity to clean up. By
> inverting the logic, I was able to reduce the depth of nested ifs,
> reduce the number of trivial helper functions and reduce the size of  
> the
> conditional compilation blocks. This is a mater of opinion of course,
> but I think the result is cleaner and easier to read and maintain.
>
> This has been tested with kernels configured with and without IPV6.

As far as I can tell, this is only an issue in nfs_match_client(), so  
there isn't any need to rewrite the other pieces.

Isn't it enough to replace the memcmp() with a call to  
nfs_sockaddr_match_ipaddr() in nfs_match_client(), and then add a  
little extra logic in nfs_match_client() to compare the port numbers  
too?

> Log:
> ======================
> Clean up address comparison code, replace nfs_sockaddr_match_ipaddr()
> with more generic nfs_sockaddr_match_addr(). Don't compare sock_addr
> structures using memcmp().
>
> Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> ======================
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 9b728f3..bc38e24 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -223,6 +223,8 @@ void nfs_put_client(struct nfs_client *clp)
> 	}
> }
>
> +enum nfs_sockaddr_match_flags { nfs_port_nomatch = 0,  
> nfs_port_match };
> +
> #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> static const struct in6_addr *nfs_map_ipv4_addr(const struct  
> sockaddr *sa, struct in6_addr *addr_mapped)
> {
> @@ -238,40 +240,56 @@ static const struct in6_addr  
> *nfs_map_ipv4_addr(const struct sockaddr *sa, struc
> 			return addr_mapped;
> 	}
> }
> +#endif
>
> -static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
> -		const struct sockaddr *sa2)
> +static int nfs_sockaddr_match_addr(const struct sockaddr *sa1,
> +				   const struct sockaddr *sa2,
> +				   enum nfs_sockaddr_match_flags flags)
> {
> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> 	const struct in6_addr *addr1;
> 	const struct in6_addr *addr2;
> 	struct in6_addr addr1_mapped;
> 	struct in6_addr addr2_mapped;
> -
> +	
> 	addr1 = nfs_map_ipv4_addr(sa1, &addr1_mapped);
> -	if (likely(addr1 != NULL)) {
> -		addr2 = nfs_map_ipv4_addr(sa2, &addr2_mapped);
> -		if (likely(addr2 != NULL))
> -			return ipv6_addr_equal(addr1, addr2);
> -	}
> -	return 0;
> -}
> +	if (unlikely(addr1 == NULL))
> +		return 0;
> +	addr2 = nfs_map_ipv4_addr(sa2, &addr2_mapped);
> +	if (unlikely(addr2 == NULL))
> +		return 0;
> +	if (likely(!ipv6_addr_equal(addr1, addr2)))
> +		return 0;
> #else
> -static int nfs_sockaddr_match_ipaddr4(const struct sockaddr_in *sa1,
> -				 const struct sockaddr_in *sa2)
> -{
> -	return sa1->sin_addr.s_addr == sa2->sin_addr.s_addr;
> -}
> -
> -static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
> -				 const struct sockaddr *sa2)
> -{
> 	if (unlikely(sa1->sa_family != AF_INET || sa2->sa_family != AF_INET))
> 		return 0;
> -	return nfs_sockaddr_match_ipaddr4((const struct sockaddr_in *)sa1,
> -			(const struct sockaddr_in *)sa2);
> -}
> +	if (likely(((const struct sockaddr_in *)sa1)->sin_addr.s_addr !=
> +		   ((const struct sockaddr_in *)sa2)->sin_addr.s_addr))
> +		return 0;
> #endif
>
> +	if (flags != nfs_port_match)
> +		return 1;
> +
> +	/* match the port */
> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> +	switch (sa1->sa_family) {
> +	default:
> +		return 0;
> +	case AF_INET:
> +		return (((const struct sockaddr_in *)sa1)->sin_port ==
> +			((const struct sockaddr_in *)sa2)->sin_port);
> +	case AF_INET6:
> +		return (((const struct sockaddr_in6 *)sa1)->sin6_port ==
> +			((const struct sockaddr_in6 *)sa2)->sin6_port);
> +	}
> +#else
> +	return (((const struct sockaddr_in *)sa1)->sin_port ==
> +		((const struct sockaddr_in *)sa2)->sin_port);
> +#endif
> +
> +}
> +
> /*
>  * Find a client by IP address and protocol version
>  * - returns NULL if no such client
> @@ -293,7 +311,7 @@ struct nfs_client *nfs_find_client(const struct  
> sockaddr *addr, u32 nfsversion)
> 			continue;
>
> 		/* Match only the IP address, not the port number */
> -		if (!nfs_sockaddr_match_ipaddr(addr, clap))
> +		if (!nfs_sockaddr_match_addr(addr, clap, nfs_port_nomatch))
> 			continue;
>
> 		atomic_inc(&clp->cl_count);
> @@ -326,7 +344,7 @@ struct nfs_client *nfs_find_client_next(struct  
> nfs_client *clp)
> 			continue;
>
> 		/* Match only the IP address, not the port number */
> -		if (!nfs_sockaddr_match_ipaddr(sap, clap))
> +		if (!nfs_sockaddr_match_addr(sap, clap, nfs_port_nomatch))
> 			continue;
>
> 		atomic_inc(&clp->cl_count);
> @@ -344,8 +362,10 @@ struct nfs_client *nfs_find_client_next(struct  
> nfs_client *clp)
> static struct nfs_client *nfs_match_client(const struct  
> nfs_client_initdata *data)
> {
> 	struct nfs_client *clp;
> +	const struct sockaddr *sap = data->addr;
>
> 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
> +	        struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;
> 		/* Don't match clients that failed to initialise properly */
> 		if (clp->cl_cons_state < 0)
> 			continue;
> @@ -358,9 +378,9 @@ static struct nfs_client *nfs_match_client(const  
> struct nfs_client_initdata *dat
> 			continue;
>
> 		/* Match the full socket address */
> -		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
> +		if (!nfs_sockaddr_match_addr(sap, clap, nfs_port_match))
> 			continue;
> -
> +		
> 		atomic_inc(&clp->cl_count);
> 		return clp;
> 	}
>
>
>
> -- 
> Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"  
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] Bug 11061, NFS mounts dropped

[Ian Dall]

On Mon, 2009-02-02 at 12:00 -0500, Chuck Lever wrote:
> On Jan 31, 2009, at 10:45 PM, Ian Dall wrote:
> > The following patch is relative to 2.6.29-rc3. The crux of the  
> > matter is
> > that sock_addr structures can't be reliably compared using memcmp()
> > because there are padding bytes in the structure which can't be
> > guaranteed to be the same even when the sock_addr structures refer to
> > the same thing.
> >
> > There have been significant changes to the way address comparison is
> > done in client.c since my original patch (Kernel Bug Tracker #11061,
> > comment 19). In particular ipv4 addresses are now mapped to ipv6
> > addresses for comparison (but only if the kernel is configured for
> > IPV6). The conditional compilation blocks were quite big and contained
> > duplication. Adding my changes in their original style would have only
> > made matters worse. So I have taken to opportunity to clean up. By
> > inverting the logic, I was able to reduce the depth of nested ifs,
> > reduce the number of trivial helper functions and reduce the size of  
> > the
> > conditional compilation blocks. This is a mater of opinion of course,
> > but I think the result is cleaner and easier to read and maintain.
> >
> > This has been tested with kernels configured with and without IPV6.
> 
> As far as I can tell, this is only an issue in nfs_match_client(), so  
> there isn't any need to rewrite the other pieces.

True. and I can do it that way easily enough if that is more acceptable.
However I felt it better to keep all the address comparison code with
the switches on sa_family and compilation conditional on CONFIG_IPV6 etc
together. That way it is less likely that something gets fixed in one
place and not another and easier to add more network address types,
should that be necessary at some time down the track. Also, there would
be a lack of symmetry in that nfs_match_client would then need to "know"
about the details of a sock_addr, whereas nfs_find_client and
nfs_find_client_next can treat the sockaddr as an opaque object. Small
points perhaps.


-- 
Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Re: [PATCH] Bug 11061, NFS mounts dropped

[Chuck Lever]

On Feb 2, 2009, at Feb 2, 2009, 4:41 PM, Ian Dall wrote:
> On Mon, 2009-02-02 at 12:00 -0500, Chuck Lever wrote:
>> On Jan 31, 2009, at 10:45 PM, Ian Dall wrote:
>>> The following patch is relative to 2.6.29-rc3. The crux of the
>>> matter is
>>> that sock_addr structures can't be reliably compared using memcmp()
>>> because there are padding bytes in the structure which can't be
>>> guaranteed to be the same even when the sock_addr structures refer  
>>> to
>>> the same thing.
>>>
>>> There have been significant changes to the way address comparison is
>>> done in client.c since my original patch (Kernel Bug Tracker #11061,
>>> comment 19). In particular ipv4 addresses are now mapped to ipv6
>>> addresses for comparison (but only if the kernel is configured for
>>> IPV6). The conditional compilation blocks were quite big and  
>>> contained
>>> duplication. Adding my changes in their original style would have  
>>> only
>>> made matters worse. So I have taken to opportunity to clean up. By
>>> inverting the logic, I was able to reduce the depth of nested ifs,
>>> reduce the number of trivial helper functions and reduce the size of
>>> the
>>> conditional compilation blocks. This is a mater of opinion of  
>>> course,
>>> but I think the result is cleaner and easier to read and maintain.
>>>
>>> This has been tested with kernels configured with and without IPV6.
>>
>> As far as I can tell, this is only an issue in nfs_match_client(), so
>> there isn't any need to rewrite the other pieces.
>
> True. and I can do it that way easily enough if that is more  
> acceptable.
> However I felt it better to keep all the address comparison code with
> the switches on sa_family and compilation conditional on CONFIG_IPV6  
> etc
> together. That way it is less likely that something gets fixed in one
> place and not another and easier to add more network address types,
> should that be necessary at some time down the track. Also, there  
> would
> be a lack of symmetry in that nfs_match_client would then need to  
> "know"
> about the details of a sock_addr, whereas nfs_find_client and
> nfs_find_client_next can treat the sockaddr as an opaque object. Small
> points perhaps.

Simply define a helper near nfs_sockaddr_match_ipaddr(), included  
inside the "#if defined(CONFIG_IPV6)," that handles port comparison,  
and call that from nfs_match_client().

Eventually we will probably extract all of this address family- 
specific kruft into something that can be shared amongst all of the  
NFS-related components, so it will likely end up split out this way in  
any event.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] Bug 11061, NFS mounts dropped

[Ian Dall]

On Mon, 2009-02-02 at 16:52 -0500, Chuck Lever wrote:

> Simply define a helper near nfs_sockaddr_match_ipaddr(), included  
> inside the "#if defined(CONFIG_IPV6)," that handles port comparison,  
> and call that from nfs_match_client().
> 
> Eventually we will probably extract all of this address family- 
> specific kruft into something that can be shared amongst all of the  
> NFS-related components, so it will likely end up split out this way in  
> any event.

OK here is version 2 of the patch. It is half the size of version 1,
although the resultant client.c is slightly bigger and IMO slightly
inferior than for version 1, but certainly it is around the margin.


Log:
======================
Don't compare sock_addr structures using memcmp().

Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
======================
diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 9b728f3..ee5e211 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -255,6 +255,22 @@ static int nfs_sockaddr_match_ipaddr(const struct
sockaddr *sa1,
 	}
 	return 0;
 }
+
+static int nfs_sockaddr_match_port(const struct sockaddr *sa1,
+				   const struct sockaddr *sa2)
+{
+	switch (sa1->sa_family) {
+	default:
+		return 0;
+	case AF_INET:
+		return (((const struct sockaddr_in *)sa1)->sin_port ==
+			((const struct sockaddr_in *)sa2)->sin_port);
+	case AF_INET6:
+		return (((const struct sockaddr_in6 *)sa1)->sin6_port ==
+			((const struct sockaddr_in6 *)sa2)->sin6_port);
+	}
+	return 0;
+}
 #else
 static int nfs_sockaddr_match_ipaddr4(const struct sockaddr_in *sa1,
 				 const struct sockaddr_in *sa2)
@@ -270,6 +286,13 @@ static int nfs_sockaddr_match_ipaddr(const struct
sockaddr *sa1,
 	return nfs_sockaddr_match_ipaddr4((const struct sockaddr_in *)sa1,
 			(const struct sockaddr_in *)sa2);
 }
+
+static int nfs_sockaddr_match_port(const struct sockaddr *sa1,
+				   const struct sockaddr *sa2)
+{
+	return (((const struct sockaddr_in *)sa1)->sin_port ==
+		((const struct sockaddr_in *)sa2)->sin_port);
+}
 #endif
 
 /*
@@ -344,8 +367,10 @@ struct nfs_client *nfs_find_client_next(struct
nfs_client *clp)
 static struct nfs_client *nfs_match_client(const struct
nfs_client_initdata *data)
 {
 	struct nfs_client *clp;
+	const struct sockaddr *sap = data->addr;
 
 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
+	        struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;
 		/* Don't match clients that failed to initialise properly */
 		if (clp->cl_cons_state < 0)
 			continue;
@@ -358,9 +383,13 @@ static struct nfs_client *nfs_match_client(const
struct nfs_client_initdata *dat
 			continue;
 
 		/* Match the full socket address */
-		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
+		if (!nfs_sockaddr_match_ipaddr(sap, clap))
+			continue;
+		/* including the port number */
+		if (!nfs_sockaddr_match_port(sap, clap))
 			continue;
 
+
 		atomic_inc(&clp->cl_count);
 		return clp;
 	}


-- 
Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Re: [PATCH] Bug 11061, NFS mounts dropped

[Chuck Lever]

On Feb 3, 2009, at Feb 3, 2009, 4:56 PM, Ian Dall wrote:
> On Mon, 2009-02-02 at 16:52 -0500, Chuck Lever wrote:
>> Simply define a helper near nfs_sockaddr_match_ipaddr(), included
>> inside the "#if defined(CONFIG_IPV6)," that handles port comparison,
>> and call that from nfs_match_client().
>>
>> Eventually we will probably extract all of this address family-
>> specific kruft into something that can be shared amongst all of the
>> NFS-related components, so it will likely end up split out this way  
>> in
>> any event.
>
> OK here is version 2 of the patch. It is half the size of version 1,
> although the resultant client.c is slightly bigger and IMO slightly
> inferior than for version 1, but certainly it is around the margin.

Why do you think this is inferior?  This way we are checking exactly  
the fields in each sockaddr that need to be checked, and using the  
same equality test that most other parts of the NFS client use.

> Log:
> ======================
> Don't compare sock_addr structures using memcmp().

You need to include a patch description (short and long), and a signed- 
off-by line.  See:

   Documentation/SubmittingPatches

The description should explain why this change is necessary and  
provide any background that reviewers need to understand you change.   
Assume that maybe someone outside the Linux NFS community might even  
read it.

Then post it to Trond (the NFS client maintainer) and copy the list.

> Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> ======================
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 9b728f3..ee5e211 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -255,6 +255,22 @@ static int nfs_sockaddr_match_ipaddr(const struct
> sockaddr *sa1,
> 	}
> 	return 0;
> }
> +
> +static int nfs_sockaddr_match_port(const struct sockaddr *sa1,
> +				   const struct sockaddr *sa2)
> +{
> +	switch (sa1->sa_family) {
> +	default:
> +		return 0;
> +	case AF_INET:
> +		return (((const struct sockaddr_in *)sa1)->sin_port ==
> +			((const struct sockaddr_in *)sa2)->sin_port);
> +	case AF_INET6:
> +		return (((const struct sockaddr_in6 *)sa1)->sin6_port ==
> +			((const struct sockaddr_in6 *)sa2)->sin6_port);
> +	}
> +	return 0;
> +}
> #else
> static int nfs_sockaddr_match_ipaddr4(const struct sockaddr_in *sa1,
> 				 const struct sockaddr_in *sa2)
> @@ -270,6 +286,13 @@ static int nfs_sockaddr_match_ipaddr(const struct
> sockaddr *sa1,
> 	return nfs_sockaddr_match_ipaddr4((const struct sockaddr_in *)sa1,
> 			(const struct sockaddr_in *)sa2);
> }
> +
> +static int nfs_sockaddr_match_port(const struct sockaddr *sa1,
> +				   const struct sockaddr *sa2)
> +{
> +	return (((const struct sockaddr_in *)sa1)->sin_port ==
> +		((const struct sockaddr_in *)sa2)->sin_port);
> +}
> #endif
>
> /*
> @@ -344,8 +367,10 @@ struct nfs_client *nfs_find_client_next(struct
> nfs_client *clp)
> static struct nfs_client *nfs_match_client(const struct
> nfs_client_initdata *data)
> {
> 	struct nfs_client *clp;
> +	const struct sockaddr *sap = data->addr;
>
> 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
> +	        struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;
> 		/* Don't match clients that failed to initialise properly */
> 		if (clp->cl_cons_state < 0)
> 			continue;
> @@ -358,9 +383,13 @@ static struct nfs_client *nfs_match_client(const
> struct nfs_client_initdata *dat
> 			continue;
>
> 		/* Match the full socket address */
> -		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
> +		if (!nfs_sockaddr_match_ipaddr(sap, clap))
> +			continue;
> +		/* including the port number */
> +		if (!nfs_sockaddr_match_port(sap, clap))
> 			continue;
>
> +

Extra blank line is perhaps unneeded.

>
> 		atomic_inc(&clp->cl_count);
> 		return clp;
> 	}

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] Bug 11061, NFS mounts dropped

[Trond Myklebust]

On Wed, 2009-02-04 at 08:26 +1030, Ian Dall wrote:
> On Mon, 2009-02-02 at 16:52 -0500, Chuck Lever wrote:
> 
> > Simply define a helper near nfs_sockaddr_match_ipaddr(), included  
> > inside the "#if defined(CONFIG_IPV6)," that handles port comparison,  
> > and call that from nfs_match_client().
> > 
> > Eventually we will probably extract all of this address family- 
> > specific kruft into something that can be shared amongst all of the  
> > NFS-related components, so it will likely end up split out this way in  
> > any event.
> 
> OK here is version 2 of the patch. It is half the size of version 1,
> although the resultant client.c is slightly bigger and IMO slightly
> inferior than for version 1, but certainly it is around the margin.
> 
> 
> Log:
> ======================
> Don't compare sock_addr structures using memcmp().
> 
> Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> ======================
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 9b728f3..ee5e211 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -255,6 +255,22 @@ static int nfs_sockaddr_match_ipaddr(const struct
> sockaddr *sa1,
>  	}
>  	return 0;
>  }
> +
> +static int nfs_sockaddr_match_port(const struct sockaddr *sa1,
> +				   const struct sockaddr *sa2)
> +{
> +	switch (sa1->sa_family) {
> +	default:
> +		return 0;

The "default:" case here should be unnecessary.

> +	case AF_INET:
> +		return (((const struct sockaddr_in *)sa1)->sin_port ==
> +			((const struct sockaddr_in *)sa2)->sin_port);
> +	case AF_INET6:
> +		return (((const struct sockaddr_in6 *)sa1)->sin6_port ==
> +			((const struct sockaddr_in6 *)sa2)->sin6_port);
> +	}
> +	return 0;
> +}
>  #else
>  static int nfs_sockaddr_match_ipaddr4(const struct sockaddr_in *sa1,
>  				 const struct sockaddr_in *sa2)
> @@ -270,6 +286,13 @@ static int nfs_sockaddr_match_ipaddr(const struct
> sockaddr *sa1,
>  	return nfs_sockaddr_match_ipaddr4((const struct sockaddr_in *)sa1,
>  			(const struct sockaddr_in *)sa2);
>  }
> +
> +static int nfs_sockaddr_match_port(const struct sockaddr *sa1,
> +				   const struct sockaddr *sa2)
> +{
> +	return (((const struct sockaddr_in *)sa1)->sin_port ==
> +		((const struct sockaddr_in *)sa2)->sin_port);
> +}
>  #endif
>  
>  /*
> @@ -344,8 +367,10 @@ struct nfs_client *nfs_find_client_next(struct
> nfs_client *clp)
>  static struct nfs_client *nfs_match_client(const struct
> nfs_client_initdata *data)
>  {
>  	struct nfs_client *clp;
> +	const struct sockaddr *sap = data->addr;
>  
>  	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
> +	        struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;

       Might as well make that a 'const struct sockaddr *', since we'll
never want this code to modify anything here...


>  		/* Don't match clients that failed to initialise properly */
>  		if (clp->cl_cons_state < 0)
>  			continue;
> @@ -358,9 +383,13 @@ static struct nfs_client *nfs_match_client(const
> struct nfs_client_initdata *dat
>  			continue;
>  
>  		/* Match the full socket address */
> -		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
> +		if (!nfs_sockaddr_match_ipaddr(sap, clap))
> +			continue;
> +		/* including the port number */
> +		if (!nfs_sockaddr_match_port(sap, clap))
>  			continue;
>  
> +
>  		atomic_inc(&clp->cl_count);
>  		return clp;
>  	}
> 
> 

Re: [PATCH] Bug 11061, NFS mounts dropped

[Ian Dall]

On Tue, 2009-02-03 at 17:16 -0500, Chuck Lever wrote: 
> On Feb 3, 2009, at Feb 3, 2009, 4:56 PM, Ian Dall wrote:
> > OK here is version 2 of the patch. It is half the size of version 1,
> > although the resultant client.c is slightly bigger and IMO slightly
> > inferior than for version 1, but certainly it is around the margin.
> 
> Why do you think this is inferior?  This way we are checking exactly  
> the fields in each sockaddr that need to be checked, and using the  
> same equality test that most other parts of the NFS client use.

There is duplicated code, namely the function headers for 
nfs_sockaddr_match_ipaddr() and nfs_sockaddr_match_port().

Granted this doesn't add to the executable size in any way because of
the conditional compilation, but it is still a potential source of
trouble down the track when one function definition gets changed and
another doesn't and further more, it won't show up unless you test both
IPV6 and non IPV6 configurations. Having function headers in two places
makes following the code with etags more confusing. And even without
etags you have to scroll up and down to see the two versions of the same
function.

Also nfs_sockaddr_match_ipaddr4() is trivial, taking 7 lines of code
(counting the call and the function definition) to do what can be
expressed in 2 and has additional overhead, albeit small.

None of these are a big deal, which is way I said *slightly* inferior.

I'm not exactly sure what you didn't like about my first patch. Maybe it
was just too big (fair enough, more to go wrong), or maybe you didn't
like the  test return; test return; ... style, or maybe you didn't like
the use of a port_match flag.

See if you like this one :-) It is small, but not quite minimal. It
doesn't change the style of logic, it doesn't use a flag. It does
address my concerns about duplicated code, it includes Trond's
suggestions and the resulting code is smallest so far (in SLOC). Granted
that less SLOC isn't always better, but in this case it is done by
removing duplication and not by making more complicated lines, so IMHO
it is a good thing!

Log:
======================
Don't compare sock_addr structures using memcmp(). Reduce conditionally
compiled address comparison code.

Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
======================
diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 9b728f3..1c2a319 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -238,10 +238,12 @@ static const struct in6_addr *nfs_map_ipv4_addr(const struct sockaddr *sa, struc
 			return addr_mapped;
 	}
 }
+#endif
 
 static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
 		const struct sockaddr *sa2)
 {
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	const struct in6_addr *addr1;
 	const struct in6_addr *addr2;
 	struct in6_addr addr1_mapped;
@@ -254,23 +256,32 @@ static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
 			return ipv6_addr_equal(addr1, addr2);
 	}
 	return 0;
-}
 #else
-static int nfs_sockaddr_match_ipaddr4(const struct sockaddr_in *sa1,
-				 const struct sockaddr_in *sa2)
-{
-	return sa1->sin_addr.s_addr == sa2->sin_addr.s_addr;
-}
-
-static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
-				 const struct sockaddr *sa2)
-{
 	if (unlikely(sa1->sa_family != AF_INET || sa2->sa_family != AF_INET))
 		return 0;
-	return nfs_sockaddr_match_ipaddr4((const struct sockaddr_in *)sa1,
-			(const struct sockaddr_in *)sa2);
+	return (((const struct sockaddr_in *)sa1)->sin_addr.s_addr ==
+		((const struct sockaddr_in *)sa2)->sin_addr.s_addr))
+#endif
 }
+
+static int nfs_sockaddr_match_port(const struct sockaddr *sa1,
+				   const struct sockaddr *sa2)
+{
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+	switch (sa1->sa_family) {
+	case AF_INET:
+		return (((const struct sockaddr_in *)sa1)->sin_port ==
+			((const struct sockaddr_in *)sa2)->sin_port);
+	case AF_INET6:
+		return (((const struct sockaddr_in6 *)sa1)->sin6_port ==
+			((const struct sockaddr_in6 *)sa2)->sin6_port);
+	}
+	return 0;
+#else
+	return (((const struct sockaddr_in *)sa1)->sin_port ==
+		((const struct sockaddr_in *)sa2)->sin_port);
 #endif
+}
 
 /*
  * Find a client by IP address and protocol version
@@ -344,8 +355,10 @@ struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
 static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *data)
 {
 	struct nfs_client *clp;
+	const struct sockaddr *sap = data->addr;
 
 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
+	        const struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;
 		/* Don't match clients that failed to initialise properly */
 		if (clp->cl_cons_state < 0)
 			continue;
@@ -358,8 +371,12 @@ static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *dat
 			continue;
 
 		/* Match the full socket address */
-		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
+		if (!nfs_sockaddr_match_ipaddr(sap, clap))
 			continue;
+		/* including the port number */
+		if (!nfs_sockaddr_match_port(sap, clap))
+			continue;
+
 
 		atomic_inc(&clp->cl_count);
 		return clp;

-- 
Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Re: [PATCH] Bug 11061, NFS mounts dropped

[Chuck Lever]

On Feb 3, 2009, at Feb 3, 2009, 5:16 PM, Trond Myklebust wrote:
> On Wed, 2009-02-04 at 08:26 +1030, Ian Dall wrote:
>> On Mon, 2009-02-02 at 16:52 -0500, Chuck Lever wrote:
>>
>>> Simply define a helper near nfs_sockaddr_match_ipaddr(), included
>>> inside the "#if defined(CONFIG_IPV6)," that handles port comparison,
>>> and call that from nfs_match_client().
>>>
>>> Eventually we will probably extract all of this address family-
>>> specific kruft into something that can be shared amongst all of the
>>> NFS-related components, so it will likely end up split out this  
>>> way in
>>> any event.
>>
>> OK here is version 2 of the patch. It is half the size of version 1,
>> although the resultant client.c is slightly bigger and IMO slightly
>> inferior than for version 1, but certainly it is around the margin.

Trond, how come 9082a5cc doesn't change nfs_match_client() to use  
mapped-IPv4 address comparison?  Was that just an oversight, or do  
these two functions really require different address comparison methods?

>> Log:
>> ======================
>> Don't compare sock_addr structures using memcmp().
>>
>> Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>> ======================
>> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
>> index 9b728f3..ee5e211 100644
>> --- a/fs/nfs/client.c
>> +++ b/fs/nfs/client.c
>> @@ -255,6 +255,22 @@ static int nfs_sockaddr_match_ipaddr(const  
>> struct
>> sockaddr *sa1,
>> 	}
>> 	return 0;
>> }
>> +
>> +static int nfs_sockaddr_match_port(const struct sockaddr *sa1,
>> +				   const struct sockaddr *sa2)
>> +{
>> +	switch (sa1->sa_family) {
>> +	default:
>> +		return 0;
>
> The "default:" case here should be unnecessary.
>
>> +	case AF_INET:
>> +		return (((const struct sockaddr_in *)sa1)->sin_port ==
>> +			((const struct sockaddr_in *)sa2)->sin_port);
>> +	case AF_INET6:
>> +		return (((const struct sockaddr_in6 *)sa1)->sin6_port ==
>> +			((const struct sockaddr_in6 *)sa2)->sin6_port);
>> +	}
>> +	return 0;
>> +}
>> #else
>> static int nfs_sockaddr_match_ipaddr4(const struct sockaddr_in *sa1,
>> 				 const struct sockaddr_in *sa2)
>> @@ -270,6 +286,13 @@ static int nfs_sockaddr_match_ipaddr(const  
>> struct
>> sockaddr *sa1,
>> 	return nfs_sockaddr_match_ipaddr4((const struct sockaddr_in *)sa1,
>> 			(const struct sockaddr_in *)sa2);
>> }
>> +
>> +static int nfs_sockaddr_match_port(const struct sockaddr *sa1,
>> +				   const struct sockaddr *sa2)
>> +{
>> +	return (((const struct sockaddr_in *)sa1)->sin_port ==
>> +		((const struct sockaddr_in *)sa2)->sin_port);
>> +}
>> #endif
>>
>> /*
>> @@ -344,8 +367,10 @@ struct nfs_client *nfs_find_client_next(struct
>> nfs_client *clp)
>> static struct nfs_client *nfs_match_client(const struct
>> nfs_client_initdata *data)
>> {
>> 	struct nfs_client *clp;
>> +	const struct sockaddr *sap = data->addr;
>>
>> 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
>> +	        struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;
>
>       Might as well make that a 'const struct sockaddr *', since we'll
> never want this code to modify anything here...
>
>
>> 		/* Don't match clients that failed to initialise properly */
>> 		if (clp->cl_cons_state < 0)
>> 			continue;
>> @@ -358,9 +383,13 @@ static struct nfs_client *nfs_match_client(const
>> struct nfs_client_initdata *dat
>> 			continue;
>>
>> 		/* Match the full socket address */
>> -		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
>> +		if (!nfs_sockaddr_match_ipaddr(sap, clap))
>> +			continue;
>> +		/* including the port number */
>> +		if (!nfs_sockaddr_match_port(sap, clap))
>> 			continue;
>>
>> +
>> 		atomic_inc(&clp->cl_count);
>> 		return clp;
>> 	}
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"  
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] Bug 11061, NFS mounts dropped

[Chuck Lever]

Hi Ian-

On Feb 4, 2009, at Feb 4, 2009, 7:52 AM, Ian Dall wrote:
> On Tue, 2009-02-03 at 17:16 -0500, Chuck Lever wrote:
>> On Feb 3, 2009, at Feb 3, 2009, 4:56 PM, Ian Dall wrote:
>>> OK here is version 2 of the patch. It is half the size of version 1,
>>> although the resultant client.c is slightly bigger and IMO slightly
>>> inferior than for version 1, but certainly it is around the margin.
>>
>> Why do you think this is inferior?  This way we are checking exactly
>> the fields in each sockaddr that need to be checked, and using the
>> same equality test that most other parts of the NFS client use.
>
> There is duplicated code, namely the function headers for
> nfs_sockaddr_match_ipaddr() and nfs_sockaddr_match_port().
>
> Granted this doesn't add to the executable size in any way because of
> the conditional compilation, but it is still a potential source of
> trouble down the track when one function definition gets changed and
> another doesn't and further more, it won't show up unless you test  
> both
> IPV6 and non IPV6 configurations. Having function headers in two  
> places
> makes following the code with etags more confusing. And even without
> etags you have to scroll up and down to see the two versions of the  
> same
> function.
>
> Also nfs_sockaddr_match_ipaddr4() is trivial, taking 7 lines of code
> (counting the call and the function definition) to do what can be
> expressed in 2 and has additional overhead, albeit small.
>
> None of these are a big deal, which is way I said *slightly* inferior.
>
> I'm not exactly sure what you didn't like about my first patch.  
> Maybe it
> was just too big (fair enough, more to go wrong), or maybe you didn't
> like the  test return; test return; ... style, or maybe you didn't  
> like
> the use of a port_match flag.

I think you are trying to address two separate issues here in a single  
patch.  Replacing the memcmp() should be done in one patch, and any  
code clean up should be done in a second.

That way we can review and argue the merits of these changes  
separately; we can (usually) accept or revert them separately if  
needed; and you can more precisely document each change in the two  
patch descriptions.

In fact I now see that we may need a separate address comparison  
function entirely for nfs_match_client.  nfs_find_client is only used  
in the callback path, so it has to convert cached AF_INET addresses to  
mapped IPv4 addresses before it can compare them with incoming server  
addresses, whenever the callback server uses an AF_INET6 listener.

nfs_match_client is used only in the forward mount path.  Does it also  
require this conversion, or can it use a more straightforward address  
comparison method (like for example nlm_cmp_addr) ?

> See if you like this one :-) It is small, but not quite minimal. It
> doesn't change the style of logic, it doesn't use a flag. It does
> address my concerns about duplicated code, it includes Trond's
> suggestions and the resulting code is smallest so far (in SLOC).  
> Granted
> that less SLOC isn't always better, but in this case it is done by
> removing duplication and not by making more complicated lines, so IMHO
> it is a good thing!

Practice has been to keep #ifdef/#else/#endif outside of functions, as  
that makes the body of the functions easier to read.  Sometimes we  
have to use two or more such constructions, and that really obfuscates  
the code; hence the above guideline.

The (slight) downside is that there are two copies of the function's  
synopsis.  However, we should be testing both configurations before  
accepting a patch with such conditionals anyway.  So it really doesn't  
provide any real maintainability advantage to keep a single copy of  
the synopsis.

> Log:
> ======================
> Don't compare sock_addr structures using memcmp(). Reduce  
> conditionally
> compiled address comparison code.

IMO your description is too terse.  We can already see that this patch  
replaces the memcmp.  You should explain why this change is needed.  A  
reference to bug 11061 in the description would also be a good thing.

> Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> ======================
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 9b728f3..1c2a319 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -238,10 +238,12 @@ static const struct in6_addr  
> *nfs_map_ipv4_addr(const struct sockaddr *sa, struc
> 			return addr_mapped;
> 	}
> }
> +#endif
>
> static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
> 		const struct sockaddr *sa2)
> {
> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> 	const struct in6_addr *addr1;
> 	const struct in6_addr *addr2;
> 	struct in6_addr addr1_mapped;
> @@ -254,23 +256,32 @@ static int nfs_sockaddr_match_ipaddr(const  
> struct sockaddr *sa1,
> 			return ipv6_addr_equal(addr1, addr2);
> 	}
> 	return 0;
> -}
> #else
> -static int nfs_sockaddr_match_ipaddr4(const struct sockaddr_in *sa1,
> -				 const struct sockaddr_in *sa2)
> -{
> -	return sa1->sin_addr.s_addr == sa2->sin_addr.s_addr;
> -}
> -
> -static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
> -				 const struct sockaddr *sa2)
> -{
> 	if (unlikely(sa1->sa_family != AF_INET || sa2->sa_family != AF_INET))
> 		return 0;
> -	return nfs_sockaddr_match_ipaddr4((const struct sockaddr_in *)sa1,
> -			(const struct sockaddr_in *)sa2);
> +	return (((const struct sockaddr_in *)sa1)->sin_addr.s_addr ==
> +		((const struct sockaddr_in *)sa2)->sin_addr.s_addr))
> +#endif
> }
> +
> +static int nfs_sockaddr_match_port(const struct sockaddr *sa1,
> +				   const struct sockaddr *sa2)
> +{
> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> +	switch (sa1->sa_family) {
> +	case AF_INET:
> +		return (((const struct sockaddr_in *)sa1)->sin_port ==
> +			((const struct sockaddr_in *)sa2)->sin_port);
> +	case AF_INET6:
> +		return (((const struct sockaddr_in6 *)sa1)->sin6_port ==
> +			((const struct sockaddr_in6 *)sa2)->sin6_port);
> +	}
> +	return 0;
> +#else
> +	return (((const struct sockaddr_in *)sa1)->sin_port ==
> +		((const struct sockaddr_in *)sa2)->sin_port);
> #endif
> +}
>
> /*
>  * Find a client by IP address and protocol version
> @@ -344,8 +355,10 @@ struct nfs_client *nfs_find_client_next(struct  
> nfs_client *clp)
> static struct nfs_client *nfs_match_client(const struct  
> nfs_client_initdata *data)
> {
> 	struct nfs_client *clp;
> +	const struct sockaddr *sap = data->addr;
>
> 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
> +	        const struct sockaddr *clap = (struct sockaddr *)&clp- 
> >cl_addr;
> 		/* Don't match clients that failed to initialise properly */
> 		if (clp->cl_cons_state < 0)
> 			continue;
> @@ -358,8 +371,12 @@ static struct nfs_client  
> *nfs_match_client(const struct nfs_client_initdata *dat
> 			continue;
>
> 		/* Match the full socket address */
> -		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
> +		if (!nfs_sockaddr_match_ipaddr(sap, clap))
> 			continue;
> +		/* including the port number */
> +		if (!nfs_sockaddr_match_port(sap, clap))
> +			continue;
> +
>
> 		atomic_inc(&clp->cl_count);
> 		return clp;

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] Bug 11061, NFS mounts dropped

[Trond Myklebust]

On Wed, 2009-02-04 at 10:59 -0500, Chuck Lever wrote:
> On Feb 3, 2009, at Feb 3, 2009, 5:16 PM, Trond Myklebust wrote:
> > On Wed, 2009-02-04 at 08:26 +1030, Ian Dall wrote:
> >> On Mon, 2009-02-02 at 16:52 -0500, Chuck Lever wrote:
> >>
> >>> Simply define a helper near nfs_sockaddr_match_ipaddr(), included
> >>> inside the "#if defined(CONFIG_IPV6)," that handles port comparison,
> >>> and call that from nfs_match_client().
> >>>
> >>> Eventually we will probably extract all of this address family-
> >>> specific kruft into something that can be shared amongst all of the
> >>> NFS-related components, so it will likely end up split out this  
> >>> way in
> >>> any event.
> >>
> >> OK here is version 2 of the patch. It is half the size of version 1,
> >> although the resultant client.c is slightly bigger and IMO slightly
> >> inferior than for version 1, but certainly it is around the margin.
> 
> Trond, how come 9082a5cc doesn't change nfs_match_client() to use  
> mapped-IPv4 address comparison?  Was that just an oversight, or do  
> these two functions really require different address comparison methods?

It wasn't an oversight. I figure that if someone is mounting using a
mapped IPv4 address, then they want to use an IPv6 socket, hence I
preferred not to touch nfs_match_client().

OTOH, nfs_find_client() is used only by the callback channel, which
doesn't distinguish between requests made over IPv4 and IPv6.

Cheers
  Trond

Re: [PATCH] Bug 11061, NFS mounts dropped

[Chuck Lever]

On Feb 4, 2009, at Feb 4, 2009, 1:20 PM, Trond Myklebust wrote:
> On Wed, 2009-02-04 at 10:59 -0500, Chuck Lever wrote:
>> On Feb 3, 2009, at Feb 3, 2009, 5:16 PM, Trond Myklebust wrote:
>>> On Wed, 2009-02-04 at 08:26 +1030, Ian Dall wrote:
>>>> On Mon, 2009-02-02 at 16:52 -0500, Chuck Lever wrote:
>>>>
>>>>> Simply define a helper near nfs_sockaddr_match_ipaddr(), included
>>>>> inside the "#if defined(CONFIG_IPV6)," that handles port  
>>>>> comparison,
>>>>> and call that from nfs_match_client().
>>>>>
>>>>> Eventually we will probably extract all of this address family-
>>>>> specific kruft into something that can be shared amongst all of  
>>>>> the
>>>>> NFS-related components, so it will likely end up split out this
>>>>> way in
>>>>> any event.
>>>>
>>>> OK here is version 2 of the patch. It is half the size of version  
>>>> 1,
>>>> although the resultant client.c is slightly bigger and IMO slightly
>>>> inferior than for version 1, but certainly it is around the margin.
>>
>> Trond, how come 9082a5cc doesn't change nfs_match_client() to use
>> mapped-IPv4 address comparison?  Was that just an oversight, or do
>> these two functions really require different address comparison  
>> methods?
>
> It wasn't an oversight. I figure that if someone is mounting using a
> mapped IPv4 address, then they want to use an IPv6 socket, hence I
> preferred not to touch nfs_match_client().
>
> OTOH, nfs_find_client() is used only by the callback channel, which
> doesn't distinguish between requests made over IPv4 and IPv6.

Right.  So, then, can nfs_match_client() use the same address  
comparison method as nfs_find_client()?  I'm inclined to think  
nfs_match_client() can use a more straightforward address comparison  
method.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] Bug 11061, NFS mounts dropped

[Trond Myklebust]

On Wed, 2009-02-04 at 13:44 -0500, Chuck Lever wrote:
> Right.  So, then, can nfs_match_client() use the same address  
> comparison method as nfs_find_client()?  I'm inclined to think  
> nfs_match_client() can use a more straightforward address comparison  
> method.

It could. We should certainly be checking that the sa_family field
matches...

Trond

Re: [PATCH] Bug 11061, NFS mounts dropped

[Ian Dall]

On Wed, 2009-02-04 at 12:43 -0500, Chuck Lever wrote:
> Hi Ian-
> 
> On Feb 4, 2009, at Feb 4, 2009, 7:52 AM, Ian Dall wrote:
> > On Tue, 2009-02-03 at 17:16 -0500, Chuck Lever wrote:
> >> On Feb 3, 2009, at Feb 3, 2009, 4:56 PM, Ian Dall wrote:
> >>> OK here is version 2 of the patch. It is half the size of version 1,
> >>> although the resultant client.c is slightly bigger and IMO slightly
> >>> inferior than for version 1, but certainly it is around the margin.
> >>
> >> Why do you think this is inferior?  This way we are checking exactly
> >> the fields in each sockaddr that need to be checked, and using the
> >> same equality test that most other parts of the NFS client use.
> >
> > There is duplicated code, namely the function headers for
> > nfs_sockaddr_match_ipaddr() and nfs_sockaddr_match_port().
> >
> > Granted this doesn't add to the executable size in any way because of
> > the conditional compilation, but it is still a potential source of
> > trouble down the track when one function definition gets changed and
> > another doesn't and further more, it won't show up unless you test  
> > both
> > IPV6 and non IPV6 configurations. Having function headers in two  
> > places
> > makes following the code with etags more confusing. And even without
> > etags you have to scroll up and down to see the two versions of the  
> > same
> > function.
> >
> > Also nfs_sockaddr_match_ipaddr4() is trivial, taking 7 lines of code
> > (counting the call and the function definition) to do what can be
> > expressed in 2 and has additional overhead, albeit small.
> >
> > None of these are a big deal, which is way I said *slightly* inferior.
> >
> > I'm not exactly sure what you didn't like about my first patch.  
> > Maybe it
> > was just too big (fair enough, more to go wrong), or maybe you didn't
> > like the  test return; test return; ... style, or maybe you didn't  
> > like
> > the use of a port_match flag.
> 
> I think you are trying to address two separate issues here in a single  
> patch.  Replacing the memcmp() should be done in one patch, and any  
> code clean up should be done in a second.

Understood. It arose because I it affected the code I was introducing.
My original patch (on bugzilla, not this list) introduced a single
function to compare ports. Then after the conditional compilation was
added, I would have had to add two versions of the same function, which
seemed bad practice.


-- 
Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Re: [PATCH] Bug 11061, NFS mounts dropped

[Ian Dall]

On Wed, 2009-02-04 at 13:47 -0500, Trond Myklebust wrote:
> On Wed, 2009-02-04 at 13:44 -0500, Chuck Lever wrote:
> > Right.  So, then, can nfs_match_client() use the same address  
> > comparison method as nfs_find_client()?  I'm inclined to think  
> > nfs_match_client() can use a more straightforward address comparison  
> > method.
> 
> It could. We should certainly be checking that the sa_family field
> matches...

Based on this, here is a new version of the patch. I've dropped the
contentious clean up of the conditional compilation. The match is done
in a new function nfs_sockaddr_match(). Even though this function
compares IPv4 and IPv6 addresses it compiles and runs OK even when IPV6
is not configured, because the necessary data structures and macros are
still defined. 

When IPV6 is not configured a few bytes (of executable) could be saved
by making two versions of nfs_sockaddr_match() conditional on
CONFIG_IPV6 but personally I don't think it is worth it.

Log:
======================
sockaddr structures can't be reliably compared using memcmp()
because there are padding bytes in the structure which can't be
guaranteed to be the same even when the sockaddr structures refer to
the same socket. 

Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
======================
diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 9b728f3..04b11d9 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -272,6 +272,42 @@ static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
 }
 #endif
 
+/* Test if two socket addresses are the same family, ipaddr and port */
+static int nfs_sockaddr_match(const struct sockaddr *sa1,
+			      const struct sockaddr *sa2)
+{
+	if (sa1->sa_family != sa2->sa_family)
+		return 0;
+  
+	switch (sa1->sa_family) {
+	case AF_INET:
+		{
+			const struct sockaddr_in * saddr1 =
+				(const struct sockaddr_in *) sa1;
+			const struct sockaddr_in * saddr2 =
+				(const struct sockaddr_in *) sa2;
+			if (likely(saddr1->sin_addr.s_addr !=
+				   saddr2->sin_addr.s_addr)) {
+				return 0;
+			}
+			return (saddr1->sin_port == saddr2->sin_port);
+		}
+	case AF_INET6:
+		{
+			const struct sockaddr_in6 * saddr1 =
+				(const struct sockaddr_in6 *) sa1;
+			const struct sockaddr_in6 * saddr2 =
+				(const struct sockaddr_in6 *) sa2;
+			if (likely(!ipv6_addr_equal(&saddr1->sin6_addr,
+						   &saddr1->sin6_addr))) {
+				return 0;
+			}
+			return (saddr1->sin6_port == saddr2->sin6_port);
+		}
+	}
+	return 0;
+}
+
 /*
  * Find a client by IP address and protocol version
  * - returns NULL if no such client
@@ -344,8 +380,10 @@ struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
 static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *data)
 {
 	struct nfs_client *clp;
+	const struct sockaddr *sap = data->addr;
 
 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
+	        const struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;
 		/* Don't match clients that failed to initialise properly */
 		if (clp->cl_cons_state < 0)
 			continue;
@@ -358,7 +396,7 @@ static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *dat
 			continue;
 
 		/* Match the full socket address */
-		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
+		if (!nfs_sockaddr_match(sap, clap))
 			continue;
 
 		atomic_inc(&clp->cl_count);


-- 
Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Re: [PATCH] Bug 11061, NFS mounts dropped

[Chuck Lever]

Hi Ian-

On Feb 8, 2009, at 6:55 AM, Ian Dall wrote:
> On Wed, 2009-02-04 at 13:47 -0500, Trond Myklebust wrote:
>> On Wed, 2009-02-04 at 13:44 -0500, Chuck Lever wrote:
>>> Right.  So, then, can nfs_match_client() use the same address
>>> comparison method as nfs_find_client()?  I'm inclined to think
>>> nfs_match_client() can use a more straightforward address comparison
>>> method.
>>
>> It could. We should certainly be checking that the sa_family field
>> matches...
>
> Based on this, here is a new version of the patch. I've dropped the
> contentious clean up of the conditional compilation. The match is done
> in a new function nfs_sockaddr_match(). Even though this function
> compares IPv4 and IPv6 addresses it compiles and runs OK even when  
> IPV6
> is not configured, because the necessary data structures and macros  
> are
> still defined.
>
> When IPV6 is not configured a few bytes (of executable) could be saved
> by making two versions of nfs_sockaddr_match() conditional on
> CONFIG_IPV6 but personally I don't think it is worth it.
>
> Log:
> ======================
> sockaddr structures can't be reliably compared using memcmp()
> because there are padding bytes in the structure which can't be
> guaranteed to be the same even when the sockaddr structures refer to
> the same socket.

It is also the case that IPv6 socket address structures contain other  
fields that don't have to match, like sin6_scope_id.  I think that is  
worth noting in the patch description.

I don't see anything immediately wrong with this approach, but I made  
a couple of additional minor comments below.

> Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> ======================
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 9b728f3..04b11d9 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -272,6 +272,42 @@ static int nfs_sockaddr_match_ipaddr(const  
> struct sockaddr *sa1,
> }
> #endif
>
> +/* Test if two socket addresses are the same family, ipaddr and  
> port */

Instead of repeating what the function does, can this comment explain  
why the NFS client needs to match on the address and port, for example?

Explaining why it is different than the existing  
nfs_sockaddr_match_ipaddr() function would be helpful for future  
generations.

> +static int nfs_sockaddr_match(const struct sockaddr *sa1,
> +			      const struct sockaddr *sa2)
> +{
> +	if (sa1->sa_family != sa2->sa_family)
> +		return 0;
> +
> +	switch (sa1->sa_family) {
> +	case AF_INET:
> +		{
> +			const struct sockaddr_in * saddr1 =
> +				(const struct sockaddr_in *) sa1;
> +			const struct sockaddr_in * saddr2 =
> +				(const struct sockaddr_in *) sa2;
> +			if (likely(saddr1->sin_addr.s_addr !=
> +				   saddr2->sin_addr.s_addr)) {
> +				return 0;
> +			}
> +			return (saddr1->sin_port == saddr2->sin_port);
> +		}

As a stylistic point, these days we prefer to use a helper function  
instead of curly braces inside the arms of the switch.  It will also  
remove the need to fold those lines.

I have used this style in the past, but there is general consensus now  
that helper functions are easier to read, and are therefore preferred.

> +	case AF_INET6:
> +		{
> +			const struct sockaddr_in6 * saddr1 =
> +				(const struct sockaddr_in6 *) sa1;
> +			const struct sockaddr_in6 * saddr2 =
> +				(const struct sockaddr_in6 *) sa2;
> +			if (likely(!ipv6_addr_equal(&saddr1->sin6_addr,
> +						   &saddr1->sin6_addr))) {
> +				return 0;
> +			}
> +			return (saddr1->sin6_port == saddr2->sin6_port);
> +		}
> +	}
> +	return 0;
> +}
> +
> /*
>  * Find a client by IP address and protocol version
>  * - returns NULL if no such client
> @@ -344,8 +380,10 @@ struct nfs_client *nfs_find_client_next(struct  
> nfs_client *clp)
> static struct nfs_client *nfs_match_client(const struct  
> nfs_client_initdata *data)
> {
> 	struct nfs_client *clp;
> +	const struct sockaddr *sap = data->addr;
>
> 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
> +	        const struct sockaddr *clap = (struct sockaddr *)&clp- 
> >cl_addr;
> 		/* Don't match clients that failed to initialise properly */
> 		if (clp->cl_cons_state < 0)
> 			continue;
> @@ -358,7 +396,7 @@ static struct nfs_client *nfs_match_client(const  
> struct nfs_client_initdata *dat
> 			continue;
>
> 		/* Match the full socket address */
> -		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
> +		if (!nfs_sockaddr_match(sap, clap))
> 			continue;
>
> 		atomic_inc(&clp->cl_count);
>


--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] Bug 11061, NFS mounts dropped

[Ian Dall]

On Mon, 2009-02-09 at 16:11 -0500, Chuck Lever wrote:
> Hi Ian-
> 
> On Feb 8, 2009, at 6:55 AM, Ian Dall wrote:
> > On Wed, 2009-02-04 at 13:47 -0500, Trond Myklebust wrote:
> >> On Wed, 2009-02-04 at 13:44 -0500, Chuck Lever wrote:
> >>> Right.  So, then, can nfs_match_client() use the same address
> >>> comparison method as nfs_find_client()?  I'm inclined to think
> >>> nfs_match_client() can use a more straightforward address comparison
> >>> method.

> > Log:
> > ======================
> > sockaddr structures can't be reliably compared using memcmp()
> > because there are padding bytes in the structure which can't be
> > guaranteed to be the same even when the sockaddr structures refer to
> > the same socket.
> 
> It is also the case that IPv6 socket address structures contain other  
> fields that don't have to match, like sin6_scope_id.  I think that is  
> worth noting in the patch description.
> 
> I don't see anything immediately wrong with this approach, but I made  
> a couple of additional minor comments below.
> 
> > Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Apparently this should be "From" (for the author) not "Signed-off-by"
According to Andrew Morton commenting on a completely different patch.

> > ======================
> > diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> > index 9b728f3..04b11d9 100644
> > --- a/fs/nfs/client.c
> > +++ b/fs/nfs/client.c
> > @@ -272,6 +272,42 @@ static int nfs_sockaddr_match_ipaddr(const  
> > struct sockaddr *sa1,
> > }
> > #endif
> >
> > +/* Test if two socket addresses are the same family, ipaddr and  
> > port */
> 
> Instead of repeating what the function does, can this comment explain  
> why the NFS client needs to match on the address and port, for example?
> 
> Explaining why it is different than the existing  
> nfs_sockaddr_match_ipaddr() function would be helpful for future  
> generations.

Wouldn't this be more appropriate where the function is called? I know
that it is only called from one place, but still, I would have thought
the obvious thing to do would be document what a function does where you
define it and document why you are calling it when you are calling it.
Either way, I don't actually *know* why we need to match on port number!
I have to confess no deep understanding of the difference between
nfs_match_client and nfs_find_client. I only did a bisection to find the
commit which caused the regression, then fixed it.

If someone who does understand wants to contribute a description, that's
fine by me! Following the principle of one change per patch, it may as
well be a different commit since it would relevant regardless of my
patch. The current code (with the memcmp) compares everything, it's just
that it compares too much.

> As a stylistic point, these days we prefer to use a helper function  
> instead of curly braces inside the arms of the switch.  It will also  
> remove the need to fold those lines.
> 
> I have used this style in the past, but there is general consensus now  
> that helper functions are easier to read, and are therefore preferred.

Yes, I blame it on the prevalence of OO programming where it is routine
to have a helper functions which to do nothing more than get a field out
of a structure ;-(  

Here is a new patch with helper functions, improved ipv6 comparison
(though I can not test this and my understanding of ipv6 is almost
certainly incomplete), better comments and a better log entry. 

Log:
======================
sockaddr structures can't be reliably compared using memcmp()
because there are padding bytes in the structure which can't be
guaranteed to be the same even when the sockaddr structures refer to
the same socket. Instead compare all the relevant fields. These are
sin_addr and sin_port number for IPv4. In the case of IPv6 they are
sin6_addr, sin6_port and sin6_scope_id, which is relevant for  link
local addresses. The sin6_flowinfo field only affects QoS and is not
compared. 

From: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
======================
diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 9b728f3..efdae09 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -272,6 +272,62 @@ static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
 }
 #endif
 
+/* Test if two ip4 socket addresses have the same ipaddr and port number */
+static int nfs_sockaddr_cmp_ip4 (const struct sockaddr_in * saddr1,
+			  const struct sockaddr_in * saddr2)
+{
+	/* Compare ip4 address */
+	if (likely(saddr1->sin_addr.s_addr !=
+		   saddr2->sin_addr.s_addr)) {
+		return 0;
+	}
+	/* Compare port */
+	return (saddr1->sin_port == saddr2->sin_port);
+}
+
+/* Test if two ip6 socket addresses have the same ipaddr, scope_id and
+ * port number.
+ */
+static int nfs_sockaddr_cmp_ip6 (const struct sockaddr_in6 * saddr1,
+			  const struct sockaddr_in6 * saddr2)
+{
+	/* Compare ip6 address */
+	if (likely(!ipv6_addr_equal(&saddr1->sin6_addr,
+				    &saddr1->sin6_addr))) {
+		return 0;
+	}
+
+	/* Compare scope_id. This may only matter for link local
+	 * addresses, but it is cheap, and should be harmless, to just
+	 * compare it anyway.
+	 */
+	if (saddr1->sin6_scope_id != saddr2->sin6_scope_id)
+		return 0;
+	/* Don't compare sin6_flowinfo since it is to do with QoS, but
+	 * doesn't effect the packets source or destination.
+	 */ 
+	/* Compare ports */
+	return (saddr1->sin6_port == saddr2->sin6_port);
+}
+
+/* Test if two socket addresses represent the same actual socket */
+static int nfs_sockaddr_cmp(const struct sockaddr *sa1,
+			      const struct sockaddr *sa2)
+{
+	if (sa1->sa_family != sa2->sa_family)
+		return 0;
+  
+	switch (sa1->sa_family) {
+	case AF_INET:
+		return nfs_sockaddr_cmp_ip4 ((const struct sockaddr_in *) sa1,
+					     (const struct sockaddr_in *) sa2);
+	case AF_INET6:
+		return nfs_sockaddr_cmp_ip6 ((const struct sockaddr_in6 *) sa1,
+					     (const struct sockaddr_in6 *) sa2);
+	}
+	return 0;
+}
+
 /*
  * Find a client by IP address and protocol version
  * - returns NULL if no such client
@@ -344,8 +400,10 @@ struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
 static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *data)
 {
 	struct nfs_client *clp;
+	const struct sockaddr *sap = data->addr;
 
 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
+	        const struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;
 		/* Don't match clients that failed to initialise properly */
 		if (clp->cl_cons_state < 0)
 			continue;
@@ -358,7 +416,7 @@ static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *dat
 			continue;
 
 		/* Match the full socket address */
-		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
+		if (!nfs_sockaddr_cmp(sap, clap))
 			continue;
 
 		atomic_inc(&clp->cl_count);



-- 
Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Re: [PATCH] Bug 11061, NFS mounts dropped

[Trond Myklebust]

On Wed, 2009-02-11 at 23:07 +1030, Ian Dall wrote:
> On Mon, 2009-02-09 at 16:11 -0500, Chuck Lever wrote:
> > Hi Ian-
> > 
> > On Feb 8, 2009, at 6:55 AM, Ian Dall wrote:
> > > On Wed, 2009-02-04 at 13:47 -0500, Trond Myklebust wrote:
> > >> On Wed, 2009-02-04 at 13:44 -0500, Chuck Lever wrote:
> > >>> Right.  So, then, can nfs_match_client() use the same address
> > >>> comparison method as nfs_find_client()?  I'm inclined to think
> > >>> nfs_match_client() can use a more straightforward address comparison
> > >>> method.
> 
> > > Log:
> > > ======================
> > > sockaddr structures can't be reliably compared using memcmp()
> > > because there are padding bytes in the structure which can't be
> > > guaranteed to be the same even when the sockaddr structures refer to
> > > the same socket.
> > 
> > It is also the case that IPv6 socket address structures contain other  
> > fields that don't have to match, like sin6_scope_id.  I think that is  
> > worth noting in the patch description.
> > 
> > I don't see anything immediately wrong with this approach, but I made  
> > a couple of additional minor comments below.
> > 
> > > Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> 
> Apparently this should be "From" (for the author) not "Signed-off-by"
> According to Andrew Morton commenting on a completely different patch.

No. Signed-off-by: is correct at the end of the changelog, however a
'From' as the very first line, helps git track who was the original
author.

> > > ======================
> > > diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> > > index 9b728f3..04b11d9 100644
> > > --- a/fs/nfs/client.c
> > > +++ b/fs/nfs/client.c
> > > @@ -272,6 +272,42 @@ static int nfs_sockaddr_match_ipaddr(const  
> > > struct sockaddr *sa1,
> > > }
> > > #endif
> > >
> > > +/* Test if two socket addresses are the same family, ipaddr and  
> > > port */
> > 
> > Instead of repeating what the function does, can this comment explain  
> > why the NFS client needs to match on the address and port, for example?
> > 
> > Explaining why it is different than the existing  
> > nfs_sockaddr_match_ipaddr() function would be helpful for future  
> > generations.
> 
> Wouldn't this be more appropriate where the function is called? I know
> that it is only called from one place, but still, I would have thought
> the obvious thing to do would be document what a function does where you
> define it and document why you are calling it when you are calling it.
> Either way, I don't actually *know* why we need to match on port number!
> I have to confess no deep understanding of the difference between
> nfs_match_client and nfs_find_client. I only did a bisection to find the
> commit which caused the regression, then fixed it.
> 
> If someone who does understand wants to contribute a description, that's
> fine by me! Following the principle of one change per patch, it may as
> well be a different commit since it would relevant regardless of my
> patch. The current code (with the memcmp) compares everything, it's just
> that it compares too much.
> 
> > As a stylistic point, these days we prefer to use a helper function  
> > instead of curly braces inside the arms of the switch.  It will also  
> > remove the need to fold those lines.
> > 
> > I have used this style in the past, but there is general consensus now  
> > that helper functions are easier to read, and are therefore preferred.
> 
> Yes, I blame it on the prevalence of OO programming where it is routine
> to have a helper functions which to do nothing more than get a field out
> of a structure ;-(  
> 
> Here is a new patch with helper functions, improved ipv6 comparison
> (though I can not test this and my understanding of ipv6 is almost
> certainly incomplete), better comments and a better log entry. 
> 
> Log:
> ======================
> sockaddr structures can't be reliably compared using memcmp()
> because there are padding bytes in the structure which can't be
> guaranteed to be the same even when the sockaddr structures refer to
> the same socket. Instead compare all the relevant fields. These are
> sin_addr and sin_port number for IPv4. In the case of IPv6 they are
> sin6_addr, sin6_port and sin6_scope_id, which is relevant for  link
> local addresses. The sin6_flowinfo field only affects QoS and is not
> compared. 
> 
> From: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> ======================
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 9b728f3..efdae09 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -272,6 +272,62 @@ static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
>  }
>  #endif
>  
> +/* Test if two ip4 socket addresses have the same ipaddr and port number */
> +static int nfs_sockaddr_cmp_ip4 (const struct sockaddr_in * saddr1,
> +			  const struct sockaddr_in * saddr2)
> +{
> +	/* Compare ip4 address */
> +	if (likely(saddr1->sin_addr.s_addr !=
> +		   saddr2->sin_addr.s_addr)) {

I don't think you really want the 'likely()' here. If you only have one
server, then the resulting "optimisation" would be incorrect. I'd
therefore leave it out.

> +		return 0;
> +	}
> +	/* Compare port */
> +	return (saddr1->sin_port == saddr2->sin_port);
> +}
> +
> +/* Test if two ip6 socket addresses have the same ipaddr, scope_id and
> + * port number.
> + */
> +static int nfs_sockaddr_cmp_ip6 (const struct sockaddr_in6 * saddr1,
> +			  const struct sockaddr_in6 * saddr2)
> +{
> +	/* Compare ip6 address */
> +	if (likely(!ipv6_addr_equal(&saddr1->sin6_addr,
> +				    &saddr1->sin6_addr))) {

Ditto...

> +		return 0;
> +	}
> +
> +	/* Compare scope_id. This may only matter for link local
> +	 * addresses, but it is cheap, and should be harmless, to just
> +	 * compare it anyway.
> +	 */
> +	if (saddr1->sin6_scope_id != saddr2->sin6_scope_id)
> +		return 0;
> +	/* Don't compare sin6_flowinfo since it is to do with QoS, but
> +	 * doesn't effect the packets source or destination.
> +	 */ 
> +	/* Compare ports */
> +	return (saddr1->sin6_port == saddr2->sin6_port);
> +}
> +
> +/* Test if two socket addresses represent the same actual socket */
> +static int nfs_sockaddr_cmp(const struct sockaddr *sa1,
> +			      const struct sockaddr *sa2)
> +{
> +	if (sa1->sa_family != sa2->sa_family)
> +		return 0;
> +  
> +	switch (sa1->sa_family) {
> +	case AF_INET:
> +		return nfs_sockaddr_cmp_ip4 ((const struct sockaddr_in *) sa1,
> +					     (const struct sockaddr_in *) sa2);
> +	case AF_INET6:
> +		return nfs_sockaddr_cmp_ip6 ((const struct sockaddr_in6 *) sa1,
> +					     (const struct sockaddr_in6 *) sa2);
> +	}
> +	return 0;
> +}
> +
>  /*
>   * Find a client by IP address and protocol version
>   * - returns NULL if no such client
> @@ -344,8 +400,10 @@ struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
>  static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *data)
>  {
>  	struct nfs_client *clp;
> +	const struct sockaddr *sap = data->addr;
>  
>  	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
> +	        const struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;
>  		/* Don't match clients that failed to initialise properly */
>  		if (clp->cl_cons_state < 0)
>  			continue;
> @@ -358,7 +416,7 @@ static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *dat
>  			continue;
>  
>  		/* Match the full socket address */
> -		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
> +		if (!nfs_sockaddr_cmp(sap, clap))
>  			continue;
>  
>  		atomic_inc(&clp->cl_count);
> 
> 
> 

Re: [PATCH] Bug 11061, NFS mounts dropped

[Chuck Lever]

On Feb 11, 2009, at 7:37 AM, Ian Dall wrote:
> On Mon, 2009-02-09 at 16:11 -0500, Chuck Lever wrote:
>> Hi Ian-
>>
>> On Feb 8, 2009, at 6:55 AM, Ian Dall wrote:
>>> On Wed, 2009-02-04 at 13:47 -0500, Trond Myklebust wrote:
>>>> On Wed, 2009-02-04 at 13:44 -0500, Chuck Lever wrote:
>>>>> Right.  So, then, can nfs_match_client() use the same address
>>>>> comparison method as nfs_find_client()?  I'm inclined to think
>>>>> nfs_match_client() can use a more straightforward address  
>>>>> comparison
>>>>> method.
>
>>> Log:
>>> ======================
>>> sockaddr structures can't be reliably compared using memcmp()
>>> because there are padding bytes in the structure which can't be
>>> guaranteed to be the same even when the sockaddr structures refer to
>>> the same socket.
>>
>> It is also the case that IPv6 socket address structures contain other
>> fields that don't have to match, like sin6_scope_id.  I think that is
>> worth noting in the patch description.
>>
>> I don't see anything immediately wrong with this approach, but I made
>> a couple of additional minor comments below.
>>
>>> Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>
> Apparently this should be "From" (for the author) not "Signed-off-by"
> According to Andrew Morton commenting on a completely different patch.

See Trond's comment on this.

You should also consider providing a short (one-line) version of your  
patch description in the Subject: line.  A reference to the bug you  
are addressing and the bug database where it resides belongs in the  
long patch description, IMO.

>>> ======================
>>> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
>>> index 9b728f3..04b11d9 100644
>>> --- a/fs/nfs/client.c
>>> +++ b/fs/nfs/client.c
>>> @@ -272,6 +272,42 @@ static int nfs_sockaddr_match_ipaddr(const
>>> struct sockaddr *sa1,
>>> }
>>> #endif
>>>
>>> +/* Test if two socket addresses are the same family, ipaddr and
>>> port */
>>
>> Instead of repeating what the function does, can this comment explain
>> why the NFS client needs to match on the address and port, for  
>> example?
>>
>> Explaining why it is different than the existing
>> nfs_sockaddr_match_ipaddr() function would be helpful for future
>> generations.
>
> Wouldn't this be more appropriate where the function is called? I know
> that it is only called from one place, but still, I would have thought
> the obvious thing to do would be document what a function does where  
> you
> define it and document why you are calling it when you are calling it.
> Either way, I don't actually *know* why we need to match on port  
> number!

OK, fair enough.

> I have to confess no deep understanding of the difference between
> nfs_match_client and nfs_find_client. I only did a bisection to find  
> the
> commit which caused the regression, then fixed it.
>
> If someone who does understand wants to contribute a description,  
> that's
> fine by me! Following the principle of one change per patch, it may as
> well be a different commit since it would relevant regardless of my
> patch. The current code (with the memcmp) compares everything, it's  
> just
> that it compares too much.
>
>> As a stylistic point, these days we prefer to use a helper function
>> instead of curly braces inside the arms of the switch.  It will also
>> remove the need to fold those lines.
>>
>> I have used this style in the past, but there is general consensus  
>> now
>> that helper functions are easier to read, and are therefore  
>> preferred.
>
> Yes, I blame it on the prevalence of OO programming where it is  
> routine
> to have a helper functions which to do nothing more than get a field  
> out
> of a structure ;-(

Nah, I think we just don't like the curly braces and extra indentation  
inside the arms of the switch statement.  :-)

Basically you need the braces because you are defining locally-scoped  
variables for each comparison.  I think this is a good case where a  
helper is better.

> Here is a new patch with helper functions, improved ipv6 comparison
> (though I can not test this and my understanding of ipv6 is almost
> certainly incomplete), better comments and a better log entry.
>
> Log:
> ======================
> sockaddr structures can't be reliably compared using memcmp()
> because there are padding bytes in the structure which can't be
> guaranteed to be the same even when the sockaddr structures refer to
> the same socket. Instead compare all the relevant fields. These are
> sin_addr and sin_port number for IPv4. In the case of IPv6 they are
> sin6_addr, sin6_port and sin6_scope_id, which is relevant for  link
> local addresses. The sin6_flowinfo field only affects QoS and is not
> compared.
>
> From: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> ======================
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 9b728f3..efdae09 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -272,6 +272,62 @@ static int nfs_sockaddr_match_ipaddr(const  
> struct sockaddr *sa1,
> }
> #endif
>
> +/* Test if two ip4 socket addresses have the same ipaddr and port  
> number */

My preference is to leave the comment out if the only thing it does is  
repeat what is evident from the body of the function.  In this case,  
the function is tiny, and it is clear what it does.  So I don't find  
these particular documenting comments helpful for these subroutines.

Comments are a good thing, generally, but it's better to write self- 
documenting code than to add comments.

The maintainers have the final word, of course.

> +static int nfs_sockaddr_cmp_ip4 (const struct sockaddr_in * saddr1,
> +			  const struct sockaddr_in * saddr2)
> +{
> +	/* Compare ip4 address */
> +	if (likely(saddr1->sin_addr.s_addr !=
> +		   saddr2->sin_addr.s_addr)) {
> +		return 0;
> +	}
> +	/* Compare port */
> +	return (saddr1->sin_port == saddr2->sin_port);

The parentheses are not needed here.

> +}
> +
> +/* Test if two ip6 socket addresses have the same ipaddr, scope_id  
> and
> + * port number.
> + */
> +static int nfs_sockaddr_cmp_ip6 (const struct sockaddr_in6 * saddr1,
> +			  const struct sockaddr_in6 * saddr2)
> +{
> +	/* Compare ip6 address */
> +	if (likely(!ipv6_addr_equal(&saddr1->sin6_addr,
> +				    &saddr1->sin6_addr))) {
> +		return 0;
> +	}

Extra braces aren't needed here, I would think.

> +
> +	/* Compare scope_id. This may only matter for link local
> +	 * addresses, but it is cheap, and should be harmless, to just
> +	 * compare it anyway.
> +	 */
> +	if (saddr1->sin6_scope_id != saddr2->sin6_scope_id)
> +		return 0;

I've been thinking about this a bit over the past couple of days.  I'm  
not yet sure we need this comparison here, but I also don't think it's  
harmless to just leave it in.

If you really want to include this particular comparison, it would be  
better IMO if you check the equivalence of the scope ID fields only if  
the addresses are link-local.

> +	/* Don't compare sin6_flowinfo since it is to do with QoS, but
> +	 * doesn't effect the packets source or destination.
> +	 */
> +	/* Compare ports */
> +	return (saddr1->sin6_port == saddr2->sin6_port);

The parentheses are not needed here.

> +}
> +
> +/* Test if two socket addresses represent the same actual socket */
> +static int nfs_sockaddr_cmp(const struct sockaddr *sa1,
> +			      const struct sockaddr *sa2)
> +{
> +	if (sa1->sa_family != sa2->sa_family)
> +		return 0;
> +
> +	switch (sa1->sa_family) {
> +	case AF_INET:
> +		return nfs_sockaddr_cmp_ip4 ((const struct sockaddr_in *) sa1,
> +					     (const struct sockaddr_in *) sa2);

Kernel C coding style frowns on the space between the function name  
and the argument list.

> +	case AF_INET6:
> +		return nfs_sockaddr_cmp_ip6 ((const struct sockaddr_in6 *) sa1,
> +					     (const struct sockaddr_in6 *) sa2);
> +	}
> +	return 0;
> +}
> +
> /*
>  * Find a client by IP address and protocol version
>  * - returns NULL if no such client
> @@ -344,8 +400,10 @@ struct nfs_client *nfs_find_client_next(struct  
> nfs_client *clp)
> static struct nfs_client *nfs_match_client(const struct  
> nfs_client_initdata *data)
> {
> 	struct nfs_client *clp;
> +	const struct sockaddr *sap = data->addr;
>
> 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
> +	        const struct sockaddr *clap = (struct sockaddr *)&clp- 
> >cl_addr;
> 		/* Don't match clients that failed to initialise properly */
> 		if (clp->cl_cons_state < 0)
> 			continue;
> @@ -358,7 +416,7 @@ static struct nfs_client *nfs_match_client(const  
> struct nfs_client_initdata *dat
> 			continue;
>
> 		/* Match the full socket address */
> -		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
> +		if (!nfs_sockaddr_cmp(sap, clap))
> 			continue;
>
> 		atomic_inc(&clp->cl_count);

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] Bug 11061, NFS mounts dropped

[Ian Dall]

On Wed, 2009-02-11 at 11:24 -0500, Chuck Lever wrote:

> > +	/* Compare scope_id. This may only matter for link local
> > +	 * addresses, but it is cheap, and should be harmless, to just
> > +	 * compare it anyway.
> > +	 */
> > +	if (saddr1->sin6_scope_id != saddr2->sin6_scope_id)
> > +		return 0;
> 
> I've been thinking about this a bit over the past couple of days.  I'm  
> not yet sure we need this comparison here, but I also don't think it's  
> harmless to just leave it in.

As I understand it, it is allowed for a host to see two identical link
local addresses which are on different interfaces but refer to different
servers, so it would need to be in for the case of link local addresses.
But in other cases it is not clear and unfortunately RFC 3493 if not
very helpful on the subject. But non link-local addresses should be
unique so at least it would be safe to not test it in those cases.

> If you really want to include this particular comparison, it would be  
> better IMO if you check the equivalence of the scope ID fields only if  
> the addresses are link-local.

This seems like the safest course of action.

With these changes, the nfs_sockaddr_cmp_ip6 function is starting to
become significant. I was trying to avoid it, but should it be within a

        #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)  ...
        #endif

block? Someone wanting a minimal (maybe embedded) kernel might object to
the "bloat".

> > +	return (saddr1->sin6_port == saddr2->sin6_port);
> 
> The parentheses are not needed here.

I know, but are they frowned upon? AFAIK there is no shortage of
parentheses and they often make things clearer even when redundant.

> Kernel C coding style frowns on the space between the function name  
> and the argument list.

Is there a style guide or is it more of an osmosis thing? Stylistically,
there are lots of things I'd do differently (4 character indents for a
start) but I try and conform with existing practice. Reading the
existing code isn't always a good guide though.

-- 
Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Re: [PATCH] Bug 11061, NFS mounts dropped

[Chuck Lever]

On Feb 11, 2009, at 8:28 PM, Ian Dall wrote:
> On Wed, 2009-02-11 at 11:24 -0500, Chuck Lever wrote:
>
>>> +	/* Compare scope_id. This may only matter for link local
>>> +	 * addresses, but it is cheap, and should be harmless, to just
>>> +	 * compare it anyway.
>>> +	 */
>>> +	if (saddr1->sin6_scope_id != saddr2->sin6_scope_id)
>>> +		return 0;
>>
>> I've been thinking about this a bit over the past couple of days.   
>> I'm
>> not yet sure we need this comparison here, but I also don't think  
>> it's
>> harmless to just leave it in.
>
> As I understand it, it is allowed for a host to see two identical link
> local addresses which are on different interfaces but refer to  
> different
> servers, so it would need to be in for the case of link local  
> addresses.
> But in other cases it is not clear and unfortunately RFC 3493 if not
> very helpful on the subject. But non link-local addresses should be
> unique so at least it would be safe to not test it in those cases.
>
>> If you really want to include this particular comparison, it would be
>> better IMO if you check the equivalence of the scope ID fields only  
>> if
>> the addresses are link-local.
>
> This seems like the safest course of action.
>
> With these changes, the nfs_sockaddr_cmp_ip6 function is starting to
> become significant. I was trying to avoid it, but should it be  
> within a
>
>        #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)  ...
>        #endif
>
> block? Someone wanting a minimal (maybe embedded) kernel might  
> object to
> the "bloat".

Define bloat -- can you report object code sizes on a couple of common  
hardware platforms?  I wouldn't expect this new routine to add more  
than a few tens of bytes of instructions.

>>> +	return (saddr1->sin6_port == saddr2->sin6_port);
>>
>> The parentheses are not needed here.
>
> I know, but are they frowned upon? AFAIK there is no shortage of
> parentheses and they often make things clearer even when redundant.

See below.

>> Kernel C coding style frowns on the space between the function name
>> and the argument list.
>
> Is there a style guide or is it more of an osmosis thing?  
> Stylistically,
> there are lots of things I'd do differently (4 character indents for a
> start) but I try and conform with existing practice. Reading the
> existing code isn't always a good guide though.

They have evolved over time, so not all of the kernel is consistent  
with current practices which you can read about here:

   Documentation/CodingStyle

I don't see a specific missive about return values, but the code  
example in Chapter 6 is to the point.  I know that Trond (the NFS  
maintainer) prefers no parens on return values.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] Bug 11061, NFS mounts dropped

[Ian Dall]

I believe this includes all comments so far. I have also swatted up on
Documentation/CodingStyle, and modified the comments somewhat to
conform.

I really hope this is acceptable now. One proviso. I don't actually have
an IPv6 network so that code path is untested (except that it compiles).

From: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Addresses: http://bugzilla.kernel.org/show_bug.cgi?id=11061

sockaddr structures can't be reliably compared using memcmp() because
there are padding bytes in the structure which can't be guaranteed to
be the same even when the sockaddr structures refer to the same
socket. Instead compare all the relevant fields. In the case of IPv6
sin6_flowinfo is not compared because it only affects QoS and
sin6_scope_id is only compared if the address is "link local" because
"link local" addresses need only be unique to a specific link.

Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 9b728f3..fa4060d 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -273,6 +273,65 @@ static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
 #endif
 
 /*
+ * Test if two ip4 socket addresses refer to the same socket, by
+ * comparing relevant fields. The padding bytes specifically, are
+ * not compared.
+ *
+ * The caller should ensure both socket addresses are AF_INET.
+ */
+static int nfs_sockaddr_cmp_ip4(const struct sockaddr_in * saddr1,
+				const struct sockaddr_in * saddr2)
+{
+	if (saddr1->sin_addr.s_addr != saddr2->sin_addr.s_addr)
+		return 0;
+	return saddr1->sin_port == saddr2->sin_port;
+}
+
+/*
+ * Test if two ip6 socket addresses refer to the same socket by
+ * comparing relevant fields. The padding bytes specifically, are not
+ * compared. sin6_flowinfo is not compared because it only affects QoS
+ * and sin6_scope_id is only compared if the address is "link local"
+ * because "link local" addresses need only be unique to a specific
+ * link. Conversely, ordinary unicast addresses might have different
+ * sin6_scope_id.
+ *
+ * The caller should ensure both socket addresses are AF_INET6.
+ */
+static int nfs_sockaddr_cmp_ip6 (const struct sockaddr_in6 * saddr1,
+				 const struct sockaddr_in6 * saddr2)
+{
+	if (!ipv6_addr_equal(&saddr1->sin6_addr,
+			     &saddr1->sin6_addr))
+		return 0;
+	if (ipv6_addr_scope(&saddr1->sin6_addr) == IPV6_ADDR_SCOPE_LINKLOCAL &&
+	    saddr1->sin6_scope_id != saddr2->sin6_scope_id)
+		return 0;
+	return saddr1->sin6_port == saddr2->sin6_port;
+}
+
+/*
+ * Test if two socket addresses represent the same actual socket,
+ * by comparing (only) relevant fields. 
+ */
+static int nfs_sockaddr_cmp(const struct sockaddr *sa1,
+			    const struct sockaddr *sa2)
+{
+	if (sa1->sa_family != sa2->sa_family)
+		return 0;
+  
+	switch (sa1->sa_family) {
+	case AF_INET:
+		return nfs_sockaddr_cmp_ip4((const struct sockaddr_in *) sa1,
+					    (const struct sockaddr_in *) sa2);
+	case AF_INET6:
+		return nfs_sockaddr_cmp_ip6((const struct sockaddr_in6 *) sa1,
+					    (const struct sockaddr_in6 *) sa2);
+	}
+	return 0;
+}
+
+/*
  * Find a client by IP address and protocol version
  * - returns NULL if no such client
  */
@@ -344,8 +403,10 @@ struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
 static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *data)
 {
 	struct nfs_client *clp;
+	const struct sockaddr *sap = data->addr;
 
 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
+	        const struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;
 		/* Don't match clients that failed to initialise properly */
 		if (clp->cl_cons_state < 0)
 			continue;
@@ -358,7 +419,7 @@ static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *dat
 			continue;
 
 		/* Match the full socket address */
-		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
+		if (!nfs_sockaddr_cmp(sap, clap))
 			continue;
 
 		atomic_inc(&clp->cl_count);



-- 
Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Re: [PATCH] Bug 11061, NFS mounts dropped

[Chuck Lever]

On Feb 15, 2009, at 10:32 AM, Ian Dall wrote:

> I believe this includes all comments so far. I have also swatted up on
> Documentation/CodingStyle, and modified the comments somewhat to
> conform.

There is also a Perl script called scripts/checkpatch.pl that you  
should run against your patch before submitting.  It can identify and  
report many common style problems.

Other than the two issues I mention below, I think this is now pretty  
close.

> I really hope this is acceptable now. One proviso. I don't actually  
> have
> an IPv6 network so that code path is untested (except that it  
> compiles).

Unfortunately my IPv6 tunnel provider just dropped off the face of the  
earth, so I currently don't have any IPv6 hosts to test this either.   
I will have to figure something out quickly.

> From: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>
> Addresses: http://bugzilla.kernel.org/show_bug.cgi?id=11061
>
> sockaddr structures can't be reliably compared using memcmp() because
> there are padding bytes in the structure which can't be guaranteed to
> be the same even when the sockaddr structures refer to the same
> socket. Instead compare all the relevant fields. In the case of IPv6
> sin6_flowinfo is not compared because it only affects QoS and
> sin6_scope_id is only compared if the address is "link local" because
> "link local" addresses need only be unique to a specific link.
>
> Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 9b728f3..fa4060d 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -273,6 +273,65 @@ static int nfs_sockaddr_match_ipaddr(const  
> struct sockaddr *sa1,
> #endif
>
> /*
> + * Test if two ip4 socket addresses refer to the same socket, by
> + * comparing relevant fields. The padding bytes specifically, are
> + * not compared.
> + *
> + * The caller should ensure both socket addresses are AF_INET.
> + */
> +static int nfs_sockaddr_cmp_ip4(const struct sockaddr_in * saddr1,
> +				const struct sockaddr_in * saddr2)
> +{
> +	if (saddr1->sin_addr.s_addr != saddr2->sin_addr.s_addr)
> +		return 0;
> +	return saddr1->sin_port == saddr2->sin_port;
> +}
> +
> +/*
> + * Test if two ip6 socket addresses refer to the same socket by
> + * comparing relevant fields. The padding bytes specifically, are not
> + * compared. sin6_flowinfo is not compared because it only affects  
> QoS
> + * and sin6_scope_id is only compared if the address is "link local"
> + * because "link local" addresses need only be unique to a specific
> + * link. Conversely, ordinary unicast addresses might have different
> + * sin6_scope_id.
> + *
> + * The caller should ensure both socket addresses are AF_INET6.
> + */
> +static int nfs_sockaddr_cmp_ip6 (const struct sockaddr_in6 * saddr1,
> +				 const struct sockaddr_in6 * saddr2)
> +{
> +	if (!ipv6_addr_equal(&saddr1->sin6_addr,
> +			     &saddr1->sin6_addr))
> +		return 0;
> +	if (ipv6_addr_scope(&saddr1->sin6_addr) ==  
> IPV6_ADDR_SCOPE_LINKLOCAL &&
> +	    saddr1->sin6_scope_id != saddr2->sin6_scope_id)
> +		return 0;

I've thought more about this.  I think we don't want the scope ID  
comparison here at all.  The scope ID is not really part of an IPv6  
address.  A unique host address is contained only in the sin6_addr  
field; the scope ID is simply local routing information.  I can't  
think of a case where two unique remotes would have identical link- 
local addresses.

So I think we should remove the scope ID check entirely.

I haven't added support for scope IDs in other areas unless it is  
needed for actually sending an RPC or reporting a problem.   
nlm_cmp_addr() for example does not have this check, and neither does  
nfs_sockaddr_match_ipaddr().

> +	return saddr1->sin6_port == saddr2->sin6_port;
> +}
> +
> +/*
> + * Test if two socket addresses represent the same actual socket,
> + * by comparing (only) relevant fields.
> + */
> +static int nfs_sockaddr_cmp(const struct sockaddr *sa1,
> +			    const struct sockaddr *sa2)
> +{
> +	if (sa1->sa_family != sa2->sa_family)
> +		return 0;
> +
> +	switch (sa1->sa_family) {
> +	case AF_INET:
> +		return nfs_sockaddr_cmp_ip4((const struct sockaddr_in *) sa1,
> +					    (const struct sockaddr_in *) sa2);
> +	case AF_INET6:
> +		return nfs_sockaddr_cmp_ip6((const struct sockaddr_in6 *) sa1,
> +					    (const struct sockaddr_in6 *) sa2);

Just a style nit here:  we usually leave out the blank between the  
type cast and the variable these days.

In other areas, I think I placed the casts in the helpers just because  
it makes the caller easier to read, but that's perhaps optional.

> +	}
> +	return 0;
> +}
> +
> +/*
>  * Find a client by IP address and protocol version
>  * - returns NULL if no such client
>  */
> @@ -344,8 +403,10 @@ struct nfs_client *nfs_find_client_next(struct  
> nfs_client *clp)
> static struct nfs_client *nfs_match_client(const struct  
> nfs_client_initdata *data)
> {
> 	struct nfs_client *clp;
> +	const struct sockaddr *sap = data->addr;
>
> 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
> +	        const struct sockaddr *clap = (struct sockaddr *)&clp- 
> >cl_addr;
> 		/* Don't match clients that failed to initialise properly */
> 		if (clp->cl_cons_state < 0)
> 			continue;
> @@ -358,7 +419,7 @@ static struct nfs_client *nfs_match_client(const  
> struct nfs_client_initdata *dat
> 			continue;
>
> 		/* Match the full socket address */
> -		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
> +		if (!nfs_sockaddr_cmp(sap, clap))
> 			continue;
>
> 		atomic_inc(&clp->cl_count);

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] Bug 11061, NFS mounts dropped

[Ian Dall]

On Mon, 2009-02-16 at 11:24 -0500, Chuck Lever wrote:
> On Feb 15, 2009, at 10:32 AM, Ian Dall wrote:
> 
> > I believe this includes all comments so far. I have also swatted up on
> > Documentation/CodingStyle, and modified the comments somewhat to
> > conform.
> 
> There is also a Perl script called scripts/checkpatch.pl that you  
> should run against your patch before submitting.  It can identify and  
> report many common style problems.

Thanks for that, it threw up a few white space issues which I have
fixed.


> I've thought more about this.  I think we don't want the scope ID  
> comparison here at all.  The scope ID is not really part of an IPv6  
> address.  A unique host address is contained only in the sin6_addr  
> field; the scope ID is simply local routing information.  I can't  
> think of a case where two unique remotes would have identical link- 
> local addresses.

Isn't that exactly what makes link local addresses, well, link local?
They only have to be unique to the link (which seems to be in practice a
lan or vlan - a bunch of hosts connected without any routing at the ip
level). They don't have to be globally unique. A single host can be on
multiple lans or vlans.

Finding anything authoritative on this is difficult. The RFC's are
vague. However, following discussion on ietf mail archives, and other
reasonably reputable sources like documentation from IBM, Sun and
FreeBSD  makes me think that my interpretation is correct. Indeed, the
way the BSD crowd seem to do it is to embed the scope_id in the link
local address (internally to the kernel) and thence forth just compare
the address. So they treat it *exactly* like part of the address.

See for example:

http://www.freebsd.org/doc/en/books/developers-handbook/ipv6.html section 8.1.1.3.

http://msdn.microsoft.com/en-us/library/ms739166(VS.85).aspx

http://books.google.com/books?id=6nNjcItz6H4C&pg=PA53&lpg=PA53&dq=sin6_scope_id+%22link+local%22&source=bl&ots=MFyiwYwO5I&sig=hZDvlVcJ8hLOwt7EXoL4lEzT4V4&hl=en&ei=eqCaSdiGD8PDkAWskMmZCw&sa=X&oi=book_result&resnum=7&ct=result

Note especially, section 1.8.1 "Since link local addresses may not be
unique on different links ..."  

The only reasons I can see which would justify omitting the check are:

      * For some reason you could never have a nfs server on a link
        local address.
      * Linux already embeds the scope index in the address for link
        local addresses, internally in the kernel a la BSD. Which I can
        find no evidence for either in the code or in documentation.

There is also the issue that sin6_scope_id == 0 is interpreted as
"unspecified" rather than a specific id. However, as far as I can tell,
the scope_id is only ever unspecified (for link_local addresses) before
a socket is bound. Once a connection is established, the scope id for
link local addresses should always be specified. So by the time this
function is called, the scope_id should be set.


-- 
Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Re: [PATCH] Bug 11061, NFS mounts dropped

[Chuck Lever]

Hi Ian-

On Feb 17, 2009, at Feb 17, 2009, 7:03 AM, Ian Dall wrote:
> On Mon, 2009-02-16 at 11:24 -0500, Chuck Lever wrote:
>>
>> I've thought more about this.  I think we don't want the scope ID
>> comparison here at all.  The scope ID is not really part of an IPv6
>> address.  A unique host address is contained only in the sin6_addr
>> field; the scope ID is simply local routing information.  I can't
>> think of a case where two unique remotes would have identical link-
>> local addresses.
>
> Isn't that exactly what makes link local addresses, well, link local?

No, an IPv6 address is link-local by virtue of having the  
IPV6_ADDR_LINKLOCAL bit set.  Scope IDs came along later.

> They only have to be unique to the link (which seems to be in  
> practice a
> lan or vlan - a bunch of hosts connected without any routing at the ip
> level). They don't have to be globally unique. A single host can be on
> multiple lans or vlans.

Generally the way a link-local address is formed on Linux is by using  
the link's MAC address to form a unique 64-bit EUID, and concatenating  
that to a standard link-local prefix, usually fe80:: .  Each NIC on a  
host therefore has it's own link-local address, generally speaking.

Now, that's certainly not the only way a link-local address can be  
constructed, but this way has the benefit that each NIC (link) on a  
host has a unique link-local address.  So, these addresses,  
practically speaking, happen to be fairly globally unique as long as  
NIC vendors adhere to the guidelines around selecting MAC addresses.

> Finding anything authoritative on this is difficult. The RFC's are
> vague.

Agreed, and I don't claim to be an authority on this either.

> However, following discussion on ietf mail archives, and other
> reasonably reputable sources like documentation from IBM, Sun and
> FreeBSD  makes me think that my interpretation is correct. Indeed, the
> way the BSD crowd seem to do it is to embed the scope_id in the link
> local address (internally to the kernel) and thence forth just compare
> the address. So they treat it *exactly* like part of the address.

On Linux, the scope ID is embedded in presentation format addresses  
with a "%n" or "%dev" suffix.  For socket addresses, it is placed in  
the scope ID field.

I haven't looked at the BSD IPv6 implementation, but there may be  
other reasons for embedding a scope ID in the address (for example,  
this could be an artifact of how link-local addresses behaved before  
there was a separate scope ID field in sockaddr_in6).  I don't see  
this as a convincing argument that this is correct for Linux.

> See for example:
>
> http://www.freebsd.org/doc/en/books/developers-handbook/ipv6.html  
> section 8.1.1.3.
>
> http://msdn.microsoft.com/en-us/library/ms739166(VS.85).aspx
>
> http://books.google.com/books?id=6nNjcItz6H4C&pg=PA53&lpg=PA53&dq=sin6_scope_id+%22link+local%22&source=bl&ots=MFyiwYwO5I&sig=hZDvlVcJ8hLOwt7EXoL4lEzT4V4&hl=en&ei=eqCaSdiGD8PDkAWskMmZCw&sa=X&oi=book_result&resnum=7&ct=result
>
> Note especially, section 1.8.1 "Since link local addresses may not be
> unique on different links ..."

Does that imply that unique hosts can have the same link-local  
address, or simply that the same link-local address is permitted to  
appear on different links?  Is it a practical configuration (ie  
something that would be widely deployed) to maintain server hosts with  
the same link-local address on different links?

The point of nfs_match_client is to detect when a mount is going to  
the same server.  If the address is the same, but the scope ID is  
different, do we know that necessarily means these are two distinct  
hosts?

A single server can have multiple NICs, and thus multiple link-local  
addresses.  In general a multi-homed server will always defeat the  
address check in nfs_match_client, so it's not a perfect world in any  
case.

> The only reasons I can see which would justify omitting the check are:
>
>      * For some reason you could never have a nfs server on a link
>        local address.

We specifically want to support accessing link-local servers to allow  
admins to "kick the tires" without assigning real IPv6 addresses.   
Perhaps it even makes sense for a private storage-area network  
configuration, where IPv6 addresses are assigned to clients and  
servers entirely automatically and without router advertisement or  
DHCPv6.

>      * Linux already embeds the scope index in the address for link
>        local addresses, internally in the kernel a la BSD. Which I can
>        find no evidence for either in the code or in documentation.

As far as I know, the scope ID is used on Linux to determine which  
local link is used to access a host.

> There is also the issue that sin6_scope_id == 0 is interpreted as
> "unspecified" rather than a specific id. However, as far as I can  
> tell,
> the scope_id is only ever unspecified (for link_local addresses)  
> before
> a socket is bound. Once a connection is established, the scope id for
> link local addresses should always be specified. So by the time this
> function is called, the scope_id should be set.

I think the correct way to proceed here is to address the original  
problem without adding the scope ID check, then explore the scope ID  
issue separately.  The presenting problem in 11061 affects IPv4  
addresses too if I'm not mistaken, and IPv4 is fairly widely deployed,  
so it needs to be fixed now.

If scope ID matching is a problem in nfs_match_client, then it is also  
an issue for other places in the NFS client, and should be fixed in  
all those places at once, in a single patch that documents the scope  
ID matching requirement.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] Bug 11061, NFS mounts dropped

[Chuck Lever]

On Feb 17, 2009, at Feb 17, 2009, 7:03 AM, Ian Dall wrote:
> On Mon, 2009-02-16 at 11:24 -0500, Chuck Lever wrote:
>> On Feb 15, 2009, at 10:32 AM, Ian Dall wrote:
>>
>>> I believe this includes all comments so far. I have also swatted  
>>> up on
>>> Documentation/CodingStyle, and modified the comments somewhat to
>>> conform.
>>
>> There is also a Perl script called scripts/checkpatch.pl that you
>> should run against your patch before submitting.  It can identify and
>> report many common style problems.
>
> Thanks for that, it threw up a few white space issues which I have
> fixed.
>
>
>> I've thought more about this.  I think we don't want the scope ID
>> comparison here at all.  The scope ID is not really part of an IPv6
>> address.  A unique host address is contained only in the sin6_addr
>> field; the scope ID is simply local routing information.  I can't
>> think of a case where two unique remotes would have identical link-
>> local addresses.
>
> Isn't that exactly what makes link local addresses, well, link local?
> They only have to be unique to the link (which seems to be in  
> practice a
> lan or vlan - a bunch of hosts connected without any routing at the ip
> level). They don't have to be globally unique. A single host can be on
> multiple lans or vlans.
>
> Finding anything authoritative on this is difficult. The RFC's are
> vague. However, following discussion on ietf mail archives, and other
> reasonably reputable sources like documentation from IBM, Sun and
> FreeBSD  makes me think that my interpretation is correct. Indeed, the
> way the BSD crowd seem to do it is to embed the scope_id in the link
> local address (internally to the kernel) and thence forth just compare
> the address. So they treat it *exactly* like part of the address.
>
> See for example:
>
> http://www.freebsd.org/doc/en/books/developers-handbook/ipv6.html  
> section 8.1.1.3.
>
> http://msdn.microsoft.com/en-us/library/ms739166(VS.85).aspx
>
> http://books.google.com/books?id=6nNjcItz6H4C&pg=PA53&lpg=PA53&dq=sin6_scope_id+%22link+local%22&source=bl&ots=MFyiwYwO5I&sig=hZDvlVcJ8hLOwt7EXoL4lEzT4V4&hl=en&ei=eqCaSdiGD8PDkAWskMmZCw&sa=X&oi=book_result&resnum=7&ct=result
>
> Note especially, section 1.8.1 "Since link local addresses may not be
> unique on different links ..."
>
> The only reasons I can see which would justify omitting the check are:
>
>      * For some reason you could never have a nfs server on a link
>        local address.

I should mention that this may be a problem for TI-RPC and NFSv4.   
There's currently no way to represent a scope ID in a universal  
address (the string used during rpcbind registration, or for sending  
the callback address to an NFSv4 server).  So, in that case, we may  
have to bail on link-local support entirely.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] Bug 11061, NFS mounts dropped

[Ian Dall]

On Tue, 2009-02-17 at 12:29 -0500, Chuck Lever wrote:

> I think the correct way to proceed here is to address the original  
> problem without adding the scope ID check, then explore the scope ID  
> issue separately.  The presenting problem in 11061 affects IPv4  
> addresses too if I'm not mistaken, and IPv4 is fairly widely deployed,  
> so it needs to be fixed now.
Absolutely. Where it bit me, and as far as I know all the the respondents, was with IPv4 networks.

> If scope ID matching is a problem in nfs_match_client, then it is also  
> an issue for other places in the NFS client, and should be fixed in  
> all those places at once, in a single patch that documents the scope  
> ID matching requirement.

Sounds reasonable. I'll change the comment somehow to indicate why the
scope_id field is not being compared.

Regards,

Ian

-- 
Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Re: [PATCH] Bug 11061, NFS mounts dropped

[Ian Dall]

On Tue, 2009-02-17 at 12:29 -0500, Chuck Lever wrote:
> Hi Ian-
> 
> On Feb 17, 2009, at Feb 17, 2009, 7:03 AM, Ian Dall wrote:
> > On Mon, 2009-02-16 at 11:24 -0500, Chuck Lever wrote:
> >>
> >> I've thought more about this.  I think we don't want the scope ID
> >> comparison here at all.  The scope ID is not really part of an IPv6
> >> address.  A unique host address is contained only in the sin6_addr
> >> field; the scope ID is simply local routing information.  I can't
> >> think of a case where two unique remotes would have identical link-
> >> local addresses.
> >
> > Isn't that exactly what makes link local addresses, well, link local?
> 
> No, an IPv6 address is link-local by virtue of having the  
> IPV6_ADDR_LINKLOCAL bit set.  Scope IDs came along later.

That is just an API and nothing to do with the semantics of "link
local". Previously it was unspecified how to denote a link, now there is
a sockaddr field (which is largely undocumented, so not much of a
change). There is nothing in the IPv6 standard which says that the
kernel has to use sockaddr internally, but clearly it has to keep track
of what link a link local address is on somehow!

> Generally the way a link-local address is formed on Linux is by using  
> the link's MAC address to form a unique 64-bit EUID, and concatenating  
> that to a standard link-local prefix, usually fe80:: .  Each NIC on a  
> host therefore has it's own link-local address, generally speaking.

Yes. But not all hosts on the "link" need be linux. Also, not all lans
have to be ethernet.

> > http://books.google.com/books?id=6nNjcItz6H4C&pg=PA53&lpg=PA53&dq=sin6_scope_id+%22link+local%22&source=bl&ots=MFyiwYwO5I&sig=hZDvlVcJ8hLOwt7EXoL4lEzT4V4&hl=en&ei=eqCaSdiGD8PDkAWskMmZCw&sa=X&oi=book_result&resnum=7&ct=result
> >
> > Note especially, section 1.8.1 "Since link local addresses may not be
> > unique on different links ..."
> 
> Does that imply that unique hosts can have the same link-local  
> address, or simply that the same link-local address is permitted to  
> appear on different links?

Both I reckon. It is the address which is link local, not (necessarilly)
the host. But if they are link local addresses on the same host but
different links, which are other wise the same, everything else on the
network should treat them just as if they were separate hosts. This is
no different to a host having two ordinary IP (v4 or v6) addresses.

Potentially a host might run two nfs servers listening on two interfaces
on two separate lans with the same link local address, serving up
different files. The servers are on the same host, but that should be
completely opaque to clients.

> I think the correct way to proceed here is to address the original  
> problem without adding the scope ID check, then explore the scope ID  
> issue separately.  The presenting problem in 11061 affects IPv4  
> addresses too if I'm not mistaken, and IPv4 is fairly widely deployed,  
> so it needs to be fixed now.
> 
> If scope ID matching is a problem in nfs_match_client, then it is also  
> an issue for other places in the NFS client, and should be fixed in  
> all those places at once, in a single patch that documents the scope  
> ID matching requirement.

From: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Addresses: http://bugzilla.kernel.org/show_bug.cgi?id=11061

sockaddr structures can't be reliably compared using memcmp() because
there are padding bytes in the structure which can't be guaranteed to
be the same even when the sockaddr structures refer to the same
socket. Instead compare all the relevant fields. In the case of IPv6
sin6_flowinfo is not compared because it only affects QoS and
sin6_scope_id is currently not supported.

Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 9b728f3..9ac4c6b 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -273,6 +273,59 @@ static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
 #endif
 
 /*
+ * Test if two ip4 socket addresses refer to the same socket, by
+ * comparing relevant fields. The padding bytes specifically, are
+ * not compared.
+ *
+ * The caller should ensure both socket addresses are AF_INET.
+ */
+static int nfs_sockaddr_cmp_ip4(const struct sockaddr_in *saddr1,
+				const struct sockaddr_in *saddr2)
+{
+	if (saddr1->sin_addr.s_addr != saddr2->sin_addr.s_addr)
+		return 0;
+	return saddr1->sin_port == saddr2->sin_port;
+}
+
+/*
+ * Test if two ip6 socket addresses refer to the same socket by
+ * comparing relevant fields. The padding bytes specifically, are not
+ * compared. sin6_flowinfo is not compared because it only affects QoS
+ * and sin6_scope_id is currently not supported.
+ *
+ * The caller should ensure both socket addresses are AF_INET6.
+ */
+static int nfs_sockaddr_cmp_ip6(const struct sockaddr_in6 *saddr1,
+				const struct sockaddr_in6 *saddr2)
+{
+	if (!ipv6_addr_equal(&saddr1->sin6_addr,
+			     &saddr1->sin6_addr))
+		return 0;
+	return saddr1->sin6_port == saddr2->sin6_port;
+}
+
+/*
+ * Test if two socket addresses represent the same actual socket,
+ * by comparing (only) relevant fields.
+ */
+static int nfs_sockaddr_cmp(const struct sockaddr *sa1,
+			    const struct sockaddr *sa2)
+{
+	if (sa1->sa_family != sa2->sa_family)
+		return 0;
+
+	switch (sa1->sa_family) {
+	case AF_INET:
+		return nfs_sockaddr_cmp_ip4((const struct sockaddr_in *) sa1,
+					    (const struct sockaddr_in *) sa2);
+	case AF_INET6:
+		return nfs_sockaddr_cmp_ip6((const struct sockaddr_in6 *) sa1,
+					    (const struct sockaddr_in6 *) sa2);
+	}
+	return 0;
+}
+
+/*
  * Find a client by IP address and protocol version
  * - returns NULL if no such client
  */
@@ -344,8 +397,10 @@ struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
 static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *data)
 {
 	struct nfs_client *clp;
+	const struct sockaddr *sap = data->addr;
 
 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
+		const struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;
 		/* Don't match clients that failed to initialise properly */
 		if (clp->cl_cons_state < 0)
 			continue;
@@ -358,7 +413,7 @@ static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *dat
 			continue;
 
 		/* Match the full socket address */
-		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
+		if (!nfs_sockaddr_cmp(sap, clap))
 			continue;
 
 		atomic_inc(&clp->cl_count);



-- 
Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Re: [PATCH] Bug 11061, NFS mounts dropped

[Chuck Lever]

I'm still not sure the IPv6 piece is completely correct, but after  
this patch is applied, IPv4 behavior is definitely better.  Since we  
can refine the IPv6 behavior as needed in subsequent patches:

   Acked-by: Chuck Lever <chuck.lever@oracle.com>

On Feb 18, 2009, at Feb 18, 2009, 7:41 AM, Ian Dall wrote:
> On Tue, 2009-02-17 at 12:29 -0500, Chuck Lever wrote:
>> Hi Ian-
>>
>> On Feb 17, 2009, at Feb 17, 2009, 7:03 AM, Ian Dall wrote:
>>> On Mon, 2009-02-16 at 11:24 -0500, Chuck Lever wrote:
>>>>
>>>> I've thought more about this.  I think we don't want the scope ID
>>>> comparison here at all.  The scope ID is not really part of an IPv6
>>>> address.  A unique host address is contained only in the sin6_addr
>>>> field; the scope ID is simply local routing information.  I can't
>>>> think of a case where two unique remotes would have identical link-
>>>> local addresses.
>>>
>>> Isn't that exactly what makes link local addresses, well, link  
>>> local?
>>
>> No, an IPv6 address is link-local by virtue of having the
>> IPV6_ADDR_LINKLOCAL bit set.  Scope IDs came along later.
>
> That is just an API and nothing to do with the semantics of "link
> local". Previously it was unspecified how to denote a link, now  
> there is
> a sockaddr field (which is largely undocumented, so not much of a
> change). There is nothing in the IPv6 standard which says that the
> kernel has to use sockaddr internally, but clearly it has to keep  
> track
> of what link a link local address is on somehow!
>
>> Generally the way a link-local address is formed on Linux is by using
>> the link's MAC address to form a unique 64-bit EUID, and  
>> concatenating
>> that to a standard link-local prefix, usually fe80:: .  Each NIC on a
>> host therefore has it's own link-local address, generally speaking.
>
> Yes. But not all hosts on the "link" need be linux. Also, not all lans
> have to be ethernet.
>
>>> http://books.google.com/books?id=6nNjcItz6H4C&pg=PA53&lpg=PA53&dq=sin6_scope_id+%22link+local%22&source=bl&ots=MFyiwYwO5I&sig=hZDvlVcJ8hLOwt7EXoL4lEzT4V4&hl=en&ei=eqCaSdiGD8PDkAWskMmZCw&sa=X&oi=book_result&resnum=7&ct=result
>>>
>>> Note especially, section 1.8.1 "Since link local addresses may not  
>>> be
>>> unique on different links ..."
>>
>> Does that imply that unique hosts can have the same link-local
>> address, or simply that the same link-local address is permitted to
>> appear on different links?
>
> Both I reckon. It is the address which is link local, not  
> (necessarilly)
> the host. But if they are link local addresses on the same host but
> different links, which are other wise the same, everything else on the
> network should treat them just as if they were separate hosts. This is
> no different to a host having two ordinary IP (v4 or v6) addresses.
>
> Potentially a host might run two nfs servers listening on two  
> interfaces
> on two separate lans with the same link local address, serving up
> different files. The servers are on the same host, but that should be
> completely opaque to clients.
>
>> I think the correct way to proceed here is to address the original
>> problem without adding the scope ID check, then explore the scope ID
>> issue separately.  The presenting problem in 11061 affects IPv4
>> addresses too if I'm not mistaken, and IPv4 is fairly widely  
>> deployed,
>> so it needs to be fixed now.
>>
>> If scope ID matching is a problem in nfs_match_client, then it is  
>> also
>> an issue for other places in the NFS client, and should be fixed in
>> all those places at once, in a single patch that documents the scope
>> ID matching requirement.
>
> From: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>
> Addresses: http://bugzilla.kernel.org/show_bug.cgi?id=11061
>
> sockaddr structures can't be reliably compared using memcmp() because
> there are padding bytes in the structure which can't be guaranteed to
> be the same even when the sockaddr structures refer to the same
> socket. Instead compare all the relevant fields. In the case of IPv6
> sin6_flowinfo is not compared because it only affects QoS and
> sin6_scope_id is currently not supported.
>
> Signed-off-by: Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 9b728f3..9ac4c6b 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -273,6 +273,59 @@ static int nfs_sockaddr_match_ipaddr(const  
> struct sockaddr *sa1,
> #endif
>
> /*
> + * Test if two ip4 socket addresses refer to the same socket, by
> + * comparing relevant fields. The padding bytes specifically, are
> + * not compared.
> + *
> + * The caller should ensure both socket addresses are AF_INET.
> + */
> +static int nfs_sockaddr_cmp_ip4(const struct sockaddr_in *saddr1,
> +				const struct sockaddr_in *saddr2)
> +{
> +	if (saddr1->sin_addr.s_addr != saddr2->sin_addr.s_addr)
> +		return 0;
> +	return saddr1->sin_port == saddr2->sin_port;
> +}
> +
> +/*
> + * Test if two ip6 socket addresses refer to the same socket by
> + * comparing relevant fields. The padding bytes specifically, are not
> + * compared. sin6_flowinfo is not compared because it only affects  
> QoS
> + * and sin6_scope_id is currently not supported.
> + *
> + * The caller should ensure both socket addresses are AF_INET6.
> + */
> +static int nfs_sockaddr_cmp_ip6(const struct sockaddr_in6 *saddr1,
> +				const struct sockaddr_in6 *saddr2)
> +{
> +	if (!ipv6_addr_equal(&saddr1->sin6_addr,
> +			     &saddr1->sin6_addr))
> +		return 0;
> +	return saddr1->sin6_port == saddr2->sin6_port;
> +}
> +
> +/*
> + * Test if two socket addresses represent the same actual socket,
> + * by comparing (only) relevant fields.
> + */
> +static int nfs_sockaddr_cmp(const struct sockaddr *sa1,
> +			    const struct sockaddr *sa2)
> +{
> +	if (sa1->sa_family != sa2->sa_family)
> +		return 0;
> +
> +	switch (sa1->sa_family) {
> +	case AF_INET:
> +		return nfs_sockaddr_cmp_ip4((const struct sockaddr_in *) sa1,
> +					    (const struct sockaddr_in *) sa2);
> +	case AF_INET6:
> +		return nfs_sockaddr_cmp_ip6((const struct sockaddr_in6 *) sa1,
> +					    (const struct sockaddr_in6 *) sa2);
> +	}
> +	return 0;
> +}
> +
> +/*
>  * Find a client by IP address and protocol version
>  * - returns NULL if no such client
>  */
> @@ -344,8 +397,10 @@ struct nfs_client *nfs_find_client_next(struct  
> nfs_client *clp)
> static struct nfs_client *nfs_match_client(const struct  
> nfs_client_initdata *data)
> {
> 	struct nfs_client *clp;
> +	const struct sockaddr *sap = data->addr;
>
> 	list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
> +		const struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;
> 		/* Don't match clients that failed to initialise properly */
> 		if (clp->cl_cons_state < 0)
> 			continue;
> @@ -358,7 +413,7 @@ static struct nfs_client *nfs_match_client(const  
> struct nfs_client_initdata *dat
> 			continue;
>
> 		/* Match the full socket address */
> -		if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
> +		if (!nfs_sockaddr_cmp(sap, clap))
> 			continue;
>
> 		atomic_inc(&clp->cl_count);

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] Bug 11061, NFS mounts dropped

[Ian Dall]

On Thu, 2009-02-19 at 11:28 -0500, Chuck Lever wrote:
> I'm still not sure the IPv6 piece is completely correct, but after  
> this patch is applied, IPv4 behavior is definitely better.  Since we  
> can refine the IPv6 behavior as needed in subsequent patches:
> 
>    Acked-by: Chuck Lever <chuck.lever@oracle.com>

What happens now? Is there anything more I need to do?

Regards,
Ian


-- 
Ian Dall <ian-Xg6/AsmM8Z5B3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>