Since commit 1b2f40ee33cdb931f849a2a8211f6b2c729b39b7 ("vfs_fruit: Fix namelength overflow errors in fruit_rename", https://gitlab.com/samba-team/samba/-/commit/1b2f40ee33cdb931f849a2a8211f6b2c729b39b7) fruit_renameat() renames the "._" sidecar before the base file, but never rolls that back. If the base rename fails, the sidecar is left under the destination name while the base file stays under the source name, so the source file loses its resource fork even though the client was told the rename failed. Reachable with ordinary filenames, for example by renaming a file that has a sidecar onto an existing directory with replace_if_exists set, where the base rename() fails with EISDIR. Also in the same function, from commit 1f6909cbb7a2fbd0784a72d4d2660b0f697d39d0 ("vfs_fruit: Allow renames for long filenames that don't have a ._ sidecar", https://gitlab.com/samba-team/samba/-/commit/1f6909cbb7a2fbd0784a72d4d2660b0f697d39d0): the fstatat() after ENAMETOOLONG handles rc == 0, ENAMETOOLONG and ENOENT, and any other errno falls through to the base rename with the failed rc overwritten. Both are master only. Backport patches for 4.25 and 4.24 are attached to bug 16238.
Is that worse than what we had before? Before we did rename the main file but if we failed to rename the sidecar file we also did not roll back the main file. We don't have transactions that would cover both files.
MR: https://gitlab.com/samba-team/samba/-/merge_requests/4717 (In reply to Volker Lendecke from comment #1) Yes, for this case. What changed is the order. Before https://gitlab.com/samba-team/samba/-/merge_requests/4695#:~:text=Merge%20requests-,!4695,-Fix%20fruit_rename%20problems: 1. rename base 2. rename sidecar A failing base rename returned at step 1, nothing had been touched. After https://gitlab.com/samba-team/samba/-/merge_requests/4695#:~:text=Merge%20requests-,!4695,-Fix%20fruit_rename%20problems: 1. rename sidecar 2. rename base A failing base rename at step 2 leaves the sidecar already moved, so the client gets the error and is left with the base file under the source name and its metadata under the destination name. EISDIR is enough to reach it: rename onto an existing directory with replace_if_exists set. Only the sidecar rename has happened at that point, so this does not need a transaction, just a rename back. Fix and reproducer in !4717.
(In reply to Priyanka Soni from comment #2) > Yes, for this case. What changed is the order. Yes, and that was deliberate. Before the patch that went in a failing side-car rename left the file system in a similarly inconsistent state. As this is all racy as hell anyway, I would rather add checks for as many error conditions before starting the renames instead of trying to clean up after something fails. Why is it worse now than it was before?
https://gitlab.com/samba-team/samba/-/merge_requests/4717
(In reply to Volker Lendecke from comment #3) Because the set of failures that can split the two files changed. Before, the base rename ran first, so every base rename failure was harmless: we returned with nothing touched. After the reorder, every base rename failure leaves the sidecar moved and the base file where it was, and the client is told the rename failed, so it has no reason to go looking for the resource fork that just went missing. That is the whole answer to "why is it worse": a class of failures that used to be free is now destructive. Now every base rename failure splits them, on ordinary names. The reproducer is a plain rename onto an existing directory with replace_if_exists set, where rename() returns EISDIR. The client is told the rename failed, so it has no reason to go looking for the resource fork that just went missing. I do not think the two approaches exclude each other. Checking up front is better where we can, but a check cannot see EACCES, a sticky bit EPERM, or the destination changing under us, and those still leave the sidecar moved and the base file where it was. Racy or not, at the point we fail we know exactly one rename has happened, and undoing it is a single syscall on an error path. If the undo fails we log it and return the original errno, so we are never worse off than we are today.