zfs_handle used after being closed/freed in change_one() callback #9165

pzakha · 2019-08-15T16:09:38Z

Motivation and Context

This is a typical case of use after free. We would call zfs_close(zhp) which would free the handle, and then call zfs_iter_children() on that handle later. This change ensures that the zfs_handle is only closed when we are ready to return.

Description

I've been running zfs inherit -r sharenfs pool which was failing with an error code without any error messages. After some debugging I've pinpointed the issue to be memory corruption, which would cause zfs to try to issue an ioctl to the wrong device and receive ENOTTY.

This failure was likely a fallout of the changes introduced in #7967.

How Has This Been Tested?

After deploying those changes, zfs inherit -r sharenfs pool succeeds.
Ran the zfs-test suite using internal Delphix tools: http://platform.jenkins.delphix.com/job/devops-gate/job/master/job/zfs-precommit/4727/console (private link)

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)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.

pzakha · 2019-08-15T16:09:55Z

cc: @sebroy @grwilson @alek-p

lib/libzfs/libzfs_changelist.c

pzakha · 2019-08-15T17:34:02Z

Added code to also fix a leak of a zfs handle.
Re-testing: http://platform.jenkins.delphix.com/job/devops-gate/job/master/job/zfs-precommit/4729/

codecov · 2019-08-16T01:32:01Z

Codecov Report

Merging #9165 into master will decrease coverage by 0.14%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #9165      +/-   ##
==========================================
- Coverage   79.24%    79.1%   -0.15%     
==========================================
  Files         400      400              
  Lines      122012   122002      -10     
==========================================
- Hits        96687    96506     -181     
- Misses      25325    25496     +171

Flag	Coverage Δ
#kernel	`79.74% <ø> (-0.06%)`	⬇️
#user	`66.82% <80%> (-0.55%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6203d2...d5c3adb. Read the comment docs.

alek-p

LGTM

lib/libzfs/libzfs_changelist.c

pzakha · 2019-08-27T15:12:04Z

I've updated the code, and had a successful run for a slightly different version.

The issue is that we do need to close the handle, but only if it is not added to the changelist. Alternatively we could have dup'd the handle that we add to the changelist and always close the one passed to change_one.

zfs-precommit link (Delphix internal).

behlendorf · 2019-08-28T16:22:24Z

@pzakha can you refresh this one last time with the final code and resolve the cstyle warning. Then this should be good to go.

pzakha · 2019-08-28T16:45:52Z

@behlendorf done, thanks for spotting that!

Signed-off-by: Pavel Zakharov <pavel.zakharov@delphix.com>

This is a typical case of use after free. We would call zfs_close(zhp) which would free the handle, and then call zfs_iter_children() on that handle later. This change ensures that the zfs_handle is only closed when we are ready to return. Running `zfs inherit -r sharenfs pool` was failing with an error code without any error messages. After some debugging I've pinpointed the issue to be memory corruption, which would cause zfs to try to issue an ioctl to the wrong device and receive ENOTTY. Reviewed-by: Paul Dagnelie <pcd@delphix.com> Reviewed-by: George Wilson <gwilson@delphix.com> Reviewed-by: Sebastien Roy <sebastien.roy@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alek Pinchuk <apinchuk@datto.com> Signed-off-by: Pavel Zakharov <pavel.zakharov@delphix.com> Issue openzfs#7967 Closes openzfs#9165

This is a typical case of use after free. We would call zfs_close(zhp) which would free the handle, and then call zfs_iter_children() on that handle later. This change ensures that the zfs_handle is only closed when we are ready to return. Running `zfs inherit -r sharenfs pool` was failing with an error code without any error messages. After some debugging I've pinpointed the issue to be memory corruption, which would cause zfs to try to issue an ioctl to the wrong device and receive ENOTTY. Reviewed-by: Paul Dagnelie <pcd@delphix.com> Reviewed-by: George Wilson <gwilson@delphix.com> Reviewed-by: Sebastien Roy <sebastien.roy@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alek Pinchuk <apinchuk@datto.com> Signed-off-by: Pavel Zakharov <pavel.zakharov@delphix.com> Issue #7967 Closes #9165

pcd1193182 approved these changes Aug 15, 2019

View reviewed changes

sebroy suggested changes Aug 15, 2019

View reviewed changes

lib/libzfs/libzfs_changelist.c Outdated Show resolved Hide resolved

sebroy approved these changes Aug 15, 2019

View reviewed changes

alek-p approved these changes Aug 16, 2019

View reviewed changes

behlendorf added the Status: Code Review Needed Ready for review and testing label Aug 16, 2019

pzakha added Status: Accepted Ready to integrate (reviewed, tested) Status: Code Review Needed Ready for review and testing and removed Status: Code Review Needed Ready for review and testing Status: Accepted Ready to integrate (reviewed, tested) labels Aug 16, 2019

behlendorf reviewed Aug 16, 2019

View reviewed changes

lib/libzfs/libzfs_changelist.c Outdated Show resolved Hide resolved

grwilson approved these changes Aug 17, 2019

View reviewed changes

lib/libzfs/libzfs_changelist.c Show resolved Hide resolved

behlendorf added the Status: Revision Needed Changes are required for the PR to be accepted label Aug 19, 2019

pzakha added Status: Work in Progress Not yet ready for general review and removed Status: Revision Needed Changes are required for the PR to be accepted Status: Work in Progress Not yet ready for general review labels Aug 21, 2019

pcd1193182 approved these changes Aug 27, 2019

View reviewed changes

behlendorf approved these changes Aug 28, 2019

View reviewed changes

zfs_handle used after being closed/freed in change_one callback

d5c3adb

Signed-off-by: Pavel Zakharov <pavel.zakharov@delphix.com>

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Aug 28, 2019

behlendorf merged commit e6cebbf into openzfs:master Aug 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zfs_handle used after being closed/freed in change_one() callback #9165

zfs_handle used after being closed/freed in change_one() callback #9165

pzakha commented Aug 15, 2019

pzakha commented Aug 15, 2019

pzakha commented Aug 15, 2019

codecov bot commented Aug 16, 2019 •

edited

Loading

alek-p left a comment

pzakha commented Aug 27, 2019

behlendorf commented Aug 28, 2019

pzakha commented Aug 28, 2019

zfs_handle used after being closed/freed in change_one() callback #9165

zfs_handle used after being closed/freed in change_one() callback #9165

Conversation

pzakha commented Aug 15, 2019

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

pzakha commented Aug 15, 2019

pzakha commented Aug 15, 2019

codecov bot commented Aug 16, 2019 • edited Loading

Codecov Report

alek-p left a comment

Choose a reason for hiding this comment

pzakha commented Aug 27, 2019

behlendorf commented Aug 28, 2019

pzakha commented Aug 28, 2019

codecov bot commented Aug 16, 2019 •

edited

Loading