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 06:28, Paulo Alcantara <pc@manguebit.org> wrote:
>>
>> ronnie sahlberg <ronniesahlberg@gmail.com> writes:
>>
>> > On Thu, 4 Sept 2025 at 05:55, ronnie sahlberg <ronniesahlberg@gmail.com> wrote:
>> >>
>> >> 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.
>> >>
>> >> 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.
>> >>
>> >> 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?
>> >>
>> >> I think the only real solution is to say "if you need posix semantics
>> >> then you need to use the posix extensions".
>> >
>> > Another potential issue is about QUERY_INFO.
>> > Many of the information classes in FSCC return the filename as as part
>> > of the data.
>> > What filename should be returned for a silly renamed file?
>> >
>> > Maybe it doesn't matter  but maybe there is some obscure windows
>> > application out there that do care and gets surprised.
>>
>> I don't think that matters as file deletion is pending on the server and
>> QUERY_INFO won't work.
>>
>> What am I missing?
>
> You missed nothing. I didn't think it through.
>
> But another potential issue is the possibility of a DOS from malicious
> windows clients.
> If the files are "hidden" from linux clients.
> How would a linux client recover if a maliciious windows client just
> creates an ordinary file with a filename that matches the "silly
> rename prefix".
> The file can not be seen or deleted from linux clients and thus the
> directory becomes undeletable too?

Yes, that's a good point.  Thanks!

One way to solve that would be doing something similar as AFS currently
does, which is finding the next non-existing silly filename that could
used by calling lookup_noperm() and then incrementing @sillycounter
until it gets the first negative dentry.