Bug 14391 - DFS links broken in 4.12
Summary: DFS links broken in 4.12
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: 4.12.3
Hardware: x64 Linux
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-22 15:46 UTC by docmax
Modified: 2020-09-24 01:20 UTC (History)
4 users (show)

See Also:


Attachments
log level 10 (1.27 MB, text/plain)
2020-05-27 15:19 UTC, docmax
no flags Details
git-am fix for master (27.35 KB, patch)
2020-06-01 21:08 UTC, Jeremy Allison
no flags Details
git-am fix for 4.12.next. (10.73 KB, patch)
2020-06-01 21:36 UTC, Jeremy Allison
no flags Details
git-am fix for 4.12.next (11.44 KB, patch)
2020-06-03 17:15 UTC, Jeremy Allison
jra: review? (asn)
slow: review+
anoopcs: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description docmax 2020-05-22 15:46:50 UTC
After v4.12 my DFS links look like files in Windows (10).

I think this commit is the reason:
https://github.com/samba-team/samba/commit/9ee1320049cf148a2bb102bbdee4a4bcc24c0de1#commitcomment-39367012

"All DFS links are now read through the VFS and not via
symlink calls." - What does this mean? Do i have to load a special VFS module?

Thanks,
DocMAX
Comment 1 docmax 2020-05-22 16:24:00 UTC
comparing 4.11 and 4.12 there is just one difference.

[2020/05/22 15:38:56.607571,  3] ../../source3/smbd/dir.c:659(dptr_create)
  creating new dirptr 0 for path stores, expect_close = 0

is missing in 4.12.
Comment 2 Jeremy Allison 2020-05-26 17:01:08 UTC
Can you upload a full debug level 10 log please, and also an ls -l <link-names> of the directory containing the DFS links. I'd like to see what the symlinks look like and also what smbd says when trying to process them.

Thanks !
Comment 3 Bernd 2020-05-27 07:52:40 UTC
I'd like to confirm that behaviour. It's easy to reproduce:

1: mkdir /tmp/relay; ln -s msdfs:srv0\\share0 /tmp/relay/srv0
2: smb.conf:
[global]
    map to guest = Bad User
    log file = /var/log/samba/log.%m
    HOST MSDFS = yes
[relay]
    path = /tmp/relay
    MSDFS ROOT = yes
    public = yes
    guest ok = yes
    guest only = yes
3: mapping "\\smbhost\relay" from a windows machine.

Works perfectly fine up to 4.11.9. and with 4.12.x 
"srv0" is void and functionless.
Comment 4 Jeremy Allison 2020-05-27 12:37:09 UTC
Sorry, I thought we were testing this in the regression tests. I'll take a look, but I might not get to this until after SambaXP.
Comment 5 docmax 2020-05-27 15:19:47 UTC
Created attachment 16010 [details]
log level 10
Comment 6 docmax 2020-05-27 15:20:31 UTC
[root@server]: /mnt/dfs/stores># ls -l *
lrwxrwxrwx 1 root root 20 22. Mai 15:06 4bay_usb3 -> 'msdfs:t420\4bay_usb3'
lrwxrwxrwx 1 root root 20 22. Mai 15:06 8bay_usb3 -> 'msdfs:t420\8bay_usb3'
lrwxrwxrwx 1 root root 14 15. Mai 11:37 hdd -> 'msdfs:t420\hdd'
Comment 7 Jeremy Allison 2020-05-27 17:39:44 UTC
(In reply to Bernd from comment #3)

Just checked your use-case:

mkdir /tmp/relay; ln -s msdfs:srv0\\share0 /tmp/relay/srv0

using smbclient locally on master. Works. So I'll keep looking.
Comment 8 Bernd 2020-05-28 10:09:49 UTC
right, but that's misleading and not the problem.

The use case is to have a mapped share ("relay") holding a list of 
junctions (e.g. "srv0").
Until 4.11.9 in windows explorer this pretty much looks like a list of 
folders and worked like an on demand mapping just by clicking on it.
Great feature.
Comment 9 Jeremy Allison 2020-05-28 14:11:19 UTC
(In reply to Bernd from comment #8)

Sure I agree. I was just checking that the "click-through" ability of these junction points still worked from our client tools. These are what we're using for regression tests, so that's why we missed the bug w.r.t. what Windows10 clients are doing.

I'll try and reproduce and then update our test suite with something that does the same as what Windows does.
Comment 10 Jeremy Allison 2020-05-29 21:51:58 UTC
Reproduced. We're not setting the FILE_ATTRIBUTE_REPARSE_POINT when returning these links. I'm looking into why now.
Comment 11 Jeremy Allison 2020-05-30 02:22:57 UTC
OK, I know how to fix this. For 4.12.next it'll be a trivial addition of an extra SMB_VFS_LSTAT() call inside is_msdfs_link() to make sure the returned smb_filename struct has a VALID_STAT returned. When we moved to calling SMB_VFS_READ_DFS_PATHAT() we dropped the previously implicit call to SMB_VFS_LSTAT() that was used to quickly see if it was a DFS pathname. Now we just do readlink() directly (as we know we're going to have to do that anyway) and we dropped the lstat call as not need. So it left the smb_fname->st structure uninitialized and we're returning garbage values when enumerating a directory containing MS-DFS links and not setting FILE_ATTRIBUTE_REPARSE_POINT (which is why the Windows10 client isn't seeing them as DFS-links anymore).

I can't fix it the "correct" way (explained below) in 4.12.next as that would require a VFS ABI change.

For master I'd like to fix it differently. I now realize I made a mistake in defining SMB_VFS_READ_DFS_PATHAT() to take a 'const struct smb_filename *'.

As SMB_VFS_READ_DFS_PATHAT() is actually called as part of filename meta-data lookup when enumerating a directory from SMB1/2/3 via smbd_dirptr_lanman2_entry(), it really has to fill in the values inside the smb_fname->st struct on return.

It's not just a "lookup dfs redirect paths" call, it's a "get all metadata about a DFS filename" call. That means it has to fill in smb_fname->st, so that when returning the info levels from the query directory we have all the needed info about the file.

Then we can correctly fill in the info level data being requested - the DOS attributes that contain FILE_ATTRIBUTE_REPARSE_POINT is just one thing we need to return (albeit the most important one).

So for 4.12.next, I'll add the extra LSTAT call to fill in smb_fname->st. That's not the "right" way as it still assumes that the name we're returning from the directory listing can be queried via an LSTAT call, still exposing that underlying it all it's being stored as a symlink - something I'm trying to move away from.

All a DFS redirect object in the filesystem really has to guarantee is that calling SMB_VFS_STAT() on it will fail, so the alternate lookup can then be invoked. So the alternate lookup of SMB_VFS_READ_DFS_PATHAT() needs to fill in smb_fname->st, so it can't be passed in as const.

All this is mostly for Ralph, so he can understand the design decision behind the changes I'm going to make for master.

First I will finish my regression test, then make sure the fix for master fixes it, then once that's passed CI and been pushed I'll do the "trivial" fix for the 4.12.next back-port and it should be in the next 4.12 release.
Comment 12 Jeremy Allison 2020-06-01 21:08:12 UTC
Created attachment 16013 [details]
git-am fix for master

What I currently have in gitlab-ci. I'll back-port a (modified) fix for 4.12 next.
Comment 13 Jeremy Allison 2020-06-01 21:36:34 UTC
Created attachment 16014 [details]
git-am fix for 4.12.next.

Checked here and works with Windows 10. Includes regression test to make sure we don't break in future.

Please let me know if this works for you. Once the master fix has passed ci and been pushed, I'll get this reviewed for 4.12.next.

Sorry for the problem.
Comment 14 Ralph Böhme 2020-06-02 13:46:14 UTC
(In reply to Jeremy Allison from comment #13)
Small nitpick for the 4.12 patch:

+	if (NT_STATUS_IS_OK(status)) {
+		int ret = SMB_VFS_LSTAT(conn, smb_fname);
+		if (ret < 0) {
+			status = map_nt_error_from_unix(errno);
+		}
+	}

Can you please split the ret variable declaration from the SMB_VFS_LSTAT() function call? Thanks!
Comment 15 Bernd 2020-06-02 14:16:05 UTC
looks good !

Patched against 4.12.3, tested for the posted trivial use-case and 
our real one.

Thank you very much ! Great work !
Comment 16 Jeremy Allison 2020-06-02 14:50:55 UTC
(In reply to Ralph Böhme from comment #14)

Will do when I update the patchset once it's gone into master.

Thanks !
Comment 17 Jeremy Allison 2020-06-03 17:15:42 UTC
Created attachment 16017 [details]
git-am fix for 4.12.next

(Mostly) cherry-picked from master, except for the last part which has to be done differently.
Comment 18 Jeremy Allison 2020-06-25 21:39:30 UTC
Comment on attachment 16017 [details]
git-am fix for 4.12.next

Andreas, we need to get this into 4.12.next in order for MSDFS links to work again. Thanks !
Comment 19 Anoop C S 2020-06-26 07:07:32 UTC
Comment on attachment 16017 [details]
git-am fix for 4.12.next

lgtm
Comment 20 Ralph Böhme 2020-06-26 07:30:46 UTC
Reassigning to Karolin for inclusion in 4.12.
Comment 21 Karolin Seeger 2020-06-26 07:51:07 UTC
(In reply to Ralph Böhme from comment #20)
Pushed to autobuild-v4-12-test.
Comment 22 Karolin Seeger 2020-06-29 06:55:16 UTC
(In reply to Karolin Seeger from comment #21)
Pushed to v4-12-test.
Closing out bug report.

Thanks!