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.
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.
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?
Thanks,
Amir.
