| Message ID | 167421519986.1321434.5887198904455029318.stgit@firesoul (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,RFC] net: introduce skb_poison_list and use in kfree_skb_list | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On Fri, 20 Jan 2023 12:46:39 +0100 Jesper Dangaard Brouer wrote: > First user of skb_poison_list is in kfree_skb_list_reason, to catch bugs > earlier like introduced in commit eedade12f4cb ("net: kfree_skb_list use > kmem_cache_free_bulk"). > > In case of a bug like mentioned commit we would have seen OOPS with: > general protection fault, probably for non-canonical address 0xdead0000000000b1 > And content of one the registers e.g. R13: dead000000000041 > > In this case skb->len is at offset 112 bytes (0x70) why fault happens at > 0x41+0x70 = 0xB1 I like the idea, FWIW. I was gonna apply the RFC but looks like there was a dependency on the fix, so better if you repost and bot gets to chew on it.
diff --git a/include/linux/poison.h b/include/linux/poison.h index 2d3249eb0e62..f44da61bb88f 100644 --- a/include/linux/poison.h +++ b/include/linux/poison.h @@ -81,6 +81,9 @@ /********** net/core/page_pool.c **********/ #define PP_SIGNATURE (0x40 + POISON_POINTER_DELTA) +/********** net/core/skbuff.c **********/ +#define SKB_LIST_POISON_NEXT ((void *)(0x41 + POISON_POINTER_DELTA)) + /********** kernel/bpf/ **********/ #define BPF_PTR_POISON ((void *)(0xeB9FUL + POISON_POINTER_DELTA)) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 4c8492401a10..3b411a40a149 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -1743,6 +1743,13 @@ static inline void skb_mark_not_on_list(struct sk_buff *skb) skb->next = NULL; } +static inline void skb_poison_list(struct sk_buff *skb) +{ +#ifdef CONFIG_DEBUG_NET + skb->next = SKB_LIST_POISON_NEXT; +#endif +} + /* Iterate through singly-linked GSO fragments of an skb. */ #define skb_list_walk_safe(first, skb, next_skb) \ for ((skb) = (first), (next_skb) = (skb) ? (skb)->next : NULL; (skb); \ diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 180df58e85c7..02a1761ed0f9 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -999,8 +999,10 @@ kfree_skb_list_reason(struct sk_buff *segs, enum skb_drop_reason reason) while (segs) { struct sk_buff *next = segs->next; - if (__kfree_skb_reason(segs, reason)) + if (__kfree_skb_reason(segs, reason)) { + skb_poison_list(segs); kfree_skb_add_bulk(segs, &sa, reason); + } segs = next; }
First user of skb_poison_list is in kfree_skb_list_reason, to catch bugs earlier like introduced in commit eedade12f4cb ("net: kfree_skb_list use kmem_cache_free_bulk"). In case of a bug like mentioned commit we would have seen OOPS with: general protection fault, probably for non-canonical address 0xdead0000000000b1 And content of one the registers e.g. R13: dead000000000041 In this case skb->len is at offset 112 bytes (0x70) why fault happens at 0x41+0x70 = 0xB1 Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- include/linux/poison.h | 3 +++ include/linux/skbuff.h | 7 +++++++ net/core/skbuff.c | 4 +++- 3 files changed, 13 insertions(+), 1 deletion(-)