Re: [PATCH net-next 3/4] flow_dissector: extract more ICMP information

[Simon Horman]

On Fri, Oct 25, 2019 at 02:27:28AM +0200, Matteo Croce wrote:
> On Wed, Oct 23, 2019 at 7:55 PM Simon Horman <simon.horman@xxxxxxxxxxxxx> wrote:
> >
> > On Wed, Oct 23, 2019 at 12:53:37PM +0200, Matteo Croce wrote:
> > > On Wed, Oct 23, 2019 at 12:00 PM Simon Horman
> > > <simon.horman@xxxxxxxxxxxxx> wrote:
> > > > On Mon, Oct 21, 2019 at 10:09:47PM +0200, Matteo Croce wrote:
> > > > > +     switch (ih->type) {
> > > > > +     case ICMP_ECHO:
> > > > > +     case ICMP_ECHOREPLY:
> > > > > +     case ICMP_TIMESTAMP:
> > > > > +     case ICMP_TIMESTAMPREPLY:
> > > > > +     case ICMPV6_ECHO_REQUEST:
> > > > > +     case ICMPV6_ECHO_REPLY:
> > > > > +             /* As we use 0 to signal that the Id field is not present,
> > > > > +              * avoid confusion with packets without such field
> > > > > +              */
> > > > > +             key_icmp->id = ih->un.echo.id ? : 1;
> > > >
> > > > Its not obvious to me why the kernel should treat id-zero as a special
> > > > value if it is not special on the wire.
> > > >
> > > > Perhaps a caller who needs to know if the id is present can
> > > > check the ICMP type as this code does, say using a helper.
> > > >
> > >
> > > Hi,
> > >
> > > The problem is that the 0-0 Type-Code pair identifies the echo replies.
> > > So instead of adding a bool is_present value I hardcoded the info in
> > > the ID field making it always non null, at the expense of a possible
> > > collision, which is harmless.
> >
> > Sorry, I feel that I'm missing something here.
> >
> > My reading of the code above is that for the cased types above
> > (echo, echo reply, ...) the id is present. Otherwise it is not.
> > My idea would be to put a check for those types in a helper.
> >
> 
> Something like icmp_has_id(), I like it.
> 
> > I do agree that the override you have used is harmless enough
> > in the context of the only user of the id which appears in
> > the following patch of this series.
> >
> >
> > Some other things I noticed in this patch on a second pass:
> >
> > * I think you can remove the icmp field from struct flow_dissector_key_ports
> >
> 
> You mean flow_dissector_key_icmp maybe?

Yes, sorry for the misinformation.

> > * I think that adding icmp to struct flow_keys should be accompanied by
> >   adding ICMP to flow_keys_dissector_symmetric_keys. But I think this is
> >   not desirable outside of the bonding use-case and rather
> >   the bonding driver should define its own structures that
> >   includes the keys it needs - basically copies of struct flow_keys
> >   and flow_keys_dissector_symmetric_keys with some modifications.
> >
> 
> Just flow_keys_dissector_symmetric_keys or flow_keys_dissector_keys too?
> Anyway, it seems that the bonding uses the flow_dissector only when
> using encap2+3 or encap3+4 hashing, which means decap some known
> tunnels (mpls and gre and pppoe I think).

That is the use case I noticed.

In that case it uses skb_flow_dissect_flow_keys() which in turn
uses struct flow_keys and flow_keys_basic_dissector_keys (which is
assigned to flow_keys_dissector_keys.

Sorry about mentioning flow_keys_dissector_symmetric_keys, I think
that was a copy-paste-error on my side.

In any case, my point is that if you update struct flow_keys then likely
some corresponding change should also be made to one or more of
*__dissector_keys. But such a change would have scope outside of bonding,
which is perhaps undesirable. So it might be better to make local
structures and call __skb_flow_dissect from within the bonding code.


As for other use cases, that do not currently use the dissector,
I think you will need to update them too to get then desired new
feature introduced in patch 4 for those use-cases, which I assume is
desired. Perhaps converting those use-cases to use the flow dissector
is a good way forwards. Perhaps not.

> For the other modes it just uses iph_to_flow_copy_v{4,6}addrs() and
> skb_flow_get_ports(), so maybe we can avoid copying that structure.
> 
> > * Modifying flow_keys_have_l4 affects the behaviour of
> >   skb_get_hash_flowi6() but there is not a corresponding update
> >   to flow_keys_have_l4(). I didn't look at all the other call sites
> >   but it strikes me that this is a) a wide-spread behavioural change
> >   and b) is perhaps not required for the bond-use case.
> 
> Right, no need to alter flow_keys_have_l4() at all.
> 
> I'll send a v2 with those suggestions.
> 
> Thanks,
> --
> Matteo Croce
> per aspera ad upstream
>