Remove checks for null out value in encryption paths #10015

dbussink · 2020-02-17T18:39:18Z

Motivation and Context

This change is driven by the discussion in #9661 and the new AES GCM optimizations that were recently added that assume out != NULL in these code paths. Investigation found that these code paths are dead and can be cleaned up.

Resolves #9661

Description

There were two callsites in crypto_update_iov & crypto_update_uio where a potential NULL value for out was passed down. This was happening when the input & output were the same values.

All calls of these two methods are in module/icp/io/aes.c in aes_encrypt_update, aes_decrypt_update, aes_encrypt_atomic & aes_decrypt_atomic. These are set as function pointers in the crypto_cipher_ops_t configuration for the AES module.

Going back to all calls into these functions, it shows there none of them have the same plaintext & ciphertext passed in. Taking crypto_encrypt & crypto_decrypt specifically, these are called in module/os/linux/zfs/zio_crypt.c with a different plaindata and cipherdata allocated on the stack there.

All other functions are never used, I guess due to it being a generic encryption interface in icp, where the zfs module never needs all of these and only uses crypto_encrypt & crypto_decrypt.

I added the assertions as a safety mechanism for now. I don't know if that's considered a good practice in cases like this, or that I should remove these?

How Has This Been Tested?

This build successfully and tests locally seem to pass. Different local runs show different unrelated tests sometimes failing, I guess those are inconsistent then.

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.

rlaager

I can’t speak to the design questions, but with the assumptions discussed, the code changes appear correct to me.

dbussink · 2020-02-17T18:58:22Z

Forgot to mention, but the diff is I think best viewed ignoring whitespace, it's more clear what is cleaned up then.

codecov · 2020-02-18T02:26:22Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #10015      +/-   ##
==========================================
+ Coverage   79.26%   79.49%   +0.22%     
==========================================
  Files         385      385              
  Lines      122389   122347      -42     
==========================================
+ Hits        97016    97258     +242     
+ Misses      25373    25089     -284

Flag	Coverage Δ
#kernel	`79.53% <29.31%> (+0.11%)`	⬆️
#user	`67.30% <28.81%> (+0.61%)`	⬆️

Impacted Files	Coverage Δ
module/zfs/spa_errlog.c	`90.83% <0.00%> (-3.06%)`	⬇️
cmd/zed/agents/fmd_api.c	`88.61% <0.00%> (-1.78%)`	⬇️
module/os/linux/zfs/arc_os.c	`59.01% <0.00%> (-1.64%)`	⬇️
cmd/zed/agents/zfs_diagnosis.c	`77.55% <0.00%> (-1.17%)`	⬇️
module/os/linux/zfs/vdev_disk.c	`84.00% <0.00%> (-0.37%)`	⬇️
lib/libzfs/libzfs_pool.c	`73.91% <0.00%> (-0.35%)`	⬇️
lib/libefi/rdwr_efi.c	`60.37% <0.00%> (-0.32%)`	⬇️
module/zfs/dmu_redact.c	`81.30% <0.00%> (-0.22%)`	⬇️
lib/libzfs/libzfs_util.c	`74.35% <0.00%> (-0.14%)`	⬇️
module/zfs/dsl_scan.c	`85.98% <0.00%> (-0.13%)`	⬇️
... 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 4fe3a84...0ea418a. Read the comment docs.

AttilaFueloep · 2020-02-18T12:46:56Z

Thanks for your analysis, I'll try to comprehend it but I'm currently quite busy so it may take a while.

dbussink · 2020-02-19T08:14:21Z

codecov/patch

I suspect this build is failing because of a few reasons:

Only the "newly added code" is what ends up being validated here, so the removed code is not part if the % calculation. This is seen as new, because of indentation changes, but it's the same code as before.
The tests only exercise the GCM and CCM paths. I also cleaned up the CBC, CTR & ECB paths. This makes me wonder, should those others be removed or are they kept for backwards compatibility with older ZFS pools that might use those algorithms? Regardless, this means that part of the diff is not exercised.
Now that GCM has optimizations in place, the coverage tests don't run both the optimized and non-optimized paths. This means that more of the diff is not covered here.

I'm not sure if any of the above is easy to fix, but it does explain the relatively low patch coverage %.

behlendorf · 2020-02-19T17:42:38Z

I'm not sure if any of the above is easy to fix, but it does explain the relatively low patch coverage %.

There's one additional non-obvious issue as well. The CI builder which performs the coverage analysis currently does not support the needed instructions, so it will not run this code. The optimized code will be used by the other CI instances so it was tested successfully. But the coverage reporting for this particular change will be misleading. Knowing that in advance I'm not too concerned about it, but thank you for investigating the results.

dbussink · 2020-02-29T09:08:15Z

@behlendorf Is there anything I can do here to move this PR forward?

behlendorf

Sorry about the delay. This looks right to me.

AttilaFueloep · 2020-03-05T23:43:23Z

@dbussink I have been busy until recently, so please give me a day or two.

The CI builder which performs the coverage analysis currently does not support the needed instructions, so it will not run this code.

This will change once #10029 is merged,

dbussink · 2020-03-06T07:27:32Z

@AttilaFueloep Ah, no problem! I don't think that changes the issue that the patch coverage job fails though. It means we move which code is covered, but it will mark code not running now as covered and vice versa. Also the code for other modes than GCM or CCM would still not be covered.

behlendorf · 2020-03-17T17:26:30Z

Now that #10029 has been merged would you mind rebasing this PR on master and force updating the PR.

dbussink · 2020-03-17T18:24:41Z

Now that #10029 has been merged would you mind rebasing this PR on master and force updating the PR.

@behlendorf Done!

dbussink · 2020-03-17T18:46:31Z

The check style failure doesn't seem to be related to any changes here:

make checkstyle
 in dir /var/lib/buildbot/slaves/zfs/Ubuntu_18_04_x86_64__STYLE_/build/zfs (timeout 1200 secs)
 watching logfiles {}
 argv: ['make', 'checkstyle']
 using PTY: False
./tests/zfs-tests/cmd/libzfs_input_check/libzfs_input_check.c: 559: line > 80 characters
Makefile:1598: recipe for target 'cstyle' failed
make: *** [cstyle] Error 1
program finished with exit code 2
elapsedTime=8.308985

behlendorf · 2020-03-17T18:52:20Z

Whoops, looks like that minor style issue snuck in accidentally. Yes, it's totally unrelated to this change. We'll get it sorted out right away.

AttilaFueloep · 2020-03-17T23:48:35Z

@dbussink

I don't think that changes the issue that the patch coverage job fails though. It means we move which code is covered, but it will mark code not running now as covered and vice versa.

Not exactly, the coverage job runs ztest (and ZTS) and ztest uses the cycle implementation which, as its name suggest, cycles through all available implementations, resulting in all implementations being covered.

Also the code for other modes than GCM or CCM would still not be covered.

That's true.

Sorry for the delay, I'm going to have a look now.

AttilaFueloep · 2020-03-18T02:00:18Z

I can follow your analysis and the changes look good to me, with one exception:

Wouldn't you want to remove all occurrences of the macro AES_ARG_INPLACE as well?
(module/icp/io/aes.c, lines 96, 416, 533, 638, 708, 950 and 1076)

Not sure about CRYPTO_INPLACE_OPERATION (module/icp/include/sys/crypto/ioctl.h line 245)

I added the assertions as a safety mechanism for now. I don't know if that's considered a good practice in cases like this, or that I should remove these?

Yes, I think that's a good idea, let alone someone adding code which tries to use inplace operations.

dbussink · 2020-03-18T07:17:15Z

Wouldn't you want to remove all occurrences of the macro AES_ARG_INPLACE as well?
(module/icp/io/aes.c, lines 96, 416, 533, 638, 708, 950 and 1076)

Not sure about CRYPTO_INPLACE_OPERATION (module/icp/include/sys/crypto/ioctl.h line 245)

@AttilaFueloep You are right, I think both of these can be dropped as well. The AES_ARG_INPLACE macro would never be used and CRYPTO_INPLACE_OPERATION doesn't seem to have any usage across the codebase, also signaling that in place operations are not used actively anywhere.

I'll update this PR to also including removing those bits.

module/icp/io/aes.c

These paths are never exercised, as the parameters given are always different cipher and plaintext `crypto_data_t` pointers. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

codecov-io · 2020-03-20T03:47:01Z

Codecov Report

Merging #10015 into master will increase coverage by 0.26%.
The diff coverage is 43.07%.

@@            Coverage Diff             @@
##           master   #10015      +/-   ##
==========================================
+ Coverage   79.17%   79.44%   +0.26%     
==========================================
  Files         385      385              
  Lines      122445   122403      -42     
==========================================
+ Hits        96949    97241     +292     
+ Misses      25496    25162     -334

Flag	Coverage Δ
#kernel	`79.83% <31.25%> (+0.25%)`	⬆️
#user	`66.46% <43.07%> (+0.53%)`	⬆️

Impacted Files	Coverage Δ
module/icp/algs/modes/cbc.c	`0.00% <0.00%> (ø)`
module/icp/algs/modes/ctr.c	`0.00% <0.00%> (ø)`
module/icp/algs/modes/ecb.c	`0.00% <0.00%> (ø)`
module/icp/api/kcf_cipher.c	`16.59% <ø> (+0.41%)`	⬆️
module/icp/api/kcf_digest.c	`0.00% <ø> (ø)`
module/icp/api/kcf_mac.c	`38.85% <ø> (+0.57%)`	⬆️
module/icp/io/aes.c	`29.13% <33.33%> (ø)`
module/icp/core/kcf_prov_lib.c	`63.76% <50.00%> (-0.42%)`	⬇️
module/icp/algs/modes/ccm.c	`84.36% <100.00%> (+0.39%)`	⬆️
module/icp/algs/modes/gcm.c	`81.80% <100.00%> (+0.19%)`	⬆️
... and 59 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 6b7028e...9c83a70. Read the comment docs.

module/icp/io/aes.c

dbussink · 2020-03-26T16:13:24Z

@behlendorf Is this good to go?

behlendorf · 2020-03-26T17:38:54Z

@dbussink yup, sorry about the delay. I'll get this merged right away. Thanks.

These paths are never exercised, as the parameters given are always different cipher and plaintext `crypto_data_t` pointers. Reviewed-by: Richard Laager <rlaager@wiktel.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Attila Fueloep <attila@fueloep.org> Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Closes openzfs#9661 Closes openzfs#10015

dbussink mentioned this pull request Feb 17, 2020

ICP: aes-gcm: Incomplete inplace encryption handling #9661

Closed

rlaager approved these changes Feb 17, 2020

View reviewed changes

behlendorf added the Status: Code Review Needed Ready for review and testing label Feb 18, 2020

behlendorf approved these changes Mar 5, 2020

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Mar 5, 2020

AttilaFueloep reviewed Mar 18, 2020

View reviewed changes

module/icp/io/aes.c Show resolved Hide resolved

Remove checks for null out value in encryption paths

9c83a70

These paths are never exercised, as the parameters given are always different cipher and plaintext `crypto_data_t` pointers. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

AttilaFueloep approved these changes Mar 21, 2020

View reviewed changes

module/icp/io/aes.c Show resolved Hide resolved

behlendorf merged commit 112c1bf into openzfs:master Mar 26, 2020

dbussink deleted the cleanup-null-out-value branch March 27, 2020 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove checks for null out value in encryption paths #10015

Remove checks for null out value in encryption paths #10015

dbussink commented Feb 17, 2020

rlaager left a comment

dbussink commented Feb 17, 2020

codecov bot commented Feb 18, 2020 •

edited

AttilaFueloep commented Feb 18, 2020

dbussink commented Feb 19, 2020

behlendorf commented Feb 19, 2020

dbussink commented Feb 29, 2020

behlendorf left a comment

AttilaFueloep commented Mar 5, 2020

dbussink commented Mar 6, 2020

behlendorf commented Mar 17, 2020

dbussink commented Mar 17, 2020

dbussink commented Mar 17, 2020

behlendorf commented Mar 17, 2020

AttilaFueloep commented Mar 17, 2020 •

edited

AttilaFueloep commented Mar 18, 2020

dbussink commented Mar 18, 2020

codecov-io commented Mar 20, 2020 •

edited

dbussink commented Mar 26, 2020

behlendorf commented Mar 26, 2020

Remove checks for null out value in encryption paths #10015

Remove checks for null out value in encryption paths #10015

Conversation

dbussink commented Feb 17, 2020

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

rlaager left a comment

Choose a reason for hiding this comment

dbussink commented Feb 17, 2020

codecov bot commented Feb 18, 2020 • edited

Codecov Report

AttilaFueloep commented Feb 18, 2020

dbussink commented Feb 19, 2020

behlendorf commented Feb 19, 2020

dbussink commented Feb 29, 2020

behlendorf left a comment

Choose a reason for hiding this comment

AttilaFueloep commented Mar 5, 2020

dbussink commented Mar 6, 2020

behlendorf commented Mar 17, 2020

dbussink commented Mar 17, 2020

dbussink commented Mar 17, 2020

behlendorf commented Mar 17, 2020

AttilaFueloep commented Mar 17, 2020 • edited

AttilaFueloep commented Mar 18, 2020

dbussink commented Mar 18, 2020

codecov-io commented Mar 20, 2020 • edited

Codecov Report

dbussink commented Mar 26, 2020

behlendorf commented Mar 26, 2020

codecov bot commented Feb 18, 2020 •

edited

AttilaFueloep commented Mar 17, 2020 •

edited

codecov-io commented Mar 20, 2020 •

edited