| Message ID | 20231117031951.710177-1-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: fix deadlock or deadcode of misusing dget() | expand |
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);
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
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c index 284424659329..6f7a56a800c2 100644 --- a/fs/ceph/caps.c +++ b/fs/ceph/caps.c @@ -4923,6 +4923,11 @@ int ceph_encode_dentry_release(void **p, struct dentry *dentry, int force = 0; int ret; + if (!dir) { + parent = dget_parent(dentry); + dir = d_inode(parent); + } + /* * force an record for the directory caps if we have a dentry lease. * this is racy (can't take i_ceph_lock and d_lock together), but it @@ -4932,14 +4937,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); @@ -4952,8 +4952,10 @@ int ceph_encode_dentry_release(void **p, struct dentry *dentry, if (IS_ENCRYPTED(dir) && fscrypt_has_encryption_key(dir)) { int ret2 = ceph_encode_encrypted_fname(dir, dentry, *p); - if (ret2 < 0) + if (ret2 < 0) { + dput(parent); return ret2; + } rel->dname_len = cpu_to_le32(ret2); *p += ret2; @@ -4965,6 +4967,8 @@ int ceph_encode_dentry_release(void **p, struct dentry *dentry, } else { spin_unlock(&dentry->d_lock); } + + dput(parent); return ret; }