linux-kernel - Re: [PATCH v2 1/3] dm-inlinecrypt: Add inline encryption support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3161a67d-66f6-775e-c67e-3b5ca1aa2e1b@redhat.com>
Date: Tue, 15 Oct 2024 12:59:13 +0200 (CEST)
From: Mikulas Patocka <mpatocka@...hat.com>
To: Md Sadre Alam <quic_mdalam@...cinc.com>
cc: axboe@...nel.dk, song@...nel.org, yukuai3@...wei.com, agk@...hat.com, 
    snitzer@...nel.org, adrian.hunter@...el.com, quic_asutoshd@...cinc.com, 
    ritesh.list@...il.com, ulf.hansson@...aro.org, andersson@...nel.org, 
    konradybcio@...nel.org, kees@...nel.org, gustavoars@...nel.org, 
    linux-block@...r.kernel.org, linux-kernel@...r.kernel.org, 
    linux-raid@...r.kernel.org, dm-devel@...ts.linux.dev, 
    linux-mmc@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
    linux-hardening@...r.kernel.org, quic_srichara@...cinc.com, 
    quic_varada@...cinc.com
Subject: Re: [PATCH v2 1/3] dm-inlinecrypt: Add inline encryption support

Hi

The patch seems OK. Should it go in via the device mapper tree or the 
block layer tree?


On Mon, 16 Sep 2024, Md Sadre Alam wrote:

> +#define DM_CRYPT_DEFAULT_MAX_READ_SIZE		131072
> +#define DM_CRYPT_DEFAULT_MAX_WRITE_SIZE		131072
> +
> +static unsigned int get_max_request_size(struct inlinecrypt_config *cc, bool wrt)
> +{
> +	unsigned int val, sector_align;
> +
> +	val = !wrt ? DM_CRYPT_DEFAULT_MAX_READ_SIZE : DM_CRYPT_DEFAULT_MAX_WRITE_SIZE;
> +	if (wrt) {
> +		if (unlikely(val > BIO_MAX_VECS << PAGE_SHIFT))
> +			val = BIO_MAX_VECS << PAGE_SHIFT;
> +	}
> +	sector_align = max(bdev_logical_block_size(cc->dev->bdev), (unsigned int)cc->sector_size);
> +	val = round_down(val, sector_align);
> +	if (unlikely(!val))
> +		val = sector_align;
> +	return val >> SECTOR_SHIFT;
> +}

This piece of code was copied from the dm-crypt target. For dm-crypt, I 
was actually benchmarking the performance for various 
DM_CRYPT_DEFAULT_MAX_READ_SIZE and DM_CRYPT_DEFAULT_MAX_WRITE_SIZE values 
and I selected the values that resulted in the best performance.

You should benchmark it too to find the optimal I/O size. Perhaps you find 
out that there is no need to split big requests and this piece of code can 
be dropped.

> +               /* Omit the key for now. */
> +               DMEMIT("%s - %llu %s %llu", ctx->cipher_string, ctx->iv_offset,
> +                      ctx->dev->name, (unsigned long long)ctx->start);

What if someone reloads the table? I think you should display the key. 
dmsetup does not display the key if the "--showkeys" parameter is not 
specified.

Mikulas