[bug report] Inconsistent state with CIFS mount after interrupted process in Linux 5.10

[bug report] Inconsistent state with CIFS mount after interrupted process in Linux 5.10

[Duncan Findlay]

There seems to be a problem with the CIFS module in Linux 5.10. Files
that are opened and not cleanly closed end up in an inconsistent
state. This can be triggered by writing to a file and interrupting the
writer with Ctrl-C. Once this happens, attempting to delete the file
causes access to the mount to hang. Afterwards, the files are visible
to ls, but cannot be accessed or deleted.

I'm running Debian unstable with a Debian unstable kernel
(5.10.5-1). I attempted to but could not reproduce this with a 4.19 kernel.


Repro steps:

$ sudo mount -t cifs //test/share /mnt/test --verbose -o
rw,user,auto,nosuid,uid=user,gid=user,vers=3.1.1,credentials=/home/user/tmp/creds
$ mkdir /mnt/test/subdir
$ cat > /mnt/test/subdir/foo
[ Hit Ctrl-C to interrupt ]
$ ls /mnt/test/subdir/
foo
$ rm /mnt/test/subdir/foo
[ Hangs for 35 seconds, errors in dmesg log -- see below ]
$ ls /mnt/test/subdir/
foo
$ stat /mnt/test/subdir/foo
stat: cannot statx '/mnt/test/subdir/foo': No such file or directory

At this point, the file still exists on the server side, and
restarting the server causes it to be deleted.

I can provide pcaps if necessary. It looks like with 4.19, when the
cat command is killed, the client sends a Close Request, and on 5.10
no commands are sent.


More details:

Client:
$ uname -a
Linux test 5.10.0-1-amd64 #1 SMP Debian 5.10.5-1 (2021-01-09) x86_64 GNU/Linux
$ sudo mount.cifs -V
mount.cifs version: 6.11

Samba Server:
$ sudo testparm --version
Version 4.13.3-Debian

In the logs below, the server and client are on the same
machine.

Partial /etc/samba/smb.conf:

[share]
comment = Test folder
path = /srv/test
read only = No

(Happy to share the rest, but it's likely uninteresting -- all default
Debian config.)


Logs:

Debug dmesg logs, while interrupting cat:

[  131.703533] CIFS: fs/cifs/file.c: Flush inode 00000000d8f49255 file
000000001e1eb942 rc 0
[  131.703541] CIFS: fs/cifs/file.c: closing last open instance for
inode 00000000d8f49255
[  131.703543] CIFS: fs/cifs/file.c: VFS: in _cifsFileInfo_put as Xid:
11 with uid: 1000
[  131.703545] CIFS: fs/cifs/smb2pdu.c: Close
[  131.703549] CIFS: fs/cifs/transport.c: signal pending before send request
[  131.703551] CIFS: fs/cifs/smb2ops.c: add 1 credits total=571
[  131.703559] cifs_small_buf_release: 2 callbacks suppressed
[  131.703559] CIFS: fs/cifs/misc.c: Null buffer passed to
cifs_small_buf_release

Debug dmesg logs from rm:

[  180.885931] CIFS: fs/cifs/inode.c: VFS: in
cifs_revalidate_dentry_attr as Xid: 13 with uid: 1000
[  180.885938] CIFS: fs/cifs/dir.c: name: \subdir
[  180.885944] CIFS: fs/cifs/inode.c: Update attributes: \subdir inode
0x000000008e2380dc count 1 dentry: 0x00000000568fd902 d_time
4294904495 jiffies 4294937505
[  180.885992] CIFS: fs/cifs/transport.c: Sending smb: smb_len=356
[  180.887242] CIFS: fs/cifs/connect.c: RFC1002 header 0x210
[  180.887255] CIFS: fs/cifs/smb2misc.c: SMB2 data length 56 offset 152
[  180.887258] smb2_calc_size: 3 callbacks suppressed
[  180.887259] CIFS: fs/cifs/smb2misc.c: SMB2 len 208
[  180.887269] CIFS: fs/cifs/smb2misc.c: SMB2 data length 114 offset 72
[  180.887271] CIFS: fs/cifs/smb2misc.c: SMB2 len 186
[  180.887274] CIFS: fs/cifs/smb2misc.c: Calculated size 186 length
192 mismatch mid 41
[  180.887281] CIFS: fs/cifs/smb2misc.c: SMB2 len 124
[  180.887284] CIFS: fs/cifs/smb2misc.c: Calculated size 124 length
128 mismatch mid 42
[  180.887287] CIFS: fs/cifs/smb2ops.c: add 30 credits total=598
[  180.887300] cifs_sync_mid_result: 2 callbacks suppressed
[  180.887302] CIFS: fs/cifs/transport.c: cifs_sync_mid_result: cmd=5
mid=40 state=4
[  180.887306] CIFS: fs/cifs/transport.c: cifs_sync_mid_result: cmd=16
mid=41 state=4
[  180.887309] CIFS: fs/cifs/transport.c: cifs_sync_mid_result: cmd=6
mid=42 state=4
[  180.887312] CIFS: fs/cifs/misc.c: Null buffer passed to
cifs_small_buf_release
[  180.887314] CIFS: fs/cifs/misc.c: Null buffer passed to
cifs_small_buf_release
[  180.887316] CIFS: fs/cifs/misc.c: Null buffer passed to
cifs_small_buf_release
[  180.887328] CIFS: fs/cifs/inode.c: cifs_revalidate_cache:
revalidating inode 10952654749510940080
[  180.887331] CIFS: fs/cifs/inode.c: cifs_revalidate_cache:
invalidating inode 10952654749510940080 mapping
[  180.887335] CIFS: fs/cifs/inode.c: VFS: leaving
cifs_revalidate_dentry_attr (xid = 13) rc = 0
[  180.887343] CIFS: fs/cifs/inode.c: VFS: in
cifs_revalidate_dentry_attr as Xid: 14 with uid: 1000
[  180.887346] CIFS: fs/cifs/dir.c: name: \foo
[  180.887348] CIFS: fs/cifs/dir.c: name: \subdir\foo
[  180.887353] CIFS: fs/cifs/inode.c: Update attributes: \subdir\foo
inode 0x00000000d8f49255 count 1 dentry: 0x000000006cae3b3c d_time 0
jiffies 4294937506
[  180.887385] CIFS: fs/cifs/transport.c: Sending smb: smb_len=364
[  180.888296] CIFS: fs/cifs/connect.c: RFC1002 header 0x218
[  180.888307] CIFS: fs/cifs/smb2misc.c: SMB2 data length 56 offset 152
[  180.888309] CIFS: fs/cifs/smb2misc.c: SMB2 len 208
[  180.888337] CIFS: fs/cifs/smb2misc.c: SMB2 data length 122 offset 72
[  180.888339] CIFS: fs/cifs/smb2misc.c: SMB2 len 194
[  180.888343] CIFS: fs/cifs/smb2misc.c: Calculated size 194 length
200 mismatch mid 44
[  180.888392] CIFS: fs/cifs/smb2misc.c: SMB2 len 124
[  180.888394] CIFS: fs/cifs/smb2misc.c: Calculated size 124 length
128 mismatch mid 45
[  180.888398] CIFS: fs/cifs/smb2ops.c: add 30 credits total=625
[  180.888427] CIFS: fs/cifs/transport.c: cifs_sync_mid_result: cmd=5
mid=43 state=4
[  180.888430] CIFS: fs/cifs/transport.c: cifs_sync_mid_result: cmd=16
mid=44 state=4
[  180.888433] CIFS: fs/cifs/transport.c: cifs_sync_mid_result: cmd=6
mid=45 state=4
[  180.888435] CIFS: fs/cifs/misc.c: Null buffer passed to
cifs_small_buf_release
[  180.888438] CIFS: fs/cifs/misc.c: Null buffer passed to
cifs_small_buf_release
[  180.888440] CIFS: fs/cifs/misc.c: Null buffer passed to
cifs_small_buf_release
[  180.888450] CIFS: fs/cifs/inode.c: cifs_revalidate_cache:
revalidating inode 10952654827393506808
[  180.888452] CIFS: fs/cifs/inode.c: cifs_revalidate_cache: inode
10952654827393506808 is unchanged
[  180.888457] CIFS: fs/cifs/inode.c: VFS: leaving
cifs_revalidate_dentry_attr (xid = 14) rc = 0
[  180.888534] CIFS: fs/cifs/inode.c: cifs_unlink,
dir=0x000000008e2380dc, dentry=0x000000006cae3b3c
[  180.888537] CIFS: fs/cifs/inode.c: VFS: in cifs_unlink as Xid: 15
with uid: 1000
[  180.888540] CIFS: fs/cifs/dir.c: name: \foo
[  180.888542] CIFS: fs/cifs/dir.c: name: \subdir\foo
[  180.888566] CIFS: fs/cifs/transport.c: Sending smb: smb_len=260
[  180.889623] CIFS: fs/cifs/connect.c: RFC1002 header 0x6c
[  180.889637] CIFS: fs/cifs/smb2misc.c: SMB2 len 108
[  180.889640] CIFS: fs/cifs/smb2misc.c: Checking for oplock break
[  180.889642] CIFS: fs/cifs/smb2misc.c: Checking for lease break
[  180.889645] CIFS: fs/cifs/smb2misc.c: Can not process lease break -
no lease matched
[  180.889647] CIFS: VFS: \\test No task to wake, unknown frame
received! NumMids 2
[  180.889748] CIFS: Received Data is: : dump of 64 bytes of data at
0x0000000009ce2743
[  180.889754] 00000000: 424d53fe 00000040 00000000 00000012  .SMB@...........
[  180.889757] 00000010: 00000001 00000000 ffffffff ffffffff  ................
[  180.889760] 00000020: 00000000 00000000 00000000 00000000  ................
[  180.889763] 00000030: 00000000 00000000 00000000 00000000  ................
[  180.890911] CIFS: fs/cifs/connect.c: RFC1002 header 0x49
[  180.890920] CIFS: fs/cifs/smb2misc.c: SMB2 data length 0 offset 0
[  180.890923] CIFS: fs/cifs/smb2misc.c: SMB2 len 73
[  215.893172] CIFS: fs/cifs/connect.c: RFC1002 header 0x150
[  215.893186] CIFS: fs/cifs/smb2misc.c: SMB2 data length 56 offset 152
[  215.893189] CIFS: fs/cifs/smb2misc.c: SMB2 len 208
[  215.893199] CIFS: fs/cifs/smb2misc.c: SMB2 len 124
[  215.893202] CIFS: fs/cifs/smb2misc.c: Calculated size 124 length
128 mismatch mid 47
[  215.893206] CIFS: fs/cifs/smb2ops.c: add 10 credits total=643
[  215.893230] CIFS: fs/cifs/transport.c: cifs_sync_mid_result: cmd=5
mid=46 state=4
[  215.893233] CIFS: fs/cifs/transport.c: cifs_sync_mid_result: cmd=6
mid=47 state=4
[  215.893236] CIFS: fs/cifs/misc.c: Null buffer passed to
cifs_small_buf_release
[  215.893240] CIFS: fs/cifs/misc.c: Null buffer passed to
cifs_small_buf_release
[  215.893249] CIFS: fs/cifs/inode.c: VFS: leaving cifs_unlink (xid = 15) rc = 0

Re: [bug report] Inconsistent state with CIFS mount after interrupted process in Linux 5.10

[Aurélien Aptel]

Duncan Findlay <duncf@duncf.ca> writes:
> There seems to be a problem with the CIFS module in Linux 5.10. Files
> that are opened and not cleanly closed end up in an inconsistent
> state. This can be triggered by writing to a file and interrupting the
> writer with Ctrl-C. Once this happens, attempting to delete the file
> causes access to the mount to hang. Afterwards, the files are visible
> to ls, but cannot be accessed or deleted.
>
> I'm running Debian unstable with a Debian unstable kernel
> (5.10.5-1). I attempted to but could not reproduce this with a 4.19 kernel.
>
>
> Repro steps:
>
> $ sudo mount -t cifs //test/share /mnt/test --verbose -o
> rw,user,auto,nosuid,uid=user,gid=user,vers=3.1.1,credentials=/home/user/tmp/creds
> $ mkdir /mnt/test/subdir
> $ cat > /mnt/test/subdir/foo
> [ Hit Ctrl-C to interrupt ]
> $ ls /mnt/test/subdir/
> foo
> $ rm /mnt/test/subdir/foo
> [ Hangs for 35 seconds, errors in dmesg log -- see below ]
> $ ls /mnt/test/subdir/
> foo
> $ stat /mnt/test/subdir/foo
> stat: cannot statx '/mnt/test/subdir/foo': No such file or directory
>
> At this point, the file still exists on the server side, and
> restarting the server causes it to be deleted.
>
> I can provide pcaps if necessary. It looks like with 4.19, when the
> cat command is killed, the client sends a Close Request, and on 5.10
> no commands are sent.

I can reproduce this on Steve's current for-next branch but only against
a Samba server.

On Windows server, doing ^C kills cat properly but the output file is
never created, which is also a bug.

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [bug report] Inconsistent state with CIFS mount after interrupted process in Linux 5.10

[Pavel Shilovsky]

Thanks for reporting the issue.

The problem is with the recent fix which changes the error code from
-EINTR to -ERESTARTSYS:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/cifs/transport.c?id=6988a619f5b79e4efadea6e19dcfe75fbcd350b5

and this problem happens here:

https://git.samba.org/sfrench/?p=sfrench/cifs-2.6.git;a=blob;f=fs/cifs/smb2pdu.c;h=067eb44c7baa863c1e7ccd2c2f599be0b067f320;hb=236237ab6de1cde004b0ab3e348fc530334270d5#l3251

So, interrupted close commands don't get restarted by the client and
the client leaks open handles on the server. The offending patch was
tagged stable, so the fix seems quite urgent. The fix itself should be
simple: replace -EINTR with -ERESTARTSYS in the IF condition or even
amend it with "||".

Adding Paulo and Steve to comment.

--
Best regards,
Pavel Shilovsky

ср, 13 янв. 2021 г. в 04:31, Aurélien Aptel <aaptel@suse.com>:
>
> Duncan Findlay <duncf@duncf.ca> writes:
> > There seems to be a problem with the CIFS module in Linux 5.10. Files
> > that are opened and not cleanly closed end up in an inconsistent
> > state. This can be triggered by writing to a file and interrupting the
> > writer with Ctrl-C. Once this happens, attempting to delete the file
> > causes access to the mount to hang. Afterwards, the files are visible
> > to ls, but cannot be accessed or deleted.
> >
> > I'm running Debian unstable with a Debian unstable kernel
> > (5.10.5-1). I attempted to but could not reproduce this with a 4.19 kernel.
> >
> >
> > Repro steps:
> >
> > $ sudo mount -t cifs //test/share /mnt/test --verbose -o
> > rw,user,auto,nosuid,uid=user,gid=user,vers=3.1.1,credentials=/home/user/tmp/creds
> > $ mkdir /mnt/test/subdir
> > $ cat > /mnt/test/subdir/foo
> > [ Hit Ctrl-C to interrupt ]
> > $ ls /mnt/test/subdir/
> > foo
> > $ rm /mnt/test/subdir/foo
> > [ Hangs for 35 seconds, errors in dmesg log -- see below ]
> > $ ls /mnt/test/subdir/
> > foo
> > $ stat /mnt/test/subdir/foo
> > stat: cannot statx '/mnt/test/subdir/foo': No such file or directory
> >
> > At this point, the file still exists on the server side, and
> > restarting the server causes it to be deleted.
> >
> > I can provide pcaps if necessary. It looks like with 4.19, when the
> > cat command is killed, the client sends a Close Request, and on 5.10
> > no commands are sent.
>
> I can reproduce this on Steve's current for-next branch but only against
> a Samba server.
>
> On Windows server, doing ^C kills cat properly but the output file is
> never created, which is also a bug.
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
>

Re: [bug report] Inconsistent state with CIFS mount after interrupted process in Linux 5.10

[Paulo Alcantara]

Pavel Shilovsky <piastryyy@gmail.com> writes:

> Thanks for reporting the issue.
>
> The problem is with the recent fix which changes the error code from
> -EINTR to -ERESTARTSYS:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/cifs/transport.c?id=6988a619f5b79e4efadea6e19dcfe75fbcd350b5
>
> and this problem happens here:
>
> https://git.samba.org/sfrench/?p=sfrench/cifs-2.6.git;a=blob;f=fs/cifs/smb2pdu.c;h=067eb44c7baa863c1e7ccd2c2f599be0b067f320;hb=236237ab6de1cde004b0ab3e348fc530334270d5#l3251
>
> So, interrupted close commands don't get restarted by the client and
> the client leaks open handles on the server. The offending patch was
> tagged stable, so the fix seems quite urgent. The fix itself should be
> simple: replace -EINTR with -ERESTARTSYS in the IF condition or even
> amend it with "||".

Yes, makes sense.

Maybe we should do something like below

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 067eb44c7baa..794fc3b68b4f 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -3248,7 +3248,7 @@ __SMB2_close(const unsigned int xid, struct cifs_tcon *tcon,
 	free_rsp_buf(resp_buftype, rsp);
 
 	/* retry close in a worker thread if this one is interrupted */
-	if (rc == -EINTR) {
+	if (is_interrupt_error(rc)) {
 		int tmp_rc;
 
 		tmp_rc = smb2_handle_cancelled_close(tcon, persistent_fid,

Re: [bug report] Inconsistent state with CIFS mount after interrupted process in Linux 5.10

[Steve French]

On Wed, Jan 13, 2021 at 10:51 AM Paulo Alcantara <pc@cjr.nz> wrote:
>
> Pavel Shilovsky <piastryyy@gmail.com> writes:
>
> > Thanks for reporting the issue.
> >
> > The problem is with the recent fix which changes the error code from
> > -EINTR to -ERESTARTSYS:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/cifs/transport.c?id=6988a619f5b79e4efadea6e19dcfe75fbcd350b5
> >
> > and this problem happens here:
> >
> > https://git.samba.org/sfrench/?p=sfrench/cifs-2.6.git;a=blob;f=fs/cifs/smb2pdu.c;h=067eb44c7baa863c1e7ccd2c2f599be0b067f320;hb=236237ab6de1cde004b0ab3e348fc530334270d5#l3251
> >
> > So, interrupted close commands don't get restarted by the client and
> > the client leaks open handles on the server. The offending patch was
> > tagged stable, so the fix seems quite urgent. The fix itself should be
> > simple: replace -EINTR with -ERESTARTSYS in the IF condition or even
> > amend it with "||".
>
> Yes, makes sense.
>
> Maybe we should do something like below
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 067eb44c7baa..794fc3b68b4f 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -3248,7 +3248,7 @@ __SMB2_close(const unsigned int xid, struct cifs_tcon *tcon,
>         free_rsp_buf(resp_buftype, rsp);
>
>         /* retry close in a worker thread if this one is interrupted */
> -       if (rc == -EINTR) {
> +       if (is_interrupt_error(rc)) {
>                 int tmp_rc;
>
>                 tmp_rc = smb2_handle_cancelled_close(tcon, persistent_fid,

Seems reasonable, the other two conditions (ERESTARTRNOHAND e.g.) are
not explicitly set by cifs.ko but may come back from other libraries
so could be worth checking for, and it seems a little safer to use
is_interrupt_error.

Paulo,
If you can do a patch quickly I will run the buildbot on it and the
other two patches currently in for-next and try to send in the next
couple days.

(I do have a fourth patch, not currently in for-next, that I am
debugging ... the '\' handling patch ... which I can send if we can
figure out what is missing in it).  I may also include the two trivial
one line style patches recently submitted to list.

-- 
Thanks,

Steve

[PATCH] cifs: fix interrupted close commands

[Paulo Alcantara]

Retry close command if it gets interrupted to not leak open handles on
the server.

Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
Reported-by: Duncan Findlay <duncf@duncf.ca>
Suggested-by: Pavel Shilovsky <pshilov@microsoft.com>
Fixes: 6988a619f5b7 ("cifs: allow syscalls to be restarted in __smb_send_rqst()")
Cc: stable@vger.kernel.org
---
 fs/cifs/smb2pdu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 067eb44c7baa..794fc3b68b4f 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -3248,7 +3248,7 @@ __SMB2_close(const unsigned int xid, struct cifs_tcon *tcon,
 	free_rsp_buf(resp_buftype, rsp);
 
 	/* retry close in a worker thread if this one is interrupted */
-	if (rc == -EINTR) {
+	if (is_interrupt_error(rc)) {
 		int tmp_rc;
 
 		tmp_rc = smb2_handle_cancelled_close(tcon, persistent_fid,
-- 
2.29.2

Re: [PATCH] cifs: fix interrupted close commands

[Pavel Shilovsky]

ср, 13 янв. 2021 г. в 09:16, Paulo Alcantara <pc@cjr.nz>:
>
> Retry close command if it gets interrupted to not leak open handles on
> the server.
>
> Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> Reported-by: Duncan Findlay <duncf@duncf.ca>
> Suggested-by: Pavel Shilovsky <pshilov@microsoft.com>
> Fixes: 6988a619f5b7 ("cifs: allow syscalls to be restarted in __smb_send_rqst()")
> Cc: stable@vger.kernel.org
> ---
>  fs/cifs/smb2pdu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 067eb44c7baa..794fc3b68b4f 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -3248,7 +3248,7 @@ __SMB2_close(const unsigned int xid, struct cifs_tcon *tcon,
>         free_rsp_buf(resp_buftype, rsp);
>
>         /* retry close in a worker thread if this one is interrupted */
> -       if (rc == -EINTR) {
> +       if (is_interrupt_error(rc)) {
>                 int tmp_rc;
>
>                 tmp_rc = smb2_handle_cancelled_close(tcon, persistent_fid,
> --
> 2.29.2
>

Thanks for the fix!

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

--
Best regards,
Pavel Shilovsky

Re: [PATCH] cifs: fix interrupted close commands

[Shyam Prasad N]

Hi Paulo,

Does is_interrupt_error contain a list of all errors for which server
can leave the file handle open?
What about EAGAIN? I see that the server error STATUS_RETRY maps to EAGAIN.

Regards,
Shyam

On Thu, Jan 14, 2021 at 12:01 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> ср, 13 янв. 2021 г. в 09:16, Paulo Alcantara <pc@cjr.nz>:
> >
> > Retry close command if it gets interrupted to not leak open handles on
> > the server.
> >
> > Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> > Reported-by: Duncan Findlay <duncf@duncf.ca>
> > Suggested-by: Pavel Shilovsky <pshilov@microsoft.com>
> > Fixes: 6988a619f5b7 ("cifs: allow syscalls to be restarted in __smb_send_rqst()")
> > Cc: stable@vger.kernel.org
> > ---
> >  fs/cifs/smb2pdu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > index 067eb44c7baa..794fc3b68b4f 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -3248,7 +3248,7 @@ __SMB2_close(const unsigned int xid, struct cifs_tcon *tcon,
> >         free_rsp_buf(resp_buftype, rsp);
> >
> >         /* retry close in a worker thread if this one is interrupted */
> > -       if (rc == -EINTR) {
> > +       if (is_interrupt_error(rc)) {
> >                 int tmp_rc;
> >
> >                 tmp_rc = smb2_handle_cancelled_close(tcon, persistent_fid,
> > --
> > 2.29.2
> >
>
> Thanks for the fix!
>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> --
> Best regards,
> Pavel Shilovsky



-- 
-Shyam

RE: [EXTERNAL] Re: [PATCH] cifs: fix interrupted close commands

[Pavel Shilovskiy]

Hi Shyam,

Is_interrupt_error contains a list of errors that correspond to interrupted requests that haven't made it to the server.

Best regards,
Pavel Shilovsky

-----Original Message-----
From: Shyam Prasad N <nspmangalore@gmail.com> 
Sent: Wednesday, January 13, 2021 11:33 PM
To: Pavel Shilovsky <piastryyy@gmail.com>
Cc: pc <pc@cjr.nz>; linux-cifs <linux-cifs@vger.kernel.org>; Steve French <smfrench@gmail.com>; Aurelien Aptel <aaptel@suse.com>; Duncan Findlay <duncf@duncf.ca>; Pavel Shilovskiy <pshilov@microsoft.com>; Stable <stable@vger.kernel.org>
Subject: [EXTERNAL] Re: [PATCH] cifs: fix interrupted close commands

Hi Paulo,

Does is_interrupt_error contain a list of all errors for which server can leave the file handle open?
What about EAGAIN? I see that the server error STATUS_RETRY maps to EAGAIN.

Regards,
Shyam

On Thu, Jan 14, 2021 at 12:01 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> ср, 13 янв. 2021 г. в 09:16, Paulo Alcantara <pc@cjr.nz>:
> >
> > Retry close command if it gets interrupted to not leak open handles 
> > on the server.
> >
> > Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> > Reported-by: Duncan Findlay <duncf@duncf.ca>
> > Suggested-by: Pavel Shilovsky <pshilov@microsoft.com>
> > Fixes: 6988a619f5b7 ("cifs: allow syscalls to be restarted in 
> > __smb_send_rqst()")
> > Cc: stable@vger.kernel.org
> > ---
> >  fs/cifs/smb2pdu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 
> > 067eb44c7baa..794fc3b68b4f 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -3248,7 +3248,7 @@ __SMB2_close(const unsigned int xid, struct cifs_tcon *tcon,
> >         free_rsp_buf(resp_buftype, rsp);
> >
> >         /* retry close in a worker thread if this one is interrupted */
> > -       if (rc == -EINTR) {
> > +       if (is_interrupt_error(rc)) {
> >                 int tmp_rc;
> >
> >                 tmp_rc = smb2_handle_cancelled_close(tcon, 
> > persistent_fid,
> > --
> > 2.29.2
> >
>
> Thanks for the fix!
>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> --
> Best regards,
> Pavel Shilovsky



--
-Shyam

Re: [bug report] Inconsistent state with CIFS mount after interrupted process in Linux 5.10

[Pavel Shilovsky]

ср, 13 янв. 2021 г. в 09:03, Steve French <smfrench@gmail.com>:
>
> On Wed, Jan 13, 2021 at 10:51 AM Paulo Alcantara <pc@cjr.nz> wrote:
> >
> > Pavel Shilovsky <piastryyy@gmail.com> writes:
> >
> > > Thanks for reporting the issue.
> > >
> > > The problem is with the recent fix which changes the error code from
> > > -EINTR to -ERESTARTSYS:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/cifs/transport.c?id=6988a619f5b79e4efadea6e19dcfe75fbcd350b5
> > >
> > > and this problem happens here:
> > >
> > > https://git.samba.org/sfrench/?p=sfrench/cifs-2.6.git;a=blob;f=fs/cifs/smb2pdu.c;h=067eb44c7baa863c1e7ccd2c2f599be0b067f320;hb=236237ab6de1cde004b0ab3e348fc530334270d5#l3251
> > >
> > > So, interrupted close commands don't get restarted by the client and
> > > the client leaks open handles on the server. The offending patch was
> > > tagged stable, so the fix seems quite urgent. The fix itself should be
> > > simple: replace -EINTR with -ERESTARTSYS in the IF condition or even
> > > amend it with "||".
> >
> > Yes, makes sense.
> >
> > Maybe we should do something like below
> >
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > index 067eb44c7baa..794fc3b68b4f 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -3248,7 +3248,7 @@ __SMB2_close(const unsigned int xid, struct cifs_tcon *tcon,
> >         free_rsp_buf(resp_buftype, rsp);
> >
> >         /* retry close in a worker thread if this one is interrupted */
> > -       if (rc == -EINTR) {
> > +       if (is_interrupt_error(rc)) {
> >                 int tmp_rc;
> >
> >                 tmp_rc = smb2_handle_cancelled_close(tcon, persistent_fid,
>
> Seems reasonable, the other two conditions (ERESTARTRNOHAND e.g.) are
> not explicitly set by cifs.ko but may come back from other libraries
> so could be worth checking for, and it seems a little safer to use
> is_interrupt_error.
>
> Paulo,
> If you can do a patch quickly I will run the buildbot on it and the
> other two patches currently in for-next and try to send in the next
> couple days.
>
> (I do have a fourth patch, not currently in for-next, that I am
> debugging ... the '\' handling patch ... which I can send if we can
> figure out what is missing in it).  I may also include the two trivial
> one line style patches recently submitted to list.
>
> --
> Thanks,
>
> Steve

Paulo,
Yes, is_interrupt_error() is cleaner here to eliminate other potential
issues. Thanks!

Steve,
Agree - we should send this ASAP with other non-controversial patches.

--
Best regards,
Pavel Shilovsky

Re: [bug report] Inconsistent state with CIFS mount after interrupted process in Linux 5.10

[Aurélien Aptel]

Thanks Paulo and Pavel for the quick fix and review.

I've added an xfstest test for it on the buildbot. It's essentially
doing this:

    set -e
    rm -f x
    sleep 5 > x &
    pid=$!
    sleep 1
    kill $pid
    wait $pid &> /dev/null || true
    timeout 2 stat x > /dev/null
    timeout 2 rm  x

this fails without the fix

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [bug report] Inconsistent state with CIFS mount after interrupted process in Linux 5.10

[Pavel Shilovsky]

Great, thanks for adding the test!
--
Best regards,
Pavel Shilovsky

чт, 14 янв. 2021 г. в 07:21, Aurélien Aptel <aaptel@suse.com>:
>
> Thanks Paulo and Pavel for the quick fix and review.
>
> I've added an xfstest test for it on the buildbot. It's essentially
> doing this:
>
>     set -e
>     rm -f x
>     sleep 5 > x &
>     pid=$!
>     sleep 1
>     kill $pid
>     wait $pid &> /dev/null || true
>     timeout 2 stat x > /dev/null
>     timeout 2 rm  x
>
> this fails without the fix
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
>

Re: [bug report] Inconsistent state with CIFS mount after interrupted process in Linux 5.10

[Steve French]

Pavel's explanation looks plausible.


On Wed, Jan 13, 2021 at 10:31 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> Thanks for reporting the issue.
>
> The problem is with the recent fix which changes the error code from
> -EINTR to -ERESTARTSYS:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/cifs/transport.c?id=6988a619f5b79e4efadea6e19dcfe75fbcd350b5
>
> and this problem happens here:
>
> https://git.samba.org/sfrench/?p=sfrench/cifs-2.6.git;a=blob;f=fs/cifs/smb2pdu.c;h=067eb44c7baa863c1e7ccd2c2f599be0b067f320;hb=236237ab6de1cde004b0ab3e348fc530334270d5#l3251
>
> So, interrupted close commands don't get restarted by the client and
> the client leaks open handles on the server. The offending patch was
> tagged stable, so the fix seems quite urgent. The fix itself should be
> simple: replace -EINTR with -ERESTARTSYS in the IF condition or even
> amend it with "||".
>
> Adding Paulo and Steve to comment.
>
> --
> Best regards,
> Pavel Shilovsky
>
> ср, 13 янв. 2021 г. в 04:31, Aurélien Aptel <aaptel@suse.com>:
> >
> > Duncan Findlay <duncf@duncf.ca> writes:
> > > There seems to be a problem with the CIFS module in Linux 5.10. Files
> > > that are opened and not cleanly closed end up in an inconsistent
> > > state. This can be triggered by writing to a file and interrupting the
> > > writer with Ctrl-C. Once this happens, attempting to delete the file
> > > causes access to the mount to hang. Afterwards, the files are visible
> > > to ls, but cannot be accessed or deleted.
> > >
> > > I'm running Debian unstable with a Debian unstable kernel
> > > (5.10.5-1). I attempted to but could not reproduce this with a 4.19 kernel.
> > >
> > >
> > > Repro steps:
> > >
> > > $ sudo mount -t cifs //test/share /mnt/test --verbose -o
> > > rw,user,auto,nosuid,uid=user,gid=user,vers=3.1.1,credentials=/home/user/tmp/creds
> > > $ mkdir /mnt/test/subdir
> > > $ cat > /mnt/test/subdir/foo
> > > [ Hit Ctrl-C to interrupt ]
> > > $ ls /mnt/test/subdir/
> > > foo
> > > $ rm /mnt/test/subdir/foo
> > > [ Hangs for 35 seconds, errors in dmesg log -- see below ]
> > > $ ls /mnt/test/subdir/
> > > foo
> > > $ stat /mnt/test/subdir/foo
> > > stat: cannot statx '/mnt/test/subdir/foo': No such file or directory
> > >
> > > At this point, the file still exists on the server side, and
> > > restarting the server causes it to be deleted.
> > >
> > > I can provide pcaps if necessary. It looks like with 4.19, when the
> > > cat command is killed, the client sends a Close Request, and on 5.10
> > > no commands are sent.
> >
> > I can reproduce this on Steve's current for-next branch but only against
> > a Samba server.
> >
> > On Windows server, doing ^C kills cat properly but the output file is
> > never created, which is also a bug.
> >
> > Cheers,
> > --
> > Aurélien Aptel / SUSE Labs Samba Team
> > GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
> >



--
Thanks,

Steve