On Wed, May 24, 2023 at 5:06 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Wed 24-05-23 11:38:17, Amir Goldstein wrote:
> > On Wed, May 24, 2023 at 9:34 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > >
> > > Hello Amir Goldstein,
> > >
> > > The patch 7ba39960c7f3: "fanotify: support reporting non-decodeable
> > > file handles" from May 2, 2023, leads to the following Smatch static
> > > checker warning:
> > >
> > > fs/notify/fanotify/fanotify.c:451 fanotify_encode_fh()
> > > warn: assigning signed to unsigned: 'fh->type = type' 's32min-(-1),1-254,256-s32max'
> > >
> > > (unpublished garbage Smatch check).
> > >
> > > fs/notify/fanotify/fanotify.c
> > > 403 static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
> > > 404 unsigned int fh_len, unsigned int *hash,
> > > 405 gfp_t gfp)
> > > 406 {
> > > 407 int dwords, type = 0;
> > > 408 char *ext_buf = NULL;
> > > 409 void *buf = fh->buf;
> > > 410 int err;
> > > 411
> > > 412 fh->type = FILEID_ROOT;
> > > 413 fh->len = 0;
> > > 414 fh->flags = 0;
> > > 415
> > > 416 /*
> > > 417 * Invalid FHs are used by FAN_FS_ERROR for errors not
> > > 418 * linked to any inode. The f_handle won't be reported
> > > 419 * back to userspace.
> > > 420 */
> > > 421 if (!inode)
> > > 422 goto out;
> > > 423
> > > 424 /*
> > > 425 * !gpf means preallocated variable size fh, but fh_len could
> > > 426 * be zero in that case if encoding fh len failed.
> > > 427 */
> > > 428 err = -ENOENT;
> > > 429 if (fh_len < 4 || WARN_ON_ONCE(fh_len % 4) || fh_len > MAX_HANDLE_SZ)
> > > 430 goto out_err;
> > > 431
> > > 432 /* No external buffer in a variable size allocated fh */
> > > 433 if (gfp && fh_len > FANOTIFY_INLINE_FH_LEN) {
> > > 434 /* Treat failure to allocate fh as failure to encode fh */
> > > 435 err = -ENOMEM;
> > > 436 ext_buf = kmalloc(fh_len, gfp);
> > > 437 if (!ext_buf)
> > > 438 goto out_err;
> > > 439
> > > 440 *fanotify_fh_ext_buf_ptr(fh) = ext_buf;
> > > 441 buf = ext_buf;
> > > 442 fh->flags |= FANOTIFY_FH_FLAG_EXT_BUF;
> > > 443 }
> > > 444
> > > 445 dwords = fh_len >> 2;
> > > 446 type = exportfs_encode_fid(inode, buf, &dwords);
> > > 447 err = -EINVAL;
> > > 448 if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
> > >
> > > exportfs_encode_fid() can return negative errors. Do we need to check
> > > if (!type etc?
> >
> > Well, it is true that exportfs_encode_fid() can return a negative value
> > in principle, as did exportfs_encode_fh() before it, if there was a
> > filesystem implementation of ->encode_fh() that returned a negative
> > value. AFAIK, there currently is no such implementation in-tree,
> > otherwise current upstream code would have been buggy.
>
> Yes, I've checked and all ->encode_fh() implementations return
> FILEID_INVALID in case of problems (which are basically always only
> problems with not enough space in the handle buffer).
>
> > Patch 2/4 adds a new possible -EOPNOTSUPP return value from
> > exportfs_encode_inode_fh() and even goes further to add a kerndoc:
> > * Returns an enum fid_type or a negative errno.
> > But this new return value is not possible from exportfs_encode_fid()
> > that is used here and in {fa,i}notify_fdinfo().
> >
> > All the rest of the callers (nfsd, overlayfs, name_to_hanle_at) already
> > check this same EOPNOTSUPP condition before calling, but there is
> > no guarantee that this will not change in the future.
> >
> > All the callers mentioned above check the unexpected return value differently:
> > nfsd: only type == FILEID_INVALID
> > fdinfo: type < 0 || type == FILEID_INVALID
> > fanotify: !type || type == FILEID_INVALID
> > overlayfs: type < 0 || type == FILEID_INVALID
> > name_to_hanle_at: (retval == FILEID_INVALID) || (retval == -ENOSPC))
> > /* As per old exportfs_encode_fh documentation
> > * we could return ENOSPC to indicate overflow
> > * But file system returned 255 always. So handle
> > * both the values
> > */
> >
> > So he have a bit of a mess.
>
> Yeah, it's a bit messy. When checking ->encode_fh() implementations I've
> also noticed quite some callers use explicit numbers and not FILEID_*
> enums. This is not directly related to the problem at hand but I'm voicing
> it in case someone looks for an easy cleanup project :)
>
> > How should we clean it up?
> >
> > Option #1: Change encode_fh to return unsigned and replace that new
> > EOPNOTSUPP with FILEID_INVALID
> > Option #2: change all callers to check negative return value
> >
> > I am in favor of option #2.
> > Shall I send a patch?
>
> When we have two different error conditions (out of buffer space, encoding
> handles unsupported), I agree it makes sense to be able to differentiate
> them in exportfs_encode_fh() return value. However then it would make sense
> to properly return -ENOSPC instead of FILEID_INVALID (as it is strange to
> have two different ways of indicating error) which means touching like 16
> different .encode_fh implementations. Not too bad but a bit tedious...
I would prefer leaving that to that "cleanup project" (maybe I will take
it on one day), but even if we leave the fs implementations as is (not
returning negative values) I think we should at least fortify the callers that
assume no negative return values.
I will send a patch and we will see the reaction.
Thanks,
Amir.
