cifs: Close deferred files that may be open via hard links

Commit Message

Ross Lagerwall May 16, 2023, 3:01 p.m. UTC

Windows Server (tested with 2016) disallows opening the same inode via
two different hardlinks at the same time. With deferred closes, this can
result in unexpected behaviour as the following Python snippet shows:

    # Create file
    fd = os.open('test', os.O_WRONLY|os.O_CREAT)
    os.write(fd, b'foo')
    os.close(fd)

    # Open and close the file to leave a pending deferred close
    fd = os.open('test', os.O_RDONLY|os.O_DIRECT)
    os.close(fd)

    # Try to open the file via a hard link
    os.link('test', 'new')
    newfd = os.open('new', os.O_RDONLY|os.O_DIRECT)

The final open returns EINVAL due to the server returning
STATUS_INVALID_PARAMETER.

Fix this by closing any deferred closes that may be open via other hard
links if we haven't successfully reused a cached handle.

Fixes: c3f207ab29f7 ("cifs: Deferred close for files")
Signed-off-by: Ross Lagerwall <[e-mail]>
---

This is kind of an RFC. Is the server doing the wrong thing? Is it
correct to work around this in the client?

 fs/cifs/file.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Tom Talpey May 16, 2023, 6:41 p.m. UTC | #1

On 5/16/2023 11:01 AM, Ross Lagerwall wrote:
> Windows Server (tested with 2016) disallows opening the same inode via
> two different hardlinks at the same time. With deferred closes, this can
> result in unexpected behaviour as the following Python snippet shows:
> 
>      # Create file
>      fd = os.open('test', os.O_WRONLY|os.O_CREAT)
>      os.write(fd, b'foo')
>      os.close(fd)
> 
>      # Open and close the file to leave a pending deferred close
>      fd = os.open('test', os.O_RDONLY|os.O_DIRECT)
>      os.close(fd)
> 
>      # Try to open the file via a hard link
>      os.link('test', 'new')
>      newfd = os.open('new', os.O_RDONLY|os.O_DIRECT)
> 
> The final open returns EINVAL due to the server returning
> STATUS_INVALID_PARAMETER.

Good sleuthing. The hardlink behavior is slightly surprising, but
it is certainly the case that certain incompatible combinations of
open/create flags can conflict with other handles, including
cached opens.

> Fix this by closing any deferred closes that may be open via other hard
> links if we haven't successfully reused a cached handle.
> 
> Fixes: c3f207ab29f7 ("cifs: Deferred close for files")
> Signed-off-by: Ross Lagerwall <[e-mail]>
> ---
> 
> This is kind of an RFC. Is the server doing the wrong thing? Is it
> correct to work around this in the client?

By definition the server is doing what it does, and it's a moot
question whether it's appropriate to work around it. However, I don't
see this behavior explicitly stated in MS-FSA. And INVALID_PARAMETER
is a strange error code to return for this. Do you have a packet
trace? We should ask Microsoft.

>   fs/cifs/file.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index c5fcefdfd797..723cbc060f57 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -749,6 +749,7 @@ int cifs_open(struct inode *inode, struct file *file)
>   			_cifsFileInfo_put(cfile, true, false);
>   		}
>   	}
> +	cifs_close_deferred_file(CIFS_I(inode));

But, is this correct? I don't believe this function blocks. And I'm
not sure this triggers the close only in the case you've identified.

Tom.

>   	if (server->oplocks)
>   		oplock = REQ_OPLOCK;

Jeremy Allison May 16, 2023, 7:26 p.m. UTC | #2

On Tue, May 16, 2023 at 02:41:06PM -0400, Tom Talpey wrote:
>On 5/16/2023 11:01 AM, Ross Lagerwall wrote:
>>Windows Server (tested with 2016) disallows opening the same inode via
>>two different hardlinks at the same time. With deferred closes, this can
>>result in unexpected behaviour as the following Python snippet shows:
>>
>>The final open returns EINVAL due to the server returning
>>STATUS_INVALID_PARAMETER.
>
>Good sleuthing. The hardlink behavior is slightly surprising, but
>it is certainly the case that certain incompatible combinations of
>open/create flags can conflict with other handles, including
>cached opens.

That certainly sounds like implementation-dependent behavior,
probably due to running into the same dev/ino numbers via
a new path :-).

>>This is kind of an RFC. Is the server doing the wrong thing? Is it
>>correct to work around this in the client?
>
>By definition the server is doing what it does, and it's a moot
>question whether it's appropriate to work around it. However, I don't
>see this behavior explicitly stated in MS-FSA. And INVALID_PARAMETER
>is a strange error code to return for this. Do you have a packet
>trace? We should ask Microsoft.

A packet trace of this would be really interesting :-).

Ross Lagerwall May 17, 2023, 8:27 a.m. UTC | #3

> From: Tom Talpey <[e-mail]>
> Sent: Tuesday, May 16, 2023 7:41 PM
> To: Ross Lagerwall <[e-mail]>; [e-mail] <[e-mail]>
> Cc: Steve French <[e-mail]>; Paulo Alcantara <[e-mail]>; Ronnie Sahlberg <[e-mail]>; Shyam Prasad N <[e-mail]>; Rohith Surabattula <[e-mail]>
> Subject: Re: [PATCH] cifs: Close deferred files that may be open via hard links 
>  
> On 5/16/2023 11:01 AM, Ross Lagerwall wrote:
> > Windows Server (tested with 2016) disallows opening the same inode via
> > two different hardlinks at the same time. With deferred closes, this can
> > result in unexpected behaviour as the following Python snippet shows:
> > 
> >      # Create file
> >      fd = os.open('test', os.O_WRONLY|os.O_CREAT)
> >      os.write(fd, b'foo')
> >      os.close(fd)
> > 
> >      # Open and close the file to leave a pending deferred close
> >      fd = os.open('test', os.O_RDONLY|os.O_DIRECT)
> >      os.close(fd)
> > 
> >      # Try to open the file via a hard link
> >      os.link('test', 'new')
> >      newfd = os.open('new', os.O_RDONLY|os.O_DIRECT)
> > 
> > The final open returns EINVAL due to the server returning
> > STATUS_INVALID_PARAMETER.
> 
> Good sleuthing. The hardlink behavior is slightly surprising, but
> it is certainly the case that certain incompatible combinations of
> open/create flags can conflict with other handles, including
> cached opens.
> 
> > Fix this by closing any deferred closes that may be open via other hard
> > links if we haven't successfully reused a cached handle.
> > 
> > Fixes: c3f207ab29f7 ("cifs: Deferred close for files")
> > Signed-off-by: Ross Lagerwall <[e-mail]>
> > ---
> > 
> > This is kind of an RFC. Is the server doing the wrong thing? Is it
> > correct to work around this in the client?
> 
> By definition the server is doing what it does, and it's a moot
> question whether it's appropriate to work around it. However, I don't
> see this behavior explicitly stated in MS-FSA. And INVALID_PARAMETER
> is a strange error code to return for this. Do you have a packet
> trace? We should ask Microsoft.
> 
> >   fs/cifs/file.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index c5fcefdfd797..723cbc060f57 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -749,6 +749,7 @@ int cifs_open(struct inode *inode, struct file *file)
> >                        _cifsFileInfo_put(cfile, true, false);
> >                }
> >        }
> > +     cifs_close_deferred_file(CIFS_I(inode));
> 
> But, is this correct? I don't believe this function blocks. And I'm
> not sure this triggers the close only in the case you've identified.
> 

It calls _cifsFileInfo_put() on each cached handle which calls
server->ops->close() which is synchronous as far as I can tell.

I could restrict it to check if the inode has > 1 link but as you
mentioned above, there are various combinations of open/create flags
that can conflict with cached handles, so maybe it is correct to always
called it in the cache miss path?

In any case, I have attached a packet trace from running the above
Python reproducer.

Client IP == 10.71.56.138
Server IP == 10.71.57.49
Server OS == Windows Server 2016 1607 (build 14393.5717)

Thanks,
Ross

Ralph Boehme May 17, 2023, 10:50 a.m. UTC | #4

On 5/17/23 10:27, Ross Lagerwall wrote:
> In any case, I have attached a packet trace from running the above
> Python reproducer.

afaict the STATUS_INVALID_PARAMETER comes from the lease code as you're 
passing the same lease key for the open on the link which is illegal afair.

Can you retry the experiment without requesting a lease or ensuring the 
second open on the link uses a different lease key?

-slow

Tom Talpey May 17, 2023, 1:24 p.m. UTC | #5

On 5/17/2023 6:50 AM, Ralph Boehme wrote:
> On 5/17/23 10:27, Ross Lagerwall wrote:
>> In any case, I have attached a packet trace from running the above
>> Python reproducer.
> 
> afaict the STATUS_INVALID_PARAMETER comes from the lease code as you're 
> passing the same lease key for the open on the link which is illegal afair.
> 
> Can you retry the experiment without requesting a lease or ensuring the 
> second open on the link uses a different lease key?

Indeed, the same lease key is coming in both opens, first in
packet 3 where it opens "test", and again in packet 18 where
it opens the link "new". So it's triggering this assertion in
MS-SMB2 section 3.3.5.9.11

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

Steve/Rohith, is this new behavior in the client code?

Tom.

Ross Lagerwall May 17, 2023, 1:48 p.m. UTC | #6

> From: Tom Talpey <[e-mail]>
> Sent: Wednesday, May 17, 2023 2:24 PM
> To: Ralph Boehme <[e-mail]>; Ross Lagerwall <[e-mail]>; [e-mail] <[e-mail]>
> Cc: Steve French <[e-mail]>; Paulo Alcantara <[e-mail]>; Ronnie Sahlberg <[e-mail]>; Shyam Prasad N <[e-mail]>; Rohith Surabattula <[e-mail]>
> Subject: Re: [PATCH] cifs: Close deferred files that may be open via hard links 
>  
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> 
> On 5/17/2023 6:50 AM, Ralph Boehme wrote:
> > On 5/17/23 10:27, Ross Lagerwall wrote:
> >> In any case, I have attached a packet trace from running the above
> >> Python reproducer.
> > 
> > afaict the STATUS_INVALID_PARAMETER comes from the lease code as you're 
> > passing the same lease key for the open on the link which is illegal afair.
> > 
> > Can you retry the experiment without requesting a lease or ensuring the 
> > second open on the link uses a different lease key?
> 
> Indeed, the same lease key is coming in both opens, first in
> packet 3 where it opens "test", and again in packet 18 where
> it opens the link "new". So it's triggering this assertion in
> MS-SMB2 section 3.3.5.9.11
> 
> > 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_V2 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
> 
> Steve/Rohith, is this new behavior in the client code?
> 
> Tom.

It looks like smb2_get_lease_key() returns the lease key stored in the
inode, which clearly breaks that assertion when hard links are
involved.

I can confirm that when the lease keys are forced to be different, the
test does not fail.

Ross

Ross Lagerwall May 23, 2023, 4:30 p.m. UTC | #7

> From: Ross Lagerwall <[e-mail]>
> Sent: Wednesday, May 17, 2023 2:48 PM
> To: Tom Talpey <[e-mail]>; Ralph Boehme <[e-mail]>; [e-mail] <[e-mail]>
> Cc: Steve French <[e-mail]>; Paulo Alcantara <[e-mail]>; Ronnie Sahlberg <[e-mail]>; Shyam Prasad N <[e-mail]>; Rohith Surabattula <[e-mail]>
> Subject: Re: [PATCH] cifs: Close deferred files that may be open via hard links 
>  
> > From: Tom Talpey <[e-mail]>
> > Sent: Wednesday, May 17, 2023 2:24 PM
> > To: Ralph Boehme <[e-mail]>; Ross Lagerwall <[e-mail]>; [e-mail] <[e-mail]>
> > Cc: Steve French <[e-mail]>; Paulo Alcantara <[e-mail]>; Ronnie Sahlberg <[e-mail]>; Shyam Prasad N <[e-mail]>; Rohith Surabattula <[e-mail]>
> > Subject: Re: [PATCH] cifs: Close deferred files that may be open via hard links 
> >  
> > On 5/17/2023 6:50 AM, Ralph Boehme wrote:
> > > On 5/17/23 10:27, Ross Lagerwall wrote:
> > >> In any case, I have attached a packet trace from running the above
> > >> Python reproducer.
> > > 
> > > afaict the STATUS_INVALID_PARAMETER comes from the lease code as you're 
> > > passing the same lease key for the open on the link which is illegal afair.
> > > 
> > > Can you retry the experiment without requesting a lease or ensuring the 
> > > second open on the link uses a different lease key?
> > 
> > Indeed, the same lease key is coming in both opens, first in
> > packet 3 where it opens "test", and again in packet 18 where
> > it opens the link "new". So it's triggering this assertion in
> > MS-SMB2 section 3.3.5.9.11
> > 
> > > 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_V2 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
> > 
> > Steve/Rohith, is this new behavior in the client code?
> > 
> > Tom.
> 
> It looks like smb2_get_lease_key() returns the lease key stored in the
> inode, which clearly breaks that assertion when hard links are
> involved.
> 
> I can confirm that when the lease keys are forced to be different, the
> test does not fail.
> 

Hi,

Am I correct in the assertion that the client is associating lease keys with
the inode and that is incompatible with the spec (at least when hard links
are involved)?

A brief look at the code suggests changing that would be somewhat involved...
At least more work than I have time to spare at the moment.

Thanks,
Ross

Patch

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index c5fcefdfd797..723cbc060f57 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -749,6 +749,7 @@  int cifs_open(struct inode *inode, struct file *file)
 			_cifsFileInfo_put(cfile, true, false);
 		}
 	}
+	cifs_close_deferred_file(CIFS_I(inode));
 
 	if (server->oplocks)
 		oplock = REQ_OPLOCK;