[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