> From: Tom Talpey <tom@xxxxxxxxxx>
> Sent: Tuesday, May 16, 2023 7:41 PM
> To: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>; linux-cifs@xxxxxxxxxxxxxxx <linux-cifs@xxxxxxxxxxxxxxx>
> Cc: Steve French <sfrench@xxxxxxxxx>; Paulo Alcantara <pc@xxxxxx>; Ronnie Sahlberg <lsahlber@xxxxxxxxxx>; Shyam Prasad N <sprasad@xxxxxxxxxxxxx>; Rohith Surabattula <rohiths@xxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
> > ---
> >
> > 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,
RossAttachment:
smbtrace.pcap
Description: smbtrace.pcap
