Re: [PATCH] cifs: Improve SMB2+ stat() to work also for paths in DELETE_PENDING state

Re: [PATCH] cifs: Improve SMB2+ stat() to work also for paths in DELETE_PENDING state

[Steve French]

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

In thinking about this patch, isn't it doing the reverse of what POSIX requires?

When querying a file in delete pending state, shouldn't be invisible
in the namespace on the client.  Wouldn't your patch do the opposite?
ie Doesn't it allow stat to return information about a file which
should be invisible to everyone except the app who has it open (and if
possible we could make this easier via a "silly rename" strategy as
some other protocols do)?

Thoughts?



-- 
Thanks,

Steve

[-- Attachment #2: 0010-cifs-Improve-SMB2-stat-to-work-also-for-paths-in-DEL.patch --]
[-- Type: text/x-patch, Size: 9490 bytes --]

From ef4b193e164a98cfd0c2daf5b43b2451820c4411 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pali=20Roh=C3=A1r?= <pali@kernel.org>
Date: Mon, 23 Dec 2024 18:57:10 +0100
Subject: [PATCH 10/41] cifs: Improve SMB2+ stat() to work also for paths in
 DELETE_PENDING state
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Paths in DELETE_PENDING state cannot be opened at all. So standard way of
querying path attributes for this case is not possible.

There is an alternative way how to query limited information about file
over SMB2+ dialects without opening file itself. It is by opening the
parent directory, querying specific child with filled search filter and
asking for attributes for that child.

Implement this fallback when standard case in smb2_query_path_info fails
with STATUS_DELETE_PENDING error and stat was asked for path which is not
top level one (because top level does not have parent directory at all).

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/smb/client/cifsglob.h  |   1 +
 fs/smb/client/smb2glob.h  |   1 +
 fs/smb/client/smb2inode.c | 166 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 165 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 6ae170a2a042..f0d75199d439 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -2324,6 +2324,7 @@ struct smb2_compound_vars {
 	struct smb_rqst rqst[MAX_COMPOUND];
 	struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
 	struct kvec qi_iov;
+	struct kvec qd_iov[SMB2_QUERY_DIRECTORY_IOV_SIZE];
 	struct kvec io_iov[SMB2_IOCTL_IOV_SIZE];
 	struct kvec si_iov[SMB2_SET_INFO_IOV_SIZE];
 	struct kvec close_iov;
diff --git a/fs/smb/client/smb2glob.h b/fs/smb/client/smb2glob.h
index 224495322a05..1cb219605e75 100644
--- a/fs/smb/client/smb2glob.h
+++ b/fs/smb/client/smb2glob.h
@@ -39,6 +39,7 @@ enum smb2_compound_ops {
 	SMB2_OP_GET_REPARSE,
 	SMB2_OP_QUERY_WSL_EA,
 	SMB2_OP_OPEN_QUERY,
+	SMB2_OP_QUERY_DIRECTORY,
 };
 
 /* Used when constructing chained read requests. */
diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 57d9bfbadd97..f4e2fe8991d0 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -176,6 +176,8 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 			    struct kvec *out_iov, int *out_buftype, struct dentry *dentry)
 {
 
+	struct smb2_query_directory_req *qd_rqst = NULL;
+	struct smb2_query_directory_rsp *qd_rsp = NULL;
 	struct smb2_create_rsp *create_rsp = NULL;
 	struct smb2_query_info_rsp *qi_rsp = NULL;
 	struct smb2_compound_vars *vars = NULL;
@@ -344,6 +346,39 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 			trace_smb3_posix_query_info_compound_enter(xid, tcon->tid,
 								   ses->Suid, full_path);
 			break;
+		case SMB2_OP_QUERY_DIRECTORY:
+			rqst[num_rqst].rq_iov = &vars->qd_iov[0];
+			rqst[num_rqst].rq_nvec = SMB2_QUERY_DIRECTORY_IOV_SIZE;
+
+			rc = SMB2_query_directory_init(xid,
+						       tcon,
+						       server,
+						       &rqst[num_rqst],
+						       cfile ? cfile->fid.persistent_fid : COMPOUND_FID,
+						       cfile ? cfile->fid.volatile_fid : COMPOUND_FID,
+						       0,
+						       (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) ?
+						        SMB_FIND_FILE_ID_FULL_DIR_INFO :
+						        SMB_FIND_FILE_FULL_DIRECTORY_INFO);
+			if (!rc) {
+				/*
+				 * Change the default search wildcard pattern '*'
+				 * to the requested file name stored in in_iov[i]
+				 * and request for only one single entry.
+				 */
+				qd_rqst = rqst[num_rqst].rq_iov[0].iov_base;
+				qd_rqst->Flags |= SMB2_RETURN_SINGLE_ENTRY;
+				qd_rqst->FileNameLength = cpu_to_le16(in_iov[i].iov_len);
+				rqst[num_rqst].rq_iov[1] = in_iov[i];
+			}
+			if (!rc && (!cfile || num_rqst > 1)) {
+				smb2_set_next_command(tcon, &rqst[num_rqst]);
+				smb2_set_related(&rqst[num_rqst]);
+			} else if (rc) {
+				goto finished;
+			}
+			num_rqst++;
+			break;
 		case SMB2_OP_DELETE:
 			trace_smb3_delete_enter(xid, tcon->tid, ses->Suid, full_path);
 			break;
@@ -728,6 +763,55 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 				trace_smb3_posix_query_info_compound_done(xid, tcon->tid,
 									  ses->Suid);
 			break;
+		case SMB2_OP_QUERY_DIRECTORY:
+			if (rc == 0) {
+				qd_rsp = (struct smb2_query_directory_rsp *)
+					rsp_iov[i + 1].iov_base;
+				rc = smb2_validate_iov(le16_to_cpu(qd_rsp->OutputBufferOffset),
+						       le32_to_cpu(qd_rsp->OutputBufferLength),
+						       &rsp_iov[i + 1],
+						       (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) ?
+						        sizeof(SEARCH_ID_FULL_DIR_INFO) :
+						        sizeof(FILE_FULL_DIRECTORY_INFO));
+			}
+			if (rc == 0) {
+				/*
+				 * Both SEARCH_ID_FULL_DIR_INFO and FILE_FULL_DIRECTORY_INFO
+				 * have same member offsets except the UniqueId and FileName.
+				 */
+				SEARCH_ID_FULL_DIR_INFO *si = (SEARCH_ID_FULL_DIR_INFO *)qd_rsp->Buffer;
+				idata = in_iov[i + 1].iov_base;
+				idata->fi.CreationTime = si->CreationTime;
+				idata->fi.LastAccessTime = si->LastAccessTime;
+				idata->fi.LastWriteTime = si->LastWriteTime;
+				idata->fi.ChangeTime = si->ChangeTime;
+				idata->fi.Attributes = si->ExtFileAttributes;
+				idata->fi.AllocationSize = si->AllocationSize;
+				idata->fi.EndOfFile = si->EndOfFile;
+				idata->fi.EASize = si->EaSize;
+				/*
+				 * UniqueId is present only in struct SEARCH_ID_FULL_DIR_INFO.
+				 * It is not present in struct FILE_FULL_DIRECTORY_INFO.
+				 * struct SEARCH_ID_FULL_DIR_INFO was requested only when
+				 * CIFS_MOUNT_SERVER_INUM is set.
+				 */
+				if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM)
+					idata->fi.IndexNumber = si->UniqueId;
+				if (le32_to_cpu(idata->fi.NumberOfLinks) == 0)
+					idata->fi.NumberOfLinks = cpu_to_le32(1); /* dummy value */
+				idata->fi.DeletePending = 0;
+				idata->fi.Directory = !!(le32_to_cpu(si->ExtFileAttributes) & ATTR_DIRECTORY);
+			}
+			SMB2_query_directory_free(&rqst[num_rqst++]);
+			if (rc)
+				trace_smb3_query_dir_err(xid,
+							 cfile ? cfile->fid.persistent_fid : COMPOUND_FID,
+							 tcon->tid, ses->Suid, 0, 0, rc);
+			else
+				trace_smb3_query_dir_done(xid,
+							  cfile ? cfile->fid.persistent_fid : COMPOUND_FID,
+							  tcon->tid, ses->Suid, 0, 0);
+			break;
 		case SMB2_OP_DELETE:
 			if (rc)
 				trace_smb3_delete_err(xid, tcon->tid, ses->Suid, rc);
@@ -1087,9 +1171,9 @@ int smb2_query_path_info(const unsigned int xid,
 		break;
 	case -EREMOTE:
 		break;
-	default:
-		if (hdr->Status != STATUS_OBJECT_NAME_INVALID)
-			break;
+	}
+
+	if (hdr->Status == STATUS_OBJECT_NAME_INVALID) {
 		rc2 = cifs_inval_name_dfs_link_error(xid, tcon, cifs_sb,
 						     full_path, &islink);
 		if (rc2) {
@@ -1098,6 +1182,82 @@ int smb2_query_path_info(const unsigned int xid,
 		}
 		if (islink)
 			rc = -EREMOTE;
+	} else if (hdr->Status == STATUS_DELETE_PENDING && full_path[0]) {
+		/*
+		 * If SMB2 OPEN/CREATE fails with STATUS_DELETE_PENDING error,
+		 * it means that the path is in delete pending state and it is
+		 * not possible to open it until some other client clears delete
+		 * pending state or all other clients close all opened handles
+		 * to that path.
+		 *
+		 * There is an alternative way how to query limited information
+		 * about path which is in delete pending state still suitable
+		 * for the stat() syscall. It is by opening the parent directory,
+		 * querying specific child with filled search filer and asking
+		 * for attributes for that child.
+		 */
+
+		char *parent_path;
+		const char *basename;
+		__le16 *basename_utf16;
+		int basename_utf16_len;
+		struct cifsFileInfo *parent_cfile;
+
+		basename = strrchr(full_path, CIFS_DIR_SEP(cifs_sb));
+		if (basename) {
+			parent_path = kstrndup(full_path, basename - full_path, GFP_KERNEL);
+			basename++;
+		} else {
+			parent_path = kstrdup("", GFP_KERNEL);
+			basename = full_path;
+		}
+
+		if (!parent_path) {
+			rc = -ENOMEM;
+			goto out;
+		}
+
+		basename_utf16 = cifs_convert_path_to_utf16(basename, cifs_sb);
+		if (!basename_utf16) {
+			kfree(parent_path);
+			rc = -ENOMEM;
+			goto out;
+		}
+
+		basename_utf16_len = 2 * UniStrnlen((wchar_t *)basename_utf16, PATH_MAX);
+
+retry_query_directory:
+		num_cmds = 1;
+		cmds[0] = SMB2_OP_QUERY_DIRECTORY;
+		in_iov[0].iov_base = basename_utf16;
+		in_iov[0].iov_len = basename_utf16_len;
+		in_iov[1].iov_base = data;
+		in_iov[1].iov_len = sizeof(*data);
+		oparms = CIFS_OPARMS(cifs_sb, tcon, parent_path, FILE_READ_DATA,
+				     FILE_OPEN, CREATE_NOT_FILE, ACL_NO_MODE);
+		cifs_get_readable_path(tcon, parent_path, &parent_cfile);
+		free_rsp_iov(out_iov, out_buftype, ARRAY_SIZE(out_iov));
+		rc = smb2_compound_op(xid, tcon, cifs_sb, parent_path,
+				      &oparms, in_iov, cmds, num_cmds,
+				      parent_cfile, out_iov, out_buftype, NULL);
+		if (rc == -EOPNOTSUPP && (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM)) {
+			/*
+			 * If querying of server inode numbers is not supported
+			 * but is enabled, then disable it and try again.
+			 */
+			cifs_autodisable_serverino(cifs_sb);
+			goto retry_query_directory;
+		}
+
+		kfree(parent_path);
+		kfree(basename_utf16);
+
+		hdr = out_iov[0].iov_base;
+		if (!hdr || out_buftype[0] == CIFS_NO_BUFFER)
+			goto out;
+
+		/* As we are in code path for STATUS_DELETE_PENDING, set DeletePending. */
+		data->fi.DeletePending = 1;
 	}
 
 out:
-- 
2.43.0

Re: [PATCH] cifs: Improve SMB2+ stat() to work also for paths in DELETE_PENDING state

[Pali Rohár]

Hello, this change is not reverse of what POSIX requires.

Delete pending state is special thing in Windows and is just a temporary
state, which may be reverted.

In POSIX, the real unlink is the final state and once unlink finish, the
directory entry is not in the parent directory anymore, and such
directory can be removed.

But on the other hand, if the file in the directory is in delete pending
state, then the directory cannot be removed. Directory can be removed
once all files are not in delete pending state anymore and are really
removed.

Any file in delete pending state is visible over SMB protocol to any
application, not just to application which opened it.

Silly rename is not an option here, because file can be put into delete
pending state by windows server itself -- which does not do any silly
rename strategy.

And for example msvcrt.dll stat() function on Windows is doing same
thing. Stat is querying also files in delete pending state.

On Monday 24 March 2025 22:53:35 Steve French wrote:
> In thinking about this patch, isn't it doing the reverse of what POSIX requires?
> 
> When querying a file in delete pending state, shouldn't be invisible
> in the namespace on the client.  Wouldn't your patch do the opposite?
> ie Doesn't it allow stat to return information about a file which
> should be invisible to everyone except the app who has it open (and if
> possible we could make this easier via a "silly rename" strategy as
> some other protocols do)?
> 
> Thoughts?
> 
> 
> 
> -- 
> Thanks,
> 
> Steve