diff mbox series

[SMB3,client] fix potential deadlock in cifs_sync_mid_result

Message ID CAH2r5mtJuttkzHDQB=-U3o=bBnv5U9r2w+JG-mXg1TPBT1wFog@mail.gmail.com (mailing list archive)
State New, archived
Headers show
Series [SMB3,client] fix potential deadlock in cifs_sync_mid_result | expand

Commit Message

Steve French April 24, 2024, 3:46 a.m. UTC 
Coverity spotted that the cifs_sync_mid_result function could deadlock
since cifs_server_dbg graps the srv_lock while we are still holding
the mid_lock

"Thread deadlock (ORDER_REVERSAL) lock_order: Calling spin_lock acquires
lock TCP_Server_Info.srv_lock while holding lock TCP_Server_Info.mid_lock"

Addresses-Coverity: 1590401 ("Thread deadlock (ORDER_REVERSAL)")

See attached patch

Comments

Shyam Prasad N April 25, 2024, 5:44 p.m. UTC | #1 
On Wed, Apr 24, 2024 at 9:16 AM Steve French <smfrench@gmail.com> wrote:
>
> Coverity spotted that the cifs_sync_mid_result function could deadlock
> since cifs_server_dbg graps the srv_lock while we are still holding
> the mid_lock
>
> "Thread deadlock (ORDER_REVERSAL) lock_order: Calling spin_lock acquires
> lock TCP_Server_Info.srv_lock while holding lock TCP_Server_Info.mid_lock"
>
> Addresses-Coverity: 1590401 ("Thread deadlock (ORDER_REVERSAL)")
>
> See attached patch
>
>
> --
> Thanks,
>
> Steve

Looks good to me.
Steve French April 25, 2024, 5:53 p.m. UTC | #2 
Minor update to patch (shrink slightly by using a goto)


On Thu, Apr 25, 2024 at 12:44 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> On Wed, Apr 24, 2024 at 9:16 AM Steve French <smfrench@gmail.com> wrote:
> >
> > Coverity spotted that the cifs_sync_mid_result function could deadlock
> > since cifs_server_dbg graps the srv_lock while we are still holding
> > the mid_lock
> >
> > "Thread deadlock (ORDER_REVERSAL) lock_order: Calling spin_lock acquires
> > lock TCP_Server_Info.srv_lock while holding lock TCP_Server_Info.mid_lock"
> >
> > Addresses-Coverity: 1590401 ("Thread deadlock (ORDER_REVERSAL)")
> >
> > See attached patch
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
> Looks good to me.
>
> --
> Regards,
> Shyam
Shyam Prasad N April 25, 2024, 5:56 p.m. UTC | #3 
On Thu, Apr 25, 2024 at 11:23 PM Steve French <smfrench@gmail.com> wrote:
>
> Minor update to patch (shrink slightly by using a goto)
>
>
> On Thu, Apr 25, 2024 at 12:44 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >
> > On Wed, Apr 24, 2024 at 9:16 AM Steve French <smfrench@gmail.com> wrote:
> > >
> > > Coverity spotted that the cifs_sync_mid_result function could deadlock
> > > since cifs_server_dbg graps the srv_lock while we are still holding
> > > the mid_lock
> > >
> > > "Thread deadlock (ORDER_REVERSAL) lock_order: Calling spin_lock acquires
> > > lock TCP_Server_Info.srv_lock while holding lock TCP_Server_Info.mid_lock"
> > >
> > > Addresses-Coverity: 1590401 ("Thread deadlock (ORDER_REVERSAL)")
> > >
> > > See attached patch
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> >
> > Looks good to me.
> >
> > --
> > Regards,
> > Shyam
>
>
>
> --
> Thanks,
>
> Steve

This one looks even better. Thanks.
diff mbox series

Patch

From 9b42329261067a500f2452f131c88c8cb0b62aa5 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Tue, 23 Apr 2024 22:35:28 -0500
Subject: [PATCH] smb3: fix lock ordering potential deadlock in
 cifs_sync_mid_result

Coverity spotted that the cifs_sync_mid_result function could deadlock

"Thread deadlock (ORDER_REVERSAL) lock_order: Calling spin_lock acquires
lock TCP_Server_Info.srv_lock while holding lock TCP_Server_Info.mid_lock"

Addresses-Coverity: 1590401 ("Thread deadlock (ORDER_REVERSAL)")
Cc: stable@vger.kernel.org
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/smb/client/transport.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index 994d70193432..443b4b89431d 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -909,9 +909,11 @@  cifs_sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
 			list_del_init(&mid->qhead);
 			mid->mid_flags |= MID_DELETED;
 		}
+		spin_unlock(&server->mid_lock);
 		cifs_server_dbg(VFS, "%s: invalid mid state mid=%llu state=%d\n",
 			 __func__, mid->mid, mid->mid_state);
-		rc = -EIO;
+		release_mid(mid);
+		return -EIO;
 	}
 	spin_unlock(&server->mid_lock);
 
-- 
2.40.1