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

Stop segfaulting on unmount error case #12804

Merged
merged 1 commit into from
Nov 30, 2021

Conversation

rincebrain
Copy link
Contributor

Motivation and Context

After a ZTS run errored out, trying to zpool export testpool2 segfaulted on trying to deref a NULL zhp.

I'm not a fan of my tools segfaulting, personally, unless the world is on fire.

(If there's a better way to extract the error code once this happens, I'm all ears, but with NULL zhp, we've got no hdl...could special-case getting back something negative more than 1, I guess, but that's a big change just to smuggle an error out...)

Description

Add a NULL check.

How Has This Been Tested?

It passes testing, but unfortunately I unwedged the system with some zpool online commands and haven't had it reproduce since, so I'm basing this working on having a core dump saying it was NULL dereferencing there.

#0  0x00007f06cc5eb6a0 in unmount_one (zhp=0x0, mountpoint=0x55e4c18bf350 "/var/tmp/zpool_scrub_offline_device", flags=<optimized out>) at /usr/src/debug/zfs-2.1.99-538_g2408c4a3c.fc34.x86_64/lib/libzfs/libzfs_mount.c:598
        libzfs_err = 2007
        error = <optimized out>
(gdb) list
598                     return (zfs_error_fmt(zhp->zfs_hdl, libzfs_err,
599                         dgettext(TEXT_DOMAIN, "cannot unmount '%s'"),
600                         mountpoint));

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:

After interrupting ZTS runs that errored out, I found that
"zpool export testpool2" was segfaulting.

This seems unnecessary.

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
@lundman
Copy link
Contributor

lundman commented Nov 29, 2021

I am probably at fault here. Looks good!

return (zfs_error_fmt(zhp->zfs_hdl, libzfs_err,
dgettext(TEXT_DOMAIN, "cannot unmount '%s'"),
mountpoint));
if (zhp) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If zhp == NULL invoking unmount_one doesn't make sense at all. Most likely null value is injected from

if (unmount_one(sets[i].dataset, sets[i].mountpoint,
and the if-guard should go there, much like it is done few lines below:
if (sets[i].dataset)

and
if (sets[i].dataset)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mostly put the guard in there because of the comment at

suggesting that sometimes calling with a NULL was deliberately permitted - and if you look, do_unmount on Linux doesn't actually use the zhp arg anywhere, so it could have something reasonable happen when called there.

I don't particularly care either way, I just went for the option that changed less behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, Rich. Indeed zhp is used in neither of OS-specific implementations, what really matters is the mountpoint. Quite a surprise to be honest :)

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 29, 2021
@behlendorf behlendorf merged commit 5dc6fc2 into openzfs:master Nov 30, 2021
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
After interrupting ZTS runs that errored out, I found that
"zpool export testpool2" was segfaulting.

This seems unnecessary.

Reviewed-by: szubersk <szuberskidamian@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Jorgen Lundman <lundman@lundman.net>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12804
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
After interrupting ZTS runs that errored out, I found that
"zpool export testpool2" was segfaulting.

This seems unnecessary.

Reviewed-by: szubersk <szuberskidamian@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Jorgen Lundman <lundman@lundman.net>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12804
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Oct 22, 2022
After interrupting ZTS runs that errored out, I found that
"zpool export testpool2" was segfaulting.

This seems unnecessary.

Reviewed-by: szubersk <szuberskidamian@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Jorgen Lundman <lundman@lundman.net>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12804
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Oct 22, 2022
After interrupting ZTS runs that errored out, I found that
"zpool export testpool2" was segfaulting.

This seems unnecessary.

Reviewed-by: szubersk <szuberskidamian@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Jorgen Lundman <lundman@lundman.net>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12804
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Oct 23, 2022
After interrupting ZTS runs that errored out, I found that
"zpool export testpool2" was segfaulting.

This seems unnecessary.

Reviewed-by: szubersk <szuberskidamian@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Jorgen Lundman <lundman@lundman.net>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants