[dm-devel] Changes in DM_MULTIPATH_DEVICE_PATH in multipath-tools 0.7.7
Hi, folks!
So I've just been on a two-day debugging odyssey into why Fedora 29's
installer couldn't see firmware RAID devices, and the culprit turned
out to be multipath-tools:
https://bugzilla.redhat.com/show_bug.cgi?id=1628192
to summarize, the problem is in how multipath-tool's udev rules set the
DM_MULTIPATH_DEVICE_PATH udev device property. The relevant commits are
these two:
https://git.opensvc.com/gitweb.cgi?p=multipath-tools/.git;a=commit;h=b729b8f3a21fe6867165b2533496ebdfbc493263
https://git.opensvc.com/gitweb.cgi?p=multipath-tools/.git;a=commit;h=7e867718d1d0133e8ed34835320451f93358f2f9
Prior to the first commit, for a device that is a valid multipath
device, the property would be set with a value of '1'. For a device
that is *not* a valid multipath device, the property would *not be set
at all*. The udev rule that set it was:
ENV{DM_MULTIPATH_DEVICE_PATH}!="1", \
PROGRAM=="$env{MPATH_SBIN_PATH}/multipath -u %k", \
ENV{DM_MULTIPATH_DEVICE_PATH}="1", ENV{ID_FS_TYPE}="mpath_member", \
ENV{SYSTEMD_READY}="0"
...which basically means "run multipath -u (device), if that command
succeeds, set DM_MULTIPATH_DEVICE_PATH to '1' (and a couple other
values too). If it fails, don't do anything."
So obviously, for non-multipath devices, the property wasn't set. The
first commit changed this. It changed the udev rule to this:
IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
That basically means "run multipath -u (device), expect its output to
be lines of 'KEY=value' pairs, and import those pairs as device
properties". At the same time, multipath -u itself was changed so that
if the device *is* a valid multipath device, it outputs this to stdout:
DM_MULTIPATH_DEVICE_PATH="1"
if the device is *not* a valid multipath device, it outputs this:
DM_MULTIPATH_DEVICE_PATH="0"
so it's pretty easy to see that the behaviour has changed: now, if the
device is not a valid multipath device, DM_MULTIPATH_DEVICE_PATH is set
to the value "0", instead of not being set at all.
This turns out to have consequences, because two *other* udev rules
files (in Fedora at least) use this check:
ENV{DM_MULTIPATH_DEVICE_PATH}=="?*"
that check will match if DM_MULTIPATH_DEVICE_PATH is set to *any one or
more characters at all*...including '0'.
The upshot of this is that 65-md-incremental.rules basically gets
skipped entirely any time this happens, and our firmware RAID set
doesn't get initialized by that rule set as it should. The other rules
file that uses this check is 80-udisks2.rules; the impact there is that
various UDISKS_MD_(FOO) device properties may not get set. I'm not sure
what the consequences of that are.
(In passing I'll note that the commit message claims "The exit status
remains as usual.", but that is not true. The old 'multipath -u' exited
1 if the device was not a valid multipath device. The new one exits 0
in this case. I don't know if this is intentional or an oversight).
The *second* commit introduces a new possible value for
DM_MULTIPATH_DEVICE_PATH - "2", meaning "this *might* be a multipath
device, but we're not sure yet." This *may* also be significant to
other rules, because there are several that use this check:
ENV{DM_MULTIPATH_DEVICE_PATH}=="1"
it's at least *possible* that some of them should do this instead:
ENV{DM_MULTIPATH_DEVICE_PATH}=="[12]"
but I'm not sure. If they should go ahead and do their thing anyway in
the '2' case, obviously, it's fine. Also, I'm not sure the '2' value
ever really "escapes" the multipath rules file - it looks like it may
only ever be handled there, and other rules will only ever see 0, 1 or
'unset'. Again, in that case this wouldn't be a problem.
So basically I just wanted to flag this up and see what folks want to
do about it. To fix the immediate issue for Fedora I sent an mdadm
build that changes the check from "?*" to "1". However, we may want to
consider changing all the checks to "1" or "[12]", and/or changing
multipath -u to behave more like it used to. It could just print
*nothing* if the device is not a valid multipath one, for instance
(which should result in the device property not being set at all, just
like the old behaviour). Or it could print this:
DM_MULTIPATH_DEVICE_PATH=""
though I'm not 100% sure of the consequences of doing that, in the
context of udev device properties.
Thanks folks!
--
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] linux-next: Tree for Sep 13 (dm-thin-pool)
On Thu, Sep 13 2018 at 8:45pm -0400, Randy Dunlap wrote: > On 9/13/18 11:51 AM, Mike Snitzer wrote: > > On Thu, Sep 13 2018 at 1:28pm -0400, > > Randy Dunlap wrote: > > > >> On 9/12/18 10:27 PM, Stephen Rothwell wrote: > >>> Hi all, > >>> > >>> News: there will be no linux-next releases on Friday or Monday. > >>> > >>> Changes since 20180912: > >>> > >> > >> on i386: > >> > >> ERROR: "__udivdi3" [drivers/md/dm-thin-pool.ko] undefined! > > > > Well, as I pointed out in reply that nobody will see to the buildbots: > > > > There is something off in this report... I cannot reproduce. It is > > almost like the warning was generated when building an older version of > > this change, but then reported against the latest commit. > > > > I switched to sector_div() specifically because of the undefined > > __udivdi3 error. > > > > So I'm ignoring this given I cannot reproduce when using 'make ARCH=i386' > > > gcc --version > gcc (SUSE Linux) 4.8.5 > > # CONFIG_LBDAF is not set > > Perhaps you could try to reproduce it with the attached randconfig file. Ah, yeap.. sector_div() is only viable for use with sector_t. dm_block_t is typedef'd to uint64_t -- so that explains it. Need to use div_u64() instead. Thanks, Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] linux-next: Tree for Sep 13 (dm-thin-pool)
On Thu, Sep 13 2018 at 1:28pm -0400, Randy Dunlap wrote: > On 9/12/18 10:27 PM, Stephen Rothwell wrote: > > Hi all, > > > > News: there will be no linux-next releases on Friday or Monday. > > > > Changes since 20180912: > > > > on i386: > > ERROR: "__udivdi3" [drivers/md/dm-thin-pool.ko] undefined! Well, as I pointed out in reply that nobody will see to the buildbots: There is something off in this report... I cannot reproduce. It is almost like the warning was generated when building an older version of this change, but then reported against the latest commit. I switched to sector_div() specifically because of the undefined __udivdi3 error. So I'm ignoring this given I cannot reproduce when using 'make ARCH=i386' -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v8 5/9] dm: Remove VLA usage from hashes
On Tue, Aug 07 2018 at 5:18pm -0400,
Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this uses
> the new HASH_MAX_DIGESTSIZE from the crypto layer to allocate the upper
> bounds on stack usage.
>
> [1]
> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>
> Signed-off-by: Kees Cook
> ---
> drivers/md/dm-integrity.c | 23 +--
> drivers/md/dm-verity-fec.c | 5 -
> 2 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
> index 86438b2f10dd..884edd7cf1d0 100644
> --- a/drivers/md/dm-integrity.c
> +++ b/drivers/md/dm-integrity.c
> @@ -521,7 +521,12 @@ static void section_mac(struct dm_integrity_c *ic,
> unsigned section, __u8 result
> }
> memset(result + size, 0, JOURNAL_MAC_SIZE - size);
> } else {
> - __u8 digest[size];
> + __u8 digest[HASH_MAX_DIGESTSIZE];
> +
> + if (WARN_ON(size > sizeof(digest))) {
> + dm_integrity_io_error(ic, "digest_size", -EINVAL);
> + goto err;
> + }
> r = crypto_shash_final(desc, digest);
> if (unlikely(r)) {
> dm_integrity_io_error(ic, "crypto_shash_final", r);
> @@ -1244,7 +1249,7 @@ static void integrity_metadata(struct work_struct *w)
> struct bio *bio = dm_bio_from_per_bio_data(dio, sizeof(struct
> dm_integrity_io));
> char *checksums;
> unsigned extra_space = unlikely(digest_size > ic->tag_size) ?
> digest_size - ic->tag_size : 0;
> - char checksums_onstack[ic->tag_size + extra_space];
> + char checksums_onstack[HASH_MAX_DIGESTSIZE];
> unsigned sectors_to_process = dio->range.n_sectors;
> sector_t sector = dio->range.logical_sector;
>
> @@ -1253,8 +1258,14 @@ static void integrity_metadata(struct work_struct *w)
>
> checksums = kmalloc((PAGE_SIZE >> SECTOR_SHIFT >>
> ic->sb->log2_sectors_per_block) * ic->tag_size + extra_space,
> GFP_NOIO | __GFP_NORETRY | __GFP_NOWARN);
> - if (!checksums)
> + if (!checksums) {
> checksums = checksums_onstack;
> + if (WARN_ON(extra_space &&
> + digest_size > sizeof(checksums_onstack))) {
> + r = -EINVAL;
> + goto error;
> + }
> + }
>
> __bio_for_each_segment(bv, bio, iter, dio->orig_bi_iter) {
> unsigned pos;
Given the length of the kmalloc() just prior to this new WARN_ON() line
I'm not seeing why you've elected to split the WARN_ON across multiple
lines.
But that style nit aside:
Acked-by: Mike Snitzer
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] Dealing with constantly failing paths
On Thu, Sep 13, 2018 at 12:42:54PM +0300, Özkan Göksu wrote: >Hello. >I'm sorry to have e-mailed you here but I did not really find the answer. >When a disk starts to die slowly multipath starts to Failing & Reinstating >paths and this keeps forever.. (I'm using LSI-3008HBA card with SAS-JBOD >not FC-Network) >Because kernel do not echo to offline faulted disk. This is causing >terrible problems to me. >I'm using: multipath-tools 0.7.4-1 >Linux DEV2 4.14.67-1-lts #1 >Dmesg; > Sep 13 11:20:17 DEV2 kernel: sd 0:0:190:0: attempting task abort! >scmd(88110e632948) > Sep 13 11:20:17 DEV2 kernel: sd 0:0:190:0: [sdft] tag#3 CDB: >opcode=0x0 00 00 00 00 00 00 > Sep 13 11:20:17 DEV2 kernel: scsi target0:0:190: handle(0x0037), >sas_address(0x5000c50093d4e7c6), phy(38) > Sep 13 11:20:17 DEV2 kernel: scsi target0:0:190: >enclosure_logical_id(0x500304800929ec7f), slot(37) > Sep 13 11:20:17 DEV2 kernel: scsi target0:0:190: enclosure >level(0x0001),connector name(1 ) > Sep 13 11:20:17 DEV2 kernel: sd 0:0:190:0: task abort: SUCCESS >scmd(88110e632948) > Sep 13 11:20:18 DEV2 kernel: device-mapper: multipath: Failing path >130:240. > Sep 13 11:25:34 DEV2 kernel: device-mapper: multipath: Reinstating >path 130:240. >Full dmesg example: [1]https://paste.ubuntu.com/p/H9NMWxNfgD/ > >As you can see kernel aborted the mission and after that multipath failed. >So I want to get rid of this problem via telling Multipath "do not >Reinstate the path". >This method will keep dead the zombie disk. >If I dont kick the disk out its causing HBA reset and I'm losing all disk >in my pool and ZFS pool suspending. >I'm not saying this problem related to multipathd, I'm just thinking this >will save me. >So how can I tell the multipath do not Reinstate X times failed path? >Thank you. In recent releases there are two seperate methods to do this. Both of them involve setting multiple multipath.conf parameters. The older method is to set "delay_wait_checks" and "delay_watch_checks". The newer one is to set "marginal_path_double_failed_time", "marginal_path_err_sample_time", "marginal_path_err_rate_threshold", and "marginal_path_err_recheck_gap_time". You can look in the multipath.conf man page to see if both sets of options are available to you, and how they work. If the version of the multipath tools you are using has both sets of options, the "marginal_path_*" options should do a better job at finding these marginal paths. -Ben > > References > >Visible links >1. https://paste.ubuntu.com/p/H9NMWxNfgD/ > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] linux-next: Tree for Sep 13 (dm-thin-pool)
On 9/12/18 10:27 PM, Stephen Rothwell wrote: > Hi all, > > News: there will be no linux-next releases on Friday or Monday. > > Changes since 20180912: > on i386: ERROR: "__udivdi3" [drivers/md/dm-thin-pool.ko] undefined! -- ~Randy -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v8 5/9] dm: Remove VLA usage from hashes
On Mon, Sep 3, 2018 at 8:13 PM, Herbert Xu wrote: > On Tue, Aug 07, 2018 at 02:18:39PM -0700, Kees Cook wrote: >> In the quest to remove all stack VLA usage from the kernel[1], this uses >> the new HASH_MAX_DIGESTSIZE from the crypto layer to allocate the upper >> bounds on stack usage. >> >> [1] >> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com >> >> Signed-off-by: Kees Cook > > Can the dm folks please review this patch? Mike or Alasdair, can you Ack this patch so Herbert can include it in the crypto tree? This is blocking some VLA removals[1]... Thanks! -Kees [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com -- Kees Cook Pixel Security -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] Dealing with constantly failing paths
Hello. I'm sorry to have e-mailed you here but I did not really find the answer. When a disk starts to die slowly multipath starts to Failing & Reinstating paths and this keeps forever.. (I'm using LSI-3008HBA card with SAS-JBOD not FC-Network) Because kernel do not echo to offline faulted disk. This is causing terrible problems to me. I'm using: multipath-tools 0.7.4-1 Linux DEV2 4.14.67-1-lts #1 Dmesg; Sep 13 11:20:17 DEV2 kernel: sd 0:0:190:0: attempting task abort! scmd(88110e632948) Sep 13 11:20:17 DEV2 kernel: sd 0:0:190:0: [sdft] tag#3 CDB: opcode=0x0 00 00 00 00 00 00 Sep 13 11:20:17 DEV2 kernel: scsi target0:0:190: handle(0x0037), sas_address(0x5000c50093d4e7c6), phy(38) Sep 13 11:20:17 DEV2 kernel: scsi target0:0:190: enclosure_logical_id(0x500304800929ec7f), slot(37) Sep 13 11:20:17 DEV2 kernel: scsi target0:0:190: enclosure level(0x0001),connector name(1 ) Sep 13 11:20:17 DEV2 kernel: sd 0:0:190:0: task abort: SUCCESS scmd(88110e632948) Sep 13 11:20:18 DEV2 kernel: device-mapper: multipath: Failing path 130:240. Sep 13 11:25:34 DEV2 kernel: device-mapper: multipath: Reinstating path 130:240. Full dmesg example: https://paste.ubuntu.com/p/H9NMWxNfgD/ As you can see kernel aborted the mission and after that multipath failed. So I want to get rid of this problem via telling Multipath "do not Reinstate the path". This method will keep dead the zombie disk. If I dont kick the disk out its causing HBA reset and I'm losing all disk in my pool and ZFS pool suspending. I'm not saying this problem related to multipathd, I'm just thinking this will save me. So how can I tell the multipath do not Reinstate X times failed path? Thank you. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 2/2] crypto: lrw - Do not use auxiliary buffer
On Wed, Sep 12, 2018 at 8:51 AM Eric Biggers wrote:
> On Tue, Sep 11, 2018 at 09:42:39AM +0200, Ondrej Mosnacek wrote:
> > This patch simplifies the LRW template to recompute the LRW tweaks from
> > scratch in the second pass and thus also removes the need to allocate a
> > dynamic buffer using kmalloc().
> >
> > As discussed at [1], the use of kmalloc causes deadlocks with dm-crypt.
> >
> > PERFORMANCE MEASUREMENTS (x86_64)
> > Performed using: https://gitlab.com/omos/linux-crypto-bench
> > Crypto driver used: lrw(ecb-aes-aesni)
> >
> > The results show that the new code has about the same performance as the
> > old code. For 512-byte message it seems to be even slightly faster, but
> > that might be just noise.
> >
> > Before:
> >ALGORITHM KEY (b)DATA (B) TIME ENC (ns) TIME DEC (ns)
> > lrw(aes) 256 64 200 203
> > lrw(aes) 320 64 202 204
> > lrw(aes) 384 64 204 205
> > lrw(aes) 256 512 415 415
> > lrw(aes) 320 512 432 440
> > lrw(aes) 384 512 449 451
> > lrw(aes) 256409618381995
> > lrw(aes) 320409621231980
> > lrw(aes) 384409621002119
> > lrw(aes) 256 1638471836954
> > lrw(aes) 320 1638478447631
> > lrw(aes) 384 1638482568126
> > lrw(aes) 256 32768 14772 14484
> > lrw(aes) 320 32768 15281 15431
> > lrw(aes) 384 32768 16469 16293
> >
> > After:
> >ALGORITHM KEY (b)DATA (B) TIME ENC (ns) TIME DEC (ns)
> > lrw(aes) 256 64 197 196
> > lrw(aes) 320 64 200 197
> > lrw(aes) 384 64 203 199
> > lrw(aes) 256 512 385 380
> > lrw(aes) 320 512 401 395
> > lrw(aes) 384 512 415 415
> > lrw(aes) 256409618691846
> > lrw(aes) 320409620801981
> > lrw(aes) 384409621602109
> > lrw(aes) 256 1638470777127
> > lrw(aes) 320 1638478077766
> > lrw(aes) 384 1638481088357
> > lrw(aes) 256 32768 14111 14454
> > lrw(aes) 320 32768 15268 15082
> > lrw(aes) 384 32768 16581 16250
> >
> > [1] https://lkml.org/lkml/2018/8/23/1315
> >
> > Signed-off-by: Ondrej Mosnacek
> > ---
> > crypto/lrw.c | 280 ++-
> > 1 file changed, 51 insertions(+), 229 deletions(-)
> >
> > diff --git a/crypto/lrw.c b/crypto/lrw.c
> > index b4f30b6f16d6..d5d2fba9af59 100644
> > --- a/crypto/lrw.c
> > +++ b/crypto/lrw.c
> > @@ -29,8 +29,6 @@
> > #include
> > #include
> >
> > -#define LRW_BUFFER_SIZE 128u
> > -
> > #define LRW_BLOCK_SIZE 16
> >
> > struct priv {
> > @@ -56,19 +54,7 @@ struct priv {
> > };
> >
> > struct rctx {
> > - be128 buf[LRW_BUFFER_SIZE / sizeof(be128)];
> > -
> > - be128 t;
> > -
> > - be128 *ext;
> > -
> > - struct scatterlist srcbuf[2];
> > - struct scatterlist dstbuf[2];
> > - struct scatterlist *src;
> > - struct scatterlist *dst;
> > -
> > - unsigned int left;
> > -
> > + be128 t, orig_iv;
> > struct skcipher_request subreq;
> > };
> >
> > @@ -135,86 +121,31 @@ static int next_index(u32 *counter)
> > return res;
> > }
> >
> > -static int post_crypt(struct skcipher_request *req)
> > +/*
> > + * We compute the tweak masks twice (both before and after the ECB
> > encryption or
> > + * decryption) to avoid having to allocate a temporary buffer and/or make
> > + * mutliple calls to the 'ecb(..)' instance, which usually would be slower
> > than
> > + * just doing the gf128mul_x_ble() calls again.
> > + */
>
> next_index(), not gf128mul_x_ble().
Fixed for next revision, thanks.
>
> > +static void init_crypt(struct skcipher_request *req)
> > {
> > + struct priv *ctx = crypto_skcipher_ctx(crypto_skcipher_reqtfm(req));
> > struct rctx *rctx = skcipher_request_ctx(req);
> > - struct skcipher_request *subreq;
> >
Re: [dm-devel] [PATCH v3 1/2] crypto: lrw - Optimize tweak computation
Hi Ondrej,
On Tue, Sep 11, 2018 at 09:42:38AM +0200, Ondrej Mosnacek wrote:
> This patch rewrites the tweak computation to a slightly simpler method
> that performs less bswaps. Based on performance measurements the new
> code seems to provide slightly better performance than the old one.
>
> PERFORMANCE MEASUREMENTS (x86_64)
> Performed using: https://gitlab.com/omos/linux-crypto-bench
> Crypto driver used: lrw(ecb-aes-aesni)
>
> Before:
>ALGORITHM KEY (b)DATA (B) TIME ENC (ns) TIME DEC (ns)
> lrw(aes) 256 64 204 286
> lrw(aes) 320 64 227 203
> lrw(aes) 384 64 208 204
> lrw(aes) 256 512 441 439
> lrw(aes) 320 512 456 455
> lrw(aes) 384 512 469 483
> lrw(aes) 256409621362190
> lrw(aes) 320409621612213
> lrw(aes) 384409622952369
> lrw(aes) 256 1638476927868
> lrw(aes) 320 1638482308691
> lrw(aes) 384 1638489718813
> lrw(aes) 256 32768 15336 15560
> lrw(aes) 320 32768 16410 16346
> lrw(aes) 384 32768 18023 17465
>
> After:
>ALGORITHM KEY (b)DATA (B) TIME ENC (ns) TIME DEC (ns)
> lrw(aes) 256 64 200 203
> lrw(aes) 320 64 202 204
> lrw(aes) 384 64 204 205
> lrw(aes) 256 512 415 415
> lrw(aes) 320 512 432 440
> lrw(aes) 384 512 449 451
> lrw(aes) 256409618381995
> lrw(aes) 320409621231980
> lrw(aes) 384409621002119
> lrw(aes) 256 1638471836954
> lrw(aes) 320 1638478447631
> lrw(aes) 384 1638482568126
> lrw(aes) 256 32768 14772 14484
> lrw(aes) 320 32768 15281 15431
> lrw(aes) 384 32768 16469 16293
>
> Signed-off-by: Ondrej Mosnacek
> ---
> crypto/lrw.c | 49 +
> 1 file changed, 25 insertions(+), 24 deletions(-)
>
> diff --git a/crypto/lrw.c b/crypto/lrw.c
> index 393a782679c7..b4f30b6f16d6 100644
> --- a/crypto/lrw.c
> +++ b/crypto/lrw.c
> @@ -120,30 +120,19 @@ static int setkey(struct crypto_skcipher *parent, const
> u8 *key,
> return 0;
> }
>
> -static inline void inc(be128 *iv)
> +static int next_index(u32 *counter)
> {
> - be64_add_cpu(>b, 1);
> - if (!iv->b)
> - be64_add_cpu(>a, 1);
> -}
> -
> -/* this returns the number of consequative 1 bits starting
> - * from the right, get_index128(00 00 00 00 00 00 ... 00 00 10 FB) = 2 */
> -static inline int get_index128(be128 *block)
> -{
> - int x;
> - __be32 *p = (__be32 *) block;
> -
> - for (p += 3, x = 0; x < 128; p--, x += 32) {
> - u32 val = be32_to_cpup(p);
> -
> - if (!~val)
> - continue;
> + int i, res = 0;
>
> - return x + ffz(val);
> + for (i = 0; i < 4; i++) {
> + if (counter[i] + 1 != 0) {
> + res += ffz(counter[i]++);
> + break;
> + }
> + counter[i] = 0;
> + res += 32;
> }
> -
> - return x;
> + return res;
> }
This looks good, but can you leave the comment that says it returns the number
of leading 1's in the counter? And now that it increments the counter too.
Actually, I think it's wrong in the case where the counter is all 1's and wraps
around. It will XOR with ->mulinc[128], which is off the end of the array,
instead of the correct value of ->mulinc[127]... But that's an existing bug :-/
(If you do want to fix that, please do it in a separate patch, probably
preceding this one in the series, and add a test vector that covers it...)
>
> static int post_crypt(struct skcipher_request *req)
> @@ -209,8 +198,9 @@ static int pre_crypt(struct skcipher_request *req)
> struct scatterlist *sg;
> unsigned cryptlen;
> unsigned offset;
> - be128 *iv;
>
Re: [dm-devel] [PATCH v3 1/2] crypto: lrw - Optimize tweak computation
On Wed, Sep 12, 2018 at 8:28 AM Eric Biggers wrote:
> Hi Ondrej,
>
> On Tue, Sep 11, 2018 at 09:42:38AM +0200, Ondrej Mosnacek wrote:
> > This patch rewrites the tweak computation to a slightly simpler method
> > that performs less bswaps. Based on performance measurements the new
> > code seems to provide slightly better performance than the old one.
> >
> > PERFORMANCE MEASUREMENTS (x86_64)
> > Performed using: https://gitlab.com/omos/linux-crypto-bench
> > Crypto driver used: lrw(ecb-aes-aesni)
> >
> > Before:
> >ALGORITHM KEY (b)DATA (B) TIME ENC (ns) TIME DEC (ns)
> > lrw(aes) 256 64 204 286
> > lrw(aes) 320 64 227 203
> > lrw(aes) 384 64 208 204
> > lrw(aes) 256 512 441 439
> > lrw(aes) 320 512 456 455
> > lrw(aes) 384 512 469 483
> > lrw(aes) 256409621362190
> > lrw(aes) 320409621612213
> > lrw(aes) 384409622952369
> > lrw(aes) 256 1638476927868
> > lrw(aes) 320 1638482308691
> > lrw(aes) 384 1638489718813
> > lrw(aes) 256 32768 15336 15560
> > lrw(aes) 320 32768 16410 16346
> > lrw(aes) 384 32768 18023 17465
> >
> > After:
> >ALGORITHM KEY (b)DATA (B) TIME ENC (ns) TIME DEC (ns)
> > lrw(aes) 256 64 200 203
> > lrw(aes) 320 64 202 204
> > lrw(aes) 384 64 204 205
> > lrw(aes) 256 512 415 415
> > lrw(aes) 320 512 432 440
> > lrw(aes) 384 512 449 451
> > lrw(aes) 256409618381995
> > lrw(aes) 320409621231980
> > lrw(aes) 384409621002119
> > lrw(aes) 256 1638471836954
> > lrw(aes) 320 1638478447631
> > lrw(aes) 384 1638482568126
> > lrw(aes) 256 32768 14772 14484
> > lrw(aes) 320 32768 15281 15431
> > lrw(aes) 384 32768 16469 16293
> >
> > Signed-off-by: Ondrej Mosnacek
> > ---
> > crypto/lrw.c | 49 +
> > 1 file changed, 25 insertions(+), 24 deletions(-)
> >
> > diff --git a/crypto/lrw.c b/crypto/lrw.c
> > index 393a782679c7..b4f30b6f16d6 100644
> > --- a/crypto/lrw.c
> > +++ b/crypto/lrw.c
> > @@ -120,30 +120,19 @@ static int setkey(struct crypto_skcipher *parent,
> > const u8 *key,
> > return 0;
> > }
> >
> > -static inline void inc(be128 *iv)
> > +static int next_index(u32 *counter)
> > {
> > - be64_add_cpu(>b, 1);
> > - if (!iv->b)
> > - be64_add_cpu(>a, 1);
> > -}
> > -
> > -/* this returns the number of consequative 1 bits starting
> > - * from the right, get_index128(00 00 00 00 00 00 ... 00 00 10 FB) = 2 */
> > -static inline int get_index128(be128 *block)
> > -{
> > - int x;
> > - __be32 *p = (__be32 *) block;
> > -
> > - for (p += 3, x = 0; x < 128; p--, x += 32) {
> > - u32 val = be32_to_cpup(p);
> > -
> > - if (!~val)
> > - continue;
> > + int i, res = 0;
> >
> > - return x + ffz(val);
> > + for (i = 0; i < 4; i++) {
> > + if (counter[i] + 1 != 0) {
> > + res += ffz(counter[i]++);
> > + break;
> > + }
> > + counter[i] = 0;
> > + res += 32;
> > }
> > -
> > - return x;
> > + return res;
> > }
>
> This looks good, but can you leave the comment that says it returns the number
> of leading 1's in the counter? And now that it increments the counter too.
Good idea, will do.
>
> Actually, I think it's wrong in the case where the counter is all 1's and
> wraps
> around. It will XOR with ->mulinc[128], which is off the end of the array,
> instead of the correct value of ->mulinc[127]... But that's an existing bug
> :-/
Oh, right, good catch!
> (If you do want to fix that, please do it in a separate patch, probably
> preceding
Re: [dm-devel] [PATCH v3 2/2] crypto: lrw - Do not use auxiliary buffer
On Tue, Sep 11, 2018 at 09:42:39AM +0200, Ondrej Mosnacek wrote:
> This patch simplifies the LRW template to recompute the LRW tweaks from
> scratch in the second pass and thus also removes the need to allocate a
> dynamic buffer using kmalloc().
>
> As discussed at [1], the use of kmalloc causes deadlocks with dm-crypt.
>
> PERFORMANCE MEASUREMENTS (x86_64)
> Performed using: https://gitlab.com/omos/linux-crypto-bench
> Crypto driver used: lrw(ecb-aes-aesni)
>
> The results show that the new code has about the same performance as the
> old code. For 512-byte message it seems to be even slightly faster, but
> that might be just noise.
>
> Before:
>ALGORITHM KEY (b)DATA (B) TIME ENC (ns) TIME DEC (ns)
> lrw(aes) 256 64 200 203
> lrw(aes) 320 64 202 204
> lrw(aes) 384 64 204 205
> lrw(aes) 256 512 415 415
> lrw(aes) 320 512 432 440
> lrw(aes) 384 512 449 451
> lrw(aes) 256409618381995
> lrw(aes) 320409621231980
> lrw(aes) 384409621002119
> lrw(aes) 256 1638471836954
> lrw(aes) 320 1638478447631
> lrw(aes) 384 1638482568126
> lrw(aes) 256 32768 14772 14484
> lrw(aes) 320 32768 15281 15431
> lrw(aes) 384 32768 16469 16293
>
> After:
>ALGORITHM KEY (b)DATA (B) TIME ENC (ns) TIME DEC (ns)
> lrw(aes) 256 64 197 196
> lrw(aes) 320 64 200 197
> lrw(aes) 384 64 203 199
> lrw(aes) 256 512 385 380
> lrw(aes) 320 512 401 395
> lrw(aes) 384 512 415 415
> lrw(aes) 256409618691846
> lrw(aes) 320409620801981
> lrw(aes) 384409621602109
> lrw(aes) 256 1638470777127
> lrw(aes) 320 1638478077766
> lrw(aes) 384 1638481088357
> lrw(aes) 256 32768 14111 14454
> lrw(aes) 320 32768 15268 15082
> lrw(aes) 384 32768 16581 16250
>
> [1] https://lkml.org/lkml/2018/8/23/1315
>
> Signed-off-by: Ondrej Mosnacek
> ---
> crypto/lrw.c | 280 ++-
> 1 file changed, 51 insertions(+), 229 deletions(-)
>
> diff --git a/crypto/lrw.c b/crypto/lrw.c
> index b4f30b6f16d6..d5d2fba9af59 100644
> --- a/crypto/lrw.c
> +++ b/crypto/lrw.c
> @@ -29,8 +29,6 @@
> #include
> #include
>
> -#define LRW_BUFFER_SIZE 128u
> -
> #define LRW_BLOCK_SIZE 16
>
> struct priv {
> @@ -56,19 +54,7 @@ struct priv {
> };
>
> struct rctx {
> - be128 buf[LRW_BUFFER_SIZE / sizeof(be128)];
> -
> - be128 t;
> -
> - be128 *ext;
> -
> - struct scatterlist srcbuf[2];
> - struct scatterlist dstbuf[2];
> - struct scatterlist *src;
> - struct scatterlist *dst;
> -
> - unsigned int left;
> -
> + be128 t, orig_iv;
> struct skcipher_request subreq;
> };
>
> @@ -135,86 +121,31 @@ static int next_index(u32 *counter)
> return res;
> }
>
> -static int post_crypt(struct skcipher_request *req)
> +/*
> + * We compute the tweak masks twice (both before and after the ECB
> encryption or
> + * decryption) to avoid having to allocate a temporary buffer and/or make
> + * mutliple calls to the 'ecb(..)' instance, which usually would be slower
> than
> + * just doing the gf128mul_x_ble() calls again.
> + */
next_index(), not gf128mul_x_ble().
> +static void init_crypt(struct skcipher_request *req)
> {
> + struct priv *ctx = crypto_skcipher_ctx(crypto_skcipher_reqtfm(req));
> struct rctx *rctx = skcipher_request_ctx(req);
> - struct skcipher_request *subreq;
> + struct skcipher_request *subreq = >subreq;
>
> - subreq = >subreq;
> -
> - while (!err && rctx->left) {
> - err = pre_crypt(req) ?:
> - crypto_skcipher_decrypt(subreq) ?:
> - post_crypt(req);
> + skcipher_request_set_tfm(subreq,
