| Message ID | CAH2r5muJSARbGJ4cOZoGy32mCtUTG9wyEyw8aF06zexshAmqfQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | smb3: fix refcount underflow warning on unmount when no directory leases | expand |
пн, 9 дек. 2019 г. в 20:48, Steve French <smfrench@gmail.com>: > > Fix refcount underflow warning when unmounting to servers which didn't grant > directory leases. > > [ 301.680095] refcount_t: underflow; use-after-free. > [ 301.680192] WARNING: CPU: 1 PID: 3569 at lib/refcount.c:28 > refcount_warn_saturate+0xb4/0xf3 > ... > [ 301.682139] Call Trace: > [ 301.682240] close_shroot+0x97/0xda [cifs] > [ 301.682351] SMB2_tdis+0x7c/0x176 [cifs] > [ 301.682456] ? _get_xid+0x58/0x91 [cifs] > [ 301.682563] cifs_put_tcon.part.0+0x99/0x202 [cifs] > [ 301.682637] ? ida_free+0x99/0x10a > [ 301.682727] ? cifs_umount+0x3d/0x9d [cifs] > [ 301.682829] cifs_put_tlink+0x3a/0x50 [cifs] > [ 301.682929] cifs_umount+0x44/0x9d [cifs] > > Fixes: 72e73c78c446 ("cifs: close the shared root handle on tree disconnect") > > Signed-off-by: Steve French <stfrench@microsoft.com> > Acked-by: Ronnie Sahlberg <lsahlber@redhat.com> > Reviewed-by: Aurelien Aptel <aaptel@suse.com> > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> > Reported-and-tested-by: Arthur Marsh <arthur.marsh@internode.on.net> > > -- > Thanks, > > Steve Looking at this more, I think that the fact that the handle is valid doesn't mean that it has a directory lease. So, I think we need to track that fact separately. I coded a quick follow-on fix (untested) to describe my idea - see the attached patch. Thoughts? -- Best regards, Pavel Shilovsky
This makes sense. Thanks for this patch. Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com> On Wed, Dec 11, 2019 at 5:53 AM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > пн, 9 дек. 2019 г. в 20:48, Steve French <smfrench@gmail.com>: > > > > Fix refcount underflow warning when unmounting to servers which didn't grant > > directory leases. > > > > [ 301.680095] refcount_t: underflow; use-after-free. > > [ 301.680192] WARNING: CPU: 1 PID: 3569 at lib/refcount.c:28 > > refcount_warn_saturate+0xb4/0xf3 > > ... > > [ 301.682139] Call Trace: > > [ 301.682240] close_shroot+0x97/0xda [cifs] > > [ 301.682351] SMB2_tdis+0x7c/0x176 [cifs] > > [ 301.682456] ? _get_xid+0x58/0x91 [cifs] > > [ 301.682563] cifs_put_tcon.part.0+0x99/0x202 [cifs] > > [ 301.682637] ? ida_free+0x99/0x10a > > [ 301.682727] ? cifs_umount+0x3d/0x9d [cifs] > > [ 301.682829] cifs_put_tlink+0x3a/0x50 [cifs] > > [ 301.682929] cifs_umount+0x44/0x9d [cifs] > > > > Fixes: 72e73c78c446 ("cifs: close the shared root handle on tree disconnect") > > > > Signed-off-by: Steve French <stfrench@microsoft.com> > > Acked-by: Ronnie Sahlberg <lsahlber@redhat.com> > > Reviewed-by: Aurelien Aptel <aaptel@suse.com> > > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> > > Reported-and-tested-by: Arthur Marsh <arthur.marsh@internode.on.net> > > > > -- > > Thanks, > > > > Steve > > Looking at this more, I think that the fact that the handle is valid > doesn't mean that it has a directory lease. So, I think we need to > track that fact separately. I coded a quick follow-on fix (untested) > to describe my idea - see the attached patch. > > Thoughts? > > -- > Best regards, > Pavel Shilovsky
tentatively merged into cifs-2.6.git for-next pending more testing On Thu, Dec 12, 2019 at 1:36 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > > This makes sense. Thanks for this patch. > > Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com> > > On Wed, Dec 11, 2019 at 5:53 AM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > > > пн, 9 дек. 2019 г. в 20:48, Steve French <smfrench@gmail.com>: > > > > > > Fix refcount underflow warning when unmounting to servers which didn't grant > > > directory leases. > > > > > > [ 301.680095] refcount_t: underflow; use-after-free. > > > [ 301.680192] WARNING: CPU: 1 PID: 3569 at lib/refcount.c:28 > > > refcount_warn_saturate+0xb4/0xf3 > > > ... > > > [ 301.682139] Call Trace: > > > [ 301.682240] close_shroot+0x97/0xda [cifs] > > > [ 301.682351] SMB2_tdis+0x7c/0x176 [cifs] > > > [ 301.682456] ? _get_xid+0x58/0x91 [cifs] > > > [ 301.682563] cifs_put_tcon.part.0+0x99/0x202 [cifs] > > > [ 301.682637] ? ida_free+0x99/0x10a > > > [ 301.682727] ? cifs_umount+0x3d/0x9d [cifs] > > > [ 301.682829] cifs_put_tlink+0x3a/0x50 [cifs] > > > [ 301.682929] cifs_umount+0x44/0x9d [cifs] > > > > > > Fixes: 72e73c78c446 ("cifs: close the shared root handle on tree disconnect") > > > > > > Signed-off-by: Steve French <stfrench@microsoft.com> > > > Acked-by: Ronnie Sahlberg <lsahlber@redhat.com> > > > Reviewed-by: Aurelien Aptel <aaptel@suse.com> > > > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> > > > Reported-and-tested-by: Arthur Marsh <arthur.marsh@internode.on.net> > > > > > > -- > > > Thanks, > > > > > > Steve > > > > Looking at this more, I think that the fact that the handle is valid > > doesn't mean that it has a directory lease. So, I think we need to > > track that fact separately. I coded a quick follow-on fix (untested) > > to describe my idea - see the attached patch. > > > > Thoughts? > > > > -- > > Best regards, > > Pavel Shilovsky
From 281393894af9cc3f9483204475014e89d728987c Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Mon, 9 Dec 2019 19:47:10 -0600
Subject: [PATCH 1/2] smb3: fix refcount underflow warning on unmount when no
directory leases
Fix refcount underflow warning when unmounting to servers which didn't grant
directory leases.
[ 301.680095] refcount_t: underflow; use-after-free.
[ 301.680192] WARNING: CPU: 1 PID: 3569 at lib/refcount.c:28
refcount_warn_saturate+0xb4/0xf3
...
[ 301.682139] Call Trace:
[ 301.682240] close_shroot+0x97/0xda [cifs]
[ 301.682351] SMB2_tdis+0x7c/0x176 [cifs]
[ 301.682456] ? _get_xid+0x58/0x91 [cifs]
[ 301.682563] cifs_put_tcon.part.0+0x99/0x202 [cifs]
[ 301.682637] ? ida_free+0x99/0x10a
[ 301.682727] ? cifs_umount+0x3d/0x9d [cifs]
[ 301.682829] cifs_put_tlink+0x3a/0x50 [cifs]
[ 301.682929] cifs_umount+0x44/0x9d [cifs]
Fixes: 72e73c78c446 ("cifs: close the shared root handle on tree disconnect")
Signed-off-by: Steve French <stfrench@microsoft.com>
Acked-by: Ronnie Sahlberg <lsahlber@redhat.com>
Reviewed-by: Aurelien Aptel <aaptel@suse.com>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
Reported-and-tested-by: Arthur Marsh <arthur.marsh@internode.on.net>
---
fs/cifs/smb2pdu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 0ab6b1200288..d2658f51ff60 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1847,7 +1847,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);
+ if (tcon->crfid.is_valid)
+ close_shroot(&tcon->crfid);
rc = smb2_plain_req_init(SMB2_TREE_DISCONNECT, tcon, (void **) &req,
&total_len);
--
2.23.0
Fix refcount underflow warning when unmounting to servers which didn't grant directory leases. [ 301.680095] refcount_t: underflow; use-after-free. [ 301.680192] WARNING: CPU: 1 PID: 3569 at lib/refcount.c:28 refcount_warn_saturate+0xb4/0xf3 ... [ 301.682139] Call Trace: [ 301.682240] close_shroot+0x97/0xda [cifs] [ 301.682351] SMB2_tdis+0x7c/0x176 [cifs] [ 301.682456] ? _get_xid+0x58/0x91 [cifs] [ 301.682563] cifs_put_tcon.part.0+0x99/0x202 [cifs] [ 301.682637] ? ida_free+0x99/0x10a [ 301.682727] ? cifs_umount+0x3d/0x9d [cifs] [ 301.682829] cifs_put_tlink+0x3a/0x50 [cifs] [ 301.682929] cifs_umount+0x44/0x9d [cifs] Fixes: 72e73c78c446 ("cifs: close the shared root handle on tree disconnect") Signed-off-by: Steve French <stfrench@microsoft.com> Acked-by: Ronnie Sahlberg <lsahlber@redhat.com> Reviewed-by: Aurelien Aptel <aaptel@suse.com> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> Reported-and-tested-by: Arthur Marsh <arthur.marsh@internode.on.net>