| Message ID | 20210325212609.492188-1-satyat@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ensure bios aren't split in middle of crypto data unit | expand |
On 3/25/21 2:26 PM, Satya Tangirala wrote: > When a bio has an encryption context, its size must be aligned to its > crypto data unit size. A bio must not be split in the middle of a data > unit. Currently, bios are split at logical block boundaries [...] Hi Satya, Are you sure that the block layer core splits bios at logical block boundaries? Commit 9cc5169cd478 ("block: Improve physical block alignment of split bios") should have changed the behavior from splitting at logical block boundaries into splitting at physical block boundaries. Without having looked at this patch series, can the same effect be achieved by reporting the crypto data unit size as the physical block size? Thanks, Bart.
On Thu, Mar 25, 2021 at 02:51:31PM -0700, Bart Van Assche wrote: > On 3/25/21 2:26 PM, Satya Tangirala wrote: > > When a bio has an encryption context, its size must be aligned to its > > crypto data unit size. A bio must not be split in the middle of a data > > unit. Currently, bios are split at logical block boundaries [...] > > Hi Satya, > > Are you sure that the block layer core splits bios at logical block > boundaries? Commit 9cc5169cd478 ("block: Improve physical block > alignment of split bios") should have changed the behavior from > splitting at logical block boundaries into splitting at physical block > boundaries. Ah, what I really meant with that sentence was "Currently, if a bio is split, the size of the resulting bio is guaranteed to be aligned to a the lbs. The endpoint of the bio might also be aligned to a physical block boundary (which 9cc5169cd478 tries to achieve if possible), but the bio's size (and hence also its endpoint since the start of the bio is always lbs aligned) is *at least* lbs aligned". Does that sound accurate? With inline encryption, that lbs alignment guarantee is no longer enough - the bio now needs to have a size that's aligned to the bio's data unit size (which may be larger than the logical block size, but is also still a power of 2). > Without having looked at this patch series, can the same > effect be achieved by reporting the crypto data unit size as the > physical block size? That would've been awesome, but I don't think we can do that :( 1) There isn't only one crypto data unit size. A device can support, and upper layers are free to use, many different data unit sizes. 2) IIUC 9cc5169cd478 (or more accurately get_max_io_size() since the function has been changed slightly since your original patch) doesn't align the size of the bio to the pbs - it only aligns the endpoint of the bio to the pbs (and it may actually not even do that if it turns out to not be possible). Is that right? If so, that means that if the startpoint of the bio isn't pbs aligned, the size of the bio won't be pbs aligned either. > > Thanks, > > Bart.
On 3/25/21 6:39 PM, Satya Tangirala wrote: > On Thu, Mar 25, 2021 at 02:51:31PM -0700, Bart Van Assche wrote: >> Are you sure that the block layer core splits bios at logical block >> boundaries? Commit 9cc5169cd478 ("block: Improve physical block >> alignment of split bios") should have changed the behavior from >> splitting at logical block boundaries into splitting at physical block >> boundaries. > > Ah, what I really meant with that sentence was "Currently, if a bio is > split, the size of the resulting bio is guaranteed to be aligned to a > the lbs. The endpoint of the bio might also be aligned to a physical > block boundary (which 9cc5169cd478 tries to achieve if possible), but > the bio's size (and hence also its endpoint since the start of the bio > is always lbs aligned) is *at least* lbs aligned". Does that sound > accurate? That sounds better to me :-) >> Without having looked at this patch series, can the same >> effect be achieved by reporting the crypto data unit size as the >> physical block size? > > That would've been awesome, but I don't think we can do that :( > 1) There isn't only one crypto data unit size. A device can support, > and upper layers are free to use, many different data unit sizes. > 2) IIUC 9cc5169cd478 (or more accurately get_max_io_size() since the > function has been changed slightly since your original patch) > doesn't align the size of the bio to the pbs - it only aligns the > endpoint of the bio to the pbs (and it may actually not even do > that if it turns out to not be possible). Is that right? If so, > that means that if the startpoint of the bio isn't pbs aligned, the > size of the bio won't be pbs aligned either. Hmm ... if the start of a bio is not aligned to the physical block size I don't think that the block layer can do anything about the start of the bio. Anyway, I have taken a quick look at this patch series and the patch series looks pretty clean to me. I will let Christoph review this patch series since he already reviewed the previous version of this series. Bart.
On Thu, Mar 25, 2021 at 08:46:20PM -0700, Bart Van Assche wrote: > On 3/25/21 6:39 PM, Satya Tangirala wrote: > > On Thu, Mar 25, 2021 at 02:51:31PM -0700, Bart Van Assche wrote: > >> Are you sure that the block layer core splits bios at logical block > >> boundaries? Commit 9cc5169cd478 ("block: Improve physical block > >> alignment of split bios") should have changed the behavior from > >> splitting at logical block boundaries into splitting at physical block > >> boundaries. > > > > Ah, what I really meant with that sentence was "Currently, if a bio is > > split, the size of the resulting bio is guaranteed to be aligned to a > > the lbs. The endpoint of the bio might also be aligned to a physical > > block boundary (which 9cc5169cd478 tries to achieve if possible), but > > the bio's size (and hence also its endpoint since the start of the bio > > is always lbs aligned) is *at least* lbs aligned". Does that sound > > accurate? > That sounds better to me :-) > > >> Without having looked at this patch series, can the same > >> effect be achieved by reporting the crypto data unit size as the > >> physical block size? > > > > That would've been awesome, but I don't think we can do that :( > > 1) There isn't only one crypto data unit size. A device can support, > > and upper layers are free to use, many different data unit sizes. > > 2) IIUC 9cc5169cd478 (or more accurately get_max_io_size() since the > > function has been changed slightly since your original patch) > > doesn't align the size of the bio to the pbs - it only aligns the > > endpoint of the bio to the pbs (and it may actually not even do > > that if it turns out to not be possible). Is that right? If so, > > that means that if the startpoint of the bio isn't pbs aligned, the > > size of the bio won't be pbs aligned either. > > Hmm ... if the start of a bio is not aligned to the physical block size > I don't think that the block layer can do anything about the start of > the bio. Anyway, I have taken a quick look at this patch series and the > patch series looks pretty clean to me. I will let Christoph review this > patch series since he already reviewed the previous version of this series. Sounds good. Thanks for looking through the series! > > Bart.
When a bio has an encryption context, its size must be aligned to its crypto data unit size. A bio must not be split in the middle of a data unit. Currently, bios are split at logical block boundaries, but a crypto data unit size might be larger than the logical block size - e.g. a machine could be using fscrypt (which uses 4K crypto data units) with an eMMC block device with inline encryption hardware that has a logical block size of 512 bytes. So we need to support cases where the data unit size is larger than the logical block size. Patch 1 introduces blk_ksm_is_empty() that checks whether a keyslot manager advertises a non-zero number of crypto capabilities. This function helps clean up code a little. Patch 2 handles the error code from blk_ksm_register() in all callers. This return code was previously ignored by all callers because the function could only fail if the request_queue had integrity support, which the callers ensured would not be the case. But the later patches in this series will add more cases where this function might fail, so it's better to just handle the return code properly in all the callers. Patch 3 and 4 introduce blk_crypto_bio_sectors_alignment() and bio_required_sector_alignment() respectively. The former returns the required sector alignment due to any crypto requirements the bio has. The latter returns the required sector alignment due to any reason. The number of sectors in any bio (and in particular, the number of sectors passed to bio_split) *must* be aligned to the value returned by the latter function (which, of course, calls the former function to decide what to return). Patch 5 updates blk-crypto-fallback.c to respect bio_required_sector_alignment() when calling bio_split(), so that any split bio's size has the required alignment. Patch 6 introduces restrictions on the data unit sizes advertised by a keyslot manager. These restrictions come about due to the request_queue's queue_limits, and are required to ensure that blk_bio_segment_split() can always split a bio so that it has a limited number of sectors and segments, and that the number of sectors it has is non-zero and aligned to bio_required_sector_alignment(). Patch 7 updates get_max_io_size() and blk_bio_segment_split() to respect bio_required_sector_alignment(). get_max_io_size() always returns a value that is aligned to bio_required_sector_alignment(), and together with Patch 6, this is enough to ensure that if the bio is split, it is split at a crypto data unit size boundary. Since all callers to bio_split() should have been updated by the previous patches, Patch 8 adds a WARN_ON() to bio_split() when sectors isn't aligned to bio_required_sector_alignment() (the one exception is bounce.c which is legacy code and won't interact with inline encryption). This patch series was tested by running android xfstests on the SDM630 chipset (which has eMMC inline encryption hardware with logical block size 512 bytes) with test_dummy_encryption with and without the 'inlinecrypt' mount option. Satya Tangirala (8): block: introduce blk_ksm_is_empty() dm,mmc,ufshcd: handle error from blk_ksm_register() block: blk-crypto: introduce blk_crypto_bio_sectors_alignment() block: introduce bio_required_sector_alignment() block: respect bio_required_sector_alignment() in blk-crypto-fallback block: keyslot-manager: introduce blk_ksm_restrict_dus_to_queue_limits() blk-merge: Ensure bios aren't split in middle of a crypto data unit block: add WARN() in bio_split() for sector alignment block/bio.c | 1 + block/blk-crypto-fallback.c | 3 ++ block/blk-crypto-internal.h | 18 ++++++++ block/blk-merge.c | 49 ++++++++++++++-------- block/blk.h | 12 ++++++ block/keyslot-manager.c | 72 ++++++++++++++++++++++++++++++++ drivers/md/dm-table.c | 14 ++----- drivers/mmc/core/crypto.c | 6 ++- drivers/scsi/ufs/ufshcd-crypto.c | 6 ++- include/linux/keyslot-manager.h | 2 + 10 files changed, 150 insertions(+), 33 deletions(-)