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@xxxxxxxxxx>
