| Message ID | f8685ec7702c4a448a1371a8b34b43217b583b9d.1699898008.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7cd5af0e937a197295f3aa3721031f0fbae49cff |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: sched: do not offload flows with a helper in act_ct | expand |
On Mon, Nov 13, 2023 at 12:53 PM Xin Long <lucien.xin@gmail.com> wrote: > > There is no hardware supporting ct helper offload. However, prior to this > patch, a flower filter with a helper in the ct action can be successfully > set into the HW, for example (eth1 is a bnxt NIC): > > # tc qdisc add dev eth1 ingress_block 22 ingress > # tc filter add block 22 proto ip flower skip_sw ip_proto tcp \ > dst_port 21 ct_state -trk action ct helper ipv4-tcp-ftp > # tc filter show dev eth1 ingress > > filter block 22 protocol ip pref 49152 flower chain 0 handle 0x1 > eth_type ipv4 > ip_proto tcp > dst_port 21 > ct_state -trk > skip_sw > in_hw in_hw_count 1 <---- > action order 1: ct zone 0 helper ipv4-tcp-ftp pipe > index 2 ref 1 bind 1 > used_hw_stats delayed > > This might cause the flower filter not to work as expected in the HW. > > This patch avoids this problem by simply returning -EOPNOTSUPP in > tcf_ct_offload_act_setup() to not allow to offload flows with a helper > in act_ct. > > Fixes: a21b06e73191 ("net: sched: add helper support in act_ct") > Signed-off-by: Xin Long <lucien.xin@gmail.com> I didnt quite follow: The driver accepted the config, but the driver "kind of '' supports it. (enough to not complain and then display it when queried). Should the driver have rejected something it doesnt fully support? cheers, jamal > --- > include/net/tc_act/tc_ct.h | 9 +++++++++ > net/sched/act_ct.c | 3 +++ > 2 files changed, 12 insertions(+) > > diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h > index 8a6dbfb23336..77f87c622a2e 100644 > --- a/include/net/tc_act/tc_ct.h > +++ b/include/net/tc_act/tc_ct.h > @@ -58,6 +58,11 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) > return to_ct_params(a)->nf_ft; > } > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) > +{ > + return to_ct_params(a)->helper; > +} > + > #else > static inline uint16_t tcf_ct_zone(const struct tc_action *a) { return 0; } > static inline int tcf_ct_action(const struct tc_action *a) { return 0; } > @@ -65,6 +70,10 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) > { > return NULL; > } > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) > +{ > + return NULL; > +} > #endif /* CONFIG_NF_CONNTRACK */ > > #if IS_ENABLED(CONFIG_NET_ACT_CT) > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c > index 0db0ecf1d110..b3f4a503ee2b 100644 > --- a/net/sched/act_ct.c > +++ b/net/sched/act_ct.c > @@ -1549,6 +1549,9 @@ static int tcf_ct_offload_act_setup(struct tc_action *act, void *entry_data, > if (bind) { > struct flow_action_entry *entry = entry_data; > > + if (tcf_ct_helper(act)) > + return -EOPNOTSUPP; > + > entry->id = FLOW_ACTION_CT; > entry->ct.action = tcf_ct_action(act); > entry->ct.zone = tcf_ct_zone(act); > -- > 2.39.1 >
On Mon, Nov 13, 2023 at 4:37 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > On Mon, Nov 13, 2023 at 12:53 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > There is no hardware supporting ct helper offload. However, prior to this > > patch, a flower filter with a helper in the ct action can be successfully > > set into the HW, for example (eth1 is a bnxt NIC): > > > > # tc qdisc add dev eth1 ingress_block 22 ingress > > # tc filter add block 22 proto ip flower skip_sw ip_proto tcp \ > > dst_port 21 ct_state -trk action ct helper ipv4-tcp-ftp > > # tc filter show dev eth1 ingress > > > > filter block 22 protocol ip pref 49152 flower chain 0 handle 0x1 > > eth_type ipv4 > > ip_proto tcp > > dst_port 21 > > ct_state -trk > > skip_sw > > in_hw in_hw_count 1 <---- > > action order 1: ct zone 0 helper ipv4-tcp-ftp pipe > > index 2 ref 1 bind 1 > > used_hw_stats delayed > > > > This might cause the flower filter not to work as expected in the HW. > > > > This patch avoids this problem by simply returning -EOPNOTSUPP in > > tcf_ct_offload_act_setup() to not allow to offload flows with a helper > > in act_ct. > > > > Fixes: a21b06e73191 ("net: sched: add helper support in act_ct") > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > I didnt quite follow: > The driver accepted the config, but the driver "kind of '' supports > it. (enough to not complain and then display it when queried). > Should the driver have rejected something it doesnt fully support? Hi, Jamal, The sad thing is now it does not pass the 'helper' param to the HW in tcf_ct_offload_act_setup() via struct flow_action_entry, in fact flow_action_entry does not even have a member to keep 'helper'. Since no HW currently supports 'helper', we can stop it setting to HW from here for now. In future, if HWs and struct flow_action_entry support it, we can set it to the entry and reply on HWs to reject it when not supported, as you mentioned above. Thanks. > > > cheers, > jamal > > > --- > > include/net/tc_act/tc_ct.h | 9 +++++++++ > > net/sched/act_ct.c | 3 +++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h > > index 8a6dbfb23336..77f87c622a2e 100644 > > --- a/include/net/tc_act/tc_ct.h > > +++ b/include/net/tc_act/tc_ct.h > > @@ -58,6 +58,11 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) > > return to_ct_params(a)->nf_ft; > > } > > > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) > > +{ > > + return to_ct_params(a)->helper; > > +} > > + > > #else > > static inline uint16_t tcf_ct_zone(const struct tc_action *a) { return 0; } > > static inline int tcf_ct_action(const struct tc_action *a) { return 0; } > > @@ -65,6 +70,10 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) > > { > > return NULL; > > } > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) > > +{ > > + return NULL; > > +} > > #endif /* CONFIG_NF_CONNTRACK */ > > > > #if IS_ENABLED(CONFIG_NET_ACT_CT) > > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c > > index 0db0ecf1d110..b3f4a503ee2b 100644 > > --- a/net/sched/act_ct.c > > +++ b/net/sched/act_ct.c > > @@ -1549,6 +1549,9 @@ static int tcf_ct_offload_act_setup(struct tc_action *act, void *entry_data, > > if (bind) { > > struct flow_action_entry *entry = entry_data; > > > > + if (tcf_ct_helper(act)) > > + return -EOPNOTSUPP; > > + > > entry->id = FLOW_ACTION_CT; > > entry->ct.action = tcf_ct_action(act); > > entry->ct.zone = tcf_ct_zone(act); > > -- > > 2.39.1 > >
Hi Xin, On Tue, Nov 14, 2023 at 10:18 AM Xin Long <lucien.xin@gmail.com> wrote: > > On Mon, Nov 13, 2023 at 4:37 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > > > On Mon, Nov 13, 2023 at 12:53 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > There is no hardware supporting ct helper offload. However, prior to this > > > patch, a flower filter with a helper in the ct action can be successfully > > > set into the HW, for example (eth1 is a bnxt NIC): > > > > > > # tc qdisc add dev eth1 ingress_block 22 ingress > > > # tc filter add block 22 proto ip flower skip_sw ip_proto tcp \ > > > dst_port 21 ct_state -trk action ct helper ipv4-tcp-ftp > > > # tc filter show dev eth1 ingress > > > > > > filter block 22 protocol ip pref 49152 flower chain 0 handle 0x1 > > > eth_type ipv4 > > > ip_proto tcp > > > dst_port 21 > > > ct_state -trk > > > skip_sw > > > in_hw in_hw_count 1 <---- > > > action order 1: ct zone 0 helper ipv4-tcp-ftp pipe > > > index 2 ref 1 bind 1 > > > used_hw_stats delayed > > > > > > This might cause the flower filter not to work as expected in the HW. > > > > > > This patch avoids this problem by simply returning -EOPNOTSUPP in > > > tcf_ct_offload_act_setup() to not allow to offload flows with a helper > > > in act_ct. > > > > > > Fixes: a21b06e73191 ("net: sched: add helper support in act_ct") > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > I didnt quite follow: > > The driver accepted the config, but the driver "kind of '' supports > > it. (enough to not complain and then display it when queried). > > Should the driver have rejected something it doesnt fully support? > Hi, Jamal, > > The sad thing is now it does not pass the 'helper' param to the HW in > tcf_ct_offload_act_setup() via struct flow_action_entry, in fact > flow_action_entry does not even have a member to keep 'helper'. > > Since no HW currently supports 'helper', we can stop it setting to HW > from here for now. In future, if HWs and struct flow_action_entry > support it, we can set it to the entry and reply on HWs to reject > it when not supported, as you mentioned above. That makes sense - so i am wondering why that was ever added there to begin with. Would there be any hardware that would have any helper support? If no, Shouldnt that code be deleted altogether? In any case, to the code correctness: Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal > Thanks. > > > > > > cheers, > > jamal > > > > > --- > > > include/net/tc_act/tc_ct.h | 9 +++++++++ > > > net/sched/act_ct.c | 3 +++ > > > 2 files changed, 12 insertions(+) > > > > > > diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h > > > index 8a6dbfb23336..77f87c622a2e 100644 > > > --- a/include/net/tc_act/tc_ct.h > > > +++ b/include/net/tc_act/tc_ct.h > > > @@ -58,6 +58,11 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) > > > return to_ct_params(a)->nf_ft; > > > } > > > > > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) > > > +{ > > > + return to_ct_params(a)->helper; > > > +} > > > + > > > #else > > > static inline uint16_t tcf_ct_zone(const struct tc_action *a) { return 0; } > > > static inline int tcf_ct_action(const struct tc_action *a) { return 0; } > > > @@ -65,6 +70,10 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) > > > { > > > return NULL; > > > } > > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) > > > +{ > > > + return NULL; > > > +} > > > #endif /* CONFIG_NF_CONNTRACK */ > > > > > > #if IS_ENABLED(CONFIG_NET_ACT_CT) > > > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c > > > index 0db0ecf1d110..b3f4a503ee2b 100644 > > > --- a/net/sched/act_ct.c > > > +++ b/net/sched/act_ct.c > > > @@ -1549,6 +1549,9 @@ static int tcf_ct_offload_act_setup(struct tc_action *act, void *entry_data, > > > if (bind) { > > > struct flow_action_entry *entry = entry_data; > > > > > > + if (tcf_ct_helper(act)) > > > + return -EOPNOTSUPP; > > > + > > > entry->id = FLOW_ACTION_CT; > > > entry->ct.action = tcf_ct_action(act); > > > entry->ct.zone = tcf_ct_zone(act); > > > -- > > > 2.39.1 > > >
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Mon, 13 Nov 2023 12:53:28 -0500 you wrote: > There is no hardware supporting ct helper offload. However, prior to this > patch, a flower filter with a helper in the ct action can be successfully > set into the HW, for example (eth1 is a bnxt NIC): > > # tc qdisc add dev eth1 ingress_block 22 ingress > # tc filter add block 22 proto ip flower skip_sw ip_proto tcp \ > dst_port 21 ct_state -trk action ct helper ipv4-tcp-ftp > # tc filter show dev eth1 ingress > > [...] Here is the summary with links: - [net] net: sched: do not offload flows with a helper in act_ct https://git.kernel.org/netdev/net/c/7cd5af0e937a You are awesome, thank you!
On Wed, Nov 15, 2023 at 11:37:51AM -0500, Jamal Hadi Salim wrote: > Hi Xin, > > On Tue, Nov 14, 2023 at 10:18 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Mon, Nov 13, 2023 at 4:37 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > > > > > On Mon, Nov 13, 2023 at 12:53 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > There is no hardware supporting ct helper offload. However, prior to this > > > > patch, a flower filter with a helper in the ct action can be successfully > > > > set into the HW, for example (eth1 is a bnxt NIC): > > > > > > > > # tc qdisc add dev eth1 ingress_block 22 ingress > > > > # tc filter add block 22 proto ip flower skip_sw ip_proto tcp \ > > > > dst_port 21 ct_state -trk action ct helper ipv4-tcp-ftp > > > > # tc filter show dev eth1 ingress > > > > > > > > filter block 22 protocol ip pref 49152 flower chain 0 handle 0x1 > > > > eth_type ipv4 > > > > ip_proto tcp > > > > dst_port 21 > > > > ct_state -trk > > > > skip_sw > > > > in_hw in_hw_count 1 <---- > > > > action order 1: ct zone 0 helper ipv4-tcp-ftp pipe > > > > index 2 ref 1 bind 1 > > > > used_hw_stats delayed > > > > > > > > This might cause the flower filter not to work as expected in the HW. > > > > > > > > This patch avoids this problem by simply returning -EOPNOTSUPP in > > > > tcf_ct_offload_act_setup() to not allow to offload flows with a helper > > > > in act_ct. > > > > > > > > Fixes: a21b06e73191 ("net: sched: add helper support in act_ct") > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > > > I didnt quite follow: > > > The driver accepted the config, but the driver "kind of '' supports > > > it. (enough to not complain and then display it when queried). > > > Should the driver have rejected something it doesnt fully support? > > Hi, Jamal, > > > > The sad thing is now it does not pass the 'helper' param to the HW in > > tcf_ct_offload_act_setup() via struct flow_action_entry, in fact > > flow_action_entry does not even have a member to keep 'helper'. > > > > Since no HW currently supports 'helper', we can stop it setting to HW > > from here for now. In future, if HWs and struct flow_action_entry > > support it, we can set it to the entry and reply on HWs to reject > > it when not supported, as you mentioned above. > > That makes sense - so i am wondering why that was ever added there to > begin with. Would there be any hardware that would have any helper > support? If no, Shouldnt that code be deleted altogether? Not sure if I follow you, Jamal. There's no code at all to pass the helper information down to the drivers. So drivers ended up accepting this flow because they had no idea that a helper was attached to it. Then yes, ideally, it should be driver the one to reject the flow that it doesn't support. But as currently zero drivers support it, and I doubt one will in the future [*], this patch simplifies it by instead of adding all the helper stuff to flow_action_entry, to just abort the offload earlier. [*] it requires parsing TCP payload, including over packet boundaries. This is very expensive in hw. And leads to another problem: the HW having to tell the SW stack about new conntrack expectations. Chers, Marcelo > > In any case, to the code correctness: > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> > > cheers, > jamal > > Thanks. > > > > > > > > > cheers, > > > jamal > > > > > > > --- > > > > include/net/tc_act/tc_ct.h | 9 +++++++++ > > > > net/sched/act_ct.c | 3 +++ > > > > 2 files changed, 12 insertions(+) > > > > > > > > diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h > > > > index 8a6dbfb23336..77f87c622a2e 100644 > > > > --- a/include/net/tc_act/tc_ct.h > > > > +++ b/include/net/tc_act/tc_ct.h > > > > @@ -58,6 +58,11 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) > > > > return to_ct_params(a)->nf_ft; > > > > } > > > > > > > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) > > > > +{ > > > > + return to_ct_params(a)->helper; > > > > +} > > > > + > > > > #else > > > > static inline uint16_t tcf_ct_zone(const struct tc_action *a) { return 0; } > > > > static inline int tcf_ct_action(const struct tc_action *a) { return 0; } > > > > @@ -65,6 +70,10 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) > > > > { > > > > return NULL; > > > > } > > > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) > > > > +{ > > > > + return NULL; > > > > +} > > > > #endif /* CONFIG_NF_CONNTRACK */ > > > > > > > > #if IS_ENABLED(CONFIG_NET_ACT_CT) > > > > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c > > > > index 0db0ecf1d110..b3f4a503ee2b 100644 > > > > --- a/net/sched/act_ct.c > > > > +++ b/net/sched/act_ct.c > > > > @@ -1549,6 +1549,9 @@ static int tcf_ct_offload_act_setup(struct tc_action *act, void *entry_data, > > > > if (bind) { > > > > struct flow_action_entry *entry = entry_data; > > > > > > > > + if (tcf_ct_helper(act)) > > > > + return -EOPNOTSUPP; > > > > + > > > > entry->id = FLOW_ACTION_CT; > > > > entry->ct.action = tcf_ct_action(act); > > > > entry->ct.zone = tcf_ct_zone(act); > > > > -- > > > > 2.39.1 > > > >
Hi Marcelo, On Thu, Nov 16, 2023 at 6:36 AM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Wed, Nov 15, 2023 at 11:37:51AM -0500, Jamal Hadi Salim wrote: > > Hi Xin, > > > > On Tue, Nov 14, 2023 at 10:18 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > On Mon, Nov 13, 2023 at 4:37 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > > > > > > > On Mon, Nov 13, 2023 at 12:53 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > > > There is no hardware supporting ct helper offload. However, prior to this > > > > > patch, a flower filter with a helper in the ct action can be successfully > > > > > set into the HW, for example (eth1 is a bnxt NIC): > > > > > > > > > > # tc qdisc add dev eth1 ingress_block 22 ingress > > > > > # tc filter add block 22 proto ip flower skip_sw ip_proto tcp \ > > > > > dst_port 21 ct_state -trk action ct helper ipv4-tcp-ftp > > > > > # tc filter show dev eth1 ingress > > > > > > > > > > filter block 22 protocol ip pref 49152 flower chain 0 handle 0x1 > > > > > eth_type ipv4 > > > > > ip_proto tcp > > > > > dst_port 21 > > > > > ct_state -trk > > > > > skip_sw > > > > > in_hw in_hw_count 1 <---- > > > > > action order 1: ct zone 0 helper ipv4-tcp-ftp pipe > > > > > index 2 ref 1 bind 1 > > > > > used_hw_stats delayed > > > > > > > > > > This might cause the flower filter not to work as expected in the HW. > > > > > > > > > > This patch avoids this problem by simply returning -EOPNOTSUPP in > > > > > tcf_ct_offload_act_setup() to not allow to offload flows with a helper > > > > > in act_ct. > > > > > > > > > > Fixes: a21b06e73191 ("net: sched: add helper support in act_ct") > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > > > > > I didnt quite follow: > > > > The driver accepted the config, but the driver "kind of '' supports > > > > it. (enough to not complain and then display it when queried). > > > > Should the driver have rejected something it doesnt fully support? > > > Hi, Jamal, > > > > > > The sad thing is now it does not pass the 'helper' param to the HW in > > > tcf_ct_offload_act_setup() via struct flow_action_entry, in fact > > > flow_action_entry does not even have a member to keep 'helper'. > > > > > > Since no HW currently supports 'helper', we can stop it setting to HW > > > from here for now. In future, if HWs and struct flow_action_entry > > > support it, we can set it to the entry and reply on HWs to reject > > > it when not supported, as you mentioned above. > > > > That makes sense - so i am wondering why that was ever added there to > > begin with. Would there be any hardware that would have any helper > > support? If no, Shouldnt that code be deleted altogether? > > Not sure if I follow you, Jamal. There's no code at all to pass the > helper information down to the drivers. So drivers ended up accepting > this flow because they had no idea that a helper was attached to it. > So is the goal: a) if there's a helper it doesnt make sense to offload the flow or b) if there's a helper then it(the helper) works in s/w only but the flow offload is still legit? If it is #a then my question was why is that code even there in the offload path... Likely i am missing something.. cheers, jamal > Then yes, ideally, it should be driver the one to reject the flow that > it doesn't support. But as currently zero drivers support it, and I > doubt one will in the future [*], this patch simplifies it by instead > of adding all the helper stuff to flow_action_entry, to just abort the > offload earlier. > > [*] it requires parsing TCP payload, including over packet boundaries. > This is very expensive in hw. And leads to another problem: the HW > having to tell the SW stack about new conntrack expectations. > > Chers, > Marcelo > > > > > In any case, to the code correctness: > > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> > > > > cheers, > > jamal > > > Thanks. > > > > > > > > > > > > cheers, > > > > jamal > > > > > > > > > --- > > > > > include/net/tc_act/tc_ct.h | 9 +++++++++ > > > > > net/sched/act_ct.c | 3 +++ > > > > > 2 files changed, 12 insertions(+) > > > > > > > > > > diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h > > > > > index 8a6dbfb23336..77f87c622a2e 100644 > > > > > --- a/include/net/tc_act/tc_ct.h > > > > > +++ b/include/net/tc_act/tc_ct.h > > > > > @@ -58,6 +58,11 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) > > > > > return to_ct_params(a)->nf_ft; > > > > > } > > > > > > > > > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) > > > > > +{ > > > > > + return to_ct_params(a)->helper; > > > > > +} > > > > > + > > > > > #else > > > > > static inline uint16_t tcf_ct_zone(const struct tc_action *a) { return 0; } > > > > > static inline int tcf_ct_action(const struct tc_action *a) { return 0; } > > > > > @@ -65,6 +70,10 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) > > > > > { > > > > > return NULL; > > > > > } > > > > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) > > > > > +{ > > > > > + return NULL; > > > > > +} > > > > > #endif /* CONFIG_NF_CONNTRACK */ > > > > > > > > > > #if IS_ENABLED(CONFIG_NET_ACT_CT) > > > > > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c > > > > > index 0db0ecf1d110..b3f4a503ee2b 100644 > > > > > --- a/net/sched/act_ct.c > > > > > +++ b/net/sched/act_ct.c > > > > > @@ -1549,6 +1549,9 @@ static int tcf_ct_offload_act_setup(struct tc_action *act, void *entry_data, > > > > > if (bind) { > > > > > struct flow_action_entry *entry = entry_data; > > > > > > > > > > + if (tcf_ct_helper(act)) > > > > > + return -EOPNOTSUPP; > > > > > + > > > > > entry->id = FLOW_ACTION_CT; > > > > > entry->ct.action = tcf_ct_action(act); > > > > > entry->ct.zone = tcf_ct_zone(act); > > > > > -- > > > > > 2.39.1 > > > > >
On Thu, Nov 16, 2023 at 10:29:39AM -0500, Jamal Hadi Salim wrote: > Hi Marcelo, heya! > > On Thu, Nov 16, 2023 at 6:36 AM Marcelo Ricardo Leitner > <marcelo.leitner@gmail.com> wrote: > > > > On Wed, Nov 15, 2023 at 11:37:51AM -0500, Jamal Hadi Salim wrote: > > > Hi Xin, > > > > > > On Tue, Nov 14, 2023 at 10:18 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > On Mon, Nov 13, 2023 at 4:37 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > > > > > > > > > On Mon, Nov 13, 2023 at 12:53 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > > > > > There is no hardware supporting ct helper offload. However, prior to this > > > > > > patch, a flower filter with a helper in the ct action can be successfully > > > > > > set into the HW, for example (eth1 is a bnxt NIC): > > > > > > > > > > > > # tc qdisc add dev eth1 ingress_block 22 ingress > > > > > > # tc filter add block 22 proto ip flower skip_sw ip_proto tcp \ > > > > > > dst_port 21 ct_state -trk action ct helper ipv4-tcp-ftp > > > > > > # tc filter show dev eth1 ingress > > > > > > > > > > > > filter block 22 protocol ip pref 49152 flower chain 0 handle 0x1 > > > > > > eth_type ipv4 > > > > > > ip_proto tcp > > > > > > dst_port 21 > > > > > > ct_state -trk > > > > > > skip_sw > > > > > > in_hw in_hw_count 1 <---- > > > > > > action order 1: ct zone 0 helper ipv4-tcp-ftp pipe > > > > > > index 2 ref 1 bind 1 > > > > > > used_hw_stats delayed > > > > > > > > > > > > This might cause the flower filter not to work as expected in the HW. > > > > > > > > > > > > This patch avoids this problem by simply returning -EOPNOTSUPP in > > > > > > tcf_ct_offload_act_setup() to not allow to offload flows with a helper > > > > > > in act_ct. > > > > > > > > > > > > Fixes: a21b06e73191 ("net: sched: add helper support in act_ct") > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > > > > > > > I didnt quite follow: > > > > > The driver accepted the config, but the driver "kind of '' supports > > > > > it. (enough to not complain and then display it when queried). > > > > > Should the driver have rejected something it doesnt fully support? > > > > Hi, Jamal, > > > > > > > > The sad thing is now it does not pass the 'helper' param to the HW in > > > > tcf_ct_offload_act_setup() via struct flow_action_entry, in fact > > > > flow_action_entry does not even have a member to keep 'helper'. > > > > > > > > Since no HW currently supports 'helper', we can stop it setting to HW > > > > from here for now. In future, if HWs and struct flow_action_entry > > > > support it, we can set it to the entry and reply on HWs to reject > > > > it when not supported, as you mentioned above. > > > > > > That makes sense - so i am wondering why that was ever added there to > > > begin with. Would there be any hardware that would have any helper > > > support? If no, Shouldnt that code be deleted altogether? > > > > Not sure if I follow you, Jamal. There's no code at all to pass the > > helper information down to the drivers. So drivers ended up accepting > > this flow because they had no idea that a helper was attached to it. > > > > So is the goal: > a) if there's a helper it doesnt make sense to offload the flow or > b) if there's a helper then it(the helper) works in s/w only but the > flow offload is still legit? It is #a. > > If it is #a then my question was why is that code even there in the > offload path... > Likely i am missing something.. And the part I am missing is, which code are you referring to? :D This check is being done at tcf_ct_offload_act_setup() because that's the callback that gets executed when it tries to serialize/export/whatever act_ct info into flow_action_entry. With this, it notices that for this instance it can't serialize and aborts it. AFAIK there isn't an earlier moment on which this check can be done. Cheers, Marcelo > > cheers, > jamal > > > Then yes, ideally, it should be driver the one to reject the flow that > > it doesn't support. But as currently zero drivers support it, and I > > doubt one will in the future [*], this patch simplifies it by instead > > of adding all the helper stuff to flow_action_entry, to just abort the > > offload earlier. > > > > [*] it requires parsing TCP payload, including over packet boundaries. > > This is very expensive in hw. And leads to another problem: the HW > > having to tell the SW stack about new conntrack expectations. > > > > > > Chers, > > Marcelo > > > > > > > > In any case, to the code correctness: > > > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> > > > > > > cheers, > > > jamal > > > > Thanks. > > > > > > > > > > > > > > > cheers, > > > > > jamal > > > > > > > > > > > --- > > > > > > include/net/tc_act/tc_ct.h | 9 +++++++++ > > > > > > net/sched/act_ct.c | 3 +++ > > > > > > 2 files changed, 12 insertions(+) > > > > > > > > > > > > diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h > > > > > > index 8a6dbfb23336..77f87c622a2e 100644 > > > > > > --- a/include/net/tc_act/tc_ct.h > > > > > > +++ b/include/net/tc_act/tc_ct.h > > > > > > @@ -58,6 +58,11 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) > > > > > > return to_ct_params(a)->nf_ft; > > > > > > } > > > > > > > > > > > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) > > > > > > +{ > > > > > > + return to_ct_params(a)->helper; > > > > > > +} > > > > > > + > > > > > > #else > > > > > > static inline uint16_t tcf_ct_zone(const struct tc_action *a) { return 0; } > > > > > > static inline int tcf_ct_action(const struct tc_action *a) { return 0; } > > > > > > @@ -65,6 +70,10 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) > > > > > > { > > > > > > return NULL; > > > > > > } > > > > > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) > > > > > > +{ > > > > > > + return NULL; > > > > > > +} > > > > > > #endif /* CONFIG_NF_CONNTRACK */ > > > > > > > > > > > > #if IS_ENABLED(CONFIG_NET_ACT_CT) > > > > > > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c > > > > > > index 0db0ecf1d110..b3f4a503ee2b 100644 > > > > > > --- a/net/sched/act_ct.c > > > > > > +++ b/net/sched/act_ct.c > > > > > > @@ -1549,6 +1549,9 @@ static int tcf_ct_offload_act_setup(struct tc_action *act, void *entry_data, > > > > > > if (bind) { > > > > > > struct flow_action_entry *entry = entry_data; > > > > > > > > > > > > + if (tcf_ct_helper(act)) > > > > > > + return -EOPNOTSUPP; > > > > > > + > > > > > > entry->id = FLOW_ACTION_CT; > > > > > > entry->ct.action = tcf_ct_action(act); > > > > > > entry->ct.zone = tcf_ct_zone(act); > > > > > > -- > > > > > > 2.39.1 > > > > > >
diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h index 8a6dbfb23336..77f87c622a2e 100644 --- a/include/net/tc_act/tc_ct.h +++ b/include/net/tc_act/tc_ct.h @@ -58,6 +58,11 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) return to_ct_params(a)->nf_ft; } +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) +{ + return to_ct_params(a)->helper; +} + #else static inline uint16_t tcf_ct_zone(const struct tc_action *a) { return 0; } static inline int tcf_ct_action(const struct tc_action *a) { return 0; } @@ -65,6 +70,10 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a) { return NULL; } +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a) +{ + return NULL; +} #endif /* CONFIG_NF_CONNTRACK */ #if IS_ENABLED(CONFIG_NET_ACT_CT) diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c index 0db0ecf1d110..b3f4a503ee2b 100644 --- a/net/sched/act_ct.c +++ b/net/sched/act_ct.c @@ -1549,6 +1549,9 @@ static int tcf_ct_offload_act_setup(struct tc_action *act, void *entry_data, if (bind) { struct flow_action_entry *entry = entry_data; + if (tcf_ct_helper(act)) + return -EOPNOTSUPP; + entry->id = FLOW_ACTION_CT; entry->ct.action = tcf_ct_action(act); entry->ct.zone = tcf_ct_zone(act);
There is no hardware supporting ct helper offload. However, prior to this patch, a flower filter with a helper in the ct action can be successfully set into the HW, for example (eth1 is a bnxt NIC): # tc qdisc add dev eth1 ingress_block 22 ingress # tc filter add block 22 proto ip flower skip_sw ip_proto tcp \ dst_port 21 ct_state -trk action ct helper ipv4-tcp-ftp # tc filter show dev eth1 ingress filter block 22 protocol ip pref 49152 flower chain 0 handle 0x1 eth_type ipv4 ip_proto tcp dst_port 21 ct_state -trk skip_sw in_hw in_hw_count 1 <---- action order 1: ct zone 0 helper ipv4-tcp-ftp pipe index 2 ref 1 bind 1 used_hw_stats delayed This might cause the flower filter not to work as expected in the HW. This patch avoids this problem by simply returning -EOPNOTSUPP in tcf_ct_offload_act_setup() to not allow to offload flows with a helper in act_ct. Fixes: a21b06e73191 ("net: sched: add helper support in act_ct") Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/net/tc_act/tc_ct.h | 9 +++++++++ net/sched/act_ct.c | 3 +++ 2 files changed, 12 insertions(+)