Project

General

Profile

Bug #25485

Fix vfs_winmsa

Added by Ash Gokhale over 3 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Blocks Until Resolved
Assignee:
John Hixson
Category:
OS
Target version:
Seen in:
Severity:
Reason for Closing:
Reason for Blocked:
Needs QA:
No
Needs Doc:
Yes
Needs Merging:
Yes
Needs Automation:
No
Support Suite Ticket:
n/a
Hardware Configuration:
ChangeLog Required:
No

Description

Creating and immediately renaming a directory at the share root from explorer overwrites file group and ownership permissions, and ACL's for the entire share. The effect cascades through the entire share when executed from a windows role with elevated privilege; on a system with nontrival acl's this is destructive. Also this makes create+rename possibly an O(n^2) operation.

The correct behavior is to clone the shares' acl to the newly renamed directory, and not to touch the other heirarchy rooted at the sister level of a renamed destination.

Removing winmsa from the vfs stack stops the behaviour.

Steps to reproduce on FN-nightly-recent-from July

- set up an smb export using winmsa module
- fill a directory with a simple herarchy with some dissimilar acl from the share root
- create a dir in the share root; immediately rename it at the share root.
- observe all acl's are overwritten throughout the share

winmsa is recursively resetting permissions outside the renamed file

This is a syscall histogram from a system that has just murdered all the ACL's in a share  after being asked to rename a single top level directory. 
produced with 
dtrace -n 'syscall:::entry /execname == "smbd"/ { @[probefunc] = count ();}

unlink 1
setsockopt 2
getuid 3
connect 4
lseek 4
socket 5
sigaction 6
__getcwd 7
dup2 7
extattr_list_file 7
fstatat 7
mprotect 12
pread 13
umask 14
getgroups 18
sigprocmask 19
writev 24
access 27
setgroups 29
setregid 30
setreuid 30
write 35
__sysctl 39
__acl_get_file 44
extattr_get_file 44
readv 46
getpid 59
poll 62
getegid 71
openat 79
read 83
munmap 87
fstat 116
geteuid 128
stat 165
mmap 215
fcntl 834
fstatfs 2962
open 2985
close 3081
getdirentries 5951
__acl_set_file 24748    <----------- that can't be what we meant to do
chown 24748
lstat 314871

This is reproducable on my system - adding a new top level directory via explorere's new directorty destroys the acl's on sister directories; recursively spidering the filesystem stamping over all files; I suspect some incorrect parent or '..' processing in vfs_winmsa
Proof; we watch the operation opendir() every file in the partent's directory, it's only entry point is through winmsa.so`0x815334b3a which is very likely in the set acls routine called on the parent rather than the new directory.
Unfortunately goto's and multiple returns obsucate the symbols that dtrace can extract. 

#dtrace -n 'pid20844::opendir:entry { printf ( "d: %s\n", copyinstr ( arg0)); ustack ();}' :root:~/env/dt:20:54:09:1062badger
#h :root:~/env/dt:21:02:02:1062badger
libsmbd-base-samba4.so`0x8011ca5c2
libsmbd-base-samba4.so`0x8011c9c86
libtevent.so.0`0x804c1d05c
libtevent.so.0`0x804c1c4a6
libtevent.so.0`_tevent_loop_once+0x118
libtevent.so.0`0x804c1c548
libtevent.so.0`_tevent_loop_wait+0x30

4 77747 opendir:entry d: /mnt/z/GROUP/TOP-wip/New folder (3)

libc.so.7`opendir+0x1
winmsa.so`0x815334b3a
winmsa.so`0x815334dda
winmsa.so`0x815334382
libsmbd-base-samba4.so`smb_vfs_call_rename+0x57
shadow_copy2.so`0x81553f071
libsmbd-base-samba4.so`smb_vfs_call_rename+0x57
libsmbd-base-samba4.so`rename_internals_fsp+0xdca
libsmbd-base-samba4.so`0x80114bc8f
libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x719
libsmbd-base-samba4.so`0x8011f0b8a
libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x528
libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x14d4
libsmbd-base-samba4.so`0x8011ca5c2
libsmbd-base-samba4.so`0x8011c9c86
libtevent.so.0`0x804c1d05c
libtevent.so.0`0x804c1c4a6
libtevent.so.0`_tevent_loop_once+0x118
libtevent.so.0`0x804c1c548
libtevent.so.0`_tevent_loop_wait+0x30

4 77747 opendir:entry d: /mnt/z/GROUP/TOP-wip/c2

libc.so.7`opendir+0x1
winmsa.so`0x815334b3a
winmsa.so`0x815334dda
winmsa.so`0x815334382
libsmbd-base-samba4.so`smb_vfs_call_rename+0x57
shadow_copy2.so`0x81553f071
libsmbd-base-samba4.so`smb_vfs_call_rename+0x57
libsmbd-base-samba4.so`rename_internals_fsp+0xdca
libsmbd-base-samba4.so`0x80114bc8f
libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x719
libsmbd-base-samba4.so`0x8011f0b8a
libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x528
libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x14d4
libsmbd-base-samba4.so`0x8011ca5c2
libsmbd-base-samba4.so`0x8011c9c86
libtevent.so.0`0x804c1d05c
libtevent.so.0`0x804c1c4a6
libtevent.so.0`_tevent_loop_once+0x118
libtevent.so.0`0x804c1c548
libtevent.so.0`_tevent_loop_wait+0x30

4 77747 opendir:entry d: /mnt/z/GROUP/TOP-wip/child

libc.so.7`opendir+0x1
winmsa.so`0x815334b3a
winmsa.so`0x815334dda
winmsa.so`0x815334382
libsmbd-base-samba4.so`smb_vfs_call_rename+0x57
shadow_copy2.so`0x81553f071
libsmbd-base-samba4.so`smb_vfs_call_rename+0x57
libsmbd-base-samba4.so`rename_internals_fsp+0xdca
libsmbd-base-samba4.so`0x80114bc8f
libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x719
libsmbd-base-samba4.so`0x8011f0b8a
libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x528
libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x14d4
libsmbd-base-samba4.so`0x8011ca5c2
libsmbd-base-samba4.so`0x8011c9c86
libtevent.so.0`0x804c1d05c
libtevent.so.0`0x804c1c4a6
libtevent.so.0`_tevent_loop_once+0x118
libtevent.so.0`0x804c1c548
libtevent.so.0`_tevent_loop_wait+0x30

4 77747 opendir:entry d: /mnt/z/GROUP/TOP-wip/New folder (2)

libc.so.7`opendir+0x1
winmsa.so`0x815334b3a
winmsa.so`0x815334dda
winmsa.so`0x815334382
libsmbd-base-samba4.so`smb_vfs_call_rename+0x57
shadow_copy2.so`0x81553f071
libsmbd-base-samba4.so`smb_vfs_call_rename+0x57
libsmbd-base-samba4.so`rename_internals_fsp+0xdca
libsmbd-base-samba4.so`0x80114bc8f
libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x719
libsmbd-base-samba4.so`0x8011f0b8a
libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x528
libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x14d4
libsmbd-base-samba4.so`0x8011ca5c2
libsmbd-base-samba4.so`0x8011c9c86
libtevent.so.0`0x804c1d05c
libtevent.so.0`0x804c1c4a6
libtevent.so.0`_tevent_loop_once+0x118
libtevent.so.0`0x804c1c548
libtevent.so.0`_tevent_loop_wait+0x30

Also running chown on a system this time overrwriting ~100k acl's on a system:

The win_msa module is caught in the cookie jar:

[root@XXXXXXXX-CTL2] /mnt/tank/Data/GROUP/PUB/PUB_WIP/TRADES# dtrace -n 'syscall::chown:entry {@[ustack()] = count ();}'
dtrace: description 'syscall::chown:entry ' matched 2 probes

libc.so.7`chown+0xa
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358f419
libsmbd-base-samba4.so`smb_vfs_call_rename+0x5a
shadow_copy2.so`0x81545ec27
libsmbd-base-samba4.so`smb_vfs_call_rename+0x5a
libsmbd-base-samba4.so`rename_internals_fsp+0xe33
libsmbd-base-samba4.so`0x800f57928
libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x5eb
libsmbd-base-samba4.so`0x801003549
libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x5a1
libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x170b
libsmbd-base-samba4.so`0x800fdb359
9184

libc.so.7`chown+0xa
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358f419
libsmbd-base-samba4.so`smb_vfs_call_rename+0x5a
shadow_copy2.so`0x81545ec27
libsmbd-base-samba4.so`smb_vfs_call_rename+0x5a
libsmbd-base-samba4.so`rename_internals_fsp+0xe33
libsmbd-base-samba4.so`0x800f57928
libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x5eb
libsmbd-base-samba4.so`0x801003549
libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x5a1
libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x170b
libsmbd-base-samba4.so`0x800fdb359
libsmbd-base-samba4.so`0x800fda976
libtevent.so.0`0x804e6a2d9
13693

libc.so.7`chown+0xa
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358feb4
winmsa.so`0x81358f419
libsmbd-base-samba4.so`smb_vfs_call_rename+0x5a
shadow_copy2.so`0x81545ec27
libsmbd-base-samba4.so`smb_vfs_call_rename+0x5a
libsmbd-base-samba4.so`rename_internals_fsp+0xe33
libsmbd-base-samba4.so`0x800f57928
libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x5eb
libsmbd-base-samba4.so`0x801003549
libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x5a1
libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x170b
libsmbd-base-samba4.so`0x800fdb359
libsmbd-base-samba4.so`0x800fda976
13784
Digging in winmsa.c yeilds a clue. Although i'm not sure about my solution; but we should definitely be using the child's path to request recursive decent acl rewrite and not the parent.
vfs_winmsa.c: modified, readonly: line 250 of 289
info->path = talloc_size(ctx, PATH_MAX);
if (realpath(parent, info->path) == NULL) {
DEBUG(3, ("winmsa_rename: realpath failed for %s\n", parent));
result = -1;
goto out;
}

during any rename of a new directory :
this codepath installs the parent's path in to info->path and then later recurses set_acl_ on everything in the parent's directory adding the acl from info_path, 
This beahviour is ok when the parent's acl's are the same as the child.

The fix appears to be somthing like 
diff --git a/source3/modules/vfs_winmsa.c b/source3/modules/vfs_winmsa.c
index 39d1af716f3..1be7f25a851 100644
--- a/source3/modules/vfs_winmsa.c
+++ b/source3/modules/vfs_winmsa.c
@@ -269,7 +269,9 @@ static int winmsa_rename(struct vfs_handle_struct *handle,
goto out;
}

- result = winmsa_set_acls(ctx, handle, info, info->path);
+ //>ash XXXX 
+ //result = winmsa_set_acls(ctx, handle, info, info->path); /* info->path is the parent */
+ result = winmsa_set_acls(ctx, handle, info, smb_file_dst->base_name); // recurse on the child instead

out:
TALLOC_FREE(ctx);
winmsa working.png (152 KB) winmsa working.png Joe Maloney, 08/30/2017 12:14 PM
12339

Associated revisions

Revision bede27df (diff)
Added by John Hixson over 3 years ago

Don't recurse in the parent (Thanks Ash!) Ticket: #25485

Revision 1bd9976c (diff)
Added by John Hixson over 3 years ago

vfs_winmsa fix Ticket: #25485

Revision f42e3878 (diff)
Added by John Hixson over 3 years ago

Don't recurse in the parent (Thanks Ash!) Ticket: #25485 (cherry picked from commit bede27df5aa144163c12cfbd0825ffbe2f709ab3)

Revision fd0d1eb9 (diff)
Added by John Hixson over 3 years ago

Update samba port for winmsa fix Ticket: #25485

Revision c3c5cbb8 (diff)
Added by John Hixson over 3 years ago

Update samba port for winmsa fix Ticket: #25485 (cherry picked from commit fd0d1eb95ba13dc777ef2b63d3d9976f31a3405d)

Revision 6b5b621b (diff)
Added by John Hixson over 3 years ago

Don't recurse in the parent (Thanks Ash!) Ticket: #25485 (cherry picked from commit bede27df5aa144163c12cfbd0825ffbe2f709ab3) (cherry picked from commit f42e38784f84067716f0feca922e25ba33944d01)

Revision 602f725b (diff)
Added by John Hixson over 3 years ago

Don't recurse in the parent (Thanks Ash!) Ticket: #25485 (cherry picked from commit bede27df5aa144163c12cfbd0825ffbe2f709ab3)

Revision bf54cc87 (diff)
Added by John Hixson over 3 years ago

Don't recurse in the parent (Thanks Ash!) Ticket: #25485 (cherry picked from commit bede27df5aa144163c12cfbd0825ffbe2f709ab3)

Revision 9191aa2e (diff)
Added by John Hixson about 3 years ago

Don't recurse in the parent (Thanks Ash!) Ticket: #25485 (cherry picked from commit bede27df5aa144163c12cfbd0825ffbe2f709ab3) (cherry picked from commit 602f725b265bbf00a85673675599bc43b5b3dd55)

Revision dc3b601b (diff)
Added by John Hixson over 2 years ago

Don't recurse in the parent (Thanks Ash!) Ticket: #25485 (cherry picked from commit bede27df5aa144163c12cfbd0825ffbe2f709ab3) (cherry picked from commit 602f725b265bbf00a85673675599bc43b5b3dd55)

History

#1 Updated by Dru Lavigne over 3 years ago

  • Assignee changed from Release Council to John Hixson

#2 Updated by Timur Bakeyev over 3 years ago

Looks OK to me.

So here we extract dst parent dir ACLs and then should apply them recursively to the destination itself. The patch does exactly that, while current code shifts one level up, which breaks the things...

#3 Updated by John Hixson over 3 years ago

  • Status changed from Unscreened to Screened

#4 Updated by John Hixson over 3 years ago

The winmsa module was written to emulate windows MoveSecurityAttributes feature. This is the intended behavior (unless I am missing something). The entire purpose of this module is for this ability. If you move a file, a directory, or directory hierarchy to another directory (the parent), the file, or directory, or directory hierarchy will inherit the ACL structure. Using the child here is pointless and defeats the purpose. This behavior is configurable on Windows, but is not on Macs. Mac screws up ACLs badly.

#5 Updated by Ash Gokhale over 3 years ago

It's not ok to overwrite all the permissions on the sisters of a renamed directory; did you review my suggested change?

#6 Updated by John Hixson over 3 years ago

Ash Gokhale wrote:

It's not ok to overwrite all the permissions on the sisters of a renamed directory; did you review my suggested change?

Your changes look fine. Obviously it was not intentional to do this to neighboring directories, so no need claim I feel it is okay ;-)

#7 Updated by John Hixson over 3 years ago

  • Assignee changed from John Hixson to Ash Gokhale

#8 Updated by Ash Gokhale over 3 years ago

  • Assignee changed from Ash Gokhale to John Hixson

I would really appreciate it if you could take this patch into the repo and flog it - It will be some time before I can have a local build env idea that's ready to do this.

#9 Updated by John Hixson over 3 years ago

I've tested this and it breaks functionality. I'm investigating why.

#11 Updated by John Hixson over 3 years ago

Testing out proper fix. The issue with the specified patch (besides variable name that is wrong), is the top level directory ACL does not get set.

#12 Updated by John Hixson over 3 years ago

Still working on this.

#13 Updated by Vaibhav Chauhan over 3 years ago

please make sure that this fix lands in freenas/11.0-u2.1-stable, freenas/11.0-stable branches of for samba repo.

#14 Updated by John Hixson over 3 years ago

Vaibhav Chauhan wrote:

please make sure that this fix lands in freenas/11.0-u2.1-stable, freenas/11.0-stable branches of for samba repo.

I'm still working on this.

#15 Updated by Vaibhav Chauhan over 3 years ago

  • Target version changed from 11.1 to 11.0-U3

setting this to U3 as this will be part of next FreeNAS-U3 release, however please keep in mind that TrueNAS-11.0-U2.1 release is blocked by this.(#25509)

#16 Updated by John Hixson over 3 years ago

Vaibhav Chauhan wrote:

setting this to U3 as this will be part of next FreeNAS-U3 release, however please keep in mind that TrueNAS-11.0-U2.1 release is blocked by this.(#25509)

I'm still working on this.

#17 Updated by John Hixson over 3 years ago

I had lots & lots of problems fixing & testing this ;-) However, we are good now.

#18 Updated by John Hixson over 3 years ago

Fixed in commit: bede27df5aa144163c12cfbd0825ffbe2f709ab3
pull request: https://github.com/freenas/samba/pull/7
pull request: https://github.com/freenas/ports/pull/11

#19 Updated by John Hixson over 3 years ago

  • Status changed from Screened to Needs Developer Review
  • Assignee changed from John Hixson to Release Council

#20 Updated by Vaibhav Chauhan over 3 years ago

  • Assignee changed from Release Council to Timur Bakeyev

over to you timur

#21 Updated by Timur Bakeyev over 3 years ago

  • Status changed from Needs Developer Review to Reviewed by Developer
  • Assignee changed from Timur Bakeyev to Release Council

Looks OK to me.

#22 Updated by Vaibhav Chauhan over 3 years ago

  • Status changed from Reviewed by Developer to 47

#23 Updated by Dru Lavigne over 3 years ago

  • Assignee changed from Release Council to John Hixson

#24 Updated by Dru Lavigne over 3 years ago

  • Subject changed from winmsa recursively sets permissions of a rename destination's parent to Fix vfs_winmsa

#25 Updated by Joe Maloney over 3 years ago

  • Assignee changed from John Hixson to Joe Maloney

#26 Updated by Joe Maloney over 3 years ago

  • File winmsa working.png winmsa working.png added
  • Status changed from 47 to Ready For Release
  • Needs QA changed from Yes to No
  • QA Status Test Passes added
  • QA Status deleted (Not Tested)
12339

Seems to be working as expected. Renamed folder does not modify ACLs on sister folder as shown in screenshot.

#27 Updated by Vaibhav Chauhan over 3 years ago

  • Status changed from Ready For Release to Resolved

#28 Updated by Joe Maloney over 3 years ago

  • Assignee changed from Joe Maloney to John Hixson

#29 Updated by Dru Lavigne over 3 years ago

  • Description updated (diff)

#30 Updated by Dru Lavigne over 3 years ago

  • Private changed from Yes to No

Also available in: Atom PDF