All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lars Ellenberg <lars.ellenberg@linbit.com>
To: Kees Cook <keescook@chromium.org>
Cc: Jens Axboe <axboe@kernel.dk>,
	Philipp Reisner <philipp.reisner@linbit.com>,
	linux-block <linux-block@vger.kernel.org>,
	drbd-dev@lists.linbit.com, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v7] drbd: Convert from ahash to shash
Date: Wed, 5 Sep 2018 10:33:20 +0200	[thread overview]
Message-ID: <20180905083320.GA28462@soda.linbit> (raw)
In-Reply-To: <CAGXu5jLyynGbnuhwabbViwzvRFzn-Vj5ktYOrypRGj_qk8UvQw@mail.gmail.com>

On Tue, Sep 04, 2018 at 08:04:18PM -0700, Kees Cook wrote:
> On Mon, Sep 3, 2018 at 11:04 PM, Kees Cook <keescook@chromium.org> wrote:
> > On Mon, Aug 6, 2018 at 4:32 PM, Kees Cook <keescook@chromium.org> wrote:
> >> In preparing to remove all stack VLA usage from the kernel[1], this
> >> removes the discouraged use of AHASH_REQUEST_ON_STACK in favor of
> >> the smaller SHASH_DESC_ON_STACK by converting from ahash-wrapped-shash
> >> to direct shash. By removing a layer of indirection this both improves
> >> performance and reduces stack usage. The stack allocation will be made
> >> a fixed size in a later patch to the crypto subsystem.
> >>
> >> The bulk of the lines in this change are simple s/ahash/shash/, but the
> >> main logic differences are in drbd_csum_ee() and drbd_csum_bio(), which
> >> externalizes the page walking with k(un)map_atomic() instead of using
> >> scattergather.
> >
> > Hi Lars! How does this look to you? If you can Ack I assume Jens would
> > be able to take this.

Sure, I should have ACKed it a month ago already.  As I said, I believe
you the crypto. And you added the kmap_atomic as I pointed out.
All good.

> FWIW I've tested a simple drbd configuration before/after this change
> and things seem to be working correctly.

You'd need "data-integrity-alg" set (or "verify-alg", and then have it
do an online-verify) to excercise the crypto stuff,
and you'd need a highmem system (are these still out there?)
to have the kmap not be a no-op.  But I don't see any potential problem.

Thanks!

    Lars

WARNING: multiple messages have this Message-ID (diff)
From: Lars Ellenberg <lars.ellenberg@linbit.com>
To: Kees Cook <keescook@chromium.org>
Cc: Jens Axboe <axboe@kernel.dk>,
	linux-block <linux-block@vger.kernel.org>,
	Philipp Reisner <philipp.reisner@linbit.com>,
	LKML <linux-kernel@vger.kernel.org>,
	drbd-dev@lists.linbit.com
Subject: Re: [Drbd-dev] [PATCH v7] drbd: Convert from ahash to shash
Date: Wed, 5 Sep 2018 10:33:20 +0200	[thread overview]
Message-ID: <20180905083320.GA28462@soda.linbit> (raw)
In-Reply-To: <CAGXu5jLyynGbnuhwabbViwzvRFzn-Vj5ktYOrypRGj_qk8UvQw@mail.gmail.com>

On Tue, Sep 04, 2018 at 08:04:18PM -0700, Kees Cook wrote:
> On Mon, Sep 3, 2018 at 11:04 PM, Kees Cook <keescook@chromium.org> wrote:
> > On Mon, Aug 6, 2018 at 4:32 PM, Kees Cook <keescook@chromium.org> wrote:
> >> In preparing to remove all stack VLA usage from the kernel[1], this
> >> removes the discouraged use of AHASH_REQUEST_ON_STACK in favor of
> >> the smaller SHASH_DESC_ON_STACK by converting from ahash-wrapped-shash
> >> to direct shash. By removing a layer of indirection this both improves
> >> performance and reduces stack usage. The stack allocation will be made
> >> a fixed size in a later patch to the crypto subsystem.
> >>
> >> The bulk of the lines in this change are simple s/ahash/shash/, but the
> >> main logic differences are in drbd_csum_ee() and drbd_csum_bio(), which
> >> externalizes the page walking with k(un)map_atomic() instead of using
> >> scattergather.
> >
> > Hi Lars! How does this look to you? If you can Ack I assume Jens would
> > be able to take this.

Sure, I should have ACKed it a month ago already.  As I said, I believe
you the crypto. And you added the kmap_atomic as I pointed out.
All good.

> FWIW I've tested a simple drbd configuration before/after this change
> and things seem to be working correctly.

You'd need "data-integrity-alg" set (or "verify-alg", and then have it
do an online-verify) to excercise the crypto stuff,
and you'd need a highmem system (are these still out there?)
to have the kmap not be a no-op.  But I don't see any potential problem.

Thanks!

    Lars

  reply	other threads:[~2018-09-05  8:33 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-06 23:32 [PATCH v7] drbd: Convert from ahash to shash Kees Cook
2018-08-06 23:32 ` [Drbd-dev] " Kees Cook
2018-09-04  6:04 ` Kees Cook
2018-09-04  6:04   ` [Drbd-dev] " Kees Cook
2018-09-05  3:04   ` Kees Cook
2018-09-05  3:04     ` [Drbd-dev] " Kees Cook
2018-09-05  8:33     ` Lars Ellenberg [this message]
2018-09-05  8:33       ` Lars Ellenberg
2018-09-05 16:04       ` Kees Cook
2018-09-05 16:04         ` [Drbd-dev] " Kees Cook
2018-09-05 16:10         ` Jens Axboe
2018-09-05 16:10           ` [Drbd-dev] " Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180905083320.GA28462@soda.linbit \
    --to=lars.ellenberg@linbit.com \
    --cc=axboe@kernel.dk \
    --cc=drbd-dev@lists.linbit.com \
    --cc=keescook@chromium.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=philipp.reisner@linbit.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.