[PATCH 0/3] smb/client: Fix some issues

[PATCH 0/3] smb/client: Fix some issues

[Ze Tan]

Based on the analysis from Sashiko, fix some issues.

Ze Tan (3):
  smb/client: mark missing nlink values as unknown
  smb/client: preserve open info type across compound queries
  smb/client: decode reparse metadata using its payload type

 fs/smb/client/inode.c     |  9 +++++----
 fs/smb/client/reparse.h   | 17 ++++++++++-------
 fs/smb/client/smb1ops.c   |  8 +++++++-
 fs/smb/client/smb2inode.c | 16 ++++++++++------
 fs/smb/client/smb2pdu.c   |  1 +
 5 files changed, 33 insertions(+), 18 deletions(-)


base-commit: c0a27675eaf08255017b3cabc28c99c0cd71f468
-- 
2.43.0

[PATCH 1/3] smb/client: mark missing nlink values as unknown

[Ze Tan]

Several SMB1 fallback and open responses do not provide the hard link
count. The SMB2 create-only query fallback has the same limitation.
These paths currently leave a zero link count or synthesize a value of
one and then expose it as authoritative metadata.

Mark those results with unknown_nlink so existing inodes keep their
cached link count and new inodes receive the usual sane default.

This was tested against Samba with "server min protocol = NT1". Mount
the share using SMB1 with Unix extensions disabled:

  mount -t cifs //<server>/<share> /mnt/cifs \
        -o username=<user>,vers=1.0,nounix

Create three names for the same inode and cache its real link count:

  TESTDIR=/mnt/cifs/nlink-repro-$$
  mkdir "$TESTDIR"
  touch "$TESTDIR/file1"
  ln "$TESTDIR/file1" "$TESTDIR/file2"
  ln "$TESTDIR/file1" "$TESTDIR/file3"
  stat -c 'before open: %h' "$TESTDIR/file1"

Open the file and read the link count through the open descriptor:

  exec 3<"$TESTDIR/file1"
  stat -Lc 'after open: %h' /proc/$$/fd/3
  exec 3<&-

Clean up the test files:

  rm -f "$TESTDIR/file1" "$TESTDIR/file2" "$TESTDIR/file3"
  rmdir "$TESTDIR"

Before this change, the two stat commands report 3 and 1 because the
SMB1 open response overwrites the known link count. With this change,
both commands report 3.

Signed-off-by: Ze Tan <tanze@kylinos.cn>
---
 fs/smb/client/smb1ops.c   | 8 +++++++-
 fs/smb/client/smb2inode.c | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c
index dc5a8c1da623..7e2b29060f51 100644
--- a/fs/smb/client/smb1ops.c
+++ b/fs/smb/client/smb1ops.c
@@ -542,6 +542,7 @@ static int cifs_query_path_info(const unsigned int xid,
 
 	data->reparse_point = false;
 	data->adjust_tz = false;
+	data->unknown_nlink = false;
 
 	/*
 	 * First try CIFSSMBQPathInfo() function which returns more info
@@ -608,6 +609,7 @@ static int cifs_query_path_info(const unsigned int xid,
 				fi.EASize = di->EaSize;
 			}
 			fi.NumberOfLinks = cpu_to_le32(1);
+			data->unknown_nlink = true;
 			fi.DeletePending = 0;
 			fi.Directory = !!(le32_to_cpu(fi.Attributes) & ATTR_DIRECTORY);
 			cifs_buf_release(search_info.ntwrk_buf_start);
@@ -630,6 +632,8 @@ static int cifs_query_path_info(const unsigned int xid,
 		rc = SMBQueryInformation(xid, tcon, full_path, &fi, cifs_sb->local_nls,
 					 cifs_remap(cifs_sb));
 		data->adjust_tz = true;
+		if (!rc)
+			data->unknown_nlink = true;
 	} else if ((rc == -EOPNOTSUPP || rc == -EINVAL) && non_unicode_wildcard) {
 		/* Path with non-UNICODE wildcard character cannot exist. */
 		rc = -ENOENT;
@@ -893,8 +897,10 @@ static int cifs_open_file(const unsigned int xid, struct cifs_open_parms *oparms
 	else
 		rc = CIFS_open(xid, oparms, oplock, &fi);
 
-	if (!rc && data)
+	if (!rc && data) {
 		move_cifs_info_to_smb2(&data->fi, &fi);
+		data->unknown_nlink = true;
+	}
 
 	return rc;
 }
diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 213bc298cdf2..d4ae8a5ad463 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -576,6 +576,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 		idata->fi.EndOfFile = create_rsp->EndofFile;
 		if (le32_to_cpu(idata->fi.NumberOfLinks) == 0)
 			idata->fi.NumberOfLinks = cpu_to_le32(1); /* dummy value */
+		idata->unknown_nlink = true;
 		idata->fi.DeletePending = 0; /* successful open = not delete pending */
 		idata->fi.Directory = !!(le32_to_cpu(create_rsp->FileAttributes) & ATTR_DIRECTORY);
 
-- 
2.43.0

[PATCH 2/3] smb/client: preserve open info type across compound queries

[Ze Tan]

contains_posix_file_info describes the metadata stored in the
fi/posix_fi union. GET_REPARSE and QUERY_WSL_EA do not update that
union, so clearing the flag while processing those responses can make
POSIX metadata look like FILE_ALL_INFORMATION.

Set the flag when CREATE or a validated query response actually
populates the union, and leave it unchanged for auxiliary compound
operations. This also avoids changing the type when a query fails
before copying any metadata.

The issue can be reproduced against a Samba server with SMB3 UNIX
extensions enabled:

  mount -t cifs //<server>/<share> /mnt/cifs \
        -o vers=3.1.1,posix,reparse=nfs,actimeo=0
  mkfifo /mnt/cifs/test-fifo
  umount /mnt/cifs
  mount -t cifs //<server>/<share> /mnt/cifs \
        -o vers=3.1.1,posix,reparse=nfs,actimeo=0
  stat -c '%F %s' /mnt/cifs/test-fifo

Before this change, stat reports "fifo 1024" although the server-side
EOF is zero. After this change, it reports "fifo 0".

Fixes: 9df23801c83d ("smb311: failure to open files of length 1040 when mounting with SMB3.1.1 POSIX extensions")
Signed-off-by: Ze Tan <tanze@kylinos.cn>
---
 fs/smb/client/smb2inode.c | 9 +++++----
 fs/smb/client/smb2pdu.c   | 1 +
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index d4ae8a5ad463..058b05f7a3e5 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -574,6 +574,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 		idata->fi.Attributes = create_rsp->FileAttributes;
 		idata->fi.AllocationSize = create_rsp->AllocationSize;
 		idata->fi.EndOfFile = create_rsp->EndofFile;
+		idata->contains_posix_file_info = false;
 		if (le32_to_cpu(idata->fi.NumberOfLinks) == 0)
 			idata->fi.NumberOfLinks = cpu_to_le32(1); /* dummy value */
 		idata->unknown_nlink = true;
@@ -597,7 +598,6 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 		switch (cmds[i]) {
 		case SMB2_OP_QUERY_INFO:
 			idata = in_iov[i].iov_base;
-			idata->contains_posix_file_info = false;
 			if (rc == 0 && cfile && cfile->symlink_target) {
 				idata->symlink_target = kstrdup(cfile->symlink_target, GFP_KERNEL);
 				if (!idata->symlink_target)
@@ -610,6 +610,8 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 					le16_to_cpu(qi_rsp->OutputBufferOffset),
 					le32_to_cpu(qi_rsp->OutputBufferLength),
 					&rsp_iov[i + 1], sizeof(idata->fi), (char *)&idata->fi);
+				if (!rc)
+					idata->contains_posix_file_info = false;
 			}
 			SMB2_query_info_free(&rqst[num_rqst++]);
 			if (rc)
@@ -621,7 +623,6 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 			break;
 		case SMB2_OP_POSIX_QUERY_INFO:
 			idata = in_iov[i].iov_base;
-			idata->contains_posix_file_info = true;
 			if (rc == 0 && cfile && cfile->symlink_target) {
 				idata->symlink_target = kstrdup(cfile->symlink_target, GFP_KERNEL);
 				if (!idata->symlink_target)
@@ -635,6 +636,8 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 					le32_to_cpu(qi_rsp->OutputBufferLength),
 					&rsp_iov[i + 1], sizeof(idata->posix_fi) /* add SIDs */,
 					(char *)&idata->posix_fi);
+				if (!rc)
+					idata->contains_posix_file_info = true;
 			}
 			if (rc == 0)
 				rc = parse_posix_sids(idata, &rsp_iov[i + 1]);
@@ -706,7 +709,6 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 				idata = in_iov[i].iov_base;
 				idata->reparse.io.iov = *iov;
 				idata->reparse.io.buftype = resp_buftype[i + 1];
-				idata->contains_posix_file_info = false; /* BB VERIFY */
 				rbuf = reparse_buf_ptr(iov);
 				if (IS_ERR(rbuf)) {
 					rc = PTR_ERR(rbuf);
@@ -728,7 +730,6 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 		case SMB2_OP_QUERY_WSL_EA:
 			if (!rc) {
 				idata = in_iov[i].iov_base;
-				idata->contains_posix_file_info = false;
 				qi_rsp = rsp_iov[i + 1].iov_base;
 				data[0] = (u8 *)qi_rsp + le16_to_cpu(qi_rsp->OutputBufferOffset);
 				size[0] = le32_to_cpu(qi_rsp->OutputBufferLength);
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 4ce165e40657..72393e54fe2b 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -3372,6 +3372,7 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
 #endif /* CIFS_DEBUG2 */
 
 	if (file_info) {
+		buf->contains_posix_file_info = false;
 		file_info->CreationTime = rsp->CreationTime;
 		file_info->LastAccessTime = rsp->LastAccessTime;
 		file_info->LastWriteTime = rsp->LastWriteTime;
-- 
2.43.0

[PATCH 3/3] smb/client: decode reparse metadata using its payload type

[Ze Tan]

cifs_open_info_data stores FILE_ALL_INFORMATION and SMB3 POSIX query
information in a union. reparse_info_to_fattr() selects a union member
from the mount mode, while several directory checks always read
fi.Attributes.

The metadata can instead come from an SMB2 CREATE response on a POSIX
mount, or from a POSIX query while processing a reparse point. In those
cases the mount mode and hard-coded fi accesses select the wrong union
member.

See the procedures below:

  cifs_nt_open
    smb2_open_file
      SMB2_open
        data->fi = SMB2 CREATE response
        data->contains_posix_file_info = false
    cifs_get_inode_info
      reparse_info_to_fattr
        if (tcon->posix_extensions) // true
          smb311_posix_info_to_fattr
            data->posix_fi // wrong union member

  smb311_posix_get_fattr
    smb2_query_path_info
      smb2_compound_op
        data->posix_fi = SMB3 POSIX query response
        data->contains_posix_file_info = true
    reparse_info_to_fattr
      data->fi.Attributes // wrong union member

Add a common DOS attribute accessor and use contains_posix_file_info
both for attribute reads and for the final fattr conversion.

Signed-off-by: Ze Tan <tanze@kylinos.cn>
---
 fs/smb/client/inode.c     |  9 +++++----
 fs/smb/client/reparse.h   | 17 ++++++++++-------
 fs/smb/client/smb2inode.c |  6 ++++--
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index 0afff761aab9..e7ef50b74378 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -1215,7 +1215,7 @@ static int reparse_info_to_fattr(struct cifs_open_info_data *data,
 		break;
 	case IO_REPARSE_TAG_INTERNAL:
 		rc = 0;
-		if (le32_to_cpu(data->fi.Attributes) & ATTR_DIRECTORY) {
+		if (cifs_open_data_attrs(data) & ATTR_DIRECTORY) {
 			cifs_create_junction_fattr(fattr, sb);
 			goto out;
 		}
@@ -1239,7 +1239,7 @@ static int reparse_info_to_fattr(struct cifs_open_info_data *data,
 			 */
 			if (rc == -EOPNOTSUPP &&
 			    IS_REPARSE_TAG_NAME_SURROGATE(data->reparse.tag) &&
-			    (le32_to_cpu(data->fi.Attributes) & ATTR_DIRECTORY)) {
+			    (cifs_open_data_attrs(data) & ATTR_DIRECTORY)) {
 				rc = 0;
 				cifs_create_junction_fattr(fattr, sb);
 				goto out;
@@ -1257,13 +1257,14 @@ static int reparse_info_to_fattr(struct cifs_open_info_data *data,
 		}
 
 		if (data->reparse.tag == IO_REPARSE_TAG_SYMLINK && !rc) {
-			bool directory = le32_to_cpu(data->fi.Attributes) & ATTR_DIRECTORY;
+			bool directory = cifs_open_data_attrs(data) & ATTR_DIRECTORY;
+
 			rc = smb2_fix_symlink_target_type(&data->symlink_target, directory, cifs_sb);
 		}
 		break;
 	}
 
-	if (tcon->posix_extensions)
+	if (data->contains_posix_file_info)
 		smb311_posix_info_to_fattr(fattr, data, sb);
 	else
 		cifs_open_info_to_fattr(fattr, data, sb);
diff --git a/fs/smb/client/reparse.h b/fs/smb/client/reparse.h
index 0164dc47bdfd..49efd85b1e94 100644
--- a/fs/smb/client/reparse.h
+++ b/fs/smb/client/reparse.h
@@ -98,15 +98,21 @@ static inline bool reparse_inode_match(struct inode *inode,
 		timespec64_equal(&ctime, &fattr->cf_ctime);
 }
 
+static inline u32 cifs_open_data_attrs(const struct cifs_open_info_data *data)
+{
+	if (data->contains_posix_file_info)
+		return le32_to_cpu(data->posix_fi.DosAttributes);
+
+	return le32_to_cpu(data->fi.Attributes);
+}
+
 static inline bool cifs_open_data_reparse(struct cifs_open_info_data *data)
 {
-	u32 attrs;
-	bool ret;
+	u32 attrs = cifs_open_data_attrs(data);
 
 	if (data->contains_posix_file_info) {
 		struct smb311_posix_qinfo *fi = &data->posix_fi;
 
-		attrs = le32_to_cpu(fi->DosAttributes);
 		if (data->reparse_point) {
 			attrs |= ATTR_REPARSE_POINT;
 			fi->DosAttributes = cpu_to_le32(attrs);
@@ -115,16 +121,13 @@ static inline bool cifs_open_data_reparse(struct cifs_open_info_data *data)
 	} else {
 		struct smb2_file_all_info *fi = &data->fi;
 
-		attrs = le32_to_cpu(fi->Attributes);
 		if (data->reparse_point) {
 			attrs |= ATTR_REPARSE_POINT;
 			fi->Attributes = cpu_to_le32(attrs);
 		}
 	}
 
-	ret = attrs & ATTR_REPARSE_POINT;
-
-	return ret;
+	return attrs & ATTR_REPARSE_POINT;
 }
 
 bool cifs_reparse_point_to_fattr(struct cifs_sb_info *cifs_sb,
diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 058b05f7a3e5..bcaa44814b71 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -22,6 +22,7 @@
 #include "smb2glob.h"
 #include "smb2proto.h"
 #include "cached_dir.h"
+#include "reparse.h"
 #include "../common/smb2status.h"
 #include "../common/smbfsctl.h"
 
@@ -1002,12 +1003,13 @@ int smb2_query_path_info(const unsigned int xid,
 		/*
 		 * If the symlink was already parsed in create response then it is needed to fix
 		 * its type now (after the second call with OPEN_REPARSE_POINT which filled the
-		 * data->fi.Attributes). If the symlink was not parsed in create response then
+		 * metadata attributes). If the symlink was not parsed in create response then
 		 * the data->symlink_target was not filled yet and then the type will be fixed
 		 * later after data->symlink_target is filled.
 		 */
 		if (data->reparse.tag == IO_REPARSE_TAG_SYMLINK && !rc && data->symlink_target) {
-			bool directory = le32_to_cpu(data->fi.Attributes) & ATTR_DIRECTORY;
+			bool directory = cifs_open_data_attrs(data) & ATTR_DIRECTORY;
+
 			rc = smb2_fix_symlink_target_type(&data->symlink_target, directory, cifs_sb);
 		}
 		break;
-- 
2.43.0