(+ Jason) On Tue, 15 Sep 2020 at 06:30, Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx> > Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxx> > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
