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

[Dan Carpenter <dan.carpenter@linaro.org>]

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

Using likely/unlikely() should only be done if there is a reason to
think it will affect benchmarking data.  Otherwise, you're just making
the code messy for no reason.

> 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;

Most of the time, these sorts of NULL initializers are just there
to silence "uninitialize variable" warnings when the code is too
complicated for GCC to parse.

> >
> > 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>

If this is really a bug it should have a Fixes tag.

> > 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;
> >                 }

This "goto done;" is from the if (!rinfo->head->is_target) { test,
so we know

> >
> > +               if (!in) {

that this NULL check is not required.

regards,
dan carpenter

> > +                       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
> >
>