| Message ID | a1253d4c38990854e5369074e4cbc9cd2098c532.1632133123.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sched: drop ct and dst for the packets toward ingress in act_mirred | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 3 blamed authors not CCed: yossiku@mellanox.com paulb@mellanox.com jiri@resnulli.us; 5 maintainers not CCed: jhs@mojatatu.com jiri@resnulli.us yossiku@mellanox.com paulb@mellanox.com xiyou.wangcong@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 14 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Mon, Sep 20, 2021 at 7:12 AM Xin Long <lucien.xin@gmail.com> wrote: > > Without dropping dst, the packets sent from local mirred/redirected > to ingress will may still use the old dst. ip_rcv() will drop it as > the old dst is for output and its .input is dst_discard. > > This patch is to fix by also dropping dst for those packets that are > mirred or redirected to ingress in act_mirred. Similar question: what about redirecting from ingress to egress? BTW, please CC TC maintainers for TC patches. Thanks.
On Tue, Sep 21, 2021 at 2:34 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Mon, Sep 20, 2021 at 7:12 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > Without dropping dst, the packets sent from local mirred/redirected > > to ingress will may still use the old dst. ip_rcv() will drop it as > > the old dst is for output and its .input is dst_discard. > > > > This patch is to fix by also dropping dst for those packets that are > > mirred or redirected to ingress in act_mirred. > > Similar question: what about redirecting from ingress to egress? We can do it IF there's any user case needing it. But for now, The problem I've met occurred in ip_rcv() for the user case. > > BTW, please CC TC maintainers for TC patches. added Jamal and Jiri. Thanks. > > Thanks.
On Tue, Sep 21, 2021 at 12:02 AM Xin Long <lucien.xin@gmail.com> wrote: > > On Tue, Sep 21, 2021 at 2:34 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > On Mon, Sep 20, 2021 at 7:12 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > Without dropping dst, the packets sent from local mirred/redirected > > > to ingress will may still use the old dst. ip_rcv() will drop it as > > > the old dst is for output and its .input is dst_discard. > > > > > > This patch is to fix by also dropping dst for those packets that are > > > mirred or redirected to ingress in act_mirred. > > > > Similar question: what about redirecting from ingress to egress? > We can do it IF there's any user case needing it. > But for now, The problem I've met occurred in ip_rcv() for the user case. I think input route is different from output route, so essentially we need a reset when changing the direction, but I don't see any bugs so far, except this one. Thanks.
On Wed, Sep 22, 2021 at 11:53 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Tue, Sep 21, 2021 at 12:02 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Tue, Sep 21, 2021 at 2:34 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > > > On Mon, Sep 20, 2021 at 7:12 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > Without dropping dst, the packets sent from local mirred/redirected > > > > to ingress will may still use the old dst. ip_rcv() will drop it as > > > > the old dst is for output and its .input is dst_discard. > > > > > > > > This patch is to fix by also dropping dst for those packets that are > > > > mirred or redirected to ingress in act_mirred. > > > > > > Similar question: what about redirecting from ingress to egress? > > We can do it IF there's any user case needing it. > > But for now, The problem I've met occurred in ip_rcv() for the user case. > > I think input route is different from output route, so essentially we need > a reset when changing the direction, but I don't see any bugs so far, > except this one. Yes, this one seems more reasonable to do this reset when changing the direction. Maybe because in common env, it rarely redirects an egress pkt to ingress by TC. OVS does flow control quite flexibly(complicatedly), when it offloads NAT to TC, this problem starts showing up.
+CC Shmulik. Unfortunately we dont have good test cases in tdc to test different scenarios of this setup (packets being redirected in both directions once or several times). cheers, jamal On 2021-09-21 11:52 p.m., Cong Wang wrote: > On Tue, Sep 21, 2021 at 12:02 AM Xin Long <lucien.xin@gmail.com> wrote: >> >> On Tue, Sep 21, 2021 at 2:34 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: >>> >>> On Mon, Sep 20, 2021 at 7:12 AM Xin Long <lucien.xin@gmail.com> wrote: >>>> >>>> Without dropping dst, the packets sent from local mirred/redirected >>>> to ingress will may still use the old dst. ip_rcv() will drop it as >>>> the old dst is for output and its .input is dst_discard. >>>> >>>> This patch is to fix by also dropping dst for those packets that are >>>> mirred or redirected to ingress in act_mirred. >>> >>> Similar question: what about redirecting from ingress to egress? >> We can do it IF there's any user case needing it. >> But for now, The problem I've met occurred in ip_rcv() for the user case. > > I think input route is different from output route, so essentially we need > a reset when changing the direction, but I don't see any bugs so far, > except this one. > > Thanks. >
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c index 46dff1f1e7c8..5e30b3e64b63 100644 --- a/net/sched/act_mirred.c +++ b/net/sched/act_mirred.c @@ -19,6 +19,7 @@ #include <linux/if_arp.h> #include <net/net_namespace.h> #include <net/netlink.h> +#include <net/dst.h> #include <net/pkt_sched.h> #include <net/pkt_cls.h> #include <linux/tc_act/tc_mirred.h> @@ -209,6 +210,7 @@ static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb) return tcf_dev_queue_xmit(skb, dev_queue_xmit); nf_reset_ct(skb); + skb_dst_drop(skb); return netif_receive_skb(skb); }
Without dropping dst, the packets sent from local mirred/redirected to ingress will may still use the old dst. ip_rcv() will drop it as the old dst is for output and its .input is dst_discard. This patch is to fix by also dropping dst for those packets that are mirred or redirected to ingress in act_mirred. Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct") Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/sched/act_mirred.c | 2 ++ 1 file changed, 2 insertions(+)