On Tue, 15 Sep 2020 at 01:43, Linus Torvalds torvalds@linux-foundation.org wrote:

On Mon, Sep 14, 2020 at 3:24 PM Linus Torvalds torvalds@linux-foundation.org wrote:

...

Ard and Herbert added to participants: see chacha20poly1305_crypt_sg_inplace(), which does

    flags = SG_MITER_TO_SG;
    if (!preemptible())
            flags |= SG_MITER_ATOMIC;

introduced in commit d95312a3ccc0 ("crypto: lib/chacha20poly1305 - reimplement crypt_from_sg() routine").

As far as I can tell, the only reason for this all is to try to use "kmap()" rather than "kmap_atomic()".

And kmap() actually has the much more complex "might_sleep()" tests, and apparently the "preemptible()" check wasn't even the proper full debug check, it was just a complete hack to catch the one that triggered.

This was not driven by a failing check.

The documentation of kmap_atomic() states the following:

* The use of kmap_atomic/kunmap_atomic is discouraged - kmap/kunmap * gives a more generic (and caching) interface. But kmap_atomic can * be used in IRQ contexts, so in some (very limited) cases we need * it.

so if this is no longer accurate, perhaps we should fix it?

But another reason I tried to avoid kmap_atomic() is that it disables preemption unconditionally, even on 64-bit architectures where HIGHMEM is irrelevant. So using kmap_atomic() here means that the bulk of WireGuard packet encryption runs with preemption disabled, essentially for legacy reasons.

From a quick look, that code should probably just get rid of SG_MITER_ATOMIC entirely, and alwayse use kmap_atomic().

kmap_atomic() is actually the faster and proper interface to use anyway (never mind that any of this matters on any sane hardware). The old kmap() and kunmap() interfaces should generally be avoided like the plague - yes, they allow sleeping in the middle and that is sometimes required, but if you don't need that, you should never ever use them.

We used to have a very nasty kmap_atomic() that required people to be very careful and know exactly which atomic entry to use, and that was admitedly quite nasty.

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?

Because if there is, that choice should come from the outside, not from inside lib/scatterlist.c trying to make some bad guess based on the wrong thing entirely.

             Linus