* [bug report] nfsd: Protect session creation and client confirm using client_lock
@ 2021-09-07 8:07 Dan Carpenter
2021-09-07 11:00 ` Jeff Layton
0 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2021-09-07 8:07 UTC (permalink / raw)
To: jlayton; +Cc: linux-nfs
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [bug report] nfsd: Protect session creation and client confirm using client_lock 2021-09-07 8:07 [bug report] nfsd: Protect session creation and client confirm using client_lock Dan Carpenter @ 2021-09-07 11:00 ` Jeff Layton 2021-09-07 15:00 ` Chuck Lever III 0 siblings, 1 reply; 11+ messages in thread From: Jeff Layton @ 2021-09-07 11:00 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-nfs, Bruce Fields, Chuck Lever 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [bug report] nfsd: Protect session creation and client confirm using client_lock 2021-09-07 11:00 ` Jeff Layton @ 2021-09-07 15:00 ` Chuck Lever III 2021-09-08 21:26 ` Bruce Fields 0 siblings, 1 reply; 11+ messages in thread From: Chuck Lever III @ 2021-09-07 15:00 UTC (permalink / raw) To: Jeff Layton; +Cc: Dan Carpenter, Linux NFS Mailing List, Bruce Fields > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [bug report] nfsd: Protect session creation and client confirm using client_lock 2021-09-07 15:00 ` Chuck Lever III @ 2021-09-08 21:26 ` Bruce Fields 2021-09-08 21:32 ` Chuck Lever III 0 siblings, 1 reply; 11+ messages in thread From: Bruce Fields @ 2021-09-08 21:26 UTC (permalink / raw) To: Chuck Lever III; +Cc: Jeff Layton, Dan Carpenter, Linux NFS Mailing List 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, ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [bug report] nfsd: Protect session creation and client confirm using client_lock 2021-09-08 21:26 ` Bruce Fields @ 2021-09-08 21:32 ` Chuck Lever III 2021-09-09 10:19 ` Jeff Layton 0 siblings, 1 reply; 11+ messages in thread From: Chuck Lever III @ 2021-09-08 21:32 UTC (permalink / raw) To: Bruce Fields, Jeff Layton; +Cc: Dan Carpenter, Linux NFS Mailing List > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [bug report] nfsd: Protect session creation and client confirm using client_lock 2021-09-08 21:32 ` Chuck Lever III @ 2021-09-09 10:19 ` Jeff Layton 2021-09-09 14:31 ` Chuck Lever III 0 siblings, 1 reply; 11+ messages in thread From: Jeff Layton @ 2021-09-09 10:19 UTC (permalink / raw) To: Chuck Lever III, Bruce Fields; +Cc: Dan Carpenter, Linux NFS Mailing List 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [bug report] nfsd: Protect session creation and client confirm using client_lock 2021-09-09 10:19 ` Jeff Layton @ 2021-09-09 14:31 ` Chuck Lever III 2021-09-09 14:56 ` Jeff Layton 0 siblings, 1 reply; 11+ messages in thread From: Chuck Lever III @ 2021-09-09 14:31 UTC (permalink / raw) To: Jeff Layton; +Cc: Bruce Fields, Dan Carpenter, Linux NFS Mailing List > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [bug report] nfsd: Protect session creation and client confirm using client_lock 2021-09-09 14:31 ` Chuck Lever III @ 2021-09-09 14:56 ` Jeff Layton 2021-09-14 16:37 ` Bruce Fields 0 siblings, 1 reply; 11+ messages in thread From: Jeff Layton @ 2021-09-09 14:56 UTC (permalink / raw) To: Chuck Lever III; +Cc: Bruce Fields, Dan Carpenter, Linux NFS Mailing List 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [bug report] nfsd: Protect session creation and client confirm using client_lock 2021-09-09 14:56 ` Jeff Layton @ 2021-09-14 16:37 ` Bruce Fields 2021-09-14 16:48 ` Chuck Lever III 0 siblings, 1 reply; 11+ messages in thread From: Bruce Fields @ 2021-09-14 16:37 UTC (permalink / raw) To: Jeff Layton; +Cc: Chuck Lever III, Dan Carpenter, Linux NFS Mailing List 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [bug report] nfsd: Protect session creation and client confirm using client_lock 2021-09-14 16:37 ` Bruce Fields @ 2021-09-14 16:48 ` Chuck Lever III 2021-09-15 14:03 ` Chuck Lever III 0 siblings, 1 reply; 11+ messages in thread From: Chuck Lever III @ 2021-09-14 16:48 UTC (permalink / raw) To: Bruce Fields; +Cc: Jeff Layton, Dan Carpenter, Linux NFS Mailing List > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [bug report] nfsd: Protect session creation and client confirm using client_lock 2021-09-14 16:48 ` Chuck Lever III @ 2021-09-15 14:03 ` Chuck Lever III 0 siblings, 0 replies; 11+ messages in thread From: Chuck Lever III @ 2021-09-15 14:03 UTC (permalink / raw) To: Bruce Fields; +Cc: Jeff Layton, Dan Carpenter, Linux NFS Mailing List > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-09-15 14:03 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-09-07 8:07 [bug report] nfsd: Protect session creation and client confirm using client_lock Dan Carpenter 2021-09-07 11:00 ` Jeff Layton 2021-09-07 15:00 ` Chuck Lever III 2021-09-08 21:26 ` Bruce Fields 2021-09-08 21:32 ` Chuck Lever III 2021-09-09 10:19 ` Jeff Layton 2021-09-09 14:31 ` Chuck Lever III 2021-09-09 14:56 ` Jeff Layton 2021-09-14 16:37 ` Bruce Fields 2021-09-14 16:48 ` Chuck Lever III 2021-09-15 14:03 ` Chuck Lever III
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox