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
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 <[email protected]>
> On Sep 7, 2021, at 7:00 AM, Jeff Layton <[email protected]> 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
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,
> On Sep 8, 2021, at 5:26 PM, Bruce Fields <[email protected]> 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
On Wed, 2021-09-08 at 21:32 +0000, Chuck Lever III wrote:
>
> > On Sep 8, 2021, at 5:26 PM, Bruce Fields <[email protected]> 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 <[email protected]>
> On Sep 9, 2021, at 6:19 AM, Jeff Layton <[email protected]> wrote:
>
> On Wed, 2021-09-08 at 21:32 +0000, Chuck Lever III wrote:
>>
>>> On Sep 8, 2021, at 5:26 PM, Bruce Fields <[email protected]> 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 <[email protected]>
--
Chuck Lever
On Thu, 2021-09-09 at 14:31 +0000, Chuck Lever III wrote:
>
> > On Sep 9, 2021, at 6:19 AM, Jeff Layton <[email protected]> wrote:
> >
> > On Wed, 2021-09-08 at 21:32 +0000, Chuck Lever III wrote:
> > >
> > > > On Sep 8, 2021, at 5:26 PM, Bruce Fields <[email protected]> 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 <[email protected]>
>
> --
> Chuck Lever
>
>
>
--
Jeff Layton <[email protected]>
From: "J. Bruce Fields" <[email protected]>
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 <[email protected]>
Fixes: d20c11d86d8f ("nfsd: Protect session creation and client...")
Signed-off-by: J. Bruce Fields <[email protected]>
---
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
> On Sep 14, 2021, at 12:37 PM, Bruce Fields <[email protected]> wrote:
>
> From: "J. Bruce Fields" <[email protected]>
> 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 <[email protected]>
> Fixes: d20c11d86d8f ("nfsd: Protect session creation and client...")
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
>
> 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
> On Sep 14, 2021, at 12:48 PM, Chuck Lever III <[email protected]> wrote:
>
>
>
>> On Sep 14, 2021, at 12:37 PM, Bruce Fields <[email protected]> wrote:
>>
>> From: "J. Bruce Fields" <[email protected]>
>> 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 <[email protected]>
>> Fixes: d20c11d86d8f ("nfsd: Protect session creation and client...")
>> Signed-off-by: J. Bruce Fields <[email protected]>
>> ---
>>
>> 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