* [PATCH] smb3: fix refcount underflow warning on unmount when no directory leases
@ 2019-12-10 4:48 Steve French
2019-12-10 19:53 ` Pavel Shilovsky
0 siblings, 1 reply; 4+ messages in thread
From: Steve French @ 2019-12-10 4:48 UTC (permalink / raw)
To: CIFS; +Cc: SCSI development list, Arthur Marsh
[-- Attachment #1: Type: text/plain, Size: 1012 bytes --]
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
[-- Attachment #2: 0001-smb3-fix-refcount-underflow-warning-on-unmount-when-.patch --]
[-- Type: text/x-patch, Size: 1811 bytes --]
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] smb3: fix refcount underflow warning on unmount when no directory leases 2019-12-10 4:48 [PATCH] smb3: fix refcount underflow warning on unmount when no directory leases Steve French @ 2019-12-10 19:53 ` Pavel Shilovsky 2019-12-12 19:36 ` ronnie sahlberg 0 siblings, 1 reply; 4+ messages in thread From: Pavel Shilovsky @ 2019-12-10 19:53 UTC (permalink / raw) To: Steve French, ronnie sahlberg; +Cc: CIFS [-- Attachment #1: Type: text/plain, Size: 1475 bytes --] пн, 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 [-- Attachment #2: 0001-CIFS-Close-cached-root-handle-only-if-it-has-a-lease.patch --] [-- Type: application/octet-stream, Size: 4566 bytes --] From 9e576ff356e78debac207ccc947725ebf1801c4f Mon Sep 17 00:00:00 2001 From: Pavel Shilovsky <pshilov@microsoft.com> Date: Tue, 10 Dec 2019 11:44:52 -0800 Subject: [PATCH] CIFS: Close cached root handle only if it has a lease SMB2_tdis() checks if a root handle is valid in order to decide whether it needs to close the handle or not. However if another thread has reference for the handle, it may end up with putting the reference twice. The extra reference that we want to put during the tree disconnect is the reference that has a directory lease. So, track the fact that we have a directory lease and close the handle only in that case. Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com> --- fs/cifs/cifsglob.h | 2 +- fs/cifs/cifssmb.c | 3 +++ fs/cifs/smb2ops.c | 19 ++++++++++++++++++- fs/cifs/smb2pdu.c | 3 +-- fs/cifs/smb2proto.h | 2 ++ 5 files changed, 25 insertions(+), 4 deletions(-) diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 39ca1990eb1d..b48702b9f05e 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -1003,7 +1003,7 @@ cap_unix(struct cifs_ses *ses) struct cached_fid { bool is_valid:1; /* Do we have a useable root fid */ bool file_all_info_is_valid:1; - + bool has_lease:1; struct kref refcount; struct cifs_fid *fid; struct mutex fid_mutex; diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index 3907653e63c7..a535c6a9d20c 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -42,6 +42,7 @@ #include "cifsproto.h" #include "cifs_unicode.h" #include "cifs_debug.h" +#include "smb2proto.h" #include "fscache.h" #include "smbdirect.h" #ifdef CONFIG_CIFS_DFS_UPCALL @@ -112,6 +113,8 @@ cifs_mark_open_files_invalid(struct cifs_tcon *tcon) mutex_lock(&tcon->crfid.fid_mutex); tcon->crfid.is_valid = false; + /* cached handle is not valid, so SMB2_CLOSE won't be sent below */ + close_shroot_lease_locked(&tcon->crfid); memset(tcon->crfid.fid, 0, sizeof(struct cifs_fid)); mutex_unlock(&tcon->crfid.fid_mutex); diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 5a13687bf547..2dbf6aad11df 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -603,6 +603,7 @@ smb2_close_cached_fid(struct kref *ref) cfid->fid->volatile_fid); cfid->is_valid = false; cfid->file_all_info_is_valid = false; + cfid->has_lease = false; } } @@ -613,13 +614,28 @@ void close_shroot(struct cached_fid *cfid) mutex_unlock(&cfid->fid_mutex); } +void close_shroot_lease_locked(struct cached_fid *cfid) +{ + if (cfid->has_lease) { + cfid->has_lease = false; + kref_put(&cfid->refcount, smb2_close_cached_fid); + } +} + +void close_shroot_lease(struct cached_fid *cfid) +{ + mutex_lock(&cfid->fid_mutex); + close_shroot_lease_locked(cfid); + mutex_unlock(&cfid->fid_mutex); +} + void smb2_cached_lease_break(struct work_struct *work) { struct cached_fid *cfid = container_of(work, struct cached_fid, lease_break); - close_shroot(cfid); + close_shroot_lease(cfid); } /* @@ -754,6 +770,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) /* BB TBD check to see if oplock level check can be removed below */ if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) { kref_get(&tcon->crfid.refcount); + tcon->crfid.has_lease = true; smb2_parse_contexts(server, o_rsp, &oparms.fid->epoch, oparms.fid->lease_key, &oplock, NULL); diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index f4a0f6345386..60f98c2a3f22 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -1804,8 +1804,7 @@ SMB2_tdis(const unsigned int xid, struct cifs_tcon *tcon) if ((tcon->need_reconnect) || (tcon->ses->need_reconnect)) return 0; - if (tcon->crfid.is_valid) - close_shroot(&tcon->crfid); + close_shroot_lease(&tcon->crfid); rc = smb2_plain_req_init(SMB2_TREE_DISCONNECT, tcon, (void **) &req, &total_len); diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h index e239f98093a9..82f8daa8c590 100644 --- a/fs/cifs/smb2proto.h +++ b/fs/cifs/smb2proto.h @@ -69,6 +69,8 @@ extern int smb3_handle_read_data(struct TCP_Server_Info *server, extern int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid); extern void close_shroot(struct cached_fid *cfid); +extern void close_shroot_lease(struct cached_fid *cfid); +extern void close_shroot_lease_locked(struct cached_fid *cfid); extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src); extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, -- 2.17.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] smb3: fix refcount underflow warning on unmount when no directory leases 2019-12-10 19:53 ` Pavel Shilovsky @ 2019-12-12 19:36 ` ronnie sahlberg 2019-12-12 23:08 ` Steve French 0 siblings, 1 reply; 4+ messages in thread From: ronnie sahlberg @ 2019-12-12 19:36 UTC (permalink / raw) To: Pavel Shilovsky; +Cc: Steve French, CIFS 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] smb3: fix refcount underflow warning on unmount when no directory leases 2019-12-12 19:36 ` ronnie sahlberg @ 2019-12-12 23:08 ` Steve French 0 siblings, 0 replies; 4+ messages in thread From: Steve French @ 2019-12-12 23:08 UTC (permalink / raw) To: ronnie sahlberg; +Cc: Pavel Shilovsky, CIFS 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 -- Thanks, Steve ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-12-12 23:08 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-10 4:48 [PATCH] smb3: fix refcount underflow warning on unmount when no directory leases Steve French 2019-12-10 19:53 ` Pavel Shilovsky 2019-12-12 19:36 ` ronnie sahlberg 2019-12-12 23:08 ` Steve French
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).