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

FreeBSD: disable the use of hardware crypto offload drivers for now #11612

Merged
merged 1 commit into from
Feb 18, 2021

Conversation

markjdb
Copy link
Contributor

@markjdb markjdb commented Feb 17, 2021

Disable the use of hardware crypto offloads when using ZFS' encryption capabilities on FreeBSD.

Motivation and Context

A couple of problems, detailed in the commit message, prevent ZFS encryption from working properly with hardware offload drivers on FreeBSD. As a temporary measure, prevent ZFS from using such drivers.

Description

The change modifies FreeBSD-specific crypto session setup code such that only software drivers (e.g., AES-NI wrappers) will be used. I believe this will not present a regression for any users since a bug in the FreeBSD crypto request completion handler means that asynchronous completions are not handled correctly.

How Has This Been Tested?

Build-tested, made the corresponding modification to FreeBSD's in-tree copy of ZFS and verified that it refuses to attach to hardware drivers.

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)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@ghost ghost added Component: Encryption "native encryption" feature Status: Code Review Needed Ready for review and testing labels Feb 17, 2021
@amotin
Copy link
Member

amotin commented Feb 17, 2021

IIRC, software AES-CCM crypto on FreeBSD is single-threaded and extremely slow.

@amotin
Copy link
Member

amotin commented Feb 17, 2021

Ah, nevermind probably, on 13 we have aesni(4) reporting as CRYPTOCAP_F_SOFTWARE | CRYPTOCAP_F_ACCEL_SOFTWARE, so it should be acceptable.

@markjdb
Copy link
Contributor Author

markjdb commented Feb 17, 2021

Ah, nevermind probably, on 13 we have aesni(4) reporting as CRYPTOCAP_F_SOFTWARE | CRYPTOCAP_F_ACCEL_SOFTWARE, so it should be acceptable.

Right, it is not my intention to disable use of aesni(4). But you are right, on 12 aesni(4) reports as a hardware driver...

First, the crypto request completion handler contains a bug in that it
fails to reset fs_done correctly after the request is completed.  This
is only a problem for asynchronous drivers.  Second, some hardware
drivers have input constraints which ZFS does not satisfy.  For
instance, ccp(4) apparently requires the AAD length for AES-GCM to be a
multiple of the cipher block size, and with qat(4) the AES-GCM AAD
length may not be longer than 240 bytes.  FreeBSD's generic crypto
framework doesn't have a mechanism to automatically fall back to a
software implementation if a hardware driver cannot process a request,
and ZFS does not tolerate such errors.

The plan is to implement such a fallback mechanism, but with FreeBSD
13.0 approaching we should simply disable the use hardware drivers for
now.

Signed-off-by: Mark Johnston <[email protected]>
@markjdb
Copy link
Contributor Author

markjdb commented Feb 17, 2021

Updated to limit the change to FreeBSD 13 and later, and to a comment explaining the difference.

@amotin
Copy link
Member

amotin commented Feb 17, 2021

I see the new comment, but not version checks. What am I missing?

@markjdb
Copy link
Contributor Author

markjdb commented Feb 17, 2021

There are two implementations of freebsd_crypt_newsession(), guarded by #if __FreeBSD_version >= 1300087.

@amotin
Copy link
Member

amotin commented Feb 17, 2021

Ah. OK. :)

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

@markjdb thanks! @amotin I'll go ahead and merge this if you're happy with it to.

@amotin
Copy link
Member

amotin commented Feb 18, 2021

@behlendorf I have no idea about the original problem, but the workaround is OK for me.

@behlendorf behlendorf merged commit e7adccf into openzfs:master Feb 18, 2021
behlendorf pushed a commit that referenced this pull request Mar 5, 2021
First, the crypto request completion handler contains a bug in that it
fails to reset fs_done correctly after the request is completed.  This
is only a problem for asynchronous drivers.  Second, some hardware
drivers have input constraints which ZFS does not satisfy.  For
instance, ccp(4) apparently requires the AAD length for AES-GCM to be a
multiple of the cipher block size, and with qat(4) the AES-GCM AAD
length may not be longer than 240 bytes.  FreeBSD's generic crypto
framework doesn't have a mechanism to automatically fall back to a
software implementation if a hardware driver cannot process a request,
and ZFS does not tolerate such errors.

The plan is to implement such a fallback mechanism, but with FreeBSD
13.0 approaching we should simply disable the use hardware drivers for
now.

Reviewed-by: Ryan Moeller <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Mark Johnston <[email protected]>
Closes #11612
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
First, the crypto request completion handler contains a bug in that it
fails to reset fs_done correctly after the request is completed.  This
is only a problem for asynchronous drivers.  Second, some hardware
drivers have input constraints which ZFS does not satisfy.  For
instance, ccp(4) apparently requires the AAD length for AES-GCM to be a
multiple of the cipher block size, and with qat(4) the AES-GCM AAD
length may not be longer than 240 bytes.  FreeBSD's generic crypto
framework doesn't have a mechanism to automatically fall back to a
software implementation if a hardware driver cannot process a request,
and ZFS does not tolerate such errors.

The plan is to implement such a fallback mechanism, but with FreeBSD
13.0 approaching we should simply disable the use hardware drivers for
now.

Reviewed-by: Ryan Moeller <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Mark Johnston <[email protected]>
Closes openzfs#11612
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
First, the crypto request completion handler contains a bug in that it
fails to reset fs_done correctly after the request is completed.  This
is only a problem for asynchronous drivers.  Second, some hardware
drivers have input constraints which ZFS does not satisfy.  For
instance, ccp(4) apparently requires the AAD length for AES-GCM to be a
multiple of the cipher block size, and with qat(4) the AES-GCM AAD
length may not be longer than 240 bytes.  FreeBSD's generic crypto
framework doesn't have a mechanism to automatically fall back to a
software implementation if a hardware driver cannot process a request,
and ZFS does not tolerate such errors.

The plan is to implement such a fallback mechanism, but with FreeBSD
13.0 approaching we should simply disable the use hardware drivers for
now.

Reviewed-by: Ryan Moeller <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Mark Johnston <[email protected]>
Closes openzfs#11612
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Encryption "native encryption" feature Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants