diff mbox series

[v2] ceph: fix deadlock or deadcode of misusing dget()

Message ID 20231117053627.716877-1-xiubli@redhat.com (mailing list archive)
State New, archived
Headers show
Series [v2] ceph: fix deadlock or deadcode of misusing dget() | expand

Commit Message

Xiubo Li Nov. 17, 2023, 5:36 a.m. UTC
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.

But since this deadcode is never used and the parent dir will always
be set from the callers, let's just remove it.

Reported-by: Al Viro <viro@zeniv.linux.org.uk>
URL: https://www.spinics.net/lists/ceph-devel/msg58622.html
Cc: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---


V2:
- Just remove the deadcode. Actually this just removed the deadcode,
  not fixing any deadlock issue and I also just removed ccing the
  stable list.


 fs/ceph/caps.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Jeff Layton Nov. 17, 2023, 10:39 a.m. UTC | #1
On Fri, 2023-11-17 at 13:36 +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.
> 
> But since this deadcode is never used and the parent dir will always
> be set from the callers, let's just remove it.
> 
> Reported-by: Al Viro <viro@zeniv.linux.org.uk>
> URL: https://www.spinics.net/lists/ceph-devel/msg58622.html
> Cc: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
> 
> 
> V2:
> - Just remove the deadcode. Actually this just removed the deadcode,
>   not fixing any deadlock issue and I also just removed ccing the
>   stable list.
> 
> 
>  fs/ceph/caps.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 284424659329..a9e19f1f26e0 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -4916,13 +4916,15 @@ 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;
>  	int force = 0;
>  	int ret;
>  
> +	/* This shouldn't happen */
> +	BUG_ON(!dir);
> +
>  	/*
>  	 * 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 +4934,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);

Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff mbox series

Patch

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index 284424659329..a9e19f1f26e0 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -4916,13 +4916,15 @@  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;
 	int force = 0;
 	int ret;
 
+	/* This shouldn't happen */
+	BUG_ON(!dir);
+
 	/*
 	 * 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 +4934,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);