dfs path construction fix - send dfs paths on all path based operations on share in dfs

dfs path construction fix - send dfs paths on all path based operations on share in dfs

[Steve French]

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

Samba was not handling paths without the \\server\share prefix (which
our current code sends on QueryPathInfo) when "SHARE_IN_DFS" on
operations such as rmdir and delete.

This patch fixes that:

-- 
Thanks,

Steve

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-path-requests-when-share-in-dfs.patch --]
[-- Type: text/x-diff; name=fix-path-requests-when-share-in-dfs.patch, Size: 6936 bytes --]

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index e4e0078..05afe33 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -49,18 +49,25 @@ build_path_from_dentry(struct dentry *direntry)
 	struct dentry *temp;
 	int namelen;
 	int pplen;
+	int dfsplen;
 	char *full_path;
 	char dirsep;
+	struct cifs_sb_info *cifs_sb;
 
 	if (direntry == NULL)
 		return NULL;  /* not much we can do if dentry is freed and
 		we need to reopen the file after it was closed implicitly
 		when the server crashed */
 
-	dirsep = CIFS_DIR_SEP(CIFS_SB(direntry->d_sb));
-	pplen = CIFS_SB(direntry->d_sb)->prepathlen;
+	cifs_sb = CIFS_SB(direntry->d_sb);
+	dirsep = CIFS_DIR_SEP(cifs_sb);
+	pplen = cifs_sb->prepathlen;
+	if (cifs_sb->tcon && (cifs_sb->tcon->Flags & SMB_SHARE_IS_IN_DFS))
+		dfsplen = strnlen(cifs_sb->tcon->treeName, MAX_TREE_SIZE + 1);
+	else
+		dfsplen = 0;
 cifs_bp_rename_retry:
-	namelen = pplen;
+	namelen = pplen + dfsplen;
 	for (temp = direntry; !IS_ROOT(temp);) {
 		namelen += (1 + temp->d_name.len);
 		temp = temp->d_parent;
@@ -91,7 +98,7 @@ cifs_bp_rename_retry:
 			return NULL;
 		}
 	}
-	if (namelen != pplen) {
+	if (namelen != pplen + dfsplen) {
 		cERROR(1,
 		       ("did not end path lookup where expected namelen is %d",
 			namelen));
@@ -107,7 +114,18 @@ cifs_bp_rename_retry:
 	   since the '\' is a valid posix character so we can not switch
 	   those safely to '/' if any are found in the middle of the prepath */
 	/* BB test paths to Windows with '/' in the midst of prepath */
-	strncpy(full_path, CIFS_SB(direntry->d_sb)->prepath, pplen);
+
+	if (dfsplen) {
+		strncpy(full_path, cifs_sb->tcon->treeName, dfsplen);
+		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) {
+			int i;
+			for (i = 0; i < dfsplen; i++) {
+				if (full_path[i] == '\\')
+					full_path[i] = '/';
+			}
+		}
+	}
+	strncpy(full_path + dfsplen, CIFS_SB(direntry->d_sb)->prepath, pplen);
 	return full_path;
 }
 
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index fcbdbb6..b867606 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -161,52 +161,18 @@ static void cifs_unix_info_to_inode(struct inode *inode,
 	spin_unlock(&inode->i_lock);
 }
 
-static const unsigned char *cifs_get_search_path(struct cifs_sb_info *cifs_sb,
-						const char *search_path)
-{
-	int tree_len;
-	int path_len;
-	int i;
-	char *tmp_path;
-	struct cifsTconInfo *pTcon = cifs_sb->tcon;
-
-	if (!(pTcon->Flags & SMB_SHARE_IS_IN_DFS))
-		return search_path;
-
-	/* use full path name for working with DFS */
-	tree_len = strnlen(pTcon->treeName, MAX_TREE_SIZE + 1);
-	path_len = strnlen(search_path, MAX_PATHCONF);
-
-	tmp_path = kmalloc(tree_len+path_len+1, GFP_KERNEL);
-	if (tmp_path == NULL)
-		return search_path;
-
-	strncpy(tmp_path, pTcon->treeName, tree_len);
-	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS)
-		for (i = 0; i < tree_len; i++) {
-			if (tmp_path[i] == '\\')
-				tmp_path[i] = '/';
-		}
-	strncpy(tmp_path+tree_len, search_path, path_len);
-	tmp_path[tree_len+path_len] = 0;
-	return tmp_path;
-}
-
 int cifs_get_inode_info_unix(struct inode **pinode,
-	const unsigned char *search_path, struct super_block *sb, int xid)
+	const unsigned char *full_path, struct super_block *sb, int xid)
 {
 	int rc = 0;
 	FILE_UNIX_BASIC_INFO findData;
 	struct cifsTconInfo *pTcon;
 	struct inode *inode;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
-	const unsigned char *full_path;
 	bool is_dfs_referral = false;
 
 	pTcon = cifs_sb->tcon;
-	cFYI(1, ("Getting info on %s", search_path));
-
-	full_path = cifs_get_search_path(cifs_sb, search_path);
+	cFYI(1, ("Getting info on %s", full_path));
 
 try_again_CIFSSMBUnixQPathInfo:
 	/* could have done a find first instead but this returns more info */
@@ -218,10 +184,6 @@ try_again_CIFSSMBUnixQPathInfo:
 	if (rc) {
 		if (rc == -EREMOTE && !is_dfs_referral) {
 			is_dfs_referral = true;
-			if (full_path != search_path) {
-				kfree(full_path);
-				full_path = search_path;
-			}
 			goto try_again_CIFSSMBUnixQPathInfo;
 		}
 		goto cgiiu_exit;
@@ -271,8 +233,6 @@ try_again_CIFSSMBUnixQPathInfo:
 		cifs_set_ops(inode, is_dfs_referral);
 	}
 cgiiu_exit:
-	if (full_path != search_path)
-		kfree(full_path);
 	return rc;
 }
 
@@ -380,20 +340,19 @@ static int get_sfu_mode(struct inode *inode,
 }
 
 int cifs_get_inode_info(struct inode **pinode,
-	const unsigned char *search_path, FILE_ALL_INFO *pfindData,
+	const unsigned char *full_path, FILE_ALL_INFO *pfindData,
 	struct super_block *sb, int xid, const __u16 *pfid)
 {
 	int rc = 0;
 	struct cifsTconInfo *pTcon;
 	struct inode *inode;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
-	const unsigned char *full_path = NULL;
 	char *buf = NULL;
 	bool adjustTZ = false;
 	bool is_dfs_referral = false;
 
 	pTcon = cifs_sb->tcon;
-	cFYI(1, ("Getting info on %s", search_path));
+	cFYI(1, ("Getting info on %s", full_path));
 
 	if ((pfindData == NULL) && (*pinode != NULL)) {
 		if (CIFS_I(*pinode)->clientCanCacheRead) {
@@ -409,8 +368,6 @@ int cifs_get_inode_info(struct inode **pinode,
 			return -ENOMEM;
 		pfindData = (FILE_ALL_INFO *)buf;
 
-		full_path = cifs_get_search_path(cifs_sb, search_path);
-
 try_again_CIFSSMBQPathInfo:
 		/* could do find first instead but this returns more info */
 		rc = CIFSSMBQPathInfo(xid, pTcon, full_path, pfindData,
@@ -432,10 +389,6 @@ try_again_CIFSSMBQPathInfo:
 	if (rc) {
 		if (rc == -EREMOTE && !is_dfs_referral) {
 			is_dfs_referral = true;
-			if (full_path != search_path) {
-				kfree(full_path);
-				full_path = search_path;
-			}
 			goto try_again_CIFSSMBQPathInfo;
 		}
 		goto cgii_exit;
@@ -470,7 +423,7 @@ try_again_CIFSSMBQPathInfo:
 				__u64 inode_num;
 
 				rc1 = CIFSGetSrvInodeNumber(xid, pTcon,
-					search_path, &inode_num,
+					full_path, &inode_num,
 					cifs_sb->local_nls,
 					cifs_sb->mnt_cifs_flags &
 						CIFS_MOUNT_MAP_SPECIAL_CHR);
@@ -539,7 +492,7 @@ try_again_CIFSSMBQPathInfo:
 			   (cifsInfo->cifsAttrs & ATTR_SYSTEM)) {
 			if (decode_sfu_inode(inode,
 					 le64_to_cpu(pfindData->EndOfFile),
-					 search_path,
+					 full_path,
 					 cifs_sb, xid))
 				cFYI(1, ("Unrecognized sfu inode type"));
 
@@ -582,12 +535,12 @@ try_again_CIFSSMBQPathInfo:
 		/* fill in 0777 bits from ACL */
 		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_ACL) {
 			cFYI(1, ("Getting mode bits from ACL"));
-			acl_to_uid_mode(inode, search_path, pfid);
+			acl_to_uid_mode(inode, full_path, pfid);
 		}
 #endif
 		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL) {
 			/* fill in remaining high mode bits e.g. SUID, VTX */
-			get_sfu_mode(inode, search_path, cifs_sb, xid);
+			get_sfu_mode(inode, full_path, cifs_sb, xid);
 		} else if (atomic_read(&cifsInfo->inUse) == 0) {
 			inode->i_uid = cifs_sb->mnt_uid;
 			inode->i_gid = cifs_sb->mnt_gid;
@@ -599,8 +552,6 @@ try_again_CIFSSMBQPathInfo:
 		cifs_set_ops(inode, is_dfs_referral);
 	}
 cgii_exit:
-	if (full_path != search_path)
-		kfree(full_path);
 	kfree(buf);
 	return rc;
 }

Re: dfs path construction fix - send dfs paths on all path based operations on share in dfs

[Igor Mammedov]

Steve French wrote:
> Samba was not handling paths without the \\server\share prefix (which
> our current code sends on QueryPathInfo) when "SHARE_IN_DFS" on
> operations such as rmdir and delete.
> 
> This patch fixes that:


Attempted to test patch and

-
 at the first attempt I've got oops caused by a missing CIFSGetDFSRefer patch,
 after digging it up and applying it, oops is disappeared. But it doesn't
 help much because your patch broke traversal over a dfs link,  at the moment
 dfs lookup relies on the second lookup attempt with a short path name (without
 tree name).
 BTW, you said that you almost made patch for a fake dfs inode,  it could fix
 things up.

+
 mkdir and rmdir on a dfs enabled share work (at least with samba), can't check 
 it on MS however.

PS:
 commiting CIFSGetDFSRefer patch will help to avoid oops-es and more people
 would be able to test the DFS related patches.

-- 

Best regards,

-------------------------
Igor Mammedov,
niallain "at" gmail.com

Re: dfs path construction fix - send dfs paths on all path based operations on share in dfs

[Steve French]

just checked in some of the GetDFSRefer cleanup into cifs-2.6. (with
minor style changes)

Missing still:
1) faking up inode after initial PATH_NOT_COVERED
2) cleanup of the code which parses the SMB response in GetDFSRefer
function in fs/cifs/cifssmb.c

On Wed, May 14, 2008 at 8:53 PM, Steve French <smfrench@gmail.com> wrote:
> I found the december version - and checked in part.   The next two
> checkins will be:
>
> 1) the part of GetDFSRefer before the smb send which should work as is
> 2) the post-processing (looking at the SMB response buffer and taking
> the dfs structs out of it) which Christoph says was too big and needs
> to be put in a different function - so needs more coding
>
> On Wed, May 14, 2008 at 1:50 PM, Steve French <smfrench@gmail.com> wrote:
>> Do you have a newer version of the GetDFSRefer patch - or point me to
>> which you think is most current?
>>
>> On Wed, May 14, 2008 at 6:50 AM, Igor Mammedov <niallain@gmail.com> wrote:
>>> Steve French wrote:
>>>> Samba was not handling paths without the \\server\share prefix (which
>>>> our current code sends on QueryPathInfo) when "SHARE_IN_DFS" on
>>>> operations such as rmdir and delete.
>>>>
>>>> This patch fixes that:
>>>
>>>
>>> Attempted to test patch and
>>>
>>> -
>>>  at the first attempt I've got oops caused by a missing CIFSGetDFSRefer patch,
>>>  after digging it up and applying it, oops is disappeared. But it doesn't
>>>  help much because your patch broke traversal over a dfs link,  at the moment
>>>  dfs lookup relies on the second lookup attempt with a short path name (without
>>>  tree name).
>>>  BTW, you said that you almost made patch for a fake dfs inode,  it could fix
>>>  things up.
>>>
>>> +
>>>  mkdir and rmdir on a dfs enabled share work (at least with samba), can't check
>>>  it on MS however.
>>>
>>> PS:
>>>  commiting CIFSGetDFSRefer patch will help to avoid oops-es and more people
>>>  would be able to test the DFS related patches.
>>>
>>> --
>>>
>>> Best regards,
>>>
>>> -------------------------
>>> Igor Mammedov,
>>> niallain "at" gmail.com
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>> --
>> Thanks,
>>
>> Steve
>>
>
>
>
> --
> Thanks,
>
> Steve
>



-- 
Thanks,

Steve

Re: dfs path construction fix - send dfs paths on all path based operations on share in dfs

[Igor Mammedov]

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

Steve French wrote:
> Samba was not handling paths without the \\server\share prefix (which
> our current code sends on QueryPathInfo) when "SHARE_IN_DFS" on
> operations such as rmdir and delete.
> 
> This patch fixes that:

A set of patches that fixes path handling (broken by 646dd539878a194) 
in the DFS related parts of code in accordance with
a new 'build_path_from_dentry' behavior.


-- 

Best regards,

-------------------------
Igor Mammedov,
niallain "at" gmail.com





[-- Attachment #2: 0002-Fixed-DFS-code-to-work-with-new-build_path_from_den.patch --]
[-- Type: text/x-patch, Size: 2335 bytes --]

>From b5a37238de56db6a99db746b169fe3deba27b510 Mon Sep 17 00:00:00 2001
From: Igor Mammedov <niallain@gmail.com>
Date: Fri, 16 May 2008 13:10:32 +0400
Subject: [PATCH] Fixed DFS code to work with new 'build_path_from_dentry', that returns
 full path if share in the dfs, now.

Signed-off-by: Igor Mammedov <niallain@gmail.com>
---
 fs/cifs/cifs_dfs_ref.c |   49 +-----------------------------------------------
 1 files changed, 1 insertions(+), 48 deletions(-)

diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
index f6fdecf..d82374c 100644
--- a/fs/cifs/cifs_dfs_ref.c
+++ b/fs/cifs/cifs_dfs_ref.c
@@ -219,53 +219,6 @@ static struct vfsmount *cifs_dfs_do_refmount(const struct vfsmount *mnt_parent,
 
 }
 
-static char *build_full_dfs_path_from_dentry(struct dentry *dentry)
-{
-	char *full_path = NULL;
-	char *search_path;
-	char *tmp_path;
-	size_t l_max_len;
-	struct cifs_sb_info *cifs_sb;
-
-	if (dentry->d_inode == NULL)
-		return NULL;
-
-	cifs_sb = CIFS_SB(dentry->d_inode->i_sb);
-
-	if (cifs_sb->tcon == NULL)
-		return NULL;
-
-	search_path = build_path_from_dentry(dentry);
-	if (search_path == NULL)
-		return NULL;
-
-	if (cifs_sb->tcon->Flags & SMB_SHARE_IS_IN_DFS) {
-		int i;
-		/* we should use full path name for correct working with DFS */
-		l_max_len = strnlen(cifs_sb->tcon->treeName, MAX_TREE_SIZE+1) +
-					strnlen(search_path, MAX_PATHCONF) + 1;
-		tmp_path = kmalloc(l_max_len, GFP_KERNEL);
-		if (tmp_path == NULL) {
-			kfree(search_path);
-			return NULL;
-		}
-		strncpy(tmp_path, cifs_sb->tcon->treeName, l_max_len);
-		tmp_path[l_max_len-1] = 0;
-		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS)
-			for (i = 0; i < l_max_len; i++) {
-				if (tmp_path[i] == '\\')
-					tmp_path[i] = '/';
-			}
-		strncat(tmp_path, search_path, l_max_len - strlen(tmp_path));
-
-		full_path = tmp_path;
-		kfree(search_path);
-	} else {
-		full_path = search_path;
-	}
-	return full_path;
-}
-
 static int add_mount_helper(struct vfsmount *newmnt, struct nameidata *nd,
 				struct list_head *mntlist)
 {
@@ -333,7 +286,7 @@ cifs_dfs_follow_mountpoint(struct dentry *dentry, struct nameidata *nd)
 		goto out_err;
 	}
 
-	full_path = build_full_dfs_path_from_dentry(dentry);
+	full_path = build_path_from_dentry(dentry);
 	if (full_path == NULL) {
 		rc = -ENOMEM;
 		goto out_err;
-- 
1.5.3.7


[-- Attachment #3: 0003-Fixed-inode-lookup-code-DFS-related-part-to-suppor.patch --]
[-- Type: text/x-patch, Size: 1473 bytes --]

>From fd33ede1ca0b157ecf26711bdf12c2b15336e0cd Mon Sep 17 00:00:00 2001
From: Igor Mammedov <niallain@gmail.com>
Date: Fri, 16 May 2008 13:24:59 +0400
Subject: [PATCH] Fixed inode lookup code (DFS related part) to support
 new build_path_from_dentry behaviour.

Signed-off-by: Igor Mammedov <niallain@gmail.com>
---
 fs/cifs/inode.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 9d9b56a..c7c5242 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -184,6 +184,9 @@ try_again_CIFSSMBUnixQPathInfo:
 	if (rc) {
 		if (rc == -EREMOTE && !is_dfs_referral) {
 			is_dfs_referral = true;
+			full_path = strchr(full_path + 2, '/');
+			full_path++;
+			full_path = strchr(full_path + 2, '/');
 			goto try_again_CIFSSMBUnixQPathInfo;
 		}
 		goto cgiiu_exit;
@@ -230,6 +233,11 @@ try_again_CIFSSMBUnixQPathInfo:
 			(unsigned long) inode->i_size,
 			(unsigned long long)inode->i_blocks));
 
+		if (is_dfs_referral && ((inode->i_mode & S_IFMT) == S_IFLNK)) {
+			inode->i_mode &= ~S_IFLNK;
+			inode->i_mode |= S_IFDIR;
+		}
+
 		cifs_set_ops(inode, is_dfs_referral);
 	}
 cgiiu_exit:
@@ -389,6 +397,9 @@ try_again_CIFSSMBQPathInfo:
 	if (rc) {
 		if (rc == -EREMOTE && !is_dfs_referral) {
 			is_dfs_referral = true;
+			full_path = strchr(full_path + 2, '\\');
+			full_path++;
+			full_path = strchr(full_path + 2, '\\');
 			goto try_again_CIFSSMBQPathInfo;
 		}
 		goto cgii_exit;
-- 
1.5.3.7