> On Sep 14, 2021, at 12:37 PM, Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
>
> From: "J. Bruce Fields" <bfields@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> Fixes: d20c11d86d8f ("nfsd: Protect session creation and client...")
> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
> ---
>
> 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
