Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <30808b0b-367a-266a-7ef4-de69c08e1319@internode.on.net>
 <09396dca-3643-9a4b-070a-e7db2a07235e@internode.on.net> <CAHk-=wjj8SQx4YzS8yw7ZJJKiVLBY0g=d8rCSyPCM=8Pzmz+Zg@mail.gmail.com>
 <CAH2r5mvAv651DcX0--8oRYR8BXmBr8F=ymeBVDXm5YoQfcnK2A@mail.gmail.com>
In-Reply-To: <CAH2r5mvAv651DcX0--8oRYR8BXmBr8F=ymeBVDXm5YoQfcnK2A@mail.gmail.com>
From:   Steve French <smfrench@gmail.com>
Date:   Sun, 8 Dec 2019 21:27:46 -0600
Message-ID: <CAH2r5ms_BXSYRUxWvzNiwYHZFTqRoqo8gEfdz+ZcT1fYmLUgUg@mail.gmail.com>
Subject: Re: refcount_t: underflow; use-after-free with CIFS umount after
 scsi-misc commit ef2cc88e2a205b8a11a19e78db63a70d3728cdf5
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Arthur Marsh <arthur.marsh@internode.on.net>,
        SCSI development list <linux-scsi@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        CIFS <linux-cifs@vger.kernel.org>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        ronnie sahlberg <ronniesahlberg@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Arthur,
If you want to avoid the issue which causes the oops you can remove
this one line change (see below) but I (or Ronnie) will post a patch
for this - but wanted to discuss with him briefly first.

# git show 72e73c78c446e
commit 72e73c78c446e3c009a29b017c7fa3d79463e2aa
Author: Ronnie Sahlberg <lsahlber@redhat.com>
Date:   Thu Nov 7 17:00:38 2019 +1000

    cifs: close the shared root handle on tree disconnect

    Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
    Signed-off-by: Steve French <stfrench@microsoft.com>

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 05149862aea4..acb70f67efc9 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1807,6 +1807,8 @@ SMB2_tdis(const unsigned int xid, struct cifs_tcon *tcon)
        if ((tcon->need_reconnect) || (tcon->ses->need_reconnect))
                return 0;

+       close_shroot(&tcon->crfid);
+
        rc = smb2_plain_req_init(SMB2_TREE_DISCONNECT, tcon, (void **) &req,
                             &total_len);
        if (rc)

On Sun, Dec 8, 2019 at 9:18 PM Steve French <smfrench@gmail.com> wrote:
>
> On Sun, Dec 8, 2019 at 8:23 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Sun, Dec 8, 2019 at 5:49 PM Arthur Marsh
> > <arthur.marsh@internode.on.net> wrote:
> > >
> > > This still happens with 5.5.0-rc1:
> >
> > Does it happen 100% of the time?
>
> I can reproduce it (although it was a little more difficult since WiFi doesn't
> work on RC1 on some of my hardware - due to the 802.11 driver regression oops.
> I was able to reproduce it to Samba localhost).
>
>
> > Your bisection result looks pretty nonsensical - not that it's
> > impossible (anything is possible), but it really doesn't look very
> > likely. Which makes me think maybe it's slightly timing-sensitive or
> > something?
>
> The bisection result is implausible.  I just did some experiments and
> it looks far more likely is that it is related to commit
> 72e73c78c446e ("cifs: close the shared root handle on tree disconnect")
> so added Ronnie to the cc.  That patch added a call (at unmount time)
> to close_shroot.
> The idea of that patch made sense - although tree disconnect (and then
> logoff of the session)
> will indirectly free any open handles on the server for that session,
> it is a little
> cleaner to close the cached root SMB3 file handle explicitly.
>
> void close_shroot(struct cached_fid *cfid)
> {
>         mutex_lock(&cfid->fid_mutex);
>         kref_put(&cfid->refcount, smb2_close_cached_fid);
>         mutex_unlock(&cfid->fid_mutex);
> }
>
>
> Taking out the one line change in the patch from last week that calls
> close_shroot from
> umount (SMB2_tdis, ie tree_disconnect) I don't see the problem so far
> more likely
> that it is related to that commit.   The problem seems to be related
> to servers which
> don't support directory leases.  Will spin up a patch to fix this if
> Ronnie hasn't already fixed it


-- 
Thanks,

Steve