| Message ID | 1473260003-5271-1-git-send-email-sprabhu@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sachin,
As you said, patch is similar to what I proposed few month ago. There's
only one thing that bothers me. cifs_build_path_to_root() has this bit
before the DFS test:
/* 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;
}
I think it should be dropped. It was already inconsistent because if the
share was in a DFS without a prepath it would have returned an empty
string instead of a path with the tree name prefix.
I can send a patch after yours to correct that you think it should be in
a separate commit.
Cheers,
On Thu, 2016-09-08 at 12:49 +0200, Aurélien Aptel wrote: > Hi Sachin, > > As you said, patch is similar to what I proposed few month ago. > There's > only one thing that bothers me. cifs_build_path_to_root() has this > bit > before the DFS test: > > /* 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; > } > > > I think it should be dropped. It was already inconsistent because if > the > share was in a DFS without a prepath it would have returned an empty > string instead of a path with the tree name prefix. > > I can send a patch after yours to correct that you think it should be > in > a separate commit. > > Cheers, > I suspect that is because the root of the share cannot be a DFS mount. Maybe Steve can confirm it. Sachin Prabhu -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What is the status on this? If we explicitely ask for a tree name prefix via the new parameter, we should always get it. It should not return an empty string if there is no prepath. Sachin Prabhu <sprabhu@redhat.com> writes: > I suspect that is because the root of the share cannot be a DFS mount. > Maybe Steve can confirm it. So that means we can safely remove that early return, IIUC.
On Thu, 2016-10-06 at 14:15 +0200, Aurélien Aptel wrote: > What is the status on this? > > If we explicitely ask for a tree name prefix via the new parameter, > we > should always get it. It should not return an empty string if there > is > no prepath. > > Sachin Prabhu <sprabhu@redhat.com> writes: > > > > I suspect that is because the root of the share cannot be a DFS > > mount. > > Maybe Steve can confirm it. > > So that means we can safely remove that early return, IIUC. > Aurelien, I think Steve had requested for a reproducer for this issue. The only place I've encountered this was with Netapp servers. Do you have a reproder which works with Samba or Windows servers? Sachin Prabhu -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sachin Prabhu <sprabhu@redhat.com> writes: > I think Steve had requested for a reproducer for this issue. The only > place I've encountered this was with Netapp servers. Do you have a > reproder which works with Samba or Windows servers? The reproducer was for Samba but the root cause was something different (later fixed by your last DFS fix). The thing I used to reproduce it doesn't reproduce it anymore. I have found that the tree name prefix is needed, at least when mounting. I guess Steve already knew this but it wasn't obvious for me so I'm sharing it. When mounting //server/dfsroot/target with 'target' linking to //server/dfstarget With tree name prefixed: => Trans2 Request, QUERY_PATH_INFO, Query File All Info, Path: //10.160.64.222/dfsroot/target <= Trans2 Response, QUERY_PATH_INFO, Error: STATUS_PATH_NOT_COVERED Without tree name prefixed: => Trans2 Request, QUERY_PATH_INFO, Query File All Info, Path: /target <= Trans2 Response, QUERY_PATH_INFO, Error: STATUS_OBJECT_NAME_NOT_FOUND PATH_NOT_COVERED is mapped to EREMOTE whereas OBJECT_NAME_NOT_FOUND is mapped to ENOENT. So *not* prefixing with the tree name actually breaks mounting a path containing a DFS link in its components. Which is the *opposite* of what I initially thought. I did not analyze the problem and traces enough, I'm sorry. So if we merge this patch, we have to be careful about when to not prefix the tree name.
On Tue, 2016-10-11 at 08:18 +0200, Aurélien Aptel wrote: > Sachin Prabhu <sprabhu@redhat.com> writes: > > > > I think Steve had requested for a reproducer for this issue. The > > only > > place I've encountered this was with Netapp servers. Do you have a > > reproder which works with Samba or Windows servers? > > The reproducer was for Samba but the root cause was something > different > (later fixed by your last DFS fix). The thing I used to reproduce it > doesn't reproduce it anymore. > > I have found that the tree name prefix is needed, at least when > mounting. I guess Steve already knew this but it wasn't obvious for > me > so I'm sharing it. > > When mounting //server/dfsroot/target with 'target' linking to > //server/dfstarget > > With tree name prefixed: > > => Trans2 Request, QUERY_PATH_INFO, Query File All Info, Path: > //10.160.64.222/dfsroot/target > <= Trans2 Response, QUERY_PATH_INFO, Error: STATUS_PATH_NOT_COVERED > > Without tree name prefixed: > > => Trans2 Request, QUERY_PATH_INFO, Query File All Info, Path: > /target > <= Trans2 Response, QUERY_PATH_INFO, Error: > STATUS_OBJECT_NAME_NOT_FOUND > > PATH_NOT_COVERED is mapped to EREMOTE whereas OBJECT_NAME_NOT_FOUND > is > mapped to ENOENT. > > So *not* prefixing with the tree name actually breaks mounting a path > containing a DFS link in its components. Which is the *opposite* of > what > I initially thought. I did not analyze the problem and traces enough, > I'm sorry. > > So if we merge this patch, we have to be careful about when to not > prefix the tree name. > Hello Aurelien, The patch maintains current behaviour except for the call to cifs_build_path_to_root in cifs_get_root() where the logic expects the full path to the file/directory without the hostname as it iterates over each component of the path. This fixes a behaviour seen with Netapp where the tcon connect returns a SMB_SHARE_IS_IN_DFS even though there doesn't isn't a DFS share in the path. Sachin Prabhu -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-09-07 at 15:53 +0100, Sachin Prabhu wrote: > When a server returns the optional flag SMB_SHARE_IS_IN_DFS in response > to a tree connect, cifs_build_path_to_root() will return a pathname > which includes the tree name. This causes problems with cifs_get_root() > which separates each component and does a lookup for each component of > the path. > > We encountered a problem with dfs shares hosted by a Netapp. when > connecting to nodes pointed to by the DFS share. The tree connect for > these nodes return SMB_SHARE_IS_IN_DFS even after the DFS node is > resolved and the node returned doesn't contain a DFS share. Other > servers(tested with MS Win2012 R2) do not and hence do not face this > problem. A workaround for Netapp servers is to mount with the > nodfs flag. > The return flag results in cifs_build_path_to_root() using the tree name > in building the path which is then split into its components and > individual lookup done for each component in cifs_get_root(). The first > component of the path in this case is the hostname which results in > object not found errors. > > We have tested this patch both on our internal test servers as well as > the user's own servers and can confirm that it fixes the problem. > > Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> > Reported-by: Pierguido Lambri <plambri@redhat.com> > --- > fs/cifs/cifsfs.c | 2 +- > fs/cifs/cifsproto.h | 3 ++- > fs/cifs/connect.c | 3 ++- > fs/cifs/dir.c | 4 ++-- > 4 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > index 6bbec5e..b6f57dd 100644 > --- a/fs/cifs/cifsfs.c > +++ b/fs/cifs/cifsfs.c > @@ -610,7 +610,7 @@ 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), 0); Ok, sorry if I'm being dense here, but I haven't been following the various patches in this area closely... cifs_get_root basically does a lookup of the dentry that we want to be the root of the mount on the client. You're saying here that we need to _not_ send a DFS style path query when doing that lookup, even though the server is setting the SMB_SHARE_IS_IN_DFS flag in the tcon response? Is this just a workaround for misbehaving servers, or is the client actually doing this wrong? > if (full_path == NULL) > return ERR_PTR(-ENOMEM); > > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > index 1243bd3..52c307b 100644 > --- a/fs/cifs/cifsproto.h > +++ b/fs/cifs/cifsproto.h > @@ -63,7 +63,8 @@ extern void exit_cifs_spnego(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, > + int add_treename); > 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, > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index 9878943..afca9ee 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -3649,7 +3649,8 @@ 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, > + tcon->Flags & SMB_SHARE_IS_IN_DFS); > if (full_path == NULL) { > rc = -ENOMEM; > goto mount_fail_check; > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c > index 4716c54..4c0c62c 100644 > --- a/fs/cifs/dir.c > +++ b/fs/cifs/dir.c > @@ -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, int add_treename) > { > int pplen = vol->prepath ? strlen(vol->prepath) + 1 : 0; > int dfsplen; > @@ -59,7 +59,7 @@ 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 (add_treename) > dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1); > else > dfsplen = 0;
On Tue, 2016-12-13 at 07:33 -0500, Jeff Layton wrote: > On Wed, 2016-09-07 at 15:53 +0100, Sachin Prabhu wrote: > > > > When a server returns the optional flag SMB_SHARE_IS_IN_DFS in response > > to a tree connect, cifs_build_path_to_root() will return a pathname > > which includes the tree name. This causes problems with cifs_get_root() > > which separates each component and does a lookup for each component of > > the path. > > > > We encountered a problem with dfs shares hosted by a Netapp. when > > connecting to nodes pointed to by the DFS share. The tree connect for > > these nodes return SMB_SHARE_IS_IN_DFS even after the DFS node is > > resolved and the node returned doesn't contain a DFS share. Other > > servers(tested with MS Win2012 R2) do not and hence do not face this > > problem. A workaround for Netapp servers is to mount with the > > nodfs flag. > > The return flag results in cifs_build_path_to_root() using the tree name > > in building the path which is then split into its components and > > individual lookup done for each component in cifs_get_root(). The first > > component of the path in this case is the hostname which results in > > object not found errors. > > > > We have tested this patch both on our internal test servers as well as > > the user's own servers and can confirm that it fixes the problem. > > > > Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> > > Reported-by: Pierguido Lambri <plambri@redhat.com> > > --- > > fs/cifs/cifsfs.c | 2 +- > > fs/cifs/cifsproto.h | 3 ++- > > fs/cifs/connect.c | 3 ++- > > fs/cifs/dir.c | 4 ++-- > > 4 files changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > > index 6bbec5e..b6f57dd 100644 > > --- a/fs/cifs/cifsfs.c > > +++ b/fs/cifs/cifsfs.c > > @@ -610,7 +610,7 @@ 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), 0); > > Ok, sorry if I'm being dense here, but I haven't been following the > various patches in this area closely... > > cifs_get_root basically does a lookup of the dentry that we want to be > the root of the mount on the client. > > You're saying here that we need to _not_ send a DFS style path query > when doing that lookup, even though the server is setting > the SMB_SHARE_IS_IN_DFS flag in the tcon response? > > Is this just a workaround for misbehaving servers, or is the client > actually doing this wrong? > Doh! I just needed another cup of coffee... I get it now. We build the path to root, and then try to issue a lookup on each component (a'la lookup_one_len_unlocked). That fails in this case because we're trying to lookup the DFS hostname component like we would a normal path component. I think is this a reasonable fix for now. That said, this whole Rube Goldberg contraption could really use a serious re-think, IMO... You can add: Reviewed-by: Jeff Layton <jlayton@redhat.com> > > > > if (full_path == NULL) > > return ERR_PTR(-ENOMEM); > > > > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > > index 1243bd3..52c307b 100644 > > --- a/fs/cifs/cifsproto.h > > +++ b/fs/cifs/cifsproto.h > > @@ -63,7 +63,8 @@ extern void exit_cifs_spnego(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, > > + int add_treename); > > 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, > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > > index 9878943..afca9ee 100644 > > --- a/fs/cifs/connect.c > > +++ b/fs/cifs/connect.c > > @@ -3649,7 +3649,8 @@ 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, > > + tcon->Flags & SMB_SHARE_IS_IN_DFS); > > if (full_path == NULL) { > > rc = -ENOMEM; > > goto mount_fail_check; > > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c > > index 4716c54..4c0c62c 100644 > > --- a/fs/cifs/dir.c > > +++ b/fs/cifs/dir.c > > @@ -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, int add_treename) > > { > > int pplen = vol->prepath ? strlen(vol->prepath) + 1 : 0; > > int dfsplen; > > @@ -59,7 +59,7 @@ 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 (add_treename) > > dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1); > > else > > dfsplen = 0; > -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jeff Layton <jlayton@redhat.com> writes: > On Tue, 2016-12-13 at 07:33 -0500, Jeff Layton wrote: > I think is this a reasonable fix for now. That said, this whole Rube > Goldberg contraption could really use a serious re-think, IMO... Indeed. I find cifs_mount() in particular really hard to follow.
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index 6bbec5e..b6f57dd 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -610,7 +610,7 @@ 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), 0); if (full_path == NULL) return ERR_PTR(-ENOMEM); diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index 1243bd3..52c307b 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -63,7 +63,8 @@ extern void exit_cifs_spnego(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, + int add_treename); 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, diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 9878943..afca9ee 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -3649,7 +3649,8 @@ 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, + tcon->Flags & SMB_SHARE_IS_IN_DFS); if (full_path == NULL) { rc = -ENOMEM; goto mount_fail_check; diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c index 4716c54..4c0c62c 100644 --- a/fs/cifs/dir.c +++ b/fs/cifs/dir.c @@ -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, int add_treename) { int pplen = vol->prepath ? strlen(vol->prepath) + 1 : 0; int dfsplen; @@ -59,7 +59,7 @@ 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 (add_treename) dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1); else dfsplen = 0;
When a server returns the optional flag SMB_SHARE_IS_IN_DFS in response to a tree connect, cifs_build_path_to_root() will return a pathname which includes the tree name. This causes problems with cifs_get_root() which separates each component and does a lookup for each component of the path. We encountered a problem with dfs shares hosted by a Netapp. when connecting to nodes pointed to by the DFS share. The tree connect for these nodes return SMB_SHARE_IS_IN_DFS even after the DFS node is resolved and the node returned doesn't contain a DFS share. Other servers(tested with MS Win2012 R2) do not and hence do not face this problem. A workaround for Netapp servers is to mount with the nodfs flag. The return flag results in cifs_build_path_to_root() using the tree name in building the path which is then split into its components and individual lookup done for each component in cifs_get_root(). The first component of the path in this case is the hostname which results in object not found errors. We have tested this patch both on our internal test servers as well as the user's own servers and can confirm that it fixes the problem. Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> Reported-by: Pierguido Lambri <plambri@redhat.com> --- fs/cifs/cifsfs.c | 2 +- fs/cifs/cifsproto.h | 3 ++- fs/cifs/connect.c | 3 ++- fs/cifs/dir.c | 4 ++-- 4 files changed, 7 insertions(+), 5 deletions(-)