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

[Simon Horman <simon.horman@netronome.com>]

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@netronome.com> 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.

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

* 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.

* 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.