| Message ID | 20200915033024.GB25789@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: lib/chacha20poly1305 - Set SG_MITER_ATOMIC unconditionally | expand |
(+ 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
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,
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
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
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,
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
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.
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
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.
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,
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?
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,
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
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);