Re: [PATCH][SMB3 client] fix potential deadlock in cifs_sync_mid_result

[Steve French <smfrench@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

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

[-- Attachment #2: 0001-smb3-fix-lock-ordering-potential-deadlock-in-cifs_sy.patch --]
[-- Type: text/x-patch, Size: 1360 bytes --]

From 8861fd5180476f45f9e8853db154600469a0284f Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Thu, 25 Apr 2024 12:49:50 -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
Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/smb/client/transport.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index e1a79e031b28..ddf1a3aafee5 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -909,12 +909,15 @@ 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;
+		goto sync_mid_done;
 	}
 	spin_unlock(&server->mid_lock);
 
+sync_mid_done:
 	release_mid(mid);
 	return rc;
 }
-- 
2.40.1