From: Xiubo Li <xiubli@redhat.com>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: ceph-devel@vger.kernel.org, idryomov@gmail.com,
jlayton@kernel.org, vshankar@redhat.com, mchangir@redhat.com,
stable@vger.kernel.org
Subject: Re: [PATCH] ceph: fix deadlock or deadcode of misusing dget()
Date: Fri, 17 Nov 2023 13:20:06 +0800 [thread overview]
Message-ID: <539fee36-cbd2-bb9b-b983-130a60bd0171@redhat.com> (raw)
In-Reply-To: <20231117044546.GC1957730@ZenIV>
On 11/17/23 12:45, Al Viro wrote:
> On Fri, Nov 17, 2023 at 11:19:51AM +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> The lock order is incorrect between denty and its parent, we should
>> always make sure that the parent get the lock first.
>>
>> Switch to use the 'dget_parent()' to get the parent dentry and also
>> keep holding the parent until the corresponding inode is not being
>> refereenced.
>>
>> Cc: stable@vger.kernel.org
>> Reported-by: Al Viro <viro@zeniv.linux.org.uk>
>> URL: https://www.spinics.net/lists/ceph-devel/msg58622.html
>> Fixes: adf0d68701c7 ("ceph: fix unsafe dcache access in ceph_encode_dentry_release")
>> Cc: Jeff Layton <jlayton@kernel.org>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> + if (!dir) {
>> + parent = dget_parent(dentry);
>> + dir = d_inode(parent);
>> + }
> It's never actually called with dir == NULL. Not since 2016.
>
> All you need is this, _maybe_ with BUG_ON(!dir); added in the beginning of the function.
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 2c0b8dc3dd0d..22d6ea97938f 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -4887,7 +4887,6 @@ int ceph_encode_dentry_release(void **p, struct dentry *dentry,
> struct inode *dir,
> int mds, int drop, int unless)
> {
> - struct dentry *parent = NULL;
> struct ceph_mds_request_release *rel = *p;
> struct ceph_dentry_info *di = ceph_dentry(dentry);
> struct ceph_client *cl;
> @@ -4903,14 +4902,9 @@ int ceph_encode_dentry_release(void **p, struct dentry *dentry,
> spin_lock(&dentry->d_lock);
> if (di->lease_session && di->lease_session->s_mds == mds)
> force = 1;
> - if (!dir) {
> - parent = dget(dentry->d_parent);
> - dir = d_inode(parent);
> - }
> spin_unlock(&dentry->d_lock);
>
> ret = ceph_encode_inode_release(p, dir, mds, drop, unless, force);
> - dput(parent);
>
> cl = ceph_inode_to_client(dir);
> spin_lock(&dentry->d_lock);
Yeah, you are right.
Will update it.
Thanks
- Xiubo
prev parent reply other threads:[~2023-11-17 5:20 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-17 3:19 [PATCH] ceph: fix deadlock or deadcode of misusing dget() xiubli
2023-11-17 4:45 ` Al Viro
2023-11-17 5:20 ` Xiubo Li [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=539fee36-cbd2-bb9b-b983-130a60bd0171@redhat.com \
--to=xiubli@redhat.com \
--cc=ceph-devel@vger.kernel.org \
--cc=idryomov@gmail.com \
--cc=jlayton@kernel.org \
--cc=mchangir@redhat.com \
--cc=stable@vger.kernel.org \
--cc=viro@zeniv.linux.org.uk \
--cc=vshankar@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).