Re: [PATCH 2/2] ceph/inode: drop extra reference from ceph_get_reply_dir() in ceph_fill_trace()

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

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@redhat.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;
>  }