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.

Sign up for GitHub

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

Jump to bottom

Fix use-after-free in case of L2ARC prefetch failure. #9648

Merged
merged 1 commit into from
Dec 3, 2019
Merged

Fix use-after-free in case of L2ARC prefetch failure. #9648

merged 1 commit into from
Dec 3, 2019

Conversation

amotin
Copy link
Member

@amotin amotin commented Nov 28, 2019

In case L2ARC read failed, l2arc_read_done() creates different ZIO
to read data from the original storage device. Unfortunately pointer
to the failed ZIO remains in hdr->b_l1hdr.b_acb->acb_zio_head, and if
some other read try to bump the ZIO priority, it will crash.

The problem is reproducible by corrupting L2ARC content and reading
some data with prefetch if l2arc_noprefetch tunable is changed to 0.
With the default setting the issue is probably not reproducible now.

The change was tested on FreeBSD head and solves the kernel panic, easily reproducible without it.

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:

@PrivatePuffin
Copy link
Contributor

Considering #9582 maybe @gamanakis wants to have a look at this PR...

@amotin
Copy link
Member Author

amotin commented Nov 28, 2019 •
edited
Loading

Also I think @tcaputi, since he added the code for re-prioritizing asynchronous prefetches.

@gdevenyi
Copy link
Contributor

The problem is reproducible by corrupting L2ARC content and reading
some data with prefetch if l2arc_noprefetch tunable is changed to 0.
With the default setting the issue is probably not reproducible now.

Is this a hint that perhaps the test suite should run through all possible (allowable?) combinations of turntables to possibly expose such bugs? (this would of course depend on the test suite having a test that corrupts the L2ARC)...

@gdevenyi gdevenyi mentioned this pull request Nov 29, 2019
Open
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 30, 2019
tcaputi
tcaputi approved these changes Dec 2, 2019
View reviewed changes
Copy link
Contributor

@tcaputi tcaputi left a comment

Choose a reason for hiding this comment

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

The fix LGTM. I'm not sure what we want to do for testing, though.

behlendorf
behlendorf approved these changes Dec 2, 2019
View reviewed changes
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.

Nice find. Functionally this looks good just a few small suggestions.

module/zfs/arc.c Show resolved Hide resolved
module/zfs/arc.c Show resolved Hide resolved
@amotin
Fix use-after-free in case of L2ARC prefetch failure.
164b2c0 
In case L2ARC read failed, l2arc_read_done() creates _different_ ZIO
to read data from the original storage device.  Unfortunately pointer
to the failed ZIO remains in hdr->b_l1hdr.b_acb->acb_zio_head, and if
some other read try to bump the ZIO priority, it will crash.

The problem is reproducible by corrupting L2ARC content and reading
some data with prefetch if l2arc_noprefetch tunable is changed to 0.
With the default setting the issue is probably not reproducible now.

Sponsored-By: iXsystems, Inc.
Signed-off-by: Alexander Motin <[email protected]>
@amotin
Copy link
Member Author

amotin commented Dec 2, 2019

Updated the cosmetics.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 2, 2019
@codecov
Copy link

codecov bot commented Dec 3, 2019 •
edited
Loading

Codecov Report

Merging #9648 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9648      +/-   ##
==========================================
- Coverage   79.43%    79.4%   -0.04%     
==========================================
  Files         418      418              
  Lines      123544   123549       +5     
==========================================
- Hits        98142    98102      -40     
- Misses      25402    25447      +45
Flag Coverage Δ
#kernel 79.95% <0%> (-0.07%) ⬇️
#user 67.35% <0%> (+0.14%) ⬆️

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 e69bb31...164b2c0. Read the comment docs.

@behlendorf behlendorf merged commit 5ff2249 into openzfs:master Dec 3, 2019
gamanakis pushed a commit to gamanakis/zfs that referenced this pull request Dec 24, 2019
@amotin @gamanakis
Fix use-after-free in case of L2ARC prefetch failure
dd4008b 
In case L2ARC read failed, l2arc_read_done() creates _different_ ZIO
to read data from the original storage device.  Unfortunately pointer
to the failed ZIO remains in hdr->b_l1hdr.b_acb->acb_zio_head, and if
some other read try to bump the ZIO priority, it will crash.

The problem is reproducible by corrupting L2ARC content and reading
some data with prefetch if l2arc_noprefetch tunable is changed to 0.
With the default setting the issue is probably not reproducible now.

Reviewed-by: Tom Caputi <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored-By: iXsystems, Inc.
Closes openzfs#9648
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 26, 2019
@amotin @tonyhutter
Fix use-after-free in case of L2ARC prefetch failure
6a3ba7e 
In case L2ARC read failed, l2arc_read_done() creates _different_ ZIO
to read data from the original storage device.  Unfortunately pointer
to the failed ZIO remains in hdr->b_l1hdr.b_acb->acb_zio_head, and if
some other read try to bump the ZIO priority, it will crash.

The problem is reproducible by corrupting L2ARC content and reading
some data with prefetch if l2arc_noprefetch tunable is changed to 0.
With the default setting the issue is probably not reproducible now.

Reviewed-by: Tom Caputi <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored-By: iXsystems, Inc.
Closes openzfs#9648
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 27, 2019
@amotin @tonyhutter
Fix use-after-free in case of L2ARC prefetch failure
f05d403 
In case L2ARC read failed, l2arc_read_done() creates _different_ ZIO
to read data from the original storage device.  Unfortunately pointer
to the failed ZIO remains in hdr->b_l1hdr.b_acb->acb_zio_head, and if
some other read try to bump the ZIO priority, it will crash.

The problem is reproducible by corrupting L2ARC content and reading
some data with prefetch if l2arc_noprefetch tunable is changed to 0.
With the default setting the issue is probably not reproducible now.

Reviewed-by: Tom Caputi <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored-By: iXsystems, Inc.
Closes openzfs#9648
tonyhutter pushed a commit that referenced this pull request Jan 23, 2020
@amotin @tonyhutter
Fix use-after-free in case of L2ARC prefetch failure
388ef04 
In case L2ARC read failed, l2arc_read_done() creates _different_ ZIO
to read data from the original storage device.  Unfortunately pointer
to the failed ZIO remains in hdr->b_l1hdr.b_acb->acb_zio_head, and if
some other read try to bump the ZIO priority, it will crash.

The problem is reproducible by corrupting L2ARC content and reading
some data with prefetch if l2arc_noprefetch tunable is changed to 0.
With the default setting the issue is probably not reproducible now.

Reviewed-by: Tom Caputi <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored-By: iXsystems, Inc.
Closes #9648
@amotin amotin deleted the l2arc_pref_err branch August 24, 2021 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcaputi tcaputi tcaputi approved these changes

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@amotin @PrivatePuffin @gdevenyi @behlendorf @tcaputi