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

spa_prop_get: require caller to supply output nvlist #16505

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

robn
Copy link
Member

@robn robn commented Sep 4, 2024

[Sponsors: Klara, Inc., Wasabi Technology, Inc.]

Motivation and Context

@lundman reported a use-after-free. This is the fix.

Description

All callers to spa_prop_get() and spa_prop_get_nvlist() supplied their own preallocated nvlist (except ztest), so we can remove the option to have them allocate one if none is supplied.

This sidesteps a bug in spa_prop_get(), where the error var wasn't initialised, which could lead to the provided nvlist being freed at the end.

How Has This Been Tested?

Completed ZTS run.

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:

module/zfs/spa.c Outdated Show resolved Hide resolved
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Sep 4, 2024
All callers to spa_prop_get() and spa_prop_get_nvlist() supplied their
own preallocated nvlist (except ztest), so we can remove the option to
have them allocate one if none is supplied.

This sidesteps a bug in spa_prop_get(), where the error var wasn't
initialised, which could lead to the provided nvlist being freed at the
end.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@lundman
Copy link
Contributor

lundman commented Sep 5, 2024


$ git revert 9adc94db5c6094bc460f7b8f0c8c17684b1c87c6
[temp cdfe80041d] Revert "Temp fix: add support for prefetching tables into the ARC"
$ git cherry-pick 366d6fecf6bb3c59668b0f3b89f2a610595f3d2f
[temp 235d223322] spa_prop_get: require caller to supply output nvlist
$ zpool create BOOM PHYSICALDRIVE1
$ zpool destroy BOOM

$ git diff
diff --git a/module/zfs/spa.c b/module/zfs/spa.c
-       int err = 0;
+       int err = 4815162342;

Looks good to me, both in success and simulated-error-case.

@glebius
Copy link
Contributor

glebius commented Sep 5, 2024

This is exactly the patch I was writing right now until @amotin pointed me to this pull request. I also observe the use after free panic.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 6, 2024
@behlendorf behlendorf merged commit b109925 into openzfs:master Sep 6, 2024
18 of 22 checks passed
pfannc pushed a commit to pfannc/zfs that referenced this pull request Nov 6, 2024
All callers to spa_prop_get() and spa_prop_get_nvlist() supplied their
own preallocated nvlist (except ztest), so we can remove the option to
have them allocate one if none is supplied.

This sidesteps a bug in spa_prop_get(), where the error var wasn't
initialised, which could lead to the provided nvlist being freed at the
end.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Jorgen Lundman <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes openzfs#16505
pfannc pushed a commit to pfannc/zfs that referenced this pull request Nov 7, 2024
All callers to spa_prop_get() and spa_prop_get_nvlist() supplied their
own preallocated nvlist (except ztest), so we can remove the option to
have them allocate one if none is supplied.

This sidesteps a bug in spa_prop_get(), where the error var wasn't
initialised, which could lead to the provided nvlist being freed at the
end.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Jorgen Lundman <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes openzfs#16505
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.

5 participants