[PATCH] ceph: fix potential NULL dereferenced issue in ceph_fill_trace()

[PATCH] ceph: fix potential NULL dereferenced issue in ceph_fill_trace()

[Viacheslav Dubeyko]

From: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

The Coverity Scan service has detected a potential dereference of
an explicit NULL value in ceph_fill_trace() [1].

The variable in is declared in the beggining of
ceph_fill_trace() [2]:

struct inode *in = NULL;

However, the initialization of the variable is happening under
condition [3]:

if (rinfo->head->is_target) {
    <skipped>
    in = req->r_target_inode;
    <skipped>
}

Potentially, if rinfo->head->is_target == FALSE, then
in variable continues to be NULL and later the dereference of
NULL value could happen in ceph_fill_trace() logic [4,5]:

else if ((req->r_op == CEPH_MDS_OP_LOOKUPSNAP ||
            req->r_op == CEPH_MDS_OP_MKSNAP) &&
            test_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags) &&
             !test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags)) {
<skipped>
     ihold(in);
     err = splice_dentry(&req->r_dentry, in);
     if (err < 0)
         goto done;
}

This patch adds the checking of in variable for NULL value
and it returns -EINVAL error code if it has NULL value.

[1] https://scan5.scan.coverity.com/#/project-view/64304/10063?selectedIssue=1141197
[2] https://elixir.bootlin.com/linux/v6.17-rc3/source/fs/ceph/inode.c#L1522
[3] https://elixir.bootlin.com/linux/v6.17-rc3/source/fs/ceph/inode.c#L1629
[4] https://elixir.bootlin.com/linux/v6.17-rc3/source/fs/ceph/inode.c#L1745
[5] https://elixir.bootlin.com/linux/v6.17-rc3/source/fs/ceph/inode.c#L1777

Signed-off-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
cc: Alex Markuze <amarkuze@redhat.com>
cc: Ilya Dryomov <idryomov@gmail.com>
cc: Ceph Development <ceph-devel@vger.kernel.org>
---
 fs/ceph/inode.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index fc543075b827..dee2793d822f 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1739,6 +1739,11 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
 			goto done;
 		}
 
+		if (!in) {
+			err = -EINVAL;
+			goto done;
+		}
+
 		/* attach proper inode */
 		if (d_really_is_negative(dn)) {
 			ceph_dir_clear_ordered(dir);
@@ -1774,6 +1779,12 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
 		doutc(cl, " linking snapped dir %p to dn %p\n", in,
 		      req->r_dentry);
 		ceph_dir_clear_ordered(dir);
+
+		if (!in) {
+			err = -EINVAL;
+			goto done;
+		}
+
 		ihold(in);
 		err = splice_dentry(&req->r_dentry, in);
 		if (err < 0)
-- 
2.51.0

Re: [PATCH] ceph: fix potential NULL dereferenced issue in ceph_fill_trace()

[Alex Markuze]

Considering we hadn't seen any related issues, I would add an unlikely
macro for that if.

On Wed, Aug 27, 2025 at 10:02 PM Viacheslav Dubeyko <slava@dubeyko.com> wrote:
>
> From: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
>
> The Coverity Scan service has detected a potential dereference of
> an explicit NULL value in ceph_fill_trace() [1].
>
> The variable in is declared in the beggining of
> ceph_fill_trace() [2]:
>
> struct inode *in = NULL;
>
> However, the initialization of the variable is happening under
> condition [3]:
>
> if (rinfo->head->is_target) {
>     <skipped>
>     in = req->r_target_inode;
>     <skipped>
> }
>
> Potentially, if rinfo->head->is_target == FALSE, then
> in variable continues to be NULL and later the dereference of
> NULL value could happen in ceph_fill_trace() logic [4,5]:
>
> else if ((req->r_op == CEPH_MDS_OP_LOOKUPSNAP ||
>             req->r_op == CEPH_MDS_OP_MKSNAP) &&
>             test_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags) &&
>              !test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags)) {
> <skipped>
>      ihold(in);
>      err = splice_dentry(&req->r_dentry, in);
>      if (err < 0)
>          goto done;
> }
>
> This patch adds the checking of in variable for NULL value
> and it returns -EINVAL error code if it has NULL value.
>
> [1] https://scan5.scan.coverity.com/#/project-view/64304/10063?selectedIssue=1141197
> [2] https://elixir.bootlin.com/linux/v6.17-rc3/source/fs/ceph/inode.c#L1522
> [3] https://elixir.bootlin.com/linux/v6.17-rc3/source/fs/ceph/inode.c#L1629
> [4] https://elixir.bootlin.com/linux/v6.17-rc3/source/fs/ceph/inode.c#L1745
> [5] https://elixir.bootlin.com/linux/v6.17-rc3/source/fs/ceph/inode.c#L1777
>
> Signed-off-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> cc: Alex Markuze <amarkuze@redhat.com>
> cc: Ilya Dryomov <idryomov@gmail.com>
> cc: Ceph Development <ceph-devel@vger.kernel.org>
> ---
>  fs/ceph/inode.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index fc543075b827..dee2793d822f 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1739,6 +1739,11 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
>                         goto done;
>                 }
>
> +               if (!in) {
> +                       err = -EINVAL;
> +                       goto done;
> +               }
> +
>                 /* attach proper inode */
>                 if (d_really_is_negative(dn)) {
>                         ceph_dir_clear_ordered(dir);
> @@ -1774,6 +1779,12 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
>                 doutc(cl, " linking snapped dir %p to dn %p\n", in,
>                       req->r_dentry);
>                 ceph_dir_clear_ordered(dir);
> +
> +               if (!in) {
> +                       err = -EINVAL;
> +                       goto done;
> +               }
> +
>                 ihold(in);
>                 err = splice_dentry(&req->r_dentry, in);
>                 if (err < 0)
> --
> 2.51.0
>

Re: [PATCH] ceph: fix potential NULL dereferenced issue in ceph_fill_trace()

[Viacheslav Dubeyko]

On Thu, 2025-08-28 at 12:28 +0300, Alex Markuze wrote:
> Considering we hadn't seen any related issues, I would add an
> unlikely
> macro for that if.
> 

Makes sense to me. Let me rework the patch.

Thanks,
Slava.