linux-kernel - Re: [PATCH 2/2] ceph/inode: drop extra reference from ceph_get_reply_dir() in ceph_fill

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1e0d2fb235e306fac4b003fc62c961405aef7c6b.camel@ibm.com>
Date: Wed, 3 Sep 2025 18:51:48 +0000
From: Viacheslav Dubeyko <Slava.Dubeyko@....com>
To: Alex Markuze <amarkuze@...hat.com>,
        "ceph-devel@...r.kernel.org"
	<ceph-devel@...r.kernel.org>
CC: "idryomov@...il.com" <idryomov@...il.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re:  [PATCH 2/2] ceph/inode: drop extra reference from
 ceph_get_reply_dir() in ceph_fill_trace()

On Mon, 2025-09-01 at 15:14 +0000, Alex Markuze wrote:
> ceph_get_reply_dir() may return a different, referenced inode when r_parent is stale and the parent directory lock is not held.
> ceph_fill_trace() used that inode but failed to drop the reference when it differed from req->r_parent, leaking an inode reference.
> 
> Keep the directory inode in a local and iput() it at function end if it does not match req->r_parent.
> 
> Signed-off-by: Alex Markuze <amarkuze@...hat.com>
> ---
>  fs/ceph/inode.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 470ee595ecf2..439c08ece283 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1585,6 +1585,7 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
>  	struct ceph_fs_client *fsc = ceph_sb_to_fs_client(sb);
>  	struct ceph_client *cl = fsc->client;
>  	int err = 0;
> +	struct inode *dir = NULL;

Probably, we need to declare the dir pointer before err declaration. What do you
think?

>  
>  	doutc(cl, "%p is_dentry %d is_target %d\n", req,
>  	      rinfo->head->is_dentry, rinfo->head->is_target);
> @@ -1601,7 +1602,11 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
>  		 * r_parent may be stale, in cases when R_PARENT_LOCKED is not set,
>  		 * so we need to get the correct inode
>  		 */
> -		struct inode *dir = ceph_get_reply_dir(sb, req->r_parent, rinfo);
> +		dir = ceph_get_reply_dir(sb, req->r_parent, rinfo);
> +		if (IS_ERR(dir)) {
> +			err = PTR_ERR(dir);
> +			goto done;
> +		}
>  		if (dir) {
>  			err = ceph_fill_inode(dir, NULL, &rinfo->diri,
>  					      rinfo->dirfrag, session, -1,
> @@ -1869,6 +1874,9 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
>  					    &dvino, ptvino);
>  	}
>  done:
> +	/* Drop extra ref from ceph_get_reply_dir() if it returned a new inode */
> +	if (!IS_ERR(dir) && dir && dir != req->r_parent)

I think it makes sense to check the dir on NULL at first, then on error.
Maybe, we need to name dir variable as parent_dir or simply parent?

Thanks,
Slava.

> +		iput(dir);
>  	doutc(cl, "done err=%d\n", err);
>  	return err;
>  }