[PATCH] NFS: fix nfs_parse_ip_address() corner case

[PATCH] NFS: fix nfs_parse_ip_address() corner case

[Chuck Lever]

Bruce observed that nfs_parse_ip_address() will successfully parse an IPv6
address that looks like this:

  "::1%"

A scope delimiter is present, but there is no scope ID following it.
This is harmless, as it would simply set the scope ID to zero.  However,
in some cases we would like to flag this as an improperly formed
address.

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

 fs/nfs/super.c |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 5b2aa04..f73e068 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -727,19 +727,21 @@ static void nfs_parse_ipv4_address(char *string, size_t str_len,
 #define IPV6_SCOPE_DELIMITER	'%'
 
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
-static void nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
-				    const char *delim,
-				    struct sockaddr_in6 *sin6)
+static int nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
+				   const char *delim,
+				   struct sockaddr_in6 *sin6)
 {
 	char *p;
 	size_t len;
 
 	if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL))
-		return ;
+		return 0;
 	if (*delim != IPV6_SCOPE_DELIMITER)
-		return;
-
+		return 0;
 	len = (string + str_len) - delim - 1;
+	if (len == 0)
+		return 0;
+
 	p = kstrndup(delim + 1, len, GFP_KERNEL);
 	if (p) {
 		unsigned long scope_id = 0;
@@ -758,6 +760,8 @@ static void nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
 		sin6->sin6_scope_id = scope_id;
 		dfprintk(MOUNT, "NFS: IPv6 scope ID = %lu\n", scope_id);
 	}
+
+	return 1;
 }
 
 static void nfs_parse_ipv6_address(char *string, size_t str_len,
@@ -773,9 +777,11 @@ static void nfs_parse_ipv6_address(char *string, size_t str_len,
 
 		sin6->sin6_family = AF_INET6;
 		*addr_len = sizeof(*sin6);
-		if (in6_pton(string, str_len, addr, IPV6_SCOPE_DELIMITER, &delim)) {
-			nfs_parse_ipv6_scope_id(string, str_len, delim, sin6);
-			return;
+		if (in6_pton(string, str_len, addr,
+					IPV6_SCOPE_DELIMITER, &delim)) {
+			if (nfs_parse_ipv6_scope_id(string,
+							str_len, delim, sin6))
+				return;
 		}
 	}
 

Re: [PATCH] NFS: fix nfs_parse_ip_address() corner case

[J. Bruce Fields]

On Fri, Aug 22, 2008 at 02:24:22PM -0400, Chuck Lever wrote:
> Bruce observed that nfs_parse_ip_address() will successfully parse an IPv6
> address that looks like this:
> 
>   "::1%"
> 
> A scope delimiter is present, but there is no scope ID following it.
> This is harmless, as it would simply set the scope ID to zero.  However,
> in some cases we would like to flag this as an improperly formed
> address.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/nfs/super.c |   24 +++++++++++++++---------
>  1 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 5b2aa04..f73e068 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -727,19 +727,21 @@ static void nfs_parse_ipv4_address(char *string, size_t str_len,
>  #define IPV6_SCOPE_DELIMITER	'%'
>  
>  #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> -static void nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
> -				    const char *delim,
> -				    struct sockaddr_in6 *sin6)
> +static int nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
> +				   const char *delim,
> +				   struct sockaddr_in6 *sin6)
>  {
>  	char *p;
>  	size_t len;
>  
>  	if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL))
> -		return ;
> +		return 0;
>  	if (*delim != IPV6_SCOPE_DELIMITER)
> -		return;
> -
> +		return 0;

What happens in the case where there's no scope delimiter?  In that case
can't *delim correctly point to something else here?

Arguably kstrndup() and dev_get_by_name() failures should also result in
parser failures.  It seems safer to me to reject bad addresses than to
try to use them anyway (possibly resulting in mounting a different
server from what was intended).

--b.

>  	len = (string + str_len) - delim - 1;
> +	if (len == 0)
> +		return 0;
> +
>  	p = kstrndup(delim + 1, len, GFP_KERNEL);
>  	if (p) {
>  		unsigned long scope_id = 0;
> @@ -758,6 +760,8 @@ static void nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
>  		sin6->sin6_scope_id = scope_id;
>  		dfprintk(MOUNT, "NFS: IPv6 scope ID = %lu\n", scope_id);
>  	}
> +
> +	return 1;
>  }
>  
>  static void nfs_parse_ipv6_address(char *string, size_t str_len,
> @@ -773,9 +777,11 @@ static void nfs_parse_ipv6_address(char *string, size_t str_len,
>  
>  		sin6->sin6_family = AF_INET6;
>  		*addr_len = sizeof(*sin6);
> -		if (in6_pton(string, str_len, addr, IPV6_SCOPE_DELIMITER, &delim)) {
> -			nfs_parse_ipv6_scope_id(string, str_len, delim, sin6);
> -			return;
> +		if (in6_pton(string, str_len, addr,
> +					IPV6_SCOPE_DELIMITER, &delim)) {
> +			if (nfs_parse_ipv6_scope_id(string,
> +							str_len, delim, sin6))
> +				return;
>  		}
>  	}
>  
> 

Re: [PATCH] NFS: fix nfs_parse_ip_address() corner case

[Chuck Lever]

On Aug 26, 2008, at Aug 26, 2008, 2:39 PM, J. Bruce Fields wrote:
> On Fri, Aug 22, 2008 at 02:24:22PM -0400, Chuck Lever wrote:
>> Bruce observed that nfs_parse_ip_address() will successfully parse  
>> an IPv6
>> address that looks like this:
>>
>>  "::1%"
>>
>> A scope delimiter is present, but there is no scope ID following it.
>> This is harmless, as it would simply set the scope ID to zero.   
>> However,
>> in some cases we would like to flag this as an improperly formed
>> address.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/nfs/super.c |   24 +++++++++++++++---------
>> 1 files changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index 5b2aa04..f73e068 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -727,19 +727,21 @@ static void nfs_parse_ipv4_address(char  
>> *string, size_t str_len,
>> #define IPV6_SCOPE_DELIMITER	'%'
>>
>> #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>> -static void nfs_parse_ipv6_scope_id(const char *string, const  
>> size_t str_len,
>> -				    const char *delim,
>> -				    struct sockaddr_in6 *sin6)
>> +static int nfs_parse_ipv6_scope_id(const char *string, const  
>> size_t str_len,
>> +				   const char *delim,
>> +				   struct sockaddr_in6 *sin6)
>> {
>> 	char *p;
>> 	size_t len;
>>
>> 	if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL))
>> -		return ;
>> +		return 0;
>> 	if (*delim != IPV6_SCOPE_DELIMITER)
>> -		return;
>> -
>> +		return 0;
>
> What happens in the case where there's no scope delimiter?  In that  
> case
> can't *delim correctly point to something else here?

When we get to nfs_parse_ipv6_scope_id(), *delim points to the first  
character following the 128-bit IPv6 address string.  We should fail  
if *delim doesn't point to either '%' or '\0'.  So we need another  
check here -- succeed immediately if *delim points to '\0'.

Then, I think we should check if the address is link-local _after_ we  
know we have a valid scope delimiter.

> Arguably kstrndup() and dev_get_by_name() failures should also  
> result in
> parser failures.  It seems safer to me to reject bad addresses than to
> try to use them anyway (possibly resulting in mounting a different
> server from what was intended).

Well, if kstrndup() fails, that doesn't necessarily mean we have a bad  
address; simply that there wasn't memory to parse it.  But it's  
reasonable to return 0 in that case.

If dev_get_by_name() fails, then the next step is to check if we were  
passed a numeric value instead of a device name.  If the strtoul()  
call fails to find a real numeric there, then yes, address parsing  
should fail.

If you agree I will repost with corrections.

>
>
> --b.
>
>> 	len = (string + str_len) - delim - 1;
>> +	if (len == 0)
>> +		return 0;
>> +
>> 	p = kstrndup(delim + 1, len, GFP_KERNEL);
>> 	if (p) {
>> 		unsigned long scope_id = 0;
>> @@ -758,6 +760,8 @@ static void nfs_parse_ipv6_scope_id(const char  
>> *string, const size_t str_len,
>> 		sin6->sin6_scope_id = scope_id;
>> 		dfprintk(MOUNT, "NFS: IPv6 scope ID = %lu\n", scope_id);
>> 	}
>> +
>> +	return 1;
>> }
>>
>> static void nfs_parse_ipv6_address(char *string, size_t str_len,
>> @@ -773,9 +777,11 @@ static void nfs_parse_ipv6_address(char  
>> *string, size_t str_len,
>>
>> 		sin6->sin6_family = AF_INET6;
>> 		*addr_len = sizeof(*sin6);
>> -		if (in6_pton(string, str_len, addr, IPV6_SCOPE_DELIMITER,  
>> &delim)) {
>> -			nfs_parse_ipv6_scope_id(string, str_len, delim, sin6);
>> -			return;
>> +		if (in6_pton(string, str_len, addr,
>> +					IPV6_SCOPE_DELIMITER, &delim)) {
>> +			if (nfs_parse_ipv6_scope_id(string,
>> +							str_len, delim, sin6))
>> +				return;
>> 		}
>> 	}
>>
>>

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

Re: [PATCH] NFS: fix nfs_parse_ip_address() corner case

[J. Bruce Fields]

On Tue, Aug 26, 2008 at 04:24:12PM -0400, Chuck Lever wrote:
> On Aug 26, 2008, at Aug 26, 2008, 2:39 PM, J. Bruce Fields wrote:
>> On Fri, Aug 22, 2008 at 02:24:22PM -0400, Chuck Lever wrote:
>>> Bruce observed that nfs_parse_ip_address() will successfully parse  
>>> an IPv6
>>> address that looks like this:
>>>
>>>  "::1%"
>>>
>>> A scope delimiter is present, but there is no scope ID following it.
>>> This is harmless, as it would simply set the scope ID to zero.   
>>> However,
>>> in some cases we would like to flag this as an improperly formed
>>> address.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> fs/nfs/super.c |   24 +++++++++++++++---------
>>> 1 files changed, 15 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>> index 5b2aa04..f73e068 100644
>>> --- a/fs/nfs/super.c
>>> +++ b/fs/nfs/super.c
>>> @@ -727,19 +727,21 @@ static void nfs_parse_ipv4_address(char  
>>> *string, size_t str_len,
>>> #define IPV6_SCOPE_DELIMITER	'%'
>>>
>>> #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>>> -static void nfs_parse_ipv6_scope_id(const char *string, const  
>>> size_t str_len,
>>> -				    const char *delim,
>>> -				    struct sockaddr_in6 *sin6)
>>> +static int nfs_parse_ipv6_scope_id(const char *string, const size_t 
>>> str_len,
>>> +				   const char *delim,
>>> +				   struct sockaddr_in6 *sin6)
>>> {
>>> 	char *p;
>>> 	size_t len;
>>>
>>> 	if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL))
>>> -		return ;
>>> +		return 0;
>>> 	if (*delim != IPV6_SCOPE_DELIMITER)
>>> -		return;
>>> -
>>> +		return 0;
>>
>> What happens in the case where there's no scope delimiter?  In that  
>> case
>> can't *delim correctly point to something else here?
>
> When we get to nfs_parse_ipv6_scope_id(), *delim points to the first  
> character following the 128-bit IPv6 address string.  We should fail if 
> *delim doesn't point to either '%' or '\0'.  So we need another check 
> here -- succeed immediately if *delim points to '\0'.

The string isn't necessarily null-delimited.

> Then, I think we should check if the address is link-local _after_ we  
> know we have a valid scope delimiter.
>
>> Arguably kstrndup() and dev_get_by_name() failures should also result 
>> in
>> parser failures.  It seems safer to me to reject bad addresses than to
>> try to use them anyway (possibly resulting in mounting a different
>> server from what was intended).
>
> Well, if kstrndup() fails, that doesn't necessarily mean we have a bad  
> address; simply that there wasn't memory to parse it.  But it's  
> reasonable to return 0 in that case.
>
> If dev_get_by_name() fails, then the next step is to check if we were  
> passed a numeric value instead of a device name.  If the strtoul() call 
> fails to find a real numeric there, then yes, address parsing should 
> fail.

What does %numeric-value mean?

--b.

>
> If you agree I will repost with corrections.

Re: [PATCH] NFS: fix nfs_parse_ip_address() corner case

[Chuck Lever]

On Aug 26, 2008, at Aug 26, 2008, 4:28 PM, J. Bruce Fields wrote:
> On Tue, Aug 26, 2008 at 04:24:12PM -0400, Chuck Lever wrote:
>> On Aug 26, 2008, at Aug 26, 2008, 2:39 PM, J. Bruce Fields wrote:
>>> On Fri, Aug 22, 2008 at 02:24:22PM -0400, Chuck Lever wrote:
>>>> Bruce observed that nfs_parse_ip_address() will successfully parse
>>>> an IPv6
>>>> address that looks like this:
>>>>
>>>> "::1%"
>>>>
>>>> A scope delimiter is present, but there is no scope ID following  
>>>> it.
>>>> This is harmless, as it would simply set the scope ID to zero.
>>>> However,
>>>> in some cases we would like to flag this as an improperly formed
>>>> address.
>>>>
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>>
>>>> fs/nfs/super.c |   24 +++++++++++++++---------
>>>> 1 files changed, 15 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>>> index 5b2aa04..f73e068 100644
>>>> --- a/fs/nfs/super.c
>>>> +++ b/fs/nfs/super.c
>>>> @@ -727,19 +727,21 @@ static void nfs_parse_ipv4_address(char
>>>> *string, size_t str_len,
>>>> #define IPV6_SCOPE_DELIMITER	'%'
>>>>
>>>> #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>>>> -static void nfs_parse_ipv6_scope_id(const char *string, const
>>>> size_t str_len,
>>>> -				    const char *delim,
>>>> -				    struct sockaddr_in6 *sin6)
>>>> +static int nfs_parse_ipv6_scope_id(const char *string, const  
>>>> size_t
>>>> str_len,
>>>> +				   const char *delim,
>>>> +				   struct sockaddr_in6 *sin6)
>>>> {
>>>> 	char *p;
>>>> 	size_t len;
>>>>
>>>> 	if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL))
>>>> -		return ;
>>>> +		return 0;
>>>> 	if (*delim != IPV6_SCOPE_DELIMITER)
>>>> -		return;
>>>> -
>>>> +		return 0;
>>>
>>> What happens in the case where there's no scope delimiter?  In that
>>> case
>>> can't *delim correctly point to something else here?
>>
>> When we get to nfs_parse_ipv6_scope_id(), *delim points to the first
>> character following the 128-bit IPv6 address string.  We should  
>> fail if
>> *delim doesn't point to either '%' or '\0'.  So we need another check
>> here -- succeed immediately if *delim points to '\0'.
>
> The string isn't necessarily null-delimited.

OK, we just need to take str_len into account.

>> Then, I think we should check if the address is link-local _after_ we
>> know we have a valid scope delimiter.
>>
>>> Arguably kstrndup() and dev_get_by_name() failures should also  
>>> result
>>> in
>>> parser failures.  It seems safer to me to reject bad addresses  
>>> than to
>>> try to use them anyway (possibly resulting in mounting a different
>>> server from what was intended).
>>
>> Well, if kstrndup() fails, that doesn't necessarily mean we have a  
>> bad
>> address; simply that there wasn't memory to parse it.  But it's
>> reasonable to return 0 in that case.
>>
>> If dev_get_by_name() fails, then the next step is to check if we were
>> passed a numeric value instead of a device name.  If the strtoul()  
>> call
>> fails to find a real numeric there, then yes, address parsing should
>> fail.
>
> What does %numeric-value mean?

'%eth0' means find and use the scope ID of the eth0 device.  '%2'  
means use the scope ID 2.  If eth0 has a interface index of 2, then  
both of these are equivalent.  The numeric index is the scope ID.

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

Re: [PATCH] NFS: fix nfs_parse_ip_address() corner case

[J. Bruce Fields]

On Tue, Aug 26, 2008 at 04:36:10PM -0400, Chuck Lever wrote:
> On Aug 26, 2008, at Aug 26, 2008, 4:28 PM, J. Bruce Fields wrote:
>> On Tue, Aug 26, 2008 at 04:24:12PM -0400, Chuck Lever wrote:
>>> On Aug 26, 2008, at Aug 26, 2008, 2:39 PM, J. Bruce Fields wrote:
>>>> On Fri, Aug 22, 2008 at 02:24:22PM -0400, Chuck Lever wrote:
>>>>> Bruce observed that nfs_parse_ip_address() will successfully parse
>>>>> an IPv6
>>>>> address that looks like this:
>>>>>
>>>>> "::1%"
>>>>>
>>>>> A scope delimiter is present, but there is no scope ID following  
>>>>> it.
>>>>> This is harmless, as it would simply set the scope ID to zero.
>>>>> However,
>>>>> in some cases we would like to flag this as an improperly formed
>>>>> address.
>>>>>
>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>> ---
>>>>>
>>>>> fs/nfs/super.c |   24 +++++++++++++++---------
>>>>> 1 files changed, 15 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>>>> index 5b2aa04..f73e068 100644
>>>>> --- a/fs/nfs/super.c
>>>>> +++ b/fs/nfs/super.c
>>>>> @@ -727,19 +727,21 @@ static void nfs_parse_ipv4_address(char
>>>>> *string, size_t str_len,
>>>>> #define IPV6_SCOPE_DELIMITER	'%'
>>>>>
>>>>> #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>>>>> -static void nfs_parse_ipv6_scope_id(const char *string, const
>>>>> size_t str_len,
>>>>> -				    const char *delim,
>>>>> -				    struct sockaddr_in6 *sin6)
>>>>> +static int nfs_parse_ipv6_scope_id(const char *string, const  
>>>>> size_t
>>>>> str_len,
>>>>> +				   const char *delim,
>>>>> +				   struct sockaddr_in6 *sin6)
>>>>> {
>>>>> 	char *p;
>>>>> 	size_t len;
>>>>>
>>>>> 	if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL))
>>>>> -		return ;
>>>>> +		return 0;
>>>>> 	if (*delim != IPV6_SCOPE_DELIMITER)
>>>>> -		return;
>>>>> -
>>>>> +		return 0;
>>>>
>>>> What happens in the case where there's no scope delimiter?  In that
>>>> case
>>>> can't *delim correctly point to something else here?
>>>
>>> When we get to nfs_parse_ipv6_scope_id(), *delim points to the first
>>> character following the 128-bit IPv6 address string.  We should fail 
>>> if
>>> *delim doesn't point to either '%' or '\0'.  So we need another check
>>> here -- succeed immediately if *delim points to '\0'.
>>
>> The string isn't necessarily null-delimited.
>
> OK, we just need to take str_len into account.

OK.  A minor nit, but I'd also find this  a little easier to read if it
attempted to stick to the pattern

	if (something_bad)
		fail;
	if (something_else_bad)
		fail;
	...
	succeed;

rather than

	if (successful) {
		if (still_successful) {
			...
			succeed;
		}
	}
	fail;

--b.

>
>>> Then, I think we should check if the address is link-local _after_ we
>>> know we have a valid scope delimiter.
>>>
>>>> Arguably kstrndup() and dev_get_by_name() failures should also  
>>>> result
>>>> in
>>>> parser failures.  It seems safer to me to reject bad addresses  
>>>> than to
>>>> try to use them anyway (possibly resulting in mounting a different
>>>> server from what was intended).
>>>
>>> Well, if kstrndup() fails, that doesn't necessarily mean we have a  
>>> bad
>>> address; simply that there wasn't memory to parse it.  But it's
>>> reasonable to return 0 in that case.
>>>
>>> If dev_get_by_name() fails, then the next step is to check if we were
>>> passed a numeric value instead of a device name.  If the strtoul()  
>>> call
>>> fails to find a real numeric there, then yes, address parsing should
>>> fail.
>>
>> What does %numeric-value mean?
>
> '%eth0' means find and use the scope ID of the eth0 device.  '%2' means 
> use the scope ID 2.  If eth0 has a interface index of 2, then both of 
> these are equivalent.  The numeric index is the scope ID.
>
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com

[PATCH] NFS: fix nfs_parse_ip_address() corner case

[Chuck Lever]

Bruce observed that nfs_parse_ip_address() will successfully parse an
IPv6 address that looks like this:

  "::1%"

A scope delimiter is present, but there is no scope ID following it.
This is harmless, as it would simply set the scope ID to zero.  However,
in some cases we would like to flag this as an improperly formed
address.

We are now also careful to reject addresses where garbage follows the
address (up to the length of the string), instead of ignoring the
non-address characters; and where the scope ID is nonsense (not a valid
device name, but also not numeric).  Before, both of these cases would
result in a harmless zero scope ID.
    
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

This has seen some local testing.  I'm not sure it gets every corner case
right, as the mount.nfs4 command passes the server name/address through DNS,
so it already doesn't allow patently bogus addresses like the above example.

But I thought I would post this for comments.

 fs/nfs/super.c |   37 ++++++++++++++++++++++++-------------
 1 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 5b2aa04..92de5e2 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -727,19 +727,22 @@ static void nfs_parse_ipv4_address(char *string, size_t str_len,
 #define IPV6_SCOPE_DELIMITER	'%'
 
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
-static void nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
-				    const char *delim,
-				    struct sockaddr_in6 *sin6)
+static int nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
+				   const char *delim,
+				   struct sockaddr_in6 *sin6)
 {
+	size_t len = (string + str_len) - delim - 1;
 	char *p;
-	size_t len;
 
-	if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL))
-		return ;
+	if (len == 0)
+		return 1;
+
 	if (*delim != IPV6_SCOPE_DELIMITER)
-		return;
+		return 0;
+
+	if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL))
+		return 0;
 
-	len = (string + str_len) - delim - 1;
 	p = kstrndup(delim + 1, len, GFP_KERNEL);
 	if (p) {
 		unsigned long scope_id = 0;
@@ -750,14 +753,20 @@ static void nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
 			scope_id = dev->ifindex;
 			dev_put(dev);
 		} else {
-			/* scope_id is set to zero on error */
-			strict_strtoul(p, 10, &scope_id);
+			if (strict_strtoul(p, 10, &scope_id) == 0) {
+				kfree(p);
+				return 0;
+			}
 		}
 
 		kfree(p);
+
 		sin6->sin6_scope_id = scope_id;
 		dfprintk(MOUNT, "NFS: IPv6 scope ID = %lu\n", scope_id);
+		return 1;
 	}
+
+	return 0;
 }
 
 static void nfs_parse_ipv6_address(char *string, size_t str_len,
@@ -773,9 +782,11 @@ static void nfs_parse_ipv6_address(char *string, size_t str_len,
 
 		sin6->sin6_family = AF_INET6;
 		*addr_len = sizeof(*sin6);
-		if (in6_pton(string, str_len, addr, IPV6_SCOPE_DELIMITER, &delim)) {
-			nfs_parse_ipv6_scope_id(string, str_len, delim, sin6);
-			return;
+		if (in6_pton(string, str_len, addr,
+					IPV6_SCOPE_DELIMITER, &delim) != 0) {
+			if (nfs_parse_ipv6_scope_id(string, str_len,
+							delim, sin6) != 0)
+				return;
 		}
 	}
 

Re: [PATCH] NFS: fix nfs_parse_ip_address() corner case

[J. Bruce Fields]

On Wed, Sep 03, 2008 at 04:35:57PM -0400, Chuck Lever wrote:
> Bruce observed that nfs_parse_ip_address() will successfully parse an
> IPv6 address that looks like this:
> 
>   "::1%"
> 
> A scope delimiter is present, but there is no scope ID following it.
> This is harmless, as it would simply set the scope ID to zero.  However,
> in some cases we would like to flag this as an improperly formed
> address.
> 
> We are now also careful to reject addresses where garbage follows the
> address (up to the length of the string), instead of ignoring the
> non-address characters; and where the scope ID is nonsense (not a valid
> device name, but also not numeric).  Before, both of these cases would
> result in a harmless zero scope ID.

Slightly irrelevant, but same comment as before--wouldn't it be easier
to follow the logic if instead of:

	p = kstrndup(...)
	if (p) {
		do stuff for successful case
		....
		return 1;
	}

	return 0;

it were:

	p = kstrndup(...)
	if (!p)
		return 0;
	do stuff for successful case
	...
	return 1;

e.g., the below, on top of yours. (Untested.)

Might also combine the two final exits in the usual way:

	out:
		kfree(p);
		return ret;

--b.

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index a9f5a12..fcee897 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -733,6 +733,8 @@ static int nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
 {
 	size_t len = (string + str_len) - delim - 1;
 	char *p;
+	unsigned long scope_id = 0;
+	struct net_device *dev;
 
 	if (len == 0)
 		return 1;
@@ -744,29 +746,25 @@ static int nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
 		return 0;
 
 	p = kstrndup(delim + 1, len, GFP_KERNEL);
-	if (p) {
-		unsigned long scope_id = 0;
-		struct net_device *dev;
-
-		dev = dev_get_by_name(&init_net, p);
-		if (dev != NULL) {
-			scope_id = dev->ifindex;
-			dev_put(dev);
-		} else {
-			if (strict_strtoul(p, 10, &scope_id) == 0) {
-				kfree(p);
-				return 0;
-			}
-		}
-
-		kfree(p);
+	if (!p)
+		return 0;
 
-		sin6->sin6_scope_id = scope_id;
-		dfprintk(MOUNT, "NFS: IPv6 scope ID = %lu\n", scope_id);
-		return 1;
+	dev = dev_get_by_name(&init_net, p);
+	if (dev != NULL) {
+		scope_id = dev->ifindex;
+		dev_put(dev);
+	} else {
+		if (strict_strtoul(p, 10, &scope_id) == 0) {
+			kfree(p);
+			return 0;
+		}
 	}
 
-	return 0;
+	kfree(p);
+
+	sin6->sin6_scope_id = scope_id;
+	dfprintk(MOUNT, "NFS: IPv6 scope ID = %lu\n", scope_id);
+	return 1;
 }
 
 static void nfs_parse_ipv6_address(char *string, size_t str_len,

Re: [PATCH] NFS: fix nfs_parse_ip_address() corner case

[Chuck Lever]

On Thu, Sep 4, 2008 at 4:23 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Wed, Sep 03, 2008 at 04:35:57PM -0400, Chuck Lever wrote:
>> Bruce observed that nfs_parse_ip_address() will successfully parse an
>> IPv6 address that looks like this:
>>
>>   "::1%"
>>
>> A scope delimiter is present, but there is no scope ID following it.
>> This is harmless, as it would simply set the scope ID to zero.  However,
>> in some cases we would like to flag this as an improperly formed
>> address.
>>
>> We are now also careful to reject addresses where garbage follows the
>> address (up to the length of the string), instead of ignoring the
>> non-address characters; and where the scope ID is nonsense (not a valid
>> device name, but also not numeric).  Before, both of these cases would
>> result in a harmless zero scope ID.
>
> Slightly irrelevant, but same comment as before--wouldn't it be easier
> to follow the logic if instead of:
>
>        p = kstrndup(...)
>        if (p) {
>                do stuff for successful case
>                ....
>                return 1;
>        }
>
>        return 0;
>
> it were:
>
>        p = kstrndup(...)
>        if (!p)
>                return 0;
>        do stuff for successful case
>        ...
>        return 1;

I tried that.  It made this patch about 3x larger than it needed to
be, obscured the changes introduced by the patch, and made it harder
to show that the new logic is correct.  As we are not introducing new
code here but repairing existing issues, such a clean up would need to
be in a separate patch.

I generally prefer to use a structured language as it was intended,
rather than to abuse "goto" as is the trend in the Linux kernel.  By
creating named subroutines instead of using lambda functions and using
brackets creatively and intelligently, we codify the assumptions and
architecture of a set of tasks without excessive comments.  This also
limits the scope of automatic variables and keeps each function nearly
as simple as possible, making the code more robust in the face of
changes over time by multiple forgetful developers.

In addition, using positive instead of negative logic groups the
"success" path together into a few lines rather than spreading it out
over the whole function.  Writing this way is a style preference;
Knuth (?) and others make an argument in favor of it.  Often this is a
good way to ensure error handling is correct (eg memory allocations
are guaranteed to be properly freed in every error case because the
structuring forces it).

C suffers from a lack of proper exception handling constructs, which
would help readability and correct error case handling.  But these
functions are small enough that writing them using structured precepts
is simple enough and doesn't have a strong negative effect on
readability.

Also, the C compiler assumes the "if" expression will evaluate to TRUE
more often than FALSE so it arranges the branch instructions so that
the CPU's branch prediction logic will work more efficiently.  Not
really critical in this particular case.

Finally when practical I try to use a single exit/return point to
reduce code duplication and to arrange the code in advance for easier
debugging, though sometimes that obfuscates.

Though I am not always consistent on these points, I don't see a
strong need to re-arrange as you suggest in this case.

>
> e.g., the below, on top of yours. (Untested.)
>
> Might also combine the two final exits in the usual way:
>
>        out:
>                kfree(p);
>                return ret;
>
> --b.
>
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index a9f5a12..fcee897 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -733,6 +733,8 @@ static int nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
>  {
>        size_t len = (string + str_len) - delim - 1;
>        char *p;
> +       unsigned long scope_id = 0;
> +       struct net_device *dev;
>
>        if (len == 0)
>                return 1;
> @@ -744,29 +746,25 @@ static int nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
>                return 0;
>
>        p = kstrndup(delim + 1, len, GFP_KERNEL);
> -       if (p) {
> -               unsigned long scope_id = 0;
> -               struct net_device *dev;
> -
> -               dev = dev_get_by_name(&init_net, p);
> -               if (dev != NULL) {
> -                       scope_id = dev->ifindex;
> -                       dev_put(dev);
> -               } else {
> -                       if (strict_strtoul(p, 10, &scope_id) == 0) {
> -                               kfree(p);
> -                               return 0;
> -                       }
> -               }
> -
> -               kfree(p);
> +       if (!p)
> +               return 0;
>
> -               sin6->sin6_scope_id = scope_id;
> -               dfprintk(MOUNT, "NFS: IPv6 scope ID = %lu\n", scope_id);
> -               return 1;
> +       dev = dev_get_by_name(&init_net, p);
> +       if (dev != NULL) {
> +               scope_id = dev->ifindex;
> +               dev_put(dev);
> +       } else {
> +               if (strict_strtoul(p, 10, &scope_id) == 0) {
> +                       kfree(p);
> +                       return 0;
> +               }
>        }
>
> -       return 0;
> +       kfree(p);
> +
> +       sin6->sin6_scope_id = scope_id;
> +       dfprintk(MOUNT, "NFS: IPv6 scope ID = %lu\n", scope_id);
> +       return 1;
>  }
>
>  static void nfs_parse_ipv6_address(char *string, size_t str_len,
> --
> 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
>



-- 
"If you simplify your English, you are freed from the worst follies of
orthodoxy."
 -- George Orwell

Re: [PATCH] NFS: fix nfs_parse_ip_address() corner case

[J. Bruce Fields]

On Thu, Sep 04, 2008 at 05:36:17PM -0400, Chuck Lever wrote:
> On Thu, Sep 4, 2008 at 4:23 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > Slightly irrelevant, but same comment as before--wouldn't it be easier
> > to follow the logic if instead of:
> >
> >        p = kstrndup(...)
> >        if (p) {
> >                do stuff for successful case
> >                ....
> >                return 1;
> >        }
> >
> >        return 0;
> >
> > it were:
> >
> >        p = kstrndup(...)
> >        if (!p)
> >                return 0;
> >        do stuff for successful case
> >        ...
> >        return 1;
> 
> I tried that.  It made this patch about 3x larger than it needed to
> be, obscured the changes introduced by the patch, and made it harder
> to show that the new logic is correct.  As we are not introducing new
> code here but repairing existing issues, such a clean up would need to
> be in a separate patch.

I agree.

> I generally prefer to use a structured language as it was intended,
> rather than to abuse "goto" as is the trend in the Linux kernel.

That's a well-established practice, not a trend, and in any case I
didn't add a goto.

> By creating named subroutines instead of using lambda functions

I don't see any difference over named subroutines here?  Does c even
support lambda functions?

To me the second example is obviously more readable, partly since it
helps clear up the return convention ("oh, we're returning 0 on failure
of kstrndup!  0 must mean failure..."), while the 1st delays one of the
two branches of the if longer than necessary.

But I don't care enough to insist.  Take it as an indicator of what'd be
clearer to me for code I have to read a lot, if you like.

--b.