Re: [PATCH net-next v2 2/5] net/tcp-ao: Use crypto library API instead of crypto_ahash

["Ard Biesheuvel" <ardb@kernel.org>]

On Tue, 28 Apr 2026, at 12:10, David Laight wrote:
> On Tue, 28 Apr 2026 08:34:47 +0200
> "Ard Biesheuvel" <ardb@kernel.org> wrote:
>
>> On Tue, 28 Apr 2026, at 03:24, David Laight wrote:
>> > On Mon, 27 Apr 2026 10:27:24 -0700
>> > Eric Biggers <ebiggers@kernel.org> wrote:
>> >  
>> >> Currently the kernel's TCP-AO implementation does the MAC and KDF
>> >> computations using the crypto_ahash API.  This API is inefficient and
>> >> difficult to use, and it has required extensive workarounds in the form
>> >> of per-CPU preallocated objects (tcp_sigpool) to work at all.
>> >> 
>> >> Let's use lib/crypto/ instead.  This means switching to straightforward
>> >> stack-allocated structures, virtually addressed buffers, and direct
>> >> function calls.  It also means removing quite a bit of error handling.
>> >> This makes TCP-AO quite a bit faster.
>> >> 
>> >> This also enables many additional cleanups, which later commits will
>> >> handle: removing tcp-sigpool, removing support for crypto_tfm cloning,
>> >> removing more error handling, and replacing more dynamically-allocated
>> >> buffers with stack buffers based on the now-statically-known limits.
>> >> 
>> >> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>> >> Signed-off-by: Eric Biggers <ebiggers@kernel.org>  
>> > ...  
>> >> @@ -344,33 +444,26 @@ static int tcp_v4_ao_calc_key(struct tcp_ao_key *mkt, u8 *key,
>> >>  	struct kdf_input_block {
>> >>  		u8                      counter;
>> >>  		u8                      label[6];
>> >>  		struct tcp4_ao_context	ctx;
>> >>  		__be16                  outlen;
>> >> -	} __packed * tmp;  
>> >
>> > That looks a bit horrid.
>> > I also had a feeling that the compiler sometimes rejects non-packed structures
>> > inside packed ones.
>> > Perhaps nest the whole thing inside another structure that has an initial
>> > u8 pad and is marked __packed __aligned(4).
>> > Then the assignments to the fields of 'ctx' will be known to be aligned
>> > even when tcp4_ao_context is also __packed.
>> >  
>> 
>> Agree with Eric that this has no bearing on this patch,
>
> true - just the in the same code.
>
>> but I'm not sure
>> I see the problem here. 'ctx' will not be packed, and appear misaligned
>> in struct kdf_input_block, but that would only matter if the address of
>> the ctx field were taken and passed to a function taking a pointer to
>> struct tcp4_ao_context (which would expect it to appear naturally
>> aligned).
>> 
>> Having a feeling about what the compiler sometimes rejects is not
>> actionable feedback - could you be more specific about which problem
>> you think needs to be solved here? Are you concerned about unaligned
>> accesses when populating the struct?
>
> (It was 2am and the side effects of a cold were stopping me sleeping...)
>
> I tend to double-check __packed because it gets misused in places
> where you really want the compiler to error implicit padding rather
> than generate expensive misaligned access code.
>
> But I am sure I remember some build warning that needed __packed added
> to the definition of a structure embedded in a __packed structure.
> I don't think it was only the arm OABI (which pads structures to 2 bytes).
> Historically this has never mattered (even the 'address of packed member'
> error is moderately recent - well sometime in the last 20 years).
>
> In this case (and the ipv6 code) 'struct tcp4_ao_context' can just be
> marked __packed.
> Or, since this is the only place it is used, possibly just inlined
> into 'struct kdf_input_block' - which may not even need to be named.
>

What would that achieve, exactly? You still haven't explained what is
wrong with the code. Or are you really claiming that structs lacking
the packed attribute are not permitted as fields in __packed structs?