* [bug report] fanotify: support reporting non-decodeable file handles
@ 2023-05-24 6:34 Dan Carpenter
2023-05-24 8:38 ` Amir Goldstein
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2023-05-24 6:34 UTC (permalink / raw)
To: amir73il; +Cc: linux-fsdevel
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?
449 goto out_err;
450
--> 451 fh->type = type;
452 fh->len = fh_len;
453
454 out:
455 /*
456 * Mix fh into event merge key. Hash might be NULL in case of
457 * unhashed FID events (i.e. FAN_FS_ERROR).
458 */
459 if (hash)
460 *hash ^= fanotify_hash_fh(fh);
461
462 return FANOTIFY_FH_HDR_LEN + fh_len;
463
464 out_err:
465 pr_warn_ratelimited("fanotify: failed to encode fid (type=%d, len=%d, err=%i)\n",
466 type, fh_len, err);
467 kfree(ext_buf);
468 *fanotify_fh_ext_buf_ptr(fh) = NULL;
469 /* Report the event without a file identifier on encode error */
470 fh->type = FILEID_INVALID;
471 fh->len = 0;
472 return 0;
473 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [bug report] fanotify: support reporting non-decodeable file handles
2023-05-24 6:34 [bug report] fanotify: support reporting non-decodeable file handles Dan Carpenter
@ 2023-05-24 8:38 ` Amir Goldstein
2023-05-24 14:06 ` Jan Kara
0 siblings, 1 reply; 9+ messages in thread
From: Amir Goldstein @ 2023-05-24 8:38 UTC (permalink / raw)
To: Jan Kara, Jeff Layton
Cc: linux-fsdevel, Dan Carpenter, Chuck Lever, Linux NFS Mailing List
On Wed, May 24, 2023 at 9:34 AM Dan Carpenter <dan.carpenter@linaro.org> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [bug report] fanotify: support reporting non-decodeable file handles
2023-05-24 8:38 ` Amir Goldstein
@ 2023-05-24 14:06 ` Jan Kara
2023-05-24 14:49 ` Amir Goldstein
2023-05-25 4:46 ` Dan Carpenter
0 siblings, 2 replies; 9+ messages in thread
From: Jan Kara @ 2023-05-24 14:06 UTC (permalink / raw)
To: Amir Goldstein
Cc: Jan Kara, Jeff Layton, linux-fsdevel, Dan Carpenter, Chuck Lever,
Linux NFS Mailing List
On Wed 24-05-23 11:38:17, Amir Goldstein wrote:
> On Wed, May 24, 2023 at 9:34 AM Dan Carpenter <dan.carpenter@linaro.org> 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@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [bug report] fanotify: support reporting non-decodeable file handles
2023-05-24 14:06 ` Jan Kara
@ 2023-05-24 14:49 ` Amir Goldstein
2023-05-25 4:46 ` Dan Carpenter
1 sibling, 0 replies; 9+ messages in thread
From: Amir Goldstein @ 2023-05-24 14:49 UTC (permalink / raw)
To: Jan Kara
Cc: Jeff Layton, linux-fsdevel, Dan Carpenter, Chuck Lever,
Linux NFS Mailing List
On Wed, May 24, 2023 at 5:06 PM Jan Kara <jack@suse.cz> 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@linaro.org> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [bug report] fanotify: support reporting non-decodeable file handles
2023-05-24 14:06 ` Jan Kara
2023-05-24 14:49 ` Amir Goldstein
@ 2023-05-25 4:46 ` Dan Carpenter
2023-05-25 9:48 ` Amir Goldstein
2023-05-25 10:23 ` Jan Kara
1 sibling, 2 replies; 9+ messages in thread
From: Dan Carpenter @ 2023-05-25 4:46 UTC (permalink / raw)
To: Jan Kara
Cc: Amir Goldstein, Jeff Layton, linux-fsdevel, Chuck Lever,
Linux NFS Mailing List
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*) |
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [bug report] fanotify: support reporting non-decodeable file handles
2023-05-25 4:46 ` Dan Carpenter
@ 2023-05-25 9:48 ` Amir Goldstein
2023-05-25 10:19 ` Jeff Layton
2023-06-01 13:02 ` Jan Kara
2023-05-25 10:23 ` Jan Kara
1 sibling, 2 replies; 9+ messages in thread
From: Amir Goldstein @ 2023-05-25 9:48 UTC (permalink / raw)
To: Dan Carpenter, Jeff Layton
Cc: Jan Kara, linux-fsdevel, Chuck Lever, Linux NFS Mailing List
On Thu, May 25, 2023 at 12:26 PM Dan Carpenter <dan.carpenter@linaro.org> 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*) |
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [bug report] fanotify: support reporting non-decodeable file handles
2023-05-25 9:48 ` Amir Goldstein
@ 2023-05-25 10:19 ` Jeff Layton
2023-06-01 13:02 ` Jan Kara
1 sibling, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2023-05-25 10:19 UTC (permalink / raw)
To: Amir Goldstein, Dan Carpenter, Xiubo Li
Cc: Jan Kara, linux-fsdevel, Chuck Lever, Linux NFS Mailing List
On Thu, 2023-05-25 at 12:48 +0300, Amir Goldstein wrote:
> On Thu, May 25, 2023 at 12:26 PM Dan Carpenter <dan.carpenter@linaro.org> 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 <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [bug report] fanotify: support reporting non-decodeable file handles
2023-05-25 4:46 ` Dan Carpenter
2023-05-25 9:48 ` Amir Goldstein
@ 2023-05-25 10:23 ` Jan Kara
1 sibling, 0 replies; 9+ messages in thread
From: Jan Kara @ 2023-05-25 10:23 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jan Kara, Amir Goldstein, Jeff Layton, linux-fsdevel, Chuck Lever,
Linux NFS Mailing List
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 <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [bug report] fanotify: support reporting non-decodeable file handles
2023-05-25 9:48 ` Amir Goldstein
2023-05-25 10:19 ` Jeff Layton
@ 2023-06-01 13:02 ` Jan Kara
1 sibling, 0 replies; 9+ messages in thread
From: Jan Kara @ 2023-06-01 13:02 UTC (permalink / raw)
To: Amir Goldstein
Cc: Dan Carpenter, Jeff Layton, Jan Kara, linux-fsdevel, Chuck Lever,
Linux NFS Mailing List
On Thu 25-05-23 12:48:33, Amir Goldstein wrote:
> On Thu, May 25, 2023 at 12:26 PM Dan Carpenter <dan.carpenter@linaro.org> 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 <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-06-01 13:02 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-24 6:34 [bug report] fanotify: support reporting non-decodeable file handles Dan Carpenter
2023-05-24 8:38 ` Amir Goldstein
2023-05-24 14:06 ` Jan Kara
2023-05-24 14:49 ` Amir Goldstein
2023-05-25 4:46 ` Dan Carpenter
2023-05-25 9:48 ` Amir Goldstein
2023-05-25 10:19 ` Jeff Layton
2023-06-01 13:02 ` Jan Kara
2023-05-25 10:23 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox