Add full_path_type arg to cifs_build_path_to_root()

PATCH In review Aurélien Aptel 1 participant 2 messages Apr 20, 2016 – Aug 1, 2016

Subsystems (2)

SMB client (CIFS) Kernel SMB3 server (ksmbd)

lore

Share & tools

lore

Aurélien Aptel

author

2016-04-20 16:16 UTC #0 lore raw

PATCH 1/1 Add full_path_type arg to cifs_build_path_to_root()

Hi,

When cifs_get_root() calls cifs_build_path_to_root(), it expects a full path from the root, even in the presence of a DFS link.

e.g. in the case of a DFS link like

    //A/shareA/link -> //B/shareB/sub/dir/

When mounting shareA and doing a "cd link", cifs_get_root() was getting

    "//B/shareB//sub/dir"

Instead of

    "/sub/dir"

Resulting in

sh: cd: link: No such file or directory

When the DFS link points to a share without a sub-dir suffix (e.g. //A/shareA/link -> //B/shareB/) cifs_build_path_to_root() returns an empty string which seems inconsistent. I would expect it have the same behaviour as when there is a sub-dir and prefix the whole DFS UNC path (it returns "" where I would expect "//B/shareB/"). This might be a bug? I will try to return the expected path to see what happens.

I would like to know what you think.

There are 2 ways to fix this:
- never prefix an UNC path, even when using a DFS link i.e. remove all code dealing with DFS from cifs_build_path_to_root)
- add a new arg to decide the behaviour of cifs_build_path_to_root().

The attached patch implements the second solution and fixes the problem.
- lets the caller of cifs_build_path_to_root() decide the behaviour via a new argument.
- make cifs_get_root() always use a simple path.
- keep old behaviour on the other code paths (only one, in cifs_do_mount)

Signature · 5 lines

Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

From 3f877b42d8d2850b4c384e0c394dc7b71fca491b Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <aaptel-IBi9RG/[email protected]>
Date: Wed, 20 Apr 2016 16:32:46 +0200
Subject: [PATCH 1/2] fs/cifs: Add full_path_type enum

Signed-off-by Aurelien Aptel <aaptel-IBi9RG/[email protected]>

---
 fs/cifs/cifsglob.h | 6 ++++++
 1 file changed, 6 insertions(+)

fs/cifs/cifsglob.h +6 -0

index f2cc0b3..2b5b796 100644
@@ -1644,4 +1644,10 @@ extern struct smb_version_values smb302_values;
 #define ALT_SMB311_VERSION_STRING "3.11"
 extern struct smb_version_operations smb311_operations;
 extern struct smb_version_values smb311_values;
+
+enum full_path_type {
+	CIFS_NO_PREFIX,
+	CIFS_WITH_DFS_UNC_PREFIX,
+};
+
 #endif	/* _CIFS_GLOB_H */

Signature · 1 line

2.1.4

From 1caea1e53763df316b6c7b4cc9dc7b54c3f3300e Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <aaptel-IBi9RG/[email protected]>
Date: Wed, 20 Apr 2016 16:34:01 +0200
Subject: [PATCH 2/2] fs/cifs: Add full_path_type arg to
cifs_build_path_to_root()

Only add the server prefix in the case of a DFS link when explicitely asked.

This function is called from:
- cifs_mount()
- cifs_get_root()

The later expects a full path from the root, even in the presence of a DFS link.

e.g. in the case of a DFS link like

    //A/shareA/link -> //B/shareB/sub/dir/

When doing a "cd link", cifs_get_root() was getting

    "//B/shareB//sub/dir"

Instead of

    "/sub/dir"

Resulting in

sh: cd: link: No such file or directory

This commit
- lets the caller of cifs_build_path_to_root() decide the behaviour via a new argument.
- make cifs_get_root() always use a simple path.

Thanks to Josef Cejka <jcejka-IBi9RG/[email protected]> for finding the bug&fix.

Reported-by Fons Jongh <fons.dejongh-/[email protected]>

Signed-off-by Aurelien Aptel <aaptel-IBi9RG/[email protected]>

---
 fs/cifs/cifsfs.c    | 3 ++-
 fs/cifs/cifsproto.h | 3 ++-
 fs/cifs/connect.c   | 2 +-
 fs/cifs/dir.c       | 6 ++++--
 4 files changed, 9 insertions(+), 5 deletions(-)

fs/cifs/cifsfs.c +2 -1

index 8920156..d56a5ed 100644
@@ -607,7 +607,8 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
 	char sep;
 
 	full_path = cifs_build_path_to_root(vol, cifs_sb,
-					    cifs_sb_master_tcon(cifs_sb));
+					    cifs_sb_master_tcon(cifs_sb), CIFS_NO_PREFIX);
+
 	if (full_path == NULL)
 		return ERR_PTR(-ENOMEM);

fs/cifs/cifsproto.h +2 -1

index eed7ff50..f6a0902 100644
@@ -63,7 +63,8 @@ extern void exit_cifs_idmap(void);
 extern char *build_path_from_dentry(struct dentry *);
 extern char *cifs_build_path_to_root(struct smb_vol *vol,
 				     struct cifs_sb_info *cifs_sb,
-				     struct cifs_tcon *tcon);
+				     struct cifs_tcon *tcon,
+	                             enum full_path_type);
 extern char *build_wildcard_path_from_dentry(struct dentry *direntry);
 extern char *cifs_compose_mount_options(const char *sb_mountdata,
 		const char *fullpath, const struct dfs_info3_param *ref,

fs/cifs/connect.c +1 -1

index 6f62ac8..b054218 100644
@@ -3660,7 +3660,7 @@ remote_path_check:
 		/*
 		 * cifs_build_path_to_root works only when we have a valid tcon
 		 */
-		full_path = cifs_build_path_to_root(volume_info, cifs_sb, tcon);
+		full_path = cifs_build_path_to_root(volume_info, cifs_sb, tcon, CIFS_SERVER_PREFIX);
 		if (full_path == NULL) {
 			rc = -ENOMEM;
 			goto mount_fail_check;

fs/cifs/dir.c +4 -2

index c3eb998..b5fe596 100644
@@ -47,7 +47,7 @@ renew_parental_timestamps(struct dentry *direntry)
 
 char *
 cifs_build_path_to_root(struct smb_vol *vol, struct cifs_sb_info *cifs_sb,
-			struct cifs_tcon *tcon)
+			struct cifs_tcon *tcon, enum full_path_type prefix)
 {
 	int pplen = vol->prepath ? strlen(vol->prepath) + 1 : 0;
 	int dfsplen;

@@ -59,7 +59,8 @@ cifs_build_path_to_root(struct smb_vol *vol, struct cifs_sb_info *cifs_sb,
 		return full_path;
 	}
 
-	if (tcon->Flags & SMB_SHARE_IS_IN_DFS)
+
+	if (prefix == CIFS_WITH_DFS_UNC_PREFIX && (tcon->Flags & SMB_SHARE_IS_IN_DFS))
 		dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1);
 	else
 		dfsplen = 0;

@@ -68,6 +69,7 @@ cifs_build_path_to_root(struct smb_vol *vol, struct cifs_sb_info *cifs_sb,
 	if (full_path == NULL)
 		return full_path;
 
+
 	if (dfsplen)
 		strncpy(full_path, tcon->treeName, dfsplen);
 	full_path[dfsplen] = CIFS_DIR_SEP(cifs_sb);

Signature · 1 line

2.1.4

Aurélien Aptel

author

2016-08-01 12:32 UTC #1 lore raw

Quoted 4 lines · On Wed, 20 Apr 2016 18:16:34 +0200 Aurélien Aptel…

There are 2 ways to fix this:
- never prefix an UNC path, even when using a DFS link i.e. remove all code dealing with DFS from cifs_build_path_to_root)
- add a new arg to decide the behaviour of cifs_build_path_to_root().

We have 2 customers reporting a bug involving this problem internally at SUSE. I'm letting you know the first solution (patch attached) will be in the next update of SUSE (SLE) kernels.

Signature · 5 lines

Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

From df54ca09437a11438e1ec880c80c92e93ba98d71 Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <aaptel-IBi9RG/[email protected]>
Date: Tue, 26 Apr 2016 17:59:06 +0200
Subject: [PATCH] fs/cifs: remove DFS handling from cifs_build_path_to_root()

This function is called from:
- cifs_mount()
- cifs_get_root()

The later expects a full path from the root, even in the presence of a DFS link.

e.g. in the case of a DFS link like

    //A/shareA/link -> //B/shareB/sub/dir/

When doing a "cd link", cifs_get_root() was getting

    "//B/shareB//sub/dir"

Instead of

    "/sub/dir"

Resulting in

sh: cd: link: No such file or directory

Thanks to Josef Cejka <jcejka-IBi9RG/[email protected]> for finding the bug&fix.

Reported-by Fons Jongh <fons.dejongh-/[email protected]>

Signed-off-by Aurelien Aptel <aaptel-IBi9RG/[email protected]>

---
 fs/cifs/dir.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

fs/cifs/dir.c +4 ~~-19~~

index fb0903f..05f7480 100644
@@ -49,31 +49,16 @@ char *
 cifs_build_path_to_root(struct smb_vol *vol, struct cifs_sb_info *cifs_sb,
 			struct cifs_tcon *tcon)
 {
-	int pplen = vol->prepath ? strlen(vol->prepath) + 1 : 0;
-	int dfsplen;
+	int pplen = vol->prepath ? strlen(vol->prepath) : 0;
 	char *full_path = NULL;
 
-	/* if no prefix path, simply set path to the root of share to "" */
-	if (pplen == 0) {
-		full_path = kzalloc(1, GFP_KERNEL);
-		return full_path;
-	}
-
-	if (tcon->Flags & SMB_SHARE_IS_IN_DFS)
-		dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1);
-	else
-		dfsplen = 0;
-
-	full_path = kmalloc(dfsplen + pplen + 1, GFP_KERNEL);
+	full_path = kmalloc(pplen + 1, GFP_KERNEL);
 	if (full_path == NULL)
 		return full_path;
 
-	if (dfsplen)
-		strncpy(full_path, tcon->treeName, dfsplen);
-	full_path[dfsplen] = CIFS_DIR_SEP(cifs_sb);
-	strncpy(full_path + dfsplen + 1, vol->prepath, pplen);
+	strncpy(full_path, vol->prepath, pplen);
 	convert_delimiter(full_path, CIFS_DIR_SEP(cifs_sb));
-	full_path[dfsplen + pplen] = 0; /* add trailing null */
+	full_path[pplen] = 0; /* add trailing null */
 	return full_path;
 }

Signature · 1 line

2.1.4