| Message ID | d3624fab-e6fc-f14d-c780-db703c809068@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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?
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.
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. For block level changes: https://patchwork.kernel.org/patch/10432255/ For dmcrypt changes for Full Disk Encryption support: https://lkml.org/lkml/2018/5/28/1027 UFS Host Controller driver changes, patch-series for Inline Encryption support: https://lkml.org/lkml/2018/5/28/1187 -- cover letter 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 Best Regards, Ladvine
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.
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
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)
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(+)