On Wed, May 24, 2023 at 9:34 AM Dan Carpenter <[email protected]> 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.
On Wed 24-05-23 11:38:17, Amir Goldstein wrote:
> On Wed, May 24, 2023 at 9:34 AM Dan Carpenter <[email protected]> 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 <[email protected]>
SUSE Labs, CR
On Wed, May 24, 2023 at 5:06 PM Jan Kara <[email protected]> wrote:
>
> On Wed 24-05-23 11:38:17, Amir Goldstein wrote:
> > On Wed, May 24, 2023 at 9:34 AM Dan Carpenter <[email protected]> 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.
On Wed, May 24, 2023 at 04:06:48PM +0200, Jan Kara wrote:
> 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).
ceph_encode_fh() can return -EINVAL
$ smdb.py functions encode_fh > where
$ for i in $(cut -d '|' -f 3 where | sort -u) ; do smdb.py return_states $i ; done | grep INTER | tee out
regards,
dan carpenter
fs/btrfs/export.c | btrfs_encode_fh | 36 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/btrfs/export.c | btrfs_encode_fh | 37 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/btrfs/export.c | btrfs_encode_fh | 43 | 77| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/btrfs/export.c | btrfs_encode_fh | 44 | 79| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/btrfs/export.c | btrfs_encode_fh | 45 | 78| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/ceph/export.c | ceph_encode_fh | 69 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/ceph/export.c | ceph_encode_fh | 70 | (-22)| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/ceph/export.c | ceph_encode_fh | 71 | 78| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/ceph/export.c | ceph_encode_fh | 72 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/ceph/export.c | ceph_encode_fh | 73 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/ceph/export.c | ceph_encode_fh | 88 | 2| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/ceph/export.c | ceph_encode_fh | 89 | 1| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/fat/nfs.c | fat_encode_fh_nostale | 84 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/fat/nfs.c | fat_encode_fh_nostale | 85 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/fat/nfs.c | fat_encode_fh_nostale | 88 | 114| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/fat/nfs.c | fat_encode_fh_nostale | 89 | 113| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/fuse/inode.c | fuse_encode_fh | 475 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/fuse/inode.c | fuse_encode_fh | 478 | 130| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/fuse/inode.c | fuse_encode_fh | 479 | 129| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/gfs2/export.c | gfs2_encode_fh | 37 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/gfs2/export.c | gfs2_encode_fh | 38 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/gfs2/export.c | gfs2_encode_fh | 40 | 4| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/gfs2/export.c | gfs2_encode_fh | 42 | 8| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/isofs/export.c | isofs_export_encode_fh | 93 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/isofs/export.c | isofs_export_encode_fh | 94 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/isofs/export.c | isofs_export_encode_fh | 96 | 2| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/isofs/export.c | isofs_export_encode_fh | 97 | 1| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/kernfs/mount.c | kernfs_encode_fh | 59 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/kernfs/mount.c | kernfs_encode_fh | 60 | 254| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/nfs/export.c | nfs_encode_fh | 39 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/nfs/export.c | nfs_encode_fh | 45 | s32min-s32max| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/nilfs2/namei.c | nilfs_encode_fh | 289 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/nilfs2/namei.c | nilfs_encode_fh | 290 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/nilfs2/namei.c | nilfs_encode_fh | 291 | 98| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/nilfs2/namei.c | nilfs_encode_fh | 292 | 97| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/ocfs2/export.c | ocfs2_encode_fh | 213 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/ocfs2/export.c | ocfs2_encode_fh | 214 | 2| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/ocfs2/export.c | ocfs2_encode_fh | 215 | 1| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/orangefs/super.c | orangefs_encode_fh | 100 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/orangefs/super.c | orangefs_encode_fh | 101 | 2| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/orangefs/super.c | orangefs_encode_fh | 102 | 1| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/overlayfs/export.c | ovl_encode_fh | 111 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/overlayfs/export.c | ovl_encode_fh | 112 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/overlayfs/export.c | ovl_encode_fh | 113 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/overlayfs/export.c | ovl_encode_fh | 114 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/overlayfs/export.c | ovl_encode_fh | 115 | 248| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/reiserfs/inode.c | reiserfs_encode_fh | 740 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/reiserfs/inode.c | reiserfs_encode_fh | 741 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/reiserfs/inode.c | reiserfs_encode_fh | 744 | 3| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/reiserfs/inode.c | reiserfs_encode_fh | 745 | 6| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/reiserfs/inode.c | reiserfs_encode_fh | 746 | 5| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
mm/shmem.c | shmem_encode_fh | 2144 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
mm/shmem.c | shmem_encode_fh | 2149 | 1| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/udf/namei.c | udf_encode_fh | 447 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/udf/namei.c | udf_encode_fh | 448 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/udf/namei.c | udf_encode_fh | 450 | 82| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/udf/namei.c | udf_encode_fh | 451 | 81| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/xfs/xfs_export.c | xfs_fs_encode_fh | 48 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/xfs/xfs_export.c | xfs_fs_encode_fh | 53 | 130| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/xfs/xfs_export.c | xfs_fs_encode_fh | 54 | 129| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/xfs/xfs_export.c | xfs_fs_encode_fh | 55 | 1| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
fs/xfs/xfs_export.c | xfs_fs_encode_fh | 56 | 2| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
On Thu, May 25, 2023 at 12:26 PM Dan Carpenter <[email protected]> wrote:
>
> On Wed, May 24, 2023 at 04:06:48PM +0200, Jan Kara wrote:
> > 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).
>
> ceph_encode_fh() can return -EINVAL
Ouch! thanks for pointing this out
Jeff,
In your own backyard ;-)
Do you think this new information calls for rebasing my fix on top of master
and marking it for stable? or is this still low risk in your opinion?
Thanks,
Amir.
>
> $ smdb.py functions encode_fh > where
> $ for i in $(cut -d '|' -f 3 where | sort -u) ; do smdb.py return_states $i ; done | grep INTER | tee out
>
> regards,
> dan carpenter
>
> fs/btrfs/export.c | btrfs_encode_fh | 36 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/btrfs/export.c | btrfs_encode_fh | 37 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/btrfs/export.c | btrfs_encode_fh | 43 | 77| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/btrfs/export.c | btrfs_encode_fh | 44 | 79| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/btrfs/export.c | btrfs_encode_fh | 45 | 78| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/ceph/export.c | ceph_encode_fh | 69 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/ceph/export.c | ceph_encode_fh | 70 | (-22)| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/ceph/export.c | ceph_encode_fh | 71 | 78| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/ceph/export.c | ceph_encode_fh | 72 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/ceph/export.c | ceph_encode_fh | 73 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/ceph/export.c | ceph_encode_fh | 88 | 2| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/ceph/export.c | ceph_encode_fh | 89 | 1| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/fat/nfs.c | fat_encode_fh_nostale | 84 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/fat/nfs.c | fat_encode_fh_nostale | 85 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/fat/nfs.c | fat_encode_fh_nostale | 88 | 114| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/fat/nfs.c | fat_encode_fh_nostale | 89 | 113| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/fuse/inode.c | fuse_encode_fh | 475 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/fuse/inode.c | fuse_encode_fh | 478 | 130| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/fuse/inode.c | fuse_encode_fh | 479 | 129| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/gfs2/export.c | gfs2_encode_fh | 37 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/gfs2/export.c | gfs2_encode_fh | 38 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/gfs2/export.c | gfs2_encode_fh | 40 | 4| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/gfs2/export.c | gfs2_encode_fh | 42 | 8| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/isofs/export.c | isofs_export_encode_fh | 93 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/isofs/export.c | isofs_export_encode_fh | 94 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/isofs/export.c | isofs_export_encode_fh | 96 | 2| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/isofs/export.c | isofs_export_encode_fh | 97 | 1| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/kernfs/mount.c | kernfs_encode_fh | 59 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/kernfs/mount.c | kernfs_encode_fh | 60 | 254| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/nfs/export.c | nfs_encode_fh | 39 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/nfs/export.c | nfs_encode_fh | 45 | s32min-s32max| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/nilfs2/namei.c | nilfs_encode_fh | 289 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/nilfs2/namei.c | nilfs_encode_fh | 290 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/nilfs2/namei.c | nilfs_encode_fh | 291 | 98| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/nilfs2/namei.c | nilfs_encode_fh | 292 | 97| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/ocfs2/export.c | ocfs2_encode_fh | 213 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/ocfs2/export.c | ocfs2_encode_fh | 214 | 2| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/ocfs2/export.c | ocfs2_encode_fh | 215 | 1| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/orangefs/super.c | orangefs_encode_fh | 100 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/orangefs/super.c | orangefs_encode_fh | 101 | 2| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/orangefs/super.c | orangefs_encode_fh | 102 | 1| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/overlayfs/export.c | ovl_encode_fh | 111 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/overlayfs/export.c | ovl_encode_fh | 112 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/overlayfs/export.c | ovl_encode_fh | 113 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/overlayfs/export.c | ovl_encode_fh | 114 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/overlayfs/export.c | ovl_encode_fh | 115 | 248| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/reiserfs/inode.c | reiserfs_encode_fh | 740 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/reiserfs/inode.c | reiserfs_encode_fh | 741 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/reiserfs/inode.c | reiserfs_encode_fh | 744 | 3| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/reiserfs/inode.c | reiserfs_encode_fh | 745 | 6| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/reiserfs/inode.c | reiserfs_encode_fh | 746 | 5| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> mm/shmem.c | shmem_encode_fh | 2144 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> mm/shmem.c | shmem_encode_fh | 2149 | 1| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/udf/namei.c | udf_encode_fh | 447 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/udf/namei.c | udf_encode_fh | 448 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/udf/namei.c | udf_encode_fh | 450 | 82| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/udf/namei.c | udf_encode_fh | 451 | 81| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/xfs/xfs_export.c | xfs_fs_encode_fh | 48 | 255| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/xfs/xfs_export.c | xfs_fs_encode_fh | 53 | 130| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/xfs/xfs_export.c | xfs_fs_encode_fh | 54 | 129| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/xfs/xfs_export.c | xfs_fs_encode_fh | 55 | 1| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
> fs/xfs/xfs_export.c | xfs_fs_encode_fh | 56 | 2| INTERNAL | -1 | | int(*)(struct inode*, uint*, int*, struct inode*) |
On Thu 25-05-23 07:46:22, Dan Carpenter wrote:
> On Wed, May 24, 2023 at 04:06:48PM +0200, Jan Kara wrote:
> > 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).
>
> ceph_encode_fh() can return -EINVAL
>
> $ smdb.py functions encode_fh > where
> $ for i in $(cut -d '|' -f 3 where | sort -u) ; do smdb.py return_states $i ; done | grep INTER | tee out
Ah, I've missed that return hidden in ceph_encode_snapfh(). This is indeed
cool use of static analysis ;) Thanks for noticing!
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu, 2023-05-25 at 12:48 +0300, Amir Goldstein wrote:
> On Thu, May 25, 2023 at 12:26 PM Dan Carpenter <[email protected]> wrote:
> >
> > On Wed, May 24, 2023 at 04:06:48PM +0200, Jan Kara wrote:
> > > 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).
> >
> > ceph_encode_fh() can return -EINVAL
>
> Ouch! thanks for pointing this out
>
> Jeff,
>
> In your own backyard ;-)
> Do you think this new information calls for rebasing my fix on top of master
> and marking it for stable? or is this still low risk in your opinion?
>
(cc'ing Xiubo in case he has thoughts here)
It looks like that mainly happens when trying to encode the fh for a
snapshot inode, if you can't find a dentry for it, or if it's not a
directory inode and it has no parent.
The logic of the ceph snapshot code has always escaped me,
unfortunately, and I don't have a good feel for how likely this is. It
seems like it's a non-zero chance though, and this patch is unlikely to
harm anything so marking it for stable might be a good idea.
--
Jeff Layton <[email protected]>
On Thu 25-05-23 12:48:33, Amir Goldstein wrote:
> On Thu, May 25, 2023 at 12:26 PM Dan Carpenter <[email protected]> wrote:
> >
> > On Wed, May 24, 2023 at 04:06:48PM +0200, Jan Kara wrote:
> > > 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).
> >
> > ceph_encode_fh() can return -EINVAL
>
> Ouch! thanks for pointing this out
>
> Jeff,
>
> In your own backyard ;-)
> Do you think this new information calls for rebasing my fix on top of master
> and marking it for stable? or is this still low risk in your opinion?
OK, since I don't see a strong push for merging this ASAP, I'm keeping the
fix in my fsnotify branch and will push it to Linus during the merge
window.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR