Re: [PATCH] block: Add block level changes for inline encryption

Re: [PATCH] block: Add block level changes for inline encryption

[Ladvine D Almeida]

On Monday 28 May 2018 04:54 PM, Jens Axboe wrote:
> On 5/28/18 7:43 AM, Ladvine D Almeida wrote:
>> This patch introduces new variable under bio structure to
>> facilitate inline encryption. This variable is used to
>> associate I/O requests to crypto information.
> Hard no on this, for two reasons:
>
> 1) Any additions to struct bio are scrutinized heavily and
>    need strong justification.

Thanks for sharing your feedback on the patch.
I am providing reference to an earlier article related to inline encryption support below:
https://lwn.net/Articles/717754/

In the Existing approach, the crypto transformation happens on the I/O requests before
they are actually submitted to the block level drivers for the payload transfer. This is
accomplished using the software algorithms or crypto accelerators invoked by the
device mapper or the file systems.

The inline encryption engine sits inside the controller(unlike the crypto accelerators). The
challenging part now is that we cannot perform encryption outside controller and there
is a need to communicate this crypto information to the block device drivers so they can
use the same for forming the transfer requests to the controller. This is possible
only by associating the bio to the crypto information, because the crypto context is
different for individual I/O requests.

This modification is generic that it can be used by any host controllers with the inline encryption
engine like UFS Host Controller, Mobile Storage Host Controller etc.

>
> 2) Without an actual use case, any change is always denied.
>    This is just a stand-alone patch.

The Use case is supporting the Inline Encryption Engine inside the UFS Host Controller.
UFS Host Controller has multiple key slots available for use. Each key slot can be used when
we setup disks for encryption using different keys with the device mapper.
When the I/O requests happens on each disks configured, there is a need to associate the bio to
crypto context before submitting the bio to the block layer. UFS Host Controller driver will use this
information from the bio to form the UTRD requests with associated Key ID(different ID for different
key slots). The actual encryption happens inside the controller for the I/O requests making use of the
key programmed in the key slot associated to same.

The List of patches related to the Inline Encryption support are shared below:
https://lkml.org/lkml/2018/5/28/1027
https://lkml.org/lkml/2018/5/28/1153
https://lkml.org/lkml/2018/5/28/1196
https://lkml.org/lkml/2018/5/28/1158
https://lkml.org/lkml/2018/5/28/1173
https://lkml.org/lkml/2018/5/28/1178

The implementation has been tested for Full Disk Encryption using the UFS Host Controller
with inline encryption engine on Synopsys HAPS-70 FPGA-based Prototyping System. We seen
a considerable performance improvement over the traditional approach of using crypto
accelerators that much overhead involved in the device mapper layer can now be avoided.

>
> On top of that, you have it inside BLK_CGROUP, which is
> probably not what you want.

Yes. It has to be corrected.

>
Best Regards,

Ladvine

Re: [PATCH] block: Add block level changes for inline encryption

[Jens Axboe]

On 5/31/18 1:47 AM, Ladvine D Almeida wrote:
> On Monday 28 May 2018 04:54 PM, Jens Axboe wrote:
>> On 5/28/18 7:43 AM, Ladvine D Almeida wrote:
>>> This patch introduces new variable under bio structure to
>>> facilitate inline encryption. This variable is used to
>>> associate I/O requests to crypto information.
>> Hard no on this, for two reasons:
>>
>> 1) Any additions to struct bio are scrutinized heavily and
>>    need strong justification.
> 
> Thanks for sharing your feedback on the patch.
> I am providing reference to an earlier article related to inline encryption support below:
> https://lwn.net/Articles/717754/

Took a quick look, and this looks like a classic case of something
that should just be a cloned bio. If you clone, you own the bi_private
field, which is what you need.

-- 
Jens Axboe

Re: [PATCH] block: Add block level changes for inline encryption

[Ladvine D Almeida]

On Thursday 31 May 2018 04:46 PM, Jens Axboe wrote:
> On 5/31/18 1:47 AM, Ladvine D Almeida wrote:
>> On Monday 28 May 2018 04:54 PM, Jens Axboe wrote:
>>> On 5/28/18 7:43 AM, Ladvine D Almeida wrote:
>>>> This patch introduces new variable under bio structure to
>>>> facilitate inline encryption. This variable is used to
>>>> associate I/O requests to crypto information.
>>> Hard no on this, for two reasons:
>>>
>>> 1) Any additions to struct bio are scrutinized heavily and
>>>    need strong justification.
>> Thanks for sharing your feedback on the patch.
>> I am providing reference to an earlier article related to inline encryption support below:
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lwn.net_Articles_717754_&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=z00zRD9ARrwHpe-XSl1OtUp1uNKGYoXI1G2DhOaDDBI&m=m8U0bg9QiswO2oVJgJKq3MmJpqPPK_tN667XwsjojcM&s=9VPcl80YTKwbf8T-oCxWTRahYzS2xNDHZMexpFbuepY&e=
> Took a quick look, and this looks like a classic case of something
> that should just be a cloned bio. If you clone, you own the bi_private
> field, which is what you need.

Cloning the bio gives ownership of the bi_private variable which i can use to refer to the crypto context.
But i have the following problem here:
1. In the dm-crypt subsystem, we clone the bio and assign the bi_private variable. Afterwards, generic_make_request() is done to submit I/O request to block device.
2. The bio will be cloned further in the below layers. The reference in the bi_private variable is now lost as the bio_clone function will not copy the bi_private variable.

Also, the bi_private variable is already used in the dm-crypt layer for storing its private data. This prevents me from using the same.

>

Thanks,

Ladvine

Re: [PATCH] block: Add block level changes for inline encryption

[Jens Axboe]

On 6/1/18 12:27 AM, Ladvine D Almeida wrote:
> On Thursday 31 May 2018 04:46 PM, Jens Axboe wrote:
>> On 5/31/18 1:47 AM, Ladvine D Almeida wrote:
>>> On Monday 28 May 2018 04:54 PM, Jens Axboe wrote:
>>>> On 5/28/18 7:43 AM, Ladvine D Almeida wrote:
>>>>> This patch introduces new variable under bio structure to
>>>>> facilitate inline encryption. This variable is used to
>>>>> associate I/O requests to crypto information.
>>>> Hard no on this, for two reasons:
>>>>
>>>> 1) Any additions to struct bio are scrutinized heavily and
>>>>    need strong justification.
>>> Thanks for sharing your feedback on the patch.
>>> I am providing reference to an earlier article related to inline encryption support below:
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lwn.net_Articles_717754_&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=z00zRD9ARrwHpe-XSl1OtUp1uNKGYoXI1G2DhOaDDBI&m=m8U0bg9QiswO2oVJgJKq3MmJpqPPK_tN667XwsjojcM&s=9VPcl80YTKwbf8T-oCxWTRahYzS2xNDHZMexpFbuepY&e=
>> Took a quick look, and this looks like a classic case of something
>> that should just be a cloned bio. If you clone, you own the bi_private
>> field, which is what you need.
> 
> Cloning the bio gives ownership of the bi_private variable which i can
> use to refer to the crypto context.  But i have the following problem
> here:
> 1. In the dm-crypt subsystem, we clone the bio and assign the
> bi_private variable. Afterwards, generic_make_request() is done to
> submit I/O request to block device.
> 2. The bio will be cloned further in the below layers. The reference
> in the bi_private variable is now lost as the bio_clone function will
> not copy the bi_private variable.
> 
> Also, the bi_private variable is already used in the dm-crypt layer
> for storing its private data. This prevents me from using the same.

If you clone or allocate a bio, you are the owner of bi_private. If
someone further down the stack clones it again, then they own the NEW
bi_private of the newly returned cloned. Nobody will mess with yours,
that would be a layering violation. That is the way to store data on a
per bio basis, not by adding a new random field to the bio structure.

-- 
Jens Axboe

Re: [PATCH] block: Add block level changes for inline encryption

[Jens Axboe]

On 6/6/18 1:35 AM, Ladvine D Almeida wrote:
> On Friday 01 June 2018 09:13 AM, Christoph Hellwig wrote:
>> On Mon, May 28, 2018 at 02:43:09PM +0100, Ladvine D Almeida wrote:
>>> This patch introduces new variable under bio structure to
>>> facilitate inline encryption. This variable is used to
>>> associate I/O requests to crypto information.
>> This seems to be missing a whole lot of context.  Where is the whole
>> series showing what you are trying to do?
>>
> Christoph,
> 
> The patches are generated in the below > manner, with a thought of
> sending separately to the MAINTAINERS responsible for each.

What both Christoph and I have said is that it's _impossible_ to review
changes when you don't know what is being built on top of it. The block
change, by itself, is utterly useless. The use case needs to be seen.
But apart from that, my comments on why it's doing it completely
backwards still apply, and I've outlined how you need to fix it. The
patch, in its current form, isn't going anywhere.

-- 
Jens Axboe

Re: [PATCH] block: Add block level changes for inline encryption

[Ladvine D Almeida]

On Thursday 07 June 2018 02:53 AM, Jens Axboe wrote:
> On 6/6/18 1:35 AM, Ladvine D Almeida wrote:
>> On Friday 01 June 2018 09:13 AM, Christoph Hellwig wrote:
>>> On Mon, May 28, 2018 at 02:43:09PM +0100, Ladvine D Almeida wrote:
>>>> This patch introduces new variable under bio structure to
>>>> facilitate inline encryption. This variable is used to
>>>> associate I/O requests to crypto information.
>>> This seems to be missing a whole lot of context.  Where is the whole
>>> series showing what you are trying to do?
>>>
>> Christoph,
>>
>> The patches are generated in the below > manner, with a thought of
>> sending separately to the MAINTAINERS responsible for each.
> What both Christoph and I have said is that it's _impossible_ to review
> changes when you don't know what is being built on top of it. The block
> change, by itself, is utterly useless. The use case needs to be seen.
> But apart from that, my comments on why it's doing it completely
> backwards still apply, and I've outlined how you need to fix it. The
> patch, in its current form, isn't going anywhere.
>
Jens,

Since there are implementation level concerns on both device mapper layer and block layer, I will investigate

more and work on those lines. I can send the full patch series to the relevant maintainers after addressing the

issues.

Regards,

Ladvine