Skip to content
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

FreeBSD .zfs fixups #14774

Closed
wants to merge 2 commits into from
Closed

FreeBSD .zfs fixups #14774

wants to merge 2 commits into from

Conversation

mjguzik
Copy link
Contributor

@mjguzik mjguzik commented Apr 20, 2023

please add to the release if there is time, not a big deal if there is not

fixes find /.zfs:

# find /.zfs
/.zfs
/.zfs/snapshot
find: /.zfs: Invalid argument

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Without the change:
/.zfs
/.zfs/snapshot
find: /.zfs: Invalid argument

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
@mjguzik
Copy link
Contributor Author

mjguzik commented Apr 20, 2023

@amotin

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

I may be wrong, but as I see it, zfsctl_root_readdir() should normally handle short reads or reads at non-zero offset, so I am not sure the FIXME comment is correct.
I have no objections about the added condition, but wonder if it could be done less explicitly. It feels like a workaround to me.

@KungFuJesus
Copy link

Correct me if I'm wrong here but, wouldn't this lead to undesired traversal into your snapshots when running commands such as find or updatedb?

@amotin
Copy link
Member

amotin commented Apr 20, 2023

Correct me if I'm wrong here but, wouldn't this lead to undesired traversal into your snapshots when running commands such as find or updatedb?

Are you saying the bug is intentional? I doubt. It would be a pretty weird way. Plus usually .zfs directory is hidden, so no tools should see it to traverse unless pointed explicitly.

@mjguzik
Copy link
Contributor Author

mjguzik commented Apr 20, 2023

I am saying that the routine as implemented right now only supports dumping out the 3 entries every time which fails for buffers which can only hold fewer entries (not fixed) and fails for the idiomatic call to validate there are no trailing entries (fixed). Normally callers do have sufficient space so only the latter demands fixing right now.

@amotin
Copy link
Member

amotin commented Apr 20, 2023

To me it looks like ENAMETOOLONG checks should handle short buffers. And offset checks should handle reading at different offsets.

@mjguzik
Copy link
Contributor Author

mjguzik commented Apr 20, 2023

they should but they don't, instead this bit:

      if (zfs_uio_offset(&uio) != dots_offset)
               return (SET_ERROR(EINVAL));

results in EINVAL in the idiomatic use.

If you are volunteering to make the routine fully api-compliant, have fun.

@KungFuJesus
Copy link

Correct me if I'm wrong here but, wouldn't this lead to undesired traversal into your snapshots when running commands such as find or updatedb?

Are you saying the bug is intentional? I doubt. It would be a pretty weird way. Plus usually .zfs directory is hidden, so no tools should see it to traverse unless pointed explicitly.

Ah, forgive my ignorance with hidden file semantics. I do recall some mechanism in place that prevents things from crawling these directories, maybe it was the on demand mounting mechanism?

@mjguzik
Copy link
Contributor Author

mjguzik commented Apr 20, 2023

Correct me if I'm wrong here but, wouldn't this lead to undesired traversal into your snapshots when running commands such as find or updatedb?

Are you saying the bug is intentional? I doubt. It would be a pretty weird way. Plus usually .zfs directory is hidden, so no tools should see it to traverse unless pointed explicitly.

Ah, forgive my ignorance with hidden file semantics. I do recall some mechanism in place that prevents things from crawling these directories, maybe it was the on demand mounting mechanism?

.zfs does not show up in directory listings, but it can be accessed if explicitly referred to. there is no magic here.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Apr 20, 2023
behlendorf pushed a commit that referenced this pull request Apr 26, 2023
Without the change:
/.zfs
/.zfs/snapshot
find: /.zfs: Invalid argument

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Closes #14774
@behlendorf
Copy link
Contributor

I've gone ahead and merged these changes. If a better solution is implemented we can update to that.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Apr 26, 2023
@amotin
Copy link
Member

amotin commented Apr 26, 2023

@mjguzik @behlendorf This broke checkstyle.

@behlendorf
Copy link
Contributor

Ugh, so it did. Let me open a PR to quickly fix that.

andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request May 1, 2023
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Closes openzfs#14774
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request May 1, 2023
Without the change:
/.zfs
/.zfs/snapshot
find: /.zfs: Invalid argument

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Closes openzfs#14774
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request May 26, 2023
Without the change:
/.zfs
/.zfs/snapshot
find: /.zfs: Invalid argument

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Closes openzfs#14774
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request May 26, 2023
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Closes openzfs#14774
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request May 28, 2023
Without the change:
/.zfs
/.zfs/snapshot
find: /.zfs: Invalid argument

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Closes openzfs#14774
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request May 28, 2023
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Closes openzfs#14774
behlendorf pushed a commit that referenced this pull request May 30, 2023
Without the change:
/.zfs
/.zfs/snapshot
find: /.zfs: Invalid argument

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Closes #14774
behlendorf pushed a commit that referenced this pull request May 30, 2023
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Closes #14774
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.

4 participants