On 26/09/2019 08:30, Paul Blakey wrote: > Ok, I thought you meant merging the rules because we do want to support > those modifications use-cases. I think the point is that your use-case is sufficiently weird and obscure that code in the core to support it needs to be unintrusive; and this clearly wasn't (you managed to piss off Linus...) so it should be reverted, and held off until a more palatable solution can be produced. I agree with Alexei on this. Neither currently-supported-by-drivers cases nor the first step that's likely to be added (the simple conntrack with modifications only at the end) needs this, it's for capabilities that are farther in the future, so there's really no need for it to be in the tree when it's not ready, which appears to be the case at present. > In nat scenarios the packet will be modified, and then there can be a miss: > > -trk .... CT(zone X, Restore NAT),goto chain 1 > > +trk+est, match on ipv4, CT(zone Y), goto chain 2 > > +trk+est, output.. I'm confused, I thought the usual nat scenario looked more like 0: -trk ... action ct(zone x), goto chain 1 1: +trk+new ... action ct(commit, nat=foo) # sw only 1: +trk+est ... action ct(nat), mirred eth1 i.e. the NAT only happens after conntrack has matched (and thus provided the saved NAT metadata), at the end of the pipe. I don't see how you can NAT a -trk packet.
> Also, there are stats issues if we already accounted for some actions in > hardware. AFAICT only 'deliverish' actions (i.e. mirred and drop) in TC have stats. So stats are unlikely to be a problem unless you've got (say) a mirred mirror before you send to ct and goto chain, in which case the extra copy of the packet is a rather bigger problem for idempotency than mere stats ;-)
