Re: [PATCH net-next 6/8] ovpn: consolidate crypto allocations in one chunk

[Sabrina Dubroca <sd@queasysnail.net>]

2025-11-11, 22:47:39 +0100, Antonio Quartulli wrote:
> From: Ralf Lici <ralf@mandelbit.com>
> 
> Currently ovpn uses three separate dynamically allocated structures to
> set up cryptographic operations for both encryption and decryption. This
> adds overhead to performance-critical paths and contribute to memory
> fragmentation.
> 
> This commit consolidates those allocations into a single temporary blob,
> similar to what esp_alloc_temp() does.

nit: esp_alloc_tmp (no 'e')

> The resulting performance gain is +7.7% and +4.3% for UDP when using AES
> and ChaChaPoly respectively, and +4.3% for TCP.

Nice improvement! I didn't think it would be that much.


> Signed-off-by: Ralf Lici <ralf@mandelbit.com>
> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>

BTW, I didn't see any of these patches posted on the openvpn-devel
list or on netdev before this pull request. Otherwise I'd have
reviewed them earlier.


>  drivers/net/ovpn/crypto_aead.c | 151 +++++++++++++++++++++++++--------
>  drivers/net/ovpn/io.c          |   8 +-
>  drivers/net/ovpn/skb.h         |  13 ++-
>  3 files changed, 129 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/ovpn/crypto_aead.c b/drivers/net/ovpn/crypto_aead.c
> index cb6cdf8ec317..9ace27fc130a 100644
> --- a/drivers/net/ovpn/crypto_aead.c
> +++ b/drivers/net/ovpn/crypto_aead.c
> @@ -36,6 +36,105 @@ static int ovpn_aead_encap_overhead(const struct ovpn_crypto_key_slot *ks)
>  		crypto_aead_authsize(ks->encrypt);	/* Auth Tag */
>  }
>  
> +/*

nit: missing a 2nd * to make it kdoc?

> + * ovpn_aead_crypto_tmp_size - compute the size of a temporary object containing
> + *			       an AEAD request structure with extra space for SG
> + *			       and IV.
> + * @tfm: the AEAD cipher handle
> + * @nfrags: the number of fragments in the skb
> + *
> + * This function calculates the size of a contiguous memory block that includes
> + * the initialization vector (IV), the AEAD request, and an array of scatterlist
> + * entries. For alignment considerations, the IV is placed first, followed by
> + * the request, and then the scatterlist.
> + * Additional alignment is applied according to the requirements of the
> + * underlying structures.
> + *
> + * Return: the size of the temporary memory that needs to be allocated
> + */
> +static unsigned int ovpn_aead_crypto_tmp_size(struct crypto_aead *tfm,
> +					      const unsigned int nfrags)
> +{
> +	unsigned int len = crypto_aead_ivsize(tfm);
> +
> +	if (likely(len)) {

Is that right?

Previously iv was reserved with a constant size (OVPN_NONCE_SIZE), and
we're always going to write some data into ->iv via
ovpn_pktid_aead_write, but now we're only reserving the crypto
algorithm's IV size (which appear to be 12, ie OVPN_NONCE_SIZE, for
both chachapoly and gcm(aes), so maybe it doesn't matter).


> @@ -71,13 +171,15 @@ int ovpn_aead_encrypt(struct ovpn_peer *peer, struct ovpn_crypto_key_slot *ks,
>  	if (unlikely(nfrags + 2 > (MAX_SKB_FRAGS + 2)))
>  		return -ENOSPC;
>  
> -	/* sg may be required by async crypto */
> -	ovpn_skb_cb(skb)->sg = kmalloc(sizeof(*ovpn_skb_cb(skb)->sg) *
> -				       (nfrags + 2), GFP_ATOMIC);
> -	if (unlikely(!ovpn_skb_cb(skb)->sg))
> +	/* allocate temporary memory for iv, sg and req */
> +	tmp = kmalloc(ovpn_aead_crypto_tmp_size(ks->encrypt, nfrags),
> +		      GFP_ATOMIC);
> +	if (unlikely(!tmp))
>  		return -ENOMEM;
>  
> -	sg = ovpn_skb_cb(skb)->sg;
> +	iv = ovpn_aead_crypto_tmp_iv(ks->encrypt, tmp);
> +	req = ovpn_aead_crypto_tmp_req(ks->encrypt, iv);
> +	sg = ovpn_aead_crypto_req_sg(ks->encrypt, req);
>  
>  	/* sg table:
>  	 * 0: op, wire nonce (AD, len=OVPN_OP_SIZE_V2+OVPN_NONCE_WIRE_SIZE),
> @@ -105,13 +207,6 @@ int ovpn_aead_encrypt(struct ovpn_peer *peer, struct ovpn_crypto_key_slot *ks,
>  	if (unlikely(ret < 0))
>  		return ret;
>  
> -	/* iv may be required by async crypto */
> -	ovpn_skb_cb(skb)->iv = kmalloc(OVPN_NONCE_SIZE, GFP_ATOMIC);
> -	if (unlikely(!ovpn_skb_cb(skb)->iv))
> -		return -ENOMEM;
> -
> -	iv = ovpn_skb_cb(skb)->iv;
> -
>  	/* concat 4 bytes packet id and 8 bytes nonce tail into 12 bytes
>  	 * nonce
>  	 */
> @@ -130,11 +225,7 @@ int ovpn_aead_encrypt(struct ovpn_peer *peer, struct ovpn_crypto_key_slot *ks,
>  	/* AEAD Additional data */
>  	sg_set_buf(sg, skb->data, OVPN_AAD_SIZE);
>  
> -	req = aead_request_alloc(ks->encrypt, GFP_ATOMIC);
> -	if (unlikely(!req))
> -		return -ENOMEM;
> -
> -	ovpn_skb_cb(skb)->req = req;
> +	ovpn_skb_cb(skb)->crypto_tmp = tmp;

That should be done immediately after the allocation, so that any
failure before this (skb_to_sgvec_nomark, ovpn_pktid_xmit_next) will
not leak this blob? ovpn_aead_encrypt returns directly and lets
ovpn_encrypt_post handle the error and free the memory, but only after
 ->crypto_tmp has been set.

(same thing on the decrypt path)

-- 
Sabrina