[bug report] nfsd: Protect session creation and client confirm using client_lock

[bug report] nfsd: Protect session creation and client confirm using client_lock

[Dan Carpenter]

Hello Jeff Layton,

The patch d20c11d86d8f: "nfsd: Protect session creation and client
confirm using client_lock" from Jul 30, 2014, leads to the following
Smatch static checker warning:

	net/sunrpc/addr.c:178 rpc_parse_scope_id()
	warn: sleeping in atomic context

net/sunrpc/addr.c
    161 static int rpc_parse_scope_id(struct net *net, const char *buf,
    162                               const size_t buflen, const char *delim,
    163                               struct sockaddr_in6 *sin6)
    164 {
    165         char *p;
    166         size_t len;
    167 
    168         if ((buf + buflen) == delim)
    169                 return 1;
    170 
    171         if (*delim != IPV6_SCOPE_DELIMITER)
    172                 return 0;
    173 
    174         if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL))
    175                 return 0;
    176 
    177         len = (buf + buflen) - delim - 1;
--> 178         p = kmemdup_nul(delim + 1, len, GFP_KERNEL);
    179         if (p) {
    180                 u32 scope_id = 0;
    181                 struct net_device *dev;
    182 
    183                 dev = dev_get_by_name(net, p);
    184                 if (dev != NULL) {
    185                         scope_id = dev->ifindex;
    186                         dev_put(dev);
    187                 } else {
    188                         if (kstrtou32(p, 10, &scope_id) != 0) {
    189                                 kfree(p);
    190                                 return 0;
    191                         }
    192                 }
    193 
    194                 kfree(p);
    195 
    196                 sin6->sin6_scope_id = scope_id;
    197                 return 1;
    198         }
    199 
    200         return 0;
    201 }

The call tree is:

nfsd4_setclientid() <- disables preempt
-> gen_callback()
   -> rpc_uaddr2sockaddr()
      -> rpc_pton()
         -> rpc_pton6()
            -> rpc_parse_scope_id()

The commit added a spin_lock to nfsd4_setclientid().

fs/nfsd/nfs4state.c
  4023                          trace_nfsd_clid_verf_mismatch(conf, rqstp,
  4024                                                        &clverifier);
  4025          } else
  4026                  trace_nfsd_clid_fresh(new);
  4027          new->cl_minorversion = 0;
  4028          gen_callback(new, setclid, rqstp);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

  4029          add_to_unconfirmed(new);
  4030          setclid->se_clientid.cl_boot = new->cl_clientid.cl_boot;
  4031          setclid->se_clientid.cl_id = new->cl_clientid.cl_id;
  4032          memcpy(setclid->se_confirm.data, new->cl_confirm.data, sizeof(setclid->se_confirm.data));
  4033          new = NULL;
  4034          status = nfs_ok;
  4035  out:
  4036          spin_unlock(&nn->client_lock);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is the new lock.

  4037          if (new)
  4038                  free_client(new);
  4039          if (unconf) {
  4040                  trace_nfsd_clid_expire_unconf(&unconf->cl_clientid);
  4041                  expire_client(unconf);
  4042          }
  4043          return status;

regards,
dan carpenter

Re: [bug report] nfsd: Protect session creation and client confirm using client_lock

[Jeff Layton]

On Tue, 2021-09-07 at 11:07 +0300, Dan Carpenter wrote:
> Hello Jeff Layton,
> 
> The patch d20c11d86d8f: "nfsd: Protect session creation and client
> confirm using client_lock" from Jul 30, 2014, leads to the following
> Smatch static checker warning:
> 
> 	net/sunrpc/addr.c:178 rpc_parse_scope_id()
> 	warn: sleeping in atomic context
> 
> net/sunrpc/addr.c
>     161 static int rpc_parse_scope_id(struct net *net, const char *buf,
>     162                               const size_t buflen, const char *delim,
>     163                               struct sockaddr_in6 *sin6)
>     164 {
>     165         char *p;
>     166         size_t len;
>     167 
>     168         if ((buf + buflen) == delim)
>     169                 return 1;
>     170 
>     171         if (*delim != IPV6_SCOPE_DELIMITER)
>     172                 return 0;
>     173 
>     174         if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL))
>     175                 return 0;
>     176 
>     177         len = (buf + buflen) - delim - 1;
> --> 178         p = kmemdup_nul(delim + 1, len, GFP_KERNEL);
>     179         if (p) {
>     180                 u32 scope_id = 0;
>     181                 struct net_device *dev;
>     182 
>     183                 dev = dev_get_by_name(net, p);
>     184                 if (dev != NULL) {
>     185                         scope_id = dev->ifindex;
>     186                         dev_put(dev);
>     187                 } else {
>     188                         if (kstrtou32(p, 10, &scope_id) != 0) {
>     189                                 kfree(p);
>     190                                 return 0;
>     191                         }
>     192                 }
>     193 
>     194                 kfree(p);
>     195 
>     196                 sin6->sin6_scope_id = scope_id;
>     197                 return 1;
>     198         }
>     199 
>     200         return 0;
>     201 }
> 
> The call tree is:
> 
> nfsd4_setclientid() <- disables preempt
> -> gen_callback()
>    -> rpc_uaddr2sockaddr()
>       -> rpc_pton()
>          -> rpc_pton6()
>             -> rpc_parse_scope_id()
> 
> The commit added a spin_lock to nfsd4_setclientid().
> 
> fs/nfsd/nfs4state.c
>   4023                          trace_nfsd_clid_verf_mismatch(conf, rqstp,
>   4024                                                        &clverifier);
>   4025          } else
>   4026                  trace_nfsd_clid_fresh(new);
>   4027          new->cl_minorversion = 0;
>   4028          gen_callback(new, setclid, rqstp);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>   4029          add_to_unconfirmed(new);
>   4030          setclid->se_clientid.cl_boot = new->cl_clientid.cl_boot;
>   4031          setclid->se_clientid.cl_id = new->cl_clientid.cl_id;
>   4032          memcpy(setclid->se_confirm.data, new->cl_confirm.data, sizeof(setclid->se_confirm.data));
>   4033          new = NULL;
>   4034          status = nfs_ok;
>   4035  out:
>   4036          spin_unlock(&nn->client_lock);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This is the new lock.
> 
>   4037          if (new)
>   4038                  free_client(new);
>   4039          if (unconf) {
>   4040                  trace_nfsd_clid_expire_unconf(&unconf->cl_clientid);
>   4041                  expire_client(unconf);
>   4042          }
>   4043          return status;
> 
> regards,
> dan carpenter

(cc'ing Bruce and Chuck)

Wow that is an oldie, but it does seem to be a legit bug.

Probably we should just make rpc_parse_scope_id use an on-stack buffer
instead of calling kmemdup_nul there. Thoughts?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [bug report] nfsd: Protect session creation and client confirm using client_lock

[Chuck Lever III]

> On Sep 7, 2021, at 7:00 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Tue, 2021-09-07 at 11:07 +0300, Dan Carpenter wrote:
>> Hello Jeff Layton,
>> 
>> The patch d20c11d86d8f: "nfsd: Protect session creation and client
>> confirm using client_lock" from Jul 30, 2014, leads to the following
>> Smatch static checker warning:
>> 
>> 	net/sunrpc/addr.c:178 rpc_parse_scope_id()
>> 	warn: sleeping in atomic context
>> 
>> net/sunrpc/addr.c
>>    161 static int rpc_parse_scope_id(struct net *net, const char *buf,
>>    162                               const size_t buflen, const char *delim,
>>    163                               struct sockaddr_in6 *sin6)
>>    164 {
>>    165         char *p;
>>    166         size_t len;
>>    167 
>>    168         if ((buf + buflen) == delim)
>>    169                 return 1;
>>    170 
>>    171         if (*delim != IPV6_SCOPE_DELIMITER)
>>    172                 return 0;
>>    173 
>>    174         if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL))
>>    175                 return 0;
>>    176 
>>    177         len = (buf + buflen) - delim - 1;
>> --> 178         p = kmemdup_nul(delim + 1, len, GFP_KERNEL);
>>    179         if (p) {
>>    180                 u32 scope_id = 0;
>>    181                 struct net_device *dev;
>>    182 
>>    183                 dev = dev_get_by_name(net, p);
>>    184                 if (dev != NULL) {
>>    185                         scope_id = dev->ifindex;
>>    186                         dev_put(dev);
>>    187                 } else {
>>    188                         if (kstrtou32(p, 10, &scope_id) != 0) {
>>    189                                 kfree(p);
>>    190                                 return 0;
>>    191                         }
>>    192                 }
>>    193 
>>    194                 kfree(p);
>>    195 
>>    196                 sin6->sin6_scope_id = scope_id;
>>    197                 return 1;
>>    198         }
>>    199 
>>    200         return 0;
>>    201 }
>> 
>> The call tree is:
>> 
>> nfsd4_setclientid() <- disables preempt
>> -> gen_callback()
>>   -> rpc_uaddr2sockaddr()
>>      -> rpc_pton()
>>         -> rpc_pton6()
>>            -> rpc_parse_scope_id()
>> 
>> The commit added a spin_lock to nfsd4_setclientid().
>> 
>> fs/nfsd/nfs4state.c
>>  4023                          trace_nfsd_clid_verf_mismatch(conf, rqstp,
>>  4024                                                        &clverifier);
>>  4025          } else
>>  4026                  trace_nfsd_clid_fresh(new);
>>  4027          new->cl_minorversion = 0;
>>  4028          gen_callback(new, setclid, rqstp);
>>                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> 
>>  4029          add_to_unconfirmed(new);
>>  4030          setclid->se_clientid.cl_boot = new->cl_clientid.cl_boot;
>>  4031          setclid->se_clientid.cl_id = new->cl_clientid.cl_id;
>>  4032          memcpy(setclid->se_confirm.data, new->cl_confirm.data, sizeof(setclid->se_confirm.data));
>>  4033          new = NULL;
>>  4034          status = nfs_ok;
>>  4035  out:
>>  4036          spin_unlock(&nn->client_lock);
>>                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> This is the new lock.
>> 
>>  4037          if (new)
>>  4038                  free_client(new);
>>  4039          if (unconf) {
>>  4040                  trace_nfsd_clid_expire_unconf(&unconf->cl_clientid);
>>  4041                  expire_client(unconf);
>>  4042          }
>>  4043          return status;
>> 
>> regards,
>> dan carpenter
> 
> (cc'ing Bruce and Chuck)
> 
> Wow that is an oldie, but it does seem to be a legit bug.
> 
> Probably we should just make rpc_parse_scope_id use an on-stack buffer
> instead of calling kmemdup_nul there. Thoughts?

We have IPV6_SCOPE_ID_LEN as a maximum size of the scope ID,
and it's not a big value. As long as boundary checking is made
to be sufficient, then a stack residency for the device name
should be safe.


--
Chuck Lever

Re: [bug report] nfsd: Protect session creation and client confirm using client_lock

[Bruce Fields]

On Tue, Sep 07, 2021 at 03:00:23PM +0000, Chuck Lever III wrote:
> We have IPV6_SCOPE_ID_LEN as a maximum size of the scope ID,
> and it's not a big value. As long as boundary checking is made
> to be sufficient, then a stack residency for the device name
> should be safe.

Something like this?  (Or are you making a patch?  I'm not even sure how
to test.)

--b.

diff --git a/net/sunrpc/addr.c b/net/sunrpc/addr.c
index 6e4dbd577a39..d435bffc6199 100644
--- a/net/sunrpc/addr.c
+++ b/net/sunrpc/addr.c
@@ -162,8 +162,10 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
 			      const size_t buflen, const char *delim,
 			      struct sockaddr_in6 *sin6)
 {
-	char *p;
+	char p[IPV6_SCOPE_ID_LEN + 1];
 	size_t len;
+	u32 scope_id = 0;
+	struct net_device *dev;
 
 	if ((buf + buflen) == delim)
 		return 1;
@@ -175,29 +177,23 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
 		return 0;
 
 	len = (buf + buflen) - delim - 1;
-	p = kmemdup_nul(delim + 1, len, GFP_KERNEL);
-	if (p) {
-		u32 scope_id = 0;
-		struct net_device *dev;
-
-		dev = dev_get_by_name(net, p);
-		if (dev != NULL) {
-			scope_id = dev->ifindex;
-			dev_put(dev);
-		} else {
-			if (kstrtou32(p, 10, &scope_id) != 0) {
-				kfree(p);
-				return 0;
-			}
-		}
-
-		kfree(p);
-
-		sin6->sin6_scope_id = scope_id;
-		return 1;
+	if (len > IPV6_SCOPE_ID_LEN)
+		return 0;
+
+	memcpy(p, delim + 1, len);
+	p[len] = 0;
+
+	dev = dev_get_by_name(net, p);
+	if (dev != NULL) {
+		scope_id = dev->ifindex;
+		dev_put(dev);
+	} else {
+		if (kstrtou32(p, 10, &scope_id) != 0)
+			return 0;
 	}
 
-	return 0;
+	sin6->sin6_scope_id = scope_id;
+	return 1;
 }
 
 static size_t rpc_pton6(struct net *net, const char *buf, const size_t buflen,

Re: [bug report] nfsd: Protect session creation and client confirm using client_lock

[Chuck Lever III]

> On Sep 8, 2021, at 5:26 PM, Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Tue, Sep 07, 2021 at 03:00:23PM +0000, Chuck Lever III wrote:
>> We have IPV6_SCOPE_ID_LEN as a maximum size of the scope ID,
>> and it's not a big value. As long as boundary checking is made
>> to be sufficient, then a stack residency for the device name
>> should be safe.
> 
> Something like this?  (Or are you making a patch?

I thought Jeff was going to handle it? More below.


> I'm not even sure how to test.)
> 
> --b.
> 
> diff --git a/net/sunrpc/addr.c b/net/sunrpc/addr.c
> index 6e4dbd577a39..d435bffc6199 100644
> --- a/net/sunrpc/addr.c
> +++ b/net/sunrpc/addr.c
> @@ -162,8 +162,10 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
> 			      const size_t buflen, const char *delim,
> 			      struct sockaddr_in6 *sin6)
> {
> -	char *p;
> +	char p[IPV6_SCOPE_ID_LEN + 1];
> 	size_t len;
> +	u32 scope_id = 0;
> +	struct net_device *dev;
> 
> 	if ((buf + buflen) == delim)
> 		return 1;
> @@ -175,29 +177,23 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
> 		return 0;
> 
> 	len = (buf + buflen) - delim - 1;
> -	p = kmemdup_nul(delim + 1, len, GFP_KERNEL);
> -	if (p) {
> -		u32 scope_id = 0;
> -		struct net_device *dev;
> -
> -		dev = dev_get_by_name(net, p);
> -		if (dev != NULL) {
> -			scope_id = dev->ifindex;
> -			dev_put(dev);
> -		} else {
> -			if (kstrtou32(p, 10, &scope_id) != 0) {
> -				kfree(p);
> -				return 0;
> -			}
> -		}
> -
> -		kfree(p);
> -
> -		sin6->sin6_scope_id = scope_id;
> -		return 1;
> +	if (len > IPV6_SCOPE_ID_LEN)
> +		return 0;
> +
> +	memcpy(p, delim + 1, len);
> +	p[len] = 0;

If I recall correctly, Linus prefers us to use the str*()
functions instead of raw memcpy() in cases like this.


> +
> +	dev = dev_get_by_name(net, p);
> +	if (dev != NULL) {
> +		scope_id = dev->ifindex;
> +		dev_put(dev);
> +	} else {
> +		if (kstrtou32(p, 10, &scope_id) != 0)
> +			return 0;
> 	}
> 
> -	return 0;
> +	sin6->sin6_scope_id = scope_id;
> +	return 1;
> }
> 
> static size_t rpc_pton6(struct net *net, const char *buf, const size_t buflen,

--
Chuck Lever

Re: [bug report] nfsd: Protect session creation and client confirm using client_lock

[Jeff Layton]

On Wed, 2021-09-08 at 21:32 +0000, Chuck Lever III wrote:
> 
> > On Sep 8, 2021, at 5:26 PM, Bruce Fields <bfields@fieldses.org> wrote:
> > 
> > On Tue, Sep 07, 2021 at 03:00:23PM +0000, Chuck Lever III wrote:
> > > We have IPV6_SCOPE_ID_LEN as a maximum size of the scope ID,
> > > and it's not a big value. As long as boundary checking is made
> > > to be sufficient, then a stack residency for the device name
> > > should be safe.
> > 
> > Something like this?  (Or are you making a patch?
> 
> I thought Jeff was going to handle it? More below.
> 

No, sorry... I was just suggesting a potential fix. I'd probably rather
you guys fix it since you're better positioned to test this at the
moment.

> 
> > I'm not even sure how to test.)
> > are
> > --b.
> > 
> > diff --git a/net/sunrpc/addr.c b/net/sunrpc/addr.c
> > index 6e4dbd577a39..d435bffc6199 100644
> > --- a/net/sunrpc/addr.c
> > +++ b/net/sunrpc/addr.c
> > @@ -162,8 +162,10 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
> > 			      const size_t buflen, const char *delim,
> > 			      struct sockaddr_in6 *sin6)
> > {
> > -	char *p;
> > +	char p[IPV6_SCOPE_ID_LEN + 1];
> > 	size_t len;
> > +	u32 scope_id = 0;
> > +	struct net_device *dev;
> > 
> > 	if ((buf + buflen) == delim)
> > 		return 1;
> > @@ -175,29 +177,23 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
> > 		return 0;
> > 
> > 	len = (buf + buflen) - delim - 1;
> > -	p = kmemdup_nul(delim + 1, len, GFP_KERNEL);
> > -	if (p) {
> > -		u32 scope_id = 0;
> > -		struct net_device *dev;
> > -
> > -		dev = dev_get_by_name(net, p);
> > -		if (dev != NULL) {
> > -			scope_id = dev->ifindex;
> > -			dev_put(dev);
> > -		} else {
> > -			if (kstrtou32(p, 10, &scope_id) != 0) {
> > -				kfree(p);
> > -				return 0;
> > -			}
> > -		}
> > -
> > -		kfree(p);
> > -
> > -		sin6->sin6_scope_id = scope_id;
> > -		return 1;
> > +	if (len > IPV6_SCOPE_ID_LEN)
> > +		return 0;
> > +
> > +	memcpy(p, delim + 1, len);
> > +	p[len] = 0;
> 
> If I recall correctly, Linus prefers us to use the str*()
> functions instead of raw memcpy() in cases like this.
> 

I hadn't heard that. If you already know the length to be copied, then
strcpy and the like tend to be less efficient since they continually
have to check for null terminators as they walk the source string.

> 
> > +
> > +	dev = dev_get_by_name(net, p);
> > +	if (dev != NULL) {
> > +		scope_id = dev->ifindex;
> > +		dev_put(dev);
> > +	} else {
> > +		if (kstrtou32(p, 10, &scope_id) != 0)
> > +			return 0;
> > 	}
> > 
> > -	return 0;
> > +	sin6->sin6_scope_id = scope_id;
> > +	return 1;
> > }
> > 
> > static size_t rpc_pton6(struct net *net, const char *buf, const size_t buflen,
> 
> --
> Chuck Lever
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [bug report] nfsd: Protect session creation and client confirm using client_lock

[Chuck Lever III]

> On Sep 9, 2021, at 6:19 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Wed, 2021-09-08 at 21:32 +0000, Chuck Lever III wrote:
>> 
>>> On Sep 8, 2021, at 5:26 PM, Bruce Fields <bfields@fieldses.org> wrote:
>>> 
>>> On Tue, Sep 07, 2021 at 03:00:23PM +0000, Chuck Lever III wrote:
>>>> We have IPV6_SCOPE_ID_LEN as a maximum size of the scope ID,
>>>> and it's not a big value. As long as boundary checking is made
>>>> to be sufficient, then a stack residency for the device name
>>>> should be safe.
>>> 
>>> Something like this?  (Or are you making a patch?
>> 
>> I thought Jeff was going to handle it? More below.
>> 
> 
> No, sorry... I was just suggesting a potential fix. I'd probably rather
> you guys fix it since you're better positioned to test this at the
> moment.
> 
>> 
>>> I'm not even sure how to test.)
>>> are
>>> --b.
>>> 
>>> diff --git a/net/sunrpc/addr.c b/net/sunrpc/addr.c
>>> index 6e4dbd577a39..d435bffc6199 100644
>>> --- a/net/sunrpc/addr.c
>>> +++ b/net/sunrpc/addr.c
>>> @@ -162,8 +162,10 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
>>> 			      const size_t buflen, const char *delim,
>>> 			      struct sockaddr_in6 *sin6)
>>> {
>>> -	char *p;
>>> +	char p[IPV6_SCOPE_ID_LEN + 1];
>>> 	size_t len;
>>> +	u32 scope_id = 0;
>>> +	struct net_device *dev;
>>> 
>>> 	if ((buf + buflen) == delim)
>>> 		return 1;
>>> @@ -175,29 +177,23 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
>>> 		return 0;
>>> 
>>> 	len = (buf + buflen) - delim - 1;
>>> -	p = kmemdup_nul(delim + 1, len, GFP_KERNEL);
>>> -	if (p) {
>>> -		u32 scope_id = 0;
>>> -		struct net_device *dev;
>>> -
>>> -		dev = dev_get_by_name(net, p);
>>> -		if (dev != NULL) {
>>> -			scope_id = dev->ifindex;
>>> -			dev_put(dev);
>>> -		} else {
>>> -			if (kstrtou32(p, 10, &scope_id) != 0) {
>>> -				kfree(p);
>>> -				return 0;
>>> -			}
>>> -		}
>>> -
>>> -		kfree(p);
>>> -
>>> -		sin6->sin6_scope_id = scope_id;
>>> -		return 1;
>>> +	if (len > IPV6_SCOPE_ID_LEN)
>>> +		return 0;
>>> +
>>> +	memcpy(p, delim + 1, len);
>>> +	p[len] = 0;
>> 
>> If I recall correctly, Linus prefers us to use the str*()
>> functions instead of raw memcpy() in cases like this.
> 
> I hadn't heard that.

I'm paraphrasing these:

https://lore.kernel.org/lkml/CAHk-=wj5Pp5J-CAPck22RSQ13k3cEOVnJHUA-WocAZqCJK1BZw@mail.gmail.com/

https://lore.kernel.org/lkml/CAHk-=wjWosrcv2=6m-=YgXRKev=5cnCg-1EhqDpbRXT5z6eQmg@mail.gmail.com/


> If you already know the length to be copied, then
> strcpy and the like tend to be less efficient since they continually
> have to check for null terminators as they walk the source string.

I'm sure there's one str helper that comes close to what we need.
Here efficiency doesn't really matter, and the size of the device
string is always going to be in the single digits.

The priority is correctness.


>>> +
>>> +	dev = dev_get_by_name(net, p);
>>> +	if (dev != NULL) {
>>> +		scope_id = dev->ifindex;
>>> +		dev_put(dev);
>>> +	} else {
>>> +		if (kstrtou32(p, 10, &scope_id) != 0)
>>> +			return 0;
>>> 	}
>>> 
>>> -	return 0;
>>> +	sin6->sin6_scope_id = scope_id;
>>> +	return 1;
>>> }
>>> 
>>> static size_t rpc_pton6(struct net *net, const char *buf, const size_t buflen,
>> 
>> --
>> Chuck Lever
>> 
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever

Re: [bug report] nfsd: Protect session creation and client confirm using client_lock

[Jeff Layton]

On Thu, 2021-09-09 at 14:31 +0000, Chuck Lever III wrote:
> 
> > On Sep 9, 2021, at 6:19 AM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Wed, 2021-09-08 at 21:32 +0000, Chuck Lever III wrote:
> > > 
> > > > On Sep 8, 2021, at 5:26 PM, Bruce Fields <bfields@fieldses.org> wrote:
> > > > 
> > > > On Tue, Sep 07, 2021 at 03:00:23PM +0000, Chuck Lever III wrote:
> > > > > We have IPV6_SCOPE_ID_LEN as a maximum size of the scope ID,
> > > > > and it's not a big value. As long as boundary checking is made
> > > > > to be sufficient, then a stack residency for the device name
> > > > > should be safe.
> > > > 
> > > > Something like this?  (Or are you making a patch?
> > > 
> > > I thought Jeff was going to handle it? More below.
> > > 
> > 
> > No, sorry... I was just suggesting a potential fix. I'd probably rather
> > you guys fix it since you're better positioned to test this at the
> > moment.
> > 
> > > 
> > > > I'm not even sure how to test.)
> > > > are
> > > > --b.
> > > > 
> > > > diff --git a/net/sunrpc/addr.c b/net/sunrpc/addr.c
> > > > index 6e4dbd577a39..d435bffc6199 100644
> > > > --- a/net/sunrpc/addr.c
> > > > +++ b/net/sunrpc/addr.c
> > > > @@ -162,8 +162,10 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
> > > > 			      const size_t buflen, const char *delim,
> > > > 			      struct sockaddr_in6 *sin6)
> > > > {
> > > > -	char *p;
> > > > +	char p[IPV6_SCOPE_ID_LEN + 1];
> > > > 	size_t len;
> > > > +	u32 scope_id = 0;
> > > > +	struct net_device *dev;
> > > > 
> > > > 	if ((buf + buflen) == delim)
> > > > 		return 1;
> > > > @@ -175,29 +177,23 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
> > > > 		return 0;
> > > > 
> > > > 	len = (buf + buflen) - delim - 1;
> > > > -	p = kmemdup_nul(delim + 1, len, GFP_KERNEL);
> > > > -	if (p) {
> > > > -		u32 scope_id = 0;
> > > > -		struct net_device *dev;
> > > > -
> > > > -		dev = dev_get_by_name(net, p);
> > > > -		if (dev != NULL) {
> > > > -			scope_id = dev->ifindex;
> > > > -			dev_put(dev);
> > > > -		} else {
> > > > -			if (kstrtou32(p, 10, &scope_id) != 0) {
> > > > -				kfree(p);
> > > > -				return 0;
> > > > -			}
> > > > -		}
> > > > -
> > > > -		kfree(p);
> > > > -
> > > > -		sin6->sin6_scope_id = scope_id;
> > > > -		return 1;
> > > > +	if (len > IPV6_SCOPE_ID_LEN)
> > > > +		return 0;
> > > > +
> > > > +	memcpy(p, delim + 1, len);
> > > > +	p[len] = 0;
> > > 
> > > If I recall correctly, Linus prefers us to use the str*()
> > > functions instead of raw memcpy() in cases like this.
> > 
> > I hadn't heard that.
> 
> I'm paraphrasing these:
> 
> https://lore.kernel.org/lkml/CAHk-=wj5Pp5J-CAPck22RSQ13k3cEOVnJHUA-WocAZqCJK1BZw@mail.gmail.com/
> 
> https://lore.kernel.org/lkml/CAHk-=wjWosrcv2=6m-=YgXRKev=5cnCg-1EhqDpbRXT5z6eQmg@mail.gmail.com/
> 
> 
> > If you already know the length to be copied, then
> > strcpy and the like tend to be less efficient since they continually
> > have to check for null terminators as they walk the source string.
> 
> I'm sure there's one str helper that comes close to what we need.
> Here efficiency doesn't really matter, and the size of the device
> string is always going to be in the single digits.
> 
> The priority is correctness.
> 
> 

Hmm, it sounds line in the second email he suggests using memcpy():

"Your "memcpy()" example implies that the source is always a fixed-size
thing. In that case, maybe that's the rigth thing to do, and you
should just create a real function for it."

Maybe I'm missing the context though.

In any case, when you're certain about the length of the source and
destination buffers, there's no real benefit to avoiding memcpy in favor
of strcpy and the like. It's just as correct.

Your call though, of course. ;)

> > > > +
> > > > +	dev = dev_get_by_name(net, p);
> > > > +	if (dev != NULL) {
> > > > +		scope_id = dev->ifindex;
> > > > +		dev_put(dev);
> > > > +	} else {
> > > > +		if (kstrtou32(p, 10, &scope_id) != 0)
> > > > +			return 0;
> > > > 	}
> > > > 
> > > > -	return 0;
> > > > +	sin6->sin6_scope_id = scope_id;
> > > > +	return 1;
> > > > }
> > > > 
> > > > static size_t rpc_pton6(struct net *net, const char *buf, const size_t buflen,
> > > 
> > > --
> > > Chuck Lever
> > > 
> > > 
> > > 
> > 
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> 
> --
> Chuck Lever
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [bug report] nfsd: Protect session creation and client confirm using client_lock

[Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>
Subject: [PATCH] nfsd: don't alloc under spinlock in rpc_parse_scope_id

Dan Carpenter says:

  The patch d20c11d86d8f: "nfsd: Protect session creation and client
  confirm using client_lock" from Jul 30, 2014, leads to the following
  Smatch static checker warning:

        net/sunrpc/addr.c:178 rpc_parse_scope_id()
        warn: sleeping in atomic context

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: d20c11d86d8f ("nfsd: Protect session creation and client...")
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---

 net/sunrpc/addr.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

On Thu, Sep 09, 2021 at 10:56:33AM -0400, Jeff Layton wrote:
> Hmm, it sounds line in the second email he suggests using memcpy():
> 
> "Your "memcpy()" example implies that the source is always a fixed-size
> thing. In that case, maybe that's the rigth thing to do, and you
> should just create a real function for it."
> 
> Maybe I'm missing the context though.
> 
> In any case, when you're certain about the length of the source and
> destination buffers, there's no real benefit to avoiding memcpy in favor
> of strcpy and the like. It's just as correct.

OK, queueing this up as is for 5.16 unless someone objects.  (But, could
really use testing, I'm not currently testing over ipv6.)--b.

diff --git a/net/sunrpc/addr.c b/net/sunrpc/addr.c
index 6e4dbd577a39..d435bffc6199 100644
--- a/net/sunrpc/addr.c
+++ b/net/sunrpc/addr.c
@@ -162,8 +162,10 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
 			      const size_t buflen, const char *delim,
 			      struct sockaddr_in6 *sin6)
 {
-	char *p;
+	char p[IPV6_SCOPE_ID_LEN + 1];
 	size_t len;
+	u32 scope_id = 0;
+	struct net_device *dev;
 
 	if ((buf + buflen) == delim)
 		return 1;
@@ -175,29 +177,23 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
 		return 0;
 
 	len = (buf + buflen) - delim - 1;
-	p = kmemdup_nul(delim + 1, len, GFP_KERNEL);
-	if (p) {
-		u32 scope_id = 0;
-		struct net_device *dev;
-
-		dev = dev_get_by_name(net, p);
-		if (dev != NULL) {
-			scope_id = dev->ifindex;
-			dev_put(dev);
-		} else {
-			if (kstrtou32(p, 10, &scope_id) != 0) {
-				kfree(p);
-				return 0;
-			}
-		}
-
-		kfree(p);
-
-		sin6->sin6_scope_id = scope_id;
-		return 1;
+	if (len > IPV6_SCOPE_ID_LEN)
+		return 0;
+
+	memcpy(p, delim + 1, len);
+	p[len] = 0;
+
+	dev = dev_get_by_name(net, p);
+	if (dev != NULL) {
+		scope_id = dev->ifindex;
+		dev_put(dev);
+	} else {
+		if (kstrtou32(p, 10, &scope_id) != 0)
+			return 0;
 	}
 
-	return 0;
+	sin6->sin6_scope_id = scope_id;
+	return 1;
 }
 
 static size_t rpc_pton6(struct net *net, const char *buf, const size_t buflen,
-- 
2.31.1

Re: [bug report] nfsd: Protect session creation and client confirm using client_lock

[Chuck Lever III]

> On Sep 14, 2021, at 12:37 PM, Bruce Fields <bfields@fieldses.org> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> Subject: [PATCH] nfsd: don't alloc under spinlock in rpc_parse_scope_id
> 
> Dan Carpenter says:
> 
>  The patch d20c11d86d8f: "nfsd: Protect session creation and client
>  confirm using client_lock" from Jul 30, 2014, leads to the following
>  Smatch static checker warning:
> 
>        net/sunrpc/addr.c:178 rpc_parse_scope_id()
>        warn: sleeping in atomic context
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Fixes: d20c11d86d8f ("nfsd: Protect session creation and client...")
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
> 
> net/sunrpc/addr.c | 40 ++++++++++++++++++----------------------
> 1 file changed, 18 insertions(+), 22 deletions(-)
> 
> On Thu, Sep 09, 2021 at 10:56:33AM -0400, Jeff Layton wrote:
>> Hmm, it sounds line in the second email he suggests using memcpy():
>> 
>> "Your "memcpy()" example implies that the source is always a fixed-size
>> thing. In that case, maybe that's the rigth thing to do, and you
>> should just create a real function for it."
>> 
>> Maybe I'm missing the context though.

The scope identifier isn't fixed in size, so I'm not sure how you
got there.


>> In any case, when you're certain about the length of the source and
>> destination buffers, there's no real benefit to avoiding memcpy in favor
>> of strcpy and the like. It's just as correct.
> 
> OK, queueing this up as is for 5.16 unless someone objects.

IMO Linus prefers strscpy() over open-coded memcpys, but it's not
a hill I'm going to fight and die on.


> (But, could
> really use testing, I'm not currently testing over ipv6.)--b.

Seems like you could generate some artificial test cases without
needing to set up IPv6.


> diff --git a/net/sunrpc/addr.c b/net/sunrpc/addr.c
> index 6e4dbd577a39..d435bffc6199 100644
> --- a/net/sunrpc/addr.c
> +++ b/net/sunrpc/addr.c
> @@ -162,8 +162,10 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
> 			      const size_t buflen, const char *delim,
> 			      struct sockaddr_in6 *sin6)
> {
> -	char *p;
> +	char p[IPV6_SCOPE_ID_LEN + 1];
> 	size_t len;
> +	u32 scope_id = 0;
> +	struct net_device *dev;
> 
> 	if ((buf + buflen) == delim)
> 		return 1;
> @@ -175,29 +177,23 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
> 		return 0;
> 
> 	len = (buf + buflen) - delim - 1;
> -	p = kmemdup_nul(delim + 1, len, GFP_KERNEL);
> -	if (p) {
> -		u32 scope_id = 0;
> -		struct net_device *dev;
> -
> -		dev = dev_get_by_name(net, p);
> -		if (dev != NULL) {
> -			scope_id = dev->ifindex;
> -			dev_put(dev);
> -		} else {
> -			if (kstrtou32(p, 10, &scope_id) != 0) {
> -				kfree(p);
> -				return 0;
> -			}
> -		}
> -
> -		kfree(p);
> -
> -		sin6->sin6_scope_id = scope_id;
> -		return 1;
> +	if (len > IPV6_SCOPE_ID_LEN)
> +		return 0;
> +
> +	memcpy(p, delim + 1, len);
> +	p[len] = 0;
> +
> +	dev = dev_get_by_name(net, p);
> +	if (dev != NULL) {
> +		scope_id = dev->ifindex;
> +		dev_put(dev);
> +	} else {
> +		if (kstrtou32(p, 10, &scope_id) != 0)
> +			return 0;
> 	}
> 
> -	return 0;
> +	sin6->sin6_scope_id = scope_id;
> +	return 1;
> }
> 
> static size_t rpc_pton6(struct net *net, const char *buf, const size_t buflen,
> -- 
> 2.31.1
> 

--
Chuck Lever

Re: [bug report] nfsd: Protect session creation and client confirm using client_lock

[Chuck Lever III]

> On Sep 14, 2021, at 12:48 PM, Chuck Lever III <chuck.lever@oracle.com> wrote:
> 
> 
> 
>> On Sep 14, 2021, at 12:37 PM, Bruce Fields <bfields@fieldses.org> wrote:
>> 
>> From: "J. Bruce Fields" <bfields@redhat.com>
>> Subject: [PATCH] nfsd: don't alloc under spinlock in rpc_parse_scope_id
>> 
>> Dan Carpenter says:
>> 
>> The patch d20c11d86d8f: "nfsd: Protect session creation and client
>> confirm using client_lock" from Jul 30, 2014, leads to the following
>> Smatch static checker warning:
>> 
>>       net/sunrpc/addr.c:178 rpc_parse_scope_id()
>>       warn: sleeping in atomic context
>> 
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Fixes: d20c11d86d8f ("nfsd: Protect session creation and client...")
>> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>> ---
>> 
>> net/sunrpc/addr.c | 40 ++++++++++++++++++----------------------
>> 1 file changed, 18 insertions(+), 22 deletions(-)
>> 
>> On Thu, Sep 09, 2021 at 10:56:33AM -0400, Jeff Layton wrote:
>>> Hmm, it sounds line in the second email he suggests using memcpy():
>>> 
>>> "Your "memcpy()" example implies that the source is always a fixed-size
>>> thing. In that case, maybe that's the rigth thing to do, and you
>>> should just create a real function for it."
>>> 
>>> Maybe I'm missing the context though.
> 
> The scope identifier isn't fixed in size, so I'm not sure how you
> got there.
> 
> 
>>> In any case, when you're certain about the length of the source and
>>> destination buffers, there's no real benefit to avoiding memcpy in favor
>>> of strcpy and the like. It's just as correct.
>> 
>> OK, queueing this up as is for 5.16 unless someone objects.
> 
> IMO Linus prefers strscpy() over open-coded memcpys, but it's not
> a hill I'm going to fight and die on.
> 
> 
>> (But, could
>> really use testing, I'm not currently testing over ipv6.)--b.
> 
> Seems like you could generate some artificial test cases without
> needing to set up IPv6.

Actually, a scope ID is needed when using link-local addresses,
which are set up automatically simply when IPv6 is enabled.
If you see an address like this:

    inet6 fe80::2eae:686b:8c82:fad2/64 scope link noprefixroute 
       valid_lft forever preferred_lft forever

Then you can use this address to mount with by adding that
interface name as the scope ID.


>> diff --git a/net/sunrpc/addr.c b/net/sunrpc/addr.c
>> index 6e4dbd577a39..d435bffc6199 100644
>> --- a/net/sunrpc/addr.c
>> +++ b/net/sunrpc/addr.c
>> @@ -162,8 +162,10 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
>> 			      const size_t buflen, const char *delim,
>> 			      struct sockaddr_in6 *sin6)
>> {
>> -	char *p;
>> +	char p[IPV6_SCOPE_ID_LEN + 1];
>> 	size_t len;
>> +	u32 scope_id = 0;
>> +	struct net_device *dev;
>> 
>> 	if ((buf + buflen) == delim)
>> 		return 1;
>> @@ -175,29 +177,23 @@ static int rpc_parse_scope_id(struct net *net, const char *buf,
>> 		return 0;
>> 
>> 	len = (buf + buflen) - delim - 1;
>> -	p = kmemdup_nul(delim + 1, len, GFP_KERNEL);
>> -	if (p) {
>> -		u32 scope_id = 0;
>> -		struct net_device *dev;
>> -
>> -		dev = dev_get_by_name(net, p);
>> -		if (dev != NULL) {
>> -			scope_id = dev->ifindex;
>> -			dev_put(dev);
>> -		} else {
>> -			if (kstrtou32(p, 10, &scope_id) != 0) {
>> -				kfree(p);
>> -				return 0;
>> -			}
>> -		}
>> -
>> -		kfree(p);
>> -
>> -		sin6->sin6_scope_id = scope_id;
>> -		return 1;
>> +	if (len > IPV6_SCOPE_ID_LEN)
>> +		return 0;
>> +
>> +	memcpy(p, delim + 1, len);
>> +	p[len] = 0;
>> +
>> +	dev = dev_get_by_name(net, p);
>> +	if (dev != NULL) {
>> +		scope_id = dev->ifindex;
>> +		dev_put(dev);
>> +	} else {
>> +		if (kstrtou32(p, 10, &scope_id) != 0)
>> +			return 0;
>> 	}
>> 
>> -	return 0;
>> +	sin6->sin6_scope_id = scope_id;
>> +	return 1;
>> }
>> 
>> static size_t rpc_pton6(struct net *net, const char *buf, const size_t buflen,
>> -- 
>> 2.31.1
>> 
> 
> --
> Chuck Lever

--
Chuck Lever