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

Fix double mutex_init bug in send code #10374

Merged
merged 1 commit into from
Jun 4, 2020

Conversation

pcd1193182
Copy link
Contributor

Signed-off-by: Paul Dagnelie pcd@delphix.com

Motivation and Context

It was possible to cause a kernel panic in the send code by initializing an already-initialized mutex, if a record was created with type DATA, destroyed with a different type (bypassing the mutex_destroy call) and then re-allocated as a DATA record again.

Description

We tweak the logic to not change the type of a record once it has been created, avoiding the issue.

How Has This Been Tested?

Passes zfs test suite

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.
  • I have run the ZFS Test Suite with this change applied.
  • All commit messages are properly formatted and contain Signed-off-by.

@lundman
Copy link
Contributor

lundman commented May 26, 2020

Ah neat - I was set to drill down further to try to figure out where it goes wrong, but a PR is even better :)

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 26, 2020
Copy link
Contributor

@lundman lundman left a comment

Choose a reason for hiding this comment

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

I can confirm this removes the panic during the zfs-tester run.

cheers,

@codecov-commenter
Copy link

codecov-commenter commented May 27, 2020

Codecov Report

Merging #10374 into master will increase coverage by 0.23%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10374      +/-   ##
==========================================
+ Coverage   79.30%   79.53%   +0.23%     
==========================================
  Files         391      391              
  Lines      123603   123604       +1     
==========================================
+ Hits        98018    98305     +287     
+ Misses      25585    25299     -286     
Flag Coverage Δ
#kernel 80.16% <100.00%> (+0.22%) ⬆️
#user 66.00% <0.00%> (+0.95%) ⬆️
Impacted Files Coverage Δ
module/zfs/dmu_send.c 84.56% <80.00%> (-0.07%) ⬇️
cmd/zdb/zdb_il.c 30.86% <0.00%> (-24.08%) ⬇️
cmd/zed/agents/fmd_api.c 88.61% <0.00%> (-1.78%) ⬇️
module/zfs/spa_log_spacemap.c 93.12% <0.00%> (-1.72%) ⬇️
module/zfs/zrlock.c 89.23% <0.00%> (-1.54%) ⬇️
cmd/zed/agents/zfs_diagnosis.c 77.55% <0.00%> (-1.17%) ⬇️
module/os/linux/zfs/zfs_file_os.c 84.15% <0.00%> (-1.00%) ⬇️
module/zfs/dmu_traverse.c 96.01% <0.00%> (-0.67%) ⬇️
module/zfs/zio_checksum.c 97.75% <0.00%> (-0.57%) ⬇️
module/zfs/btree.c 83.25% <0.00%> (-0.47%) ⬇️
... and 54 more

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 4d829ad...d3b30bf. Read the comment docs.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 27, 2020
@behlendorf
Copy link
Contributor

Looks good, would you just mind fixing the style warning and rebasing this on master.

./module/zfs/dmu_send.c: 1164: line > 80 characters

@behlendorf
Copy link
Contributor

@pcd1193182 I've love to get this merged if you can just address style issue and rebase.

Signed-off-by: Paul Dagnelie <pcd@delphix.com>
@behlendorf behlendorf merged commit 99b281f into openzfs:master Jun 4, 2020
as-com pushed a commit to as-com/zfs that referenced this pull request Jun 20, 2020
It was possible to cause a kernel panic in the send code by 
initializing an already-initialized mutex, if a record was created 
with type DATA, destroyed with a different type (bypassing the 
mutex_destroy call) and then re-allocated as a DATA record again.

We tweak the logic to not change the type of a record once it has 
been created, avoiding the issue.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Jorgen Lundman <lundman@lundman.net>
Signed-off-by: Paul Dagnelie <pcd@delphix.com>
Closes openzfs#10374 
(cherry picked from commit 99b281f)
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
It was possible to cause a kernel panic in the send code by 
initializing an already-initialized mutex, if a record was created 
with type DATA, destroyed with a different type (bypassing the 
mutex_destroy call) and then re-allocated as a DATA record again.

We tweak the logic to not change the type of a record once it has 
been created, avoiding the issue.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Jorgen Lundman <lundman@lundman.net>
Signed-off-by: Paul Dagnelie <pcd@delphix.com>
Closes openzfs#10374
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