On Fri, Sep 06, 2019 at 02:37:16PM +0100, Edward Cree wrote:
> On 06/09/2019 14:14, Pablo Neira Ayuso wrote:
> > OK, I can document this semantics, I need just _time_ to write that
> > documentation. I was expecting this patch description is enough by now
> > until I can get to finish that documentation.
>
> I think for two structs with apparently the same contents but different
> semantics (one has the mask bitwise complemented) it's best to hold up
> the code change until the comment is ready to come with it, because
> until then it's a dangerously confusing situation.
The idea is that flow rule API != tc front-end anymore. So the flow
rule API can evolve regardless the front-end requirements. Before this
update there was no other choice than using the tc pedit layout since
it was exposed to the drivers, this is not the case anymore.
> >> And you can't just coalesce all consecutive mangles, because if you
> >> mangle two consecutive fields (e.g. UDP sport and dport) the driver
> >> still needs to disentangle that if it works on a 'fields' (rather
> >> than 'u32s') level.
> >
> > This infrastructure is _not_ coalescing two consecutive field, e.g.
> > UDP sport and dport is _not_ coalesced. The coalesce routine does
> > _not_ handle multiple tc pedit ex actions.
>
> So an IPv6 address mangle only comes as a single action if it's from
> netfilter, not if it's coming from TC pedit.
Driver gets one single action from tc/netfilter (and potentially
ethtool if it would support for this), it comes as a single action
from both subsystems:
front-end -----> flow_rule API -----> drivers
Front-end need to translate their representation to this
FLOW_ACTION_MANGLE layout.
By front-end, I refer to ethtool/netfilter/tc.
> Therefore drivers still need to handle an IPv6 or MAC address
> mangle coming in multiple actions, therefore your driver
> simplifications are invalid. No?
No. IPv6 and MAC come as a single action. All subsystems use the same
flow_rule API, they use the same layout.
> > The model you propose would still need this code for tc pedit to
> > adjust offset/length and coalesce u32 fields.
>
> Yes, but we don't add code/features to the kernel based on what we
> *could* use it for later
This is already useful: Look at the cxgb driver in particular, it's
way easier to read.
Other existing drivers do not need to do things like:
case round_down(offsetof(struct iphdr, tos), 4):
and the play with masks to infer if the user wants to mangle the TOS
field, they can just do:
case offsetof(struct iphdr, tos):
which is way more natural representation.
