Re: [RFC PATCH v3] crypto: Add IV generation algorithms

[Binoy Jayan]

Hi Gilad,

On 19 January 2017 at 15:17, Gilad Ben-Yossef  wrote:
> I tried adding sg_init_table() where I thought it was appropriate and
> it didn't resolve the issue.
>
> For what it's worth, my guess is that the difference between our
> setups is not related to Arm but to other options or the storage I'm
> using.

I was able to reproduce this again on my qemu setup with a 1GB virtual
disk. That is the same thing I do with the x86 setup as well.

> Are you using cryptd?

You mean config CRYPTO_CRYPTD?

-Binoy
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v3] crypto: Add IV generation algorithms

[Gilad Ben-Yossef]

On Thu, Jan 19, 2017 at 6:42 AM, Binoy Jayan  wrote:
> Hi Gilad,
>
> On 18 January 2017 at 20:51, Gilad Ben-Yossef  wrote:
>> I have some review comments and a bug report -
>
> Thank you very much for testing this on ARM and for the comments.

My pleasure. Thanks for writing the code :-)

>
>>> +   rinfo.iv_sector = ctx->cc_sector;
>>> +   rinfo.nents = nents;
>>> +   rinfo.iv = iv;
>>> +
>>> +   skcipher_request_set_crypt(req, dmreq->sg_in, dmreq->sg_out,
>>
>> Also, where do the scatterlist src2 and dst2 that you use
>> sg_set_page() get sg_init_table() called on?
>> I couldn't figure it out...
>
> Thank you pointing this out. I missed out to add sg_init_table(src2, 1)
> and sg_init_table(dst2, 1), but sg_set_page is used in geniv_iter_block.
> This is probably the reason for the panic on ARM platform. However it
> ran fine under qemu-x86. May be I should setup an arm platform too
> for testing.

I tried adding sg_init_table() where I thought it was appropriate and
it didn't resolve the issue.

For what it's worth, my guess is that the difference between our
setups is not related to Arm but to other options or the storage I'm
using.

Are you using cryptd?

Thanks,
Gilad

>
> Regards,
> Binoy



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v3] crypto: Add IV generation algorithms

[Binoy Jayan]

Hi Gilad,

On 18 January 2017 at 20:51, Gilad Ben-Yossef  wrote:
> I have some review comments and a bug report -

Thank you very much for testing this on ARM and for the comments.

> I'm pretty sure this needs to be
>
>  n2 = bio_segments(ctx->bio_out);

Yes you are right, that was a typo :)

>> +
>> +   rinfo.is_write = bio_data_dir(ctx->bio_in) == WRITE;
>
> Please consider wrapping the above boolean expression in parenthesis.

Well, I can do that to enhance the clarity.

>> +   rinfo.iv_sector = ctx->cc_sector;
>> +   rinfo.nents = nents;
>> +   rinfo.iv = iv;
>> +
>> +   skcipher_request_set_crypt(req, dmreq->sg_in, dmreq->sg_out,
>
> Also, where do the scatterlist src2 and dst2 that you use
> sg_set_page() get sg_init_table() called on?
> I couldn't figure it out...

Thank you pointing this out. I missed out to add sg_init_table(src2, 1)
and sg_init_table(dst2, 1), but sg_set_page is used in geniv_iter_block.
This is probably the reason for the panic on ARM platform. However it
ran fine under qemu-x86. May be I should setup an arm platform too
for testing.

Regards,
Binoy
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v3] crypto: Add IV generation algorithms

[Gilad Ben-Yossef]

Hi Binoy,


On Wed, Jan 18, 2017 at 11:40 AM, Binoy Jayan  wrote:
> Currently, the iv generation algorithms are implemented in dm-crypt.c.
> The goal is to move these algorithms from the dm layer to the kernel
> crypto layer by implementing them as template ciphers so they can be
> implemented in hardware for performance. As part of this patchset, the
> iv-generation code is moved from the dm layer to the crypto layer and
> adapt the dm-layer to send a whole 'bio' (as defined in the block layer)
> at a time. Each bio contains an in memory representation of physically
> contiguous disk blocks. The dm layer sets up a chained scatterlist of
> these blocks split into physically contiguous segments in memory so that
> DMA can be performed. Also, the key management code is moved from dm layer
> to the cryto layer since the key selection for encrypting neighboring
> sectors depend on the keycount.
>
> Synchronous crypto requests to encrypt/decrypt a sector are processed
> sequentially. Asynchronous requests if processed in parallel, are freed
> in the async callback. The dm layer allocates space for iv. The hardware
> implementations can choose to make use of this space to generate their IVs
> sequentially or allocate it on their own.
> Interface to the crypto layer - include/crypto/geniv.h
>
> Signed-off-by: Binoy Jayan 
> ---

I have some review comments and a bug report -



>   */
> -static int crypt_convert(struct crypt_config *cc,
> -struct convert_context *ctx)
> +
> +static int crypt_convert_bio(struct crypt_config *cc,
> +struct convert_context *ctx)
>  {
> +   unsigned int cryptlen, n1, n2, nents, i = 0, bytes = 0;
> +   struct skcipher_request *req;
> +   struct dm_crypt_request *dmreq;
> +   struct geniv_req_info rinfo;
> +   struct bio_vec bv_in, bv_out;
> int r;
> +   u8 *iv;
>
> atomic_set(>cc_pending, 1);
> +   crypt_alloc_req(cc, ctx);
> +
> +   req = ctx->req;
> +   dmreq = dmreq_of_req(cc, req);
> +   iv = iv_of_dmreq(cc, dmreq);
>
> -   while (ctx->iter_in.bi_size && ctx->iter_out.bi_size) {
> +   n1 = bio_segments(ctx->bio_in);
> +   n2 = bio_segments(ctx->bio_in);


I'm pretty sure this needs to be

 n2 = bio_segments(ctx->bio_out);

> +   nents = n1 > n2 ? n1 : n2;
> +   nents = nents > MAX_SG_LIST ? MAX_SG_LIST : nents;
> +   cryptlen = ctx->iter_in.bi_size;
>
> -   crypt_alloc_req(cc, ctx);
> +   DMDEBUG("dm-crypt:%s: segments:[in=%u, out=%u] bi_size=%u\n",
> +   bio_data_dir(ctx->bio_in) == WRITE ? "write" : "read",
> +   n1, n2, cryptlen);
>


>
> -   /* There was an error while processing the request. */
> -   default:
> -   atomic_dec(>cc_pending);
> -   return r;
> -   }
> +   sg_set_page(>sg_in[i], bv_in.bv_page, bv_in.bv_len,
> +   bv_in.bv_offset);
> +   sg_set_page(>sg_out[i], bv_out.bv_page, bv_out.bv_len,
> +   bv_out.bv_offset);
> +
> +   bio_advance_iter(ctx->bio_in, >iter_in, bv_in.bv_len);
> +   bio_advance_iter(ctx->bio_out, >iter_out, bv_out.bv_len);
> +
> +   bytes += bv_in.bv_len;
> +   i++;
> }
>
> -   return 0;
> +   DMDEBUG("dm-crypt: Processed %u of %u bytes\n", bytes, cryptlen);
> +
> +   rinfo.is_write = bio_data_dir(ctx->bio_in) == WRITE;

Please consider wrapping the above boolean expression in parenthesis.


> +   rinfo.iv_sector = ctx->cc_sector;
> +   rinfo.nents = nents;
> +   rinfo.iv = iv;
> +
> +   skcipher_request_set_crypt(req, dmreq->sg_in, dmreq->sg_out,

Also, where do the scatterlist src2 and dst2 that you use
sg_set_page() get sg_init_table() called on?
I couldn't figure it out...

Last but not least, when performing the following sequence on Arm64
(on latest Qemu Virt platform) -

1. cryptsetup luksFormat fs3.img
2. cryptsetup open --type luks fs3.img croot
3. mke2fs /dev/mapper/croot


[ fs3.img is a 16MB file for loopback ]

The attached kernel panic happens. The same does not occur without the patch.

Let me know if you need any additional information to recreate it.

I've tried to debug it a little but did not came up with anything
useful aside from above review notes.

Thanks!


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru
# 
# 
# mk
mkdir mkdosfs   mke2fsmkfifomknod mkpasswd  mkswapmktemp
# mk
mkdir mkdosfs   mke2fsmkfifomknod mkpasswd  mkswapmktemp
# mke2fs /dev/mapper/croot 
Filesystem label=
OS type: Linux
Block size=1024 (log=0)
Fragment size=1024 (log=0)
3584 inodes, 14336 blocks
716 blocks (5%) reserved for the super user