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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



ср, 13 янв. 2021 г. в 09:03, Steve French <smfrench@xxxxxxxxx>:
>
> On Wed, Jan 13, 2021 at 10:51 AM Paulo Alcantara <pc@xxxxxx> wrote:
> >
> > Pavel Shilovsky <piastryyy@xxxxxxxxx> 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




[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux