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

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

[Ladvine D Almeida]

This patch introduces new variable under bio structure to
facilitate inline encryption. This variable is used to
associate I/O requests to crypto information.

Signed-off-by: Ladvine D Almeida <ladvine@synopsys.com>
---
 block/Kconfig             | 13 +++++++++++++
 block/bio.c               |  6 ++++++
 include/linux/blk_types.h |  3 +++
 3 files changed, 22 insertions(+)

diff --git a/block/Kconfig b/block/Kconfig
index 28ec557..a48ecec 100644
--- a/block/Kconfig
+++ b/block/Kconfig
@@ -128,6 +128,19 @@ config BLK_DEV_THROTTLING_LOW
 
 	Note, this is an experimental interface and could be changed someday.
 
+config BLK_DEV_INLINE_ENCRYPTION
+	bool "Enable support for block device inline encryption"
+	default n
+	help
+	Enable block devices for Inline Encryption support.
+
+	This option is required to support the inline encryption capability
+	of the block device driver, including UFS Host Controller driver.
+	When the Inline Encryption feature is enabled in the block device
+	drivers, this option will be automatically enabled.
+
+	If unsure, say N.
+
 config BLK_CMDLINE_PARSER
 	bool "Block device command line partition parser"
 	default n
diff --git a/block/bio.c b/block/bio.c
index 0a4df92..91aecf5 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -606,6 +606,9 @@ void __bio_clone_fast(struct bio *bio, struct bio *bio_src)
 	bio->bi_write_hint = bio_src->bi_write_hint;
 	bio->bi_iter = bio_src->bi_iter;
 	bio->bi_io_vec = bio_src->bi_io_vec;
+#ifdef CONFIG_BLK_DEV_INLINE_ENCRYPTION
+	bio->bi_ie_private = bio_src->bi_ie_private;
+#endif
 
 	bio_clone_blkcg_association(bio, bio_src);
 }
@@ -690,6 +693,9 @@ struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
 	bio->bi_write_hint	= bio_src->bi_write_hint;
 	bio->bi_iter.bi_sector	= bio_src->bi_iter.bi_sector;
 	bio->bi_iter.bi_size	= bio_src->bi_iter.bi_size;
+#ifdef CONFIG_BLK_DEV_INLINE_ENCRYPTION
+	bio->bi_ie_private      = bio_src->bi_ie_private;
+#endif
 
 	switch (bio_op(bio)) {
 	case REQ_OP_DISCARD:
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 4cb970c..11df096 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -183,6 +183,9 @@ struct bio {
 	void			*bi_cg_private;
 	struct bio_issue	bi_issue;
 #endif
+#ifdef CONFIG_BLK_DEV_INLINE_ENCRYPTION
+	void                    *bi_ie_private;
+#endif
 #endif
 	union {
 #if defined(CONFIG_BLK_DEV_INTEGRITY)
-- 
2.7.4

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

[Jens Axboe]

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.

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

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

-- 
Jens Axboe

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

[Ladvine D Almeida]

On Monday 28 May 2018 04:54 PM, Jens Axboe wrote:=0A=
> On 5/28/18 7:43 AM, Ladvine D Almeida wrote:=0A=
>> This patch introduces new variable under bio structure to=0A=
>> facilitate inline encryption. This variable is used to=0A=
>> associate I/O requests to crypto information.=0A=
> Hard no on this, for two reasons:=0A=
>=0A=
> 1) Any additions to struct bio are scrutinized heavily and=0A=
>    need strong justification.=0A=
=0A=
Thanks for sharing your feedback on the patch.=0A=
I am providing reference to an earlier article related to inline encryption=
 support below:=0A=
https://lwn.net/Articles/717754/=0A=
=0A=
In the Existing approach, the crypto transformation happens on the I/O requ=
ests before=0A=
they are actually submitted to the block level drivers for the payload tran=
sfer. This is=0A=
accomplished using the software algorithms or crypto accelerators invoked b=
y the=0A=
device mapper or the file systems.=0A=
=0A=
The inline encryption engine sits inside the controller(unlike the crypto a=
ccelerators). The=0A=
challenging part now is that we cannot perform encryption outside controlle=
r and there=0A=
is a need to communicate this crypto information to the block device driver=
s so they can=0A=
use the same for forming the transfer requests to the controller. This is p=
ossible=0A=
only by associating the bio to the crypto information, because the crypto c=
ontext is=0A=
different for individual I/O requests.=0A=
=0A=
This modification is generic that it can be used by any host controllers wi=
th the inline encryption=0A=
engine like UFS Host Controller, Mobile Storage Host Controller etc.=0A=
=0A=
>=0A=
> 2) Without an actual use case, any change is always denied.=0A=
>    This is just a stand-alone patch.=0A=
=0A=
The Use case is supporting the Inline Encryption Engine inside the UFS Host=
 Controller.=0A=
UFS Host Controller has multiple key slots available for use. Each key slot=
 can be used when=0A=
we setup disks for encryption using different keys with the device mapper.=
=0A=
When the I/O requests happens on each disks configured, there is a need to =
associate the bio to=0A=
crypto context before submitting the bio to the block layer. UFS Host Contr=
oller driver will use this=0A=
information from the bio to form the UTRD requests with associated Key ID(d=
ifferent ID for different=0A=
key slots). The actual encryption happens inside the controller for the I/O=
 requests making use of the=0A=
key programmed in the key slot associated to same.=0A=
=0A=
The List of patches related to the Inline Encryption support are shared bel=
ow:=0A=
https://lkml.org/lkml/2018/5/28/1027=0A=
https://lkml.org/lkml/2018/5/28/1153=0A=
https://lkml.org/lkml/2018/5/28/1196=0A=
https://lkml.org/lkml/2018/5/28/1158=0A=
https://lkml.org/lkml/2018/5/28/1173=0A=
https://lkml.org/lkml/2018/5/28/1178=0A=
=0A=
The implementation has been tested for Full Disk Encryption using the UFS H=
ost Controller=0A=
with inline encryption engine on Synopsys HAPS-70 FPGA-based Prototyping Sy=
stem. We seen=0A=
a considerable performance improvement over the traditional approach of usi=
ng crypto=0A=
accelerators that much overhead involved in the device mapper layer can now=
 be avoided.=0A=
=0A=
>=0A=
> On top of that, you have it inside BLK_CGROUP, which is=0A=
> probably not what you want.=0A=
=0A=
Yes. It has to be corrected.=0A=
=0A=
>=0A=
Best Regards,=0A=
=0A=
Ladvine=0A=
=0A=

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:=0A=
> On 5/31/18 1:47 AM, Ladvine D Almeida wrote:=0A=
>> On Monday 28 May 2018 04:54 PM, Jens Axboe wrote:=0A=
>>> On 5/28/18 7:43 AM, Ladvine D Almeida wrote:=0A=
>>>> This patch introduces new variable under bio structure to=0A=
>>>> facilitate inline encryption. This variable is used to=0A=
>>>> associate I/O requests to crypto information.=0A=
>>> Hard no on this, for two reasons:=0A=
>>>=0A=
>>> 1) Any additions to struct bio are scrutinized heavily and=0A=
>>>    need strong justification.=0A=
>> Thanks for sharing your feedback on the patch.=0A=
>> I am providing reference to an earlier article related to inline encrypt=
ion support below:=0A=
>> https://urldefense.proofpoint.com/v2/url?u=3Dhttps-3A__lwn.net_Articles_=
717754_&d=3DDwICaQ&c=3DDPL6_X_6JkXFx7AXWqB0tg&r=3Dz00zRD9ARrwHpe-XSl1OtUp1u=
NKGYoXI1G2DhOaDDBI&m=3Dm8U0bg9QiswO2oVJgJKq3MmJpqPPK_tN667XwsjojcM&s=3D9VPc=
l80YTKwbf8T-oCxWTRahYzS2xNDHZMexpFbuepY&e=3D=0A=
> Took a quick look, and this looks like a classic case of something=0A=
> that should just be a cloned bio. If you clone, you own the bi_private=0A=
> field, which is what you need.=0A=
=0A=
Cloning the bio gives ownership of the bi_private variable which i can use =
to refer to the crypto context.=0A=
But i have the following problem here:=0A=
1. In the dm-crypt subsystem, we clone the bio and assign the bi_private va=
riable. Afterwards, generic_make_request() is done to submit I/O request to=
 block device.=0A=
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 th=
e bi_private variable.=0A=
=0A=
Also, the bi_private variable is already used in the dm-crypt layer for sto=
ring its private data. This prevents me from using the same.=0A=
=0A=
>=0A=
=0A=
Thanks,=0A=
=0A=
Ladvine=0A=
=0A=

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

[Christoph Hellwig]

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?

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

[Ladvine D Almeida]

On Friday 01 June 2018 09:13 AM, Christoph Hellwig wrote:=0A=
> On Mon, May 28, 2018 at 02:43:09PM +0100, Ladvine D Almeida wrote:=0A=
>> This patch introduces new variable under bio structure to=0A=
>> facilitate inline encryption. This variable is used to=0A=
>> associate I/O requests to crypto information.=0A=
> This seems to be missing a whole lot of context.  Where is the whole=0A=
> series showing what you are trying to do?=0A=
>=0A=
Christoph,=0A=
=0A=
The patches are generated in the below manner, with a thought of sending se=
parately to the MAINTAINERS responsible for each.=0A=
=0A=
For block level changes:=0A=
https://patchwork.kernel.org/patch/10432255/=0A=
=0A=
For dmcrypt changes for Full Disk Encryption support:=0A=
https://lkml.org/lkml/2018/5/28/1027=0A=
=0A=
UFS Host Controller driver changes, patch-series for Inline Encryption supp=
ort:=0A=
https://lkml.org/lkml/2018/5/28/1187 -- cover letter=0A=
https://lkml.org/lkml/2018/5/28/1153=0A=
https://lkml.org/lkml/2018/5/28/1196=0A=
https://lkml.org/lkml/2018/5/28/1158=0A=
https://lkml.org/lkml/2018/5/28/1173=0A=
https://lkml.org/lkml/2018/5/28/1178=0A=
=0A=
Best Regards,=0A=
=0A=
Ladvine=0A=
=0A=
=0A=

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:=0A=
> On 6/6/18 1:35 AM, Ladvine D Almeida wrote:=0A=
>> On Friday 01 June 2018 09:13 AM, Christoph Hellwig wrote:=0A=
>>> On Mon, May 28, 2018 at 02:43:09PM +0100, Ladvine D Almeida wrote:=0A=
>>>> This patch introduces new variable under bio structure to=0A=
>>>> facilitate inline encryption. This variable is used to=0A=
>>>> associate I/O requests to crypto information.=0A=
>>> This seems to be missing a whole lot of context.  Where is the whole=0A=
>>> series showing what you are trying to do?=0A=
>>>=0A=
>> Christoph,=0A=
>>=0A=
>> The patches are generated in the below > manner, with a thought of=0A=
>> sending separately to the MAINTAINERS responsible for each.=0A=
> What both Christoph and I have said is that it's _impossible_ to review=
=0A=
> changes when you don't know what is being built on top of it. The block=
=0A=
> change, by itself, is utterly useless. The use case needs to be seen.=0A=
> But apart from that, my comments on why it's doing it completely=0A=
> backwards still apply, and I've outlined how you need to fix it. The=0A=
> patch, in its current form, isn't going anywhere.=0A=
>=0A=
Jens,=0A=
=0A=
Since there are implementation level concerns on both device mapper layer a=
nd block layer, I will investigate=0A=
=0A=
more and work on those lines. I can send the full patch series to the relev=
ant maintainers after addressing the=0A=
=0A=
issues.=0A=
=0A=
Regards,=0A=
=0A=
Ladvine=0A=
=0A=

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