[2/2] smb: client: retry compound request without reusing lease

Commit Message

Meetakshi Setiya Dec. 29, 2023, 2:35 p.m. UTC

From: Meetakshi Setiya <[e-mail]>

There is a shortcoming in the current implementation of the file
lease mechanism exposed when the lease keys were attempted to be reused
for unlink, rename and set_path_size operations for a client. As per
MS-SMB2, lease keys are associated with the file name. Linux cifs client
maintains lease keys with the inode. If the file has any hardlinks,
it is possible that the lease for a file be wrongly reused for an
operation on the hardlink or vice versa. In these cases, the mentioned
compound operations fail with STATUS_INVALID_PARAMETER.
This patch adds a fallback to the old mechanism of not sending any
lease with these compound operations if the request with lease key fails
with STATUS_INVALID_PARAMETER. Resending the same request without lease
key should not hurt any functionality, but might impact performance
especially in cases where the error is not because of the usage of wrong
lease key and we might end up doing an extra roundtrip.

Signed-off-by: Meetakshi Setiya <[e-mail]>
---
 fs/smb/client/smb2inode.c | 40 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

Comments

Paulo Alcantara Dec. 29, 2023, 3:43 p.m. UTC | #1

[e-mail] writes:

> From: Meetakshi Setiya <[e-mail]>
>
> There is a shortcoming in the current implementation of the file
> lease mechanism exposed when the lease keys were attempted to be reused
> for unlink, rename and set_path_size operations for a client. As per
> MS-SMB2, lease keys are associated with the file name. Linux cifs client
> maintains lease keys with the inode. If the file has any hardlinks,
> it is possible that the lease for a file be wrongly reused for an
> operation on the hardlink or vice versa. In these cases, the mentioned
> compound operations fail with STATUS_INVALID_PARAMETER.
> This patch adds a fallback to the old mechanism of not sending any
> lease with these compound operations if the request with lease key fails
> with STATUS_INVALID_PARAMETER. Resending the same request without lease
> key should not hurt any functionality, but might impact performance
> especially in cases where the error is not because of the usage of wrong
> lease key and we might end up doing an extra roundtrip.

What's the problem with checking ->i_nlink to decide whether reusing
lease key?

Meetakshi Setiya Jan. 3, 2024, 4:35 a.m. UTC | #2

On Fri, Dec 29, 2023 at 9:13 PM Paulo Alcantara <[e-mail]> wrote:
>
> [e-mail] writes:
>
> > From: Meetakshi Setiya <[e-mail]>
> >
> > There is a shortcoming in the current implementation of the file
> > lease mechanism exposed when the lease keys were attempted to be reused
> > for unlink, rename and set_path_size operations for a client. As per
> > MS-SMB2, lease keys are associated with the file name. Linux cifs client
> > maintains lease keys with the inode. If the file has any hardlinks,
> > it is possible that the lease for a file be wrongly reused for an
> > operation on the hardlink or vice versa. In these cases, the mentioned
> > compound operations fail with STATUS_INVALID_PARAMETER.
> > This patch adds a fallback to the old mechanism of not sending any
> > lease with these compound operations if the request with lease key fails
> > with STATUS_INVALID_PARAMETER. Resending the same request without lease
> > key should not hurt any functionality, but might impact performance
> > especially in cases where the error is not because of the usage of wrong
> > lease key and we might end up doing an extra roundtrip.
>
> What's the problem with checking ->i_nlink to decide whether reusing
> lease key?

As per the discussion with Tom on the previous version of the changes, I
conferred with Shyam and Steve about possible workarounds and this seemed like a
choice which did the job without much perf drawbacks and code changes. One
highlighted difference between the two could be that in the previous
version, lease
would not be reused for any file with hardlinks at all, even though the inode
may hold the correct lease for that particular file. The current changes
would take care of this by sending the lease at least once, irrespective of the
number of hardlinks.

Thanks
Meetakshi

Paulo Alcantara Jan. 3, 2024, 2:37 p.m. UTC | #3

Meetakshi Setiya <[e-mail]> writes:

> As per the discussion with Tom on the previous version of the changes, I
> conferred with Shyam and Steve about possible workarounds and this seemed like a
> choice which did the job without much perf drawbacks and code changes. One
> highlighted difference between the two could be that in the previous
> version, lease
> would not be reused for any file with hardlinks at all, even though the inode
> may hold the correct lease for that particular file. The current changes
> would take care of this by sending the lease at least once, irrespective of the
> number of hardlinks.

Thanks for the explanation.  However, the code change size is no excuse
for providing workarounds rather than the actual fix.

A possible way to handle such case would be keeping a list of
pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
could look up the lease key by using @dentry.  I'm not sure if there's a
better way to handle it as I haven't looked into it further.

Tom Talpey Jan. 4, 2024, 9:09 p.m. UTC | #4

On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
> Meetakshi Setiya <[e-mail]> writes:
> 
>> As per the discussion with Tom on the previous version of the changes, I
>> conferred with Shyam and Steve about possible workarounds and this seemed like a
>> choice which did the job without much perf drawbacks and code changes. One
>> highlighted difference between the two could be that in the previous
>> version, lease
>> would not be reused for any file with hardlinks at all, even though the inode
>> may hold the correct lease for that particular file. The current changes
>> would take care of this by sending the lease at least once, irrespective of the
>> number of hardlinks.
> 
> Thanks for the explanation.  However, the code change size is no excuse
> for providing workarounds rather than the actual fix.

I have to agree. And it really isn't much of a workaround either.

> A possible way to handle such case would be keeping a list of
> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
> could look up the lease key by using @dentry.  I'm not sure if there's a
> better way to handle it as I haven't looked into it further.

A list would also allow for better handling of lease revocation.
It seems to me this approach basically discards the original lease,
putting the client's cached data at risk, no? And what happens if
EINVAL comes back again, or the connection breaks? Is this a
recoverable situation?

Also, what's up with the xfstest the robot mailed about?

Tom.

Paulo Alcantara Jan. 4, 2024, 11:09 p.m. UTC | #5

Tom Talpey <[e-mail]> writes:

> On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
>> A possible way to handle such case would be keeping a list of
>> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
>> could look up the lease key by using @dentry.  I'm not sure if there's a
>> better way to handle it as I haven't looked into it further.
>
> A list would also allow for better handling of lease revocation.

It's also worth mentioning that we could also map the dentry directly to
lease key, so no need to store pathnames inside the inode.

> It seems to me this approach basically discards the original lease,
> putting the client's cached data at risk, no? And what happens if
> EINVAL comes back again, or the connection breaks? Is this a
> recoverable situation?

These are really good points and would need to be investigated before
coming up with an implementation.

Shyam Prasad N Jan. 5, 2024, 9:18 a.m. UTC | #6

On Fri, Jan 5, 2024 at 4:39 AM Paulo Alcantara <[e-mail]> wrote:
>
> Tom Talpey <[e-mail]> writes:
>
> > On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
> >> A possible way to handle such case would be keeping a list of
> >> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
> >> could look up the lease key by using @dentry.  I'm not sure if there's a
> >> better way to handle it as I haven't looked into it further.
> >
> > A list would also allow for better handling of lease revocation.
>
> It's also worth mentioning that we could also map the dentry directly to
> lease key, so no need to store pathnames inside the inode.

We were discussing just this yesterday. That we don't really need path
names as the key. It could be the dentry.

>
> > It seems to me this approach basically discards the original lease,
> > putting the client's cached data at risk, no? And what happens if
> > EINVAL comes back again, or the connection breaks? Is this a
> > recoverable situation?
>
> These are really good points and would need to be investigated before
> coming up with an implementation.

Shyam Prasad N Jan. 5, 2024, 9:39 a.m. UTC | #7

On Fri, Jan 5, 2024 at 2:39 AM Tom Talpey <[e-mail]> wrote:
>
> On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
> > Meetakshi Setiya <[e-mail]> writes:
> >
> >> As per the discussion with Tom on the previous version of the changes, I
> >> conferred with Shyam and Steve about possible workarounds and this seemed like a
> >> choice which did the job without much perf drawbacks and code changes. One
> >> highlighted difference between the two could be that in the previous
> >> version, lease
> >> would not be reused for any file with hardlinks at all, even though the inode
> >> may hold the correct lease for that particular file. The current changes
> >> would take care of this by sending the lease at least once, irrespective of the
> >> number of hardlinks.
> >
> > Thanks for the explanation.  However, the code change size is no excuse
> > for providing workarounds rather than the actual fix.
>
> I have to agree. And it really isn't much of a workaround either.
>

The original problem, i.e. compound operations like
unlink/rename/setsize not sending a lease key is very prevalent on the
field.
Unfortunately, fixing that exposed this problem with hard links.
So Steve suggested getting this fix to a shape where it's fixing the
original problem, even if it means that it does not fix it for the
case of where there are open handles to multiple hard links to the
same file.
Only thing we need to be careful of is that it does not make things
worse for other workloads.

> > A possible way to handle such case would be keeping a list of
> > pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
> > could look up the lease key by using @dentry.  I'm not sure if there's a
> > better way to handle it as I haven't looked into it further.
>

This seems like a reasonable change to make. That will make sure that
we stick to what the protocol recommends.
I'm not sure that this change will be a simple one. There could be
several places where we make an assumption that the lease is
associated with an inode, and not a link.

And I'm not yet fully convinced that the spec itself is doing the
right thing by tying the lease with the link, rather than the file.
Shouldn't the lease protect the data of the file, and not the link
itself? If opening two links to the same file with two different lease
keys end up breaking each other's leases, what's the point?

> A list would also allow for better handling of lease revocation.
> It seems to me this approach basically discards the original lease,
> putting the client's cached data at risk, no? And what happens if
> EINVAL comes back again, or the connection breaks? Is this a
> recoverable situation?
>
> Also, what's up with the xfstest the robot mailed about?
Meetakshi is investigating this one.
Initial investigations led us to believe that this is related to
deferred closes. The failing tests do show that the close is getting
delayed, and that setting closetimeo=0 causes the test to pass.
However, it is not clear why the test started failing only now. We'll
have the answers soon.

>
> Tom.

Stefan Metzmacher Jan. 5, 2024, 10 a.m. UTC | #8

Am 05.01.24 um 10:39 schrieb Shyam Prasad N via samba-technical:
> On Fri, Jan 5, 2024 at 2:39 AM Tom Talpey <[e-mail]> wrote:
>>
>> On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
>>> Meetakshi Setiya <[e-mail]> writes:
>>>
>>>> As per the discussion with Tom on the previous version of the changes, I
>>>> conferred with Shyam and Steve about possible workarounds and this seemed like a
>>>> choice which did the job without much perf drawbacks and code changes. One
>>>> highlighted difference between the two could be that in the previous
>>>> version, lease
>>>> would not be reused for any file with hardlinks at all, even though the inode
>>>> may hold the correct lease for that particular file. The current changes
>>>> would take care of this by sending the lease at least once, irrespective of the
>>>> number of hardlinks.
>>>
>>> Thanks for the explanation.  However, the code change size is no excuse
>>> for providing workarounds rather than the actual fix.
>>
>> I have to agree. And it really isn't much of a workaround either.
>>
> 
> The original problem, i.e. compound operations like
> unlink/rename/setsize not sending a lease key is very prevalent on the
> field.
> Unfortunately, fixing that exposed this problem with hard links.
> So Steve suggested getting this fix to a shape where it's fixing the
> original problem, even if it means that it does not fix it for the
> case of where there are open handles to multiple hard links to the
> same file.
> Only thing we need to be careful of is that it does not make things
> worse for other workloads.
> 
>>> A possible way to handle such case would be keeping a list of
>>> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
>>> could look up the lease key by using @dentry.  I'm not sure if there's a
>>> better way to handle it as I haven't looked into it further.
>>
> 
> This seems like a reasonable change to make. That will make sure that
> we stick to what the protocol recommends.
> I'm not sure that this change will be a simple one. There could be
> several places where we make an assumption that the lease is
> associated with an inode, and not a link.
> 
> And I'm not yet fully convinced that the spec itself is doing the
> right thing by tying the lease with the link, rather than the file.
> Shouldn't the lease protect the data of the file, and not the link
> itself? If opening two links to the same file with two different lease
> keys end up breaking each other's leases, what's the point?

I guess the reason for making the lease key per path on
the client is that the client can't know about possible hardlinks
before opening the file, but that open wants to use a leasekey...
Or a "stat" open that won't any lease needs to be done first,
which doubles the roundtrip for every open.

And hard links are not that common...

Maybe choosing und using a new leasekey would be the
way to start with and when a hardlink is detected
the open on the hardlink is closed again and retried
with the former lease key, which would also upgrade it again.

But sharing the handle lease for two pathnames seems wrong,
as the idea of the handle lease is to cache the patchname on the client.

While sharing the RW lease between two hardlinks would be desired.

metze

Shyam Prasad N Jan. 5, 2024, 10:23 a.m. UTC | #9

Hi Metze,

On Fri, Jan 5, 2024 at 3:30 PM Stefan Metzmacher <[e-mail]> wrote:
>
> Am 05.01.24 um 10:39 schrieb Shyam Prasad N via samba-technical:
> > On Fri, Jan 5, 2024 at 2:39 AM Tom Talpey <[e-mail]> wrote:
> >>
> >> On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
> >>> Meetakshi Setiya <[e-mail]> writes:
> >>>
> >>>> As per the discussion with Tom on the previous version of the changes, I
> >>>> conferred with Shyam and Steve about possible workarounds and this seemed like a
> >>>> choice which did the job without much perf drawbacks and code changes. One
> >>>> highlighted difference between the two could be that in the previous
> >>>> version, lease
> >>>> would not be reused for any file with hardlinks at all, even though the inode
> >>>> may hold the correct lease for that particular file. The current changes
> >>>> would take care of this by sending the lease at least once, irrespective of the
> >>>> number of hardlinks.
> >>>
> >>> Thanks for the explanation.  However, the code change size is no excuse
> >>> for providing workarounds rather than the actual fix.
> >>
> >> I have to agree. And it really isn't much of a workaround either.
> >>
> >
> > The original problem, i.e. compound operations like
> > unlink/rename/setsize not sending a lease key is very prevalent on the
> > field.
> > Unfortunately, fixing that exposed this problem with hard links.
> > So Steve suggested getting this fix to a shape where it's fixing the
> > original problem, even if it means that it does not fix it for the
> > case of where there are open handles to multiple hard links to the
> > same file.
> > Only thing we need to be careful of is that it does not make things
> > worse for other workloads.
> >
> >>> A possible way to handle such case would be keeping a list of
> >>> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
> >>> could look up the lease key by using @dentry.  I'm not sure if there's a
> >>> better way to handle it as I haven't looked into it further.
> >>
> >
> > This seems like a reasonable change to make. That will make sure that
> > we stick to what the protocol recommends.
> > I'm not sure that this change will be a simple one. There could be
> > several places where we make an assumption that the lease is
> > associated with an inode, and not a link.
> >
> > And I'm not yet fully convinced that the spec itself is doing the
> > right thing by tying the lease with the link, rather than the file.
> > Shouldn't the lease protect the data of the file, and not the link
> > itself? If opening two links to the same file with two different lease
> > keys end up breaking each other's leases, what's the point?
>
> I guess the reason for making the lease key per path on
> the client is that the client can't know about possible hardlinks
> before opening the file, but that open wants to use a leasekey...
> Or a "stat" open that won't any lease needs to be done first,
> which doubles the roundtrip for every open.
>
> And hard links are not that common...
>

That does makes sense.

> Maybe choosing und using a new leasekey would be the
> way to start with and when a hardlink is detected
> the open on the hardlink is closed again and retried
> with the former lease key, which would also upgrade it again.

That would not work today, as the former lease key would be associated
with the other hardlink. And would result in the server returning
STATUS_INVALID_PARAMETER.

>
> But sharing the handle lease for two pathnames seems wrong,
> as the idea of the handle lease is to cache the patchname on the client.
>
> While sharing the RW lease between two hardlinks would be desired.
>
> metze

Stefan Metzmacher Jan. 5, 2024, 10:38 a.m. UTC | #10

Hi Shyam,

>> Maybe choosing und using a new leasekey would be the
>> way to start with and when a hardlink is detected
>> the open on the hardlink is closed again and retried
>> with the former lease key, which would also upgrade it again.
> 
> That would not work today, as the former lease key would be associated
> with the other hardlink. And would result in the server returning
> STATUS_INVALID_PARAMETER.

And that's the original problem you try to solve, correct?

Then there's nothing we can do expect for using a dentry pased
lease key and live with the fact that they don't allow write caching
anymore. The RH state should still be granted to both lease keys...

metze

Shyam Prasad N Jan. 5, 2024, 10:58 a.m. UTC | #11

On Fri, Jan 5, 2024 at 4:08 PM Stefan Metzmacher <[e-mail]> wrote:
>
> Hi Shyam,
>
> >> Maybe choosing und using a new leasekey would be the
> >> way to start with and when a hardlink is detected
> >> the open on the hardlink is closed again and retried
> >> with the former lease key, which would also upgrade it again.
> >
> > That would not work today, as the former lease key would be associated
> > with the other hardlink. And would result in the server returning
> > STATUS_INVALID_PARAMETER.
>
> And that's the original problem you try to solve, correct?
Correct. I thought you were proposing this as a solution.
>
> Then there's nothing we can do expect for using a dentry pased
> lease key and live with the fact that they don't allow write caching
> anymore. The RH state should still be granted to both lease keys...

Yes. It's not ideal. But I guess we need to live with it.
Thanks for the inputs.

Steve/Paulo/Tom: What do you feel about fixing this in two phases?

First, take Meetakshi's earlier patch, which would fix the problem of
unnecessary lease breaks (and possible deadlock situation with the
server) due to unlink/rename/setfilesize for files that do not have
multiple hard links. i
.e. during these operations, check if link count for the file is 1.
Only if that is the case, send the lease key for the file. This would
mean that the problem remains for files that have multiple hard links.
But at least the hard link xfstest would pass.

As a following patch, work on the full fix. i.e. maintain a list of
lease keys for the file, keyed by the dentry.
This patch would replace the cinode->lease_key with a map/list, lookup
the correct lease from the list on usage.
This would obviously remove the check for the link count done by the
above patch.

Reason being that we already have the first patch, and I'm not sure
we'll be able to work on the second one soon enough.

>
> metze
>

Steve French Jan. 5, 2024, 6:42 p.m. UTC | #12

On Fri, Jan 5, 2024 at 4:58 AM Shyam Prasad N <[e-mail]> wrote:
>
> On Fri, Jan 5, 2024 at 4:08 PM Stefan Metzmacher <[e-mail]> wrote:
> >
> > Hi Shyam,
> >
> > >> Maybe choosing und using a new leasekey would be the
> > >> way to start with and when a hardlink is detected
> > >> the open on the hardlink is closed again and retried
> > >> with the former lease key, which would also upgrade it again.
> > >
> > > That would not work today, as the former lease key would be associated
> > > with the other hardlink. And would result in the server returning
> > > STATUS_INVALID_PARAMETER.
> >
> > And that's the original problem you try to solve, correct?
> Correct. I thought you were proposing this as a solution.
> >
> > Then there's nothing we can do expect for using a dentry pased
> > lease key and live with the fact that they don't allow write caching
> > anymore. The RH state should still be granted to both lease keys...
>
> Yes. It's not ideal. But I guess we need to live with it.
> Thanks for the inputs.
>
> Steve/Paulo/Tom: What do you feel about fixing this in two phases?
>
> First, take Meetakshi's earlier patch, which would fix the problem of
> unnecessary lease breaks (and possible deadlock situation with the
> server) due to unlink/rename/setfilesize for files that do not have
> multiple hard links. i
> .e. during these operations, check if link count for the file is 1.
> Only if that is the case, send the lease key for the file. This would
> mean that the problem remains for files that have multiple hard links.
> But at least the hard link xfstest would pass.

Since this approach could be a huge performance degradation for some
(albeit rare)
workloads (e.g. if hardlinks exist for files, but such files are not opened by
different names at the same time), I prefer the two phase approach
that simply retries when we get invalid argument without the lease key
(which has no risk since the current code just fails, and retry will "fix" the
issue albeit not as good as being able to cache the second open)

> As a following patch, work on the full fix. i.e. maintain a list of
> lease keys for the file, keyed by the dentry.
> This patch would replace the cinode->lease_key with a map/list, lookup
> the correct lease from the list on usage.
> This would obviously remove the check for the link count done by the
> above patch.

I don't like the idea of hurting caching for all hardlinked files as a hack,
so for the longer term solution I prefer a solution that caches the
dentry pointer with the lease key, although that brings up the obvious
question of whether the dentry could be freed and reallocated
in some deferred close cases and cause the lease key to be valid
but us not to match it due to new dentry).

I lean toward something like:
1) store the dentry for the lease key, not just the lease key in the
cifs inode info (today we only store the lease key).
We could store an array of lease key/dentry pairs but I worry that
this would run the risk of use after free and/or lock contention bugs
(and additional memory usage if a malicious app tried to open all
hardlinked files)
2) if link count is greater than 1, check that the dentry matches when
deciding whether to use the lease key (presumably
we don't have to worry about it link count is 1)

Meetakshi Setiya Jan. 17, 2024, 2:15 p.m. UTC | #13

Hi

Here is why xfstest 591 must be failing when the lease key is being
reused for the
unlink compound operation and closetimeo is set to any value other than 0
(deferred close is supported).

The splice_test program xfstest 591 uses calls unlink() at the end of the
program to delete the file A without closing the file handle first. This
file is thus marked for delete and the server returns STATUS_DELETE_PENDING.
If the mount options have specified anything other than closetimeo=0 i.e. if
we have enabled deferred closes, the open handle to this file (which was not
closed by the application), would have deferred closed once the application
ends. When the shell script runs the splice_test program again, the same
handle from the previous run of the splice_test program - that was supposed
to be closed because the file is deleted- is used by the next open to
open file A
since handle caching is still allowed for its lease.
This leads to the error: No such file or directory which we see in
the file 591.out.bad.
This error was not observed when unlink operation broke leases because when
the server issued a lease break to the client, it made the client flush its
remaining writes/locks to the server and downgrade its lease from RWH to R.
Since handle caching is not involved here anymore, the handle was also not
reused anymore by the next open.
Now that the patch has removed the lease break communication with the
server, something similar to what happens when a client gets lease break
notification might need to be done. One solution could be to flush all
cached writes to the server and downgrade all leases for open handles to
file A to R or 0 as soon as unlink is issued for the file A. In this case, even
if some file handles are deferred closed, they would not be reused.
A simple repro for this bug when the above patch 1/2 is applied (courtesy of
@bharath):
1.  Mount share with closetimeo=30
2.  (shell1): tail -f /dev/null > myfile
3.  (shell2): rm myfile
4.  (shell1): ctrl+c to close myfile handle. This would be deferred closed
5.  Touch myfile will fail for a period  = closetimeo value

Thanks
Meetakshi

On Sat, Jan 6, 2024 at 12:12 AM Steve French <[e-mail]> wrote:
>
> On Fri, Jan 5, 2024 at 4:58 AM Shyam Prasad N <[e-mail]> wrote:
> >
> > On Fri, Jan 5, 2024 at 4:08 PM Stefan Metzmacher <[e-mail]> wrote:
> > >
> > > Hi Shyam,
> > >
> > > >> Maybe choosing und using a new leasekey would be the
> > > >> way to start with and when a hardlink is detected
> > > >> the open on the hardlink is closed again and retried
> > > >> with the former lease key, which would also upgrade it again.
> > > >
> > > > That would not work today, as the former lease key would be associated
> > > > with the other hardlink. And would result in the server returning
> > > > STATUS_INVALID_PARAMETER.
> > >
> > > And that's the original problem you try to solve, correct?
> > Correct. I thought you were proposing this as a solution.
> > >
> > > Then there's nothing we can do expect for using a dentry pased
> > > lease key and live with the fact that they don't allow write caching
> > > anymore. The RH state should still be granted to both lease keys...
> >
> > Yes. It's not ideal. But I guess we need to live with it.
> > Thanks for the inputs.
> >
> > Steve/Paulo/Tom: What do you feel about fixing this in two phases?
> >
> > First, take Meetakshi's earlier patch, which would fix the problem of
> > unnecessary lease breaks (and possible deadlock situation with the
> > server) due to unlink/rename/setfilesize for files that do not have
> > multiple hard links. i
> > .e. during these operations, check if link count for the file is 1.
> > Only if that is the case, send the lease key for the file. This would
> > mean that the problem remains for files that have multiple hard links.
> > But at least the hard link xfstest would pass.
>
> Since this approach could be a huge performance degradation for some
> (albeit rare)
> workloads (e.g. if hardlinks exist for files, but such files are not opened by
> different names at the same time), I prefer the two phase approach
> that simply retries when we get invalid argument without the lease key
> (which has no risk since the current code just fails, and retry will "fix" the
> issue albeit not as good as being able to cache the second open)
>
> > As a following patch, work on the full fix. i.e. maintain a list of
> > lease keys for the file, keyed by the dentry.
> > This patch would replace the cinode->lease_key with a map/list, lookup
> > the correct lease from the list on usage.
> > This would obviously remove the check for the link count done by the
> > above patch.
>
> I don't like the idea of hurting caching for all hardlinked files as a hack,
> so for the longer term solution I prefer a solution that caches the
> dentry pointer with the lease key, although that brings up the obvious
> question of whether the dentry could be freed and reallocated
> in some deferred close cases and cause the lease key to be valid
> but us not to match it due to new dentry).
>
> I lean toward something like:
> 1) store the dentry for the lease key, not just the lease key in the
> cifs inode info (today we only store the lease key).
> We could store an array of lease key/dentry pairs but I worry that
> this would run the risk of use after free and/or lock contention bugs
> (and additional memory usage if a malicious app tried to open all
> hardlinked files)
> 2) if link count is greater than 1, check that the dentry matches when
> deciding whether to use the lease key (presumably
> we don't have to worry about it link count is 1)
>
> --
> Thanks,
>
> Steve

Meetakshi Setiya Feb. 2, 2024, 12:50 p.m. UTC | #14

Hi

There was a pending fix to reuse lease key in compound operations on
the smb client, essentially proposed to resolve a customer-reported
bug. A scenario similar to what the client faced would be this:
Imagine a file that has a lot of dirty pages to be written to the
server. If rename/unlink/set path size compound operations are
performed on this file, with the current implementation, we do not
send the lease key. Resultantly, this would lead to the server
assuming that the request came from a new client and it would send a
lease break notification to the same client, on the same connection.
This lease break can lead the client to consume several credits while
writing its dirty pages to the server. A deadlock may arise when the
server stops granting more credits to this connection and the deadlock
would be lifted only when the lease timer on the server expires.

The fix initially proposed just copied the lease key from the inode
and passed it along to SMB2_open_init() from unlink, rename and set
path size compound operations. Incidentally, that exposed a
shortcoming in the smb client-side code. As per MS-SMB2, lease keys
are associated with the file name. Linux cifs client maintains lease
keys with the inode. If the file has any hardlinks, it is possible
that the lease for a file be wrongly reused for an operation on the
hardlink or vice versa. In these cases, the mentioned compound
operations fail with STATUS_INVALID_PARAMETER.

A simple, but hacky fix for the above, as per my discussion with Steve
would be to have a two-phased approach and resend the compound op
request again without the lease key if STATUS_INVALID_PARAMETER is
received. This fix could be easily backported to older kernels since
it would be pretty straightforward and does not involve a lot of code
changes. Such a fallback mechanism should not hurt any functionality
but might impact performance especially in cases where the error is
not because of the usage of wrong lease key and we might end up doing
an extra roundtrip. From what I know, use cases where two or more
file+hardlinks are being used together at the same time are kind of
rare, so we might not run into cases where resending requests in this
fashion has to be performed often.

I understand this is not a perfect or correct fix to the problem, but
for the time being, it might help solve the customer reported issue I
mentioned at the start and could also be backported readily. Since
hurting caching for all hardlinked files is not ideal, I am already
working on another fix that stores the dentry pointer with the lease
key in the cinode object.

From my discussion with Steve and Shyam, the fix proposed was this:
Instead of having a list of dentries/key dentry pairs, we can store a
dentry pointer in addition to the lease key in the cinode object. The
dentry (as a proxy for filename/path) would be the one the lease key
is associated with. There would be a reference counter to maintain the
number of open handles for that file(path)/dentry. When a new file is
created, not only its lease key, but its dentry too be set in the
cinode object. Whenever there is a new handle opened to this dentry,
the reference count to the leasekey/dentry in the cinode object would
be increased. While reusing the lease key, check if dentry (from the
request) matches the dentry stored in the cinode. If it does, copy the
lease key, if it does not, do not use that lease key. When all file
handles to a dentry have been closed, clear the dentry and lease key
from the cinode object. This has the potential to fix the hardlink
issue since dentries for different hardlinks would be different and
one would not end up reusing lease from another. Some implementation
details I am unsure about is should the open request for a file with
mismatched dentry (which would not get to reuse the lease key) send no
lease context at all, or create a new lease key and use that, or
something else?

I am writing this mail to seek advice/feedback on the situation. Any
suggestions or better solutions to any of the issues mentioned in this
mail are very much appreciated.

Thanks
Meetakshi




On Fri, Jan 5, 2024 at 3:30 PM Stefan Metzmacher <[e-mail]> wrote:
>
> Am 05.01.24 um 10:39 schrieb Shyam Prasad N via samba-technical:
> > On Fri, Jan 5, 2024 at 2:39 AM Tom Talpey <[e-mail]> wrote:
> >>
> >> On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
> >>> Meetakshi Setiya <[e-mail]> writes:
> >>>
> >>>> As per the discussion with Tom on the previous version of the changes, I
> >>>> conferred with Shyam and Steve about possible workarounds and this seemed like a
> >>>> choice which did the job without much perf drawbacks and code changes. One
> >>>> highlighted difference between the two could be that in the previous
> >>>> version, lease
> >>>> would not be reused for any file with hardlinks at all, even though the inode
> >>>> may hold the correct lease for that particular file. The current changes
> >>>> would take care of this by sending the lease at least once, irrespective of the
> >>>> number of hardlinks.
> >>>
> >>> Thanks for the explanation.  However, the code change size is no excuse
> >>> for providing workarounds rather than the actual fix.
> >>
> >> I have to agree. And it really isn't much of a workaround either.
> >>
> >
> > The original problem, i.e. compound operations like
> > unlink/rename/setsize not sending a lease key is very prevalent on the
> > field.
> > Unfortunately, fixing that exposed this problem with hard links.
> > So Steve suggested getting this fix to a shape where it's fixing the
> > original problem, even if it means that it does not fix it for the
> > case of where there are open handles to multiple hard links to the
> > same file.
> > Only thing we need to be careful of is that it does not make things
> > worse for other workloads.
> >
> >>> A possible way to handle such case would be keeping a list of
> >>> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
> >>> could look up the lease key by using @dentry.  I'm not sure if there's a
> >>> better way to handle it as I haven't looked into it further.
> >>
> >
> > This seems like a reasonable change to make. That will make sure that
> > we stick to what the protocol recommends.
> > I'm not sure that this change will be a simple one. There could be
> > several places where we make an assumption that the lease is
> > associated with an inode, and not a link.
> >
> > And I'm not yet fully convinced that the spec itself is doing the
> > right thing by tying the lease with the link, rather than the file.
> > Shouldn't the lease protect the data of the file, and not the link
> > itself? If opening two links to the same file with two different lease
> > keys end up breaking each other's leases, what's the point?
>
> I guess the reason for making the lease key per path on
> the client is that the client can't know about possible hardlinks
> before opening the file, but that open wants to use a leasekey...
> Or a "stat" open that won't any lease needs to be done first,
> which doubles the roundtrip for every open.
>
> And hard links are not that common...
>
> Maybe choosing und using a new leasekey would be the
> way to start with and when a hardlink is detected
> the open on the hardlink is closed again and retried
> with the former lease key, which would also upgrade it again.
>
> But sharing the handle lease for two pathnames seems wrong,
> as the idea of the handle lease is to cache the patchname on the client.
>
> While sharing the RW lease between two hardlinks would be desired.
>
> metze

Patch

diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 446433606a04..d3d7a4652a5b 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -121,6 +121,17 @@  static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 	}
 
 	/* if there is an existing lease, reuse it */
+
+	/*
+	 * note: files with hardlinks cause unexpected behaviour. As per MS-SMB2,
+	 * lease keys are associated with the file name. We are maintaining lease keys
+	 * with the inode on the client. If the file has hardlinks associated with it,
+	 * it could be possible that the lease for a file be reused for an operation
+	 * on the hardlink or vice versa.
+	 * As a workaround, send request using an existing lease key and if the server
+	 * returns STATUS_INVALID_PARAMETER, which maps to EINVAL, send the request
+	 * again without the lease.
+	 */
 	if (dentry) {
 		inode = d_inode(dentry);
 		cinode = CIFS_I(inode);
@@ -907,10 +918,18 @@  int
 smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
 	    struct cifs_sb_info *cifs_sb, struct dentry *dentry)
 {
-	return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
+	int rc = smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
 				CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT,
 				ACL_NO_MODE, NULL, &(int){SMB2_OP_DELETE}, 1,
 				NULL, NULL, NULL, NULL, NULL, dentry);
+	if (rc == -EINVAL) {
+		cifs_dbg(FYI, "invalid lease key, resending request without lease");
+		rc = smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
+				CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT,
+				ACL_NO_MODE, NULL, &(int){SMB2_OP_DELETE}, 1,
+				NULL, NULL, NULL, NULL, NULL, NULL);
+	}
+	return rc;
 }
 
 static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
@@ -950,8 +969,14 @@  int smb2_rename_path(const unsigned int xid,
 	drop_cached_dir_by_name(xid, tcon, from_name, cifs_sb);
 	cifs_get_writable_path(tcon, from_name, FIND_WR_WITH_DELETE, &cfile);
 
-	return smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
+	int rc = smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
 				  co, DELETE, SMB2_OP_RENAME, cfile, source_dentry);
+	if (rc == -EINVAL) {
+		cifs_dbg(FYI, "invalid lease key, resending request without lease");
+		smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
+				  co, DELETE, SMB2_OP_RENAME, cfile, NULL);
+	}
+	return rc;
 }
 
 int smb2_create_hardlink(const unsigned int xid,
@@ -979,11 +1004,20 @@  smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
 	in_iov.iov_base = &eof;
 	in_iov.iov_len = sizeof(eof);
 	cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
-	return smb2_compound_op(xid, tcon, cifs_sb, full_path,
+	int rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
 				FILE_WRITE_DATA, FILE_OPEN,
 				0, ACL_NO_MODE, &in_iov,
 				&(int){SMB2_OP_SET_EOF}, 1,
 				cfile, NULL, NULL, NULL, NULL, dentry);
+	if (rc == -EINVAL) {
+		cifs_dbg(FYI, "invalid lease key, resending request without lease");
+		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
+				FILE_WRITE_DATA, FILE_OPEN,
+				0, ACL_NO_MODE, &in_iov,
+				&(int){SMB2_OP_SET_EOF}, 1,
+				cfile, NULL, NULL, NULL, NULL, NULL);
+	}
+	return rc;
 }
 
 int