Re: [PATCH] cifs: Reuse file lease key in compound operations

[Tom Talpey <tom@talpey.com>]

On 12/6/2023 12:31 AM, Shyam Prasad N wrote:
> On Wed, Dec 6, 2023 at 1:02 AM Paulo Alcantara <pc@manguebit.com> wrote:
>>
>> meetakshisetiyaoss@gmail.com writes:
>>
>>> From: Meetakshi Setiya <msetiya@microsoft.com>
>>>
>>> Lock contention during unlink operation causes cifs lease break ack
>>> worker thread to block and delay sending lease break acks to server.
>>> This case occurs when multiple threads perform unlink, write and lease
>>> break acks on the same file. Thhis patch fixes the problem by reusing
>>> the existing lease keys for rename, unlink and set path size compound
>>> operations so that the client does not break its own lease.
>>>
>>> Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
>>> ---
>>>   fs/smb/client/cifsglob.h  |  6 ++---
>>>   fs/smb/client/cifsproto.h |  8 +++----
>>>   fs/smb/client/cifssmb.c   |  6 ++---
>>>   fs/smb/client/inode.c     | 12 +++++-----
>>>   fs/smb/client/smb2inode.c | 49 +++++++++++++++++++++++++--------------
>>>   fs/smb/client/smb2proto.h |  8 +++----
>>>   6 files changed, 51 insertions(+), 38 deletions(-)
>>
>> NAK.  This patch broke some xfstests.
>>
>> Consider this reproducer:
>>
>> $ cat repro.sh
>> #!/bin/sh
>>
>> umount /mnt/1 &>/dev/null
>> mount.cifs //srv/share /mnt/1 -o ...,vers=3.1.1
>> rm /mnt/1/* &>/dev/null
>> pushd /mnt/1 >/dev/null
>> touch foo
>> ln -v foo bar
>> rm -v bar
>> popd >/dev/null
>> umount /mnt/1 &>/dev/null
>> $ ./repro.sh
>> 'bar' => 'foo'
>> rm: cannot remove 'bar': Invalid argument
>>
>> This is what going on
>>
>> - client creates 'foo' with RHW lease granted.
>> - client creates hardlink file 'bar'.
>>
>> At this point, we have two positive dentries (foo & bar) which share
>> same inode.
>>
>> - The client then attempts to remove 'bar' by re-using lease key from
>> 'foo' through compound request CREATE(DELETE)+CLOSE, which fails with
>> STATUS_INVALID_PARAMETER.
> 
> That's interesting. I'm assuming that the STATUS_INVALID_PARAMETER is
> due to the server not recognizing the lease id for the file bar.
> I'm not sure that this is a client bug.
> If the server supports hard links, then should it not be aware that
> foo and bar are the same files? AFAIK, file lease is associated with a
> file, and not the dentry.

Lease keys are tied to the _filename_, including the full path. They
are basically guid's, and are used as lookup keys in the lease list,
from which the lease itself is the resulting value. The value is then
inspected for a match to the filename for which it was created.
They are not about the _handle_, which is what is apparently being
assumed here.

MS-SMB2 section 3.3.5.9.8 says this, which the server in question is
correctly enforcing [emphasis added]:

> The server MUST attempt to locate a Lease by performing a lookup in the LeaseTable.LeaseList
> using the LeaseKey in the SMB2_CREATE_REQUEST_LEASE as the lookup key. If a lease is found,
> Lease.FileDeleteOnClose is FALSE, and *Lease.Filename does not match the file name* for the
> incoming request, the request MUST be failed with STATUS_INVALID_PARAMETER

IOW, hard links are, from a protocol leasing standpoint, not the
same "file".

Tom.


> Meetakshi, can you please follow the repro steps provided by Paulo
> (against Windows file server) and check why the invalid parameter
> error is being returned?
>