| Message ID | 20230516150153.1864023-1-ross.lagerwall@citrix.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: Close deferred files that may be open via hard links | expand |
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 <ross.lagerwall@citrix.com> > --- > > 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;
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 :-).
> From: Tom Talpey <tom@talpey.com> > Sent: Tuesday, May 16, 2023 7:41 PM > To: Ross Lagerwall <ross.lagerwall@citrix.com>; linux-cifs@vger.kernel.org <linux-cifs@vger.kernel.org> > Cc: Steve French <sfrench@samba.org>; Paulo Alcantara <pc@cjr.nz>; Ronnie Sahlberg <lsahlber@redhat.com>; Shyam Prasad N <sprasad@microsoft.com>; Rohith Surabattula <rohiths@microsoft.com> > 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 <ross.lagerwall@citrix.com> > > --- > > > > 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
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
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.
> From: Tom Talpey <tom@talpey.com> > Sent: Wednesday, May 17, 2023 2:24 PM > To: Ralph Boehme <slow@samba.org>; Ross Lagerwall <ross.lagerwall@citrix.com>; linux-cifs@vger.kernel.org <linux-cifs@vger.kernel.org> > Cc: Steve French <sfrench@samba.org>; Paulo Alcantara <pc@cjr.nz>; Ronnie Sahlberg <lsahlber@redhat.com>; Shyam Prasad N <sprasad@microsoft.com>; Rohith Surabattula <rohiths@microsoft.com> > 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
> From: Ross Lagerwall <ross.lagerwall@citrix.com> > Sent: Wednesday, May 17, 2023 2:48 PM > To: Tom Talpey <tom@talpey.com>; Ralph Boehme <slow@samba.org>; linux-cifs@vger.kernel.org <linux-cifs@vger.kernel.org> > Cc: Steve French <sfrench@samba.org>; Paulo Alcantara <pc@cjr.nz>; Ronnie Sahlberg <lsahlber@redhat.com>; Shyam Prasad N <sprasad@microsoft.com>; Rohith Surabattula <rohiths@microsoft.com> > Subject: Re: [PATCH] cifs: Close deferred files that may be open via hard links > > > From: Tom Talpey <tom@talpey.com> > > Sent: Wednesday, May 17, 2023 2:24 PM > > To: Ralph Boehme <slow@samba.org>; Ross Lagerwall <ross.lagerwall@citrix.com>; linux-cifs@vger.kernel.org <linux-cifs@vger.kernel.org> > > Cc: Steve French <sfrench@samba.org>; Paulo Alcantara <pc@cjr.nz>; Ronnie Sahlberg <lsahlber@redhat.com>; Shyam Prasad N <sprasad@microsoft.com>; Rohith Surabattula <rohiths@microsoft.com> > > 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
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;
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 <ross.lagerwall@citrix.com> --- 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(+)