Re: [RFC][PATCHSET] hopefully saner handling of pathnames in cifs

[Steve French <smfrench@gmail.com>]

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

FYI - on a loosely related point  about / to \ conversion, I had been
experimenting with moving the conversion of '/' to '\' later depending
on connection type (see attached WIP patch for example).


On Sun, Mar 21, 2021 at 9:40 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sun, Mar 21, 2021 at 09:19:53PM -0500, Steve French wrote:
> > automated tests failed so will need to dig in a little more and see
> > what is going on
> >
> > http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/533
>
> <looks>
>
> Oh, bugger...  I think I see a braino that might be responsible for that;
> whether it's all that's going on or not, that's an obvious bug.  Incremental
> for that one would be
>
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 3febf667d119..ed16f75ac0fa 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -132,7 +132,7 @@ build_path_from_dentry_optional_prefix(struct dentry *direntry, void *page,
>         }
>         if (dfsplen) {
>                 s -= dfsplen;
> -               memcpy(page, tcon->treeName, dfsplen);
> +               memcpy(s, tcon->treeName, dfsplen);
>                 if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) {
>                         int i;
>                         for (i = 0; i < dfsplen; i++) {
>
>
> Folded and force-pushed (same branch).  My apologies...



-- 
Thanks,

Steve

[-- Attachment #2: slash-v2.patch --]
[-- Type: text/x-patch, Size: 4497 bytes --]

diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
index 9bd03a231032..4c0cc0677d80 100644
--- a/fs/cifs/cifs_unicode.c
+++ b/fs/cifs/cifs_unicode.c
@@ -98,6 +98,9 @@ convert_sfm_char(const __u16 src_char, char *target)
 	case SFM_PERIOD:
 		*target = '.';
 		break;
+	case SFM_SLASH:
+		*target = '\\';
+		break;
 	default:
 		return false;
 	}
@@ -401,6 +404,8 @@ static __le16 convert_to_sfu_char(char src_char)
 	return dest_char;
 }
 
+#define UCS2_SLASH 0x005C
+
 static __le16 convert_to_sfm_char(char src_char, bool end_of_string)
 {
 	__le16 dest_char;
@@ -431,6 +436,9 @@ static __le16 convert_to_sfm_char(char src_char, bool end_of_string)
 	case '|':
 		dest_char = cpu_to_le16(SFM_PIPE);
 		break;
+	case '\\':
+		dest_char = cpu_to_le16(SFM_SLASH);
+		break;
 	case '.':
 		if (end_of_string)
 			dest_char = cpu_to_le16(SFM_PERIOD);
@@ -443,6 +451,10 @@ static __le16 convert_to_sfm_char(char src_char, bool end_of_string)
 		else
 			dest_char = 0;
 		break;
+	case '/':
+
+		dest_char = cpu_to_le16(UCS2_SLASH);
+		break;
 	default:
 		dest_char = 0;
 	}
@@ -502,11 +516,7 @@ cifsConvertToUTF16(__le16 *target, const char *source, int srclen,
 			dst_char = convert_to_sfm_char(src_char, end_of_string);
 		} else
 			dst_char = 0;
-		/*
-		 * FIXME: We can not handle remapping backslash (UNI_SLASH)
-		 * until all the calls to build_path_from_dentry are modified,
-		 * as they use backslash as separator.
-		 */
+
 		if (dst_char == 0) {
 			charlen = cp->char2uni(source + i, srclen - i, &tmp);
 			dst_char = cpu_to_le16(tmp);
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 3de3c5908a72..f0f96dddc483 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1430,10 +1430,10 @@ CIFS_FILE_SB(struct file *file)
 
 static inline char CIFS_DIR_SEP(const struct cifs_sb_info *cifs_sb)
 {
-	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS)
+/*	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) */
 		return '/';
-	else
-		return '\\';
+/*	else
+		return '\\'; */
 }
 
 static inline void
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 97ac363b5df1..f534e2f991d9 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -209,12 +210,18 @@ check_name(struct dentry *direntry, struct cifs_tcon *tcon)
 		     le32_to_cpu(tcon->fsAttrInfo.MaxPathNameComponentLength)))
 		return -ENAMETOOLONG;
 
-	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS)) {
-		for (i = 0; i < direntry->d_name.len; i++) {
-			if (direntry->d_name.name[i] == '\\') {
-				cifs_dbg(FYI, "Invalid file name\n");
-				return -EINVAL;
-			}
+	/*
+	 * SMB3.1.1 POSIX Extensions, CIFS Unix Extensions and SFM mappings
+	 * allow \ in paths (or in latter case remaps \ to 0xF026)
+	 */
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) ||
+	    (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SFM_CHR))
+		return 0;
+		 
+	for (i = 0; i < direntry->d_name.len; i++) {
+		if (direntry->d_name.name[i] == '\\') {
+			cifs_dbg(FYI, "Invalid file name\n");
+			return -EINVAL;
 		}
 	}
 	return 0;
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 82e176720ca6..9361644f7310 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -1186,7 +1186,7 @@ int update_super_prepath(struct cifs_tcon *tcon, char *prefix)
 			goto out;
 		}
 
-		convert_delimiter(cifs_sb->prepath, CIFS_DIR_SEP(cifs_sb));
+		convert_delimiter(cifs_sb->prepath, CIFS_DIR_SEP(cifs_sb)); /* BB Does this need to be changed for / ? */
 	} else
 		cifs_sb->prepath = NULL;
 
diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index 60d4bd1eae2b..ce4f00069653 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -476,13 +476,17 @@ cifs_convert_path_to_utf16(const char *from, struct cifs_sb_info *cifs_sb)
 	if (from[0] == '\\')
 		start_of_path = from + 1;
 
-	/* SMB311 POSIX extensions paths do not include leading slash */
-	else if (cifs_sb_master_tlink(cifs_sb) &&
-		 cifs_sb_master_tcon(cifs_sb)->posix_extensions &&
-		 (from[0] == '/')) {
-		start_of_path = from + 1;
-	} else
-		start_of_path = from;
+	start_of_path = from;
+	/*
+	 * Only old CIFS Unix extensions paths include leading slash
+	 * Need to skip if for SMB3.1.1 POSIX Extensions and SMB1/2/3
+	 */
+	if (from[0] == '/') {
+		if (((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) == false) ||
+		    (cifs_sb_master_tlink(cifs_sb) &&
+		     (cifs_sb_master_tcon(cifs_sb)->posix_extensions)))
+			start_of_path = from + 1;
+	}
 
 	to = cifs_strndup_to_utf16(start_of_path, PATH_MAX, &len,
 				   cifs_sb->local_nls, map_type);