vfs_crossrename not working in samba-4.15.*
Jeremy Allison
jra at samba.org
Thu Oct 6 17:55:10 UTC 2022
On Thu, Oct 06, 2022 at 10:40:57AM -0400, Andrew Walker via samba-technical wrote:
>On Thu, Oct 6, 2022 at 10:28 AM Pavel Filipenský via samba-technical <
>samba-technical at lists.samba.org> wrote:
>
>> Hi,
>>
>> I am working on a fix for RH bugzillahttps://
>> bugzilla.redhat.com/show_bug.cgi?id=2125339
>>
>> If the share is using vfs_recycle and vfs_crossrename modules and the
>> recycle repo is on a different filesystem, like here:
>>
>> vfs objects = recycle crossrename
>> recycle:repository = /different_filesystem/
>>
>> The deleted file should be moved to /different_filesystem/recycle, but it
>> fails. Since this is a different fs, we cannot use
>>
>> rename("File", "/different_filesystem/recycle/File")
>>
>> but instead samba code uses two syscalls:
>>
>> unlink("/different_filesystem/recycle/File")
>> openat(AT_FDCWD, "/different_filesystem/recycle/File", O_CREAT, 0600)
>>
>> Normally, the destination file "/different_filesystem/recycle/File" does
>> not exist and unlink() fails with ENOENT. This is correctly handled in
>> samba-4.12:
>>
>>
>> 51 static int copy_reg(const char *source, const char *dest)
>> 52 {
>> ...
>> 76 »···if (unlink (dest) && errno != ENOENT) {
>> 77 »···»···close(ifd);
>> 78 »···»···return -1;
>> 79 »···}
>>
>> But newer versions of samba miss the check for ENOENT and the operation
>> fails, doing map_nt_error_from_unix(errno). I have tried to the same fix,
>> and added to copy_reg() this:
>>
>> - if (ret == -1) {
>> + if (ret == -1 && errno != ENOENT) {
>> status = map_nt_error_from_unix(errno);
>> goto out;
>> }
>>
>> Unfortunately, it does not work. It causes this panic:
>>
>>
>>
>> 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
>> #8 0x00007fc04b215b14 in open_file_ntcreate (conn=0x5653790d46b0,
>> req=0x0, access_mask=137, share_access=7, create_disposition=1,
>> create_options=0, new_dos_attributes=0, oplock_request=8, lease=0x0,
>> private_flags=0, parent_dir_fname=0x5653790019b0,
>> smb_fname_atname=0x565379156ec0, pinfo=0x7fffa990111c, fsp=0x565379162bc0)
>> at ../../source3/smbd/open.c:4342
>> #9 0x00007fc04b21a8c8 in create_file_unixpath (conn=0x5653790d46b0,
>> req=0x0, dirfsp=0x56537905a9d0, smb_fname=0x5653791497d0, access_mask=137,
>> share_access=7, create_disposition=1, create_options=0,
>> file_attributes=128, oplock_request=8, lease=0x0, allocation_size=0,
>> private_flags=0, sd=0x0, ea_list=0x0, result=0x7fffa99012e0,
>> pinfo=0x7fffa99012ec) at ../../source3/smbd/open.c:6337
>> #10 0x00007fc04b21b4ae in create_file_default (conn=0x5653790d46b0,
>> req=0x0, dirfsp=0x56537905a9d0, smb_fname=0x5653791497d0, access_mask=137,
>> share_access=7, create_disposition=1, create_options=0,
>> file_attributes=128, oplock_request=0, lease=0x0, allocation_size=0,
>> private_flags=0, sd=0x0, ea_list=0x0, result=0x7fffa9901548,
>> pinfo=0x7fffa990153c, in_context_blobs=0x0, out_context_blobs=0x0)
>> at ../../source3/smbd/open.c:6656
>> #11 0x00007fc04b1cb932 in vfswrap_create_file (handle=0x565379152be0,
>> req=0x0, dirfsp=0x56537905a9d0, smb_fname=0x5653791497d0, access_mask=137,
>> share_access=7, create_disposition=1, create_options=0,
>> file_attributes=128, oplock_request=0, lease=0x0, allocation_size=0,
>> private_flags=0, sd=0x0, ea_list=0x0, result=0x7fffa9901548,
>> pinfo=0x7fffa990153c, in_context_blobs=0x0, out_context_blobs=0x0)
>> at ../../source3/modules/vfs_default.c:827
>> #12 0x00007fc04b227e43 in smb_vfs_call_create_file (handle=0x565379152be0,
>> req=0x0, dirfsp=0x56537905a9d0, smb_fname=0x5653791497d0, access_mask=137,
>> share_access=7, create_disposition=1, create_options=0,
>> file_attributes=128, oplock_request=0, lease=0x0, allocation_size=0,
>> private_flags=0, sd=0x0, ea_list=0x0, result=0x7fffa9901548,
>> pinfo=0x7fffa990153c, in_context_blobs=0x0, out_context_blobs=0x0)
>> at ../../source3/smbd/vfs.c:1576
>> #13 0x00007fc04b1e4b77 in copy_internals (ctx=0x5653790e1330,
>> conn=0x5653790d46b0, req=0x0, src_dirfsp=0x56537905a9d0,
>> smb_fname_src=0x5653791497d0, dst_dirfsp=0x5653790d4aa0,
>> smb_fname_dst=0x565379162700,
>> attrs=6) at ../../source3/smbd/smb2_nttrans.c:248
>> #14 0x00007fc0365e4060 in copy_reg (handle=0x565379148050,
>> srcfsp=0x56537905a9d0, source=0x565379155650, dstfsp=0x5653790d4aa0,
>> dest=0x5653790e28e0) at ../../source3/modules/vfs_crossrename.c:115
>> #15 0x00007fc0365e41aa in crossrename_renameat (handle=0x565379148050,
>> srcfsp=0x56537905a9d0, smb_fname_src=0x565379155650, dstfsp=0x5653790d4aa0,
>> smb_fname_dst=0x5653790e28e0)
>> at ../../source3/modules/vfs_crossrename.c:166
>> #16 0x00007fc04b228c91 in smb_vfs_call_renameat (handle=0x565379148050,
>> srcfsp=0x56537905a9d0, smb_fname_src=0x565379155650, dstfsp=0x5653790d4aa0,
>> smb_fname_dst=0x5653790e28e0) at ../../source3/smbd/vfs.c:1775
>> #17 0x00007fc0365ded2f in recycle_unlink_internal (handle=0x565379148680,
>> dirfsp=0x56537905a9d0, smb_fname=0x565379155650, flags=0) at
>> ../../source3/modules/vfs_recycle.c:690
>> #18 0x00007fc0365defaa in recycle_unlinkat (handle=0x565379148680,
>> dirfsp=0x56537905a9d0, smb_fname=0x565379155650, flags=0) at
>> ../../source3/modules/vfs_recycle.c:735
>> #19 0x00007fc04b2296fe in smb_vfs_call_unlinkat (handle=0x565379148680,
>> dirfsp=0x56537905a9d0, smb_fname=0x565379155650, flags=0) at
>> ../../source3/smbd/vfs.c:1932
>> #20 0x00007fc04b21d106 in close_remove_share_mode (fsp=0x565379152c80,
>> close_type=NORMAL_CLOSE) at ../../source3/smbd/close.c:581
>> #21 0x00007fc04b21dcf2 in close_normal_file (req=0x5653790ce430,
>> fsp=0x565379152c80, close_type=NORMAL_CLOSE) at
>> ../../source3/smbd/close.c:910
>> #22 0x00007fc04b22061c in close_file_smb (req=0x5653790ce430,
>> fsp=0x565379152c80, close_type=NORMAL_CLOSE) at
>> ../../source3/smbd/close.c:1663
>>
>>
>> The cause of the panic is "assert failed: share_mode_lock_key_refcount ==
>> 0" in source3/locking/share_mode_lock.c:3010
>> The share_mode_lock_key_refcount is already 1, the first reference is
>> taken here:
>>
>> Hardware watchpoint 2: share_mode_lock_key_refcount
>> Old value = 0
>> New value = 1
>> get_share_mode_lock_internal (id=..., servicepath=0x0, smb_fname=0x0,
>> old_write_time=0x0, lck=0x7fffa9901980) at
>> ../../source3/locking/share_mode_lock.c:972
>> 972»»···if (static_share_mode_data != NULL) {
>> #0 get_share_mode_lock_internal (id=..., servicepath=0x0, smb_fname=0x0,
>> old_write_time=0x0, lck=0x7fffa9901980) at
>> ../../source3/locking/share_mode_lock.c:972
>> #1 0x00007fc04b1bfa24 in share_mode_entry_prepare_lock_fn
>> (glck=0x7fffa9901190, cb_private=0x7fffa9901760) at
>> ../../source3/locking/share_mode_lock.c:2952
>> #2 0x00007fc04b05c7d2 in g_lock_lock_cb_run_and_store
>> (cb_state=0x7fffa9901190) at ../../source3/lib/g_lock.c:597
>> #3 0x00007fc04b05e8c1 in g_lock_lock_simple_fn (rec=0x7fffa99014a0,
>> value=..., private_data=0x7fffa9901630) at ../../source3/lib/g_lock.c:1212
>> #4 0x00007fc04b05833d in dbwrap_watched_do_locked_fn
>> (backend_rec=0x7fffa9901350, backend_value=...,
>> private_data=0x7fffa9901470) at ../../source3/lib/dbwrap/dbwrap_watch.c:458
>> #5 0x00007fc04aa33e44 in db_tdb_do_locked (db=0x5653790ca050, key=...,
>> fn=0x7fc04b0582ab <dbwrap_watched_do_locked_fn>,
>> private_data=0x7fffa9901470) at ../../lib/dbwrap/dbwrap_tdb.c:208
>> #6 0x00007fc04aa304db in dbwrap_do_locked (db=0x5653790ca050, key=...,
>> fn=0x7fc04b0582ab <dbwrap_watched_do_locked_fn>,
>> private_data=0x7fffa9901470) at ../../lib/dbwrap/dbwrap.c:553
>> #7 0x00007fc04b058435 in dbwrap_watched_do_locked (db=0x5653790cf910,
>> key=..., fn=0x7fc04b05e4fa <g_lock_lock_simple_fn>,
>> private_data=0x7fffa9901630) at ../../source3/lib/dbwrap/dbwrap_watch.c:480
>> #8 0x00007fc04aa304db in dbwrap_do_locked (db=0x5653790cf910, key=...,
>> fn=0x7fc04b05e4fa <g_lock_lock_simple_fn>, private_data=0x7fffa9901630) at
>> ../../lib/dbwrap/dbwrap.c:553
>> #9 0x00007fc04b05eb28 in g_lock_lock (ctx=0x565379020280, key=...,
>> type=G_LOCK_WRITE, timeout=..., cb_fn=0x7fc04b1bf90d
>> <share_mode_entry_prepare_lock_fn>, cb_private=0x7fffa9901760) at
>> ../../source3/lib/g_loc…
>> #10 0x00007fc04b1bfe1c in _share_mode_entry_prepare_lock
>> (prepare_state=0x7fffa9901960, id=..., servicepath=0x0, smb_fname=0x0,
>> old_write_time=0x0, fn=0x7fc04b21c0b5 <close_share_mode_lock_prepare>,
>> private_dat…
>> #11 0x00007fc04b21c99b in close_remove_share_mode (fsp=0x565379152c80,
>> close_type=NORMAL_CLOSE) at ../../source3/smbd/close.c:451
>> #12 0x00007fc04b21dcf2 in close_normal_file (req=0x5653790ce430,
>> fsp=0x565379152c80, close_type=NORMAL_CLOSE) at
>> ../../source3/smbd/close.c:910
>> #13 0x00007fc04b22061c in close_file_smb (req=0x5653790ce430,
>> fsp=0x565379152c80, close_type=NORMAL_CLOSE) at
>> ../../source3/smbd/close.c:1663
>>
>>
>> So the issue comes from the logic in close_remove_share_mode()
>>
>> static NTSTATUS close_remove_share_mode(files_struct *fsp, enum
>> file_close_type close_type)
>> {
>> status =
>> share_mode_entry_prepare_lock_del(&lck_state.prepare_state,
>>
>> ret = SMB_VFS_UNLINKAT(conn,
>>
>> ulstatus =
>> share_mode_entry_prepare_unlock(&lck_state.prepare_state,
>> }
>>
>> Each process can hold just one share_mode_lock_key_refcount, but here we
>> have two nested calls, both grabbing share_mode_lock_key_refcount:
>> 1. share_mode_entry_prepare_lock_del()
>> 2. SMB_VFS_UNLINKAT() -> recycle_unlink_internal() ->
>> crossrename_renameat() -> open_file_ntcreate() ->
>> _share_mode_entry_prepare_lock()
>>
>>
>>
>> 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?
>>
>>
>> Thanks,
>> Pavel
>>
>
>Perhaps instead of layering the recyclebin over vfs_crossrename, we should
>generate separate recyclebins at filesystem mountpoints when the SMB
>share contains nested ones?
>
>This would preserve atomicity of the rename operation and would avoid
>exposing users to limits of vfs_crossrename (e.g. purging of files that
>exceed the cross-rename limit -- which is a pretty significant POLA
>violation).
>
>If we want to go this route, I can make a merge request to add it (since I
>already have POC code to do this).
That sounds like a plan to me ! Can we see the MR request ?
More information about the samba-technical
mailing list