Re: [PATCH 04/33] lib/crypto: aes: Add CTR and XCTR support

[David Laight]

On Tue, 14 Jul 2026 06:53:32 +0200
Thomas Huth <thuth@xxxxxxxxxx> wrote:

> On 14/07/2026 01.54, Eric Biggers wrote:
> > On Mon, Jul 13, 2026 at 10:39:53AM +0200, Thomas Huth wrote:  
> >>> diff --git a/Documentation/crypto/libcrypto-unauth-encryption.rst b/Documentation/crypto/libcrypto-unauth-encryption.rst
> >>> index fb8106034089..6aca01d715da 100644
> >>> --- a/Documentation/crypto/libcrypto-unauth-encryption.rst
> >>> +++ b/Documentation/crypto/libcrypto-unauth-encryption.rst
> >>> @@ -27,6 +27,13 @@ Support for AES in the CBC and CBC-CTS modes of operation.
> >>>    .. kernel-doc:: include/crypto/aes-cbc.h
> >>> +AES-CTR and AES-XCTR
> >>> +--------------------
> >>> +
> >>> +Support for AES in the CTR and XCTR modes of operation.  
> >>
> >> I guess you already have this on your radar, but just in case: It would be
> >> nice to turn this into a full sentence, too.  
> > 
> > Yes, I'm making all of them full sentences.
> >   
> >>> +/**
> >>> + * aes_ctr() - AES-CTR en/decryption
> >>> + * @dst: The destination buffer.  Can be in-place or out-of-place.  For other
> >>> + *	 overlaps the behavior is unspecified.
> >>> + * @src: The source data
> >>> + * @len: Number of bytes to en/decrypt
> >>> + * @ctr: The counter.  It will be incremented by ceil(@len / AES_BLOCK_SIZE).
> >>> + * @key: The key
> >>> + *
> >>> + * This implements AES in counter mode with a 128-bit big endian counter.
> >>> + *
> >>> + * This supports incremental en/decryption.  The length of each non-final chunk
> >>> + * must be a multiple of AES_BLOCK_SIZE, and the updated @ctr must be passed in
> >>> + * each time.  
> >>
> >> Maybe add some wording that ctr ideally should not be 0 for the first call,
> >> i.e. a "nonce" value?  
> > 
> > It depends on the usage.  If a distinct key is used for each message for
> > example, always starting at 0 is perfectly fine.
> > 
> > I'm not sure how far we should go to document the proper use of each
> > algorithm.  Really the AES-CTR support is just for internal use by
> > AES-GCM and AES-CCM, and a few odd users that implement specific other
> > protocols that need AES-CTR.  It's not intended to be a place to go to
> > receive an introduction to CTR mode.
> >   
> >>> +static __always_inline void inc_be128_ctr(u8 ctr[AES_BLOCK_SIZE])
> >>> +{
> >>> +	/* Casts to u8 are needed because of the implicit integer promotion. */
> >>> +	if (((u8)++ctr[AES_BLOCK_SIZE - 1]) != 0)
> >>> +		return;  
> >>
> >> Why do you handle the first value separately here? The code could be
> >> simplified to start with "int i = AES_BLOCK_SIZE -1" in the for-loop
> >> instead?  
> > 
> > Just a trick to optimize performance by unrolling the first iteration,
> > since 255 times out of 256 the first iteration is enough.  
> Ok, but then maybe add a comment here. Otherwise people will wonder why it 
> has been done like this when reading the code later.
> 
> FWIW, I doubt that this really makes a big difference here. Looking at a 
> function that contains your code, the disassembly currently looks like this 
> (with -O2):
> 
> 0000000000000000 <inc_be128_ctr>:
>     0:   80 47 0f 01             addb   $0x1,0xf(%rdi)
>     4:   48 8d 57 0e             lea    0xe(%rdi),%rdx
Those two run in the same clock.
>     8:   74 0a                   je     14 <inc_be128_ctr+0x14>
Depends on the 'addb', hopefully/likely predicted non-taken.
>     a:   c3                      ret
Predicted taken using the RSB.
Execution continues speculatively after the call even before the memory read
for 0xf(%rdi) has completed.
>     b:   0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
>    10:   48 83 ea 01             sub    $0x1,%rdx
>    14:   80 02 01                addb   $0x1,(%rdx)
>    17:   75 05                   jne    1e <inc_be128_ctr+0x1e>
>    19:   48 39 d7                cmp    %rdx,%rdi
>    1c:   75 f2                   jne    10 <inc_be128_ctr+0x10>
>    1e:   c3                      ret
> 
> So that's 3 assembly instructions 'til you reach the "ret".
> 
> When you drop the optimization, it looks like this:
> 
> 0000000000000000 <inc_be128_ctr>:
>     0:   48 8d 57 0f             lea    0xf(%rdi),%rdx
>     4:   eb 0e                   jmp    14 <inc_be128_ctr+0x14>
>     6:   66 2e 0f 1f 84 00 00    cs nopw 0x0(%rax,%rax,1)
>     d:   00 00 00
>    10:   48 83 ea 01             sub    $0x1,%rdx
>    14:   80 02 01                addb   $0x1,(%rdx)
That depends on the 'sub'
>    17:   75 05                   jne    1e <inc_be128_ctr+0x1e>
That is likely predicted not taken - so you get a misprediction penalty
>    19:   48 39 d7                cmp    %rdx,%rdi
>    1c:   75 f2                   jne    10 <inc_be128_ctr+0x10>
>    1e:   c3                      ret
> 
> That's 4 assembly instructions 'til you reach the "ret". Not such a big 
> difference...?

There is an extra 'jmp' - that will cost in the fetch-decode part.
The alignment pad (not there in the kernel) might also force another
i-cache line be accessed.
The first conditional branch is mispredicted - say 20 clocks. 

In any case the instruction count doesn't really matter.
What matters is the length of the register dependency chain and how
the branches get predicted.
I've gone back and annotated the asm.

> 
> And with -O3, both variants end up with the same code.

No one sane would compile the kernel (or much else) with -O3.
It bloats things too much.
gcc isn't that good at optimising loops for modern cpu. 

-- David

> 
>   Thomas
> 
>