| Message ID | 20191021200948.23775-4-mcroce@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | ICMP flow improvements | expand |
On Mon, Oct 21, 2019 at 10:09:47PM +0200, Matteo Croce wrote: > The ICMP flow dissector currently parses only the Type and Code fields. > Some ICMP packets (echo, timestamp) have a 16 bit Identifier field which > is used to correlate packets. > Add such field in flow_dissector_key_icmp and replace skb_flow_get_be16() > with a more complex function which populate this field. > > Signed-off-by: Matteo Croce <mcroce@redhat.com> > --- > include/net/flow_dissector.h | 10 +++++- > net/core/flow_dissector.c | 64 ++++++++++++++++++++++-------------- > 2 files changed, 49 insertions(+), 25 deletions(-) > > diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h > index 7747af3cc500..86c6bf5eab31 100644 > --- a/include/net/flow_dissector.h > +++ b/include/net/flow_dissector.h > @@ -6,6 +6,8 @@ > #include <linux/in6.h> > #include <uapi/linux/if_ether.h> > > +struct sk_buff; > + > /** > * struct flow_dissector_key_control: > * @thoff: Transport header offset > @@ -160,6 +162,7 @@ struct flow_dissector_key_ports { > * icmp: ICMP type (high) and code (low) > * type: ICMP type > * code: ICMP code > + * id: session identifier > */ > struct flow_dissector_key_icmp { > union { > @@ -169,6 +172,7 @@ struct flow_dissector_key_icmp { > u8 code; > }; > }; > + u16 id; > }; > > /** > @@ -282,6 +286,7 @@ struct flow_keys { > struct flow_dissector_key_vlan cvlan; > struct flow_dissector_key_keyid keyid; > struct flow_dissector_key_ports ports; > + struct flow_dissector_key_icmp icmp; > /* 'addrs' must be the last member */ > struct flow_dissector_key_addrs addrs; > }; > @@ -312,10 +317,13 @@ void make_flow_keys_digest(struct flow_keys_digest *digest, > > static inline bool flow_keys_have_l4(const struct flow_keys *keys) > { > - return (keys->ports.ports || keys->tags.flow_label); > + return keys->ports.ports || keys->tags.flow_label || keys->icmp.id; > } > > u32 flow_hash_from_keys(struct flow_keys *keys); > +void skb_flow_get_icmp_tci(const struct sk_buff *skb, > + struct flow_dissector_key_icmp *key_icmp, > + void *data, int thoff, int hlen); > > static inline bool dissector_uses_key(const struct flow_dissector *flow_dissector, > enum flow_dissector_key_id key_id) > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > index 6443fac65ce8..90dcf6f2ef19 100644 > --- a/net/core/flow_dissector.c > +++ b/net/core/flow_dissector.c > @@ -147,27 +147,6 @@ int skb_flow_dissector_bpf_prog_detach(const union bpf_attr *attr) > mutex_unlock(&flow_dissector_mutex); > return 0; > } > -/** > - * skb_flow_get_be16 - extract be16 entity > - * @skb: sk_buff to extract from > - * @poff: offset to extract at > - * @data: raw buffer pointer to the packet > - * @hlen: packet header length > - * > - * The function will try to retrieve a be32 entity at > - * offset poff > - */ > -static __be16 skb_flow_get_be16(const struct sk_buff *skb, int poff, > - void *data, int hlen) > -{ > - __be16 *u, _u; > - > - u = __skb_header_pointer(skb, poff, sizeof(_u), data, hlen, &_u); > - if (u) > - return *u; > - > - return 0; > -} > > /** > * __skb_flow_get_ports - extract the upper layer ports and return them > @@ -203,8 +182,44 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto, > } > EXPORT_SYMBOL(__skb_flow_get_ports); > > -/* If FLOW_DISSECTOR_KEY_ICMP is set, get the Type and Code from an ICMP packet > - * using skb_flow_get_be16(). > +/** > + * skb_flow_get_icmp_tci - extract ICMP(6) Type, Code and Identifier fields > + * @skb: sk_buff to extract from > + * @key_icmp: struct flow_dissector_key_icmp to fill > + * @data: raw buffer pointer to the packet > + * @toff: offset to extract at > + * @hlen: packet header length > + */ > +void skb_flow_get_icmp_tci(const struct sk_buff *skb, > + struct flow_dissector_key_icmp *key_icmp, > + void *data, int thoff, int hlen) > +{ > + struct icmphdr *ih, _ih; > + > + ih = __skb_header_pointer(skb, thoff, sizeof(_ih), data, hlen, &_ih); > + if (!ih) > + return; > + > + key_icmp->type = ih->type; > + key_icmp->code = ih->code; > + key_icmp->id = 0; > + 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. > + } > +} > +EXPORT_SYMBOL(skb_flow_get_icmp_tci); > + > +/* If FLOW_DISSECTOR_KEY_ICMP is set, dissect an ICMP packet > + * using skb_flow_get_icmp_tci(). > */ > static void __skb_flow_dissect_icmp(const struct sk_buff *skb, > struct flow_dissector *flow_dissector, > @@ -219,7 +234,8 @@ static void __skb_flow_dissect_icmp(const struct sk_buff *skb, > key_icmp = skb_flow_dissector_target(flow_dissector, > FLOW_DISSECTOR_KEY_ICMP, > target_container); > - key_icmp->icmp = skb_flow_get_be16(skb, thoff, data, hlen); > + > + skb_flow_get_icmp_tci(skb, key_icmp, data, thoff, hlen); > } > > void skb_flow_dissect_meta(const struct sk_buff *skb, > -- > 2.21.0 >
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. -- Matteo Croce per aspera ad upstream
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.
On Wed, Oct 23, 2019 at 7:55 PM Simon Horman <simon.horman@netronome.com> 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@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. > 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? > * 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). 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
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@netronome.com> 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@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. > > > > 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 >
On Fri, Oct 25, 2019 at 8:29 AM Simon Horman <simon.horman@netronome.com> wrote: > > 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@netronome.com> 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@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. > > > > > > > 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. > np > 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. > What drawbacks will it have to have the ICMP dissector enabled with flow_keys_dissector_keys? I see three options here: 1. add the ICMP key in flow_keys_dissector_keys and change the flow_dissector behaviour, when dealing with echoes 2. do a local copy in the bonding code 3. leave flow_keys_dissector_keys as is, so the bonding will balance echoes only when not decapping tunnels I don't really know if option 1 could be a bug or a feature, sure option 2 is safer. That can be changed later easily anyway. > > 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. > I don't really know why the bonding doesn't use the dissector. Performance? Anyway, maybe converting the bonding to the flow_dissector would make sense, this can be done in the future. I have to talk with the bonding maintainers to understand what's behind this choice.
On Fri, Oct 25, 2019 at 08:24:20PM +0200, Matteo Croce wrote: > On Fri, Oct 25, 2019 at 8:29 AM Simon Horman <simon.horman@netronome.com> wrote: > > > > 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@netronome.com> 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@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. > > > > > > > > > > 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. > > > > np > > > 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. > > > > What drawbacks will it have to have the ICMP dissector enabled with > flow_keys_dissector_keys? 1. All callers of skb_flow_dissect_flow_keys() (and any other users of flow_keys_dissector_keys) will incur the cost of extracting ICMP headers for ICMP packets, this was not previously the case. 2. The behaviour of callers of skb_flow_dissect_flow_keys() may change. In particular ___skb_get_hash() will take into account ICMP headers for ICMP packets, which was not previously the case. Perhaps other side affects for other users, I have not audited them. > I see three options here: > 1. add the ICMP key in flow_keys_dissector_keys and change the > flow_dissector behaviour, when dealing with echoes > 2. do a local copy in the bonding code > 3. leave flow_keys_dissector_keys as is, so the bonding will balance > echoes only when not decapping tunnels I'm not sure that I follow option 3. I think that option 1 is not preferable due to side effects on other users. > I don't really know if option 1 could be a bug or a feature, sure > option 2 is safer. That can be changed later easily anyway. I agree option 2 seems safer. > > 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. > > > > I don't really know why the bonding doesn't use the dissector. > Performance? Anyway, maybe converting the bonding to > the flow_dissector would make sense, this can be done in the future. > I have to talk with the bonding maintainers to understand what's > behind this choice. I am not sure either but I think that any change should check for performance regressions. I think there is also the issue of for which hashing options using ICMP fields is appropriate, but perhaps it is all of them.
diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h index 7747af3cc500..86c6bf5eab31 100644 --- a/include/net/flow_dissector.h +++ b/include/net/flow_dissector.h @@ -6,6 +6,8 @@ #include <linux/in6.h> #include <uapi/linux/if_ether.h> +struct sk_buff; + /** * struct flow_dissector_key_control: * @thoff: Transport header offset @@ -160,6 +162,7 @@ struct flow_dissector_key_ports { * icmp: ICMP type (high) and code (low) * type: ICMP type * code: ICMP code + * id: session identifier */ struct flow_dissector_key_icmp { union { @@ -169,6 +172,7 @@ struct flow_dissector_key_icmp { u8 code; }; }; + u16 id; }; /** @@ -282,6 +286,7 @@ struct flow_keys { struct flow_dissector_key_vlan cvlan; struct flow_dissector_key_keyid keyid; struct flow_dissector_key_ports ports; + struct flow_dissector_key_icmp icmp; /* 'addrs' must be the last member */ struct flow_dissector_key_addrs addrs; }; @@ -312,10 +317,13 @@ void make_flow_keys_digest(struct flow_keys_digest *digest, static inline bool flow_keys_have_l4(const struct flow_keys *keys) { - return (keys->ports.ports || keys->tags.flow_label); + return keys->ports.ports || keys->tags.flow_label || keys->icmp.id; } u32 flow_hash_from_keys(struct flow_keys *keys); +void skb_flow_get_icmp_tci(const struct sk_buff *skb, + struct flow_dissector_key_icmp *key_icmp, + void *data, int thoff, int hlen); static inline bool dissector_uses_key(const struct flow_dissector *flow_dissector, enum flow_dissector_key_id key_id) diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c index 6443fac65ce8..90dcf6f2ef19 100644 --- a/net/core/flow_dissector.c +++ b/net/core/flow_dissector.c @@ -147,27 +147,6 @@ int skb_flow_dissector_bpf_prog_detach(const union bpf_attr *attr) mutex_unlock(&flow_dissector_mutex); return 0; } -/** - * skb_flow_get_be16 - extract be16 entity - * @skb: sk_buff to extract from - * @poff: offset to extract at - * @data: raw buffer pointer to the packet - * @hlen: packet header length - * - * The function will try to retrieve a be32 entity at - * offset poff - */ -static __be16 skb_flow_get_be16(const struct sk_buff *skb, int poff, - void *data, int hlen) -{ - __be16 *u, _u; - - u = __skb_header_pointer(skb, poff, sizeof(_u), data, hlen, &_u); - if (u) - return *u; - - return 0; -} /** * __skb_flow_get_ports - extract the upper layer ports and return them @@ -203,8 +182,44 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto, } EXPORT_SYMBOL(__skb_flow_get_ports); -/* If FLOW_DISSECTOR_KEY_ICMP is set, get the Type and Code from an ICMP packet - * using skb_flow_get_be16(). +/** + * skb_flow_get_icmp_tci - extract ICMP(6) Type, Code and Identifier fields + * @skb: sk_buff to extract from + * @key_icmp: struct flow_dissector_key_icmp to fill + * @data: raw buffer pointer to the packet + * @toff: offset to extract at + * @hlen: packet header length + */ +void skb_flow_get_icmp_tci(const struct sk_buff *skb, + struct flow_dissector_key_icmp *key_icmp, + void *data, int thoff, int hlen) +{ + struct icmphdr *ih, _ih; + + ih = __skb_header_pointer(skb, thoff, sizeof(_ih), data, hlen, &_ih); + if (!ih) + return; + + key_icmp->type = ih->type; + key_icmp->code = ih->code; + key_icmp->id = 0; + 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; + } +} +EXPORT_SYMBOL(skb_flow_get_icmp_tci); + +/* If FLOW_DISSECTOR_KEY_ICMP is set, dissect an ICMP packet + * using skb_flow_get_icmp_tci(). */ static void __skb_flow_dissect_icmp(const struct sk_buff *skb, struct flow_dissector *flow_dissector, @@ -219,7 +234,8 @@ static void __skb_flow_dissect_icmp(const struct sk_buff *skb, key_icmp = skb_flow_dissector_target(flow_dissector, FLOW_DISSECTOR_KEY_ICMP, target_container); - key_icmp->icmp = skb_flow_get_be16(skb, thoff, data, hlen); + + skb_flow_get_icmp_tci(skb, key_icmp, data, thoff, hlen); } void skb_flow_dissect_meta(const struct sk_buff *skb,
The ICMP flow dissector currently parses only the Type and Code fields. Some ICMP packets (echo, timestamp) have a 16 bit Identifier field which is used to correlate packets. Add such field in flow_dissector_key_icmp and replace skb_flow_get_be16() with a more complex function which populate this field. Signed-off-by: Matteo Croce <mcroce@redhat.com> --- include/net/flow_dissector.h | 10 +++++- net/core/flow_dissector.c | 64 ++++++++++++++++++++++-------------- 2 files changed, 49 insertions(+), 25 deletions(-)