| Message ID | 20230725032451.505189-1-rkannoth@marvell.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] flow_dissector: Add IPSEC dissectors | expand |
On Tue, Jul 25, 2023 at 08:54:51AM +0530, Ratheesh Kannoth wrote: > Support for dissecting IPSEC field SPI (which is > 32bits in size) for ESP and AH packets. Please don't add extra space in the beginning of lines. > > This implementation does not support NAT-T > (encapsulation of ESP packets over UDP). And how will user distinguish kernel which supports flow_dissector with NAT-T vs. without NAT-T? > > Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com> > --- > include/net/flow_dissector.h | 9 ++++++ > include/net/flow_offload.h | 6 ++++ > include/uapi/linux/pkt_cls.h | 3 ++ > net/core/flow_dissector.c | 53 +++++++++++++++++++++++++++++++++++- > net/core/flow_offload.c | 7 +++++ > net/sched/cls_flower.c | 18 ++++++++++++ > 6 files changed, 95 insertions(+), 1 deletion(-) > > diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h > index 8664ed4fbbdf..ffec739f049a 100644 > --- a/include/net/flow_dissector.h > +++ b/include/net/flow_dissector.h > @@ -301,6 +301,14 @@ struct flow_dissector_key_l2tpv3 { > __be32 session_id; > }; > > +/** > + * struct flow_dissector_key_ipsec: > + * @spi: identifier for a ipsec connection > + */ > +struct flow_dissector_key_ipsec { > + __be32 spi; > +}; > + > /** > * struct flow_dissector_key_cfm > * @mdl_ver: maintenance domain level (mdl) and cfm protocol version > @@ -353,6 +361,7 @@ enum flow_dissector_key_id { > FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */ > FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe */ > FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3 */ > + FLOW_DISSECTOR_KEY_IPSEC, /* struct flow_dissector_key_ipsec */ > FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */ > > FLOW_DISSECTOR_KEY_MAX, > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > index 118082eae48c..9efa9a59e81f 100644 > --- a/include/net/flow_offload.h > +++ b/include/net/flow_offload.h > @@ -64,6 +64,10 @@ struct flow_match_tcp { > struct flow_dissector_key_tcp *key, *mask; > }; > > +struct flow_match_ipsec { > + struct flow_dissector_key_ipsec *key, *mask; > +}; > + > struct flow_match_mpls { > struct flow_dissector_key_mpls *key, *mask; > }; > @@ -116,6 +120,8 @@ void flow_rule_match_ports_range(const struct flow_rule *rule, > struct flow_match_ports_range *out); > void flow_rule_match_tcp(const struct flow_rule *rule, > struct flow_match_tcp *out); > +void flow_rule_match_ipsec(const struct flow_rule *rule, > + struct flow_match_ipsec *out); > void flow_rule_match_icmp(const struct flow_rule *rule, > struct flow_match_icmp *out); > void flow_rule_match_mpls(const struct flow_rule *rule, > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h > index 7865f5a9885b..a90b0e3d351f 100644 > --- a/include/uapi/linux/pkt_cls.h > +++ b/include/uapi/linux/pkt_cls.h > @@ -594,6 +594,9 @@ enum { > > TCA_FLOWER_KEY_L2TPV3_SID, /* be32 */ > > + TCA_FLOWER_KEY_SPI, /* be32 */ > + TCA_FLOWER_KEY_SPI_MASK, /* be32 */ > + > TCA_FLOWER_L2_MISS, /* u8 */ > > TCA_FLOWER_KEY_CFM, /* nested */ > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > index 85a2d0d9bd39..7162594d7940 100644 > --- a/net/core/flow_dissector.c > +++ b/net/core/flow_dissector.c > @@ -205,6 +205,50 @@ static void __skb_flow_dissect_icmp(const struct sk_buff *skb, > skb_flow_get_icmp_tci(skb, key_icmp, data, thoff, hlen); > } > > +static void __skb_flow_dissect_ah(const struct sk_buff *skb, > + struct flow_dissector *flow_dissector, > + void *target_container, const void *data, > + int nhoff, int hlen) > +{ > + struct flow_dissector_key_ipsec *key_ah; > + struct ip_auth_hdr _hdr, *hdr; In tunnel mode, this struct is ip_esp_hdr and not ip_auth_hdr. Thanks > + > + if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_IPSEC)) > + return; > + > + hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr); > + if (!hdr) > + return; > + > + key_ah = skb_flow_dissector_target(flow_dissector, > + FLOW_DISSECTOR_KEY_IPSEC, > + target_container); > + > + key_ah->spi = hdr->spi; > +} > + > +static void __skb_flow_dissect_esp(const struct sk_buff *skb, > + struct flow_dissector *flow_dissector, > + void *target_container, const void *data, > + int nhoff, int hlen) > +{ > + struct flow_dissector_key_ipsec *key_esp; > + struct ip_esp_hdr _hdr, *hdr; > + > + if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_IPSEC)) > + return; > + > + hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr); > + if (!hdr) > + return; > + > + key_esp = skb_flow_dissector_target(flow_dissector, > + FLOW_DISSECTOR_KEY_IPSEC, > + target_container); > + > + key_esp->spi = hdr->spi; > +} > + > static void __skb_flow_dissect_l2tpv3(const struct sk_buff *skb, > struct flow_dissector *flow_dissector, > void *target_container, const void *data, > @@ -1571,7 +1615,14 @@ bool __skb_flow_dissect(const struct net *net, > __skb_flow_dissect_l2tpv3(skb, flow_dissector, target_container, > data, nhoff, hlen); > break; > - > + case IPPROTO_ESP: > + __skb_flow_dissect_esp(skb, flow_dissector, target_container, > + data, nhoff, hlen); > + break; > + case IPPROTO_AH: > + __skb_flow_dissect_ah(skb, flow_dissector, target_container, > + data, nhoff, hlen); > + break; > default: > break; > } > diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c > index acfc1f88ea79..bc5169482710 100644 > --- a/net/core/flow_offload.c > +++ b/net/core/flow_offload.c > @@ -146,6 +146,13 @@ void flow_rule_match_tcp(const struct flow_rule *rule, > } > EXPORT_SYMBOL(flow_rule_match_tcp); > > +void flow_rule_match_ipsec(const struct flow_rule *rule, > + struct flow_match_ipsec *out) > +{ > + FLOW_DISSECTOR_MATCH(rule, FLOW_DISSECTOR_KEY_IPSEC, out); > +} > +EXPORT_SYMBOL(flow_rule_match_ipsec); > + > void flow_rule_match_icmp(const struct flow_rule *rule, > struct flow_match_icmp *out) > { > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > index 8da9d039d964..7ba839057d3c 100644 > --- a/net/sched/cls_flower.c > +++ b/net/sched/cls_flower.c > @@ -72,6 +72,7 @@ struct fl_flow_key { > struct flow_dissector_key_num_of_vlans num_of_vlans; > struct flow_dissector_key_pppoe pppoe; > struct flow_dissector_key_l2tpv3 l2tpv3; > + struct flow_dissector_key_ipsec ipsec; > struct flow_dissector_key_cfm cfm; > } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */ > > @@ -726,6 +727,8 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = { > [TCA_FLOWER_KEY_PPPOE_SID] = { .type = NLA_U16 }, > [TCA_FLOWER_KEY_PPP_PROTO] = { .type = NLA_U16 }, > [TCA_FLOWER_KEY_L2TPV3_SID] = { .type = NLA_U32 }, > + [TCA_FLOWER_KEY_SPI] = { .type = NLA_U32 }, > + [TCA_FLOWER_KEY_SPI_MASK] = { .type = NLA_U32 }, > [TCA_FLOWER_L2_MISS] = NLA_POLICY_MAX(NLA_U8, 1), > [TCA_FLOWER_KEY_CFM] = { .type = NLA_NESTED }, > }; > @@ -1894,6 +1897,13 @@ static int fl_set_key(struct net *net, struct nlattr **tb, > return ret; > } > > + if (tb[TCA_FLOWER_KEY_SPI]) { > + fl_set_key_val(tb, &key->ipsec.spi, > + TCA_FLOWER_KEY_SPI, > + &mask->ipsec.spi, TCA_FLOWER_KEY_SPI_MASK, > + sizeof(key->ipsec.spi)); > + } > + > if (tb[TCA_FLOWER_KEY_ENC_IPV4_SRC] || > tb[TCA_FLOWER_KEY_ENC_IPV4_DST]) { > key->enc_control.addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS; > @@ -2066,6 +2076,8 @@ static void fl_init_dissector(struct flow_dissector *dissector, > FLOW_DISSECTOR_KEY_PPPOE, pppoe); > FL_KEY_SET_IF_MASKED(mask, keys, cnt, > FLOW_DISSECTOR_KEY_L2TPV3, l2tpv3); > + FL_KEY_SET_IF_MASKED(mask, keys, cnt, > + FLOW_DISSECTOR_KEY_IPSEC, ipsec); > FL_KEY_SET_IF_MASKED(mask, keys, cnt, > FLOW_DISSECTOR_KEY_CFM, cfm); > > @@ -3364,6 +3376,12 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net, > sizeof(key->l2tpv3.session_id))) > goto nla_put_failure; > > + if (key->ipsec.spi && > + fl_dump_key_val(skb, &key->ipsec.spi, TCA_FLOWER_KEY_SPI, > + &mask->ipsec.spi, TCA_FLOWER_KEY_SPI_MASK, > + sizeof(key->ipsec.spi))) > + goto nla_put_failure; > + > if ((key->basic.ip_proto == IPPROTO_TCP || > key->basic.ip_proto == IPPROTO_UDP || > key->basic.ip_proto == IPPROTO_SCTP) && > -- > 2.25.1 > >
On Tue, Jul 25, 2023 at 08:54:51AM +0530, Ratheesh Kannoth wrote: > Support for dissecting IPSEC field SPI (which is > 32bits in size) for ESP and AH packets. > > This implementation does not support NAT-T > (encapsulation of ESP packets over UDP). > > Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com> > --- > include/net/flow_dissector.h | 9 ++++++ > include/net/flow_offload.h | 6 ++++ > include/uapi/linux/pkt_cls.h | 3 ++ > net/core/flow_dissector.c | 53 +++++++++++++++++++++++++++++++++++- > net/core/flow_offload.c | 7 +++++ > net/sched/cls_flower.c | 18 ++++++++++++ Please split flow dissector and flower changes into separate patches. Also, you can't add the flow offload bits without a corresponding driver change. Nobody calls the exported flow_rule_match_ipsec() function. [...] > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h > index 7865f5a9885b..a90b0e3d351f 100644 > --- a/include/uapi/linux/pkt_cls.h > +++ b/include/uapi/linux/pkt_cls.h > @@ -594,6 +594,9 @@ enum { > > TCA_FLOWER_KEY_L2TPV3_SID, /* be32 */ > > + TCA_FLOWER_KEY_SPI, /* be32 */ > + TCA_FLOWER_KEY_SPI_MASK, /* be32 */ > + This will break existing user space on new kernels. New attributes must be added at the end. > TCA_FLOWER_L2_MISS, /* u8 */ > > TCA_FLOWER_KEY_CFM, /* nested */
On Tue, Jul 25, 2023 at 08:54:51AM +0530, Ratheesh Kannoth wrote: ... > diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h > index 8664ed4fbbdf..ffec739f049a 100644 > --- a/include/net/flow_dissector.h > +++ b/include/net/flow_dissector.h > @@ -301,6 +301,14 @@ struct flow_dissector_key_l2tpv3 { > __be32 session_id; > }; > > +/** > + * struct flow_dissector_key_ipsec: > + * @spi: identifier for a ipsec connection > + */ > +struct flow_dissector_key_ipsec { > + __be32 spi; > +}; > + > /** > * struct flow_dissector_key_cfm > * @mdl_ver: maintenance domain level (mdl) and cfm protocol version > @@ -353,6 +361,7 @@ enum flow_dissector_key_id { > FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */ > FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe */ > FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3 */ > + FLOW_DISSECTOR_KEY_IPSEC, /* struct flow_dissector_key_ipsec */ > FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */ > > FLOW_DISSECTOR_KEY_MAX, ... Hi Ratheesh, With this change, this enum now has 33 values, excluding FLOW_DISSECTOR_KEY_MAX. I.e the range of values is from 0 to 32. But dissector_uses_key() looks like this: static inline bool dissector_uses_key(const struct flow_dissector *flow_dissector, enum flow_dissector_key_id key_id) { return flow_dissector->used_keys & (1 << key_id); } And the type of the used_keys field of struct flow_dissector is unsigned int, a 32bit entity. So an overflow will now occur if key_id is FLOW_DISSECTOR_KEY_CFM. This is flagged by Sparse.
> From: Leon Romanovsky <leon@kernel.org> > Subject: Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors > > Support for dissecting IPSEC field SPI (which is 32bits in size) for > > ESP and AH packets. > > Please don't add extra space in the beginning of lines. > Ack. > > > > This implementation does not support NAT-T (encapsulation of ESP > > packets over UDP). > > And how will user distinguish kernel which supports flow_dissector with > NAT-T vs. without NAT-T? Ack, will modify code to return error if ip protocol is neither ESP nor AH. > > > > Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com> > > --- > > include/net/flow_dissector.h | 9 ++++++ > > include/net/flow_offload.h | 6 ++++ > > include/uapi/linux/pkt_cls.h | 3 ++ > > net/core/flow_dissector.c | 53 > +++++++++++++++++++++++++++++++++++- > > net/core/flow_offload.c | 7 +++++ > > net/sched/cls_flower.c | 18 ++++++++++++ > > 6 files changed, 95 insertions(+), 1 deletion(-) > > > > diff --git a/include/net/flow_dissector.h > > b/include/net/flow_dissector.h index 8664ed4fbbdf..ffec739f049a 100644 > > --- a/include/net/flow_dissector.h > > +++ b/include/net/flow_dissector.h > > @@ -301,6 +301,14 @@ struct flow_dissector_key_l2tpv3 { > > __be32 session_id; > > }; > > > > +/** > > + * struct flow_dissector_key_ipsec: > > + * @spi: identifier for a ipsec connection */ struct > > +flow_dissector_key_ipsec { > > + __be32 spi; > > +}; > > + > > /** > > * struct flow_dissector_key_cfm > > * @mdl_ver: maintenance domain level (mdl) and cfm protocol version > > @@ -353,6 +361,7 @@ enum flow_dissector_key_id { > > FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct > flow_dissector_key_num_of_vlans */ > > FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe > */ > > FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3 > */ > > + FLOW_DISSECTOR_KEY_IPSEC, /* struct flow_dissector_key_ipsec */ > > FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */ > > > > FLOW_DISSECTOR_KEY_MAX, > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > > index 118082eae48c..9efa9a59e81f 100644 > > --- a/include/net/flow_offload.h > > +++ b/include/net/flow_offload.h > > @@ -64,6 +64,10 @@ struct flow_match_tcp { > > struct flow_dissector_key_tcp *key, *mask; }; > > > > +struct flow_match_ipsec { > > + struct flow_dissector_key_ipsec *key, *mask; }; > > + > > struct flow_match_mpls { > > struct flow_dissector_key_mpls *key, *mask; }; @@ -116,6 +120,8 > @@ > > void flow_rule_match_ports_range(const struct flow_rule *rule, > > struct flow_match_ports_range *out); void > > flow_rule_match_tcp(const struct flow_rule *rule, > > struct flow_match_tcp *out); > > +void flow_rule_match_ipsec(const struct flow_rule *rule, > > + struct flow_match_ipsec *out); > > void flow_rule_match_icmp(const struct flow_rule *rule, > > struct flow_match_icmp *out); > > void flow_rule_match_mpls(const struct flow_rule *rule, diff --git > > a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h index > > 7865f5a9885b..a90b0e3d351f 100644 > > --- a/include/uapi/linux/pkt_cls.h > > +++ b/include/uapi/linux/pkt_cls.h > > @@ -594,6 +594,9 @@ enum { > > > > TCA_FLOWER_KEY_L2TPV3_SID, /* be32 */ > > > > + TCA_FLOWER_KEY_SPI, /* be32 */ > > + TCA_FLOWER_KEY_SPI_MASK, /* be32 */ > > + > > TCA_FLOWER_L2_MISS, /* u8 */ > > > > TCA_FLOWER_KEY_CFM, /* nested */ > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > > index 85a2d0d9bd39..7162594d7940 100644 > > --- a/net/core/flow_dissector.c > > +++ b/net/core/flow_dissector.c > > @@ -205,6 +205,50 @@ static void __skb_flow_dissect_icmp(const struct > sk_buff *skb, > > skb_flow_get_icmp_tci(skb, key_icmp, data, thoff, hlen); } > > > > +static void __skb_flow_dissect_ah(const struct sk_buff *skb, > > + struct flow_dissector *flow_dissector, > > + void *target_container, const void *data, > > + int nhoff, int hlen) > > +{ > > + struct flow_dissector_key_ipsec *key_ah; > > + struct ip_auth_hdr _hdr, *hdr; > > In tunnel mode, this struct is ip_esp_hdr and not ip_auth_hdr. > > Thanks > > > + > > + if (!dissector_uses_key(flow_dissector, > FLOW_DISSECTOR_KEY_IPSEC)) > > + return; > > + > > + hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, > &_hdr); > > + if (!hdr) > > + return; > > + > > + key_ah = skb_flow_dissector_target(flow_dissector, > > + FLOW_DISSECTOR_KEY_IPSEC, > > + target_container); > > + > > + key_ah->spi = hdr->spi; > > +} > > + > > +static void __skb_flow_dissect_esp(const struct sk_buff *skb, > > + struct flow_dissector *flow_dissector, > > + void *target_container, const void *data, > > + int nhoff, int hlen) > > +{ > > + struct flow_dissector_key_ipsec *key_esp; > > + struct ip_esp_hdr _hdr, *hdr; > > + > > + if (!dissector_uses_key(flow_dissector, > FLOW_DISSECTOR_KEY_IPSEC)) > > + return; > > + > > + hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, > &_hdr); > > + if (!hdr) > > + return; > > + > > + key_esp = skb_flow_dissector_target(flow_dissector, > > + FLOW_DISSECTOR_KEY_IPSEC, > > + target_container); > > + > > + key_esp->spi = hdr->spi; > > +} > > + > > static void __skb_flow_dissect_l2tpv3(const struct sk_buff *skb, > > struct flow_dissector *flow_dissector, > > void *target_container, const void *data, > @@ -1571,7 > > +1615,14 @@ bool __skb_flow_dissect(const struct net *net, > > __skb_flow_dissect_l2tpv3(skb, flow_dissector, > target_container, > > data, nhoff, hlen); > > break; > > - > > + case IPPROTO_ESP: > > + __skb_flow_dissect_esp(skb, flow_dissector, > target_container, > > + data, nhoff, hlen); > > + break; > > + case IPPROTO_AH: > > + __skb_flow_dissect_ah(skb, flow_dissector, > target_container, > > + data, nhoff, hlen); > > + break; > > default: > > break; > > } > > diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c index > > acfc1f88ea79..bc5169482710 100644 > > --- a/net/core/flow_offload.c > > +++ b/net/core/flow_offload.c > > @@ -146,6 +146,13 @@ void flow_rule_match_tcp(const struct flow_rule > > *rule, } EXPORT_SYMBOL(flow_rule_match_tcp); > > > > +void flow_rule_match_ipsec(const struct flow_rule *rule, > > + struct flow_match_ipsec *out) > > +{ > > + FLOW_DISSECTOR_MATCH(rule, FLOW_DISSECTOR_KEY_IPSEC, out); > } > > +EXPORT_SYMBOL(flow_rule_match_ipsec); > > + > > void flow_rule_match_icmp(const struct flow_rule *rule, > > struct flow_match_icmp *out) > > { > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index > > 8da9d039d964..7ba839057d3c 100644 > > --- a/net/sched/cls_flower.c > > +++ b/net/sched/cls_flower.c > > @@ -72,6 +72,7 @@ struct fl_flow_key { > > struct flow_dissector_key_num_of_vlans num_of_vlans; > > struct flow_dissector_key_pppoe pppoe; > > struct flow_dissector_key_l2tpv3 l2tpv3; > > + struct flow_dissector_key_ipsec ipsec; > > struct flow_dissector_key_cfm cfm; > > } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons > > as longs. */ > > > > @@ -726,6 +727,8 @@ static const struct nla_policy > fl_policy[TCA_FLOWER_MAX + 1] = { > > [TCA_FLOWER_KEY_PPPOE_SID] = { .type = NLA_U16 }, > > [TCA_FLOWER_KEY_PPP_PROTO] = { .type = NLA_U16 }, > > [TCA_FLOWER_KEY_L2TPV3_SID] = { .type = NLA_U32 }, > > + [TCA_FLOWER_KEY_SPI] = { .type = NLA_U32 }, > > + [TCA_FLOWER_KEY_SPI_MASK] = { .type = NLA_U32 }, > > [TCA_FLOWER_L2_MISS] = > NLA_POLICY_MAX(NLA_U8, 1), > > [TCA_FLOWER_KEY_CFM] = { .type = NLA_NESTED }, > > }; > > @@ -1894,6 +1897,13 @@ static int fl_set_key(struct net *net, struct nlattr > **tb, > > return ret; > > } > > > > + if (tb[TCA_FLOWER_KEY_SPI]) { > > + fl_set_key_val(tb, &key->ipsec.spi, > > + TCA_FLOWER_KEY_SPI, > > + &mask->ipsec.spi, TCA_FLOWER_KEY_SPI_MASK, > > + sizeof(key->ipsec.spi)); > > + } > > + > > if (tb[TCA_FLOWER_KEY_ENC_IPV4_SRC] || > > tb[TCA_FLOWER_KEY_ENC_IPV4_DST]) { > > key->enc_control.addr_type = > FLOW_DISSECTOR_KEY_IPV4_ADDRS; @@ > > -2066,6 +2076,8 @@ static void fl_init_dissector(struct flow_dissector > *dissector, > > FLOW_DISSECTOR_KEY_PPPOE, pppoe); > > FL_KEY_SET_IF_MASKED(mask, keys, cnt, > > FLOW_DISSECTOR_KEY_L2TPV3, l2tpv3); > > + FL_KEY_SET_IF_MASKED(mask, keys, cnt, > > + FLOW_DISSECTOR_KEY_IPSEC, ipsec); > > FL_KEY_SET_IF_MASKED(mask, keys, cnt, > > FLOW_DISSECTOR_KEY_CFM, cfm); > > > > @@ -3364,6 +3376,12 @@ static int fl_dump_key(struct sk_buff *skb, > struct net *net, > > sizeof(key->l2tpv3.session_id))) > > goto nla_put_failure; > > > > + if (key->ipsec.spi && > > + fl_dump_key_val(skb, &key->ipsec.spi, TCA_FLOWER_KEY_SPI, > > + &mask->ipsec.spi, TCA_FLOWER_KEY_SPI_MASK, > > + sizeof(key->ipsec.spi))) > > + goto nla_put_failure; > > + > > if ((key->basic.ip_proto == IPPROTO_TCP || > > key->basic.ip_proto == IPPROTO_UDP || > > key->basic.ip_proto == IPPROTO_SCTP) && > > -- > > 2.25.1 > > > >
> From: Ido Schimmel <idosch@idosch.org> > Sent: Tuesday, July 25, 2023 3:43 PM > Subject: [EXT] Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors > > include/net/flow_dissector.h | 9 ++++++ > > include/net/flow_offload.h | 6 ++++ > > include/uapi/linux/pkt_cls.h | 3 ++ > > net/core/flow_dissector.c | 53 > +++++++++++++++++++++++++++++++++++- > > net/core/flow_offload.c | 7 +++++ > > net/sched/cls_flower.c | 18 ++++++++++++ > > Please split flow dissector and flower changes into separate patches. > Also, you can't add the flow offload bits without a corresponding driver > change. Nobody calls the exported flow_rule_match_ipsec() function. > Ack, will remove offload related code from this patch. > [...] > > > diff --git a/include/uapi/linux/pkt_cls.h > > b/include/uapi/linux/pkt_cls.h index 7865f5a9885b..a90b0e3d351f 100644 > > --- a/include/uapi/linux/pkt_cls.h > > +++ b/include/uapi/linux/pkt_cls.h > > @@ -594,6 +594,9 @@ enum { > > > > TCA_FLOWER_KEY_L2TPV3_SID, /* be32 */ > > > > + TCA_FLOWER_KEY_SPI, /* be32 */ > > + TCA_FLOWER_KEY_SPI_MASK, /* be32 */ > > + > > This will break existing user space on new kernels. New attributes must be > added at the end. > > > TCA_FLOWER_L2_MISS, /* u8 */ > > > > TCA_FLOWER_KEY_CFM, /* nested */
> From: Simon Horman <simon.horman@corigine.com> > Sent: Wednesday, July 26, 2023 1:21 AM > Subject: [EXT] Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors > > FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct > flow_dissector_key_num_of_vlans */ > > FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe > */ > > FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3 > */ > > + FLOW_DISSECTOR_KEY_IPSEC, /* struct flow_dissector_key_ipsec */ > > FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */ > > > > FLOW_DISSECTOR_KEY_MAX, > > ... > > Hi Ratheesh, > > With this change, this enum now has 33 values, excluding > FLOW_DISSECTOR_KEY_MAX. I.e the range of values is from 0 to 32. > > But dissector_uses_key() looks like this: > > > static inline bool dissector_uses_key(const struct flow_dissector > *flow_dissector, > enum flow_dissector_key_id key_id) { > return flow_dissector->used_keys & (1 << key_id); } > > And the type of the used_keys field of struct flow_dissector is unsigned int, a > 32bit entity. > > So an overflow will now occur if key_id is FLOW_DISSECTOR_KEY_CFM. > > This is flagged by Sparse. > Thank you ! 1) How did you run sparse to detect this error. When I ran below command, it did not throw this error/warning ? make C=2 net/core/ V=s sparse version is 0.6.4 2) Is it okay to change variable type of "used_keys" from "unsigned int" to "unsigned long long" to accommodate this. This variable is used at lot of places in the code. -Ratheesh > -- > pw-bot: changes-requested
On Wed, Jul 26, 2023 at 06:34:34AM +0000, Ratheesh Kannoth wrote: > > From: Simon Horman <simon.horman@corigine.com> > > Sent: Wednesday, July 26, 2023 1:21 AM > > Subject: [EXT] Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors > > > > > FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct > > flow_dissector_key_num_of_vlans */ > > > FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe > > */ > > > FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3 > > */ > > > + FLOW_DISSECTOR_KEY_IPSEC, /* struct flow_dissector_key_ipsec */ > > > FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */ > > > > > > FLOW_DISSECTOR_KEY_MAX, > > > > ... > > > > Hi Ratheesh, > > > > With this change, this enum now has 33 values, excluding > > FLOW_DISSECTOR_KEY_MAX. I.e the range of values is from 0 to 32. > > > > But dissector_uses_key() looks like this: > > > > > > static inline bool dissector_uses_key(const struct flow_dissector > > *flow_dissector, > > enum flow_dissector_key_id key_id) { > > return flow_dissector->used_keys & (1 << key_id); } > > > > And the type of the used_keys field of struct flow_dissector is unsigned int, a > > 32bit entity. > > > > So an overflow will now occur if key_id is FLOW_DISSECTOR_KEY_CFM. > > > > This is flagged by Sparse. > > Hi Ratheesh, > Thank you ! > 1) How did you run sparse to detect this error. When I ran below command, it did not throw this error/warning ? > make C=2 net/core/ V=s I ran: make C=2 net/core/flow_dissector.o FWIIW, I also saw the warning using make C=2 net/core/ V=s > sparse version is 0.6.4 I am using sparse git commit ce1a6720f69e, because at one point that was the newest revision at a time the packaged version I had wasn't working for me, and that was the latest commit at the time. https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/ce1a6720f69e > 2) Is it okay to change variable type of "used_keys" from "unsigned int" to "unsigned long long" to accommodate this. > This variable is used at lot of places in the code. I might go for u64. But yes, I suspect making the variable wider is a viable solution. Given that it is widely used it would be good to inspect how it is used to make sure this change makes sense. I would make this change a separate patch if it is more than one line. Also, while you are there, can you fix the spelling mistake in the comment on the 'used_keys' line in struct flow_dissector ? It's the same line that needs to be changed to change the type of 'used_keys'.
diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h index 8664ed4fbbdf..ffec739f049a 100644 --- a/include/net/flow_dissector.h +++ b/include/net/flow_dissector.h @@ -301,6 +301,14 @@ struct flow_dissector_key_l2tpv3 { __be32 session_id; }; +/** + * struct flow_dissector_key_ipsec: + * @spi: identifier for a ipsec connection + */ +struct flow_dissector_key_ipsec { + __be32 spi; +}; + /** * struct flow_dissector_key_cfm * @mdl_ver: maintenance domain level (mdl) and cfm protocol version @@ -353,6 +361,7 @@ enum flow_dissector_key_id { FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */ FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe */ FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3 */ + FLOW_DISSECTOR_KEY_IPSEC, /* struct flow_dissector_key_ipsec */ FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */ FLOW_DISSECTOR_KEY_MAX, diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index 118082eae48c..9efa9a59e81f 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -64,6 +64,10 @@ struct flow_match_tcp { struct flow_dissector_key_tcp *key, *mask; }; +struct flow_match_ipsec { + struct flow_dissector_key_ipsec *key, *mask; +}; + struct flow_match_mpls { struct flow_dissector_key_mpls *key, *mask; }; @@ -116,6 +120,8 @@ void flow_rule_match_ports_range(const struct flow_rule *rule, struct flow_match_ports_range *out); void flow_rule_match_tcp(const struct flow_rule *rule, struct flow_match_tcp *out); +void flow_rule_match_ipsec(const struct flow_rule *rule, + struct flow_match_ipsec *out); void flow_rule_match_icmp(const struct flow_rule *rule, struct flow_match_icmp *out); void flow_rule_match_mpls(const struct flow_rule *rule, diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h index 7865f5a9885b..a90b0e3d351f 100644 --- a/include/uapi/linux/pkt_cls.h +++ b/include/uapi/linux/pkt_cls.h @@ -594,6 +594,9 @@ enum { TCA_FLOWER_KEY_L2TPV3_SID, /* be32 */ + TCA_FLOWER_KEY_SPI, /* be32 */ + TCA_FLOWER_KEY_SPI_MASK, /* be32 */ + TCA_FLOWER_L2_MISS, /* u8 */ TCA_FLOWER_KEY_CFM, /* nested */ diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c index 85a2d0d9bd39..7162594d7940 100644 --- a/net/core/flow_dissector.c +++ b/net/core/flow_dissector.c @@ -205,6 +205,50 @@ static void __skb_flow_dissect_icmp(const struct sk_buff *skb, skb_flow_get_icmp_tci(skb, key_icmp, data, thoff, hlen); } +static void __skb_flow_dissect_ah(const struct sk_buff *skb, + struct flow_dissector *flow_dissector, + void *target_container, const void *data, + int nhoff, int hlen) +{ + struct flow_dissector_key_ipsec *key_ah; + struct ip_auth_hdr _hdr, *hdr; + + if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_IPSEC)) + return; + + hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr); + if (!hdr) + return; + + key_ah = skb_flow_dissector_target(flow_dissector, + FLOW_DISSECTOR_KEY_IPSEC, + target_container); + + key_ah->spi = hdr->spi; +} + +static void __skb_flow_dissect_esp(const struct sk_buff *skb, + struct flow_dissector *flow_dissector, + void *target_container, const void *data, + int nhoff, int hlen) +{ + struct flow_dissector_key_ipsec *key_esp; + struct ip_esp_hdr _hdr, *hdr; + + if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_IPSEC)) + return; + + hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr); + if (!hdr) + return; + + key_esp = skb_flow_dissector_target(flow_dissector, + FLOW_DISSECTOR_KEY_IPSEC, + target_container); + + key_esp->spi = hdr->spi; +} + static void __skb_flow_dissect_l2tpv3(const struct sk_buff *skb, struct flow_dissector *flow_dissector, void *target_container, const void *data, @@ -1571,7 +1615,14 @@ bool __skb_flow_dissect(const struct net *net, __skb_flow_dissect_l2tpv3(skb, flow_dissector, target_container, data, nhoff, hlen); break; - + case IPPROTO_ESP: + __skb_flow_dissect_esp(skb, flow_dissector, target_container, + data, nhoff, hlen); + break; + case IPPROTO_AH: + __skb_flow_dissect_ah(skb, flow_dissector, target_container, + data, nhoff, hlen); + break; default: break; } diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c index acfc1f88ea79..bc5169482710 100644 --- a/net/core/flow_offload.c +++ b/net/core/flow_offload.c @@ -146,6 +146,13 @@ void flow_rule_match_tcp(const struct flow_rule *rule, } EXPORT_SYMBOL(flow_rule_match_tcp); +void flow_rule_match_ipsec(const struct flow_rule *rule, + struct flow_match_ipsec *out) +{ + FLOW_DISSECTOR_MATCH(rule, FLOW_DISSECTOR_KEY_IPSEC, out); +} +EXPORT_SYMBOL(flow_rule_match_ipsec); + void flow_rule_match_icmp(const struct flow_rule *rule, struct flow_match_icmp *out) { diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index 8da9d039d964..7ba839057d3c 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -72,6 +72,7 @@ struct fl_flow_key { struct flow_dissector_key_num_of_vlans num_of_vlans; struct flow_dissector_key_pppoe pppoe; struct flow_dissector_key_l2tpv3 l2tpv3; + struct flow_dissector_key_ipsec ipsec; struct flow_dissector_key_cfm cfm; } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */ @@ -726,6 +727,8 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = { [TCA_FLOWER_KEY_PPPOE_SID] = { .type = NLA_U16 }, [TCA_FLOWER_KEY_PPP_PROTO] = { .type = NLA_U16 }, [TCA_FLOWER_KEY_L2TPV3_SID] = { .type = NLA_U32 }, + [TCA_FLOWER_KEY_SPI] = { .type = NLA_U32 }, + [TCA_FLOWER_KEY_SPI_MASK] = { .type = NLA_U32 }, [TCA_FLOWER_L2_MISS] = NLA_POLICY_MAX(NLA_U8, 1), [TCA_FLOWER_KEY_CFM] = { .type = NLA_NESTED }, }; @@ -1894,6 +1897,13 @@ static int fl_set_key(struct net *net, struct nlattr **tb, return ret; } + if (tb[TCA_FLOWER_KEY_SPI]) { + fl_set_key_val(tb, &key->ipsec.spi, + TCA_FLOWER_KEY_SPI, + &mask->ipsec.spi, TCA_FLOWER_KEY_SPI_MASK, + sizeof(key->ipsec.spi)); + } + if (tb[TCA_FLOWER_KEY_ENC_IPV4_SRC] || tb[TCA_FLOWER_KEY_ENC_IPV4_DST]) { key->enc_control.addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS; @@ -2066,6 +2076,8 @@ static void fl_init_dissector(struct flow_dissector *dissector, FLOW_DISSECTOR_KEY_PPPOE, pppoe); FL_KEY_SET_IF_MASKED(mask, keys, cnt, FLOW_DISSECTOR_KEY_L2TPV3, l2tpv3); + FL_KEY_SET_IF_MASKED(mask, keys, cnt, + FLOW_DISSECTOR_KEY_IPSEC, ipsec); FL_KEY_SET_IF_MASKED(mask, keys, cnt, FLOW_DISSECTOR_KEY_CFM, cfm); @@ -3364,6 +3376,12 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net, sizeof(key->l2tpv3.session_id))) goto nla_put_failure; + if (key->ipsec.spi && + fl_dump_key_val(skb, &key->ipsec.spi, TCA_FLOWER_KEY_SPI, + &mask->ipsec.spi, TCA_FLOWER_KEY_SPI_MASK, + sizeof(key->ipsec.spi))) + goto nla_put_failure; + if ((key->basic.ip_proto == IPPROTO_TCP || key->basic.ip_proto == IPPROTO_UDP || key->basic.ip_proto == IPPROTO_SCTP) &&
Support for dissecting IPSEC field SPI (which is 32bits in size) for ESP and AH packets. This implementation does not support NAT-T (encapsulation of ESP packets over UDP). Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com> --- include/net/flow_dissector.h | 9 ++++++ include/net/flow_offload.h | 6 ++++ include/uapi/linux/pkt_cls.h | 3 ++ net/core/flow_dissector.c | 53 +++++++++++++++++++++++++++++++++++- net/core/flow_offload.c | 7 +++++ net/sched/cls_flower.c | 18 ++++++++++++ 6 files changed, 95 insertions(+), 1 deletion(-)