Re: Different behavior of POSIX file locks depending on cache mode

[Kevin Ottens <kevin.ottens@enioka.com>]

Hello,

On Thursday 15 August 2024 22:47:21 CEST Steve French wrote:
> This (lock_test.cpp) was a nicely done test and turned out to be
> fairly easy fix (at least for the write path), and it does help your
> test (I will look at the read path next).   David also just reviewed
> it so will try to send up to mainline (and Cc: stable) fairly soon.
> See attached fix.
> 
> Thank you again for narrowing this down.   Your testing with other
> less common configs also helped fix two additional problems - SMB1
> bugs (these two important fixes went in mainline last month).   After
> I finish the patch for the read path I also will see if anything else
> missing in the SMB3.1.1 POSIX path (on client or server side - other
> than the known Samba server bug with QFSInfo).

Thanks a lot for all this. Very much appreciated!

Regards.
 
>     smb3: fix lock breakage for cached writes
> 
>     Mandatory locking is enforced for cached writes, which violates
>     default posix semantics, and also it is enforced inconsistently.
>     This apparently breaks recent versions of libreoffice, but can
>     also be demonstrated by opening a file twice from the same
>     client, locking it from handle one and writing to it from
>     handle two (which fails, returning EACCES).
> 
>     Since there was already a mount option "forcemandatorylock"
>     (which defaults to off), with this change only when the user
>     intentionally specifies "forcemandatorylock" on mount will we
>     break posix semantics on write to a locked range (ie we will
>     only fail the write in this case, if the user mounts with
>     "forcemandatorylock").
> 
>     Fixes: 85160e03a79e ("CIFS: Implement caching mechanism for
> mandatory brlocks")
> 
> On Sun, Jun 9, 2024 at 11:41 PM Andrew Bartlett <abartlet@samba.org> wrote:
> > (resend due spam rules on list)
> > 
> > Kia Ora Steve,
> > 
> > I'm working with Kevin on this, and I set up a clean environment with
> > the latest software to make sure this is all still an issue on current
> > software:
> > 
> > I was hoping to include the old SMB1 unix extensions in this test also,
> > but these seem unsupported in current kernels.  When did they go away?
> > 
> > Anyway, here is the data.  It certainly looks like an issue with the
> > SMB3 client, as only the client changes with the cache=none
> > 
> > Server is Samba 4.20.1 from Debian Sid.  Kernel is
> > Linux debian-sid-cifs-client 6.7.9-amd64 #1 SMP PREEMPT_DYNAMIC Debian
> > 6.7.9-2 (2024-03-13) x86_64 GNU/Linux
> > 
> > With SMB1 but not unix extensions (seems unsupported):
> > 
> > root@debian-sid-cifs-client:~# mount.cifs //192.168.122.234/testuser
> > mnt -o user=testuser,pass=pass,vers=1.0
> > root@debian-sid-cifs-client:~# cd mnt/
> > root@debian-sid-cifs-client:~/mnt# ../lock_test foo
> > Testing with foo
> > Got new file descriptor 3
> > Lock set: 1
> > Second file descriptor 4
> > Read from second fd: x count: 0
> > Third file descriptor 5
> > Wrote to third fd: 1
> > 
> > root@debian-sid-cifs-client:~# mount.cifs //192.168.122.234/testuser
> > mnt -o user=testuser,pass=penguin12#,vers=3.1.1,posix
> > root@debian-sid-cifs-client:~# cd mnt/
> > root@debian-sid-cifs-client:~/mnt# ../lock_test foo
> > Testing with foo
> > Got new file descriptor 3
> > Lock set: 1
> > Second file descriptor 4
> > Read from second fd: x count: -1
> > Third file descriptor 5
> > Wrote to third fd: -1
> > root@debian-sid-cifs-client:~# mount.cifs //192.168.122.234/testuser
> > mnt -o user=testuser,pass=penguin12#,vers=3.1.1,unix
> > 
> > root@debian-sid-cifs-client:~# mount.cifs //192.168.122.234/testuser
> > mnt -o user=testuser,pass=penguin12#,vers=3.1.1,unix,nobrl
> > root@debian-sid-cifs-client:~# cd mnt/
> > root@debian-sid-cifs-client:~/mnt# ../lock_test foo
> > Testing with foo
> > Got new file descriptor 3
> > Lock set: 1
> > Second file descriptor 4
> > Read from second fd: o count: 1
> > Third file descriptor 5
> > Wrote to third fd: 1
> > 
> > And with cache=none
> > 
> > root@debian-sid-cifs-client:~# mount.cifs //192.168.122.234/testuser
> > mnt -o user=testuser,pass=penguin12#,vers=3.1.1,posix,cache=none
> > root@debian-sid-cifs-client:~# cd mnt/
> > root@debian-sid-cifs-client:~/mnt# ../lock_test foo
> > Testing with foo
> > Got new file descriptor 3
> > Lock set: 1
> > Second file descriptor 4
> > Read from second fd: o count: 1
> > Third file descriptor 5
> > Wrote to third fd: 1
> > 
> > On Thu, 2024-05-23 at 11:12 -0500, Steve French wrote:
> > > What is the behavior with "nobrl" mount option? and what is the
> > > behavior when running with the POSIX extensions enabled (e.g. to
> > > current Samba or ksmbd adding "posix" to the mount options)
> > > 
> > > On Thu, May 23, 2024 at 11:08 AM Kevin Ottens <
> > > kevin.ottens@enioka.com
> > > 
> > > > wrote:
> > > > Hello,
> > > > 
> > > > I've been hunting down a bug exhibited by Libreoffice regarding
> > > > POSIX file
> > > > locks in conjunction with CIFS mounts. In short: just before
> > > > saving, it
> > > > reopens a file on which it already holds a file lock (via another
> > > > file
> > > > descriptor in the same process) in order to read from it to create
> > > > a backup
> > > > copy... but the read call fails.
> > > > 
> > > > I've been in discussion with Andrew Bartlett for a little while
> > > > regarding this
> > > > issue and, after exploring several venues, he advised me to send an
> > > > email to
> > > > this list in order to get more opinions about it.
> > > > 
> > > > The latest discovery we did was that the cache option on the
> > > > mountpoint seems
> > > > to impact the behavior of the POSIX file locks. I made a minimal
> > > > test
> > > > application (attached to this email) which basically does the
> > > > 
> > > > following:
> > > >  * open a file for read/write
> > > >  * set a POSIX write lock on the whole file
> > > >  * open the file a second time and try to read from it
> > > >  * open the file a third time and try to write to it
> > > > 
> > > > It assumes there is already some text in the file. Also, as it goes
> > > > it outputs
> > > > information about the calls.
> > > > 
> > > > The output I get is the following with cache=strict on the mount:
> > > > ---
> > > > Testing with /mnt/foo
> > > > Got new file descriptor 3
> > > > Lock set: 1
> > > > Second file descriptor 4
> > > > Read from second fd: x count: -1
> > > > Third file descriptor 5
> > > > Wrote to third fd: -1
> > > > ---
> > > > 
> > > > If I'm using cache=none:
> > > > ---
> > > > Testing with /mnt/foo
> > > > Got new file descriptor 3
> > > > Lock set: 1
> > > > Second file descriptor 4
> > > > Read from second fd: b count: 1
> > > > Third file descriptor 5
> > > > Wrote to third fd: 1
> > > > ---
> > > > 
> > > > That's the surprising behavior which prompted the email on this
> > > > list. Is it
> > > > somehow intended that the cache option would impact the semantic of
> > > > the file
> > > > locks? At least it caught me by surprise and I wouldn't expect such
> > > > a
> > > > difference in behavior.
> > > > 
> > > > Now, since the POSIX locks are process wide, I would have expected
> > > > to have the
> > > > output I'm getting for the "cache=none" case to be also the one I'm
> > > > getting
> > > > for the "cache=strict" case.
> > > > 
> > > > I'm looking forward to feedback on this one. I really wonder if we
> > > > missed
> > > > something obvious or if there is some kind of bug in the cifs
> > > > driver.
> > > > 
> > > > Regards.
> > > > --
> > > > Kévin Ottens
> > > > kevin.ottens@enioka.com
> > > > 
> > > > +33 7 57 08 95 13
> > 
> > --
> > 
> > Andrew Bartlett (he/him)       https://samba.org/~abartlet/
> > Samba Team Member (since 2001) https://samba.org
> > Samba Team Lead                https://catalyst.net.nz/services/samba
> > Catalyst.Net Ltd
> > 
> > Proudly developing Samba for Catalyst.Net Ltd - a Catalyst IT group
> > company
> > 
> > Samba Development and Support: https://catalyst.net.nz/services/samba
> > 
> > Catalyst IT - Expert Open Source Solutions
> > 
> > --
> > Andrew Bartlett (he/him)       https://samba.org/~abartlet/
> > Samba Team Member (since 2001) https://samba.org
> > Samba Team Lead                https://catalyst.net.nz/services/samba
> > Catalyst.Net Ltd
> > 
> > Proudly developing Samba for Catalyst.Net Ltd - a Catalyst IT group
> > company
> > 
> > Samba Development and Support: https://catalyst.net.nz/services/samba
> > 
> > Catalyst IT - Expert Open Source Solutions


-- 
Kévin Ottens
kevin.ottens@enioka.com
+33 7 57 08 95 13