On Tue, Feb 27, 2024 at 8:53 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Shyam Prasad N,
>
> The patch e695a9ad0305: "cifs: missed ref-counting smb session in
> find" from May 23, 2021 (linux-next), leads to the following Smatch
> static checker warning:
>
> fs/smb/client/connect.c:1636 cifs_put_tcp_session()
> warn: sleeping in atomic context
>
> What happened here is that we recently shuffled some code around so
> now Smatch is aware that cancel_delayed_work_sync() is a might sleep
> function.
>
Hi Dan,
Thanks for running these checks.
> fs/smb/client/smb2transport.c
> 204 smb2_find_smb_tcon(struct TCP_Server_Info *server, __u64 ses_id, __u32 tid)
> 205 {
> 206 struct cifs_ses *ses;
> 207 struct cifs_tcon *tcon;
> 208
> 209 spin_lock(&cifs_tcp_ses_lock);
> 210 ses = smb2_find_smb_ses_unlocked(server, ses_id);
> 211 if (!ses) {
> 212 spin_unlock(&cifs_tcp_ses_lock);
> 213 return NULL;
> 214 }
> 215 tcon = smb2_find_smb_sess_tcon_unlocked(ses, tid);
> 216 if (!tcon) {
> 217 cifs_put_smb_ses(ses);
> 218 spin_unlock(&cifs_tcp_ses_lock);
>
> Smatch thinks calling cifs_put_smb_ses() might be a problem. This is
> a cross function thing involving reference counting and I haven't worked
> through where we're holding references so it might be a false positive...
> But I see below we drop the lock before calling cifs_put_smb_ses() so
> maybe it's okay here too?
The function here just decrements the ref count that it incremented in line 210.
There may be a theoretical chance that some other user of this session
decrements the ref count before line 217 is reached.
However, calling cifs_put_smb_ses or cifs_put_tcp_ses itself is a bad
idea and can lead to deadlocks.
So this is a valid find, but for other reasons.
The fix would be to exchange lines 218 and 217.
I'll submit a patch for this soon.
>
> 219 return NULL;
> 220 }
> 221 spin_unlock(&cifs_tcp_ses_lock);
> 222 /* tcon already has a ref to ses, so we don't need ses anymore */
> 223 cifs_put_smb_ses(ses);
> 224
> 225 return tcon;
> 226 }
>
> The call tree Smatch warns about is:
>
> smb2_find_smb_tcon() <- disables preempt
> -> cifs_put_smb_ses()
> -> __cifs_put_smb_ses()
> -> cifs_put_tcp_session()
>
> fs/smb/client/connect.c
> 1617 void
> 1618 cifs_put_tcp_session(struct TCP_Server_Info *server, int from_reconnect)
> 1619 {
> 1620 struct task_struct *task;
> 1621
> 1622 spin_lock(&cifs_tcp_ses_lock);
> 1623 if (--server->srv_count > 0) {
> 1624 spin_unlock(&cifs_tcp_ses_lock);
> 1625 return;
> 1626 }
> 1627
> 1628 /* srv_count can never go negative */
> 1629 WARN_ON(server->srv_count < 0);
> 1630
> 1631 put_net(cifs_net_ns(server));
> 1632
> 1633 list_del_init(&server->tcp_ses_list);
> 1634 spin_unlock(&cifs_tcp_ses_lock);
> 1635
> --> 1636 cancel_delayed_work_sync(&server->echo);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Sleeps.
>
> 1637
> 1638 if (from_reconnect)
> 1639 /*
>
> regards,
> dan carpenter
>
--
Regards,
Shyam
