crypto: lib/chacha20poly1305 - Set SG_MITER_ATOMIC unconditionally
diff mbox series

Message ID 20200915033024.GB25789@gondor.apana.org.au
State Accepted
Commit 255f6c2e74b136662a191652d688bd7566c94bf6
Headers show
Series
  • crypto: lib/chacha20poly1305 - Set SG_MITER_ATOMIC unconditionally
Related show

Commit Message

Herbert Xu Sept. 15, 2020, 3:30 a.m. UTC
I trimmed the cc as the mailing lists appear to be blocking this
email because of it.

On Mon, Sep 14, 2020 at 03:37:49PM -0700, Linus Torvalds wrote:
>
> So it _looks_ like this code started using kmap() - probably back when
> kmap_atomic() was so cumbersome to use - and was then converted
> (conditionally) to kmap_atomic() rather than just changed whole-sale.
> Is there actually something that wants to use those sg_miter functions
> and sleep?

I dug up the old zinc patch submissions and this wasn't present at
all in the original.  The original zinc code used blkcipher_walk
which unconditinoally does kmap_atomic.

So it's only the SG miter conversion that introduced this change,
which appears to be a simple oversight (I think Ard was working on
latency issues at that time, perhaps he was worried about keeping
preemption off unnecessarily).

---8<---
There is no reason for the chacha20poly1305 SG miter code to use
kmap instead of kmap_atomic as the critical section doesn't sleep
anyway.  So we can simply get rid of the preemptible check and
set SG_MITER_ATOMIC unconditionally.

Even if we need to reenable preemption to lower latency we should
be doing that by interrupting the SG miter walk rather than using
kmap.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Comments

Ard Biesheuvel Sept. 15, 2020, 6:03 a.m. UTC | #1
(+ Jason)

On Tue, 15 Sep 2020 at 06:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> I trimmed the cc as the mailing lists appear to be blocking this
> email because of it.
>
> On Mon, Sep 14, 2020 at 03:37:49PM -0700, Linus Torvalds wrote:
> >
> > So it _looks_ like this code started using kmap() - probably back when
> > kmap_atomic() was so cumbersome to use - and was then converted
> > (conditionally) to kmap_atomic() rather than just changed whole-sale.
> > Is there actually something that wants to use those sg_miter functions
> > and sleep?
>
> I dug up the old zinc patch submissions and this wasn't present at
> all in the original.  The original zinc code used blkcipher_walk
> which unconditinoally does kmap_atomic.
>

Remember that the Zinc patchset was very vocal about not relying on
the Linux crypto API, yet it [ab]used the crypto blkcipher_walk API
(which was already deprecated at that point) in a rather horrid way,
by going around the blkcipher API itself, and creating some mock
objects that the blkcipher scatterlist walker would expect to exist.

So instead, I opted to rewrite this code using the SG miter API so that:
- src == dst, and so we only need to traverse (and kmap) a single
scatterlist instead of two in parallel (as Wireguard has no need for
the latter)
- no elaborate handling of the scatterlist elements when they are not
a multiple of the cipher chunk size (which is not needed for a stream
cipher liker ChaCha)
- no need to use scatterwalk_map_and_copy() (and do another kmap()) to
access the tag if it was covered by the last scatterlist element.

> So it's only the SG miter conversion that introduced this change,
> which appears to be a simple oversight (I think Ard was working on
> latency issues at that time, perhaps he was worried about keeping
> preemption off unnecessarily).
>

No, the problem with using kmap_atomic() is that it disables
preemption even on !HIGHMEM architectures. So using it unconditionally
here means that all chacha/poly processing will execute with
preemption disabled on 64-bit architectures as well.

This means that, even if you avoid the SIMD accelerated ciphers for
latency reasons (as they disable preemption as well), you are still
running the bulk of the WireGuard processing with preemption disabled.

> ---8<---
> There is no reason for the chacha20poly1305 SG miter code to use
> kmap instead of kmap_atomic as the critical section doesn't sleep
> anyway.  So we can simply get rid of the preemptible check and
> set SG_MITER_ATOMIC unconditionally.
>
> Even if we need to reenable preemption to lower latency we should
> be doing that by interrupting the SG miter walk rather than using
> kmap.
>

AIUI, the common case is that the entire packet is covered by a single
scatterlist element, so there is no room for latency reduction here.

The problem is really that kmap_atomic() is not simply a no-op on
!HIGHMEM architectures. If we can fix that, I have no objections to
this patch.

> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/lib/crypto/chacha20poly1305.c b/lib/crypto/chacha20poly1305.c
> index 431e04280332..5850f3b87359 100644
> --- a/lib/crypto/chacha20poly1305.c
> +++ b/lib/crypto/chacha20poly1305.c
> @@ -251,9 +251,7 @@ bool chacha20poly1305_crypt_sg_inplace(struct scatterlist *src,
>                         poly1305_update(&poly1305_state, pad0, 0x10 - (ad_len & 0xf));
>         }
>
> -       flags = SG_MITER_TO_SG;
> -       if (!preemptible())
> -               flags |= SG_MITER_ATOMIC;
> +       flags = SG_MITER_TO_SG | SG_MITER_ATOMIC;
>
>         sg_miter_start(&miter, src, sg_nents(src), flags);
>
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu Sept. 15, 2020, 6:40 a.m. UTC | #2
On Tue, Sep 15, 2020 at 09:03:46AM +0300, Ard Biesheuvel wrote:
>
> The problem is really that kmap_atomic() is not simply a no-op on
> !HIGHMEM architectures. If we can fix that, I have no objections to
> this patch.

Yes we should definitely fix that.  However, doing so will involve
manually checking every instance of kmap_atomic.

Cheers,
Linus Torvalds Sept. 15, 2020, 6:45 a.m. UTC | #3
On Mon, Sep 14, 2020 at 8:30 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> There is no reason for the chacha20poly1305 SG miter code to use
> kmap instead of kmap_atomic as the critical section doesn't sleep
> anyway.  So we can simply get rid of the preemptible check and
> set SG_MITER_ATOMIC unconditionally.

So I'd prefer to make SG_MITER_ATOMIC go away entirely, and just
remove the non-atomic case..

A quick grep seems to imply that just about all users set the ATOMIC
bit anyway. I didn't look at everything, but every case I _did_ look
at did seem to set the ATOMIC bit.

So it really did seem like there isn't a lot of reason to have the
non-atomic case, and this flag could go away - not by virtue of the
atomic case going away, but by virtue of the atomic case being the
only actual case.

I mean, I did find one case that didn't set it (cb710-mmc.c), but
pattern-matching to the other mmc cases, that one looks like it
_should_ have set the atomic flag like everybody else did.

Did I miss something?

             Linus
Linus Torvalds Sept. 15, 2020, 6:55 a.m. UTC | #4
On Mon, Sep 14, 2020 at 11:45 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I mean, I did find one case that didn't set it (cb710-mmc.c), but
> pattern-matching to the other mmc cases, that one looks like it
> _should_ have set the atomic flag like everybody else did.

Oh, and immediately after sending that out I notice
nvmet_bdev_execute_rw(), which does seem to make allocations inside
that sg_miter loop.

So those non-atomic cases do clearly exist.

It does make the case for why kmap_atomic() wants to have the
debugging code. It will "just work" on 64-bit to do it wrong, because
the address doesn't become invalid just because you sleep or get
rescheduled. But then the code that every developer tests (since
developers tend to run on good hardware) might be completely broken on
bad old hardware.

Maybe we could hide it behind a debug option, at least.

Or, alterantively, introduce a new "debug_preempt_count" that doesn't
actually disable preemption, but warns about actual sleeping
operations..

             Linus
Herbert Xu Sept. 15, 2020, 7:05 a.m. UTC | #5
On Mon, Sep 14, 2020 at 11:55:53PM -0700, Linus Torvalds wrote:
>
> Maybe we could hide it behind a debug option, at least.
> 
> Or, alterantively, introduce a new "debug_preempt_count" that doesn't
> actually disable preemption, but warns about actual sleeping
> operations..

I'm more worried about existing users of kmap_atomic relying on
the preemption disabling semantics.  Short of someone checking
on every single instance (and that would include derived cases
such as all users of sg miter), I think the safer option is to
create something brand new and then migrate the existing users
to it.  Something like

static inline void *kmap_atomic_ifhigh(struct page *page)
{
	if (PageHighMem(page))
		return kmap_atomic(page);
	return page_address(page);
}

static inline void kunmap_atomic_ifhigh(struct page *page, void *addr)
{
	if (PageHighMem(page))
		kunmap_atomic(addr);
}

Cheers,
Ard Biesheuvel Sept. 15, 2020, 7:08 a.m. UTC | #6
On Tue, 15 Sep 2020 at 09:56, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Sep 14, 2020 at 11:45 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > I mean, I did find one case that didn't set it (cb710-mmc.c), but
> > pattern-matching to the other mmc cases, that one looks like it
> > _should_ have set the atomic flag like everybody else did.
>
> Oh, and immediately after sending that out I notice
> nvmet_bdev_execute_rw(), which does seem to make allocations inside
> that sg_miter loop.
>
> So those non-atomic cases do clearly exist.
>
> It does make the case for why kmap_atomic() wants to have the
> debugging code. It will "just work" on 64-bit to do it wrong, because
> the address doesn't become invalid just because you sleep or get
> rescheduled. But then the code that every developer tests (since
> developers tend to run on good hardware) might be completely broken on
> bad old hardware.
>

If we want code that is optimal on recent hardware, and yet still
correct on older 32-bit hardware, kmap() is definitely a better choice
here than kmap_atomic(), since it is a no-op on !HIGHMEM, and
tolerates sleeping on 32-bit. /That/ is why I wrote the code this way.

The problem is of course that kmap() itself might sleep.

So I would argue that the semantics in the name of kmap_atomic() are
not about the fact that it starts a non-preemptible section, but that
it can be *called from* a non-preemptible section. And starting a
non-preemptible section is unnecessary on !HIGHMEM, and should be
avoided if possible.

> Maybe we could hide it behind a debug option, at least.
>
> Or, alterantively, introduce a new "debug_preempt_count" that doesn't
> actually disable preemption, but warns about actual sleeping
> operations..
>
>              Linus
Ard Biesheuvel Sept. 15, 2020, 7:10 a.m. UTC | #7
On Tue, 15 Sep 2020 at 10:05, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Mon, Sep 14, 2020 at 11:55:53PM -0700, Linus Torvalds wrote:
> >
> > Maybe we could hide it behind a debug option, at least.
> >
> > Or, alterantively, introduce a new "debug_preempt_count" that doesn't
> > actually disable preemption, but warns about actual sleeping
> > operations..
>
> I'm more worried about existing users of kmap_atomic relying on
> the preemption disabling semantics.  Short of someone checking
> on every single instance (and that would include derived cases
> such as all users of sg miter), I think the safer option is to
> create something brand new and then migrate the existing users
> to it.  Something like
>
> static inline void *kmap_atomic_ifhigh(struct page *page)
> {
>         if (PageHighMem(page))
>                 return kmap_atomic(page);
>         return page_address(page);
> }
>
> static inline void kunmap_atomic_ifhigh(struct page *page, void *addr)
> {
>         if (PageHighMem(page))
>                 kunmap_atomic(addr);
> }
>

But we would still need to check all users of SG miter before we could
move it to this interface.
Thomas Gleixner Sept. 15, 2020, 9:34 a.m. UTC | #8
On Tue, Sep 15 2020 at 17:05, Herbert Xu wrote:
> On Mon, Sep 14, 2020 at 11:55:53PM -0700, Linus Torvalds wrote:
>>
>> Maybe we could hide it behind a debug option, at least.
>> 
>> Or, alterantively, introduce a new "debug_preempt_count" that doesn't
>> actually disable preemption, but warns about actual sleeping
>> operations..
>
> I'm more worried about existing users of kmap_atomic relying on
> the preemption disabling semantics.  Short of someone checking
> on every single instance (and that would include derived cases
> such as all users of sg miter), I think the safer option is to
> create something brand new and then migrate the existing users
> to it.  Something like
>
> static inline void *kmap_atomic_ifhigh(struct page *page)
> {
> 	if (PageHighMem(page))
> 		return kmap_atomic(page);
> 	return page_address(page);
> }
>
> static inline void kunmap_atomic_ifhigh(struct page *page, void *addr)
> {
> 	if (PageHighMem(page))
> 		kunmap_atomic(addr);
> }

Hmm, that still has the issue that the code between map and unmap must
not sleep and the conversion must carefully check whether anything in
this region relies on preemption being disabled by kmap_atomic()
regardless of highmem or not.

kmap_atomic() is at least consistent vs. preemption, the above not so
much.

I'd rather go for a preemptible/sleepable version of highmem mapping
which is in itself consistent for both highmen and not highmem.

Thanks,

        tglx
Ard Biesheuvel Sept. 15, 2020, 10:02 a.m. UTC | #9
On Tue, 15 Sep 2020 at 12:34, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Tue, Sep 15 2020 at 17:05, Herbert Xu wrote:
> > On Mon, Sep 14, 2020 at 11:55:53PM -0700, Linus Torvalds wrote:
> >>
> >> Maybe we could hide it behind a debug option, at least.
> >>
> >> Or, alterantively, introduce a new "debug_preempt_count" that doesn't
> >> actually disable preemption, but warns about actual sleeping
> >> operations..
> >
> > I'm more worried about existing users of kmap_atomic relying on
> > the preemption disabling semantics.  Short of someone checking
> > on every single instance (and that would include derived cases
> > such as all users of sg miter), I think the safer option is to
> > create something brand new and then migrate the existing users
> > to it.  Something like
> >
> > static inline void *kmap_atomic_ifhigh(struct page *page)
> > {
> >       if (PageHighMem(page))
> >               return kmap_atomic(page);
> >       return page_address(page);
> > }
> >
> > static inline void kunmap_atomic_ifhigh(struct page *page, void *addr)
> > {
> >       if (PageHighMem(page))
> >               kunmap_atomic(addr);
> > }
>
> Hmm, that still has the issue that the code between map and unmap must
> not sleep and the conversion must carefully check whether anything in
> this region relies on preemption being disabled by kmap_atomic()
> regardless of highmem or not.
>
> kmap_atomic() is at least consistent vs. preemption, the above not so
> much.
>

But that is really the point. I don't *want* to be forced to disable
preemption in brand new code simply because some legacy highmem API
conflates being callable from atomic context with instantiating an
atomic context by disabling preemption for no good reason. IIUC, in
the past, you would really only call kmap_atomic() if you absolutely
had to, and so you would never rely on the preemption disabling
semantics accidentally. By making kmap_atomic() the preferred API even
for calls from non-atomic contexts, this line has blurred and we no
longer know why individual kmap_atomic() occurrences exist in the
first place.

> I'd rather go for a preemptible/sleepable version of highmem mapping
> which is in itself consistent for both highmen and not highmem.
>

I don't think we need to obsess about highmem, although we should
obviously take care not to regress its performance unnecessarily. What
I want to avoid is to burden a brand new subsystem with legacy highmem
baggage simply because we could not agree on how to avoid that.
Herbert Xu Sept. 15, 2020, 10:05 a.m. UTC | #10
On Tue, Sep 15, 2020 at 01:02:10PM +0300, Ard Biesheuvel wrote:
>
> > I'd rather go for a preemptible/sleepable version of highmem mapping
> > which is in itself consistent for both highmen and not highmem.
> 
> I don't think we need to obsess about highmem, although we should
> obviously take care not to regress its performance unnecessarily. What
> I want to avoid is to burden a brand new subsystem with legacy highmem
> baggage simply because we could not agree on how to avoid that.

I think what Thomas is proposing should address your concerns Ard.
As long as nobody objects to the slight performance degradation on
legacy highmem platforms it should make kmap_atomic just go away on
modern platforms.

Cheers,
Ard Biesheuvel Sept. 15, 2020, 10:08 a.m. UTC | #11
On Tue, 15 Sep 2020 at 13:05, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Tue, Sep 15, 2020 at 01:02:10PM +0300, Ard Biesheuvel wrote:
> >
> > > I'd rather go for a preemptible/sleepable version of highmem mapping
> > > which is in itself consistent for both highmen and not highmem.
> >
> > I don't think we need to obsess about highmem, although we should
> > obviously take care not to regress its performance unnecessarily. What
> > I want to avoid is to burden a brand new subsystem with legacy highmem
> > baggage simply because we could not agree on how to avoid that.
>
> I think what Thomas is proposing should address your concerns Ard.
> As long as nobody objects to the slight performance degradation on
> legacy highmem platforms it should make kmap_atomic just go away on
> modern platforms.
>

But making atomic kmap preemptible/sleepable creates the exact same
problem, i.e., that we have no idea which existing callers are
currently relying on those preemption disabling semantics, so we can't
just take them away. Or am I missing something?
Herbert Xu Sept. 15, 2020, 10:10 a.m. UTC | #12
On Tue, Sep 15, 2020 at 01:08:31PM +0300, Ard Biesheuvel wrote:
>
> But making atomic kmap preemptible/sleepable creates the exact same
> problem, i.e., that we have no idea which existing callers are
> currently relying on those preemption disabling semantics, so we can't
> just take them away. Or am I missing something?

Good point.

Thomas mentioned that RT has been doing this for a while now so
perhaps someone has studied this problem already? Thomas?

Cheers,
Thomas Gleixner Sept. 15, 2020, 7:04 p.m. UTC | #13
On Tue, Sep 15 2020 at 20:10, Herbert Xu wrote:
> On Tue, Sep 15, 2020 at 01:08:31PM +0300, Ard Biesheuvel wrote:
>>
>> But making atomic kmap preemptible/sleepable creates the exact same
>> problem, i.e., that we have no idea which existing callers are
>> currently relying on those preemption disabling semantics, so we can't
>> just take them away. Or am I missing something?
>
> Good point.
>
> Thomas mentioned that RT has been doing this for a while now so
> perhaps someone has studied this problem already? Thomas?

RT is substituting preempt_disable() with migrate_disable() which pins
the task on the CPU so that per CPU stuff still works. And we did quite
some staring whether there is code which purely relies on the
preempt_disable() to prevent reentrancy, but there is almost none.

Though we don't have migrate disable on !RT and PeterZ is not a great
fan of making it available as it wreckages schedulability - though IMO
not much more than preempt disable :)

Thanks,

        tglx

Patch
diff mbox series

diff --git a/lib/crypto/chacha20poly1305.c b/lib/crypto/chacha20poly1305.c
index 431e04280332..5850f3b87359 100644
--- a/lib/crypto/chacha20poly1305.c
+++ b/lib/crypto/chacha20poly1305.c
@@ -251,9 +251,7 @@  bool chacha20poly1305_crypt_sg_inplace(struct scatterlist *src,
 			poly1305_update(&poly1305_state, pad0, 0x10 - (ad_len & 0xf));
 	}
 
-	flags = SG_MITER_TO_SG;
-	if (!preemptible())
-		flags |= SG_MITER_ATOMIC;
+	flags = SG_MITER_TO_SG | SG_MITER_ATOMIC;
 
 	sg_miter_start(&miter, src, sg_nents(src), flags);