| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ffe3eb53-ac49-4049-8508-bd4ffe890ce2@openvpn.net>
Date: Thu, 13 Nov 2025 09:37:29 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Ralf Lici <ralf@...delbit.com>
Subject: Re: [PATCH net-next 6/8] ovpn: consolidate crypto allocations in one
chunk
Hi Sabrina,
On 12/11/2025 17:29, Sabrina Dubroca wrote:
> 2025-11-11, 22:47:39 +0100, Antonio Quartulli wrote:
>> From: Ralf Lici <ralf@...delbit.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.
Yep! Quite impressive.
>
>
>> Signed-off-by: Ralf Lici <ralf@...delbit.com>
>> Signed-off-by: Antonio Quartulli <antonio@...nvpn.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.
You're right. I worked with Ralf to get this patch done and I directly
pulled it in my tree.
Next time we will make sure all patches land on the openvpn-devel
mailing list before being staged for net-next.
This said, Ralf will get back to you on the questions below.
Thanks!
Regards,
>
>
>> 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)
>
--
Antonio Quartulli
OpenVPN Inc.
Powered by blists - more mailing lists