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 --- fs/ceph/caps.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
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; }
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
linux-stable-mirror@lists.linaro.org