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

[David Laight]

On Tue, 28 Apr 2026 18:38:51 +0200
"Ard Biesheuvel" <ardb@xxxxxxxxxx> wrote:

> On Tue, 28 Apr 2026, at 12:10, David Laight wrote:
> > On Tue, 28 Apr 2026 08:34:47 +0200
> > "Ard Biesheuvel" <ardb@xxxxxxxxxx> wrote:
> >  
> >> On Tue, 28 Apr 2026, at 03:24, David Laight wrote:  
> >> > On Mon, 27 Apr 2026 10:27:24 -0700
> >> > Eric Biggers <ebiggers@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> >> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>    
> >> > ...    
> >> >> @@ -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?

My brain probably misfiled something :-(

	David