| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230105170812.zeq6fd2t2iwwr3fj@t14s.localdomain>
Date: Thu, 5 Jan 2023 14:08:12 -0300
From: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: Davide Caratti <dcaratti@...hat.com>, netdev@...r.kernel.org,
jiri@...nulli.us, pabeni@...hat.com, wizhao@...hat.com,
xiyou.wangcong@...il.com, lucien.xin@...il.com
Subject: Re: [RFC net-next 2/2] act_mirred: use the backlog for nested calls
to mirred ingress
Hi Jamal,
On Mon, Dec 26, 2022 at 11:03:34AM -0500, Jamal Hadi Salim wrote:
> Davide,
He's on PTO, so please let me try to answer it here.
> So this would fix the false positive you are seeing i believe;
> however, will it fix a real loop?
> In particular I am not sure if the next packet grabbed from the
> backlog will end up in the
> same CPU.
This series is not changing, functionally speaking, any about that.
As explained on the first patch, this "loop protection" already
doesn't work when RFS or skb timestamps are used. The idea on that
protection was different from the beginning already - to actually
protect from stack growth, and not packet loop. So the first patch
here amends the wording on it to something closer to reality.
Makes sense?
Cheers,
Marcelo
>
> cheers,
> jamal
>
> On Tue, Dec 20, 2022 at 1:25 PM Davide Caratti <dcaratti@...hat.com> wrote:
> >
> > William reports kernel soft-lockups on some OVS topologies when TC mirred
> > egress->ingress action is hit by local TCP traffic [1].
> > The same can also be reproduced with SCTP (thanks Xin for verifying), when
> > client and server reach themselves through mirred egress to ingress, and
> > one of the two peers sends a "heartbeat" packet (from within a timer).
> >
> > Enqueueing to backlog proved to fix this soft lockup; however, as Cong
> > noticed [2], we should preserve - when possible - the current mirred
> > behavior that counts as "overlimits" any eventual packet drop subsequent to
> > the mirred forwarding action [3]. A compromise solution might use the
> > backlog only when tcf_mirred_act() has a nest level greater than one:
> > change tcf_mirred_forward() accordingly.
> >
> > Also, add a kselftest that can reproduce the lockup and verifies TC mirred
> > ability to account for further packet drops after TC mirred egress->ingress
> > (when the nest level is 1).
> >
> > [1] https://lore.kernel.org/netdev/33dc43f587ec1388ba456b4915c75f02a8aae226.1663945716.git.dcaratti@redhat.com/
> > [2] https://lore.kernel.org/netdev/Y0w%2FWWY60gqrtGLp@pop-os.localdomain/
> > [3] such behavior is not guaranteed: for example, if RPS or skb RX
> > timestamping is enabled on the mirred target device, the kernel
> > can defer receiving the skb and return NET_RX_SUCCESS inside
> > tcf_mirred_forward().
> >
> > Reported-by: William Zhao <wizhao@...hat.com>
> > CC: Xin Long <lucien.xin@...il.com>
> > Signed-off-by: Davide Caratti <dcaratti@...hat.com>
> > ---
> > net/sched/act_mirred.c | 7 +++
> > .../selftests/net/forwarding/tc_actions.sh | 49 ++++++++++++++++++-
> > 2 files changed, 55 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> > index c8abb5136491..8037ec9b1d31 100644
> > --- a/net/sched/act_mirred.c
> > +++ b/net/sched/act_mirred.c
> > @@ -206,12 +206,19 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> > return err;
> > }
> >
> > +static bool is_mirred_nested(void)
> > +{
> > + return unlikely(__this_cpu_read(mirred_nest_level) > 1);
> > +}
> > +
> > static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
> > {
> > int err;
> >
> > if (!want_ingress)
> > err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
> > + else if (is_mirred_nested())
> > + err = netif_rx(skb);
> > else
> > err = netif_receive_skb(skb);
> >
> > diff --git a/tools/testing/selftests/net/forwarding/tc_actions.sh b/tools/testing/selftests/net/forwarding/tc_actions.sh
> > index 1e0a62f638fe..919c0dd9fe4b 100755
> > --- a/tools/testing/selftests/net/forwarding/tc_actions.sh
> > +++ b/tools/testing/selftests/net/forwarding/tc_actions.sh
> > @@ -3,7 +3,8 @@
> >
> > ALL_TESTS="gact_drop_and_ok_test mirred_egress_redirect_test \
> > mirred_egress_mirror_test matchall_mirred_egress_mirror_test \
> > - gact_trap_test mirred_egress_to_ingress_test"
> > + gact_trap_test mirred_egress_to_ingress_test \
> > + mirred_egress_to_ingress_tcp_test"
> > NUM_NETIFS=4
> > source tc_common.sh
> > source lib.sh
> > @@ -198,6 +199,52 @@ mirred_egress_to_ingress_test()
> > log_test "mirred_egress_to_ingress ($tcflags)"
> > }
> >
> > +mirred_egress_to_ingress_tcp_test()
> > +{
> > + local tmpfile=$(mktemp) tmpfile1=$(mktemp)
> > +
> > + RET=0
> > + dd conv=sparse status=none if=/dev/zero bs=1M count=2 of=$tmpfile
> > + tc filter add dev $h1 protocol ip pref 100 handle 100 egress flower \
> > + $tcflags ip_proto tcp src_ip 192.0.2.1 dst_ip 192.0.2.2 \
> > + action ct commit nat src addr 192.0.2.2 pipe \
> > + action ct clear pipe \
> > + action ct commit nat dst addr 192.0.2.1 pipe \
> > + action ct clear pipe \
> > + action skbedit ptype host pipe \
> > + action mirred ingress redirect dev $h1
> > + tc filter add dev $h1 protocol ip pref 101 handle 101 egress flower \
> > + $tcflags ip_proto icmp \
> > + action mirred ingress redirect dev $h1
> > + tc filter add dev $h1 protocol ip pref 102 handle 102 ingress flower \
> > + ip_proto icmp \
> > + action drop
> > +
> > + ip vrf exec v$h1 nc --recv-only -w10 -l -p 12345 -o $tmpfile1 &
> > + local rpid=$!
> > + ip vrf exec v$h1 nc -w1 --send-only 192.0.2.2 12345 <$tmpfile
> > + wait -n $rpid
> > + cmp -s $tmpfile $tmpfile1
> > + check_err $? "server output check failed"
> > +
> > + $MZ $h1 -c 10 -p 64 -a $h1mac -b $h1mac -A 192.0.2.1 -B 192.0.2.1 \
> > + -t icmp "ping,id=42,seq=5" -q
> > + tc_check_packets "dev $h1 egress" 101 10
> > + check_err $? "didn't mirred redirect ICMP"
> > + tc_check_packets "dev $h1 ingress" 102 10
> > + check_err $? "didn't drop mirred ICMP"
> > + local overlimits=$(tc_rule_stats_get ${h1} 101 egress .overlimits)
> > + test ${overlimits} = 10
> > + check_err $? "wrong overlimits, expected 10 got ${overlimits}"
> > +
> > + tc filter del dev $h1 egress protocol ip pref 100 handle 100 flower
> > + tc filter del dev $h1 egress protocol ip pref 101 handle 101 flower
> > + tc filter del dev $h1 ingress protocol ip pref 102 handle 102 flower
> > +
> > + rm -f $tmpfile $tmpfile1
> > + log_test "mirred_egress_to_ingress_tcp ($tcflags)"
> > +}
> > +
> > setup_prepare()
> > {
> > h1=${NETIFS[p1]}
> > --
> > 2.38.1
> >
>
Powered by blists - more mailing lists