[linux-cifs-client] [PATCH] cifs: fix regression in DFS referral option parsing code

Jeff Layton jlayton at redhat.com
Thu Dec 11 12:32:54 GMT 2008


On Wed, 10 Dec 2008 14:14:54 -0500
Jeff Layton <jlayton at redhat.com> wrote:

> I believe this patch:
> 
>     commit 2c55608f28444c3f33b10312881384c470ceed56
>     Author: Igor Mammedov <niallain at gmail.com>
>     Date:   Thu Oct 23 13:58:42 2008 +0400
> 
>         Fixed parsing of mount options when doing DFS submount
> 
> ...has caused a regression in the DFS referral handling code. I have the
> following mounted:
> 
> //hostname/dfsroot
> 
> ...in this DFS root, I have a dfs referral link that points to:
> 
> msdfs:hostname.foo.bar\export
> 
> ...when I try to traverse this link, compose_mount_options attaches a
> prefixpath to the options, even though one shouldn't be there. The
> problem is that strlen(fullpath) is != to ref->path_consumed in this
> situation so we end up trying to build a prefixpath out of nothing.
> 
> I'm not convinced here that trying to work with path_consumed is at all
> correct. Why shouldn't we just try to walk the string starting after the
> sharename and use whatever is there as the prefixpath? The following
> patch seems simpler to me and seems to fix the regressions I've seen in
> my setup.
> 
> Is there some reason we need to use the more complex scheme that was
> introduced by the patch above?
> 
> Long term, I think we need to consider adding some generic functions
> to do UNC parsing. That would be helpful in the standard cifs_mount
> codepath as well and would allow us to make a robust function that
> can handle different UNC variations.
> 
> Signed-off-by: Jeff Layton <jlayton at redhat.com>
> ---
>  fs/cifs/cifs_dfs_ref.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
> index e1c1836..aa4feef 100644
> --- a/fs/cifs/cifs_dfs_ref.c
> +++ b/fs/cifs/cifs_dfs_ref.c
> @@ -128,7 +128,6 @@ static char *compose_mount_options(const char *sb_mountdata,
>  	char *srvIP = NULL;
>  	char sep = ',';
>  	int off, noff;
> -	char *fullpath;
>  
>  	if (sb_mountdata == NULL)
>  		return ERR_PTR(-EINVAL);
> @@ -200,16 +199,13 @@ static char *compose_mount_options(const char *sb_mountdata,
>  	if (tkn_e == NULL) /* invalid unc, missing share name*/
>  		goto compose_mount_options_out;
>  
> -	fullpath = build_path_from_dentry(dentry);
>  	tkn_e = strchr(tkn_e + 1, '\\');
> -	if (tkn_e || strlen(fullpath) - (ref->path_consumed)) {
> +	if (tkn_e) {
>  		strncat(mountdata, &sep, 1);
>  		strcat(mountdata, "prefixpath=");
>  		if (tkn_e)
>  			strcat(mountdata, tkn_e + 1);
> -		strcat(mountdata, fullpath + (ref->path_consumed));
>  	}
> -	kfree(fullpath);
>  
>  	/*cFYI(1,("%s: parent mountdata: %s", __func__,sb_mountdata));*/
>  	/*cFYI(1, ("%s: submount mountdata: %s", __func__, mountdata ));*/
> -- 
> 1.5.5.1
> 

As Steve pointed out to me on IRC, this patch isn't quite what we want.
We *do* need to account for path_consumed here. I'm thinking this is an
off-by-one bug of some sort. I'll do some more investigation and see if
I can track down what's wrong.

-- 
Jeff Layton <jlayton at redhat.com>


More information about the linux-cifs-client mailing list