From: Lars Ellenberg <lars.ellenberg@linbit.com>
To: Kees Cook <keescook@chromium.org>
Cc: Philipp Reisner <philipp.reisner@linbit.com>,
Jens Axboe <axboe@kernel.dk>,
Herbert Xu <herbert@gondor.apana.org.au>,
Arnd Bergmann <arnd@arndb.de>, Eric Biggers <ebiggers@google.com>,
"Gustavo A. R. Silva" <gustavo@embeddedor.com>,
linux-crypto <linux-crypto@vger.kernel.org>,
drbd-dev@lists.linbit.com,
linux-block <linux-block@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 12/18] drbd: Convert from ahash to shash
Date: Fri, 3 Aug 2018 15:44:09 +0200 [thread overview]
Message-ID: <20180803134357.GB28982@soda.linbit> (raw)
In-Reply-To: <CAGXu5jJB6V0r8LXOGL6+sO0NVZ0HK3VCthML=T3gLEwMABhSDQ@mail.gmail.com>
On Thu, Aug 02, 2018 at 04:43:19PM -0700, Kees Cook wrote:
> On Tue, Jul 24, 2018 at 9:49 AM, 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.
>
> Philipp or Lars, what do you think of this? Can this go via your tree
> or maybe via Jens?
>
> I'd love an Ack or Review.
I'm sorry, summer time and all, limited online time ;-)
I'm happy for this to go in via Jens, or any other way.
Being not that fluent in the crypto stuff,
I will just "believe" most of it.
I still think I found something, comments below:
> > diff --git a/drivers/block/drbd/drbd_worker.c b/drivers/block/drbd/drbd_worker.c
> > index 1476cb3439f4..62dd3700dd84 100644
> > --- a/drivers/block/drbd/drbd_worker.c
> > +++ b/drivers/block/drbd/drbd_worker.c
> > @@ -295,60 +295,54 @@ void drbd_request_endio(struct bio *bio)
> > complete_master_bio(device, &m);
> > }
> >
> > -void drbd_csum_ee(struct crypto_ahash *tfm, struct drbd_peer_request *peer_req, void *digest)
> > +void drbd_csum_ee(struct crypto_shash *tfm, struct drbd_peer_request *peer_req, void *digest)
> > {
> > - AHASH_REQUEST_ON_STACK(req, tfm);
> > - struct scatterlist sg;
> > + SHASH_DESC_ON_STACK(desc, tfm);
> > struct page *page = peer_req->pages;
> > struct page *tmp;
> > unsigned len;
> >
> > - ahash_request_set_tfm(req, tfm);
> > - ahash_request_set_callback(req, 0, NULL, NULL);
> > + desc->tfm = tfm;
> > + desc->flags = 0;
> >
> > - sg_init_table(&sg, 1);
> > - crypto_ahash_init(req);
> > + crypto_shash_init(desc);
> >
> > while ((tmp = page_chain_next(page))) {
> > /* all but the last page will be fully used */
> > - sg_set_page(&sg, page, PAGE_SIZE, 0);
> > - ahash_request_set_crypt(req, &sg, NULL, sg.length);
> > - crypto_ahash_update(req);
> > + crypto_shash_update(desc, page_to_virt(page), PAGE_SIZE);
These pages may be "highmem", so page_to_virt() seem not appropriate.
I think the crypto_ahash_update() thingy did an implicit kmap() for us
in the crypto_hash_walk()?
Maybe it is good enough to add a kmap() here,
and a kunmap() on the next line?
> > page = tmp;
> > }
> > /* and now the last, possibly only partially used page */
> > len = peer_req->i.size & (PAGE_SIZE - 1);
> > - sg_set_page(&sg, page, len ?: PAGE_SIZE, 0);
> > - ahash_request_set_crypt(req, &sg, digest, sg.length);
> > - crypto_ahash_finup(req);
> > - ahash_request_zero(req);
> > + crypto_shash_update(desc, page_to_virt(page), len ?: PAGE_SIZE);
Same here ...
> > +
> > + crypto_shash_final(desc, digest);
> > + shash_desc_zero(desc);
> > }
> >
> > -void drbd_csum_bio(struct crypto_ahash *tfm, struct bio *bio, void *digest)
> > +void drbd_csum_bio(struct crypto_shash *tfm, struct bio *bio, void *digest)
> > {
> > - AHASH_REQUEST_ON_STACK(req, tfm);
> > - struct scatterlist sg;
> > + SHASH_DESC_ON_STACK(desc, tfm);
> > struct bio_vec bvec;
> > struct bvec_iter iter;
> >
> > - ahash_request_set_tfm(req, tfm);
> > - ahash_request_set_callback(req, 0, NULL, NULL);
> > + desc->tfm = tfm;
> > + desc->flags = 0;
> >
> > - sg_init_table(&sg, 1);
> > - crypto_ahash_init(req);
> > + crypto_shash_init(desc);
> >
> > bio_for_each_segment(bvec, bio, iter) {
> > - sg_set_page(&sg, bvec.bv_page, bvec.bv_len, bvec.bv_offset);
> > - ahash_request_set_crypt(req, &sg, NULL, sg.length);
> > - crypto_ahash_update(req);
> > + crypto_shash_update(desc, (u8 *)page_to_virt(bvec.bv_page) +
> > + bvec.bv_offset,
> > + bvec.bv_len);
... and here.
> > +
> > /* REQ_OP_WRITE_SAME has only one segment,
> > * checksum the payload only once. */
> > if (bio_op(bio) == REQ_OP_WRITE_SAME)
> > break;
> > }
> > - ahash_request_set_crypt(req, NULL, digest, 0);
> > - crypto_ahash_final(req);
> > - ahash_request_zero(req);
> > + crypto_shash_final(desc, digest);
> > + shash_desc_zero(desc);
> > }
> >
> > /* MAYBE merge common code with w_e_end_ov_req */
Thanks,
Lars
next prev parent reply other threads:[~2018-08-03 13:44 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-24 16:49 [PATCH v6 00/18] crypto: Remove VLA usage Kees Cook
2018-07-24 16:49 ` [PATCH v6 01/18] crypto: xcbc: " Kees Cook
2018-07-24 16:49 ` [PATCH v6 02/18] crypto: cbc: " Kees Cook
2018-07-24 16:49 ` [PATCH v6 03/18] crypto: hash: " Kees Cook
2018-07-24 16:49 ` [PATCH v6 04/18] dm: Remove VLA usage from hashes Kees Cook
2018-07-24 16:49 ` [PATCH v6 05/18] crypto alg: Introduce generic max blocksize and alignmask Kees Cook
2018-07-24 16:49 ` [PATCH v6 06/18] crypto: qat: Remove VLA usage Kees Cook
2018-07-24 16:49 ` [PATCH v6 07/18] crypto: shash: Remove VLA usage in unaligned hashing Kees Cook
2018-07-24 16:49 ` [PATCH v6 08/18] crypto: skcipher: Remove VLA usage for SKCIPHER_REQUEST_ON_STACK Kees Cook
2018-07-24 16:49 ` [PATCH v6 09/18] ppp: mppe: Remove VLA usage Kees Cook
2018-07-24 16:49 ` [PATCH v6 10/18] x86/power/64: " Kees Cook
2018-07-25 11:32 ` Rafael J. Wysocki
2018-07-25 18:01 ` Kees Cook
2018-08-06 10:28 ` Rafael J. Wysocki
2018-07-24 16:49 ` [PATCH v6 11/18] dm crypt: Convert essiv from ahash to shash Kees Cook
2018-07-24 16:49 ` [PATCH v6 12/18] drbd: Convert " Kees Cook
2018-08-02 23:43 ` Kees Cook
2018-08-03 13:44 ` Lars Ellenberg [this message]
2018-08-06 17:27 ` Kees Cook
2018-07-24 16:49 ` [PATCH v6 13/18] wireless/lib80211: " Kees Cook
2018-07-25 7:24 ` Johannes Berg
2018-07-24 16:49 ` [PATCH v6 14/18] staging: rtl8192u: ieee80211: " Kees Cook
2018-07-24 16:49 ` [PATCH v6 15/18] staging: rtl8192e: " Kees Cook
2018-07-24 16:49 ` [PATCH v6 16/18] rxrpc: Reuse SKCIPHER_REQUEST_ON_STACK buffer Kees Cook
2018-08-02 23:46 ` Kees Cook
2018-08-03 9:14 ` David Howells
2018-07-24 16:49 ` [PATCH v6 17/18] crypto: ccm: Remove VLA usage Kees Cook
2018-07-24 16:57 ` Ard Biesheuvel
2018-07-24 17:51 ` Kees Cook
2018-07-24 16:49 ` [PATCH v6 18/18] crypto: Remove AHASH_REQUEST_ON_STACK Kees Cook
2018-07-24 17:31 ` Joe Perches
2018-07-24 17:53 ` Kees Cook
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=20180803134357.GB28982@soda.linbit \
--to=lars.ellenberg@linbit.com \
--cc=arnd@arndb.de \
--cc=axboe@kernel.dk \
--cc=drbd-dev@lists.linbit.com \
--cc=ebiggers@google.com \
--cc=gustavo@embeddedor.com \
--cc=herbert@gondor.apana.org.au \
--cc=keescook@chromium.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-crypto@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).