vfs_crossrename not working in samba-4.15.*

Pavel Filipenský pfilipensky at samba.org
Mon Oct 10 13:35:00 UTC 2022


Hi metze,

On 10/10/22 12:34, Stefan Metzmacher wrote:
> Hi Pavel,
>
>> Program received signal SIGABRT, Aborted.
>> __pthread_kill_implementation (threadid=<optimized out>, 
>> signo=signo at entry=6, no_tid=no_tid at entry=0) at pthread_kill.c:44
>> 44»·      return INTERNAL_SYSCALL_ERROR_P (ret) ? 
>> INTERNAL_SYSCALL_ERRNO (ret) : 0;
>> #0  __pthread_kill_implementation (threadid=<optimized out>, 
>> signo=signo at entry=6, no_tid=no_tid at entry=0) at pthread_kill.c:44
>> #1  0x00007fc04abb7cb3 in __pthread_kill_internal (signo=6, 
>> threadid=<optimized out>) at pthread_kill.c:78
>> #2  0x00007fc04ab679c6 in __GI_raise (sig=sig at entry=6) at 
>> ../sysdeps/posix/raise.c:26
>> #3  0x00007fc04ab517f4 in __GI_abort () at abort.c:79
>> #4  0x00007fc04b076bfa in dump_core () at 
>> ../../source3/lib/dumpcore.c:338
>> #5  0x00007fc04b088661 in smb_panic_s3 (why=0x7fc04b311928 "assert 
>> failed: share_mode_lock_key_refcount == 0") at 
>> ../../source3/lib/util.c:713
>> #6  0x00007fc04ad7c9a1 in smb_panic (why=0x7fc04b311928 "assert 
>> failed: share_mode_lock_key_refcount == 0") at 
>> ../../lib/util/fault.c:198
>> #7  0x00007fc04b1bfd7a in _share_mode_entry_prepare_lock 
>> (prepare_state=0x7fffa9900f60, id=..., servicepath=0x565379144e10 
>> "/home/pfilipen/workspace/projects/samba/2571/st/ad_member/share",
>>      smb_fname=0x565379162da0, old_write_time=0x7fffa9900eb0, 
>> fn=0x7fc04b2145d9 <open_ntcreate_lock_add_entry>, 
>> private_data=0x7fffa9900f60, location=0x7fc04b32c0f0 
>> "../../source3/smbd/open.c:4342")
>>      at ../../source3/locking/share_mode_lock.c:3010
>
> ...
>
>> How this should be fixed? Can we remove the assert and allow to grab 
>> the share_mode_lock_key_refcount
>>   again give the owner is the process itself?
>
> It seems the problem got introduced by this commit:
> https://git.samba.org/?p=samba.git;a=commitdiff;h=db743ab005bc7671d4fb0f5bea895c1097b707c5 
>
> "share_mode_lock: DEBUG/ASSERT recursion deadlock detection"
>
> Before open_file_ntcreate() just failed with 
> NT_STATUS_SHARING_VIOLATION if
> get_share_mode_lock() returned NULL... But in 4.14 copy_reg() also 
> used raw open() syscalls...
>
> So the specific problem comes from
> https://git.samba.org/?p=samba.git;a=commitdiff;h=5c18f074be92beea8357a93d64e7118376255ac0 
>
>     s3: VFS: crossrename. Use real dirfsp for SMB_VFS_RENAMEAT()
>
>     Finally fix the promise from the docs that this module is 
> stackable. Re-use copy_internals().
>
>     This is a horrible module that must be removed !
>
>     Signed-off-by: Jeremy Allison <jra at samba.org>
>     Reviewed-by: Noel Power <npower at samba.org>
>
> That commit came after db743ab005bc7671d4fb0f5bea895c1097b707c5
> and seems it was not tested at all (at least together with the recycle 
> module)
>
> I guess we can't use copy_internals() here.
>

Note: The problem is already visible without vfs_recycle. The 
vfs_crossrename itself no longer works.

So we have two requirements that do not go together really well:

1) make crossrename stackable using VFS ops

2) keep the share mode locking safe

The current implementation of share mode locking asserts that the 
process can never have more than one share mode lock simultaneously,
because the current code uses static singleton for the shared mode lock, 
thus cannot provide multiple lock nesting.


> Now that we use g_lock we may allow share_mode_do_locked_vfs_allowed() or
> share_mode_entry_prepare_lock_del() with 
> close_share_mode_lock_prepare() setting
> *keep_locked = true to lock multiple file_ids at the same time...
> But it will be a tricky change.
>
If such change is done, we can use stackable copy_reg() that takes two 
share mode locks.  You wrote: /I guess we can't use copy_internals() here. /

Maybe copy_reg can be improved or do you see any serious problem with it?


BTW, there are two issues with current copy_reg() / copy_internals() 
code, both related to the existence of the destination file.


1) copy_reg() bails out if the dst file does not exist

- but non existence of dst is no problem and th ecode mus go on

  97 »·······ret = SMB_VFS_NEXT_UNLINKAT(handle,
  98 »·······»·······»·······»······· dstfsp,
  99 »·······»·······»·······»······· dest,
100 »·······»·······»·······»······· 0);
101 »·······if (ret == -1) {

we need here:

101 »·······if (ret == -1 && errno != ENOENT) {


2) copy_internals() fails if the destination file exist

- this should pass if we do force rename, e.g: smbclient -c "src dst 
-f", but we fail no matter of -f option

197 NTSTATUS copy_internals(TALLOC_CTX *ctx,
...
232 »·······/* Disallow if dst file already exists. */
233 »·······if (VALID_STAT(smb_fname_dst->st)) {
234 »·······»·······status = NT_STATUS_OBJECT_NAME_COLLISION;
235 »·······»·······goto out;
236 »·······}


> metze
>
>
>
>
>
>


More information about the samba-technical mailing list