Re: [PATCH v2] smb: client: fix data loss due to broken rename(2)

[Paulo Alcantara <pc@manguebit.org>]

ronnie sahlberg <ronniesahlberg@gmail.com> writes:

> On Thu, 4 Sept 2025 at 04:46, Paulo Alcantara <pc@manguebit.org> wrote:
>>
>> Ralph Boehme <slow@samba.org> writes:
>>
>> > Hi Paulo,
>> >
>> > On 9/2/25 9:09 PM, Paulo Alcantara wrote:
>> >> Ralph Boehme <slow@samba.org> writes:
>> >>
>> >>> Why not simply fail the rename instead of trying to implement some
>> >>> clever but complex and error prone fallback?
>> >>
>> >> We're doing this for SMB1 for a very long time and haven't heard of any
>> >> issues so far.  I've got a "safer" version [1] that does everything a
>> >> single compound request but then implemented this non-compound version
>> >> due to an existing Azure bug that seems to limit the compound in 4
>> >> commands, AFAICT.  Most applications depend on such behavior working,
>> >> which is renaming open files.
>> >
>> > maybe I'm barking of the wrong tree, but you *can* rename open files:
>> >
>> > $ bin/smbclient -U 'USER%PASS' //IP/C\$
>> > smb: \> cd Users\administrator.WINCLUSTER\Desktop\
>> > smb: \Users\administrator.WINCLUSTER\Desktop\> open
>> > t-ph-oplock-b-downgraded-s.cab
>> > open file
>> > \Users\administrator.WINCLUSTER\Desktop\t-ph-oplock-b-downgraded-s.cab:
>> > for read/write fnum 1
>> > smb: \Users\administrator.WINCLUSTER\Desktop\> rename
>> > t-ph-oplock-b-downgraded-s.cab renamed
>> > smb: \Users\administrator.WINCLUSTER\Desktop\>
>> >
>> > ...given the open is with SHARE_DELETE (had to tweak smbclient to
>> > actually allow that).
>>
>> Interesting.
>>
>> cifs.ko will get STATUS_ACCESS_DENIED when attempting to rename the open
>> file.  The file was open with SHARE_READ|SHARE_WRITE|SHARE_DELETE, BTW.
>>
>> Also, note that cifs.ko will not reuse the open handle, rather it will
>> send a compound request of create+set_info(rename)+close to the file
>> which will fail with STATUS_ACCESS_DENIED.
>>
>> What am I missing?
>>
>> > If the rename destination is open and the server rightly fails the
>> > rename for that reason, then masking that error is a mistake imho.
>> >
>> > When doing
>> >
>> > $ mv a b
>> >
>> > the user asked to rename a, he did NOT ask to rename b which becomes
>> > important, because if you do
>> >
>> > rename("b", ".renamehackXXXX")
>> >
>> > under the hood and then reattempt the rename
>> >
>> > rename("a", "b")
>> >
>> > and then the user subsequently does
>> >
>> > $ mv b ..
>> > $ cd ..
>> > $ rmdir DIR
>> >
>> > where DIR is the directory all of the above was performed inside, the
>> > rmdir will fail with ENOTEMPTY and *now* the user is confused.
>>
>> Yes, I understand your point.  That's really confusing.
>>
>> How can we resolve above cases without performing the silly renames
>> then?
>>
>
> I think you can't.  CIFS, without the posix extensions, is basically
> not posix and never will be.
> You can make it close but it will never be perfect and you will have
> to decide on how/what posix semantics you need to give up.

Agreed.

> What compounds the issue is that cifs on linux (without the posix
> extensions) will also always be a second class citizen.
> Genuine windows clients own this protocol and they expect their own
> non-posix semantics for their vfs, so you can't do anything that
> impacts windows clients, they are the first class citizen here.

Yep.

> The two main issues I see with the silly renames are the "-ENOTEMPTY"
> error when trying to delete the "empty" directory, but also
> that you can not hide them from windows clients. And what happens if
> the windows clients start accessing them?

Any potential concurrent opens on the silly-renamed file will fail with
STATUS_DELETE_PENDING.

> I think the only real solution is to say "if you need posix semantics
> then you need to use the posix extensions".

If they expect *full* POSIX semantics, yes, that makes sense.  But if
the protocol allows implementing part of it, why not doing it?