Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Sudhakar Panneerselvam]

Could someone review this patch set, please?

Thanks
Sudhakar

> -----Original Message-----
> From: Sudhakar Panneerselvam
> Sent: Wednesday, September 16, 2020 12:40 PM
> To: agk@redhat.com; snitzer@redhat.com; dm-devel@redhat.com
> Cc: Shirley Ma <shirley.ma@oracle.com>; ssudhakarp@gmail.com; Martin
> Petersen <martin.petersen@oracle.com>
> Subject: [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths
> for skcipher devices
> 
> Hi,
> 
> This changeset allows processing of unaligned bio requests in dm crypt
> for the I/Os generated from a windows guest OS in a QEMU environment. If
> this changeset is accepted, then I will be submitting another changeset that
> addresses the similar issue in AEAD disks and dm-integrity module.
> 
> Thanks
> Sudhakar
> 
> Sudhakar Panneerselvam (2):
>   dm crypt: Allow unaligned bio buffer lengths for skcipher devices
>   dm crypt: Handle unaligned bio buffer lengths for lmk and tcw
> 
>  drivers/md/dm-crypt.c | 154 +++++++++++++++++++++++++++++++++++----
> -----------
>  1 file changed, 108 insertions(+), 46 deletions(-)
> 
> --
> 1.8.3.1
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
> 

Re: [dm-crypt] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Mike Snitzer]

You've clearly done a nice job with these changes.  Looks clean.

BUT, I'm struggling to just accept that dm-crypt needs to go to these
extra lengths purely because of one bad apple usecase.

These alignment constraints aren't new.  Are there other portions of
Linux's crypto subsystem that needed comparable fixes in order to work
with Microsfot OS initiated IO through a guest?

You forecast that these same kinds of changes are needed for AEAD and
dm-integrity... that's alarming.

Are we _certain_ there is no other way forward?
(Sorry I don't have suggestions.. I'm in "fact finding mode" ;)

Thanks,
Mike

On Wed, Sep 23 2020 at  1:01pm -0400,
Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com> wrote:

> Could someone review this patch set, please?
> 
> Thanks
> Sudhakar
> 
> > -----Original Message-----
> > From: Sudhakar Panneerselvam
> > Sent: Wednesday, September 16, 2020 12:40 PM
> > To: agk@redhat.com; snitzer@redhat.com; dm-devel@redhat.com
> > Cc: Shirley Ma <shirley.ma@oracle.com>; ssudhakarp@gmail.com; Martin
> > Petersen <martin.petersen@oracle.com>
> > Subject: [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths
> > for skcipher devices
> > 
> > Hi,
> > 
> > This changeset allows processing of unaligned bio requests in dm crypt
> > for the I/Os generated from a windows guest OS in a QEMU environment. If
> > this changeset is accepted, then I will be submitting another changeset that
> > addresses the similar issue in AEAD disks and dm-integrity module.
> > 
> > Thanks
> > Sudhakar
> > 
> > Sudhakar Panneerselvam (2):
> >   dm crypt: Allow unaligned bio buffer lengths for skcipher devices
> >   dm crypt: Handle unaligned bio buffer lengths for lmk and tcw
> > 
> >  drivers/md/dm-crypt.c | 154 +++++++++++++++++++++++++++++++++++----
> > -----------
> >  1 file changed, 108 insertions(+), 46 deletions(-)
> > 
> > --
> > 1.8.3.1
> > 
> > --
> > dm-devel mailing list
> > dm-devel@redhat.com
> > https://www.redhat.com/mailman/listinfo/dm-devel
> > 
> 

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Eric Biggers]

On Wed, Sep 23, 2020 at 09:27:32PM -0400, Mike Snitzer wrote:
> You've clearly done a nice job with these changes.  Looks clean.
> 
> BUT, I'm struggling to just accept that dm-crypt needs to go to these
> extra lengths purely because of one bad apple usecase.
> 
> These alignment constraints aren't new.  Are there other portions of
> Linux's crypto subsystem that needed comparable fixes in order to work
> with Microsfot OS initiated IO through a guest?
> 
> You forecast that these same kinds of changes are needed for AEAD and
> dm-integrity... that's alarming.
> 
> Are we _certain_ there is no other way forward?
> (Sorry I don't have suggestions.. I'm in "fact finding mode" ;)
> 

I don't understand why this is needed, since dm-crypt already sets its
logical_block_size to its crypto sector_size.  Isn't it expected that I/O that
isn't aligned to logical_block_size fails?  It's the I/O submitter's
responsibility to ensure logical_block_size alignment of all I/O segments.
Exactly how is the misaligned I/O actually being submitted here?

- Eric

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Milan Broz]

On 24/09/2020 07:14, Eric Biggers wrote:
> On Wed, Sep 23, 2020 at 09:27:32PM -0400, Mike Snitzer wrote:
>> You've clearly done a nice job with these changes.  Looks clean.
>>
>> BUT, I'm struggling to just accept that dm-crypt needs to go to these
>> extra lengths purely because of one bad apple usecase.
>>
>> These alignment constraints aren't new.  Are there other portions of
>> Linux's crypto subsystem that needed comparable fixes in order to work
>> with Microsfot OS initiated IO through a guest?
>>
>> You forecast that these same kinds of changes are needed for AEAD and
>> dm-integrity... that's alarming.
>>
>> Are we _certain_ there is no other way forward?
>> (Sorry I don't have suggestions.. I'm in "fact finding mode" ;)
>>
> 
> I don't understand why this is needed, since dm-crypt already sets its
> logical_block_size to its crypto sector_size.  Isn't it expected that I/O that
> isn't aligned to logical_block_size fails?  It's the I/O submitter's
> responsibility to ensure logical_block_size alignment of all I/O segments.
> Exactly how is the misaligned I/O actually being submitted here?

Thanks for mentioning it - exactly that I asked when reading this patch...
It seems that we are here fixing a problem that is just caused when someone
ignores clearly set restrictions.

Who is submitting these bioses? Why can it not be fixed there?

What happens with writes to fs journals, etc., is it still safe if we are
processing such unaligned bios?

Milan

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Sudhakar Panneerselvam]

Hello Milan,

> -----Original Message-----
> From: Milan Broz [mailto:gmazyland@gmail.com]
> Sent: Thursday, September 24, 2020 2:16 AM
> To: Eric Biggers <ebiggers@kernel.org>; Mike Snitzer <snitzer@redhat.com>
> Cc: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>;
> Damien.LeMoal@wdc.com; ssudhakarp@gmail.com; Martin Petersen
> <martin.petersen@oracle.com>; dm-crypt@saout.de; dm-devel@redhat.com;
> Shirley Ma <shirley.ma@oracle.com>; mpatocka@redhat.com;
> agk@redhat.com
> Subject: Re: [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer
> lengths for skcipher devices
> 
> On 24/09/2020 07:14, Eric Biggers wrote:
> > On Wed, Sep 23, 2020 at 09:27:32PM -0400, Mike Snitzer wrote:
> >> You've clearly done a nice job with these changes.  Looks clean.
> >>
> >> BUT, I'm struggling to just accept that dm-crypt needs to go to these
> >> extra lengths purely because of one bad apple usecase.
> >>
> >> These alignment constraints aren't new.  Are there other portions of
> >> Linux's crypto subsystem that needed comparable fixes in order to work
> >> with Microsfot OS initiated IO through a guest?
> >>
> >> You forecast that these same kinds of changes are needed for AEAD and
> >> dm-integrity... that's alarming.
> >>
> >> Are we _certain_ there is no other way forward?
> >> (Sorry I don't have suggestions.. I'm in "fact finding mode" ;)
> >>
> >
> > I don't understand why this is needed, since dm-crypt already sets its
> > logical_block_size to its crypto sector_size.  Isn't it expected that I/O that
> > isn't aligned to logical_block_size fails?  It's the I/O submitter's
> > responsibility to ensure logical_block_size alignment of all I/O segments.
> > Exactly how is the misaligned I/O actually being submitted here?
> 
> Thanks for mentioning it - exactly that I asked when reading this patch...
> It seems that we are here fixing a problem that is just caused when someone
> ignores clearly set restrictions.
> 
> Who is submitting these bioses? Why can it not be fixed there?
> 
> What happens with writes to fs journals, etc., is it still safe if we are
> processing such unaligned bios?

I don't follow your question regarding fs journals. I am not sure why it is not safe to process unaligned bio segment lengths of fs journals writes. Could you explain with some example on why that would be a problem?

Please see my reply to Eric's/Mike's email, in that, I explained why this issue needs to be fixed in dm-crypt. I hope I have answered to your questions there. If not, let me know, I will try to answer.

Thanks
Sudhakar

> 
> Milan

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Sudhakar Panneerselvam]

Hello Eric,

> -----Original Message-----
> From: Eric Biggers [mailto:ebiggers@kernel.org]
> Sent: Wednesday, September 23, 2020 11:14 PM
> To: Mike Snitzer <snitzer@redhat.com>
> Cc: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>;
> Damien.LeMoal@wdc.com; ssudhakarp@gmail.com; Martin Petersen
> <martin.petersen@oracle.com>; dm-crypt@saout.de; dm-devel@redhat.com;
> Shirley Ma <shirley.ma@oracle.com>; mpatocka@redhat.com; Milan Broz
> <gmazyland@gmail.com>; agk@redhat.com
> Subject: Re: [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer
> lengths for skcipher devices
> 
> On Wed, Sep 23, 2020 at 09:27:32PM -0400, Mike Snitzer wrote:
> > You've clearly done a nice job with these changes.  Looks clean.
> >
> > BUT, I'm struggling to just accept that dm-crypt needs to go to these
> > extra lengths purely because of one bad apple usecase.
> >
> > These alignment constraints aren't new.  Are there other portions of
> > Linux's crypto subsystem that needed comparable fixes in order to work
> > with Microsfot OS initiated IO through a guest?
> >
> > You forecast that these same kinds of changes are needed for AEAD and
> > dm-integrity... that's alarming.
> >
> > Are we _certain_ there is no other way forward?
> > (Sorry I don't have suggestions.. I'm in "fact finding mode" ;)
> >
> 
> I don't understand why this is needed, since dm-crypt already sets its
> logical_block_size to its crypto sector_size.  Isn't it expected that I/O that
> isn't aligned to logical_block_size fails?  It's the I/O submitter's
> responsibility to ensure logical_block_size alignment of all I/O segments.
> Exactly how is the misaligned I/O actually being submitted here?

You are right that each I/O size should be a multiple of the block device's sector size, but I am not sure if there is any constraint that individual segment lengths should be aligned to its sector size, could you help me with how this is enforced in block layer? The closest I see is "dma_alignment" member in "struct request_queue" of the low-level block device driver and as mentioned in the patch description, iSCSI, MegaRaid, qla2xxx, nvme and others have much relaxed constraint.

To your other question, the IO stack looks like this:

Windows Guest <--> Vhost-Scsi <--> LIO(scsi/target/blockio) <-->  dm-crypt <--> iSCSI block device

One real example out of my debugging: Windows sends a I/O request with 6656 bytes to vhost-scsi interface. Vhost-scsi uses translate_desc() in drivers/vhost/vhost.c to convert windows user space memory buffers to kernel iovecs. Vhost-scsi then converts the iovecs to sg entries in vhost_scsi_mapal() which is then handed over to "target" subsystem and eventually submitted to dm-crypt. This 6656 bytes IO has got 3 segments, first segment had 1584, second 4096 and the last had 976 bytes. Dm-crypt rejects the I/O after seeing the first segment length 1584 which is not a 512 byte multiple.

Let me know if there are further questions.

Thanks
Sudhakar

> 
> - Eric

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Mikulas Patocka]

On Thu, 24 Sep 2020, Sudhakar Panneerselvam wrote:

> Hello Eric,
> 
> > -----Original Message-----
> > From: Eric Biggers [mailto:ebiggers@kernel.org]
> > Sent: Wednesday, September 23, 2020 11:14 PM
> > To: Mike Snitzer <snitzer@redhat.com>
> > Cc: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>;
> > Damien.LeMoal@wdc.com; ssudhakarp@gmail.com; Martin Petersen
> > <martin.petersen@oracle.com>; dm-crypt@saout.de; dm-devel@redhat.com;
> > Shirley Ma <shirley.ma@oracle.com>; mpatocka@redhat.com; Milan Broz
> > <gmazyland@gmail.com>; agk@redhat.com
> > Subject: Re: [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer
> > lengths for skcipher devices
> > 
> > On Wed, Sep 23, 2020 at 09:27:32PM -0400, Mike Snitzer wrote:
> > > You've clearly done a nice job with these changes.  Looks clean.
> > >
> > > BUT, I'm struggling to just accept that dm-crypt needs to go to these
> > > extra lengths purely because of one bad apple usecase.
> > >
> > > These alignment constraints aren't new.  Are there other portions of
> > > Linux's crypto subsystem that needed comparable fixes in order to work
> > > with Microsfot OS initiated IO through a guest?
> > >
> > > You forecast that these same kinds of changes are needed for AEAD and
> > > dm-integrity... that's alarming.
> > >
> > > Are we _certain_ there is no other way forward?
> > > (Sorry I don't have suggestions.. I'm in "fact finding mode" ;)
> > >
> > 
> > I don't understand why this is needed, since dm-crypt already sets its
> > logical_block_size to its crypto sector_size.  Isn't it expected that I/O that
> > isn't aligned to logical_block_size fails?  It's the I/O submitter's
> > responsibility to ensure logical_block_size alignment of all I/O segments.
> > Exactly how is the misaligned I/O actually being submitted here?
> 
> You are right that each I/O size should be a multiple of the block 
> device's sector size, but I am not sure if there is any constraint that 
> individual segment lengths should be aligned to its sector size, could 
> you help me with how this is enforced in block layer? The closest I see 
> is "dma_alignment" member in "struct request_queue" of the low-level 
> block device driver and as mentioned in the patch description, iSCSI, 
> MegaRaid, qla2xxx, nvme and others have much relaxed constraint.
> 
> To your other question, the IO stack looks like this:
> 
> Windows Guest <--> Vhost-Scsi <--> LIO(scsi/target/blockio) <-->  dm-crypt <--> iSCSI block device
> 
> One real example out of my debugging: Windows sends a I/O request with 
> 6656 bytes to vhost-scsi interface. Vhost-scsi uses translate_desc() in 
> drivers/vhost/vhost.c to convert windows user space memory buffers to 
> kernel iovecs. Vhost-scsi then converts the iovecs to sg entries in 
> vhost_scsi_mapal() which is then handed over to "target" subsystem and 
> eventually submitted to dm-crypt. This 6656 bytes IO has got 3 segments, 
> first segment had 1584, second 4096 and the last had 976 bytes. Dm-crypt 
> rejects the I/O after seeing the first segment length 1584 which is not 
> a 512 byte multiple.
> 
> Let me know if there are further questions.
> 
> Thanks
> Sudhakar

Hi

I think it should be fixed in vhost-scsi.

Mikulas

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Sudhakar Panneerselvam]

Hi Mikulas,

> > Windows Guest <--> Vhost-Scsi <--> LIO(scsi/target/blockio) <-->  dm-crypt
> <--> iSCSI block device
> >
> > One real example out of my debugging: Windows sends a I/O request with
> > 6656 bytes to vhost-scsi interface. Vhost-scsi uses translate_desc() in
> > drivers/vhost/vhost.c to convert windows user space memory buffers to
> > kernel iovecs. Vhost-scsi then converts the iovecs to sg entries in
> > vhost_scsi_mapal() which is then handed over to "target" subsystem and
> > eventually submitted to dm-crypt. This 6656 bytes IO has got 3 segments,
> > first segment had 1584, second 4096 and the last had 976 bytes. Dm-crypt
> > rejects the I/O after seeing the first segment length 1584 which is not
> > a 512 byte multiple.
> >
> > Let me know if there are further questions.
> >
> > Thanks
> > Sudhakar
> 
> Hi
> 
> I think it should be fixed in vhost-scsi.

In the above example of 6656 bytes I/O, windows allocates 6656 bytes virtually contiguous I/O. This IO, when it lands in the kernel, translates to 3 physically discontiguous pages, that's why translate_desc() had to create 3 iovecs to handle this I/O. I don't understand how vhost-scsi could have solved this issue. Only other possibility I see is to have windows fix it by always sending 512 byte aligned buffer lengths, but going with my earlier point that every other component in the Linux IO path handles this case well except for dm-crypt, so it make more sense to fix it in dm-crypt.

Thanks
Sudhakar

> 
> Mikulas
> 

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Mikulas Patocka]

On Thu, 24 Sep 2020, Sudhakar Panneerselvam wrote:

> Hi Mikulas,
> 
> > > Windows Guest <--> Vhost-Scsi <--> LIO(scsi/target/blockio) <-->  dm-crypt
> > <--> iSCSI block device
> > >
> > > One real example out of my debugging: Windows sends a I/O request with
> > > 6656 bytes to vhost-scsi interface. Vhost-scsi uses translate_desc() in
> > > drivers/vhost/vhost.c to convert windows user space memory buffers to
> > > kernel iovecs. Vhost-scsi then converts the iovecs to sg entries in
> > > vhost_scsi_mapal() which is then handed over to "target" subsystem and
> > > eventually submitted to dm-crypt. This 6656 bytes IO has got 3 segments,
> > > first segment had 1584, second 4096 and the last had 976 bytes. Dm-crypt
> > > rejects the I/O after seeing the first segment length 1584 which is not
> > > a 512 byte multiple.
> > >
> > > Let me know if there are further questions.
> > >
> > > Thanks
> > > Sudhakar
> > 
> > Hi
> > 
> > I think it should be fixed in vhost-scsi.
> 
> In the above example of 6656 bytes I/O, windows allocates 6656 bytes 
> virtually contiguous I/O. This IO, when it lands in the kernel, 
> translates to 3 physically discontiguous pages, that's why 
> translate_desc() had to create 3 iovecs to handle this I/O. I don't 
> understand how vhost-scsi could have solved this issue.

By copying it to a temporary aligned buffer and issuing I/O on this 
buffer.

> Only other 
> possibility I see is to have windows fix it by always sending 512 byte 
> aligned buffer lengths, but going with my earlier point that every other 
> component in the Linux IO path handles this case well except for 
> dm-crypt, so it make more sense to fix it in dm-crypt.
> 
> Thanks
> Sudhakar

Are you sure that the problem is only with dm-crypt? You haven't tried all 
the existing block device drivers, have you?

Mikulas

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Sudhakar Panneerselvam]

> By copying it to a temporary aligned buffer and issuing I/O on this
> buffer.

I don't like this idea. Because, you need to allocate additional pages for the entire I/O size(for the misaligned case, if you think through carefully, you will know why we have to allocate a temporary buffer that is as big as the original IO) and on top of it, copying the buffer from original to temporary buffer which is all unnecessary while it can simply be fixed in dm-crypt without any of these additional overheads.

> 
> > Only other
> > possibility I see is to have windows fix it by always sending 512 byte
> > aligned buffer lengths, but going with my earlier point that every other
> > component in the Linux IO path handles this case well except for
> > dm-crypt, so it make more sense to fix it in dm-crypt.
> >
> > Thanks
> > Sudhakar
> 
> Are you sure that the problem is only with dm-crypt? You haven't tried all
> the existing block device drivers, have you?

My question is, why dm-crypt worries about alignment requirement of other layers? Is there anything that impacts dm-crypt if the segment lengths are not aligned?(I believe this case is just not handled so far in dm-crypt and my patch addresses it). Should dm-crypt not just pass on all those I/O requests to those respective layers to handle it which will be more graceful?

-Sudhakar

> 
> Mikulas
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
> 

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Mikulas Patocka]

On Thu, 24 Sep 2020, Sudhakar Panneerselvam wrote:

> > By copying it to a temporary aligned buffer and issuing I/O on this
> > buffer.
> 
> I don't like this idea. Because, you need to allocate additional pages 
> for the entire I/O size(for the misaligned case, if you think through 

You can break the I/O to smaller pieces. You can use mempool for 
pre-allocation of the pages.

> carefully, you will know why we have to allocate a temporary buffer that 
> is as big as the original IO) and on top of it, copying the buffer from 
> original to temporary buffer which is all unnecessary while it can 
> simply be fixed in dm-crypt without any of these additional overheads.
> 
> > 
> > > Only other
> > > possibility I see is to have windows fix it by always sending 512 byte
> > > aligned buffer lengths, but going with my earlier point that every other
> > > component in the Linux IO path handles this case well except for
> > > dm-crypt, so it make more sense to fix it in dm-crypt.
> > >
> > > Thanks
> > > Sudhakar
> > 
> > Are you sure that the problem is only with dm-crypt? You haven't tried all
> > the existing block device drivers, have you?
> 
> My question is, why dm-crypt worries about alignment requirement of 
> other layers? Is there anything that impacts dm-crypt if the segment 
> lengths are not aligned?(I believe this case is just not handled so far 

Because the code is simpler if it assumes aligned buffers.

> in dm-crypt and my patch addresses it). Should dm-crypt not just pass on 
> all those I/O requests to those respective layers to handle it which 
> will be more graceful?
> 
> -Sudhakar

So, suppose that we change dm-crypt to handle your workload - what are you 
going to do with other block device drivers that assume aligned bio vector 
length? How will you find all the other drivers that need to be changed?

You are claiming that "every other component in the Linux IO path handles 
this case well except for dm-crypt", but this claim is wrong. There are 
other driver that don't handle misaligned length as well.

Mikulas

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Sudhakar Panneerselvam]

> 
> On Thu, 24 Sep 2020, Sudhakar Panneerselvam wrote:
> 
> > > By copying it to a temporary aligned buffer and issuing I/O on this
> > > buffer.
> >
> > I don't like this idea. Because, you need to allocate additional pages
> > for the entire I/O size(for the misaligned case, if you think through
> 
> You can break the I/O to smaller pieces. You can use mempool for
> pre-allocation of the pages.

Assuming we do this, how is this code simpler(based on your
comment below) than the fix in dm-crypt? In fact, this approach 
would make the code change look bad in vhost, at the same time
having performance penalty. By doing this, we are just moving the 
responsibility to other unrelated component.

> 
> > carefully, you will know why we have to allocate a temporary buffer that
> > is as big as the original IO) and on top of it, copying the buffer from
> > original to temporary buffer which is all unnecessary while it can
> > simply be fixed in dm-crypt without any of these additional overheads.
> >
> > >
> > > > Only other
> > > > possibility I see is to have windows fix it by always sending 512 byte
> > > > aligned buffer lengths, but going with my earlier point that every other
> > > > component in the Linux IO path handles this case well except for
> > > > dm-crypt, so it make more sense to fix it in dm-crypt.
> > > >
> > > > Thanks
> > > > Sudhakar
> > >
> > > Are you sure that the problem is only with dm-crypt? You haven't tried
> all
> > > the existing block device drivers, have you?
> >
> > My question is, why dm-crypt worries about alignment requirement of
> > other layers? Is there anything that impacts dm-crypt if the segment
> > lengths are not aligned?(I believe this case is just not handled so far
> 
> Because the code is simpler if it assumes aligned buffers.

Did you get a chance to review my changes? If you want more documentation,
improve the code, etc, let me know, I can do that if there is scope for that.

> 
> > in dm-crypt and my patch addresses it). Should dm-crypt not just pass on
> > all those I/O requests to those respective layers to handle it which
> > will be more graceful?
> >
> > -Sudhakar
> 
> So, suppose that we change dm-crypt to handle your workload - what are
> you
> going to do with other block device drivers that assume aligned bio vector
> length? How will you find all the other drivers that need to be changed?
> 
> You are claiming that "every other component in the Linux IO path handles
> this case well except for dm-crypt", but this claim is wrong. There are
> other driver that don't handle misaligned length as well.

I should not have said, "every other component", I take that back, sorry. How
about doing something like this in crypt_convert_block_skcipher():

Add a check that looks at the alignment requirement of the low-level driver
and reject the I/O if it doesn't meet that requirement. This means, we still
need to handle the case in this function where the low lever driver support
unaligned buffer lengths, that means, my other changes in this function
would still be needed.

Is this acceptable to everyone?

-Sudhakar

> 
> Mikulas
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
> 

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Damien Le Moal]

On 2020/09/25 4:14, Sudhakar Panneerselvam wrote:
>>
>> On Thu, 24 Sep 2020, Sudhakar Panneerselvam wrote:
>>
>>>> By copying it to a temporary aligned buffer and issuing I/O on this
>>>> buffer.
>>>
>>> I don't like this idea. Because, you need to allocate additional pages
>>> for the entire I/O size(for the misaligned case, if you think through
>>
>> You can break the I/O to smaller pieces. You can use mempool for
>> pre-allocation of the pages.
> 
> Assuming we do this, how is this code simpler(based on your
> comment below) than the fix in dm-crypt? In fact, this approach 
> would make the code change look bad in vhost, at the same time
> having performance penalty. By doing this, we are just moving the 
> responsibility to other unrelated component.

Because vhost is at the top of the block-io food chain. Fixing the unaligned
segments there will ensure that it does not matter what device is under it. It
will work.

I am still baffled that the unaligned segments go through in the first place...
Do we have something missing in the BIO code ?

> 
>>
>>> carefully, you will know why we have to allocate a temporary buffer that
>>> is as big as the original IO) and on top of it, copying the buffer from
>>> original to temporary buffer which is all unnecessary while it can
>>> simply be fixed in dm-crypt without any of these additional overheads.
>>>
>>>>
>>>>> Only other
>>>>> possibility I see is to have windows fix it by always sending 512 byte
>>>>> aligned buffer lengths, but going with my earlier point that every other
>>>>> component in the Linux IO path handles this case well except for
>>>>> dm-crypt, so it make more sense to fix it in dm-crypt.
>>>>>
>>>>> Thanks
>>>>> Sudhakar
>>>>
>>>> Are you sure that the problem is only with dm-crypt? You haven't tried
>> all
>>>> the existing block device drivers, have you?
>>>
>>> My question is, why dm-crypt worries about alignment requirement of
>>> other layers? Is there anything that impacts dm-crypt if the segment
>>> lengths are not aligned?(I believe this case is just not handled so far
>>
>> Because the code is simpler if it assumes aligned buffers.
> 
> Did you get a chance to review my changes? If you want more documentation,
> improve the code, etc, let me know, I can do that if there is scope for that.
> 
>>
>>> in dm-crypt and my patch addresses it). Should dm-crypt not just pass on
>>> all those I/O requests to those respective layers to handle it which
>>> will be more graceful?
>>>
>>> -Sudhakar
>>
>> So, suppose that we change dm-crypt to handle your workload - what are
>> you
>> going to do with other block device drivers that assume aligned bio vector
>> length? How will you find all the other drivers that need to be changed?
>>
>> You are claiming that "every other component in the Linux IO path handles
>> this case well except for dm-crypt", but this claim is wrong. There are
>> other driver that don't handle misaligned length as well.
> 
> I should not have said, "every other component", I take that back, sorry. How
> about doing something like this in crypt_convert_block_skcipher():
> 
> Add a check that looks at the alignment requirement of the low-level driver
> and reject the I/O if it doesn't meet that requirement. This means, we still
> need to handle the case in this function where the low lever driver support
> unaligned buffer lengths, that means, my other changes in this function
> would still be needed.
> 
> Is this acceptable to everyone?
> 
> -Sudhakar
> 
>>
>> Mikulas
>>
>> --
>> dm-devel mailing list
>> dm-devel@redhat.com
>> https://www.redhat.com/mailman/listinfo/dm-devel
>>
> 


-- 
Damien Le Moal
Western Digital Research

Re: [dm-crypt] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Mike Snitzer]

On Thu, Sep 24 2020 at  9:09pm -0400,
Damien Le Moal <Damien.LeMoal@wdc.com> wrote:

> On 2020/09/25 4:14, Sudhakar Panneerselvam wrote:
> >>
> >> On Thu, 24 Sep 2020, Sudhakar Panneerselvam wrote:
> >>
> >>>> By copying it to a temporary aligned buffer and issuing I/O on this
> >>>> buffer.
> >>>
> >>> I don't like this idea. Because, you need to allocate additional pages
> >>> for the entire I/O size(for the misaligned case, if you think through
> >>
> >> You can break the I/O to smaller pieces. You can use mempool for
> >> pre-allocation of the pages.
> > 
> > Assuming we do this, how is this code simpler(based on your
> > comment below) than the fix in dm-crypt? In fact, this approach 
> > would make the code change look bad in vhost, at the same time
> > having performance penalty. By doing this, we are just moving the 
> > responsibility to other unrelated component.
> 
> Because vhost is at the top of the block-io food chain. Fixing the unaligned
> segments there will ensure that it does not matter what device is under it. It
> will work.

Right, I agree. This should be addressed in vhost-scsi.  And vhost-scsi
probably needs to be interfacing through block core to submit IO that
respects the limits of its underlying block device.

So please lift your proposed dm-crypt changes to vhost-scsi:
https://patchwork.kernel.org/patch/11781207/
https://patchwork.kernel.org/patch/11781053/

Maybe work with vhost-scsi maintainers to see about making the code
reusable in block core; so that any future unaligned application IO is
dealt in other drivers using the same common code.

But I'm not interested in taking these changes into dm-crypt:

NAK

> I am still baffled that the unaligned segments go through in the first place...
> Do we have something missing in the BIO code ?

Cc'ing linux-block, could be.

Thanks,
Mike

Re: [dm-crypt] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Mikulas Patocka]

On Wed, 23 Sep 2020, Mike Snitzer wrote:

> You've clearly done a nice job with these changes.  Looks clean.
> 
> BUT, I'm struggling to just accept that dm-crypt needs to go to these
> extra lengths purely because of one bad apple usecase.
> 
> These alignment constraints aren't new.  Are there other portions of
> Linux's crypto subsystem that needed comparable fixes in order to work
> with Microsfot OS initiated IO through a guest?
> 
> You forecast that these same kinds of changes are needed for AEAD and
> dm-integrity... that's alarming.

... and dm-writecache also expects that the bio is aligned ...

Mikulas

> Are we _certain_ there is no other way forward?
> (Sorry I don't have suggestions.. I'm in "fact finding mode" ;)
> 
> Thanks,
> Mike

Re: [dm-crypt] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Sudhakar Panneerselvam]

Hello Mike,

> -----Original Message-----
> From: Mike Snitzer [mailto:snitzer@redhat.com]
> Sent: Wednesday, September 23, 2020 7:28 PM
> To: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
> Cc: agk@redhat.com; dm-devel@redhat.com; dm-crypt@saout.de;
> mpatocka@redhat.com; Damien.LeMoal@wdc.com; Shirley Ma
> <shirley.ma@oracle.com>; ssudhakarp@gmail.com; Martin Petersen
> <martin.petersen@oracle.com>; Milan Broz <gmazyland@gmail.com>
> Subject: Re: [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for
> skcipher devices
> 
> You've clearly done a nice job with these changes.  Looks clean.

Thanks.

> 
> BUT, I'm struggling to just accept that dm-crypt needs to go to these
> extra lengths purely because of one bad apple usecase.

During my initial stages of investigation, I had the same impression as yours, but digging further, I felt fixing dm-crypt would be more appropriate.

In the following two test cases, windows installation/formatting/booting, all were successful.

Windows Guest <--> Vhost-Scsi <--> LIO(scsi/target/blockio) <--> iSCSI block device
Windows Guest <--> Vhost-Scsi <--> LIO(scsi/target/blockio) <--> Local MegaRaid Block Device

When I inserted dm-crypt in the IO path, that is where I noticed windows boot/install/format failures. The IO path is like this:

Windows Guest <--> Vhost-Scsi <--> LIO(scsi/target/blockio) <-->  dm-crypt <--> iSCSI block device

After these tests, I realized that, when every other component in the IO stack can handle unaligned bio lengths, why can't make dm-crypt handle the same. Hence, the patch.
 
> 
> These alignment constraints aren't new.  Are there other portions of
> Linux's crypto subsystem that needed comparable fixes in order to work
> with Microsfot OS initiated IO through a guest?

I ran basic test with "--cipher aes-xts-plain64" and verified the fix. Also, noticed that the crypto subsystem uses APIs in crypto/skcipher.c which are already specifically written to handle unaligned buffers.

> 
> You forecast that these same kinds of changes are needed for AEAD and
> dm-integrity... that's alarming.

I understand the concern. I have tried to ensure the patch doesn't result in degrade in performance for the Linux use case. If at all, there is any performance impact due to this change, it will be only for windows(I didn't see any performance drop in my test, though).

Thanks
Sudhakar

> 
> Are we _certain_ there is no other way forward?
> (Sorry I don't have suggestions.. I'm in "fact finding mode" ;)
> 
> Thanks,
> Mike
> 
> On Wed, Sep 23 2020 at  1:01pm -0400,
> Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com> wrote:
> 
> > Could someone review this patch set, please?
> >
> > Thanks
> > Sudhakar
> >
> > > -----Original Message-----
> > > From: Sudhakar Panneerselvam
> > > Sent: Wednesday, September 16, 2020 12:40 PM
> > > To: agk@redhat.com; snitzer@redhat.com; dm-devel@redhat.com
> > > Cc: Shirley Ma <shirley.ma@oracle.com>; ssudhakarp@gmail.com; Martin
> > > Petersen <martin.petersen@oracle.com>
> > > Subject: [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer
> lengths
> > > for skcipher devices
> > >
> > > Hi,
> > >
> > > This changeset allows processing of unaligned bio requests in dm crypt
> > > for the I/Os generated from a windows guest OS in a QEMU environment.
> If
> > > this changeset is accepted, then I will be submitting another changeset that
> > > addresses the similar issue in AEAD disks and dm-integrity module.
> > >
> > > Thanks
> > > Sudhakar
> > >
> > > Sudhakar Panneerselvam (2):
> > >   dm crypt: Allow unaligned bio buffer lengths for skcipher devices
> > >   dm crypt: Handle unaligned bio buffer lengths for lmk and tcw
> > >
> > >  drivers/md/dm-crypt.c | 154
> +++++++++++++++++++++++++++++++++++----
> > > -----------
> > >  1 file changed, 108 insertions(+), 46 deletions(-)
> > >
> > > --
> > > 1.8.3.1
> > >
> > > --
> > > dm-devel mailing list
> > > dm-devel@redhat.com
> > > https://www.redhat.com/mailman/listinfo/dm-devel
> > >
> >
> 

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Mikulas Patocka]

On Wed, 23 Sep 2020, Sudhakar Panneerselvam wrote:

> Could someone review this patch set, please?
> 
> Thanks
> Sudhakar

Hi

I'd like to ask - what sector size do you use in dm-crypt? Could the issue 
be fixed just by using the smallest possible 512-byte sectors?

What I/O method does qemu use? Is it direct i/o + aio? What is unaligned - 
the base address? Or length? Or the sector number? Could you use strace on 
qemu and show an example of an I/O that fails due to alignment?

Mikulas

Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Sudhakar Panneerselvam]

Hello Mikulas,

> -----Original Message-----
> From: Mikulas Patocka [mailto:mpatocka@redhat.com]
> Sent: Thursday, September 24, 2020 6:40 AM
> To: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
> Cc: agk@redhat.com; snitzer@redhat.com; dm-devel@redhat.com; dm-
> crypt@saout.de; Damien.LeMoal@wdc.com; Shirley Ma
> <shirley.ma@oracle.com>; ssudhakarp@gmail.com; Martin Petersen
> <martin.petersen@oracle.com>
> Subject: RE: [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer
> lengths for skcipher devices
> 
> 
> 
> On Wed, 23 Sep 2020, Sudhakar Panneerselvam wrote:
> 
> > Could someone review this patch set, please?
> >
> > Thanks
> > Sudhakar
> 
> Hi
> 
> I'd like to ask - what sector size do you use in dm-crypt? Could the issue
> be fixed just by using the smallest possible 512-byte sectors?

My test has been with 512 bytes sector length. 

> 
> What I/O method does qemu use? Is it direct i/o + aio? What is unaligned -
> the base address? Or length? Or the sector number? Could you use strace on
> qemu and show an example of an I/O that fails due to alignment?

Please see my update on June 11 @ https://github.com/virtio-win/kvm-guest-drivers-windows/issues/446 when I originally posted this issue to windows virtio forum. It has some finer details of the reproduction steps and might answer some of your questions.

Also, please see my reply to Eric's email on some of the low level details of function call sequence leading to the issue. If there are further questions, let me know.

Thanks
Sudhakar

> 
> Mikulas
>