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 crypto build on FreeBSD HEAD #10384

Merged
merged 1 commit into from
May 30, 2020

Conversation

mattmacy
Copy link
Contributor

Update API usage to reflect recent change.

Signed-off-by: Matt Macy mmacy@FreeBSD.org

Motivation and Context

Description

How Has This Been Tested?

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.

Signed-off-by: Matt Macy <mmacy@FreeBSD.org>
@kithrup
Copy link
Contributor

kithrup commented May 28, 2020

Looks good to me (just went and looked at FreeBSD-HEAD).

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label May 28, 2020
@mattmacy
Copy link
Contributor Author

It looks like a subset of crypto operations are simply broken on HEAD. Operations which succeeded previously now return EBADMSG. Presumably John will fix this promptly - otherwise I'll have to shift ZoF to use ICP - which will need to be pulled in to FreeBSD anyway because ztest on FreeBSD still relies on it because FreeBSD's Open Crypto Framework doesn't run in user.

freebsd_crypt_newsession(0xfffffe00af48f238, { CKM_AES_CCM, 1, 24, aes-192-ccm }, { 1, 0xfffffe00af48f1f0, 192 })
	key = { de 77 50 39 1e 47 b4 6b da 05 5c 72 3d 77 b4 70 7d e4 ba d5 9c 20 b5 eb }
freebsd_crypt_uio_debug_log(encrypt, 0, { CKM_AES_CCM, 1, 24, aes-192-ccm }, 0xfffffe00af48ee60, { 1, 0xfffff800031507c0, 256 }, 0xfffffe00af48efa4, 88, 24)
	key = { 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a }
	iovec #0: <0xfffffe00af48ee90, 24>
	iovec #1: <0xfffffe00af48eff0, 24>
	iovec #2: <0xfffffe00af48efb0, 64>
	iovec #3: <0xfffffe00af48ef90, 16>
freebsd_crypt_newsession(0xfffff80008265f40, { CKM_AES_CCM, 1, 24, aes-192-ccm }, { 1, 0xfffff800031507c0, 256 })
	key = { 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a }
freebsd_crypt_uio_debug_log(decrypt, 0, { CKM_AES_CCM, 1, 24, aes-192-ccm }, 0xfffffe00af48f210, { 1, 0xfffff800031507c0, 256 }, 0xfffffe00af48f384, 88, 24)
	key = { 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a }
	iovec #0: <0xfffffe00af48f240, 24>
	iovec #1: <0xfffff800c20360a8, 24>
	iovec #2: <0xfffff800c20360c8, 64>
	iovec #3: <0xfffffe00af48f370, 16>
freebsd_crypt_newsession(0xfffff800c22b5d80, { CKM_AES_CCM, 1, 24, aes-192-ccm }, { 1, 0xfffff800031507c0, 256 })
	key = { 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a 7a }
freebsd_crypt_uio: returning error 89
panic: VERIFY3(0 == dsl_dataset_hold_obj_flags(pdd->dd_pool, obj, DS_HOLD_FLAG_DECRYPT, FTAG, &ds)) failed (0 == 13)

@mattmacy
Copy link
Contributor Author

Good news, the panic is fixed in HEAD. Bad news, this week's AMI for HEAD useless for anything but builds.

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2020

Codecov Report

Merging #10384 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10384      +/-   ##
==========================================
+ Coverage   79.56%   79.59%   +0.03%     
==========================================
  Files         391      391              
  Lines      123603   123603              
==========================================
+ Hits        98346    98385      +39     
+ Misses      25257    25218      -39     
Flag Coverage Δ
#kernel 79.89% <ø> (-0.09%) ⬇️
#user 66.25% <ø> (+0.64%) ⬆️
Impacted Files Coverage Δ
module/os/linux/spl/spl-zlib.c 55.35% <0.00%> (-28.58%) ⬇️
module/os/linux/spl/spl-kmem-cache.c 75.22% <0.00%> (-9.23%) ⬇️
module/os/linux/zfs/zfs_file_os.c 84.15% <0.00%> (-1.00%) ⬇️
module/zfs/vdev_queue.c 95.52% <0.00%> (-0.90%) ⬇️
lib/libzfs/libzfs_changelist.c 84.37% <0.00%> (-0.79%) ⬇️
module/zfs/vdev_raidz.c 92.48% <0.00%> (-0.77%) ⬇️
cmd/zed/agents/zfs_mod.c 77.55% <0.00%> (-0.67%) ⬇️
module/zfs/vdev_indirect.c 84.83% <0.00%> (-0.50%) ⬇️
module/os/linux/zfs/zfs_znode.c 84.93% <0.00%> (-0.46%) ⬇️
module/zfs/dsl_dataset.c 92.16% <0.00%> (-0.34%) ⬇️
... and 47 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 a1ba120...ed06093. Read the comment docs.

@ghost
Copy link

ghost commented May 29, 2020

I've rolled head back to the previous AMI for now. This PR will break the CI build if merged before the next AMI is available, whenever that will be.

@behlendorf
Copy link
Contributor

@mattmacy @freqlabs thanks for the update. Just let me know when the next AMI is available and we can run this PR through again and verify it resolves the issue. Then merge it.

@behlendorf behlendorf added Status: Work in Progress Not yet ready for general review and removed Status: Accepted Ready to integrate (reviewed, tested) labels May 29, 2020
@ghost
Copy link

ghost commented May 29, 2020

Unfortunately rolling back the AMI turns out to not be enough. We’re still fetching the latest src archive for the builds, and there aren’t previous versions of the src kept around anywhere afaik.

@ghost
Copy link

ghost commented May 30, 2020

@behlendorf since the HEAD bots are going to be broken no matter what until the next snapshot, we might as well merge this ASAP so I can update the FreeBSD ports.

@behlendorf behlendorf merged commit 3bf3b16 into openzfs:master May 30, 2020
@ghost ghost deleted the projects/head_crypto_update branch May 30, 2020 20:45
@behlendorf behlendorf mentioned this pull request Jun 7, 2020
17 tasks
kithrup pushed a commit to KlaraSystems/zfs that referenced this pull request Jul 16, 2020
Update API usage to reflect recent change.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Matt Macy <mmacy@FreeBSD.org>
Closes openzfs#10384
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
Update API usage to reflect recent change.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Matt Macy <mmacy@FreeBSD.org>
Closes openzfs#10384
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Work in Progress Not yet ready for general review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants