Re: r8169: Crash with TX segmentation offload on RTL8125

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 18 Jul 2024 13:02:27 -0700 Breno Leitao wrote:
> > > Yeah, that's an excellent catch and one that is bitten us before, too:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=20d1f2d1b024f6be199a3bedf1578a1d21592bc5
> > > 
> > > unclear what we would do in skb_shinfo() to help driver writers, rather 
> > > than rely upon code inspection to find such bugs.  
> > 
> > I wonder if we should add a "error injection" hook under DEBUG_NET
> > to force re-allocation of skbs in any helper which may cause it?  
> 
> Would you mind detailing a bit more how would see see it implemented?
> 
> Are you talking about something as the Fault-injection framework
> (CONFIG_FAULT_INJECTION) ?

Yes, I started typing the below but got distracted & uncertain about
the exact hooks and test coverage:

From ca7e88fb85f2e905b99c4c35029ea7ac8d35671c Mon Sep 17 00:00:00 2001
From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 29 May 2024 13:21:19 -0700
Subject: net: add fault injection for forcing skb reallocation

Some helpers (pskb_may_pull()

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/skbuff.h | 11 +++++++++++
 net/core/skbuff.c      | 27 +++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 9c29bdd5596d..dcc488875374 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2665,6 +2665,14 @@ static inline void skb_assert_len(struct sk_buff *skb)
 #endif /* CONFIG_DEBUG_NET */
 }
 
+#if defined(CONFIG_DEBUG_NET) && defined(CONFIG_FAULT_INJECTION_DEBUG_FS)
+void skb_might_realloc(struct sk_buff *skb);
+#else
+static inline void skb_might_realloc(struct sk_buff *skb)
+{
+}
+#endif
+
 /*
  *	Add data to an sk_buff
  */
@@ -2765,6 +2773,7 @@ static inline enum skb_drop_reason
 pskb_may_pull_reason(struct sk_buff *skb, unsigned int len)
 {
 	DEBUG_NET_WARN_ON_ONCE(len > INT_MAX);
+	skb_might_realloc(skb);
 
 	if (likely(len <= skb_headlen(skb)))
 		return SKB_NOT_DROPPED_YET;
@@ -3194,6 +3203,7 @@ static inline int __pskb_trim(struct sk_buff *skb, unsigned int len)
 
 static inline int pskb_trim(struct sk_buff *skb, unsigned int len)
 {
+	skb_might_realloc(skb);
 	return (len < skb->len) ? __pskb_trim(skb, len) : 0;
 }
 
@@ -3900,6 +3910,7 @@ int pskb_trim_rcsum_slow(struct sk_buff *skb, unsigned int len);
 
 static inline int pskb_trim_rcsum(struct sk_buff *skb, unsigned int len)
 {
+	skb_might_realloc(skb);
 	if (likely(len >= skb->len))
 		return 0;
 	return pskb_trim_rcsum_slow(skb, len);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 83f8cd8aa2d1..a9f4275bb783 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -58,6 +58,7 @@
 #include <linux/init.h>
 #include <linux/scatterlist.h>
 #include <linux/errqueue.h>
+#include <linux/fault-inject.h>
 #include <linux/prefetch.h>
 #include <linux/bitfield.h>
 #include <linux/if_vlan.h>
@@ -2222,6 +2223,32 @@ struct sk_buff *__pskb_copy_fclone(struct sk_buff *skb, int headroom,
 }
 EXPORT_SYMBOL(__pskb_copy_fclone);
 
+#if defined(CONFIG_DEBUG_NET) && defined(CONFIG_FAULT_INJECTION_DEBUG_FS)
+static DECLARE_FAULT_ATTR(skb_force_realloc);
+
+void skb_might_realloc(struct sk_buff *skb)
+{
+	if (should_fail(&skb_force_realloc, 1))
+		pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+}
+EXPORT_SYMBOL(skb_might_realloc);
+
+static int __init skb_force_realloc_setup(char *str)
+{
+	return setup_fault_attr(&skb_force_realloc, str);
+}
+__setup("skb_force_realloc=", skb_force_realloc_setup);
+
+static int __init skb_force_realloc_debugfs(void)
+{
+	fault_create_debugfs_attr("skb_force_realloc", NULL,
+				  &skb_force_realloc);
+	return 0;
+}
+
+late_initcall(skb_force_realloc_debugfs);
+#endif
+
 /**
  *	pskb_expand_head - reallocate header of &sk_buff
  *	@skb: buffer to reallocate
-- 
2.45.2