Open vSwitch system test suite is broken due to inability to
load/unload netfilter modules. kworker thread is getting trapped
in the infinite loop while running a net cleanup inside the
nf_conntrack_cleanup_net_list, because deferred skbuffs are still
holding nfct references and not being freed by their CPU cores.
In general, the idea that we will have an rx interrupt on every
CPU core at some point in a near future doesn't seem correct.
Devices are getting created and destroyed, interrupts are getting
re-scheduled, CPUs are going online and offline dynamically.
Any of these events may leave packets stuck in defer list for a
long time. It might be OK, if they are just a piece of memory,
but we can't afford them holding references to any other resources.
In case of OVS, nfct reference keeps the kernel thread in busy loop
while holding a 'pernet_ops_rwsem' semaphore. That blocks the
later modprobe request from user space:
# ps
299 root R 99.3 200:25.89 kworker/u96:4+
# journalctl
INFO: task modprobe:11787 blocked for more than 1228 seconds.
Not tainted 5.19.0-rc2 #8
task:modprobe state:D
Call Trace:
<TASK>
__schedule+0x8aa/0x21d0
schedule+0xcc/0x200
rwsem_down_write_slowpath+0x8e4/0x1580
down_write+0xfc/0x140
register_pernet_subsys+0x15/0x40
nf_nat_init+0xb6/0x1000 [nf_nat]
do_one_initcall+0xbb/0x410
do_init_module+0x1b4/0x640
load_module+0x4c1b/0x58d0
__do_sys_init_module+0x1d7/0x220
do_syscall_64+0x3a/0x80
entry_SYSCALL_64_after_hwframe+0x46/0xb0
At this point OVS testsuite is unresponsive and never recover,
because these skbuffs are never freed.
Solution is to make sure no external references attached to skb
before pushing it to the defer list. Using skb_release_head_state()
for that purpose. The function modified to be re-enterable, as it
will be called again during the defer list flush.
Another approach that can fix the OVS use-case, is to kick all
cores while waiting for references to be released during the net
cleanup. But that sounds more like a workaround for a current
issue rather than a proper solution and will not cover possible
issues in other parts of the code.
Additionally checking for skb_zcopy() while deferring. This might
not be necessary, as I'm not sure if we can actually have zero copy
packets on this path, but seems worth having for completeness as we
should never defer such packets regardless.
CC: Eric Dumazet <[email protected]>
Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
Signed-off-by: Ilya Maximets <[email protected]>
---
net/core/skbuff.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5b3559cb1d82..5660250e795f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -726,11 +726,9 @@ void skb_release_head_state(struct sk_buff *skb)
skb_dst_drop(skb);
if (skb->destructor) {
WARN_ON(in_hardirq());
- skb->destructor(skb);
+ skb_orphan(skb);
}
-#if IS_ENABLED(CONFIG_NF_CONNTRACK)
- nf_conntrack_put(skb_nfct(skb));
-#endif
+ nf_reset_ct(skb);
skb_ext_put(skb);
}
@@ -6502,7 +6500,8 @@ void skb_attempt_defer_free(struct sk_buff *skb)
if (WARN_ON_ONCE(cpu >= nr_cpu_ids) ||
!cpu_online(cpu) ||
- cpu == raw_smp_processor_id()) {
+ cpu == raw_smp_processor_id() ||
+ skb_zcopy(skb)) {
nodefer: __kfree_skb(skb);
return;
}
@@ -6512,6 +6511,13 @@ nodefer: __kfree_skb(skb);
if (READ_ONCE(sd->defer_count) >= defer_max)
goto nodefer;
+ /* skb can contain all kinds of external references that
+ * will prevent module unloading or destruction of other
+ * resources. Need to release them now, since skb can
+ * stay on a defer list indefinitely.
+ */
+ skb_release_head_state(skb);
+
spin_lock_irqsave(&sd->defer_lock, flags);
/* Send an IPI every time queue reaches half capacity. */
kick = sd->defer_count == (defer_max >> 1);
--
2.34.3
On Sun, Jun 19, 2022 at 2:39 AM Ilya Maximets <[email protected]> wrote:
>
> Open vSwitch system test suite is broken due to inability to
> load/unload netfilter modules. kworker thread is getting trapped
> in the infinite loop while running a net cleanup inside the
> nf_conntrack_cleanup_net_list, because deferred skbuffs are still
> holding nfct references and not being freed by their CPU cores.
>
> In general, the idea that we will have an rx interrupt on every
> CPU core at some point in a near future doesn't seem correct.
> Devices are getting created and destroyed, interrupts are getting
> re-scheduled, CPUs are going online and offline dynamically.
> Any of these events may leave packets stuck in defer list for a
> long time. It might be OK, if they are just a piece of memory,
> but we can't afford them holding references to any other resources.
>
> In case of OVS, nfct reference keeps the kernel thread in busy loop
> while holding a 'pernet_ops_rwsem' semaphore. That blocks the
> later modprobe request from user space:
>
> # ps
> 299 root R 99.3 200:25.89 kworker/u96:4+
>
> # journalctl
> INFO: task modprobe:11787 blocked for more than 1228 seconds.
> Not tainted 5.19.0-rc2 #8
> task:modprobe state:D
> Call Trace:
> <TASK>
> __schedule+0x8aa/0x21d0
> schedule+0xcc/0x200
> rwsem_down_write_slowpath+0x8e4/0x1580
> down_write+0xfc/0x140
> register_pernet_subsys+0x15/0x40
> nf_nat_init+0xb6/0x1000 [nf_nat]
> do_one_initcall+0xbb/0x410
> do_init_module+0x1b4/0x640
> load_module+0x4c1b/0x58d0
> __do_sys_init_module+0x1d7/0x220
> do_syscall_64+0x3a/0x80
> entry_SYSCALL_64_after_hwframe+0x46/0xb0
>
> At this point OVS testsuite is unresponsive and never recover,
> because these skbuffs are never freed.
>
> Solution is to make sure no external references attached to skb
> before pushing it to the defer list. Using skb_release_head_state()
> for that purpose. The function modified to be re-enterable, as it
> will be called again during the defer list flush.
>
> Another approach that can fix the OVS use-case, is to kick all
> cores while waiting for references to be released during the net
> cleanup. But that sounds more like a workaround for a current
> issue rather than a proper solution and will not cover possible
> issues in other parts of the code.
>
> Additionally checking for skb_zcopy() while deferring. This might
> not be necessary, as I'm not sure if we can actually have zero copy
> packets on this path, but seems worth having for completeness as we
> should never defer such packets regardless.
>
> CC: Eric Dumazet <[email protected]>
> Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
> net/core/skbuff.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
I do not think this patch is doing the right thing.
Packets sitting in TCP receive queues should not hold state that is
not relevant for TCP recvmsg().
This consumes extra memory for no good reason, and defer expensive
atomic operations.
We for instance release skb dst before skb is queued, we should do the
same for conntrack state.
This would increase performance anyway, as we free ct state while cpu
caches are hot.
On Wed, Jun 22, 2022 at 12:02 PM Eric Dumazet <[email protected]> wrote:
>
> On Sun, Jun 19, 2022 at 2:39 AM Ilya Maximets <[email protected]> wrote:
> >
> > Open vSwitch system test suite is broken due to inability to
> > load/unload netfilter modules. kworker thread is getting trapped
> > in the infinite loop while running a net cleanup inside the
> > nf_conntrack_cleanup_net_list, because deferred skbuffs are still
> > holding nfct references and not being freed by their CPU cores.
> >
> > In general, the idea that we will have an rx interrupt on every
> > CPU core at some point in a near future doesn't seem correct.
> > Devices are getting created and destroyed, interrupts are getting
> > re-scheduled, CPUs are going online and offline dynamically.
> > Any of these events may leave packets stuck in defer list for a
> > long time. It might be OK, if they are just a piece of memory,
> > but we can't afford them holding references to any other resources.
> >
> > In case of OVS, nfct reference keeps the kernel thread in busy loop
> > while holding a 'pernet_ops_rwsem' semaphore. That blocks the
> > later modprobe request from user space:
> >
> > # ps
> > 299 root R 99.3 200:25.89 kworker/u96:4+
> >
> > # journalctl
> > INFO: task modprobe:11787 blocked for more than 1228 seconds.
> > Not tainted 5.19.0-rc2 #8
> > task:modprobe state:D
> > Call Trace:
> > <TASK>
> > __schedule+0x8aa/0x21d0
> > schedule+0xcc/0x200
> > rwsem_down_write_slowpath+0x8e4/0x1580
> > down_write+0xfc/0x140
> > register_pernet_subsys+0x15/0x40
> > nf_nat_init+0xb6/0x1000 [nf_nat]
> > do_one_initcall+0xbb/0x410
> > do_init_module+0x1b4/0x640
> > load_module+0x4c1b/0x58d0
> > __do_sys_init_module+0x1d7/0x220
> > do_syscall_64+0x3a/0x80
> > entry_SYSCALL_64_after_hwframe+0x46/0xb0
> >
> > At this point OVS testsuite is unresponsive and never recover,
> > because these skbuffs are never freed.
> >
> > Solution is to make sure no external references attached to skb
> > before pushing it to the defer list. Using skb_release_head_state()
> > for that purpose. The function modified to be re-enterable, as it
> > will be called again during the defer list flush.
> >
> > Another approach that can fix the OVS use-case, is to kick all
> > cores while waiting for references to be released during the net
> > cleanup. But that sounds more like a workaround for a current
> > issue rather than a proper solution and will not cover possible
> > issues in other parts of the code.
> >
> > Additionally checking for skb_zcopy() while deferring. This might
> > not be necessary, as I'm not sure if we can actually have zero copy
> > packets on this path, but seems worth having for completeness as we
> > should never defer such packets regardless.
> >
> > CC: Eric Dumazet <[email protected]>
> > Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
> > Signed-off-by: Ilya Maximets <[email protected]>
> > ---
> > net/core/skbuff.c | 16 +++++++++++-----
> > 1 file changed, 11 insertions(+), 5 deletions(-)
>
> I do not think this patch is doing the right thing.
>
> Packets sitting in TCP receive queues should not hold state that is
> not relevant for TCP recvmsg().
>
> This consumes extra memory for no good reason, and defer expensive
> atomic operations.
>
> We for instance release skb dst before skb is queued, we should do the
> same for conntrack state.
>
> This would increase performance anyway, as we free ct state while cpu
> caches are hot.
I am thinking of the following instead.
A new helper can be added (and later be used in net/packet/af_packet.c
and probably elsewhere)
diff --git a/include/net/dst.h b/include/net/dst.h
index 6aa252c3fc55ccaee58faebf265510469e91d780..7c3316d9d6e73daea17223a5261f6a5c4f68eae3
100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -276,6 +276,15 @@ static inline void skb_dst_drop(struct sk_buff *skb)
}
}
+/* Before queueing skb in a receive queue, get rid of
+ * potentially expensive components.
+ */
+static inline void skb_cleanup(struct sk_buff *skb)
+{
+ skb_dst_drop(skb);
+ nf_reset_ct(skb);
+}
+
static inline void __skb_dst_copy(struct sk_buff *nskb, unsigned long refdst)
{
nskb->slow_gro |= !!refdst;
diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index fdbcf2a6d08ef4a5164247b5a5b4b222289b191a..913c98e446d56ee067b54b2c704ac1195ef1a81e
100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -177,7 +177,7 @@ void tcp_fastopen_add_skb(struct sock *sk, struct
sk_buff *skb)
if (!skb)
return;
- skb_dst_drop(skb);
+ skb_cleanup(skb);
/* segs_in has been initialized to 1 in tcp_create_openreq_child().
* Hence, reset segs_in to 0 before calling tcp_segs_in()
* to avoid double counting. Also, tcp_segs_in() expects
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 2e2a9ece9af27372e6b653d685a89a2c71ba05d1..987981a16ee34e0601e7e722abef1bb098c307c5
100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5005,7 +5005,7 @@ static void tcp_data_queue(struct sock *sk,
struct sk_buff *skb)
__kfree_skb(skb);
return;
}
- skb_dst_drop(skb);
+ skb_cleanup(skb);
__skb_pull(skb, tcp_hdr(skb)->doff * 4);
reason = SKB_DROP_REASON_NOT_SPECIFIED;
@@ -5931,7 +5931,7 @@ void tcp_rcv_established(struct sock *sk, struct
sk_buff *skb)
NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPHPHITS);
/* Bulk data transfer: receiver */
- skb_dst_drop(skb);
+ skb_cleanup(skb);
__skb_pull(skb, tcp_header_len);
eaten = tcp_queue_rcv(sk, skb, &fragstolen);
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index fe8f23b95d32ca4a35d05166d471327bc608fa91..d9acd906f28267ff07450d78d079e4e8eab74957
100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1765,7 +1765,7 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb,
*/
skb_condense(skb);
- skb_dst_drop(skb);
+ skb_cleanup(skb);
if (unlikely(tcp_checksum_complete(skb))) {
bh_unlock_sock(sk);
Eric Dumazet <[email protected]> wrote:
> On Sun, Jun 19, 2022 at 2:39 AM Ilya Maximets <[email protected]> wrote:
> >
> > Open vSwitch system test suite is broken due to inability to
> > load/unload netfilter modules. kworker thread is getting trapped
> > in the infinite loop while running a net cleanup inside the
> > nf_conntrack_cleanup_net_list, because deferred skbuffs are still
> > holding nfct references and not being freed by their CPU cores.
> >
> > In general, the idea that we will have an rx interrupt on every
> > CPU core at some point in a near future doesn't seem correct.
> > Devices are getting created and destroyed, interrupts are getting
> > re-scheduled, CPUs are going online and offline dynamically.
> > Any of these events may leave packets stuck in defer list for a
> > long time. It might be OK, if they are just a piece of memory,
> > but we can't afford them holding references to any other resources.
> >
> > In case of OVS, nfct reference keeps the kernel thread in busy loop
> > while holding a 'pernet_ops_rwsem' semaphore. That blocks the
> > later modprobe request from user space:
> >
> > # ps
> > 299 root R 99.3 200:25.89 kworker/u96:4+
> >
> > # journalctl
> > INFO: task modprobe:11787 blocked for more than 1228 seconds.
> > Not tainted 5.19.0-rc2 #8
> > task:modprobe state:D
> > Call Trace:
> > <TASK>
> > __schedule+0x8aa/0x21d0
> > schedule+0xcc/0x200
> > rwsem_down_write_slowpath+0x8e4/0x1580
> > down_write+0xfc/0x140
> > register_pernet_subsys+0x15/0x40
> > nf_nat_init+0xb6/0x1000 [nf_nat]
> > do_one_initcall+0xbb/0x410
> > do_init_module+0x1b4/0x640
> > load_module+0x4c1b/0x58d0
> > __do_sys_init_module+0x1d7/0x220
> > do_syscall_64+0x3a/0x80
> > entry_SYSCALL_64_after_hwframe+0x46/0xb0
> >
> > At this point OVS testsuite is unresponsive and never recover,
> > because these skbuffs are never freed.
> >
> > Solution is to make sure no external references attached to skb
> > before pushing it to the defer list. Using skb_release_head_state()
> > for that purpose. The function modified to be re-enterable, as it
> > will be called again during the defer list flush.
> >
> > Another approach that can fix the OVS use-case, is to kick all
> > cores while waiting for references to be released during the net
> > cleanup. But that sounds more like a workaround for a current
> > issue rather than a proper solution and will not cover possible
> > issues in other parts of the code.
> >
> > Additionally checking for skb_zcopy() while deferring. This might
> > not be necessary, as I'm not sure if we can actually have zero copy
> > packets on this path, but seems worth having for completeness as we
> > should never defer such packets regardless.
> >
> > CC: Eric Dumazet <[email protected]>
> > Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
> > Signed-off-by: Ilya Maximets <[email protected]>
> > ---
> > net/core/skbuff.c | 16 +++++++++++-----
> > 1 file changed, 11 insertions(+), 5 deletions(-)
>
> I do not think this patch is doing the right thing.
>
> Packets sitting in TCP receive queues should not hold state that is
> not relevant for TCP recvmsg().
Agree, but tcp_v4/6_rcv() already call nf_reset_ct(), else it would
not be possible to remove nf_conntrack module in practice.
I wonder where the deferred skbs are coming from, any and all
queued skbs need the conntrack state dropped.
I don't mind a new helper that does a combined dst+ct release though.
On Wed, Jun 22, 2022 at 12:28 PM Florian Westphal <[email protected]> wrote:
>
> Eric Dumazet <[email protected]> wrote:
> > On Sun, Jun 19, 2022 at 2:39 AM Ilya Maximets <[email protected]> wrote:
> > >
> > > Open vSwitch system test suite is broken due to inability to
> > > load/unload netfilter modules. kworker thread is getting trapped
> > > in the infinite loop while running a net cleanup inside the
> > > nf_conntrack_cleanup_net_list, because deferred skbuffs are still
> > > holding nfct references and not being freed by their CPU cores.
> > >
> > > In general, the idea that we will have an rx interrupt on every
> > > CPU core at some point in a near future doesn't seem correct.
> > > Devices are getting created and destroyed, interrupts are getting
> > > re-scheduled, CPUs are going online and offline dynamically.
> > > Any of these events may leave packets stuck in defer list for a
> > > long time. It might be OK, if they are just a piece of memory,
> > > but we can't afford them holding references to any other resources.
> > >
> > > In case of OVS, nfct reference keeps the kernel thread in busy loop
> > > while holding a 'pernet_ops_rwsem' semaphore. That blocks the
> > > later modprobe request from user space:
> > >
> > > # ps
> > > 299 root R 99.3 200:25.89 kworker/u96:4+
> > >
> > > # journalctl
> > > INFO: task modprobe:11787 blocked for more than 1228 seconds.
> > > Not tainted 5.19.0-rc2 #8
> > > task:modprobe state:D
> > > Call Trace:
> > > <TASK>
> > > __schedule+0x8aa/0x21d0
> > > schedule+0xcc/0x200
> > > rwsem_down_write_slowpath+0x8e4/0x1580
> > > down_write+0xfc/0x140
> > > register_pernet_subsys+0x15/0x40
> > > nf_nat_init+0xb6/0x1000 [nf_nat]
> > > do_one_initcall+0xbb/0x410
> > > do_init_module+0x1b4/0x640
> > > load_module+0x4c1b/0x58d0
> > > __do_sys_init_module+0x1d7/0x220
> > > do_syscall_64+0x3a/0x80
> > > entry_SYSCALL_64_after_hwframe+0x46/0xb0
> > >
> > > At this point OVS testsuite is unresponsive and never recover,
> > > because these skbuffs are never freed.
> > >
> > > Solution is to make sure no external references attached to skb
> > > before pushing it to the defer list. Using skb_release_head_state()
> > > for that purpose. The function modified to be re-enterable, as it
> > > will be called again during the defer list flush.
> > >
> > > Another approach that can fix the OVS use-case, is to kick all
> > > cores while waiting for references to be released during the net
> > > cleanup. But that sounds more like a workaround for a current
> > > issue rather than a proper solution and will not cover possible
> > > issues in other parts of the code.
> > >
> > > Additionally checking for skb_zcopy() while deferring. This might
> > > not be necessary, as I'm not sure if we can actually have zero copy
> > > packets on this path, but seems worth having for completeness as we
> > > should never defer such packets regardless.
> > >
> > > CC: Eric Dumazet <[email protected]>
> > > Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
> > > Signed-off-by: Ilya Maximets <[email protected]>
> > > ---
> > > net/core/skbuff.c | 16 +++++++++++-----
> > > 1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > I do not think this patch is doing the right thing.
> >
> > Packets sitting in TCP receive queues should not hold state that is
> > not relevant for TCP recvmsg().
>
> Agree, but tcp_v4/6_rcv() already call nf_reset_ct(), else it would
> not be possible to remove nf_conntrack module in practice.
Well, existing nf_reset_ct() does not catch all cases, like TCP fastopen ?
Maybe 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
only widened the problem.
>
> I wonder where the deferred skbs are coming from, any and all
> queued skbs need the conntrack state dropped.
>
> I don't mind a new helper that does a combined dst+ct release though.
On 6/22/22 12:36, Eric Dumazet wrote:
> On Wed, Jun 22, 2022 at 12:28 PM Florian Westphal <[email protected]> wrote:
>>
>> Eric Dumazet <[email protected]> wrote:
>>> On Sun, Jun 19, 2022 at 2:39 AM Ilya Maximets <[email protected]> wrote:
>>>>
>>>> Open vSwitch system test suite is broken due to inability to
>>>> load/unload netfilter modules. kworker thread is getting trapped
>>>> in the infinite loop while running a net cleanup inside the
>>>> nf_conntrack_cleanup_net_list, because deferred skbuffs are still
>>>> holding nfct references and not being freed by their CPU cores.
>>>>
>>>> In general, the idea that we will have an rx interrupt on every
>>>> CPU core at some point in a near future doesn't seem correct.
>>>> Devices are getting created and destroyed, interrupts are getting
>>>> re-scheduled, CPUs are going online and offline dynamically.
>>>> Any of these events may leave packets stuck in defer list for a
>>>> long time. It might be OK, if they are just a piece of memory,
>>>> but we can't afford them holding references to any other resources.
>>>>
>>>> In case of OVS, nfct reference keeps the kernel thread in busy loop
>>>> while holding a 'pernet_ops_rwsem' semaphore. That blocks the
>>>> later modprobe request from user space:
>>>>
>>>> # ps
>>>> 299 root R 99.3 200:25.89 kworker/u96:4+
>>>>
>>>> # journalctl
>>>> INFO: task modprobe:11787 blocked for more than 1228 seconds.
>>>> Not tainted 5.19.0-rc2 #8
>>>> task:modprobe state:D
>>>> Call Trace:
>>>> <TASK>
>>>> __schedule+0x8aa/0x21d0
>>>> schedule+0xcc/0x200
>>>> rwsem_down_write_slowpath+0x8e4/0x1580
>>>> down_write+0xfc/0x140
>>>> register_pernet_subsys+0x15/0x40
>>>> nf_nat_init+0xb6/0x1000 [nf_nat]
>>>> do_one_initcall+0xbb/0x410
>>>> do_init_module+0x1b4/0x640
>>>> load_module+0x4c1b/0x58d0
>>>> __do_sys_init_module+0x1d7/0x220
>>>> do_syscall_64+0x3a/0x80
>>>> entry_SYSCALL_64_after_hwframe+0x46/0xb0
>>>>
>>>> At this point OVS testsuite is unresponsive and never recover,
>>>> because these skbuffs are never freed.
>>>>
>>>> Solution is to make sure no external references attached to skb
>>>> before pushing it to the defer list. Using skb_release_head_state()
>>>> for that purpose. The function modified to be re-enterable, as it
>>>> will be called again during the defer list flush.
>>>>
>>>> Another approach that can fix the OVS use-case, is to kick all
>>>> cores while waiting for references to be released during the net
>>>> cleanup. But that sounds more like a workaround for a current
>>>> issue rather than a proper solution and will not cover possible
>>>> issues in other parts of the code.
>>>>
>>>> Additionally checking for skb_zcopy() while deferring. This might
>>>> not be necessary, as I'm not sure if we can actually have zero copy
>>>> packets on this path, but seems worth having for completeness as we
>>>> should never defer such packets regardless.
>>>>
>>>> CC: Eric Dumazet <[email protected]>
>>>> Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
>>>> Signed-off-by: Ilya Maximets <[email protected]>
>>>> ---
>>>> net/core/skbuff.c | 16 +++++++++++-----
>>>> 1 file changed, 11 insertions(+), 5 deletions(-)
>>>
>>> I do not think this patch is doing the right thing.
>>>
>>> Packets sitting in TCP receive queues should not hold state that is
>>> not relevant for TCP recvmsg().
>>
>> Agree, but tcp_v4/6_rcv() already call nf_reset_ct(), else it would
>> not be possible to remove nf_conntrack module in practice.
>
> Well, existing nf_reset_ct() does not catch all cases, like TCP fastopen ?
Yeah, that is kind of the main problem I have with the current
code. It's very hard to find all the cases where skb has to be
cleaned up and almost impossible for someone who doesn't know
every aspect of every network subsystem in the kernel. That's
why I went with the more or less bulletproof approach of cleaning
up while actually deferring. I can try and test the code you
proposed in the other reply, but at least, I think, we need a
bunch of debug warnings in the skb_attempt_defer_free() to catch
possible issues.
Also, what about cleaning up extensions? IIUC, at least one
of them can hold external references. SKB_EXT_SEC_PATH holds
xfrm_state. We should, probably, free them as well?
And what about zerocopy skb? I think, we should still not
allow them to be deferred as they seem to hold HW resources.
>
> Maybe 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
> only widened the problem.
>
>>
>> I wonder where the deferred skbs are coming from, any and all
>> queued skbs need the conntrack state dropped.
>>
>> I don't mind a new helper that does a combined dst+ct release though.
On Wed, Jun 22, 2022 at 1:32 PM Ilya Maximets <[email protected]> wrote:
>
> On 6/22/22 12:36, Eric Dumazet wrote:
> > On Wed, Jun 22, 2022 at 12:28 PM Florian Westphal <[email protected]> wrote:
> >>
> >> Eric Dumazet <[email protected]> wrote:
> >>> On Sun, Jun 19, 2022 at 2:39 AM Ilya Maximets <[email protected]> wrote:
> >>>>
> >>>> Open vSwitch system test suite is broken due to inability to
> >>>> load/unload netfilter modules. kworker thread is getting trapped
> >>>> in the infinite loop while running a net cleanup inside the
> >>>> nf_conntrack_cleanup_net_list, because deferred skbuffs are still
> >>>> holding nfct references and not being freed by their CPU cores.
> >>>>
> >>>> In general, the idea that we will have an rx interrupt on every
> >>>> CPU core at some point in a near future doesn't seem correct.
> >>>> Devices are getting created and destroyed, interrupts are getting
> >>>> re-scheduled, CPUs are going online and offline dynamically.
> >>>> Any of these events may leave packets stuck in defer list for a
> >>>> long time. It might be OK, if they are just a piece of memory,
> >>>> but we can't afford them holding references to any other resources.
> >>>>
> >>>> In case of OVS, nfct reference keeps the kernel thread in busy loop
> >>>> while holding a 'pernet_ops_rwsem' semaphore. That blocks the
> >>>> later modprobe request from user space:
> >>>>
> >>>> # ps
> >>>> 299 root R 99.3 200:25.89 kworker/u96:4+
> >>>>
> >>>> # journalctl
> >>>> INFO: task modprobe:11787 blocked for more than 1228 seconds.
> >>>> Not tainted 5.19.0-rc2 #8
> >>>> task:modprobe state:D
> >>>> Call Trace:
> >>>> <TASK>
> >>>> __schedule+0x8aa/0x21d0
> >>>> schedule+0xcc/0x200
> >>>> rwsem_down_write_slowpath+0x8e4/0x1580
> >>>> down_write+0xfc/0x140
> >>>> register_pernet_subsys+0x15/0x40
> >>>> nf_nat_init+0xb6/0x1000 [nf_nat]
> >>>> do_one_initcall+0xbb/0x410
> >>>> do_init_module+0x1b4/0x640
> >>>> load_module+0x4c1b/0x58d0
> >>>> __do_sys_init_module+0x1d7/0x220
> >>>> do_syscall_64+0x3a/0x80
> >>>> entry_SYSCALL_64_after_hwframe+0x46/0xb0
> >>>>
> >>>> At this point OVS testsuite is unresponsive and never recover,
> >>>> because these skbuffs are never freed.
> >>>>
> >>>> Solution is to make sure no external references attached to skb
> >>>> before pushing it to the defer list. Using skb_release_head_state()
> >>>> for that purpose. The function modified to be re-enterable, as it
> >>>> will be called again during the defer list flush.
> >>>>
> >>>> Another approach that can fix the OVS use-case, is to kick all
> >>>> cores while waiting for references to be released during the net
> >>>> cleanup. But that sounds more like a workaround for a current
> >>>> issue rather than a proper solution and will not cover possible
> >>>> issues in other parts of the code.
> >>>>
> >>>> Additionally checking for skb_zcopy() while deferring. This might
> >>>> not be necessary, as I'm not sure if we can actually have zero copy
> >>>> packets on this path, but seems worth having for completeness as we
> >>>> should never defer such packets regardless.
> >>>>
> >>>> CC: Eric Dumazet <[email protected]>
> >>>> Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
> >>>> Signed-off-by: Ilya Maximets <[email protected]>
> >>>> ---
> >>>> net/core/skbuff.c | 16 +++++++++++-----
> >>>> 1 file changed, 11 insertions(+), 5 deletions(-)
> >>>
> >>> I do not think this patch is doing the right thing.
> >>>
> >>> Packets sitting in TCP receive queues should not hold state that is
> >>> not relevant for TCP recvmsg().
> >>
> >> Agree, but tcp_v4/6_rcv() already call nf_reset_ct(), else it would
> >> not be possible to remove nf_conntrack module in practice.
> >
> > Well, existing nf_reset_ct() does not catch all cases, like TCP fastopen ?
>
> Yeah, that is kind of the main problem I have with the current
> code. It's very hard to find all the cases where skb has to be
> cleaned up and almost impossible for someone who doesn't know
> every aspect of every network subsystem in the kernel. That's
> why I went with the more or less bulletproof approach of cleaning
> up while actually deferring. I can try and test the code you
> proposed in the other reply, but at least, I think, we need a
> bunch of debug warnings in the skb_attempt_defer_free() to catch
> possible issues.
Debug warnings are expensive if they need to bring new cache lines.
So far skb_attempt_defer_free() is only used by TCP in well known conditions.
>
> Also, what about cleaning up extensions? IIUC, at least one
> of them can hold external references. SKB_EXT_SEC_PATH holds
> xfrm_state. We should, probably, free them as well?
I do not know about this, I would ask XFRM maintainers
>
> And what about zerocopy skb? I think, we should still not
> allow them to be deferred as they seem to hold HW resources.
The point of skb_attempt_defer_free() i is to make the freeing happen
at producer
much instead of consumer.
I do not think there is anything special in this regard with zero
copy. I would leave the current code as is.
A simpler patch might be to move the existing nf_reset_ct() earlier,
can you test this ?
I note that IPv6 does the nf_reset_ct() earlier, from ip6_protocol_deliver_rcu()
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index fda811a5251f2d76ac24a036e6b4f4e7d7d96d6f..a06464f96fe0cc94dd78272738ddaab2c19e87db
100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1919,6 +1919,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
struct sock *sk;
int ret;
+ nf_reset_ct(skb);
+
drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
if (skb->pkt_type != PACKET_HOST)
goto discard_it;
@@ -2046,8 +2048,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
if (drop_reason)
goto discard_and_relse;
- nf_reset_ct(skb);
-
if (tcp_filter(sk, skb)) {
drop_reason = SKB_DROP_REASON_SOCKET_FILTER;
goto discard_and_relse;
On 6/22/22 13:43, Eric Dumazet wrote:
> On Wed, Jun 22, 2022 at 1:32 PM Ilya Maximets <[email protected]> wrote:
>>
>> On 6/22/22 12:36, Eric Dumazet wrote:
>>> On Wed, Jun 22, 2022 at 12:28 PM Florian Westphal <[email protected]> wrote:
>>>>
>>>> Eric Dumazet <[email protected]> wrote:
>>>>> On Sun, Jun 19, 2022 at 2:39 AM Ilya Maximets <[email protected]> wrote:
>>>>>>
>>>>>> Open vSwitch system test suite is broken due to inability to
>>>>>> load/unload netfilter modules. kworker thread is getting trapped
>>>>>> in the infinite loop while running a net cleanup inside the
>>>>>> nf_conntrack_cleanup_net_list, because deferred skbuffs are still
>>>>>> holding nfct references and not being freed by their CPU cores.
>>>>>>
>>>>>> In general, the idea that we will have an rx interrupt on every
>>>>>> CPU core at some point in a near future doesn't seem correct.
>>>>>> Devices are getting created and destroyed, interrupts are getting
>>>>>> re-scheduled, CPUs are going online and offline dynamically.
>>>>>> Any of these events may leave packets stuck in defer list for a
>>>>>> long time. It might be OK, if they are just a piece of memory,
>>>>>> but we can't afford them holding references to any other resources.
>>>>>>
>>>>>> In case of OVS, nfct reference keeps the kernel thread in busy loop
>>>>>> while holding a 'pernet_ops_rwsem' semaphore. That blocks the
>>>>>> later modprobe request from user space:
>>>>>>
>>>>>> # ps
>>>>>> 299 root R 99.3 200:25.89 kworker/u96:4+
>>>>>>
>>>>>> # journalctl
>>>>>> INFO: task modprobe:11787 blocked for more than 1228 seconds.
>>>>>> Not tainted 5.19.0-rc2 #8
>>>>>> task:modprobe state:D
>>>>>> Call Trace:
>>>>>> <TASK>
>>>>>> __schedule+0x8aa/0x21d0
>>>>>> schedule+0xcc/0x200
>>>>>> rwsem_down_write_slowpath+0x8e4/0x1580
>>>>>> down_write+0xfc/0x140
>>>>>> register_pernet_subsys+0x15/0x40
>>>>>> nf_nat_init+0xb6/0x1000 [nf_nat]
>>>>>> do_one_initcall+0xbb/0x410
>>>>>> do_init_module+0x1b4/0x640
>>>>>> load_module+0x4c1b/0x58d0
>>>>>> __do_sys_init_module+0x1d7/0x220
>>>>>> do_syscall_64+0x3a/0x80
>>>>>> entry_SYSCALL_64_after_hwframe+0x46/0xb0
>>>>>>
>>>>>> At this point OVS testsuite is unresponsive and never recover,
>>>>>> because these skbuffs are never freed.
>>>>>>
>>>>>> Solution is to make sure no external references attached to skb
>>>>>> before pushing it to the defer list. Using skb_release_head_state()
>>>>>> for that purpose. The function modified to be re-enterable, as it
>>>>>> will be called again during the defer list flush.
>>>>>>
>>>>>> Another approach that can fix the OVS use-case, is to kick all
>>>>>> cores while waiting for references to be released during the net
>>>>>> cleanup. But that sounds more like a workaround for a current
>>>>>> issue rather than a proper solution and will not cover possible
>>>>>> issues in other parts of the code.
>>>>>>
>>>>>> Additionally checking for skb_zcopy() while deferring. This might
>>>>>> not be necessary, as I'm not sure if we can actually have zero copy
>>>>>> packets on this path, but seems worth having for completeness as we
>>>>>> should never defer such packets regardless.
>>>>>>
>>>>>> CC: Eric Dumazet <[email protected]>
>>>>>> Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
>>>>>> Signed-off-by: Ilya Maximets <[email protected]>
>>>>>> ---
>>>>>> net/core/skbuff.c | 16 +++++++++++-----
>>>>>> 1 file changed, 11 insertions(+), 5 deletions(-)
>>>>>
>>>>> I do not think this patch is doing the right thing.
>>>>>
>>>>> Packets sitting in TCP receive queues should not hold state that is
>>>>> not relevant for TCP recvmsg().
>>>>
>>>> Agree, but tcp_v4/6_rcv() already call nf_reset_ct(), else it would
>>>> not be possible to remove nf_conntrack module in practice.
>>>
>>> Well, existing nf_reset_ct() does not catch all cases, like TCP fastopen ?
>>
>> Yeah, that is kind of the main problem I have with the current
>> code. It's very hard to find all the cases where skb has to be
>> cleaned up and almost impossible for someone who doesn't know
>> every aspect of every network subsystem in the kernel. That's
>> why I went with the more or less bulletproof approach of cleaning
>> up while actually deferring. I can try and test the code you
>> proposed in the other reply, but at least, I think, we need a
>> bunch of debug warnings in the skb_attempt_defer_free() to catch
>> possible issues.
>
> Debug warnings are expensive if they need to bring new cache lines.
>
> So far skb_attempt_defer_free() is only used by TCP in well known conditions.
That's true for skb_attempt_defer_free() itself, but it's
hard to tell the same for all the parts of the code that
are enqueuing these skbuffs. Even if all the places are
well known, it looks to me highly error prone. Having
a couple of DEBUG_NET_WARN_ON_ONCE should not cause any
performance issues, IIUC, unless debug is enabled, right?
e.g.
DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
>
>
>>
>> Also, what about cleaning up extensions? IIUC, at least one
>> of them can hold external references. SKB_EXT_SEC_PATH holds
>> xfrm_state. We should, probably, free them as well?
>
> I do not know about this, I would ask XFRM maintainers
@Steffen, @Herbert, what do you think? Is it a problem if the
skb holding SKB_EXT_SEC_PATH extension is held not freed in
a defer list indefinitely (for a very long time)? Or is it
even possible for an skb with SKB_EXT_SEC_PATH extension present
to be enqueued to a TCP receive queue without releasing all
the references?
This seems like a long existing problem though, so can be fixed
separately, if it is a problem.
>
>>
>> And what about zerocopy skb? I think, we should still not
>> allow them to be deferred as they seem to hold HW resources.
>
> The point of skb_attempt_defer_free() i is to make the freeing happen
> at producer
> much instead of consumer.
>
> I do not think there is anything special in this regard with zero
> copy. I would leave the current code as is.
The problem is that these skbuffs are never actually freed,
so if they do hold HW resources, these resources will never be
released (never == for a very long time, hours, days).
Not a blocker from my point of view, just trying to highlight
a possible issue.
>
> A simpler patch might be to move the existing nf_reset_ct() earlier,
> can you test this ?
I tested the patch below and it seems to fix the issue seen
with OVS testsuite. Though it's not obvious for me why this
happens. Can you explain a bit more?
>
> I note that IPv6 does the nf_reset_ct() earlier, from ip6_protocol_deliver_rcu()
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index fda811a5251f2d76ac24a036e6b4f4e7d7d96d6f..a06464f96fe0cc94dd78272738ddaab2c19e87db
> 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1919,6 +1919,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
> struct sock *sk;
> int ret;
>
> + nf_reset_ct(skb);
> +
> drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> if (skb->pkt_type != PACKET_HOST)
> goto discard_it;
> @@ -2046,8 +2048,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
> if (drop_reason)
> goto discard_and_relse;
>
> - nf_reset_ct(skb);
> -
> if (tcp_filter(sk, skb)) {
> drop_reason = SKB_DROP_REASON_SOCKET_FILTER;
> goto discard_and_relse;
On Wed, Jun 22, 2022 at 4:26 PM Ilya Maximets <[email protected]> wrote:
>
> On 6/22/22 13:43, Eric Dumazet wrote:
> > On Wed, Jun 22, 2022 at 1:32 PM Ilya Maximets <[email protected]> wrote:
> >>
> >> On 6/22/22 12:36, Eric Dumazet wrote:
> >>> On Wed, Jun 22, 2022 at 12:28 PM Florian Westphal <[email protected]> wrote:
> >>>>
> >>>> Eric Dumazet <[email protected]> wrote:
> >>>>> On Sun, Jun 19, 2022 at 2:39 AM Ilya Maximets <[email protected]> wrote:
> >>>>>>
> >>>>>> Open vSwitch system test suite is broken due to inability to
> >>>>>> load/unload netfilter modules. kworker thread is getting trapped
> >>>>>> in the infinite loop while running a net cleanup inside the
> >>>>>> nf_conntrack_cleanup_net_list, because deferred skbuffs are still
> >>>>>> holding nfct references and not being freed by their CPU cores.
> >>>>>>
> >>>>>> In general, the idea that we will have an rx interrupt on every
> >>>>>> CPU core at some point in a near future doesn't seem correct.
> >>>>>> Devices are getting created and destroyed, interrupts are getting
> >>>>>> re-scheduled, CPUs are going online and offline dynamically.
> >>>>>> Any of these events may leave packets stuck in defer list for a
> >>>>>> long time. It might be OK, if they are just a piece of memory,
> >>>>>> but we can't afford them holding references to any other resources.
> >>>>>>
> >>>>>> In case of OVS, nfct reference keeps the kernel thread in busy loop
> >>>>>> while holding a 'pernet_ops_rwsem' semaphore. That blocks the
> >>>>>> later modprobe request from user space:
> >>>>>>
> >>>>>> # ps
> >>>>>> 299 root R 99.3 200:25.89 kworker/u96:4+
> >>>>>>
> >>>>>> # journalctl
> >>>>>> INFO: task modprobe:11787 blocked for more than 1228 seconds.
> >>>>>> Not tainted 5.19.0-rc2 #8
> >>>>>> task:modprobe state:D
> >>>>>> Call Trace:
> >>>>>> <TASK>
> >>>>>> __schedule+0x8aa/0x21d0
> >>>>>> schedule+0xcc/0x200
> >>>>>> rwsem_down_write_slowpath+0x8e4/0x1580
> >>>>>> down_write+0xfc/0x140
> >>>>>> register_pernet_subsys+0x15/0x40
> >>>>>> nf_nat_init+0xb6/0x1000 [nf_nat]
> >>>>>> do_one_initcall+0xbb/0x410
> >>>>>> do_init_module+0x1b4/0x640
> >>>>>> load_module+0x4c1b/0x58d0
> >>>>>> __do_sys_init_module+0x1d7/0x220
> >>>>>> do_syscall_64+0x3a/0x80
> >>>>>> entry_SYSCALL_64_after_hwframe+0x46/0xb0
> >>>>>>
> >>>>>> At this point OVS testsuite is unresponsive and never recover,
> >>>>>> because these skbuffs are never freed.
> >>>>>>
> >>>>>> Solution is to make sure no external references attached to skb
> >>>>>> before pushing it to the defer list. Using skb_release_head_state()
> >>>>>> for that purpose. The function modified to be re-enterable, as it
> >>>>>> will be called again during the defer list flush.
> >>>>>>
> >>>>>> Another approach that can fix the OVS use-case, is to kick all
> >>>>>> cores while waiting for references to be released during the net
> >>>>>> cleanup. But that sounds more like a workaround for a current
> >>>>>> issue rather than a proper solution and will not cover possible
> >>>>>> issues in other parts of the code.
> >>>>>>
> >>>>>> Additionally checking for skb_zcopy() while deferring. This might
> >>>>>> not be necessary, as I'm not sure if we can actually have zero copy
> >>>>>> packets on this path, but seems worth having for completeness as we
> >>>>>> should never defer such packets regardless.
> >>>>>>
> >>>>>> CC: Eric Dumazet <[email protected]>
> >>>>>> Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
> >>>>>> Signed-off-by: Ilya Maximets <[email protected]>
> >>>>>> ---
> >>>>>> net/core/skbuff.c | 16 +++++++++++-----
> >>>>>> 1 file changed, 11 insertions(+), 5 deletions(-)
> >>>>>
> >>>>> I do not think this patch is doing the right thing.
> >>>>>
> >>>>> Packets sitting in TCP receive queues should not hold state that is
> >>>>> not relevant for TCP recvmsg().
> >>>>
> >>>> Agree, but tcp_v4/6_rcv() already call nf_reset_ct(), else it would
> >>>> not be possible to remove nf_conntrack module in practice.
> >>>
> >>> Well, existing nf_reset_ct() does not catch all cases, like TCP fastopen ?
> >>
> >> Yeah, that is kind of the main problem I have with the current
> >> code. It's very hard to find all the cases where skb has to be
> >> cleaned up and almost impossible for someone who doesn't know
> >> every aspect of every network subsystem in the kernel. That's
> >> why I went with the more or less bulletproof approach of cleaning
> >> up while actually deferring. I can try and test the code you
> >> proposed in the other reply, but at least, I think, we need a
> >> bunch of debug warnings in the skb_attempt_defer_free() to catch
> >> possible issues.
> >
> > Debug warnings are expensive if they need to bring new cache lines.
> >
> > So far skb_attempt_defer_free() is only used by TCP in well known conditions.
>
> That's true for skb_attempt_defer_free() itself, but it's
> hard to tell the same for all the parts of the code that
> are enqueuing these skbuffs. Even if all the places are
> well known, it looks to me highly error prone. Having
> a couple of DEBUG_NET_WARN_ON_ONCE should not cause any
> performance issues, IIUC, unless debug is enabled, right?
>
> e.g.
>
> DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
> DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
Sure, but we need to put them at the right place.
Doing these checks in skb_attempt_defer_free() is too late.
>
> >
> >
> >>
> >> Also, what about cleaning up extensions? IIUC, at least one
> >> of them can hold external references. SKB_EXT_SEC_PATH holds
> >> xfrm_state. We should, probably, free them as well?
> >
> > I do not know about this, I would ask XFRM maintainers
>
> @Steffen, @Herbert, what do you think? Is it a problem if the
> skb holding SKB_EXT_SEC_PATH extension is held not freed in
> a defer list indefinitely (for a very long time)? Or is it
> even possible for an skb with SKB_EXT_SEC_PATH extension present
> to be enqueued to a TCP receive queue without releasing all
> the references?
>
> This seems like a long existing problem though, so can be fixed
> separately, if it is a problem.
>
> >
> >>
> >> And what about zerocopy skb? I think, we should still not
> >> allow them to be deferred as they seem to hold HW resources.
> >
> > The point of skb_attempt_defer_free() i is to make the freeing happen
> > at producer
> > much instead of consumer.
> >
> > I do not think there is anything special in this regard with zero
> > copy. I would leave the current code as is.
>
> The problem is that these skbuffs are never actually freed,
> so if they do hold HW resources, these resources will never be
> released (never == for a very long time, hours, days).
This is fine. Number of such skbs per cpu is limited.
Or if some HW feature needs buffers to be released _immediately_, we
have to implement
a method for doing so.
Please point why buffers sitting in TCP receive queues are not causing issues ?
Do we need a general method "Please scan all TCP sockets and release
skb holding HW resources immediately"
>
> Not a blocker from my point of view, just trying to highlight
> a possible issue.
The tradeoff is known really, as for any cache.
>
> >
> > A simpler patch might be to move the existing nf_reset_ct() earlier,
> > can you test this ?
>
> I tested the patch below and it seems to fix the issue seen
> with OVS testsuite. Though it's not obvious for me why this
> happens. Can you explain a bit more?
I think the bug predates my recent change.
skb_attempt_defer_free() is increasing the probability of hitting the bug.
>
> >
> > I note that IPv6 does the nf_reset_ct() earlier, from ip6_protocol_deliver_rcu()
> >
> > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > index fda811a5251f2d76ac24a036e6b4f4e7d7d96d6f..a06464f96fe0cc94dd78272738ddaab2c19e87db
> > 100644
> > --- a/net/ipv4/tcp_ipv4.c
> > +++ b/net/ipv4/tcp_ipv4.c
> > @@ -1919,6 +1919,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > struct sock *sk;
> > int ret;
> >
> > + nf_reset_ct(skb);
> > +
> > drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> > if (skb->pkt_type != PACKET_HOST)
> > goto discard_it;
> > @@ -2046,8 +2048,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > if (drop_reason)
> > goto discard_and_relse;
> >
> > - nf_reset_ct(skb);
> > -
> > if (tcp_filter(sk, skb)) {
> > drop_reason = SKB_DROP_REASON_SOCKET_FILTER;
> > goto discard_and_relse;
>
On Wed, Jun 22, 2022 at 4:26 PM Ilya Maximets <[email protected]> wrote:
>
> On 6/22/22 13:43, Eric Dumazet wrote:
>
> I tested the patch below and it seems to fix the issue seen
> with OVS testsuite. Though it's not obvious for me why this
> happens. Can you explain a bit more?
Anyway, I am not sure we can call nf_reset_ct(skb) that early.
git log seems to say that xfrm check needs to be done before
nf_reset_ct(skb), I have no idea why.
I suspect some incoming packets are not going through
xfrm4_policy_check() and end up being stored in a TCP receive queue.
Maybe something is missing before calling tcp_child_process()
>
> >
> > I note that IPv6 does the nf_reset_ct() earlier, from ip6_protocol_deliver_rcu()
> >
> > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > index fda811a5251f2d76ac24a036e6b4f4e7d7d96d6f..a06464f96fe0cc94dd78272738ddaab2c19e87db
> > 100644
> > --- a/net/ipv4/tcp_ipv4.c
> > +++ b/net/ipv4/tcp_ipv4.c
> > @@ -1919,6 +1919,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > struct sock *sk;
> > int ret;
> >
> > + nf_reset_ct(skb);
> > +
> > drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> > if (skb->pkt_type != PACKET_HOST)
> > goto discard_it;
> > @@ -2046,8 +2048,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > if (drop_reason)
> > goto discard_and_relse;
> >
> > - nf_reset_ct(skb);
> > -
> > if (tcp_filter(sk, skb)) {
> > drop_reason = SKB_DROP_REASON_SOCKET_FILTER;
> > goto discard_and_relse;
>
On Wed, Jun 22, 2022 at 6:29 PM Eric Dumazet <[email protected]> wrote:
>
> On Wed, Jun 22, 2022 at 4:26 PM Ilya Maximets <[email protected]> wrote:
> >
> > On 6/22/22 13:43, Eric Dumazet wrote:
>
> >
> > I tested the patch below and it seems to fix the issue seen
> > with OVS testsuite. Though it's not obvious for me why this
> > happens. Can you explain a bit more?
>
> Anyway, I am not sure we can call nf_reset_ct(skb) that early.
>
> git log seems to say that xfrm check needs to be done before
> nf_reset_ct(skb), I have no idea why.
Additional remark: In IPv6 side, xfrm6_policy_check() _is_ called
after nf_reset_ct(skb)
Steffen, do you have some comments ?
Some context:
commit b59c270104f03960069596722fea70340579244d
Author: Patrick McHardy <[email protected]>
Date: Fri Jan 6 23:06:10 2006 -0800
[NETFILTER]: Keep conntrack reference until IPsec policy checks are done
Keep the conntrack reference until policy checks have been performed for
IPsec NAT support. The reference needs to be dropped before a packet is
queued to avoid having the conntrack module unloadable.
Signed-off-by: Patrick McHardy <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
>
> I suspect some incoming packets are not going through
> xfrm4_policy_check() and end up being stored in a TCP receive queue.
>
> Maybe something is missing before calling tcp_child_process()
>
>
> >
> > >
> > > I note that IPv6 does the nf_reset_ct() earlier, from ip6_protocol_deliver_rcu()
> > >
> > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > > index fda811a5251f2d76ac24a036e6b4f4e7d7d96d6f..a06464f96fe0cc94dd78272738ddaab2c19e87db
> > > 100644
> > > --- a/net/ipv4/tcp_ipv4.c
> > > +++ b/net/ipv4/tcp_ipv4.c
> > > @@ -1919,6 +1919,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > > struct sock *sk;
> > > int ret;
> > >
> > > + nf_reset_ct(skb);
> > > +
> > > drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> > > if (skb->pkt_type != PACKET_HOST)
> > > goto discard_it;
> > > @@ -2046,8 +2048,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > > if (drop_reason)
> > > goto discard_and_relse;
> > >
> > > - nf_reset_ct(skb);
> > > -
> > > if (tcp_filter(sk, skb)) {
> > > drop_reason = SKB_DROP_REASON_SOCKET_FILTER;
> > > goto discard_and_relse;
> >
On Wed, Jun 22, 2022 at 6:39 PM Eric Dumazet <[email protected]> wrote:
>
> On Wed, Jun 22, 2022 at 6:29 PM Eric Dumazet <[email protected]> wrote:
> >
> > On Wed, Jun 22, 2022 at 4:26 PM Ilya Maximets <[email protected]> wrote:
> > >
> > > On 6/22/22 13:43, Eric Dumazet wrote:
> >
> > >
> > > I tested the patch below and it seems to fix the issue seen
> > > with OVS testsuite. Though it's not obvious for me why this
> > > happens. Can you explain a bit more?
> >
> > Anyway, I am not sure we can call nf_reset_ct(skb) that early.
> >
> > git log seems to say that xfrm check needs to be done before
> > nf_reset_ct(skb), I have no idea why.
>
> Additional remark: In IPv6 side, xfrm6_policy_check() _is_ called
> after nf_reset_ct(skb)
>
> Steffen, do you have some comments ?
>
> Some context:
> commit b59c270104f03960069596722fea70340579244d
> Author: Patrick McHardy <[email protected]>
> Date: Fri Jan 6 23:06:10 2006 -0800
>
> [NETFILTER]: Keep conntrack reference until IPsec policy checks are done
>
> Keep the conntrack reference until policy checks have been performed for
> IPsec NAT support. The reference needs to be dropped before a packet is
> queued to avoid having the conntrack module unloadable.
>
> Signed-off-by: Patrick McHardy <[email protected]>
> Signed-off-by: David S. Miller <[email protected]>
>
Oh well... __xfrm_policy_check() has :
nf_nat_decode_session(skb, &fl, family);
This answers my questions.
This means we are probably missing at least one XFRM check in TCP
stack in some cases.
(Only after adding this XFRM check we can call nf_reset_ct(skb))
>
> >
> > I suspect some incoming packets are not going through
> > xfrm4_policy_check() and end up being stored in a TCP receive queue.
> >
> > Maybe something is missing before calling tcp_child_process()
> >
> >
> > >
> > > >
> > > > I note that IPv6 does the nf_reset_ct() earlier, from ip6_protocol_deliver_rcu()
> > > >
> > > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > > > index fda811a5251f2d76ac24a036e6b4f4e7d7d96d6f..a06464f96fe0cc94dd78272738ddaab2c19e87db
> > > > 100644
> > > > --- a/net/ipv4/tcp_ipv4.c
> > > > +++ b/net/ipv4/tcp_ipv4.c
> > > > @@ -1919,6 +1919,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > > > struct sock *sk;
> > > > int ret;
> > > >
> > > > + nf_reset_ct(skb);
> > > > +
> > > > drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> > > > if (skb->pkt_type != PACKET_HOST)
> > > > goto discard_it;
> > > > @@ -2046,8 +2048,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > > > if (drop_reason)
> > > > goto discard_and_relse;
> > > >
> > > > - nf_reset_ct(skb);
> > > > -
> > > > if (tcp_filter(sk, skb)) {
> > > > drop_reason = SKB_DROP_REASON_SOCKET_FILTER;
> > > > goto discard_and_relse;
> > >
On Wed, Jun 22, 2022 at 6:47 PM Eric Dumazet <[email protected]> wrote:
>
> On Wed, Jun 22, 2022 at 6:39 PM Eric Dumazet <[email protected]> wrote:
> >
> > On Wed, Jun 22, 2022 at 6:29 PM Eric Dumazet <[email protected]> wrote:
> > >
> > > On Wed, Jun 22, 2022 at 4:26 PM Ilya Maximets <[email protected]> wrote:
> > > >
> > > > On 6/22/22 13:43, Eric Dumazet wrote:
> > >
> > > >
> > > > I tested the patch below and it seems to fix the issue seen
> > > > with OVS testsuite. Though it's not obvious for me why this
> > > > happens. Can you explain a bit more?
> > >
> > > Anyway, I am not sure we can call nf_reset_ct(skb) that early.
> > >
> > > git log seems to say that xfrm check needs to be done before
> > > nf_reset_ct(skb), I have no idea why.
> >
> > Additional remark: In IPv6 side, xfrm6_policy_check() _is_ called
> > after nf_reset_ct(skb)
> >
> > Steffen, do you have some comments ?
> >
> > Some context:
> > commit b59c270104f03960069596722fea70340579244d
> > Author: Patrick McHardy <[email protected]>
> > Date: Fri Jan 6 23:06:10 2006 -0800
> >
> > [NETFILTER]: Keep conntrack reference until IPsec policy checks are done
> >
> > Keep the conntrack reference until policy checks have been performed for
> > IPsec NAT support. The reference needs to be dropped before a packet is
> > queued to avoid having the conntrack module unloadable.
> >
> > Signed-off-by: Patrick McHardy <[email protected]>
> > Signed-off-by: David S. Miller <[email protected]>
> >
>
> Oh well... __xfrm_policy_check() has :
>
> nf_nat_decode_session(skb, &fl, family);
>
> This answers my questions.
>
> This means we are probably missing at least one XFRM check in TCP
> stack in some cases.
> (Only after adding this XFRM check we can call nf_reset_ct(skb))
>
Maybe this will help ?
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index fe8f23b95d32ca4a35d05166d471327bc608fa91..49c1348e40b6c7b6a98b54d716f29c948e00ba33
100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2019,12 +2019,19 @@ int tcp_v4_rcv(struct sk_buff *skb)
if (nsk == sk) {
reqsk_put(req);
tcp_v4_restore_cb(skb);
- } else if (tcp_child_process(sk, nsk, skb)) {
- tcp_v4_send_reset(nsk, skb);
- goto discard_and_relse;
} else {
- sock_put(sk);
- return 0;
+ if (!xfrm4_policy_check(nsk, XFRM_POLICY_IN, skb)) {
+ drop_reason = SKB_DROP_REASON_XFRM_POLICY;
+ goto discard_and_relse;
+ }
+ nf_reset_ct(skb);
+ if (tcp_child_process(sk, nsk, skb)) {
+ tcp_v4_send_reset(nsk, skb);
+ goto discard_and_relse;
+ } else {
+ sock_put(sk);
+ return 0;
+ }
}
}
On 6/22/22 16:35, Eric Dumazet wrote:
> On Wed, Jun 22, 2022 at 4:26 PM Ilya Maximets <[email protected]> wrote:
>>
>> On 6/22/22 13:43, Eric Dumazet wrote:
>>> On Wed, Jun 22, 2022 at 1:32 PM Ilya Maximets <[email protected]> wrote:
>>>>
>>>> On 6/22/22 12:36, Eric Dumazet wrote:
>>>>> On Wed, Jun 22, 2022 at 12:28 PM Florian Westphal <[email protected]> wrote:
>>>>>>
>>>>>> Eric Dumazet <[email protected]> wrote:
>>>>>>> On Sun, Jun 19, 2022 at 2:39 AM Ilya Maximets <[email protected]> wrote:
>>>>>>>>
>>>>>>>> Open vSwitch system test suite is broken due to inability to
>>>>>>>> load/unload netfilter modules. kworker thread is getting trapped
>>>>>>>> in the infinite loop while running a net cleanup inside the
>>>>>>>> nf_conntrack_cleanup_net_list, because deferred skbuffs are still
>>>>>>>> holding nfct references and not being freed by their CPU cores.
>>>>>>>>
>>>>>>>> In general, the idea that we will have an rx interrupt on every
>>>>>>>> CPU core at some point in a near future doesn't seem correct.
>>>>>>>> Devices are getting created and destroyed, interrupts are getting
>>>>>>>> re-scheduled, CPUs are going online and offline dynamically.
>>>>>>>> Any of these events may leave packets stuck in defer list for a
>>>>>>>> long time. It might be OK, if they are just a piece of memory,
>>>>>>>> but we can't afford them holding references to any other resources.
>>>>>>>>
>>>>>>>> In case of OVS, nfct reference keeps the kernel thread in busy loop
>>>>>>>> while holding a 'pernet_ops_rwsem' semaphore. That blocks the
>>>>>>>> later modprobe request from user space:
>>>>>>>>
>>>>>>>> # ps
>>>>>>>> 299 root R 99.3 200:25.89 kworker/u96:4+
>>>>>>>>
>>>>>>>> # journalctl
>>>>>>>> INFO: task modprobe:11787 blocked for more than 1228 seconds.
>>>>>>>> Not tainted 5.19.0-rc2 #8
>>>>>>>> task:modprobe state:D
>>>>>>>> Call Trace:
>>>>>>>> <TASK>
>>>>>>>> __schedule+0x8aa/0x21d0
>>>>>>>> schedule+0xcc/0x200
>>>>>>>> rwsem_down_write_slowpath+0x8e4/0x1580
>>>>>>>> down_write+0xfc/0x140
>>>>>>>> register_pernet_subsys+0x15/0x40
>>>>>>>> nf_nat_init+0xb6/0x1000 [nf_nat]
>>>>>>>> do_one_initcall+0xbb/0x410
>>>>>>>> do_init_module+0x1b4/0x640
>>>>>>>> load_module+0x4c1b/0x58d0
>>>>>>>> __do_sys_init_module+0x1d7/0x220
>>>>>>>> do_syscall_64+0x3a/0x80
>>>>>>>> entry_SYSCALL_64_after_hwframe+0x46/0xb0
>>>>>>>>
>>>>>>>> At this point OVS testsuite is unresponsive and never recover,
>>>>>>>> because these skbuffs are never freed.
>>>>>>>>
>>>>>>>> Solution is to make sure no external references attached to skb
>>>>>>>> before pushing it to the defer list. Using skb_release_head_state()
>>>>>>>> for that purpose. The function modified to be re-enterable, as it
>>>>>>>> will be called again during the defer list flush.
>>>>>>>>
>>>>>>>> Another approach that can fix the OVS use-case, is to kick all
>>>>>>>> cores while waiting for references to be released during the net
>>>>>>>> cleanup. But that sounds more like a workaround for a current
>>>>>>>> issue rather than a proper solution and will not cover possible
>>>>>>>> issues in other parts of the code.
>>>>>>>>
>>>>>>>> Additionally checking for skb_zcopy() while deferring. This might
>>>>>>>> not be necessary, as I'm not sure if we can actually have zero copy
>>>>>>>> packets on this path, but seems worth having for completeness as we
>>>>>>>> should never defer such packets regardless.
>>>>>>>>
>>>>>>>> CC: Eric Dumazet <[email protected]>
>>>>>>>> Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
>>>>>>>> Signed-off-by: Ilya Maximets <[email protected]>
>>>>>>>> ---
>>>>>>>> net/core/skbuff.c | 16 +++++++++++-----
>>>>>>>> 1 file changed, 11 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> I do not think this patch is doing the right thing.
>>>>>>>
>>>>>>> Packets sitting in TCP receive queues should not hold state that is
>>>>>>> not relevant for TCP recvmsg().
>>>>>>
>>>>>> Agree, but tcp_v4/6_rcv() already call nf_reset_ct(), else it would
>>>>>> not be possible to remove nf_conntrack module in practice.
>>>>>
>>>>> Well, existing nf_reset_ct() does not catch all cases, like TCP fastopen ?
>>>>
>>>> Yeah, that is kind of the main problem I have with the current
>>>> code. It's very hard to find all the cases where skb has to be
>>>> cleaned up and almost impossible for someone who doesn't know
>>>> every aspect of every network subsystem in the kernel. That's
>>>> why I went with the more or less bulletproof approach of cleaning
>>>> up while actually deferring. I can try and test the code you
>>>> proposed in the other reply, but at least, I think, we need a
>>>> bunch of debug warnings in the skb_attempt_defer_free() to catch
>>>> possible issues.
>>>
>>> Debug warnings are expensive if they need to bring new cache lines.
>>>
>>> So far skb_attempt_defer_free() is only used by TCP in well known conditions.
>>
>> That's true for skb_attempt_defer_free() itself, but it's
>> hard to tell the same for all the parts of the code that
>> are enqueuing these skbuffs. Even if all the places are
>> well known, it looks to me highly error prone. Having
>> a couple of DEBUG_NET_WARN_ON_ONCE should not cause any
>> performance issues, IIUC, unless debug is enabled, right?
>>
>> e.g.
>>
>> DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
>> DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
>
> Sure, but we need to put them at the right place.
>
> Doing these checks in skb_attempt_defer_free() is too late.
Yes, I agree that it doesn't give any useful information for
debugging, because we need to know who put those packets in
the queue, not who took them out. But 'not too late' places
should already have calls to reset these fields in a close
proximity to where the warnings should be, so it's hard to
tell if that will make a lot of sense. What I had in mind is
just a red flag that says that something went wrong, so it
can be raised while testing new changes to the kernel and
running with debug enabled. That can save some debugging and
bisecting time in the future if some unexpected nfct or dst
reference will sneak into the queue.
But I will not insist on adding these, if you think they are
not valuable.
>
>>
>>>
>>>
>>>>
>>>> Also, what about cleaning up extensions? IIUC, at least one
>>>> of them can hold external references. SKB_EXT_SEC_PATH holds
>>>> xfrm_state. We should, probably, free them as well?
>>>
>>> I do not know about this, I would ask XFRM maintainers
>>
>> @Steffen, @Herbert, what do you think? Is it a problem if the
>> skb holding SKB_EXT_SEC_PATH extension is held not freed in
>> a defer list indefinitely (for a very long time)? Or is it
>> even possible for an skb with SKB_EXT_SEC_PATH extension present
>> to be enqueued to a TCP receive queue without releasing all
>> the references?
To answer my own questions:
- Yes, it's possible.
- No, doesn't seem to be a big problem, IIUC, as xfrm_state
seem to be self-contained and keeping it around for a long
time doesn't seem to be harmful.
Some input from xfrm maintainers would still be great though.
>>
>> This seems like a long existing problem though, so can be fixed
>> separately, if it is a problem.
>>
>>>
>>>>
>>>> And what about zerocopy skb? I think, we should still not
>>>> allow them to be deferred as they seem to hold HW resources.
>>>
>>> The point of skb_attempt_defer_free() i is to make the freeing happen
>>> at producer
>>> much instead of consumer.
>>>
>>> I do not think there is anything special in this regard with zero
>>> copy. I would leave the current code as is.
>>
>> The problem is that these skbuffs are never actually freed,
>> so if they do hold HW resources, these resources will never be
>> released (never == for a very long time, hours, days).
>
> This is fine. Number of such skbs per cpu is limited.
>
> Or if some HW feature needs buffers to be released _immediately_, we
> have to implement
> a method for doing so.
>
> Please point why buffers sitting in TCP receive queues are not causing issues ?
> Do we need a general method "Please scan all TCP sockets and release
> skb holding HW resources immediately"
That's a good point. I agree that it's probably not a big deal
in this case.
>
>>
>> Not a blocker from my point of view, just trying to highlight
>> a possible issue.
>
> The tradeoff is known really, as for any cache.
>
>>
>>>
>>> A simpler patch might be to move the existing nf_reset_ct() earlier,
>>> can you test this ?
>>
>> I tested the patch below and it seems to fix the issue seen
>> with OVS testsuite. Though it's not obvious for me why this
>> happens. Can you explain a bit more?
>
> I think the bug predates my recent change.
>
> skb_attempt_defer_free() is increasing the probability of hitting the bug.
>
>>
>>>
>>> I note that IPv6 does the nf_reset_ct() earlier, from ip6_protocol_deliver_rcu()
>>>
>>> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
>>> index fda811a5251f2d76ac24a036e6b4f4e7d7d96d6f..a06464f96fe0cc94dd78272738ddaab2c19e87db
>>> 100644
>>> --- a/net/ipv4/tcp_ipv4.c
>>> +++ b/net/ipv4/tcp_ipv4.c
>>> @@ -1919,6 +1919,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>> struct sock *sk;
>>> int ret;
>>>
>>> + nf_reset_ct(skb);
>>> +
>>> drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
>>> if (skb->pkt_type != PACKET_HOST)
>>> goto discard_it;
>>> @@ -2046,8 +2048,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>> if (drop_reason)
>>> goto discard_and_relse;
>>>
>>> - nf_reset_ct(skb);
>>> -
>>> if (tcp_filter(sk, skb)) {
>>> drop_reason = SKB_DROP_REASON_SOCKET_FILTER;
>>> goto discard_and_relse;
>>
On 6/22/22 19:03, Eric Dumazet wrote:
> On Wed, Jun 22, 2022 at 6:47 PM Eric Dumazet <[email protected]> wrote:
>>
>> On Wed, Jun 22, 2022 at 6:39 PM Eric Dumazet <[email protected]> wrote:
>>>
>>> On Wed, Jun 22, 2022 at 6:29 PM Eric Dumazet <[email protected]> wrote:
>>>>
>>>> On Wed, Jun 22, 2022 at 4:26 PM Ilya Maximets <[email protected]> wrote:
>>>>>
>>>>> On 6/22/22 13:43, Eric Dumazet wrote:
>>>>
>>>>>
>>>>> I tested the patch below and it seems to fix the issue seen
>>>>> with OVS testsuite. Though it's not obvious for me why this
>>>>> happens. Can you explain a bit more?
>>>>
>>>> Anyway, I am not sure we can call nf_reset_ct(skb) that early.
>>>>
>>>> git log seems to say that xfrm check needs to be done before
>>>> nf_reset_ct(skb), I have no idea why.
>>>
>>> Additional remark: In IPv6 side, xfrm6_policy_check() _is_ called
>>> after nf_reset_ct(skb)
>>>
>>> Steffen, do you have some comments ?
>>>
>>> Some context:
>>> commit b59c270104f03960069596722fea70340579244d
>>> Author: Patrick McHardy <[email protected]>
>>> Date: Fri Jan 6 23:06:10 2006 -0800
>>>
>>> [NETFILTER]: Keep conntrack reference until IPsec policy checks are done
>>>
>>> Keep the conntrack reference until policy checks have been performed for
>>> IPsec NAT support. The reference needs to be dropped before a packet is
>>> queued to avoid having the conntrack module unloadable.
>>>
>>> Signed-off-by: Patrick McHardy <[email protected]>
>>> Signed-off-by: David S. Miller <[email protected]>
>>>
>>
>> Oh well... __xfrm_policy_check() has :
>>
>> nf_nat_decode_session(skb, &fl, family);
>>
>> This answers my questions.
>>
>> This means we are probably missing at least one XFRM check in TCP
>> stack in some cases.
>> (Only after adding this XFRM check we can call nf_reset_ct(skb))
>>
>
> Maybe this will help ?
I tested this patch and it seems to fix the OVS problem.
I did not test the xfrm part of it.
Will you post an official patch?
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index fe8f23b95d32ca4a35d05166d471327bc608fa91..49c1348e40b6c7b6a98b54d716f29c948e00ba33
> 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -2019,12 +2019,19 @@ int tcp_v4_rcv(struct sk_buff *skb)
> if (nsk == sk) {
> reqsk_put(req);
> tcp_v4_restore_cb(skb);
> - } else if (tcp_child_process(sk, nsk, skb)) {
> - tcp_v4_send_reset(nsk, skb);
> - goto discard_and_relse;
> } else {
> - sock_put(sk);
> - return 0;
> + if (!xfrm4_policy_check(nsk, XFRM_POLICY_IN, skb)) {
> + drop_reason = SKB_DROP_REASON_XFRM_POLICY;
> + goto discard_and_relse;
> + }
> + nf_reset_ct(skb);
> + if (tcp_child_process(sk, nsk, skb)) {
> + tcp_v4_send_reset(nsk, skb);
> + goto discard_and_relse;
> + } else {
> + sock_put(sk);
> + return 0;
> + }
> }
> }
On Wed, Jun 22, 2022 at 9:04 PM Eric Dumazet <[email protected]> wrote:
>
> On Wed, Jun 22, 2022 at 8:19 PM Ilya Maximets <[email protected]> wrote:
> >
> > On 6/22/22 19:03, Eric Dumazet wrote:
> > > On Wed, Jun 22, 2022 at 6:47 PM Eric Dumazet <[email protected]> wrote:
> > >>
> > >> On Wed, Jun 22, 2022 at 6:39 PM Eric Dumazet <[email protected]> wrote:
> > >>>
> > >>> On Wed, Jun 22, 2022 at 6:29 PM Eric Dumazet <[email protected]> wrote:
> > >>>>
> > >>>> On Wed, Jun 22, 2022 at 4:26 PM Ilya Maximets <[email protected]> wrote:
> > >>>>>
> > >>>>> On 6/22/22 13:43, Eric Dumazet wrote:
> > >>>>
> > >>>>>
> > >>>>> I tested the patch below and it seems to fix the issue seen
> > >>>>> with OVS testsuite. Though it's not obvious for me why this
> > >>>>> happens. Can you explain a bit more?
> > >>>>
> > >>>> Anyway, I am not sure we can call nf_reset_ct(skb) that early.
> > >>>>
> > >>>> git log seems to say that xfrm check needs to be done before
> > >>>> nf_reset_ct(skb), I have no idea why.
> > >>>
> > >>> Additional remark: In IPv6 side, xfrm6_policy_check() _is_ called
> > >>> after nf_reset_ct(skb)
> > >>>
> > >>> Steffen, do you have some comments ?
> > >>>
> > >>> Some context:
> > >>> commit b59c270104f03960069596722fea70340579244d
> > >>> Author: Patrick McHardy <[email protected]>
> > >>> Date: Fri Jan 6 23:06:10 2006 -0800
> > >>>
> > >>> [NETFILTER]: Keep conntrack reference until IPsec policy checks are done
> > >>>
> > >>> Keep the conntrack reference until policy checks have been performed for
> > >>> IPsec NAT support. The reference needs to be dropped before a packet is
> > >>> queued to avoid having the conntrack module unloadable.
> > >>>
> > >>> Signed-off-by: Patrick McHardy <[email protected]>
> > >>> Signed-off-by: David S. Miller <[email protected]>
> > >>>
> > >>
> > >> Oh well... __xfrm_policy_check() has :
> > >>
> > >> nf_nat_decode_session(skb, &fl, family);
> > >>
> > >> This answers my questions.
> > >>
> > >> This means we are probably missing at least one XFRM check in TCP
> > >> stack in some cases.
> > >> (Only after adding this XFRM check we can call nf_reset_ct(skb))
> > >>
> > >
> > > Maybe this will help ?
> >
> > I tested this patch and it seems to fix the OVS problem.
> > I did not test the xfrm part of it.
> >
> > Will you post an official patch?
>
> Yes I will. I need to double check we do not leak either the req, or the child.
>
> Maybe the XFRM check should be done even earlier, on the listening socket ?
>
> Or if we assume the SYNACK packet has been sent after the XFRM test
> has been applied to the SYN,
> maybe we could just call nf_reset_ct(skb) to lower risk of regressions.
>
> With the last patch, it would be strange that we accept the 3WHS and
> establish a socket,
> but drop the payload in the 3rd packet...
Ilya, can you test the following patch ?
I think it makes more sense to let XFRM reject the packet earlier, and
not complete the 3WHS,
if for some reason this happens.
Thanks !
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index fe8f23b95d32ca4a35d05166d471327bc608fa91..da5a3c44c4fb70f1d3ecc596e694a86267f1c44a
100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1964,7 +1964,10 @@ int tcp_v4_rcv(struct sk_buff *skb)
struct sock *nsk;
sk = req->rsk_listener;
- drop_reason = tcp_inbound_md5_hash(sk, skb,
+ if (!xfrm4_policy_check(sk, XFRM_POLICY_IN, skb))
+ drop_reason = SKB_DROP_REASON_XFRM_POLICY;
+ else
+ drop_reason = tcp_inbound_md5_hash(sk, skb,
&iph->saddr, &iph->daddr,
AF_INET, dif, sdif);
if (unlikely(drop_reason)) {
@@ -2016,6 +2019,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
}
goto discard_and_relse;
}
+ nf_reset_ct(skb);
if (nsk == sk) {
reqsk_put(req);
tcp_v4_restore_cb(skb);
On Wed, Jun 22, 2022 at 8:19 PM Ilya Maximets <[email protected]> wrote:
>
> On 6/22/22 19:03, Eric Dumazet wrote:
> > On Wed, Jun 22, 2022 at 6:47 PM Eric Dumazet <[email protected]> wrote:
> >>
> >> On Wed, Jun 22, 2022 at 6:39 PM Eric Dumazet <[email protected]> wrote:
> >>>
> >>> On Wed, Jun 22, 2022 at 6:29 PM Eric Dumazet <[email protected]> wrote:
> >>>>
> >>>> On Wed, Jun 22, 2022 at 4:26 PM Ilya Maximets <[email protected]> wrote:
> >>>>>
> >>>>> On 6/22/22 13:43, Eric Dumazet wrote:
> >>>>
> >>>>>
> >>>>> I tested the patch below and it seems to fix the issue seen
> >>>>> with OVS testsuite. Though it's not obvious for me why this
> >>>>> happens. Can you explain a bit more?
> >>>>
> >>>> Anyway, I am not sure we can call nf_reset_ct(skb) that early.
> >>>>
> >>>> git log seems to say that xfrm check needs to be done before
> >>>> nf_reset_ct(skb), I have no idea why.
> >>>
> >>> Additional remark: In IPv6 side, xfrm6_policy_check() _is_ called
> >>> after nf_reset_ct(skb)
> >>>
> >>> Steffen, do you have some comments ?
> >>>
> >>> Some context:
> >>> commit b59c270104f03960069596722fea70340579244d
> >>> Author: Patrick McHardy <[email protected]>
> >>> Date: Fri Jan 6 23:06:10 2006 -0800
> >>>
> >>> [NETFILTER]: Keep conntrack reference until IPsec policy checks are done
> >>>
> >>> Keep the conntrack reference until policy checks have been performed for
> >>> IPsec NAT support. The reference needs to be dropped before a packet is
> >>> queued to avoid having the conntrack module unloadable.
> >>>
> >>> Signed-off-by: Patrick McHardy <[email protected]>
> >>> Signed-off-by: David S. Miller <[email protected]>
> >>>
> >>
> >> Oh well... __xfrm_policy_check() has :
> >>
> >> nf_nat_decode_session(skb, &fl, family);
> >>
> >> This answers my questions.
> >>
> >> This means we are probably missing at least one XFRM check in TCP
> >> stack in some cases.
> >> (Only after adding this XFRM check we can call nf_reset_ct(skb))
> >>
> >
> > Maybe this will help ?
>
> I tested this patch and it seems to fix the OVS problem.
> I did not test the xfrm part of it.
>
> Will you post an official patch?
Yes I will. I need to double check we do not leak either the req, or the child.
Maybe the XFRM check should be done even earlier, on the listening socket ?
Or if we assume the SYNACK packet has been sent after the XFRM test
has been applied to the SYN,
maybe we could just call nf_reset_ct(skb) to lower risk of regressions.
With the last patch, it would be strange that we accept the 3WHS and
establish a socket,
but drop the payload in the 3rd packet...
>
> >
> > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > index fe8f23b95d32ca4a35d05166d471327bc608fa91..49c1348e40b6c7b6a98b54d716f29c948e00ba33
> > 100644
> > --- a/net/ipv4/tcp_ipv4.c
> > +++ b/net/ipv4/tcp_ipv4.c
> > @@ -2019,12 +2019,19 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > if (nsk == sk) {
> > reqsk_put(req);
> > tcp_v4_restore_cb(skb);
> > - } else if (tcp_child_process(sk, nsk, skb)) {
> > - tcp_v4_send_reset(nsk, skb);
> > - goto discard_and_relse;
> > } else {
> > - sock_put(sk);
> > - return 0;
> > + if (!xfrm4_policy_check(nsk, XFRM_POLICY_IN, skb)) {
> > + drop_reason = SKB_DROP_REASON_XFRM_POLICY;
> > + goto discard_and_relse;
> > + }
> > + nf_reset_ct(skb);
> > + if (tcp_child_process(sk, nsk, skb)) {
> > + tcp_v4_send_reset(nsk, skb);
> > + goto discard_and_relse;
> > + } else {
> > + sock_put(sk);
> > + return 0;
> > + }
> > }
> > }
>
On 6/22/22 21:27, Eric Dumazet wrote:
> On Wed, Jun 22, 2022 at 9:04 PM Eric Dumazet <[email protected]> wrote:
>>
>> On Wed, Jun 22, 2022 at 8:19 PM Ilya Maximets <[email protected]> wrote:
>>>
>>> On 6/22/22 19:03, Eric Dumazet wrote:
>>>> On Wed, Jun 22, 2022 at 6:47 PM Eric Dumazet <[email protected]> wrote:
>>>>>
>>>>> On Wed, Jun 22, 2022 at 6:39 PM Eric Dumazet <[email protected]> wrote:
>>>>>>
>>>>>> On Wed, Jun 22, 2022 at 6:29 PM Eric Dumazet <[email protected]> wrote:
>>>>>>>
>>>>>>> On Wed, Jun 22, 2022 at 4:26 PM Ilya Maximets <[email protected]> wrote:
>>>>>>>>
>>>>>>>> On 6/22/22 13:43, Eric Dumazet wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> I tested the patch below and it seems to fix the issue seen
>>>>>>>> with OVS testsuite. Though it's not obvious for me why this
>>>>>>>> happens. Can you explain a bit more?
>>>>>>>
>>>>>>> Anyway, I am not sure we can call nf_reset_ct(skb) that early.
>>>>>>>
>>>>>>> git log seems to say that xfrm check needs to be done before
>>>>>>> nf_reset_ct(skb), I have no idea why.
>>>>>>
>>>>>> Additional remark: In IPv6 side, xfrm6_policy_check() _is_ called
>>>>>> after nf_reset_ct(skb)
>>>>>>
>>>>>> Steffen, do you have some comments ?
>>>>>>
>>>>>> Some context:
>>>>>> commit b59c270104f03960069596722fea70340579244d
>>>>>> Author: Patrick McHardy <[email protected]>
>>>>>> Date: Fri Jan 6 23:06:10 2006 -0800
>>>>>>
>>>>>> [NETFILTER]: Keep conntrack reference until IPsec policy checks are done
>>>>>>
>>>>>> Keep the conntrack reference until policy checks have been performed for
>>>>>> IPsec NAT support. The reference needs to be dropped before a packet is
>>>>>> queued to avoid having the conntrack module unloadable.
>>>>>>
>>>>>> Signed-off-by: Patrick McHardy <[email protected]>
>>>>>> Signed-off-by: David S. Miller <[email protected]>
>>>>>>
>>>>>
>>>>> Oh well... __xfrm_policy_check() has :
>>>>>
>>>>> nf_nat_decode_session(skb, &fl, family);
>>>>>
>>>>> This answers my questions.
>>>>>
>>>>> This means we are probably missing at least one XFRM check in TCP
>>>>> stack in some cases.
>>>>> (Only after adding this XFRM check we can call nf_reset_ct(skb))
>>>>>
>>>>
>>>> Maybe this will help ?
>>>
>>> I tested this patch and it seems to fix the OVS problem.
>>> I did not test the xfrm part of it.
>>>
>>> Will you post an official patch?
>>
>> Yes I will. I need to double check we do not leak either the req, or the child.
>>
>> Maybe the XFRM check should be done even earlier, on the listening socket ?
>>
>> Or if we assume the SYNACK packet has been sent after the XFRM test
>> has been applied to the SYN,
>> maybe we could just call nf_reset_ct(skb) to lower risk of regressions.
>>
>> With the last patch, it would be strange that we accept the 3WHS and
>> establish a socket,
>> but drop the payload in the 3rd packet...
>
> Ilya, can you test the following patch ?
Tested with OVS and it works fine, the issue doesn't appear.
Still didn't test the xfrm part, as I'm not sure how.
> I think it makes more sense to let XFRM reject the packet earlier, and
> not complete the 3WHS,
> if for some reason this happens.
OK. However, now the patch looks more like two separate fixes.
>
> Thanks !
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index fe8f23b95d32ca4a35d05166d471327bc608fa91..da5a3c44c4fb70f1d3ecc596e694a86267f1c44a
> 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1964,7 +1964,10 @@ int tcp_v4_rcv(struct sk_buff *skb)
> struct sock *nsk;
>
> sk = req->rsk_listener;
> - drop_reason = tcp_inbound_md5_hash(sk, skb,
> + if (!xfrm4_policy_check(sk, XFRM_POLICY_IN, skb))
> + drop_reason = SKB_DROP_REASON_XFRM_POLICY;
> + else
> + drop_reason = tcp_inbound_md5_hash(sk, skb,
> &iph->saddr, &iph->daddr,
> AF_INET, dif, sdif);
> if (unlikely(drop_reason)) {
> @@ -2016,6 +2019,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
> }
> goto discard_and_relse;
> }
> + nf_reset_ct(skb);
> if (nsk == sk) {
> reqsk_put(req);
> tcp_v4_restore_cb(skb);
On 6/19/22 02:39, Ilya Maximets wrote:
> Open vSwitch system test suite is broken due to inability to
> load/unload netfilter modules. kworker thread is getting trapped
> in the infinite loop while running a net cleanup inside the
> nf_conntrack_cleanup_net_list, because deferred skbuffs are still
> holding nfct references and not being freed by their CPU cores.
>
> In general, the idea that we will have an rx interrupt on every
> CPU core at some point in a near future doesn't seem correct.
> Devices are getting created and destroyed, interrupts are getting
> re-scheduled, CPUs are going online and offline dynamically.
> Any of these events may leave packets stuck in defer list for a
> long time. It might be OK, if they are just a piece of memory,
> but we can't afford them holding references to any other resources.
>
> In case of OVS, nfct reference keeps the kernel thread in busy loop
> while holding a 'pernet_ops_rwsem' semaphore. That blocks the
> later modprobe request from user space:
>
> # ps
> 299 root R 99.3 200:25.89 kworker/u96:4+
>
> # journalctl
> INFO: task modprobe:11787 blocked for more than 1228 seconds.
> Not tainted 5.19.0-rc2 #8
> task:modprobe state:D
> Call Trace:
> <TASK>
> __schedule+0x8aa/0x21d0
> schedule+0xcc/0x200
> rwsem_down_write_slowpath+0x8e4/0x1580
> down_write+0xfc/0x140
> register_pernet_subsys+0x15/0x40
> nf_nat_init+0xb6/0x1000 [nf_nat]
> do_one_initcall+0xbb/0x410
> do_init_module+0x1b4/0x640
> load_module+0x4c1b/0x58d0
> __do_sys_init_module+0x1d7/0x220
> do_syscall_64+0x3a/0x80
> entry_SYSCALL_64_after_hwframe+0x46/0xb0
>
> At this point OVS testsuite is unresponsive and never recover,
> because these skbuffs are never freed.
>
> Solution is to make sure no external references attached to skb
> before pushing it to the defer list. Using skb_release_head_state()
> for that purpose. The function modified to be re-enterable, as it
> will be called again during the defer list flush.
>
> Another approach that can fix the OVS use-case, is to kick all
> cores while waiting for references to be released during the net
> cleanup. But that sounds more like a workaround for a current
> issue rather than a proper solution and will not cover possible
> issues in other parts of the code.
>
> Additionally checking for skb_zcopy() while deferring. This might
> not be necessary, as I'm not sure if we can actually have zero copy
> packets on this path, but seems worth having for completeness as we
> should never defer such packets regardless.
>
> CC: Eric Dumazet <[email protected]>
> Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
Happy Friday! :)
Resurrecting this thread because I managed to reproduce the issue again
on a latest 6.6.0-rc1.
(It doesn't mean we need to accept this particular patch, I just think
that it's an appropriate discussion thread.)
It's a bit different testsuite this time. Last year I had an issue with
OVS testsuite, today I have an issue with OVN testsuite. Their structure
is very similar, but OVN tests are a fair bit more complex.
The story is the same: Each test loads a pack of kernel modules including
OVS and nf_conntrack, sends some traffic, verifies OVN functionality,
stops OVN/OVS and unloads all the previously loaded modules to clean up
the space for the next tests.
Kernel hangs in the same way as before waiting for nf_conntrack module
to be unloaded:
13 root R 100.0 933:17.98 kworker/u80:1+netns
CPU: 12 PID: 13 Comm: kworker/u80:1 Not tainted 6.6.0-rc1+ #7
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.1-2.fc36 04/01/2014
Workqueue: netns cleanup_net
RIP: 0010:nf_ct_iterate_cleanup+0x35/0x1e0 [nf_conntrack]
Code: 56 41 55 41 54 49 89 fc 55 31 ed 53 e8 a4 db a8
ed 48 c7 c7 20 e9 e2 c0 e8 48 f3 a8 ed 39 2d 7e
2d 01 00 77 14 e9 05 01 00 00 <83> c5 01 3b 2d 6e
2d 01 00 0f 83 f6 00 00 00 48 8b 15 75 2d 01 00
RSP: 0018:ffffb23040073d98 EFLAGS: 00000202
RAX: 000000000001ce57 RBX: ffff98dbfac73958 RCX: ffff98e31f732b38
RDX: ffff98dbfac00000 RSI: ffffb23040073dd0 RDI: ffffffffc0e2e920
RBP: 000000000000e72b R08: ffff98e31f732b38 R09: ffff98e31f732b38
R10: 000000000001d5ce R11: 0000000000000000 R12: ffffffffc0e1b080
R13: ffffb23040073e28 R14: ffff98dbc47b0600 R15: ffffb23040073dd0
FS: 0000000000000000(0000) GS:ffff98e31f700000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f98bd7bca80 CR3: 000000076f420004 CR4: 0000000000370ee0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<NMI>
? nmi_cpu_backtrace+0x82/0xf0
? nmi_cpu_backtrace_handler+0xd/0x20
? nmi_handle+0x5e/0x150
? default_do_nmi+0x40/0x100
? exc_nmi+0x112/0x190
? end_repeat_nmi+0x16/0x67
? __pfx_kill_all+0x10/0x10 [nf_conntrack]
? nf_ct_iterate_cleanup+0x35/0x1e0 [nf_conntrack]
? nf_ct_iterate_cleanup+0x35/0x1e0 [nf_conntrack]
? nf_ct_iterate_cleanup+0x35/0x1e0 [nf_conntrack]
</NMI>
<TASK>
nf_conntrack_cleanup_net_list+0xac/0x140 [nf_conntrack]
cleanup_net+0x213/0x3b0
process_one_work+0x177/0x340
worker_thread+0x27e/0x390
? __pfx_worker_thread+0x10/0x10
kthread+0xe2/0x110
? __pfx_kthread+0x10/0x10
ret_from_fork+0x30/0x50
? __pfx_kthread+0x10/0x10
ret_from_fork_asm+0x1b/0x30
</TASK>
Since this testsuite is different from what I had issues with before,
I don't really know when the issue first appeared. It may have been
there even last year.
I can reproduce the issue with ubuntu 6.2.0-32-generic kernel, but
I don't see it with the latest 5.14.0-284.25.1.el9_2.x86_64 on RHEL9.
It doesn't necessarily mean that RHEL9 doesn't have the issue though,
the testsuite is not 100% reliable.
I'll try to dig deeper and bisect the problem on the upstream kernel.
For now it seems like the issue is likely in IPv6 code, because the
tests that trigger it are mostly IPv6-related.
Best regards, Ilya Maximets.
On 9/22/23 15:26, Ilya Maximets wrote:
> On 6/19/22 02:39, Ilya Maximets wrote:
>> Open vSwitch system test suite is broken due to inability to
>> load/unload netfilter modules. kworker thread is getting trapped
>> in the infinite loop while running a net cleanup inside the
>> nf_conntrack_cleanup_net_list, because deferred skbuffs are still
>> holding nfct references and not being freed by their CPU cores.
>>
>> In general, the idea that we will have an rx interrupt on every
>> CPU core at some point in a near future doesn't seem correct.
>> Devices are getting created and destroyed, interrupts are getting
>> re-scheduled, CPUs are going online and offline dynamically.
>> Any of these events may leave packets stuck in defer list for a
>> long time. It might be OK, if they are just a piece of memory,
>> but we can't afford them holding references to any other resources.
>>
>> In case of OVS, nfct reference keeps the kernel thread in busy loop
>> while holding a 'pernet_ops_rwsem' semaphore. That blocks the
>> later modprobe request from user space:
>>
>> # ps
>> 299 root R 99.3 200:25.89 kworker/u96:4+
>>
>> # journalctl
>> INFO: task modprobe:11787 blocked for more than 1228 seconds.
>> Not tainted 5.19.0-rc2 #8
>> task:modprobe state:D
>> Call Trace:
>> <TASK>
>> __schedule+0x8aa/0x21d0
>> schedule+0xcc/0x200
>> rwsem_down_write_slowpath+0x8e4/0x1580
>> down_write+0xfc/0x140
>> register_pernet_subsys+0x15/0x40
>> nf_nat_init+0xb6/0x1000 [nf_nat]
>> do_one_initcall+0xbb/0x410
>> do_init_module+0x1b4/0x640
>> load_module+0x4c1b/0x58d0
>> __do_sys_init_module+0x1d7/0x220
>> do_syscall_64+0x3a/0x80
>> entry_SYSCALL_64_after_hwframe+0x46/0xb0
>>
>> At this point OVS testsuite is unresponsive and never recover,
>> because these skbuffs are never freed.
>>
>> Solution is to make sure no external references attached to skb
>> before pushing it to the defer list. Using skb_release_head_state()
>> for that purpose. The function modified to be re-enterable, as it
>> will be called again during the defer list flush.
>>
>> Another approach that can fix the OVS use-case, is to kick all
>> cores while waiting for references to be released during the net
>> cleanup. But that sounds more like a workaround for a current
>> issue rather than a proper solution and will not cover possible
>> issues in other parts of the code.
>>
>> Additionally checking for skb_zcopy() while deferring. This might
>> not be necessary, as I'm not sure if we can actually have zero copy
>> packets on this path, but seems worth having for completeness as we
>> should never defer such packets regardless.
>>
>> CC: Eric Dumazet <[email protected]>
>> Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
>> Signed-off-by: Ilya Maximets <[email protected]>
>> ---
>
> Happy Friday! :)
>
> Resurrecting this thread because I managed to reproduce the issue again
> on a latest 6.6.0-rc1.
>
> (It doesn't mean we need to accept this particular patch, I just think
> that it's an appropriate discussion thread.)
>
> It's a bit different testsuite this time. Last year I had an issue with
> OVS testsuite, today I have an issue with OVN testsuite. Their structure
> is very similar, but OVN tests are a fair bit more complex.
>
> The story is the same: Each test loads a pack of kernel modules including
> OVS and nf_conntrack, sends some traffic, verifies OVN functionality,
> stops OVN/OVS and unloads all the previously loaded modules to clean up
> the space for the next tests.
>
> Kernel hangs in the same way as before waiting for nf_conntrack module
> to be unloaded:
>
> 13 root R 100.0 933:17.98 kworker/u80:1+netns
>
> CPU: 12 PID: 13 Comm: kworker/u80:1 Not tainted 6.6.0-rc1+ #7
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.1-2.fc36 04/01/2014
> Workqueue: netns cleanup_net
> RIP: 0010:nf_ct_iterate_cleanup+0x35/0x1e0 [nf_conntrack]
> Code: 56 41 55 41 54 49 89 fc 55 31 ed 53 e8 a4 db a8
> ed 48 c7 c7 20 e9 e2 c0 e8 48 f3 a8 ed 39 2d 7e
> 2d 01 00 77 14 e9 05 01 00 00 <83> c5 01 3b 2d 6e
> 2d 01 00 0f 83 f6 00 00 00 48 8b 15 75 2d 01 00
> RSP: 0018:ffffb23040073d98 EFLAGS: 00000202
> RAX: 000000000001ce57 RBX: ffff98dbfac73958 RCX: ffff98e31f732b38
> RDX: ffff98dbfac00000 RSI: ffffb23040073dd0 RDI: ffffffffc0e2e920
> RBP: 000000000000e72b R08: ffff98e31f732b38 R09: ffff98e31f732b38
> R10: 000000000001d5ce R11: 0000000000000000 R12: ffffffffc0e1b080
> R13: ffffb23040073e28 R14: ffff98dbc47b0600 R15: ffffb23040073dd0
> FS: 0000000000000000(0000) GS:ffff98e31f700000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f98bd7bca80 CR3: 000000076f420004 CR4: 0000000000370ee0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <NMI>
> ? nmi_cpu_backtrace+0x82/0xf0
> ? nmi_cpu_backtrace_handler+0xd/0x20
> ? nmi_handle+0x5e/0x150
> ? default_do_nmi+0x40/0x100
> ? exc_nmi+0x112/0x190
> ? end_repeat_nmi+0x16/0x67
> ? __pfx_kill_all+0x10/0x10 [nf_conntrack]
> ? nf_ct_iterate_cleanup+0x35/0x1e0 [nf_conntrack]
> ? nf_ct_iterate_cleanup+0x35/0x1e0 [nf_conntrack]
> ? nf_ct_iterate_cleanup+0x35/0x1e0 [nf_conntrack]
> </NMI>
> <TASK>
> nf_conntrack_cleanup_net_list+0xac/0x140 [nf_conntrack]
> cleanup_net+0x213/0x3b0
> process_one_work+0x177/0x340
> worker_thread+0x27e/0x390
> ? __pfx_worker_thread+0x10/0x10
> kthread+0xe2/0x110
> ? __pfx_kthread+0x10/0x10
> ret_from_fork+0x30/0x50
> ? __pfx_kthread+0x10/0x10
> ret_from_fork_asm+0x1b/0x30
> </TASK>
>
> Since this testsuite is different from what I had issues with before,
> I don't really know when the issue first appeared. It may have been
> there even last year.
>
> I can reproduce the issue with ubuntu 6.2.0-32-generic kernel, but
> I don't see it with the latest 5.14.0-284.25.1.el9_2.x86_64 on RHEL9.
> It doesn't necessarily mean that RHEL9 doesn't have the issue though,
> the testsuite is not 100% reliable.
>
> I'll try to dig deeper and bisect the problem on the upstream kernel.
>
> For now it seems like the issue is likely in IPv6 code, because the
> tests that trigger it are mostly IPv6-related.
So, I bisected this down to an unsurprising commit:
commit b0e214d212030fe497d4d150bb3474e50ad5d093
Author: Madhu Koriginja <[email protected]>
Date: Tue Mar 21 21:28:44 2023 +0530
netfilter: keep conntrack reference until IPsecv6 policy checks are done
Keep the conntrack reference until policy checks have been performed for
IPsec V6 NAT support, just like ipv4.
The reference needs to be dropped before a packet is
queued to avoid having the conntrack module unloadable.
Fixes: 58a317f1061c ("netfilter: ipv6: add IPv6 NAT support")
Signed-off-by: Madhu Koriginja <[email protected]>
Signed-off-by: Florian Westphal <[email protected]>
This commit repeated the pattern from the old ipv4 commit b59c270104f0
("[NETFILTER]: Keep conntrack reference until IPsec policy checks are done")
and hence introduced the exact same issue, but for IPv6. They both failed
to deliver on the "avoid having the conntrack module unloadable" part.
IIUC, the fix should be exactly the same as Eric did for ipv4 in commit
6f0012e35160 ("tcp: add a missing nf_reset_ct() in 3WHS handling").
I can try that and send a fix.
Would still really like to have some preventive mechanism for this kind of
issues though. Any ideas on that?
Best regards, Ilya Maximets.
On 9/22/23 21:07, Ilya Maximets wrote:
> On 9/22/23 15:26, Ilya Maximets wrote:
>> On 6/19/22 02:39, Ilya Maximets wrote:
>>> Open vSwitch system test suite is broken due to inability to
>>> load/unload netfilter modules. kworker thread is getting trapped
>>> in the infinite loop while running a net cleanup inside the
>>> nf_conntrack_cleanup_net_list, because deferred skbuffs are still
>>> holding nfct references and not being freed by their CPU cores.
>>>
>>> In general, the idea that we will have an rx interrupt on every
>>> CPU core at some point in a near future doesn't seem correct.
>>> Devices are getting created and destroyed, interrupts are getting
>>> re-scheduled, CPUs are going online and offline dynamically.
>>> Any of these events may leave packets stuck in defer list for a
>>> long time. It might be OK, if they are just a piece of memory,
>>> but we can't afford them holding references to any other resources.
>>>
>>> In case of OVS, nfct reference keeps the kernel thread in busy loop
>>> while holding a 'pernet_ops_rwsem' semaphore. That blocks the
>>> later modprobe request from user space:
>>>
>>> # ps
>>> 299 root R 99.3 200:25.89 kworker/u96:4+
>>>
>>> # journalctl
>>> INFO: task modprobe:11787 blocked for more than 1228 seconds.
>>> Not tainted 5.19.0-rc2 #8
>>> task:modprobe state:D
>>> Call Trace:
>>> <TASK>
>>> __schedule+0x8aa/0x21d0
>>> schedule+0xcc/0x200
>>> rwsem_down_write_slowpath+0x8e4/0x1580
>>> down_write+0xfc/0x140
>>> register_pernet_subsys+0x15/0x40
>>> nf_nat_init+0xb6/0x1000 [nf_nat]
>>> do_one_initcall+0xbb/0x410
>>> do_init_module+0x1b4/0x640
>>> load_module+0x4c1b/0x58d0
>>> __do_sys_init_module+0x1d7/0x220
>>> do_syscall_64+0x3a/0x80
>>> entry_SYSCALL_64_after_hwframe+0x46/0xb0
>>>
>>> At this point OVS testsuite is unresponsive and never recover,
>>> because these skbuffs are never freed.
>>>
>>> Solution is to make sure no external references attached to skb
>>> before pushing it to the defer list. Using skb_release_head_state()
>>> for that purpose. The function modified to be re-enterable, as it
>>> will be called again during the defer list flush.
>>>
>>> Another approach that can fix the OVS use-case, is to kick all
>>> cores while waiting for references to be released during the net
>>> cleanup. But that sounds more like a workaround for a current
>>> issue rather than a proper solution and will not cover possible
>>> issues in other parts of the code.
>>>
>>> Additionally checking for skb_zcopy() while deferring. This might
>>> not be necessary, as I'm not sure if we can actually have zero copy
>>> packets on this path, but seems worth having for completeness as we
>>> should never defer such packets regardless.
>>>
>>> CC: Eric Dumazet <[email protected]>
>>> Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
>>> Signed-off-by: Ilya Maximets <[email protected]>
>>> ---
>>
>> Happy Friday! :)
>>
>> Resurrecting this thread because I managed to reproduce the issue again
>> on a latest 6.6.0-rc1.
>>
>> (It doesn't mean we need to accept this particular patch, I just think
>> that it's an appropriate discussion thread.)
>>
>> It's a bit different testsuite this time. Last year I had an issue with
>> OVS testsuite, today I have an issue with OVN testsuite. Their structure
>> is very similar, but OVN tests are a fair bit more complex.
>>
>> The story is the same: Each test loads a pack of kernel modules including
>> OVS and nf_conntrack, sends some traffic, verifies OVN functionality,
>> stops OVN/OVS and unloads all the previously loaded modules to clean up
>> the space for the next tests.
>>
>> Kernel hangs in the same way as before waiting for nf_conntrack module
>> to be unloaded:
>>
>> 13 root R 100.0 933:17.98 kworker/u80:1+netns
>>
>> CPU: 12 PID: 13 Comm: kworker/u80:1 Not tainted 6.6.0-rc1+ #7
>> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.1-2.fc36 04/01/2014
>> Workqueue: netns cleanup_net
>> RIP: 0010:nf_ct_iterate_cleanup+0x35/0x1e0 [nf_conntrack]
>> Code: 56 41 55 41 54 49 89 fc 55 31 ed 53 e8 a4 db a8
>> ed 48 c7 c7 20 e9 e2 c0 e8 48 f3 a8 ed 39 2d 7e
>> 2d 01 00 77 14 e9 05 01 00 00 <83> c5 01 3b 2d 6e
>> 2d 01 00 0f 83 f6 00 00 00 48 8b 15 75 2d 01 00
>> RSP: 0018:ffffb23040073d98 EFLAGS: 00000202
>> RAX: 000000000001ce57 RBX: ffff98dbfac73958 RCX: ffff98e31f732b38
>> RDX: ffff98dbfac00000 RSI: ffffb23040073dd0 RDI: ffffffffc0e2e920
>> RBP: 000000000000e72b R08: ffff98e31f732b38 R09: ffff98e31f732b38
>> R10: 000000000001d5ce R11: 0000000000000000 R12: ffffffffc0e1b080
>> R13: ffffb23040073e28 R14: ffff98dbc47b0600 R15: ffffb23040073dd0
>> FS: 0000000000000000(0000) GS:ffff98e31f700000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 00007f98bd7bca80 CR3: 000000076f420004 CR4: 0000000000370ee0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> Call Trace:
>> <NMI>
>> ? nmi_cpu_backtrace+0x82/0xf0
>> ? nmi_cpu_backtrace_handler+0xd/0x20
>> ? nmi_handle+0x5e/0x150
>> ? default_do_nmi+0x40/0x100
>> ? exc_nmi+0x112/0x190
>> ? end_repeat_nmi+0x16/0x67
>> ? __pfx_kill_all+0x10/0x10 [nf_conntrack]
>> ? nf_ct_iterate_cleanup+0x35/0x1e0 [nf_conntrack]
>> ? nf_ct_iterate_cleanup+0x35/0x1e0 [nf_conntrack]
>> ? nf_ct_iterate_cleanup+0x35/0x1e0 [nf_conntrack]
>> </NMI>
>> <TASK>
>> nf_conntrack_cleanup_net_list+0xac/0x140 [nf_conntrack]
>> cleanup_net+0x213/0x3b0
>> process_one_work+0x177/0x340
>> worker_thread+0x27e/0x390
>> ? __pfx_worker_thread+0x10/0x10
>> kthread+0xe2/0x110
>> ? __pfx_kthread+0x10/0x10
>> ret_from_fork+0x30/0x50
>> ? __pfx_kthread+0x10/0x10
>> ret_from_fork_asm+0x1b/0x30
>> </TASK>
>>
>> Since this testsuite is different from what I had issues with before,
>> I don't really know when the issue first appeared. It may have been
>> there even last year.
>>
>> I can reproduce the issue with ubuntu 6.2.0-32-generic kernel, but
>> I don't see it with the latest 5.14.0-284.25.1.el9_2.x86_64 on RHEL9.
>> It doesn't necessarily mean that RHEL9 doesn't have the issue though,
>> the testsuite is not 100% reliable.
>>
>> I'll try to dig deeper and bisect the problem on the upstream kernel.
>>
>> For now it seems like the issue is likely in IPv6 code, because the
>> tests that trigger it are mostly IPv6-related.
>
> So, I bisected this down to an unsurprising commit:
>
> commit b0e214d212030fe497d4d150bb3474e50ad5d093
> Author: Madhu Koriginja <[email protected]>
> Date: Tue Mar 21 21:28:44 2023 +0530
>
> netfilter: keep conntrack reference until IPsecv6 policy checks are done
>
> Keep the conntrack reference until policy checks have been performed for
> IPsec V6 NAT support, just like ipv4.
>
> The reference needs to be dropped before a packet is
> queued to avoid having the conntrack module unloadable.
>
> Fixes: 58a317f1061c ("netfilter: ipv6: add IPv6 NAT support")
> Signed-off-by: Madhu Koriginja <[email protected]>
> Signed-off-by: Florian Westphal <[email protected]>
>
> This commit repeated the pattern from the old ipv4 commit b59c270104f0
> ("[NETFILTER]: Keep conntrack reference until IPsec policy checks are done")
> and hence introduced the exact same issue, but for IPv6. They both failed
> to deliver on the "avoid having the conntrack module unloadable" part.
>
> IIUC, the fix should be exactly the same as Eric did for ipv4 in commit
> 6f0012e35160 ("tcp: add a missing nf_reset_ct() in 3WHS handling").
>
> I can try that and send a fix.
Posted a fix here:
https://lore.kernel.org/netdev/[email protected]/
It fixes the problem I have with OVN testsuite.
>
> Would still really like to have some preventive mechanism for this kind of
> issues though. Any ideas on that?
>
> Best regards, Ilya Maximets.