diff mbox series

[SMB3] EBADF/EIO errors in rename/open caused by race condition in smb2_compound_op

Message ID CAH2r5msJeMzyMSSq_2xea7Qz1=e1UO0XpVVdHS-xH9tS93czsA@mail.gmail.com (mailing list archive)
State New, archived
Headers show
Series [SMB3] EBADF/EIO errors in rename/open caused by race condition in smb2_compound_op | expand

Commit Message

Steve French May 12, 2022, 3:49 p.m. UTC
Created patch and merged into cifs-2.6.git for-next (pending  the
usual regression testing runs) from the good work Ondrej did on
https://bugzilla.samba.org/show_bug.cgi?id=15051

See attached.

There is  a race condition in smb2_compound_op:

    ---
    after_close:
            num_rqst++;

            if (cfile) {
                    cifsFileInfo_put(cfile); // sends SMB2_CLOSE to the server
                    cfile = NULL;
    ---
This is triggered by smb2_query_path_info operation that happens
during revalidate_dentry. In smb2_query_path_info, get_readable_path
is called to load the cfile, increasing the reference counter. If in
the meantime, this reference becomes the very last, this call to
cifsFileInfo_put(cfile) will trigger a SMB2_CLOSE request sent to the
server just before sending this compound request – and so then the
compound request fails either with EBADF/EIO depending on the timing
at the server, because the handle is already closed.

--
Thanks,

Steve
diff mbox series

Patch

From 465301e39c1f7268b6edfb16085bebb31c507dd6 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Thu, 12 May 2022 10:18:00 -0500
Subject: [PATCH] SMB3: EBADF/EIO errors in rename/open caused by race
 condition in smb2_compound_op
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

There is  a race condition in smb2_compound_op:

---
after_close:
	num_rqst++;

	if (cfile) {
		cifsFileInfo_put(cfile); // sends SMB2_CLOSE to the server
		cfile = NULL;
---

This is triggered by smb2_query_path_info operation that happens during
revalidate_dentry. In smb2_query_path_info, get_readable_path is called to
load the cfile, increasing the reference counter. If in the meantime, this
reference becomes the very last, this call to cifsFileInfo_put(cfile) will
trigger a SMB2_CLOSE request sent to the server just before sending this compound
request – and so then the compound request fails either with EBADF/EIO depending
on the timing at the server, because the handle is already closed.

In the first scenario, the race seems to be happening between smb2_query_path_info
triggered by the rename operation, and between “cleanup” of asynchronous writes – while
fsync(fd) likely waits for the asynchronous writes to complete, releasing the writeback
structures can happen after the close(fd) call. So the EBADF/EIO errors will pop up if
the timing is such that:
1) There are still outstanding references after close(fd) in the writeback structures
2) smb2_query_path_info successfully fetches the cfile, increasing the refcounter by 1
3) All writeback structures release the same cfile, reducing refcounter to 1
4) smb2_compound_op is called with that cfile

In the second scenario, the race seems to be similar – here open triggers the
smb2_query_path_info operation, and if all other threads in the meantime decrease the
refcounter to 1 similarly to the first scenario, again SMB2_CLOSE will be sent to the
server just before issuing the compound request. This case is harder to reproduce.

See https://bugzilla.samba.org/show_bug.cgi?id=15051

Cc: stable@vger.kernel.org
Fixes: 8de9e86c67ba ("cifs: create a helper to find a writeable handle by path name")
Signed-off-by: Ondrej Hubsch <ohubsch@purestorage.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/smb2inode.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
index fe5bfa245fa7..1b89b9b8a212 100644
--- a/fs/cifs/smb2inode.c
+++ b/fs/cifs/smb2inode.c
@@ -362,8 +362,6 @@  smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 	num_rqst++;
 
 	if (cfile) {
-		cifsFileInfo_put(cfile);
-		cfile = NULL;
 		rc = compound_send_recv(xid, ses, server,
 					flags, num_rqst - 2,
 					&rqst[1], &resp_buftype[1],
-- 
2.34.1