Marcelo Ricardo Leitner <marcelo.leit...@gmail.com> writes: > On Mon, Nov 18, 2019 at 04:21:39PM -0500, Aaron Conole wrote: >> Marcelo Ricardo Leitner <marcelo.leit...@gmail.com> writes: >> >> > On Fri, Nov 08, 2019 at 04:07:14PM -0500, Aaron Conole wrote: >> >> The act_ct TC module shares a common conntrack and NAT infrastructure >> >> exposed via netfilter. It's possible that a packet needs both SNAT and >> >> DNAT manipulation, due to e.g. tuple collision. Netfilter can support >> >> this because it runs through the NAT table twice - once on ingress and >> >> again after egress. The act_ct action doesn't have such capability. >> >> >> >> Like netfilter hook infrastructure, we should run through NAT twice to >> >> keep the symmetry. >> >> >> >> Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct") >> >> >> >> Signed-off-by: Aaron Conole <acon...@redhat.com> >> >> --- >> >> net/sched/act_ct.c | 13 ++++++++++++- >> >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c >> >> index fcc46025e790..f3232a00970f 100644 >> >> --- a/net/sched/act_ct.c >> >> +++ b/net/sched/act_ct.c >> >> @@ -329,6 +329,7 @@ static int tcf_ct_act_nat(struct sk_buff *skb, >> >> bool commit) >> >> { >> >> #if IS_ENABLED(CONFIG_NF_NAT) >> >> + int err; >> >> enum nf_nat_manip_type maniptype; >> >> >> >> if (!(ct_action & TCA_CT_ACT_NAT)) >> >> @@ -359,7 +360,17 @@ static int tcf_ct_act_nat(struct sk_buff *skb, >> >> return NF_ACCEPT; >> >> } >> >> >> >> - return ct_nat_execute(skb, ct, ctinfo, range, maniptype); >> >> + err = ct_nat_execute(skb, ct, ctinfo, range, maniptype); >> >> + if (err == NF_ACCEPT && >> >> + ct->status & IPS_SRC_NAT && ct->status & IPS_DST_NAT) { >> >> + if (maniptype == NF_NAT_MANIP_SRC) >> >> + maniptype = NF_NAT_MANIP_DST; >> >> + else >> >> + maniptype = NF_NAT_MANIP_SRC; >> >> + >> >> + err = ct_nat_execute(skb, ct, ctinfo, range, maniptype); >> >> + } >> > >> > I keep thinking about this and I'm not entirely convinced that this >> > shouldn't be simpler. More like: >> > >> > if (DNAT) >> > DNAT >> > if (SNAT) >> > SNAT >> > >> > So it always does DNAT before SNAT, similarly to what iptables would >> > do on PRE/POSTROUTING chains. >> >> I can rewrite the whole function, but I wanted to start with the smaller >> fix that worked. I also think it needs more testing then (since it's >> something of a rewrite of the function). >> >> I guess it's not too important - do you think it gives any readability >> to do it this way? If so, I can respin the patch changing it like you >> describe. > > I didn't mean a rewrite, but just to never handle SNAT before DNAT. So > the fix here would be like: > > - return ct_nat_execute(skb, ct, ctinfo, range, maniptype); > + err = ct_nat_execute(skb, ct, ctinfo, range, maniptype); > + if (err == NF_ACCEPT && maniptype == NF_NAT_MANIP_DST && > + ct->status & IPS_SRC_NAT && ct->status & IPS_DST_NAT) { > + maniptype = NF_NAT_MANIP_SRC; > + err = ct_nat_execute(skb, ct, ctinfo, range, maniptype); > + } > + return err;
But the maniptype of the first call could be NAT_MANIP_SRC. In fact, that's what I see if the packet is reply direction && !related. So, we need the block to invert the manipulation type. Otherwise, we miss the DNAT manipulation. So I don't think I can use that block. >> >> + return err; >> >> #else >> >> return NF_ACCEPT; >> >> #endif >> >> -- >> >> 2.21.0 >> >> >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev