[PATCH net-next] flow_dissector: Add IPSEC dissectors

[PATCH net-next] flow_dissector: Add IPSEC dissectors

[Ratheesh Kannoth]

 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(-)

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) &&
-- 
2.25.1

Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors

[Leon Romanovsky]

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

RE: Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors

[Ratheesh Kannoth]

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

Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors

[Ido Schimmel]

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

RE: Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors

[Ratheesh Kannoth]

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

Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors

[Simon Horman]

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.

-- 
pw-bot: changes-requested

RE: Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors

[Ratheesh Kannoth]

> 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

Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors

[Simon Horman]

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