* [PATCH net-next] net: Implement fault injection forcing skb reallocation
@ 2024-10-02 11:32 Breno Leitao
2024-10-02 14:23 ` kernel test robot
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Breno Leitao @ 2024-10-02 11:32 UTC (permalink / raw)
To: kuba, davem, edumazet, pabeni, Akinobu Mita, Jonathan Corbet
Cc: horms, netdev, linux-kernel, Mina Almasry, Pavel Begunkov,
Willem de Bruijn, Alexander Lobakin, open list:DOCUMENTATION
Introduce a fault injection mechanism to force skb reallocation. The
primary goal is to catch bugs related to pointer invalidation after
potential skb reallocation.
The fault injection mechanism aims to identify scenarios where callers
retain pointers to various headers in the skb but fail to reload these
pointers after calling a function that may reallocate the data. This
type of bug can lead to memory corruption or crashes if the old,
now-invalid pointers are used.
By forcing reallocation through fault injection, we can stress-test code
paths and ensure proper pointer management after potential skb
reallocations.
Add a hook for fault injection in the following functions:
* pskb_trim_rcsum()
* pskb_may_pull_reason()
* pskb_trim()
As the other fault injection mechanism, protect it under a debug Kconfig
called CONFIG_FAIL_SKB_FORCE_REALLOC.
This patch was *heavily* inspired by Jakub's proposal from:
https://lore.kernel.org/all/20240719174140.47a868e6@kernel.org/
CC: Akinobu Mita <akinobu.mita@gmail.com>
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
.../fault-injection/fault-injection.rst | 18 ++++++++++
include/linux/skbuff.h | 9 +++++
net/Kconfig.debug | 11 +++++++
net/core/Makefile | 1 +
net/core/skb_fault_injection.c | 33 +++++++++++++++++++
5 files changed, 72 insertions(+)
create mode 100644 net/core/skb_fault_injection.c
diff --git a/Documentation/fault-injection/fault-injection.rst b/Documentation/fault-injection/fault-injection.rst
index 70380a2a01b4..2fc71330c761 100644
--- a/Documentation/fault-injection/fault-injection.rst
+++ b/Documentation/fault-injection/fault-injection.rst
@@ -45,6 +45,23 @@ Available fault injection capabilities
ALLOW_ERROR_INJECTION() macro, by setting debugfs entries
under /sys/kernel/debug/fail_function. No boot option supported.
+- fail_net_force_skb_realloc
+
+ inject skb (socket buffer) reallocation events into the network path. The
+ primary goal is to identify and prevent issues related to pointer
+ mismanagement in the network subsystem. By forcing skb reallocation at
+ strategic points, this feature creates scenarios where existing pointers to
+ skb headers become invalid.
+
+ When the fault is injected and the reallocation is triggered, these pointers
+ no longer reference valid memory locations. This deliberate invalidation
+ helps expose code paths where proper pointer updating is neglected after a
+ reallocation event.
+
+ By creating these controlled fault scenarios, the system can catch instances
+ where stale pointers are used, potentially leading to memory corruption or
+ system instability.
+
- NVMe fault injection
inject NVMe status code and retry flag on devices permitted by setting
@@ -219,6 +236,7 @@ use the boot option::
fail_usercopy=
fail_make_request=
fail_futex=
+ fail_net_force_skb_realloc=
mmc_core.fail_request=<interval>,<probability>,<space>,<times>
proc entries
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 39f1d16f3628..d9ee756a64fc 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2681,6 +2681,12 @@ static inline void skb_assert_len(struct sk_buff *skb)
#endif /* CONFIG_DEBUG_NET */
}
+#if defined(CONFIG_FAIL_SKB_FORCE_REALLOC)
+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
*/
@@ -2781,6 +2787,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;
@@ -3210,6 +3217,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;
}
@@ -3964,6 +3972,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/Kconfig.debug b/net/Kconfig.debug
index 5e3fffe707dd..f61935e028bd 100644
--- a/net/Kconfig.debug
+++ b/net/Kconfig.debug
@@ -24,3 +24,14 @@ config DEBUG_NET
help
Enable extra sanity checks in networking.
This is mostly used by fuzzers, but is safe to select.
+
+config FAIL_SKB_FORCE_REALLOC
+ bool "Fault-injection capability forcing skb to reallocate"
+ depends on FAULT_INJECTION && DEBUG_NET
+ default n
+ help
+ Provide fault-injection capability that forces the skb to be
+ reallocated, caughting possible invalid pointers to the skb.
+
+ For more information, check
+ Documentation/dev-tools/fault-injection/fault-injection.rst
diff --git a/net/core/Makefile b/net/core/Makefile
index c3ebbaf9c81e..02658807242b 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -45,3 +45,4 @@ obj-$(CONFIG_BPF_SYSCALL) += bpf_sk_storage.o
obj-$(CONFIG_OF) += of_net.o
obj-$(CONFIG_NET_TEST) += net_test.o
obj-$(CONFIG_NET_DEVMEM) += devmem.o
+obj-$(CONFIG_FAIL_SKB_FORCE_REALLOC) += skb_fault_injection.o
diff --git a/net/core/skb_fault_injection.c b/net/core/skb_fault_injection.c
new file mode 100644
index 000000000000..ccdc0f9c41be
--- /dev/null
+++ b/net/core/skb_fault_injection.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/fault-inject.h>
+#include <linux/skbuff.h>
+
+static DECLARE_FAULT_ATTR(fail_net_force_skb_realloc);
+
+void skb_might_realloc(struct sk_buff *skb)
+{
+ if (should_fail(&fail_net_force_skb_realloc, 1))
+ pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+}
+EXPORT_SYMBOL(skb_might_realloc);
+
+static int __init fail_net_force_skb_realloc_setup(char *str)
+{
+ return setup_fault_attr(&fail_net_force_skb_realloc, str);
+}
+__setup("fail_net_force_skb_realloc=", fail_net_force_skb_realloc_setup);
+
+static int __init fail_net_force_skb_realloc_debugfs(void)
+{
+ struct dentry *dir;
+
+ dir = fault_create_debugfs_attr("fail_net_force_skb_realloc", NULL,
+ &fail_net_force_skb_realloc);
+ if (IS_ERR(dir))
+ return PTR_ERR(dir);
+
+ return 0;
+}
+
+late_initcall(fail_net_force_skb_realloc_debugfs);
--
2.43.5
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH net-next] net: Implement fault injection forcing skb reallocation
2024-10-02 11:32 [PATCH net-next] net: Implement fault injection forcing skb reallocation Breno Leitao
@ 2024-10-02 14:23 ` kernel test robot
2024-10-02 15:33 ` Breno Leitao
2024-10-02 15:25 ` Kuniyuki Iwashima
2024-10-05 4:38 ` Akinobu Mita
2 siblings, 1 reply; 11+ messages in thread
From: kernel test robot @ 2024-10-02 14:23 UTC (permalink / raw)
To: Breno Leitao, kuba, davem, edumazet, pabeni, Akinobu Mita,
Jonathan Corbet
Cc: oe-kbuild-all, horms, netdev, linux-kernel, Mina Almasry,
Pavel Begunkov, Willem de Bruijn, Alexander Lobakin,
open list:DOCUMENTATION
Hi Breno,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Breno-Leitao/net-Implement-fault-injection-forcing-skb-reallocation/20241002-193852
base: net-next/main
patch link: https://lore.kernel.org/r/20241002113316.2527669-1-leitao%40debian.org
patch subject: [PATCH net-next] net: Implement fault injection forcing skb reallocation
reproduce: (https://download.01.org/0day-ci/archive/20241002/202410022209.2TB3siPB-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410022209.2TB3siPB-lkp@intel.com/
All warnings (new ones prefixed by >>):
Warning: Documentation/hwmon/g762.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/g762.txt
Warning: Documentation/userspace-api/netlink/index.rst references a file that doesn't exist: Documentation/networking/netlink_spec/index.rst
Warning: Documentation/userspace-api/netlink/specs.rst references a file that doesn't exist: Documentation/networking/netlink_spec/index.rst
Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/reserved-memory/qcom
Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>> Warning: net/Kconfig.debug references a file that doesn't exist: Documentation/dev-tools/fault-injection/fault-injection.rst
Using alabaster theme
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Implement fault injection forcing skb reallocation
2024-10-02 14:23 ` kernel test robot
@ 2024-10-02 15:33 ` Breno Leitao
0 siblings, 0 replies; 11+ messages in thread
From: Breno Leitao @ 2024-10-02 15:33 UTC (permalink / raw)
To: kernel test robot
Cc: kuba, davem, edumazet, pabeni, Akinobu Mita, Jonathan Corbet,
oe-kbuild-all, horms, netdev, linux-kernel, Mina Almasry,
Pavel Begunkov, Willem de Bruijn, Alexander Lobakin,
open list:DOCUMENTATION
On Wed, Oct 02, 2024 at 10:23:25PM +0800, kernel test robot wrote:
> Hi Breno,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on net-next/main]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Breno-Leitao/net-Implement-fault-injection-forcing-skb-reallocation/20241002-193852
> base: net-next/main
> patch link: https://lore.kernel.org/r/20241002113316.2527669-1-leitao%40debian.org
> patch subject: [PATCH net-next] net: Implement fault injection forcing skb reallocation
> reproduce: (https://download.01.org/0day-ci/archive/20241002/202410022209.2TB3siPB-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202410022209.2TB3siPB-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
> Warning: Documentation/hwmon/g762.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/g762.txt
> Warning: Documentation/userspace-api/netlink/index.rst references a file that doesn't exist: Documentation/networking/netlink_spec/index.rst
> Warning: Documentation/userspace-api/netlink/specs.rst references a file that doesn't exist: Documentation/networking/netlink_spec/index.rst
> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/reserved-memory/qcom
> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
> >> Warning: net/Kconfig.debug references a file that doesn't exist: Documentation/dev-tools/fault-injection/fault-injection.rst
For those interested, this is not an issue, since the fault-injection
documentation is changing to dev-tools, and I am referencing the new
localtion already.
https://lore.kernel.org/all/87ttethota.fsf@trenco.lwn.net/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Implement fault injection forcing skb reallocation
2024-10-02 11:32 [PATCH net-next] net: Implement fault injection forcing skb reallocation Breno Leitao
2024-10-02 14:23 ` kernel test robot
@ 2024-10-02 15:25 ` Kuniyuki Iwashima
2024-10-07 16:19 ` Breno Leitao
2024-10-05 4:38 ` Akinobu Mita
2 siblings, 1 reply; 11+ messages in thread
From: Kuniyuki Iwashima @ 2024-10-02 15:25 UTC (permalink / raw)
To: leitao
Cc: akinobu.mita, aleksander.lobakin, almasrymina, asml.silence,
corbet, davem, edumazet, horms, kuba, linux-doc, linux-kernel,
netdev, pabeni, willemb, kuniyu
From: Breno Leitao <leitao@debian.org>
Date: Wed, 2 Oct 2024 04:32:54 -0700
> diff --git a/net/Kconfig.debug b/net/Kconfig.debug
> index 5e3fffe707dd..f61935e028bd 100644
> --- a/net/Kconfig.debug
> +++ b/net/Kconfig.debug
This config is networking-specific, but I think lib/Kconfig.debug would be
a better fit as other fault injection configs are placed there together.
Now we need to enable fault injection first and go back to the net-specific
items in menuconfig.
> @@ -24,3 +24,14 @@ config DEBUG_NET
> help
> Enable extra sanity checks in networking.
> This is mostly used by fuzzers, but is safe to select.
> +
> +config FAIL_SKB_FORCE_REALLOC
> + bool "Fault-injection capability forcing skb to reallocate"
> + depends on FAULT_INJECTION && DEBUG_NET
> + default n
> + help
> + Provide fault-injection capability that forces the skb to be
> + reallocated, caughting possible invalid pointers to the skb.
> +
> + For more information, check
> + Documentation/dev-tools/fault-injection/fault-injection.rst
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Implement fault injection forcing skb reallocation
2024-10-02 15:25 ` Kuniyuki Iwashima
@ 2024-10-07 16:19 ` Breno Leitao
0 siblings, 0 replies; 11+ messages in thread
From: Breno Leitao @ 2024-10-07 16:19 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: akinobu.mita, aleksander.lobakin, almasrymina, asml.silence,
corbet, davem, edumazet, horms, kuba, linux-doc, linux-kernel,
netdev, pabeni, willemb
On Wed, Oct 02, 2024 at 08:25:40AM -0700, Kuniyuki Iwashima wrote:
> From: Breno Leitao <leitao@debian.org>
> Date: Wed, 2 Oct 2024 04:32:54 -0700
> > diff --git a/net/Kconfig.debug b/net/Kconfig.debug
> > index 5e3fffe707dd..f61935e028bd 100644
> > --- a/net/Kconfig.debug
> > +++ b/net/Kconfig.debug
>
> This config is networking-specific, but I think lib/Kconfig.debug would be
> a better fit as other fault injection configs are placed there together.
>
> Now we need to enable fault injection first and go back to the net-specific
> items in menuconfig.
Makes sense. Let me move it to lib/Kconfig.debug.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Implement fault injection forcing skb reallocation
2024-10-02 11:32 [PATCH net-next] net: Implement fault injection forcing skb reallocation Breno Leitao
2024-10-02 14:23 ` kernel test robot
2024-10-02 15:25 ` Kuniyuki Iwashima
@ 2024-10-05 4:38 ` Akinobu Mita
2024-10-07 16:20 ` Breno Leitao
2 siblings, 1 reply; 11+ messages in thread
From: Akinobu Mita @ 2024-10-05 4:38 UTC (permalink / raw)
To: Breno Leitao
Cc: kuba, davem, edumazet, pabeni, Jonathan Corbet, horms, netdev,
linux-kernel, Mina Almasry, Pavel Begunkov, Willem de Bruijn,
Alexander Lobakin, open list:DOCUMENTATION
2024年10月2日(水) 20:37 Breno Leitao <leitao@debian.org>:
>
> Introduce a fault injection mechanism to force skb reallocation. The
> primary goal is to catch bugs related to pointer invalidation after
> potential skb reallocation.
>
> The fault injection mechanism aims to identify scenarios where callers
> retain pointers to various headers in the skb but fail to reload these
> pointers after calling a function that may reallocate the data. This
> type of bug can lead to memory corruption or crashes if the old,
> now-invalid pointers are used.
>
> By forcing reallocation through fault injection, we can stress-test code
> paths and ensure proper pointer management after potential skb
> reallocations.
>
> Add a hook for fault injection in the following functions:
>
> * pskb_trim_rcsum()
> * pskb_may_pull_reason()
> * pskb_trim()
>
> As the other fault injection mechanism, protect it under a debug Kconfig
> called CONFIG_FAIL_SKB_FORCE_REALLOC.
>
> This patch was *heavily* inspired by Jakub's proposal from:
> https://lore.kernel.org/all/20240719174140.47a868e6@kernel.org/
>
> CC: Akinobu Mita <akinobu.mita@gmail.com>
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Breno Leitao <leitao@debian.org>
This new addition seems sensible. It might be more useful to have a filter
that allows you to specify things like protocol family.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Implement fault injection forcing skb reallocation
2024-10-05 4:38 ` Akinobu Mita
@ 2024-10-07 16:20 ` Breno Leitao
2024-10-07 16:48 ` Pavel Begunkov
0 siblings, 1 reply; 11+ messages in thread
From: Breno Leitao @ 2024-10-07 16:20 UTC (permalink / raw)
To: Akinobu Mita
Cc: kuba, davem, edumazet, pabeni, Jonathan Corbet, horms, netdev,
linux-kernel, Mina Almasry, Pavel Begunkov, Willem de Bruijn,
Alexander Lobakin, open list:DOCUMENTATION
On Sat, Oct 05, 2024 at 01:38:59PM +0900, Akinobu Mita wrote:
> 2024年10月2日(水) 20:37 Breno Leitao <leitao@debian.org>:
> >
> > Introduce a fault injection mechanism to force skb reallocation. The
> > primary goal is to catch bugs related to pointer invalidation after
> > potential skb reallocation.
> >
> > The fault injection mechanism aims to identify scenarios where callers
> > retain pointers to various headers in the skb but fail to reload these
> > pointers after calling a function that may reallocate the data. This
> > type of bug can lead to memory corruption or crashes if the old,
> > now-invalid pointers are used.
> >
> > By forcing reallocation through fault injection, we can stress-test code
> > paths and ensure proper pointer management after potential skb
> > reallocations.
> >
> > Add a hook for fault injection in the following functions:
> >
> > * pskb_trim_rcsum()
> > * pskb_may_pull_reason()
> > * pskb_trim()
> >
> > As the other fault injection mechanism, protect it under a debug Kconfig
> > called CONFIG_FAIL_SKB_FORCE_REALLOC.
> >
> > This patch was *heavily* inspired by Jakub's proposal from:
> > https://lore.kernel.org/all/20240719174140.47a868e6@kernel.org/
> >
> > CC: Akinobu Mita <akinobu.mita@gmail.com>
> > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > Signed-off-by: Breno Leitao <leitao@debian.org>
>
> This new addition seems sensible. It might be more useful to have a filter
> that allows you to specify things like protocol family.
I think it might make more sense to be network interface specific. For
instance, only fault inject in interface `ethx`.
Let me spend some time and have this done.
Thanks for the feedback.
--breno
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Implement fault injection forcing skb reallocation
2024-10-07 16:20 ` Breno Leitao
@ 2024-10-07 16:48 ` Pavel Begunkov
2024-10-07 17:09 ` Breno Leitao
0 siblings, 1 reply; 11+ messages in thread
From: Pavel Begunkov @ 2024-10-07 16:48 UTC (permalink / raw)
To: Breno Leitao, Akinobu Mita
Cc: kuba, davem, edumazet, pabeni, Jonathan Corbet, horms, netdev,
linux-kernel, Mina Almasry, Willem de Bruijn, Alexander Lobakin,
open list:DOCUMENTATION
On 10/7/24 17:20, Breno Leitao wrote:
> On Sat, Oct 05, 2024 at 01:38:59PM +0900, Akinobu Mita wrote:
>> 2024年10月2日(水) 20:37 Breno Leitao <leitao@debian.org>:
>>>
>>> Introduce a fault injection mechanism to force skb reallocation. The
>>> primary goal is to catch bugs related to pointer invalidation after
>>> potential skb reallocation.
>>>
>>> The fault injection mechanism aims to identify scenarios where callers
>>> retain pointers to various headers in the skb but fail to reload these
>>> pointers after calling a function that may reallocate the data. This
>>> type of bug can lead to memory corruption or crashes if the old,
>>> now-invalid pointers are used.
>>>
>>> By forcing reallocation through fault injection, we can stress-test code
>>> paths and ensure proper pointer management after potential skb
>>> reallocations.
>>>
>>> Add a hook for fault injection in the following functions:
>>>
>>> * pskb_trim_rcsum()
>>> * pskb_may_pull_reason()
>>> * pskb_trim()
>>>
>>> As the other fault injection mechanism, protect it under a debug Kconfig
>>> called CONFIG_FAIL_SKB_FORCE_REALLOC.
>>>
>>> This patch was *heavily* inspired by Jakub's proposal from:
>>> https://lore.kernel.org/all/20240719174140.47a868e6@kernel.org/
>>>
>>> CC: Akinobu Mita <akinobu.mita@gmail.com>
>>> Suggested-by: Jakub Kicinski <kuba@kernel.org>
>>> Signed-off-by: Breno Leitao <leitao@debian.org>
>>
>> This new addition seems sensible. It might be more useful to have a filter
>> that allows you to specify things like protocol family.
>
> I think it might make more sense to be network interface specific. For
> instance, only fault inject in interface `ethx`.
Wasn't there some error injection infra that allows to optionally
run bpf? That would cover the filtering problem. ALLOW_ERROR_INJECTION,
maybe?
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Implement fault injection forcing skb reallocation
2024-10-07 16:48 ` Pavel Begunkov
@ 2024-10-07 17:09 ` Breno Leitao
2024-10-07 18:00 ` Pavel Begunkov
0 siblings, 1 reply; 11+ messages in thread
From: Breno Leitao @ 2024-10-07 17:09 UTC (permalink / raw)
To: Pavel Begunkov
Cc: Akinobu Mita, kuba, davem, edumazet, pabeni, Jonathan Corbet,
horms, netdev, linux-kernel, Mina Almasry, Willem de Bruijn,
Alexander Lobakin, open list:DOCUMENTATION
Hello Pavel,
On Mon, Oct 07, 2024 at 05:48:39PM +0100, Pavel Begunkov wrote:
> On 10/7/24 17:20, Breno Leitao wrote:
> > On Sat, Oct 05, 2024 at 01:38:59PM +0900, Akinobu Mita wrote:
> > > 2024年10月2日(水) 20:37 Breno Leitao <leitao@debian.org>:
> > > >
> > > > Introduce a fault injection mechanism to force skb reallocation. The
> > > > primary goal is to catch bugs related to pointer invalidation after
> > > > potential skb reallocation.
> > > >
> > > > The fault injection mechanism aims to identify scenarios where callers
> > > > retain pointers to various headers in the skb but fail to reload these
> > > > pointers after calling a function that may reallocate the data. This
> > > > type of bug can lead to memory corruption or crashes if the old,
> > > > now-invalid pointers are used.
> > > >
> > > > By forcing reallocation through fault injection, we can stress-test code
> > > > paths and ensure proper pointer management after potential skb
> > > > reallocations.
> > > >
> > > > Add a hook for fault injection in the following functions:
> > > >
> > > > * pskb_trim_rcsum()
> > > > * pskb_may_pull_reason()
> > > > * pskb_trim()
> > > >
> > > > As the other fault injection mechanism, protect it under a debug Kconfig
> > > > called CONFIG_FAIL_SKB_FORCE_REALLOC.
> > > >
> > > > This patch was *heavily* inspired by Jakub's proposal from:
> > > > https://lore.kernel.org/all/20240719174140.47a868e6@kernel.org/
> > > >
> > > > CC: Akinobu Mita <akinobu.mita@gmail.com>
> > > > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > > > Signed-off-by: Breno Leitao <leitao@debian.org>
> > >
> > > This new addition seems sensible. It might be more useful to have a filter
> > > that allows you to specify things like protocol family.
> >
> > I think it might make more sense to be network interface specific. For
> > instance, only fault inject in interface `ethx`.
>
> Wasn't there some error injection infra that allows to optionally
> run bpf? That would cover the filtering problem. ALLOW_ERROR_INJECTION,
> maybe?
Isn't ALLOW_ERROR_INJECTION focused on specifying which function could
be faulted? I.e, you can mark that function as prone for fail injection?
In my the case I have in mind, I want to pass the interface that it
would have the error injected. For instance, only inject errors in
interface eth1. In this case, I am not sure ALLOW_ERROR_INJECTION will
help.
Thanks
--breno
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Implement fault injection forcing skb reallocation
2024-10-07 17:09 ` Breno Leitao
@ 2024-10-07 18:00 ` Pavel Begunkov
2024-10-08 11:09 ` Breno Leitao
0 siblings, 1 reply; 11+ messages in thread
From: Pavel Begunkov @ 2024-10-07 18:00 UTC (permalink / raw)
To: Breno Leitao
Cc: Akinobu Mita, kuba, davem, edumazet, pabeni, Jonathan Corbet,
horms, netdev, linux-kernel, Mina Almasry, Willem de Bruijn,
Alexander Lobakin, open list:DOCUMENTATION
On 10/7/24 18:09, Breno Leitao wrote:
> Hello Pavel,
>
> On Mon, Oct 07, 2024 at 05:48:39PM +0100, Pavel Begunkov wrote:
>> On 10/7/24 17:20, Breno Leitao wrote:
>>> On Sat, Oct 05, 2024 at 01:38:59PM +0900, Akinobu Mita wrote:
>>>> 2024年10月2日(水) 20:37 Breno Leitao <leitao@debian.org>:
>>>>>
>>>>> Introduce a fault injection mechanism to force skb reallocation. The
>>>>> primary goal is to catch bugs related to pointer invalidation after
>>>>> potential skb reallocation.
>>>>>
>>>>> The fault injection mechanism aims to identify scenarios where callers
>>>>> retain pointers to various headers in the skb but fail to reload these
>>>>> pointers after calling a function that may reallocate the data. This
>>>>> type of bug can lead to memory corruption or crashes if the old,
>>>>> now-invalid pointers are used.
>>>>>
>>>>> By forcing reallocation through fault injection, we can stress-test code
>>>>> paths and ensure proper pointer management after potential skb
>>>>> reallocations.
>>>>>
>>>>> Add a hook for fault injection in the following functions:
>>>>>
>>>>> * pskb_trim_rcsum()
>>>>> * pskb_may_pull_reason()
>>>>> * pskb_trim()
>>>>>
>>>>> As the other fault injection mechanism, protect it under a debug Kconfig
>>>>> called CONFIG_FAIL_SKB_FORCE_REALLOC.
>>>>>
>>>>> This patch was *heavily* inspired by Jakub's proposal from:
>>>>> https://lore.kernel.org/all/20240719174140.47a868e6@kernel.org/
>>>>>
>>>>> CC: Akinobu Mita <akinobu.mita@gmail.com>
>>>>> Suggested-by: Jakub Kicinski <kuba@kernel.org>
>>>>> Signed-off-by: Breno Leitao <leitao@debian.org>
>>>>
>>>> This new addition seems sensible. It might be more useful to have a filter
>>>> that allows you to specify things like protocol family.
>>>
>>> I think it might make more sense to be network interface specific. For
>>> instance, only fault inject in interface `ethx`.
>>
>> Wasn't there some error injection infra that allows to optionally
>> run bpf? That would cover the filtering problem. ALLOW_ERROR_INJECTION,
>> maybe?
>
> Isn't ALLOW_ERROR_INJECTION focused on specifying which function could
> be faulted? I.e, you can mark that function as prone for fail injection?
>
> In my the case I have in mind, I want to pass the interface that it
> would have the error injected. For instance, only inject errors in
> interface eth1. In this case, I am not sure ALLOW_ERROR_INJECTION will
> help.
I've never looked into it and might be wrong, but I view
ALLOW_ERROR_INJECTION'ed functions as a yes/no (err code) switch on
steroids enabling debug code but not doing actual failing. E.g.
if (should_fail_bio(bio)) {
bio->bi_status = status;
bio_endio(bio);
return;
}
Looking at your patch, in this case it'd be not failing a request but
pskb_expand_head(). Not exactly a perfect match as there are no "errors"
here, but if not usable directly maybe it's trivial to adapt.
That's assuming it supports bpf and lets it to specify the result of
the function, from where bpf can dig into the skb argument and do
custom filtering.
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH net-next] net: Implement fault injection forcing skb reallocation
2024-10-07 18:00 ` Pavel Begunkov
@ 2024-10-08 11:09 ` Breno Leitao
0 siblings, 0 replies; 11+ messages in thread
From: Breno Leitao @ 2024-10-08 11:09 UTC (permalink / raw)
To: Pavel Begunkov
Cc: Akinobu Mita, kuba, davem, edumazet, pabeni, Jonathan Corbet,
horms, netdev, linux-kernel, Mina Almasry, Willem de Bruijn,
Alexander Lobakin, open list:DOCUMENTATION
On Mon, Oct 07, 2024 at 07:00:28PM +0100, Pavel Begunkov wrote:
> On 10/7/24 18:09, Breno Leitao wrote:
> > Hello Pavel,
> >
> > On Mon, Oct 07, 2024 at 05:48:39PM +0100, Pavel Begunkov wrote:
> > > On 10/7/24 17:20, Breno Leitao wrote:
> > > > On Sat, Oct 05, 2024 at 01:38:59PM +0900, Akinobu Mita wrote:
> > > > > 2024年10月2日(水) 20:37 Breno Leitao <leitao@debian.org>:
> > > > > >
> > > > > > Introduce a fault injection mechanism to force skb reallocation. The
> > > > > > primary goal is to catch bugs related to pointer invalidation after
> > > > > > potential skb reallocation.
> > > > > >
> > > > > > The fault injection mechanism aims to identify scenarios where callers
> > > > > > retain pointers to various headers in the skb but fail to reload these
> > > > > > pointers after calling a function that may reallocate the data. This
> > > > > > type of bug can lead to memory corruption or crashes if the old,
> > > > > > now-invalid pointers are used.
> > > > > >
> > > > > > By forcing reallocation through fault injection, we can stress-test code
> > > > > > paths and ensure proper pointer management after potential skb
> > > > > > reallocations.
> > > > > >
> > > > > > Add a hook for fault injection in the following functions:
> > > > > >
> > > > > > * pskb_trim_rcsum()
> > > > > > * pskb_may_pull_reason()
> > > > > > * pskb_trim()
> > > > > >
> > > > > > As the other fault injection mechanism, protect it under a debug Kconfig
> > > > > > called CONFIG_FAIL_SKB_FORCE_REALLOC.
> > > > > >
> > > > > > This patch was *heavily* inspired by Jakub's proposal from:
> > > > > > https://lore.kernel.org/all/20240719174140.47a868e6@kernel.org/
> > > > > >
> > > > > > CC: Akinobu Mita <akinobu.mita@gmail.com>
> > > > > > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > > > > > Signed-off-by: Breno Leitao <leitao@debian.org>
> > > > >
> > > > > This new addition seems sensible. It might be more useful to have a filter
> > > > > that allows you to specify things like protocol family.
> > > >
> > > > I think it might make more sense to be network interface specific. For
> > > > instance, only fault inject in interface `ethx`.
> > >
> > > Wasn't there some error injection infra that allows to optionally
> > > run bpf? That would cover the filtering problem. ALLOW_ERROR_INJECTION,
> > > maybe?
> >
> > Isn't ALLOW_ERROR_INJECTION focused on specifying which function could
> > be faulted? I.e, you can mark that function as prone for fail injection?
> >
> > In my the case I have in mind, I want to pass the interface that it
> > would have the error injected. For instance, only inject errors in
> > interface eth1. In this case, I am not sure ALLOW_ERROR_INJECTION will
> > help.
>
> I've never looked into it and might be wrong, but I view
> ALLOW_ERROR_INJECTION'ed functions as a yes/no (err code) switch on
> steroids enabling debug code but not doing actual failing. E.g.
Right. I think there are two things here:
1) A function that could fail depending on your failure injection
request. For instance, you can force ALLOW_ERROR_INJECTION functions to
fail in certain conditions. See the documentation:
/*
* Whitelist generating macro. Specify functions which can be error-injectable
* using this macro. (ALLOW_ERROR_INJECTION)
For instance, you can mark any random function as part error injectable.
This is not the case for the problem this patch is solving.
2) There are helpers that will query the fault injection mechanism to
decide if a given function should fail or not. This is exactly what
should_fail_bio() does. These are helpers that will eventually call
should_fail().
in my patch, this is done by skb_might_realloc() function, where it
calls should_fail(), and if the fault injection mechanism says it is
time to "fail", then it does (in this patch context, failure means
forcing the skb to be reallocated).
That said, it is unclear to me how ALLOW_ERROR_INJECTION could help to
solve the skb reallocation mechanism.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-10-08 11:09 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-02 11:32 [PATCH net-next] net: Implement fault injection forcing skb reallocation Breno Leitao
2024-10-02 14:23 ` kernel test robot
2024-10-02 15:33 ` Breno Leitao
2024-10-02 15:25 ` Kuniyuki Iwashima
2024-10-07 16:19 ` Breno Leitao
2024-10-05 4:38 ` Akinobu Mita
2024-10-07 16:20 ` Breno Leitao
2024-10-07 16:48 ` Pavel Begunkov
2024-10-07 17:09 ` Breno Leitao
2024-10-07 18:00 ` Pavel Begunkov
2024-10-08 11:09 ` Breno Leitao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).