vfs_crossrename not working in samba-4.15.*

[Jeremy Allison]

On Mon, Oct 10, 2022 at 12:34:24PM +0200, Stefan Metzmacher via samba-technical 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)

As I recall, this was in the middle of the move to fixing our symlink
problem with dirfsp's - so I'm not going to apologise for the lack
of tests on such a rarely used module :-).

Better to fix up later (as we're doing now).

>I guess we can't use copy_internals() here.

copy_internals is horrible and I'd like to see it removed to
be honest.

>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.

A. Modest. Proposal. :-). Needing crossrename is a sign of
administrator failure to set up the share directories correctly.

Can we just delete this module ?

Pavel, who is using this and for what purpose ?