From a0bfa89790897a0f7cc4de81835e04672ce02f3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20Filipensk=C3=BD?= Date: Thu, 28 Nov 2024 18:39:53 +0100 Subject: [PATCH 1/5] s3:vfs_crossrename: avoid locking panic in copy_reg() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use low level backend functions that don't go through the FSA layer. Done via calling transfer_file() as it was in version before 5c18f07 BUG: https://bugzilla.samba.org/show_bug.cgi?id=15724 Signed-off-by: Pavel Filipenský Reviewed-by: Ralph Boehme (cherry picked from commit 0a5da82f75a43838be3419cab10a50750fa500d7) --- source3/modules/vfs_crossrename.c | 125 ++++++++++++++++++++---------- 1 file changed, 84 insertions(+), 41 deletions(-) diff --git a/source3/modules/vfs_crossrename.c b/source3/modules/vfs_crossrename.c index 042144bfc4d..7c0ae94d257 100644 --- a/source3/modules/vfs_crossrename.c +++ b/source3/modules/vfs_crossrename.c @@ -54,10 +54,12 @@ static NTSTATUS copy_reg(vfs_handle_struct *handle, struct files_struct *dstfsp, const struct smb_filename *dest) { - NTSTATUS status; - struct smb_filename *full_fname_src = NULL; - struct smb_filename *full_fname_dst = NULL; + NTSTATUS status = NT_STATUS_OK; int ret; + off_t off; + int ifd = -1; + int ofd = -1; + struct timespec ts[2]; if (!VALID_STAT(source->st)) { status = NT_STATUS_OBJECT_PATH_NOT_FOUND; @@ -79,21 +81,6 @@ static NTSTATUS copy_reg(vfs_handle_struct *handle, goto out; } - full_fname_src = full_path_from_dirfsp_atname(talloc_tos(), - srcfsp, - source); - if (full_fname_src == NULL) { - status = NT_STATUS_NO_MEMORY; - goto out; - } - full_fname_dst = full_path_from_dirfsp_atname(talloc_tos(), - dstfsp, - dest); - if (full_fname_dst == NULL) { - status = NT_STATUS_NO_MEMORY; - goto out; - } - ret = SMB_VFS_NEXT_UNLINKAT(handle, dstfsp, dest, @@ -103,40 +90,96 @@ static NTSTATUS copy_reg(vfs_handle_struct *handle, goto out; } + ifd = openat(fsp_get_pathref_fd(srcfsp), + source->base_name, + O_RDONLY, + 0); + if (ifd < 0) { + status = map_nt_error_from_unix(errno); + goto out; + } + + ofd = openat(fsp_get_pathref_fd(dstfsp), + dest->base_name, + O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW, + 0600); + if (ofd < 0) { + status = map_nt_error_from_unix(errno); + goto out; + } + + off = transfer_file(ifd, ofd, source->st.st_ex_size); + if (off == -1) { + status = map_nt_error_from_unix(errno); + goto out; + } + + ret = fchown(ofd, source->st.st_ex_uid, source->st.st_ex_gid); + if (ret == -1 && errno != EPERM) { + status = map_nt_error_from_unix(errno); + goto out; + } + /* - * copy_internals() takes attribute values from the NTrename call. - * - * From MS-CIFS: - * - * "If the attribute is 0x0000, then only normal files are renamed. - * If the system file or hidden attributes are specified, then the - * rename is inclusive of both special types." + * fchown turns off set[ug]id bits for non-root, + * so do the chmod last. */ - status = copy_internals(talloc_tos(), - handle->conn, - NULL, - srcfsp, /* src_dirfsp */ - full_fname_src, - dstfsp, /* dst_dirfsp */ - full_fname_dst, - FILE_ATTRIBUTE_HIDDEN | FILE_ATTRIBUTE_SYSTEM); - if (!NT_STATUS_IS_OK(status)) { + ret = fchmod(ofd, source->st.st_ex_mode & 07777); + if (ret == -1 && errno != EPERM) { + status = map_nt_error_from_unix(errno); goto out; } - ret = SMB_VFS_NEXT_UNLINKAT(handle, - srcfsp, - source, - 0); + /* Try to copy the old file's modtime and access time. */ + ts[0] = source->st.st_ex_atime; + ts[1] = source->st.st_ex_mtime; + ret = futimens(ofd, ts); + if (ret == -1) { + DBG_DEBUG("Updating the time stamp on destinaton '%s' failed " + "with '%s'. Rename operation can continue.\n", + dest->base_name, + strerror(errno)); + } + + ret = close(ifd); + if (ret == -1) { + status = map_nt_error_from_unix(errno); + goto out; + } + ifd = -1; + + ret = close(ofd); if (ret == -1) { status = map_nt_error_from_unix(errno); goto out; } + ofd = -1; + + ret = SMB_VFS_NEXT_UNLINKAT(handle, srcfsp, source, 0); + if (ret == -1) { + status = map_nt_error_from_unix(errno); + } - out: +out: + if (ifd != -1) { + ret = close(ifd); + if (ret == -1) { + DBG_DEBUG("Failed to close %s (%d): %s.\n", + source->base_name, + ifd, + strerror(errno)); + } + } + if (ofd != -1) { + ret = close(ofd); + if (ret == -1) { + DBG_DEBUG("Failed to close %s (%d): %s.\n", + dest->base_name, + ofd, + strerror(errno)); + } + } - TALLOC_FREE(full_fname_src); - TALLOC_FREE(full_fname_dst); return status; } -- 2.47.0 From cdfc4ba0c369af8f87a0fd5e08da4dddff0385c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20Filipensk=C3=BD?= Date: Thu, 28 Nov 2024 18:32:25 +0100 Subject: [PATCH 2/5] s3:vfs_crossrename: crossrename_renameat() needs to return 0 if copy_reg() is successful MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BUG: https://bugzilla.samba.org/show_bug.cgi?id=15724 Signed-off-by: Pavel Filipenský Reviewed-by: Ralph Boehme (cherry picked from commit 0a9adc85e77bc557bb8be12237fa31c4142dd3d5) --- source3/modules/vfs_crossrename.c | 1 + 1 file changed, 1 insertion(+) diff --git a/source3/modules/vfs_crossrename.c b/source3/modules/vfs_crossrename.c index 7c0ae94d257..a4f035d3141 100644 --- a/source3/modules/vfs_crossrename.c +++ b/source3/modules/vfs_crossrename.c @@ -211,6 +211,7 @@ static int crossrename_renameat(vfs_handle_struct *handle, smb_fname_src, dstfsp, smb_fname_dst); + result = 0; if (!NT_STATUS_IS_OK(status)) { errno = map_errno_from_nt_status(status); result = -1; -- 2.47.0 From 8fadcd0c78fadd066e7a5da52c8a537fa58c49d7 Mon Sep 17 00:00:00 2001 From: Jones Syue Date: Thu, 26 Sep 2024 17:17:14 +0800 Subject: [PATCH 3/5] s3:vfs_crossrename: add back checking for errno ENOENT strace gives a clue: samba try to remove 'file.txt' in the dst folder but actually it is not existed yet, and got an errno = ENOENT, renameat(32, "file.txt", 31, "file.txt") = -1 EXDEV (Invalid cross-device link) unlinkat(31, "file.txt", 0) = -1 ENOENT (No such file or directory) Commit 5c18f074be92 ("s3: VFS: crossrename. Use real dirfsp for SMB_VFS_RENAMEAT()") seems unintentionally removed errno ENOENT checking, so add it back could address 1st issue. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15724 Signed-off-by: Jones Syue Reviewed-by: Ralph Boehme (cherry picked from commit 1a089a16c40e0b3bc5d4fcde559157cf137056c2) --- source3/modules/vfs_crossrename.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/modules/vfs_crossrename.c b/source3/modules/vfs_crossrename.c index a4f035d3141..1da36706ecb 100644 --- a/source3/modules/vfs_crossrename.c +++ b/source3/modules/vfs_crossrename.c @@ -85,7 +85,7 @@ static NTSTATUS copy_reg(vfs_handle_struct *handle, dstfsp, dest, 0); - if (ret == -1) { + if (ret == -1 && errno != ENOENT) { status = map_nt_error_from_unix(errno); goto out; } -- 2.47.0 From 0401b36db54dd105d389830e74e54a415c4d0b4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20Filipensk=C3=BD?= Date: Mon, 2 Dec 2024 22:27:39 +0100 Subject: [PATCH 4/5] docs:manpage: vfs_crossrename is not fully stackable VFS module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BUG: https://bugzilla.samba.org/show_bug.cgi?id=15724 Signed-off-by: Pavel Filipenský Reviewed-by: Ralph Boehme (cherry picked from commit 94c9a99c56db438c391a966c927ec2f862c373e7) --- docs-xml/manpages/vfs_crossrename.8.xml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs-xml/manpages/vfs_crossrename.8.xml b/docs-xml/manpages/vfs_crossrename.8.xml index 72d67d685b1..7ea0b50cc9b 100644 --- a/docs-xml/manpages/vfs_crossrename.8.xml +++ b/docs-xml/manpages/vfs_crossrename.8.xml @@ -62,7 +62,10 @@ - This module is stackable. + This module is not fully stackable. It can be combined with other + modules, but should be the last module in the vfs objects + list. It directly access the files in the OS filesystem. + -- 2.47.0 From f6a239b5a836770344dab714eb03d92875dc076f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20Filipensk=C3=BD?= Date: Wed, 4 Dec 2024 11:02:18 +0100 Subject: [PATCH 5/5] selftest: Add test for vfs crossrename module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BUG: https://bugzilla.samba.org/show_bug.cgi?id=15724 Signed-off-by: Pavel Filipenský Reviewed-by: Ralph Boehme (cherry picked from commit 02d4f58a2f7ac2db60dd2e4d16a3cbf71b3f08a9) --- selftest/target/Samba3.pm | 12 +++++ source3/script/tests/test_recycle.sh | 80 +++++++++++++++++++++++++++- source3/selftest/tests.py | 2 +- 3 files changed, 92 insertions(+), 2 deletions(-) diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm index a7dd1b20e66..17343e63e52 100755 --- a/selftest/target/Samba3.pm +++ b/selftest/target/Samba3.pm @@ -2780,6 +2780,9 @@ sub provision($$) my $recycle_shrdir="$shrdir/recycle"; push(@dirs,$recycle_shrdir); + my $recycle_shrdir2="$shrdir/recycle2"; + push(@dirs,$recycle_shrdir2); + my $fakedircreatetimes_shrdir="$shrdir/fakedircreatetimes"; push(@dirs,$fakedircreatetimes_shrdir); @@ -3715,6 +3718,15 @@ sub provision($$) recycle : exclude = *.tmp recycle : directory_mode = 755 +[recycle2] + copy = tmp + path = $recycle_shrdir2 + vfs objects = recycle crossrename + recycle : repository = .trash + recycle : exclude = *.tmp + recycle : directory_mode = 755 + wide links = yes + [fakedircreatetimes] copy = tmp path = $fakedircreatetimes_shrdir diff --git a/source3/script/tests/test_recycle.sh b/source3/script/tests/test_recycle.sh index ba1d0a598b1..779683f4822 100755 --- a/source3/script/tests/test_recycle.sh +++ b/source3/script/tests/test_recycle.sh @@ -29,7 +29,8 @@ export SAMBA_DEPRECATED_SUPPRESS # Define the test environment/filenames. # -share_test_dir="$LOCAL_PATH" +share_test_dir="$LOCAL_PATH/recycle" +share_test_dir2="$LOCAL_PATH/recycle2" # # Cleanup function. @@ -43,6 +44,13 @@ do_cleanup() rm -f testfile2.tmp rm -rf .trash ) + ( + #subshell. + cd "$share_test_dir2" || return + rm -f testfile3 + rm -f testfile4.tmp + rm -rf .trash + ) } # @@ -50,6 +58,25 @@ do_cleanup() # do_cleanup +# Setup .trash on a different filesystem to test crossrename +# /tmp or /dev/shm should provide tmpfs +# +for T in /tmp /dev/shm +do + if df --portability --print-type $T 2>/dev/null | grep -q tmpfs; then + TRASHDIR=$T + break + fi +done + +if [ -z $TRASHDIR ]; then + echo "No tmpfs filesystem found." + exit 1 +fi + +TRASHDIR=$(mktemp -d /$TRASHDIR/.trash_XXXXXX) +chmod 0755 $TRASHDIR +ln -s $TRASHDIR $share_test_dir2/.trash test_recycle() { @@ -90,12 +117,61 @@ quit return 0 } +test_recycle_crossrename() +{ + tmpfile=$PREFIX/smbclient_interactive_prompt_commands + echo " +put $tmpfile testfile3 +put $tmpfile testfile4.tmp +del testfile3 +del testfile4.tmp +quit +" > $tmpfile + cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT -U$USERNAME%$PASSWORD //$SERVER/recycle2 -I$SERVER_IP $ADDARGS < $tmpfile 2>&1' + eval echo "$cmd" + out=$(eval "$cmd") + ret=$? + rm -f "$tmpfile" + + if [ $ret != 0 ]; then + printf "%s\n" "$out" + printf "failed recycle smbclient run with error %s\n" "$ret" + return 1 + fi + + test -e "$share_test_dir2/.trash/testfile3" || { + printf ".trash/testfile3 expected to exist but does NOT exist\n" + return 1 + } + test -e "$share_test_dir2/.trash/testfile4.tmp" && { + printf ".trash/testfile4.tmp not expected to exist but DOES exist\n" + return 1 + } + deviceid1=`stat -c '%d' "$share_test_dir2/"` + deviceid2=`stat -c '%d' "$share_test_dir2/.trash/"` + test "$deviceid1=" != "$deviceid2" || { + printf ".trash/ should be on a different filesystem!\n" + return 1 + } + perm_want=755 + perm_is=`stat -c '%a' "$share_test_dir2/.trash/"` + test "$perm_is" = "$perm_want" || { + printf ".trash/ permission should be $perm_want but is $perm_is\n" + return 1 + } + return 0 +} + panic_count_0=$(grep -c PANIC $SMBD_TEST_LOG) testit "recycle" \ test_recycle || failed=$((failed + 1)) +testit "recycle_crossrename" \ + test_recycle_crossrename || + failed=$((failed + 1)) + panic_count_1=$(grep -c PANIC $SMBD_TEST_LOG) testit "check_panic" test $panic_count_0 -eq $panic_count_1 || failed=$(expr $failed + 1) @@ -103,5 +179,7 @@ testit "check_panic" test $panic_count_0 -eq $panic_count_1 || failed=$(expr $fa # # Cleanup. do_cleanup +# Cleanup above only deletes a symlink, delete also /tmp/.trash_XXXXXX dir +rm -rf "$TRASHDIR" testok "$0" "$failed" diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index 88151caea11..fe67a4df896 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -784,7 +784,7 @@ for env in ["fileserver"]: plantestsuite("samba3.blackbox.force_create_mode", env, [os.path.join(samba3srcdir, "script/tests/test_force_create_mode.sh"), '$SERVER', '$DOMAIN', '$USERNAME', '$PASSWORD', '$PREFIX', env, smbclient3]) plantestsuite("samba3.blackbox.dropbox", env, [os.path.join(samba3srcdir, "script/tests/test_dropbox.sh"), '$SERVER', '$DOMAIN', 'gooduser', '$PASSWORD', '$PREFIX', env, smbclient3]) plantestsuite("samba3.blackbox.offline", env, [os.path.join(samba3srcdir, "script/tests/test_offline.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/offline', smbclient3]) - plantestsuite("samba3.blackbox.recycle", env, [os.path.join(samba3srcdir, "script/tests/test_recycle.sh"), '$SERVER', '$SERVER_IP', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/recycle', '$PREFIX', smbclient3]) + plantestsuite("samba3.blackbox.recycle", env, [os.path.join(samba3srcdir, "script/tests/test_recycle.sh"), '$SERVER', '$SERVER_IP', '$USERNAME', '$PASSWORD', '$LOCAL_PATH', '$PREFIX', smbclient3]) plantestsuite("samba3.blackbox.fakedircreatetimes", env, [os.path.join(samba3srcdir, "script/tests/test_fakedircreatetimes.sh"), '$SERVER', '$SERVER_IP', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/fakedircreatetimes', '$PREFIX', smbclient3]) plantestsuite("samba3.blackbox.shadow_copy2.NT1", env + "_smb1_done", [os.path.join(samba3srcdir, "script/tests/test_shadow_copy.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/shadow', smbclient3, '-m', 'NT1']) plantestsuite("samba3.blackbox.shadow_copy2.SMB3", env, [os.path.join(samba3srcdir, "script/tests/test_shadow_copy.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/shadow', smbclient3, '-m', 'SMB3']) -- 2.47.0