Question regarding the patch "cifs: Fix the target file was deleted when rename failed."

From: Jan Melcher <mail@jan-melcher.de>
To: linux-cifs@vger.kernel.org
Subject: Question regarding the patch "cifs: Fix the target file was deleted when rename failed."
Date: Wed, 5 May 2021 13:09:20 +0200	[thread overview]
Message-ID: <CAHFuRQaCL4nWp0W1WBbQ-ycZAOW0gV9LgT7RmqiPaUaVaac-6w@mail.gmail.com> (raw)

I hope this is the right place for me to start a discussion regarding
a problem in the cifs file system.

I'm experiencing the problem the patch "cifs: Fix the target file was
deleted when rename failed."
(https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=9ffad9263b467efd8f8dc7ae1941a0a655a2bab2)
was trying to solve. It was further described in the samba-technical
mailing list (https://lists.samba.org/archive/samba-technical/2020-July/135592.html).
The patch was eventually reverted
(https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=0e6705182d4e1b77248a93470d6d7b3013d59b30).

Before I found the patch and the mailing list entry, I produced the
problem with the following sequence:

$ exec 20>file # open file and leave file descriptor open
$ touch file.lock
$ mv file.lock file
mv: cannot move 'file.lock' to 'file': Permission denied
$ ls -la
total 16
drwxr-xr-x 2 2000 2000     0 May  4 12:17 .
drwxr-xr-x 2 2000 2000 16384 May  4 12:17 ..
-rwxr-xr-x 1 2000 2000     0 May  4 12:17 file.lock

In other words, renaming a file onto an existing file that has an open
file descriptor effectively deletes that original target file. This
happens both with samba and with a Windows server. I thzink this
command sequence seems is quite common because that's the way many
applications do file locking on posix file systems. In our case, the
problem corrupted Git repositories multiple times because of packfile
indices getting deleted.

The patch I linked would have reduced the problem from a corruption to
a mere failed operation (the unlink-then-rename strategy is the last
resort at that place; if it is skipped, the rename fails).

Digging through the cifs history, I found this patch
(https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=14121bdccc17b8c0e4368a9c0e4f82c3dd47f240)
from 2008: "cifs: make cifs_rename handle -EACCES errors". It tried to
rename the target file to a random name (a "silly rename" I guess) and
also marked it for deletion, then tried the actual rename operation.
In my understanding, this solution should solve the mentioned problem
because renames are still allowed for files that have open file
handles. (Of course with the problem that for a short time, the target
file does not exist at all, but this problem also exists today).

The patch has been reverted shortly after
(https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=8d281efb67463fe8aac8f6e10b31492fc218bf2b)
because it would cause problems with servers that do not support busy
file renames. Maybe the situation changed since 2008 and there are
less servers that do not support busy file renames (my Windows machine
supports it), or we could find a way to re-implement the patch for
servers that do support busy file renames. The logic to handle a file
handle on the source file already tries to to a busy-file-rename by
the way.

These are just my thoughts after two days of digging into the problems
and never having seen the cifs code before, so please forgive me if
I'm just talking nonsense. But it would be great to hear what you
think about this.

Jan