Replace the dynamic skb extension allocator (->chunks + per-object offset[] array) with fixed per-id slots with offsets computed at compile time. The runtime-computed offsets seem to be a leftover from the initial posting [1], where skb_ext memory was reallocated when a new extension was activated. This can make extension delete-then-re-add unsafe, as pointed out by Sashiko [2]: with bump allocation, re-adding an extension appends a second copy, eventually overflowing the skb_ext chunks area. While this does not happen today, because all extensions get dropped on skb scrub, the BPF metadata skb extension work aims to preserve an extension across skb scrubbing, which opens the door to this scenario. [1] https://lore.kernel.org/all/20181210145006.19098-3-fw@strlen.de/ [2] https://lore.kernel.org/all/20260815081452.0DB521F00A3E@smtp.kernel.org/ Signed-off-by: Jakub Sitnicki --- I'm not sure if there is another easy alternative. Moving the skb ext chunks around to close the gaps would involve making sure that nobody is holding a pointer to them. Looking for feedback & ideas. --- include/linux/skbuff.h | 72 ++++++++++++++++++++++++++++++++----------- net/core/skbuff.c | 83 +++++++++++++++++++------------------------------- 2 files changed, 85 insertions(+), 70 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 95184183180f..5a5143e47b30 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -5011,45 +5011,81 @@ static inline void skb_set_nfct(struct sk_buff *skb, unsigned long nfct) } #ifdef CONFIG_SKB_EXTENSIONS -enum skb_ext_id { + #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) - SKB_EXT_BRIDGE_NF, +#define SKB_EXT_X_BRIDGE_NF X(SKB_EXT_BRIDGE_NF, struct nf_bridge_info) +#else +#define SKB_EXT_X_BRIDGE_NF #endif -#ifdef CONFIG_XFRM - SKB_EXT_SEC_PATH, + +#if IS_ENABLED(CONFIG_XFRM) +#define SKB_EXT_X_SEC_PATH X(SKB_EXT_SEC_PATH, struct sec_path) +#else +#define SKB_EXT_X_SEC_PATH #endif + #if IS_ENABLED(CONFIG_NET_TC_SKB_EXT) - TC_SKB_EXT, +#define SKB_EXT_X_TC X(TC_SKB_EXT, struct tc_skb_ext) +#else +#define SKB_EXT_X_TC #endif + #if IS_ENABLED(CONFIG_MPTCP) - SKB_EXT_MPTCP, +#define SKB_EXT_X_MPTCP X(SKB_EXT_MPTCP, struct mptcp_ext) +#else +#define SKB_EXT_X_MPTCP #endif + #if IS_ENABLED(CONFIG_MCTP_FLOWS) - SKB_EXT_MCTP, +#define SKB_EXT_X_MCTP X(SKB_EXT_MCTP, struct mctp_flow) +#else +#define SKB_EXT_X_MCTP #endif + #if IS_ENABLED(CONFIG_INET_PSP) - SKB_EXT_PSP, +#define SKB_EXT_X_PSP X(SKB_EXT_PSP, struct psp_skb_ext) +#else +#define SKB_EXT_X_PSP #endif + #if IS_ENABLED(CONFIG_CAN) - SKB_EXT_CAN, +#define SKB_EXT_X_CAN X(SKB_EXT_CAN, struct can_skb_ext) +#else +#define SKB_EXT_X_CAN #endif - SKB_EXT_NUM, /* must be last */ + +#define SKB_EXT_FOREACH(X) \ + SKB_EXT_X_BRIDGE_NF \ + SKB_EXT_X_SEC_PATH \ + SKB_EXT_X_TC \ + SKB_EXT_X_MPTCP \ + SKB_EXT_X_MCTP \ + SKB_EXT_X_PSP \ + SKB_EXT_X_CAN + +enum skb_ext_id { +#define X(id, type) id, + SKB_EXT_FOREACH(X) +#undef X + SKB_EXT_NUM, }; +extern const u8 skb_ext_offset[SKB_EXT_NUM]; + /** * struct skb_ext - sk_buff extensions * @refcnt: 1 on allocation, deallocated on 0 - * @offset: offset to add to @data to obtain extension address - * @chunks: size currently allocated, stored in SKB_EXT_ALIGN_SHIFT units + * @present_extensions: bitmap of extensions stored in @data * @data: start of extension data, variable sized * - * Note: offsets/lengths are stored in chunks of 8 bytes, this allows - * to use 'u8' types while allowing up to 2kb worth of extension data. + * Each extension id occupies a fixed slot within @data, located at + * skb_ext_offset[id] chunks of 8 bytes. Storing offsets/lengths + * in 8-byte chunks allows 'u8' types while allowing up to 2kb worth + * of extension data. */ struct skb_ext { refcount_t refcnt; - u8 offset[SKB_EXT_NUM]; /* in chunks of 8 bytes */ - u8 chunks; /* same */ + u8 present_extensions; char data[] __aligned(8); }; @@ -5087,7 +5123,7 @@ static inline void skb_ext_copy(struct sk_buff *dst, const struct sk_buff *src) static inline bool __skb_ext_exist(const struct skb_ext *ext, enum skb_ext_id i) { - return !!ext->offset[i]; + return ext->present_extensions & (1 << i); } static inline bool skb_ext_exist(const struct sk_buff *skb, enum skb_ext_id id) @@ -5106,7 +5142,7 @@ static inline void *skb_ext_find(const struct sk_buff *skb, enum skb_ext_id id) if (skb_ext_exist(skb, id)) { struct skb_ext *ext = skb->extensions; - return (void *)ext + (ext->offset[id] << 3); + return (void *)ext + (skb_ext_offset[id] << 3); } return NULL; diff --git a/net/core/skbuff.c b/net/core/skbuff.c index c82a1472a5ea..8e5db579725d 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -5134,47 +5134,32 @@ EXPORT_SYMBOL_GPL(skb_segment); #define SKB_EXT_CHUNKSIZEOF(x) (ALIGN((sizeof(x)), SKB_EXT_ALIGN_VALUE) / SKB_EXT_ALIGN_VALUE) static const u8 skb_ext_type_len[] = { -#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) - [SKB_EXT_BRIDGE_NF] = SKB_EXT_CHUNKSIZEOF(struct nf_bridge_info), -#endif -#ifdef CONFIG_XFRM - [SKB_EXT_SEC_PATH] = SKB_EXT_CHUNKSIZEOF(struct sec_path), -#endif -#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT) - [TC_SKB_EXT] = SKB_EXT_CHUNKSIZEOF(struct tc_skb_ext), -#endif -#if IS_ENABLED(CONFIG_MPTCP) - [SKB_EXT_MPTCP] = SKB_EXT_CHUNKSIZEOF(struct mptcp_ext), -#endif -#if IS_ENABLED(CONFIG_MCTP_FLOWS) - [SKB_EXT_MCTP] = SKB_EXT_CHUNKSIZEOF(struct mctp_flow), -#endif -#if IS_ENABLED(CONFIG_INET_PSP) - [SKB_EXT_PSP] = SKB_EXT_CHUNKSIZEOF(struct psp_skb_ext), -#endif -#if IS_ENABLED(CONFIG_CAN) - [SKB_EXT_CAN] = SKB_EXT_CHUNKSIZEOF(struct can_skb_ext), -#endif +#define X(id, type) [id] = SKB_EXT_CHUNKSIZEOF(type), + SKB_EXT_FOREACH(X) +#undef X }; -static __always_inline __no_profile unsigned int skb_ext_total_length(void) -{ - unsigned int l = SKB_EXT_CHUNKSIZEOF(struct skb_ext); - int i; - - for (i = 0; i < ARRAY_SIZE(skb_ext_type_len); i++) - l += skb_ext_type_len[i]; +struct skb_ext_layout { + u8 header[sizeof(struct skb_ext)] __aligned(SKB_EXT_ALIGN_VALUE); +#define X(id, type) type f_##id __aligned(SKB_EXT_ALIGN_VALUE); + SKB_EXT_FOREACH(X) +#undef X +}; - return l; -} +const u8 skb_ext_offset[SKB_EXT_NUM] = { +#define X(id, type) [id] = offsetof(struct skb_ext_layout, f_##id) / SKB_EXT_ALIGN_VALUE, + SKB_EXT_FOREACH(X) +#undef X +}; +EXPORT_SYMBOL(skb_ext_offset); static noinline void __init __no_profile skb_extensions_init(void) { BUILD_BUG_ON(SKB_EXT_NUM > 8); - BUILD_BUG_ON(skb_ext_total_length() > 255); + BUILD_BUG_ON(sizeof(struct skb_ext_layout) > 255 * SKB_EXT_ALIGN_VALUE); skbuff_ext_cache = kmem_cache_create("skbuff_ext_cache", - SKB_EXT_ALIGN_VALUE * skb_ext_total_length(), + sizeof(struct skb_ext_layout), 0, SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL); @@ -7083,7 +7068,7 @@ EXPORT_SYMBOL(skb_condense); #ifdef CONFIG_SKB_EXTENSIONS static void *skb_ext_get_ptr(struct skb_ext *ext, enum skb_ext_id id) { - return (void *)ext + (ext->offset[id] * SKB_EXT_ALIGN_VALUE); + return (void *)ext + (skb_ext_offset[id] * SKB_EXT_ALIGN_VALUE); } /** @@ -7100,7 +7085,7 @@ struct skb_ext *__skb_ext_alloc(gfp_t flags) struct skb_ext *new = kmem_cache_alloc(skbuff_ext_cache, flags); if (new) { - memset(new->offset, 0, sizeof(new->offset)); + new->present_extensions = 0; refcount_set(&new->refcnt, 1); } @@ -7111,6 +7096,7 @@ static struct skb_ext *skb_ext_maybe_cow(struct skb_ext *old, unsigned int old_active) { struct skb_ext *new; + int i; if (refcount_read(&old->refcnt) == 1) return old; @@ -7119,7 +7105,12 @@ static struct skb_ext *skb_ext_maybe_cow(struct skb_ext *old, if (!new) return NULL; - memcpy(new, old, old->chunks * SKB_EXT_ALIGN_VALUE); + memcpy(new, old, SKB_EXT_CHUNKSIZEOF(*old) * SKB_EXT_ALIGN_VALUE); + for (i = 0; i < SKB_EXT_NUM; i++) { + if (old->present_extensions & (1 << i)) + memcpy(skb_ext_get_ptr(new, i), skb_ext_get_ptr(old, i), + skb_ext_type_len[i] * SKB_EXT_ALIGN_VALUE); + } refcount_set(&new->refcnt, 1); #ifdef CONFIG_XFRM @@ -7156,12 +7147,8 @@ static struct skb_ext *skb_ext_maybe_cow(struct skb_ext *old, void *__skb_ext_set(struct sk_buff *skb, enum skb_ext_id id, struct skb_ext *ext) { - unsigned int newlen, newoff = SKB_EXT_CHUNKSIZEOF(*ext); - skb_ext_put(skb); - newlen = newoff + skb_ext_type_len[id]; - ext->chunks = newlen; - ext->offset[id] = newoff; + ext->present_extensions = 1 << id; skb->extensions = ext; skb->active_extensions = 1 << id; return skb_ext_get_ptr(ext, id); @@ -7184,31 +7171,23 @@ EXPORT_SYMBOL_NS_GPL(__skb_ext_set, "NETDEV_INTERNAL"); */ void *skb_ext_add(struct sk_buff *skb, enum skb_ext_id id) { - struct skb_ext *new, *old = NULL; - unsigned int newlen, newoff; + struct skb_ext *new; if (skb->active_extensions) { - old = skb->extensions; - - new = skb_ext_maybe_cow(old, skb->active_extensions); + new = skb_ext_maybe_cow(skb->extensions, + skb->active_extensions); if (!new) return NULL; if (__skb_ext_exist(new, id)) goto set_active; - - newoff = new->chunks; } else { - newoff = SKB_EXT_CHUNKSIZEOF(*new); - new = __skb_ext_alloc(GFP_ATOMIC); if (!new) return NULL; } - newlen = newoff + skb_ext_type_len[id]; - new->chunks = newlen; - new->offset[id] = newoff; + new->present_extensions |= 1 << id; set_active: skb->slow_gro = 1; skb->extensions = new;