Skip to content

Return required size when encode_fh size too small #11995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 7, 2021

Conversation

alyssais
Copy link
Contributor

@alyssais alyssais commented May 3, 2021

Motivation and Context

I noticed that the strace test suite did not run correctly on ZFS. This was because it called name_to_handle_at(2), with a zero-sized handle, expecting the kernel to change the handle_bytes value to the size required for a handle, so it could call name_to_handle_at() again with a correctly-sized handle. (This behavior is documented in the man page.)

I then further noticed that the t_name_to_handle_at.c example in the same man page also didn't work properly on ZFS.

Description

Quoting <linux/exportfs.h>:

encode_fh() should return the fileid_type on success and on error returns 255 (if the space needed to encode fh is greater than max_len*4 bytes). On error max_len contains the minimum size (in 4 byte unit) needed to encode the file handle.

ZFS was not setting max_len in the case where the handle was too small.

zfsctl_fid() will itself set max_len if called with a fid that is too small, so if we give zfs_fid() that behavior as well, the fix is quite easy: if the handle is too small, just use a zero-size fid instead of the handle.

How Has This Been Tested?

I ran the t_name_to_handle_at.c example from name_to_handle_at(2) on a normal file, a directory, a .zfs directory, and a snapshot.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Quoting <linux/exportfs.h>:

> encode_fh() should return the fileid_type on success and on error
> returns 255 (if the space needed to encode fh is greater than
> @max_len*4 bytes). On error @max_len contains the minimum size (in 4
> byte unit) needed to encode the file handle.

ZFS was not setting max_len in the case where the handle was too
small.  As a result of this, the `t_name_to_handle_at.c' example in
name_to_handle_at(2) did not work on ZFS.

zfsctl_fid() will itself set max_len if called with a fid that is too
small, so if we give zfs_fid() that behavior as well, the fix is quite
easy: if the handle is too small, just use a zero-size fid instead of
the handle.

Tested by running t_name_to_handle_at on a normal file, a directory, a
.zfs directory, and a snapshot.

Thanks-to: Puck Meerburg <[email protected]>
Signed-off-by: Alyssa Ross <[email protected]>
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 6, 2021
@behlendorf behlendorf requested a review from don-brady May 6, 2021 04:42
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find. Thanks for opening a PR with the fix, this LGTM.

Copy link
Contributor

@tonynguien tonynguien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 6, 2021
@behlendorf behlendorf merged commit c074a7d into openzfs:master May 7, 2021
@alyssais alyssais deleted the encode_fh branch May 9, 2021 15:57
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request May 10, 2021
Quoting <linux/exportfs.h>:

> encode_fh() should return the fileid_type on success and on error
> returns 255 (if the space needed to encode fh is greater than
> @max_len*4 bytes). On error @max_len contains the minimum size (in 4
> byte unit) needed to encode the file handle.

ZFS was not setting max_len in the case where the handle was too
small.  As a result of this, the `t_name_to_handle_at.c' example in
name_to_handle_at(2) did not work on ZFS.

zfsctl_fid() will itself set max_len if called with a fid that is too
small, so if we give zfs_fid() that behavior as well, the fix is quite
easy: if the handle is too small, just use a zero-size fid instead of
the handle.

Tested by running t_name_to_handle_at on a normal file, a directory, a
.zfs directory, and a snapshot.

Thanks-to: Puck Meerburg <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Tony Nguyen <[email protected]>
Signed-off-by: Alyssa Ross <[email protected]>
Closes openzfs#11995
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Quoting <linux/exportfs.h>:

> encode_fh() should return the fileid_type on success and on error
> returns 255 (if the space needed to encode fh is greater than
> @max_len*4 bytes). On error @max_len contains the minimum size (in 4
> byte unit) needed to encode the file handle.

ZFS was not setting max_len in the case where the handle was too
small.  As a result of this, the `t_name_to_handle_at.c' example in
name_to_handle_at(2) did not work on ZFS.

zfsctl_fid() will itself set max_len if called with a fid that is too
small, so if we give zfs_fid() that behavior as well, the fix is quite
easy: if the handle is too small, just use a zero-size fid instead of
the handle.

Tested by running t_name_to_handle_at on a normal file, a directory, a
.zfs directory, and a snapshot.

Thanks-to: Puck Meerburg <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Tony Nguyen <[email protected]>
Signed-off-by: Alyssa Ross <[email protected]>
Closes openzfs#11995
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants