Re: [bug report] ceph: add support for encrypted snapshot names

["Luís Henriques" <lhenriques@suse.de>]

Dan Carpenter <dan.carpenter@linaro.org> writes:

> Hello Luís Henriques,
>
> The patch dd66df0053ef: "ceph: add support for encrypted snapshot
> names" from Aug 25, 2022 (linux-next), leads to the following Smatch
> static checker warning:
>
> 	fs/ceph/crypto.c:252 parse_longname()
> 	warn: 'dir' is an error pointer or valid
>
> fs/ceph/crypto.c
>     211 static struct inode *parse_longname(const struct inode *parent,
>     212                                     const char *name, int *name_len)
>     213 {
>     214         struct inode *dir = NULL;
>     215         struct ceph_vino vino = { .snap = CEPH_NOSNAP };
>     216         char *inode_number;
>     217         char *name_end;
>     218         int orig_len = *name_len;
>     219         int ret = -EIO;
>     220 
>     221         /* Skip initial '_' */
>     222         name++;
>     223         name_end = strrchr(name, '_');
>     224         if (!name_end) {
>     225                 dout("Failed to parse long snapshot name: %s\n", name);
>     226                 return ERR_PTR(-EIO);
>     227         }
>     228         *name_len = (name_end - name);
>     229         if (*name_len <= 0) {
>     230                 pr_err("Failed to parse long snapshot name\n");
>     231                 return ERR_PTR(-EIO);
>     232         }
>     233 
>     234         /* Get the inode number */
>     235         inode_number = kmemdup_nul(name_end + 1,
>     236                                    orig_len - *name_len - 2,
>     237                                    GFP_KERNEL);
>     238         if (!inode_number)
>     239                 return ERR_PTR(-ENOMEM);
>     240         ret = kstrtou64(inode_number, 10, &vino.ino);
>     241         if (ret) {
>     242                 dout("Failed to parse inode number: %s\n", name);
>     243                 dir = ERR_PTR(ret);
>     244                 goto out;
>     245         }
>     246 
>     247         /* And finally the inode */
>     248         dir = ceph_find_inode(parent->i_sb, vino);
>     249         if (!dir) {
>     250                 /* This can happen if we're not mounting cephfs on the root */
>     251                 dir = ceph_get_inode(parent->i_sb, vino, NULL);
> --> 252                 if (!dir)
>     253                         dir = ERR_PTR(-ENOENT);
>
> This never returns NULL.  If it were tempted to return NULL then it
> returns -ENOMEM instead.

Oops, you're right.  But the fix should be as easy as removing that 'if'
statement and let this function simply return 'dir' as-is.

I'll send out an updated version of the patch in reply to this email.
Thanks for the report, Dan.

Cheers,
-- 
Luís

>
>     254         }
>     255         if (IS_ERR(dir))
>     256                 dout("Can't find inode %s (%s)\n", inode_number, name);
>     257 
>     258 out:
>     259         kfree(inode_number);
>     260         return dir;
>     261 }
>
> regards,
> dan carpenter