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...
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
