diff mbox

Fix regression which breaks DFS mounting

Message ID 1473164554-8062-1-git-send-email-sprabhu@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sachin Prabhu Sept. 6, 2016, 12:22 p.m. UTC
Patch a6b5058 results in -EREMOTE returned by is_path_accessible() in
cifs_mount() to be ignored which breaks DFS mounting.

Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
---
 fs/cifs/connect.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Aurélien Aptel Sept. 6, 2016, 6:29 p.m. UTC | #1
This is not enough to fix DFS. I believe we also need [1] to handle DFS
links to subdirs.

I have SERVER_A with a DFS link to //SERVER_B/sub/dir.
cifs_are_all_path_accessible() and cifs_get_root() walk along the path
components and do queries on them. But they are not skipping the initial
host + share name on the UNC resulting in failed queries:

  CLIENT_A_____ -> SERVER_B____ SMB Trans2 Request,
      QUERY_PATH_INFO, Query File All Info, Path: //SERVER_B____
  SERVER_B____ -> CLIENT_A_____ SMB Trans2 Response,
      QUERY_PATH_INFO, Error: STATUS_OBJECT_NAME_NOT_FOUND

The QUERY_PATH_INFO request here is using the first path component which
is not a file path but an incomplete UNC path.

That path is made by cifs_build_path_to_root() which only makes a full
UNC path when in a DFS. It is not necessary on the servers I've tested
(cf [1]).

Therefore the fix is to not make a full UNC path in
cifs_build_path_to_root() or to add a flag to its parameters to decide
whether to prefix the host+share or not. I've sent patches for both
solution on the thread [1] (look at all the messages of the thread).

1: http://marc.info/?i=20160801143204.7377b5a4@aaptelpc
Sachin Prabhu Sept. 7, 2016, 9:02 a.m. UTC | #2
On Tue, 2016-09-06 at 20:29 +0200, Aurélien Aptel wrote:
> This is not enough to fix DFS. I believe we also need [1] to handle
> DFS
> links to subdirs.
> 
> I have SERVER_A with a DFS link to //SERVER_B/sub/dir.
> cifs_are_all_path_accessible() and cifs_get_root() walk along the
> path
> components and do queries on them. But they are not skipping the
> initial
> host + share name on the UNC resulting in failed queries:
> 
>   CLIENT_A_____ -> SERVER_B____ SMB Trans2 Request,
>       QUERY_PATH_INFO, Query File All Info, Path: //SERVER_B____
>   SERVER_B____ -> CLIENT_A_____ SMB Trans2 Response,
>       QUERY_PATH_INFO, Error: STATUS_OBJECT_NAME_NOT_FOUND
> 
> The QUERY_PATH_INFO request here is using the first path component
> which
> is not a file path but an incomplete UNC path.
> 
> That path is made by cifs_build_path_to_root() which only makes a
> full
> UNC path when in a DFS. It is not necessary on the servers I've
> tested
> (cf [1]).
> 
> Therefore the fix is to not make a full UNC path in
> cifs_build_path_to_root() or to add a flag to its parameters to
> decide
> whether to prefix the host+share or not. I've sent patches for both
> solution on the thread [1] (look at all the messages of the thread).
> 
> 1: http://marc.info/?i=20160801143204.7377b5a4@aaptelpc
> 

Hello Auriel,

I have encountered a similar problem reported by a user a few days ago
when mounting a DFS share hosted on a Netapp. 

The problem is caused when 
1) The flag SMB_SHARE_IS_IN_DFS is set in the optional flags when a
tcon connect is returned.
2) The subsequent Trans2 request with QUERY_PATH_INFO doesn't return a
STATUS_PATH_NOT_COVERED which indicates that this is a DFS share and we
need to call a GET_DFS_REFERRAL to obtain the DFS node to connect to.

This is hit in Netapp because it returns 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.

I have a patch similar to your second variant which we have
successfully tested in our lab and is currently undergoing user
testing. I'll post it separately as soon as we have the results for
that.

In my opinion, the patch at the head of the thread here is not related
to this problem and should go in ASAP to fix a regression which affects
all DFS mounts.

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
Aurélien Aptel Sept. 7, 2016, 4:22 p.m. UTC | #3
Hi Sachin,

Sachin Prabhu <sprabhu@redhat.com> writes:
> This is hit in Netapp because it returns 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 issue I'm describing is happening on samba servers. Server A has 2
DFS links to server B. First link is pointing the the root of a share,
second link is pointing to a subdir of the same share. mounting A share
and trying to ls or cd in these links result in both pointing to the
root of the share B.

> I have a patch similar to your second variant which we have
> successfully tested in our lab and is currently undergoing user
> testing. I'll post it separately as soon as we have the results for
> that.
>
> In my opinion, the patch at the head of the thread here is not related
> to this problem and should go in ASAP to fix a regression which affects
> all DFS mounts.

Agreed. I can confirm that this patch lets me mount
//SERVER/share/dfs-main where dfs-main is a DFS link. I was previously
getting:

mount error(22): Invalid argument

Tested-by: Aurelien Aptel <aaptel@suse.com>
diff mbox

Patch

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 7ae0328..9878943 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3661,14 +3661,16 @@  remote_path_check:
 			goto mount_fail_check;
 		}
 
-		rc = cifs_are_all_path_components_accessible(server,
+		if (rc != -EREMOTE) {
+			rc = cifs_are_all_path_components_accessible(server,
 							     xid, tcon, cifs_sb,
 							     full_path);
-		if (rc != 0) {
-			cifs_dbg(VFS, "cannot query dirs between root and final path, "
-				 "enabling CIFS_MOUNT_USE_PREFIX_PATH\n");
-			cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_USE_PREFIX_PATH;
-			rc = 0;
+			if (rc != 0) {
+				cifs_dbg(VFS, "cannot query dirs between root and final path, "
+					 "enabling CIFS_MOUNT_USE_PREFIX_PATH\n");
+				cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_USE_PREFIX_PATH;
+				rc = 0;
+			}
 		}
 		kfree(full_path);
 	}