Different behavior of POSIX file locks depending on cache mode

Different behavior of POSIX file locks depending on cache mode

[Kevin Ottens]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

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

[-- Attachment #2: lock_test.cpp --]
[-- Type: text/x-c++src, Size: 1037 bytes --]

#include <iostream>

#include <unistd.h>
#include <fcntl.h>

using namespace std;

int main(int argc, char **argv) {
    if (argc <= 1) {
        cout << "Please provide path to a file" << endl;
        return 1;
    }   

    const auto filepath = argv[1];
    cout << "Testing with " << filepath << endl;

    int fd = open(filepath, O_RDWR);
    cout << "Got new file descriptor " << fd << endl;


    struct flock lock;
    lock.l_type = F_WRLCK;
    lock.l_whence = SEEK_SET;
    lock.l_start = 0;
    lock.l_len = 0;
    int code = fcntl(fd, F_SETLK, &lock);
    cout << "Lock set: " << (code == 0) << endl;

    int fd2 = open(filepath, O_RDONLY);
    cout << "Second file descriptor " << fd2 << endl;

    char c = 'x';
    int count = read(fd2, &c, 1);
    cout << "Read from second fd: " << c << " count: " << count << endl;

    int fd3 = open(filepath, O_WRONLY|O_APPEND);
    cout << "Third file descriptor " << fd3 << endl;

    c = 'o';
    count = write(fd3, &c, 1);
    cout << "Wrote to third fd: " << count << endl;
}

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

[Steve French]

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



-- 
Thanks,

Steve

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

[Kevin Ottens]

Hello,

On Thursday 23 May 2024 18:12:27 CEST Steve French wrote:
> What is the behavior with "nobrl" mount option?

With nobrl no failure shows whatever the cache policy (so my lock_test gives 
always the output of the example with cache=none below).

> 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)

By current you mean 4.20.1?

So far I tested with 4.19.6 but couldn't get it to work. I get the following 
in dmesg:
CIFS: VFS: Server does not support mounting with posix SMB3.11 extensions

Despite having the following in my smb.conf:
server max protocol = SMB3_11
smb3 unix extensions = yes

Am I using something too old? I honestly don't know when the extensions were 
added.

Regards.
 
> 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


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

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

[Andrew Bartlett]

[-- Attachment #1: Type: text/plain, Size: 5925 bytes --]

(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

[-- Attachment #2: lock_test.cpp --]
[-- Type: text/x-c++src, Size: 1037 bytes --]

#include <iostream>

#include <unistd.h>
#include <fcntl.h>

using namespace std;

int main(int argc, char **argv) {
    if (argc <= 1) {
        cout << "Please provide path to a file" << endl;
        return 1;
    }   

    const auto filepath = argv[1];
    cout << "Testing with " << filepath << endl;

    int fd = open(filepath, O_RDWR);
    cout << "Got new file descriptor " << fd << endl;


    struct flock lock;
    lock.l_type = F_WRLCK;
    lock.l_whence = SEEK_SET;
    lock.l_start = 0;
    lock.l_len = 0;
    int code = fcntl(fd, F_SETLK, &lock);
    cout << "Lock set: " << (code == 0) << endl;

    int fd2 = open(filepath, O_RDONLY);
    cout << "Second file descriptor " << fd2 << endl;

    char c = 'x';
    int count = read(fd2, &c, 1);
    cout << "Read from second fd: " << c << " count: " << count << endl;

    int fd3 = open(filepath, O_WRONLY|O_APPEND);
    cout << "Third file descriptor " << fd3 << endl;

    c = 'o';
    count = write(fd3, &c, 1);
    cout << "Wrote to third fd: " << count << endl;
}

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

[Steve French]

Yes - the reproducer helps.  The bug is easy to reproduce.

I wanted to verify that the succeeding cases are the same that I see:
- works with "cache=none"
and
- works with "nobrl"
and
- works with "vers=1.0"

All other combinations fail ...

Should be straightforward to fix in cifs.ko.  Will look at a fix for
this later today.

Note that the problem with SMB3.1.1 POSIX extensions is a Samba bug -
a serious regression in the server (but trivial fix).  We are waiting
on someone to merge the oneline fix to the server (which we tested out
ok) from David.

diff --git a/source3/smbd/smb2_trans2.c b/source3/smbd/smb2_trans2.c
index b43f8f6330a..d03250a8912 100644
--- a/source3/smbd/smb2_trans2.c
+++ b/source3/smbd/smb2_trans2.c
@@ -1992,7 +1992,7 @@ static bool
fsinfo_unix_valid_level(connection_struct *conn,
                                    uint16_t info_level)
 {
        if (conn_using_smb2(conn->sconn) &&
-           fsp->posix_flags == FSP_POSIX_FLAGS_OPEN &&
+           (fsp->posix_flags & FSP_POSIX_FLAGS_OPEN) &&
            info_level == SMB2_FS_POSIX_INFORMATION_INTERNAL)
        {
                return true;





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



--
Thanks,

Steve

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

[Andrew Bartlett]

(again, resend without HTML)

On Mon, 2024-06-10 at 15:05 -0500, Steve French wrote:
> Yes - the reproducer helps.  The bug is easy to reproduce.
> 
> I wanted to verify that the succeeding cases are the same that I see:
> - works with "cache=none"
> and
> - works with "nobrl"
> and
> - works with "vers=1.0"
> 
> All other combinations fail ...
> 
> Should be straightforward to fix in cifs.ko.  Will look at a fix for
> this later today.

Awesome, thanks. 

> Note that the problem with SMB3.1.1 POSIX extensions is a Samba bug -
> a serious regression in the server (but trivial fix).  We are waiting
> on someone to merge the oneline fix to the server (which we tested
> out
> ok) from David.

Is there an MR for this?  I couldn't find it.

Andrew Bartlett

-- 

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

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

[Steve French]

On Mon, Jun 10, 2024 at 3:48 PM Andrew Bartlett <abartlet@samba.org> wrote:
>
> On Mon, 2024-06-10 at 15:05 -0500, Steve French wrote:
>
> Yes - the reproducer helps.  The bug is easy to reproduce.
>
>
> I wanted to verify that the succeeding cases are the same that I see:
>
> - works with "cache=none"
>
> and
>
> - works with "nobrl"
>
> and
>
> - works with "vers=1.0"
>
>
> All other combinations fail ...
>
>
> Should be straightforward to fix in cifs.ko.  Will look at a fix for
>
> this later today.
>
>
> Awesome, thanks.
>
>
> Note that the problem with SMB3.1.1 POSIX extensions is a Samba bug -
>
> a serious regression in the server (but trivial fix).  We are waiting
>
> on someone to merge the oneline fix to the server (which we tested out
>
> ok) from David.
>
>
> Is there an MR for this?  I couldn't find it.

I was puzzled why there wasn't a fix immediately applied since it was
tested (and could add my Reviewed-by if that helped), and obvious
server bug (regression), but I think David (who wrote the fix) was
busy with other tasks.  I would have done one but I am out of date
with the Samba merge process.

There are two other fairly simple problems (with basic SMB3.1.1
behavior), e.g. ctime not being updated in a few obvious cases (which
is not really a POSIX only issue, as it also affects default mounts
from Windows to Samba)  which we hit in SMB3.1.1 POSIX - that I would
love to see fixes for so we could restart our regular test automation
to Samba from Linux (cifs.ko SMB3.1.1 POSIX mounts)



-- 
Thanks,

Steve

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

[Andrew Bartlett]

On Mon, 2024-06-10 at 15:53 -0500, Steve French wrote:
> On Mon, Jun 10, 2024 at 3:48 PM Andrew Bartlett <
> abartlet@samba.org
> > wrote:
> > On Mon, 2024-06-10 at 15:05 -0500, Steve French wrote:
> > 
> > Yes - the reproducer helps.  The bug is easy to reproduce.
> > 
> > 
> > I wanted to verify that the succeeding cases are the same that I
> > see:
> > 
> > - works with "cache=none"
> > 
> > and
> > 
> > - works with "nobrl"
> > 
> > and
> > 
> > - works with "vers=1.0"
> > 
> > 
> > All other combinations fail ...
> > 
> > 
> > Should be straightforward to fix in cifs.ko.  Will look at a fix
> > for
> > 
> > this later today.
> > 
> > 
> > Awesome, thanks.
> > 
> > 
> > Note that the problem with SMB3.1.1 POSIX extensions is a Samba bug
> > -
> > 
> > a serious regression in the server (but trivial fix).  We are
> > waiting
> > 
> > on someone to merge the oneline fix to the server (which we tested
> > out
> > 
> > ok) from David.
> > 
> > 
> > Is there an MR for this?  I couldn't find it.
> 
> I was puzzled why there wasn't a fix immediately applied since it was
> tested (and could add my Reviewed-by if that helped), and obvious
> server bug (regression), but I think David (who wrote the fix) was
> busy with other tasks.  I would have done one but I am out of date
> with the Samba merge process.

https://wiki.samba.org/index.php/Contribute

> There are two other fairly simple problems (with basic SMB3.1.1
> behavior), e.g. ctime not being updated in a few obvious cases (which
> is not really a POSIX only issue, as it also affects default mounts
> from Windows to Samba)  which we hit in SMB3.1.1 POSIX - that I would
> love to see fixes for so we could restart our regular test automation
> to Samba from Linux (cifs.ko SMB3.1.1 POSIX mounts)

Very happy to help you get set up for small smbd patches, particularly
if it helps move this along.

Let me know your gitlab account once you create it, and I can bless it
for access as a Samba Team member (please also update the team contacts
file).

Andrew Bartlett

-- 
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

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

[Ralph Boehme]

[-- Attachment #1.1: Type: text/plain, Size: 1253 bytes --]

On 6/10/24 10:53 PM, Steve French wrote:
> On Mon, Jun 10, 2024 at 3:48 PM Andrew Bartlett <abartlet@samba.org> wrote:
>>
>> On Mon, 2024-06-10 at 15:05 -0500, Steve French wrote:
>>
>> Yes - the reproducer helps.  The bug is easy to reproduce.
>>
>>
>> I wanted to verify that the succeeding cases are the same that I see:
>>
>> - works with "cache=none"
>>
>> and
>>
>> - works with "nobrl"
>>
>> and
>>
>> - works with "vers=1.0"
>>
>>
>> All other combinations fail ...
>>
>>
>> Should be straightforward to fix in cifs.ko.  Will look at a fix for
>>
>> this later today.
>>
>>
>> Awesome, thanks.
>>
>>
>> Note that the problem with SMB3.1.1 POSIX extensions is a Samba bug -
>>
>> a serious regression in the server (but trivial fix).  We are waiting
>>
>> on someone to merge the oneline fix to the server (which we tested out
>>
>> ok) from David.

no, Ralph.

>>
>>
>> Is there an MR for this?  I couldn't find it.
> 
> I was puzzled why there wasn't a fix immediately applied since it was
> tested (and could add my Reviewed-by if that helped),

as written before

fix + test = MR + review = master

The test is still missing and I was on vacation and am swamped with work.

Thanks!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Steve French]

On Mon, Jun 10, 2024 at 4:19 PM Ralph Boehme <slow@samba.org> wrote:
<snip>
> >> a serious regression in the server (but trivial fix).  We are waiting
> >>
> >> on someone to merge the oneline fix to the server (which we tested out
> >>
> >> ok) from David.
>
> no, Ralph.

Oops... Sorry about the typo ...

> fix + test = MR + review = master
>
> The test is still missing and I was on vacation and am swamped with work.

All of the standard ("fstest" aka "xfstests") automated tests fail, so
trivial to test/reproduce.  Does anyone have a pointer to the
smbtorture tests for SMB3.1.1 POSIX? It only needs a single query fs
info to fail so this looks like only a few line addition to the
existing SMB3.1.1 POSIX tests.   I do badly want to re-enable our
automated testing to Samba with SMB3.1.1 POSIX (the "buildbot") but
obviously this blocks every test.   We do test on every pull request
upstream to Samba (and also to Samba with vfs_btrfs for those features
like "reflink" etc are missing without vfs_btrfs installed)

-- 
Thanks,

Steve

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

[Steve French]

On Mon, Jun 10, 2024 at 5:49 PM Steve French <smfrench@gmail.com> wrote:
>
> On Mon, Jun 10, 2024 at 4:19 PM Ralph Boehme <slow@samba.org> wrote:
> <snip>
> > >> a serious regression in the server (but trivial fix).  We are waiting
> > >>
> > >> on someone to merge the oneline fix to the server (which we tested out
> > >>
> > >> ok) from David.
> >
> > no, Ralph.
>
> Oops... Sorry about the typo ...
>
> > fix + test = MR + review = master
> >
> > The test is still missing and I was on vacation and am swamped with work.
>
> All of the standard ("fstest" aka "xfstests") automated tests fail, so
> trivial to test/reproduce.  Does anyone have a pointer to the
> smbtorture tests for SMB3.1.1 POSIX? It only needs a single query fs
> info to fail so this looks like only a few line addition to the
> existing SMB3.1.1 POSIX tests.   I do badly want to re-enable our
> automated testing to Samba with SMB3.1.1 POSIX (the "buildbot") but
> obviously this blocks every test.   We do test on every pull request
> upstream to Samba (and also to Samba with vfs_btrfs for those features

Sorry about another typo:
We do test (cifs.ko) -- to Samba server -- on every pull request


-- 
Thanks,

Steve

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

[Steve French]

This was interesting to dig through (and netfs caching changes made it
a little harder to trace the original code) but it looks fixable. See
cifs_find_fid_lock_conflict() in fs/smb/client/file.c.   It does not
look new though - so let me know if you noticed that the behavior in
earlier kernels was different for the default case (smb3.1.1 mounts
with caching enabled).

The problem seems to be that locking is enforced only in some write
paths, but these places where we do write vs. byte range locking
checks (although at first glance may seem logical) obviously do not
follow POSIX semantics which would allow a write to a locked range
(even if POSIX behavior is counter-intuitive and different from
Windows semantics).  Two obvious things to fix that I see so far:

1) It was harder than expected to trace since looks like we don't have
good dynamic (or static for that matter) tracepoints for the write and
write error paths (although netfs fortunately has a few) - so
obviously should add a few tracepoints to make it easier to narrow
this kind of thing down in the future
2) We need to make changes to how we check lock conflicts.  See
cifs_writev() and its call to cifs_find_lock_conflict(). It looks like
this is the original commit that may have caused the problem:
commit 85160e03a79e0d7f9082e61f6a784abc6f402701
Author: Pavel Shilovsky <piastry@etersoft.ru>
Date:   Sat Oct 22 15:33:29 2011 +0400

    CIFS: Implement caching mechanism for mandatory brlocks

    If we have an oplock and negotiate mandatory locking style we handle
    all brlock requests on the client.


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



-- 
Thanks,

Steve

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

[Andrew Bartlett]

Thanks so much Steve.  

Kevin can give more info, but no, my understanding is that this was
"always" problematic, the reason it came up now is LibreOffice changed
how it handled file saves, rather than a kernel change.

Even if the fix is to be in the direction that 'breaks' LibreOffice,
that is OK, I have given Kevin a suggestion on using locks ranges
towards the end of the file as a way to regain the desired 'advisory'
semantics, but we wanted to be working on solid ground with consistent,
reliable semantics that don't depend on cache modes. 

Thanks,

Andrew Bartlett

On Sun, 2024-06-23 at 23:54 -0500, Steve French wrote:
> This was interesting to dig through (and netfs caching changes made
> it
> a little harder to trace the original code) but it looks fixable. See
> cifs_find_fid_lock_conflict() in fs/smb/client/file.c.   It does not
> look new though - so let me know if you noticed that the behavior in
> earlier kernels was different for the default case (smb3.1.1 mounts
> with caching enabled).
> 
> The problem seems to be that locking is enforced only in some write
> paths, but these places where we do write vs. byte range locking
> checks (although at first glance may seem logical) obviously do not
> follow POSIX semantics which would allow a write to a locked range
> (even if POSIX behavior is counter-intuitive and different from
> Windows semantics).  Two obvious things to fix that I see so far:
> 
> 1) It was harder than expected to trace since looks like we don't
> have
> good dynamic (or static for that matter) tracepoints for the write
> and
> write error paths (although netfs fortunately has a few) - so
> obviously should add a few tracepoints to make it easier to narrow
> this kind of thing down in the future
> 2) We need to make changes to how we check lock conflicts.  See
> cifs_writev() and its call to cifs_find_lock_conflict(). It looks
> like
> this is the original commit that may have caused the problem:
> commit 85160e03a79e0d7f9082e61f6a784abc6f402701
> Author: Pavel Shilovsky <
> piastry@etersoft.ru
> >
> Date:   Sat Oct 22 15:33:29 2011 +0400
> 
>     CIFS: Implement caching mechanism for mandatory brlocks
> 
>     If we have an oplock and negotiate mandatory locking style we
> handle
>     all brlock requests on the client.
> 
> 
> 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
> 
> 
> 
-- 
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

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

[Kevin Ottens]

Hello,

On Tuesday 25 June 2024 08:00:11 CEST Andrew Bartlett wrote:
> Thanks so much Steve.

Thanks indeed!
 
> Kevin can give more info, but no, my understanding is that this was
> "always" problematic, the reason it came up now is LibreOffice changed
> how it handled file saves, rather than a kernel change.

I confirm. It's been here for a long time AFAIK, but Libreoffice recently 
enabled some code path by default (it was previously disabled by default).

> Even if the fix is to be in the direction that 'breaks' LibreOffice,
> that is OK, I have given Kevin a suggestion on using locks ranges
> towards the end of the file as a way to regain the desired 'advisory'
> semantics, but we wanted to be working on solid ground with consistent,
> reliable semantics that don't depend on cache modes.

Yes, the consistency is important. Once we have a solid and reliable behavior 
we can always get things adjusted higher in the stack.

Regards.


> On Sun, 2024-06-23 at 23:54 -0500, Steve French wrote:
> > This was interesting to dig through (and netfs caching changes made
> > it
> > a little harder to trace the original code) but it looks fixable. See
> > cifs_find_fid_lock_conflict() in fs/smb/client/file.c.   It does not
> > look new though - so let me know if you noticed that the behavior in
> > earlier kernels was different for the default case (smb3.1.1 mounts
> > with caching enabled).
> > 
> > The problem seems to be that locking is enforced only in some write
> > paths, but these places where we do write vs. byte range locking
> > checks (although at first glance may seem logical) obviously do not
> > follow POSIX semantics which would allow a write to a locked range
> > (even if POSIX behavior is counter-intuitive and different from
> > Windows semantics).  Two obvious things to fix that I see so far:
> > 
> > 1) It was harder than expected to trace since looks like we don't
> > have
> > good dynamic (or static for that matter) tracepoints for the write
> > and
> > write error paths (although netfs fortunately has a few) - so
> > obviously should add a few tracepoints to make it easier to narrow
> > this kind of thing down in the future
> > 2) We need to make changes to how we check lock conflicts.  See
> > cifs_writev() and its call to cifs_find_lock_conflict(). It looks
> > like
> > this is the original commit that may have caused the problem:
> > commit 85160e03a79e0d7f9082e61f6a784abc6f402701
> > Author: Pavel Shilovsky <
> > piastry@etersoft.ru
> > 
> > Date:   Sat Oct 22 15:33:29 2011 +0400
> > 
> >     CIFS: Implement caching mechanism for mandatory brlocks
> >     
> >     If we have an oplock and negotiate mandatory locking style we
> > 
> > handle
> > 
> >     all brlock requests on the client.
> > 
> > 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

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

[Kevin Ottens]

Hello,

Is there anyone working on a patch for this since you narrowed it down a bit? 
We can probably test the changes if that's the case.

Regards.

On Tuesday 25 June 2024 09:41:48 CEST Kevin Ottens wrote:
> Hello,
> 
> On Tuesday 25 June 2024 08:00:11 CEST Andrew Bartlett wrote:
> > Thanks so much Steve.
> 
> Thanks indeed!
> 
> > Kevin can give more info, but no, my understanding is that this was
> > "always" problematic, the reason it came up now is LibreOffice changed
> > how it handled file saves, rather than a kernel change.
> 
> I confirm. It's been here for a long time AFAIK, but Libreoffice recently
> enabled some code path by default (it was previously disabled by default).
> 
> > Even if the fix is to be in the direction that 'breaks' LibreOffice,
> > that is OK, I have given Kevin a suggestion on using locks ranges
> > towards the end of the file as a way to regain the desired 'advisory'
> > semantics, but we wanted to be working on solid ground with consistent,
> > reliable semantics that don't depend on cache modes.
> 
> Yes, the consistency is important. Once we have a solid and reliable
> behavior we can always get things adjusted higher in the stack.
> 
> Regards.
> 
> > On Sun, 2024-06-23 at 23:54 -0500, Steve French wrote:
> > > This was interesting to dig through (and netfs caching changes made
> > > it
> > > a little harder to trace the original code) but it looks fixable. See
> > > cifs_find_fid_lock_conflict() in fs/smb/client/file.c.   It does not
> > > look new though - so let me know if you noticed that the behavior in
> > > earlier kernels was different for the default case (smb3.1.1 mounts
> > > with caching enabled).
> > > 
> > > The problem seems to be that locking is enforced only in some write
> > > paths, but these places where we do write vs. byte range locking
> > > checks (although at first glance may seem logical) obviously do not
> > > follow POSIX semantics which would allow a write to a locked range
> > > (even if POSIX behavior is counter-intuitive and different from
> > > Windows semantics).  Two obvious things to fix that I see so far:
> > > 
> > > 1) It was harder than expected to trace since looks like we don't
> > > have
> > > good dynamic (or static for that matter) tracepoints for the write
> > > and
> > > write error paths (although netfs fortunately has a few) - so
> > > obviously should add a few tracepoints to make it easier to narrow
> > > this kind of thing down in the future
> > > 2) We need to make changes to how we check lock conflicts.  See
> > > cifs_writev() and its call to cifs_find_lock_conflict(). It looks
> > > like
> > > this is the original commit that may have caused the problem:
> > > commit 85160e03a79e0d7f9082e61f6a784abc6f402701
> > > Author: Pavel Shilovsky <
> > > piastry@etersoft.ru
> > > 
> > > Date:   Sat Oct 22 15:33:29 2011 +0400
> > > 
> > >     CIFS: Implement caching mechanism for mandatory brlocks
> > >     
> > >     If we have an oplock and negotiate mandatory locking style we
> > > 
> > > handle
> > > 
> > >     all brlock requests on the client.
> > > 
> > > 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

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

[Andrew Bartlett]

Kia Ora Steve,

Just wanting to know if there is anything more Kevin or I can usefully
do to nudge this along, or do you have this in hand?

It would be very good to get this consistent.

Thanks,

Andrew Bartlett

On Thu, 2024-06-27 at 17:04 +0200, Kevin Ottens wrote:
> Hello,
> 
> Is there anyone working on a patch for this since you narrowed it
> down a bit? 
> We can probably test the changes if that's the case.
> 
> Regards.
> 
> On Tuesday 25 June 2024 09:41:48 CEST Kevin Ottens wrote:
> > Hello,
> > 
> > On Tuesday 25 June 2024 08:00:11 CEST Andrew Bartlett wrote:
> > > Thanks so much Steve.
> > 
> > Thanks indeed!
> > 
> > > Kevin can give more info, but no, my understanding is that this
> > > was
> > > "always" problematic, the reason it came up now is LibreOffice
> > > changed
> > > how it handled file saves, rather than a kernel change.
> > 
> > I confirm. It's been here for a long time AFAIK, but Libreoffice
> > recently
> > enabled some code path by default (it was previously disabled by
> > default).
> > 
> > > Even if the fix is to be in the direction that 'breaks'
> > > LibreOffice,
> > > that is OK, I have given Kevin a suggestion on using locks ranges
> > > towards the end of the file as a way to regain the desired
> > > 'advisory'
> > > semantics, but we wanted to be working on solid ground with
> > > consistent,
> > > reliable semantics that don't depend on cache modes.
> > 
> > Yes, the consistency is important. Once we have a solid and
> > reliable
> > behavior we can always get things adjusted higher in the stack.
> > 
> > Regards.
> > 
> > > On Sun, 2024-06-23 at 23:54 -0500, Steve French wrote:
> > > > This was interesting to dig through (and netfs caching changes
> > > > made
> > > > it
> > > > a little harder to trace the original code) but it looks
> > > > fixable. See
> > > > cifs_find_fid_lock_conflict() in fs/smb/client/file.c.   It
> > > > does not
> > > > look new though - so let me know if you noticed that the
> > > > behavior in
> > > > earlier kernels was different for the default case (smb3.1.1
> > > > mounts
> > > > with caching enabled).
> > > > 
> > > > The problem seems to be that locking is enforced only in some
> > > > write
> > > > paths, but these places where we do write vs. byte range
> > > > locking
> > > > checks (although at first glance may seem logical) obviously do
> > > > not
> > > > follow POSIX semantics which would allow a write to a locked
> > > > range
> > > > (even if POSIX behavior is counter-intuitive and different from
> > > > Windows semantics).  Two obvious things to fix that I see so
> > > > far:
> > > > 
> > > > 1) It was harder than expected to trace since looks like we
> > > > don't
> > > > have
> > > > good dynamic (or static for that matter) tracepoints for the
> > > > write
> > > > and
> > > > write error paths (although netfs fortunately has a few) - so
> > > > obviously should add a few tracepoints to make it easier to
> > > > narrow
> > > > this kind of thing down in the future
> > > > 2) We need to make changes to how we check lock conflicts.  See
> > > > cifs_writev() and its call to cifs_find_lock_conflict(). It
> > > > looks
> > > > like
> > > > this is the original commit that may have caused the problem:
> > > > commit 85160e03a79e0d7f9082e61f6a784abc6f402701
> > > > Author: Pavel Shilovsky <
> > > > piastry@etersoft.ru
> > > > 
> > > > 
> > > > Date:   Sat Oct 22 15:33:29 2011 +0400
> > > > 
> > > >     CIFS: Implement caching mechanism for mandatory brlocks
> > > >     
> > > >     If we have an oplock and negotiate mandatory locking style
> > > > we
> > > > 
> > > > handle
> > > > 
> > > >     all brlock requests on the client.
> > > > 
> > > > 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
> 
> 
> 
-- 
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

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

[Steve French]

I haven't had a chance to look at it again this week (trying to deal
with the 6.10-rc regression in netfs/folios changes causing the
"add_credits" error) - but do plan to look at it later this week

On Wed, Jul 10, 2024 at 4:28 AM Andrew Bartlett <abartlet@samba.org> wrote:
>
> Kia Ora Steve,
>
> Just wanting to know if there is anything more Kevin or I can usefully
> do to nudge this along, or do you have this in hand?
>
> It would be very good to get this consistent.
>
> Thanks,
>
> Andrew Bartlett
>
> On Thu, 2024-06-27 at 17:04 +0200, Kevin Ottens wrote:
> > Hello,
> >
> > Is there anyone working on a patch for this since you narrowed it
> > down a bit?
> > We can probably test the changes if that's the case.
> >
> > Regards.
> >
> > On Tuesday 25 June 2024 09:41:48 CEST Kevin Ottens wrote:
> > > Hello,
> > >
> > > On Tuesday 25 June 2024 08:00:11 CEST Andrew Bartlett wrote:
> > > > Thanks so much Steve.
> > >
> > > Thanks indeed!
> > >
> > > > Kevin can give more info, but no, my understanding is that this
> > > > was
> > > > "always" problematic, the reason it came up now is LibreOffice
> > > > changed
> > > > how it handled file saves, rather than a kernel change.
> > >
> > > I confirm. It's been here for a long time AFAIK, but Libreoffice
> > > recently
> > > enabled some code path by default (it was previously disabled by
> > > default).
> > >
> > > > Even if the fix is to be in the direction that 'breaks'
> > > > LibreOffice,
> > > > that is OK, I have given Kevin a suggestion on using locks ranges
> > > > towards the end of the file as a way to regain the desired
> > > > 'advisory'
> > > > semantics, but we wanted to be working on solid ground with
> > > > consistent,
> > > > reliable semantics that don't depend on cache modes.
> > >
> > > Yes, the consistency is important. Once we have a solid and
> > > reliable
> > > behavior we can always get things adjusted higher in the stack.
> > >
> > > Regards.
> > >
> > > > On Sun, 2024-06-23 at 23:54 -0500, Steve French wrote:
> > > > > This was interesting to dig through (and netfs caching changes
> > > > > made
> > > > > it
> > > > > a little harder to trace the original code) but it looks
> > > > > fixable. See
> > > > > cifs_find_fid_lock_conflict() in fs/smb/client/file.c.   It
> > > > > does not
> > > > > look new though - so let me know if you noticed that the
> > > > > behavior in
> > > > > earlier kernels was different for the default case (smb3.1.1
> > > > > mounts
> > > > > with caching enabled).
> > > > >
> > > > > The problem seems to be that locking is enforced only in some
> > > > > write
> > > > > paths, but these places where we do write vs. byte range
> > > > > locking
> > > > > checks (although at first glance may seem logical) obviously do
> > > > > not
> > > > > follow POSIX semantics which would allow a write to a locked
> > > > > range
> > > > > (even if POSIX behavior is counter-intuitive and different from
> > > > > Windows semantics).  Two obvious things to fix that I see so
> > > > > far:
> > > > >
> > > > > 1) It was harder than expected to trace since looks like we
> > > > > don't
> > > > > have
> > > > > good dynamic (or static for that matter) tracepoints for the
> > > > > write
> > > > > and
> > > > > write error paths (although netfs fortunately has a few) - so
> > > > > obviously should add a few tracepoints to make it easier to
> > > > > narrow
> > > > > this kind of thing down in the future
> > > > > 2) We need to make changes to how we check lock conflicts.  See
> > > > > cifs_writev() and its call to cifs_find_lock_conflict(). It
> > > > > looks
> > > > > like
> > > > > this is the original commit that may have caused the problem:
> > > > > commit 85160e03a79e0d7f9082e61f6a784abc6f402701
> > > > > Author: Pavel Shilovsky <
> > > > > piastry@etersoft.ru
> > > > >
> > > > >
> > > > > Date:   Sat Oct 22 15:33:29 2011 +0400
> > > > >
> > > > >     CIFS: Implement caching mechanism for mandatory brlocks
> > > > >
> > > > >     If we have an oplock and negotiate mandatory locking style
> > > > > we
> > > > >
> > > > > handle
> > > > >
> > > > >     all brlock requests on the client.
> > > > >
> > > > > 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
> >
> >
> >
> --
> 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
>


-- 
Thanks,

Steve

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

[Andrew Bartlett]

Thanks so much for the update, we do really appreciate it. 

Please do CC us when it starts moving as I'm not on the linux-cifs list
and Kevin may also not be.

Andrew Bartlett

On Wed, 2024-07-10 at 21:03 -0500, Steve French wrote:
> I haven't had a chance to look at it again this week (trying to deal
> with the 6.10-rc regression in netfs/folios changes causing the
> "add_credits" error) - but do plan to look at it later this week
> 
> On Wed, Jul 10, 2024 at 4:28 AM Andrew Bartlett <
> abartlet@samba.org
> > wrote:
> > Kia Ora Steve,
> > 
> > Just wanting to know if there is anything more Kevin or I can
> > usefully
> > do to nudge this along, or do you have this in hand?
> > 
> > It would be very good to get this consistent.
> > 
> > Thanks,
> > 
> > Andrew Bartlett
> > 
> > On Thu, 2024-06-27 at 17:04 +0200, Kevin Ottens wrote:
> > > Hello,
> > > 
> > > Is there anyone working on a patch for this since you narrowed it
> > > down a bit?
> > > We can probably test the changes if that's the case.
> > > 
> > > Regards.
> > > 
> > > On Tuesday 25 June 2024 09:41:48 CEST Kevin Ottens wrote:
> > > > Hello,
> > > > 
> > > > On Tuesday 25 June 2024 08:00:11 CEST Andrew Bartlett wrote:
> > > > > Thanks so much Steve.
> > > > 
> > > > Thanks indeed!
> > > > 
> > > > > Kevin can give more info, but no, my understanding is that
> > > > > this
> > > > > was
> > > > > "always" problematic, the reason it came up now is
> > > > > LibreOffice
> > > > > changed
> > > > > how it handled file saves, rather than a kernel change.
> > > > 
> > > > I confirm. It's been here for a long time AFAIK, but
> > > > Libreoffice
> > > > recently
> > > > enabled some code path by default (it was previously disabled
> > > > by
> > > > default).
> > > > 
> > > > > Even if the fix is to be in the direction that 'breaks'
> > > > > LibreOffice,
> > > > > that is OK, I have given Kevin a suggestion on using locks
> > > > > ranges
> > > > > towards the end of the file as a way to regain the desired
> > > > > 'advisory'
> > > > > semantics, but we wanted to be working on solid ground with
> > > > > consistent,
> > > > > reliable semantics that don't depend on cache modes.
> > > > 
> > > > Yes, the consistency is important. Once we have a solid and
> > > > reliable
> > > > behavior we can always get things adjusted higher in the stack.
> > > > 
> > > > Regards.
> > > > 
> > > > > On Sun, 2024-06-23 at 23:54 -0500, Steve French wrote:
> > > > > > This was interesting to dig through (and netfs caching
> > > > > > changes
> > > > > > made
> > > > > > it
> > > > > > a little harder to trace the original code) but it looks
> > > > > > fixable. See
> > > > > > cifs_find_fid_lock_conflict() in fs/smb/client/file.c.   It
> > > > > > does not
> > > > > > look new though - so let me know if you noticed that the
> > > > > > behavior in
> > > > > > earlier kernels was different for the default case
> > > > > > (smb3.1.1
> > > > > > mounts
> > > > > > with caching enabled).
> > > > > > 
> > > > > > The problem seems to be that locking is enforced only in
> > > > > > some
> > > > > > write
> > > > > > paths, but these places where we do write vs. byte range
> > > > > > locking
> > > > > > checks (although at first glance may seem logical)
> > > > > > obviously do
> > > > > > not
> > > > > > follow POSIX semantics which would allow a write to a
> > > > > > locked
> > > > > > range
> > > > > > (even if POSIX behavior is counter-intuitive and different
> > > > > > from
> > > > > > Windows semantics).  Two obvious things to fix that I see
> > > > > > so
> > > > > > far:
> > > > > > 
> > > > > > 1) It was harder than expected to trace since looks like we
> > > > > > don't
> > > > > > have
> > > > > > good dynamic (or static for that matter) tracepoints for
> > > > > > the
> > > > > > write
> > > > > > and
> > > > > > write error paths (although netfs fortunately has a few) -
> > > > > > so
> > > > > > obviously should add a few tracepoints to make it easier to
> > > > > > narrow
> > > > > > this kind of thing down in the future
> > > > > > 2) We need to make changes to how we check lock
> > > > > > conflicts.  See
> > > > > > cifs_writev() and its call to cifs_find_lock_conflict(). It
> > > > > > looks
> > > > > > like
> > > > > > this is the original commit that may have caused the
> > > > > > problem:
> > > > > > commit 85160e03a79e0d7f9082e61f6a784abc6f402701
> > > > > > Author: Pavel Shilovsky <
> > > > > > piastry@etersoft.ru
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Date:   Sat Oct 22 15:33:29 2011 +0400
> > > > > > 
> > > > > >     CIFS: Implement caching mechanism for mandatory brlocks
> > > > > > 
> > > > > >     If we have an oplock and negotiate mandatory locking
> > > > > > style
> > > > > > we
> > > > > > 
> > > > > > handle
> > > > > > 
> > > > > >     all brlock requests on the client.
> > > > > > 
> > > > > > 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
> > > 
> > > 
> > > 
> > 
> > --
> > 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

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

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 8138 bytes --]

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).

    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



-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb3-fix-lock-breakage-for-cached-writes.patch --]
[-- Type: text/x-patch, Size: 2502 bytes --]

From 26a3a5ed783b0a5c42261b608e957229f00c1ced Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Thu, 15 Aug 2024 14:03:43 -0500
Subject: [PATCH] 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")
Cc: stable@vger.kernel.org
Cc: Pavel Shilovsky <piastryyy@gmail.com>
Reported-by: abartlet@samba.org
Reported-by: Kevin Ottens <kevin.ottens@enioka.com>
Reviewed-by: David Howells <dhowells@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/smb/client/file.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index 45459af5044d..0438c999defc 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -2753,6 +2753,7 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
 	struct inode *inode = file->f_mapping->host;
 	struct cifsInodeInfo *cinode = CIFS_I(inode);
 	struct TCP_Server_Info *server = tlink_tcon(cfile->tlink)->ses->server;
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
 	ssize_t rc;
 
 	rc = netfs_start_io_write(inode);
@@ -2769,12 +2770,15 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
 	if (rc <= 0)
 		goto out;
 
-	if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from),
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOPOSIXBRL) &&
+	    (cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from),
 				     server->vals->exclusive_lock_type, 0,
-				     NULL, CIFS_WRITE_OP))
-		rc = netfs_buffered_write_iter_locked(iocb, from, NULL);
-	else
+				     NULL, CIFS_WRITE_OP))) {
 		rc = -EACCES;
+		goto out;
+	} else
+		rc = netfs_buffered_write_iter_locked(iocb, from, NULL);
+
 out:
 	up_read(&cinode->lock_sem);
 	netfs_end_io_write(inode);
-- 
2.43.0

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

[Kevin Ottens]

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

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

[Steve French]

the fixes for both the read and write lock problems you pointed out
are in mainline, and I see they have made it to some stable releases
(6.10) but will likely need to be rebased to be backported to some of
the older stable kernels (hopefully some of the distros pick these up
soon as well)

On Thu, Aug 22, 2024 at 12:06 PM Kevin Ottens <kevin.ottens@enioka.com> wrote:
>
> 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
>
>


--
Thanks,

Steve