Skip to content

perf: embed Hmac in cookie_checker to eliminate make_cookie malloc #22

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MarkAtwood wants to merge 1 commit into
base: master
Choose a base branch
from
Open

perf: embed Hmac in cookie_checker to eliminate make_cookie malloc #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 23 additions & 26 deletions kernel-src/cookie.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,52 +103,49 @@ static int __must_check compute_mac2(u8 mac2[COOKIE_LEN], const void *message, s
static int __must_check make_cookie(u8 cookie[COOKIE_LEN], struct sk_buff *skb,
struct cookie_checker *checker)
{
int ret;
struct Hmac *wc_hmac; /* sizeof(struct Hmac) is 832 if SHA3 is enabled. */

wc_hmac = (struct Hmac *)malloc(sizeof(*wc_hmac));
if (! wc_hmac)
return -ENOMEM;
int ret = 0;

ret = wc_HmacInit(wc_hmac, NULL /* heap */, INVALID_DEVID);
if (ret < 0)
goto out_hmac_uninited;
/* Take write lock for the entire operation to ensure exclusive access
* to checker->make_cookie_hmac. make_cookie is only invoked under
* rate-limiting conditions, so serialising on the write lock is fine.
*/
down_write(&checker->secret_lock);

if ((ret == 0) && wg_birthdate_has_expired(checker->secret_birthdate,
if (wg_birthdate_has_expired(checker->secret_birthdate,
COOKIE_SECRET_MAX_AGE)) {
down_write(&checker->secret_lock);
ret = wc_get_random_bytes(checker->secret, NOISE_HASH_LEN);
if (ret == 0)
checker->secret_birthdate = ktime_get_coarse_boottime_ns();
up_write(&checker->secret_lock);
}

if (ret == 0) {
down_read(&checker->secret_lock);
if (ret == 0)
ret = wc_HmacInit(&checker->make_cookie_hmac, NULL /* heap */, INVALID_DEVID);

ret = wc_HmacSetKey(wc_hmac, WC_SHA256, checker->secret, NOISE_HASH_LEN);
if (ret == 0) {
ret = wc_HmacSetKey(&checker->make_cookie_hmac, WC_SHA256,
checker->secret, NOISE_HASH_LEN);

if ((ret == 0) && (skb->protocol == htons(ETH_P_IP)))
ret = wc_HmacUpdate(wc_hmac, (u8 *)&ip_hdr(skb)->saddr,
ret = wc_HmacUpdate(&checker->make_cookie_hmac,
(u8 *)&ip_hdr(skb)->saddr,
(word32)sizeof(struct in_addr));
else if ((ret == 0) && (skb->protocol == htons(ETH_P_IPV6)))
ret = wc_HmacUpdate(wc_hmac, (u8 *)&ipv6_hdr(skb)->saddr,
ret = wc_HmacUpdate(&checker->make_cookie_hmac,
(u8 *)&ipv6_hdr(skb)->saddr,
(word32)sizeof(struct in6_addr));

if (ret == 0)
ret = wc_HmacUpdate(wc_hmac, (u8 *)&udp_hdr(skb)->source, sizeof(__be16));
ret = wc_HmacUpdate(&checker->make_cookie_hmac,
(u8 *)&udp_hdr(skb)->source,
sizeof(__be16));

if (ret == 0)
ret = wc_HmacFinal(wc_hmac, cookie);
ret = wc_HmacFinal(&checker->make_cookie_hmac, cookie);

up_read(&checker->secret_lock);
wc_HmacFree(&checker->make_cookie_hmac);
}

wc_HmacFree(wc_hmac);

out_hmac_uninited:

free(wc_hmac);
up_write(&checker->secret_lock);
Comment on lines +108 to +148
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding secret_lock as a write-lock across the full HMAC computation blocks all potential readers of secret_lock for the duration of wc_HmacInit/Update/Final, not just secret refresh. Under sustained rate-limited traffic (e.g., an active attacker), this can amplify contention and potentially delay other threads that only need read access to the cookie secret. A more scalable approach is to keep secret_lock semantics focused on guarding checker->secret (e.g., take down_read, copy checker->secret into a small stack/local buffer like u8 secret[NOISE_HASH_LEN], then up_read), and separately serialize access to make_cookie_hmac with a dedicated mutex/lock; this preserves the “no per-call allocation” goal without turning every cookie generation into a global write-side stop-the-world for the checker.

Copilot uses AI. Check for mistakes.

return ret;
}
Expand Down
5 changes: 5 additions & 0 deletions kernel-src/cookie.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#define _WG_COOKIE_H

#include "messages.h"
#include "wolfcrypt_glue.h"
#include <linux/rwsem.h>

struct wg_peer;
Expand All @@ -19,6 +20,10 @@ struct cookie_checker {
u8 message_mac1_key[NOISE_SYMMETRIC_KEY_LEN];
u64 secret_birthdate;
struct rw_semaphore secret_lock;
/* Scratch buffer for make_cookie() — avoids per-call kmalloc of the
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment uses a Unicode em dash (—), which is often discouraged in kernel-style codebases that prefer plain ASCII in source for consistency/tooling, and it also says “kmalloc” even though the prior code path used malloc() (or a wrapper). Consider switching to ASCII punctuation (e.g., -- or -) and using allocator-neutral wording (“allocation”) or the exact allocator name used in this module to prevent confusion and style/toolchain issues.

Suggested change
/* Scratch buffer for make_cookie() — avoids per-call kmalloc of the
/* Scratch buffer for make_cookie() - avoids per-call allocation of the

Copilot uses AI. Check for mistakes.
* 832-byte Hmac struct. Used only while secret_lock is held for write.
*/
struct Hmac make_cookie_hmac;
Comment on lines +23 to +26
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Embedding an ~832-byte struct Hmac directly into struct cookie_checker increases the size of what is likely a frequently-touched structure, which can degrade cache locality for unrelated cookie-checking operations. To keep the “allocate once, reuse many” benefit without inflating the hot struct, consider storing struct Hmac *make_cookie_hmac (or a dedicated scratch sub-struct) allocated once at device init/teardown; this avoids per-call allocation while reducing steady-state cache footprint.

Suggested change
/* Scratch buffer for make_cookie() — avoids per-call kmalloc of the
* 832-byte Hmac struct. Used only while secret_lock is held for write.
*/
struct Hmac make_cookie_hmac;
/* Scratch buffer for make_cookie(); allocate once during init and reuse
* to avoid per-call allocation without inflating this hot structure.
* Used only while secret_lock is held for write.
*/
struct Hmac *make_cookie_hmac;

Copilot uses AI. Check for mistakes.
struct wg_device *device;
};

Expand Down