[PATCH net-next 0/4] ICMP flow improvements

[PATCH net-next 0/4] ICMP flow improvements

[Matteo Croce]

This series improves the flow inspector handling of ICMP packets:
The first two patches just add some comments in the code which would have saved
me a few minutes of time, and refactor a piece of code.
The third one adds to the flow inspector the capability to extract the
Identifier field, if present, so echo requests and replies are classified
as part of the same flow.
The fourth patch uses the function introduced earlier to the bonding driver,
so echo replies can be balanced across bonding slaves.

Matteo Croce (4):
  flow_dissector: add meaningful comments
  flow_dissector: skip the ICMP dissector for non ICMP packets
  flow_dissector: extract more ICMP information
  bonding: balance ICMP echoes in layer3+4 mode

 drivers/net/bonding/bond_main.c | 22 ++++++--
 include/net/flow_dissector.h    | 11 +++-
 net/core/flow_dissector.c       | 98 +++++++++++++++++++++++----------
 3 files changed, 95 insertions(+), 36 deletions(-)

-- 
2.21.0

[PATCH net-next 1/4] flow_dissector: add meaningful comments

[Matteo Croce]

Documents two piece of code which can't be understood at a glance.

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 include/net/flow_dissector.h | 1 +
 net/core/flow_dissector.c    | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 90bd210be060..7747af3cc500 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -282,6 +282,7 @@ struct flow_keys {
 	struct flow_dissector_key_vlan cvlan;
 	struct flow_dissector_key_keyid keyid;
 	struct flow_dissector_key_ports ports;
+	/* 'addrs' must be the last member */
 	struct flow_dissector_key_addrs addrs;
 };
 
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 7c09d87d3269..affde70dad47 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1374,6 +1374,9 @@ static inline size_t flow_keys_hash_length(const struct flow_keys *flow)
 {
 	size_t diff = FLOW_KEYS_HASH_OFFSET + sizeof(flow->addrs);
 	BUILD_BUG_ON((sizeof(*flow) - FLOW_KEYS_HASH_OFFSET) % sizeof(u32));
+	/* flow.addrs MUST be the last member in struct flow_keys because
+	 * different L3 protocols have different address length
+	 */
 	BUILD_BUG_ON(offsetof(typeof(*flow), addrs) !=
 		     sizeof(*flow) - sizeof(flow->addrs));
 
@@ -1421,6 +1424,9 @@ __be32 flow_get_u32_dst(const struct flow_keys *flow)
 }
 EXPORT_SYMBOL(flow_get_u32_dst);
 
+/* Sort the source and destination IP (and the ports if the IP are the same),
+ * to have consistent hash within the two directions
+ */
 static inline void __flow_hash_consistentify(struct flow_keys *keys)
 {
 	int addr_diff, i;
-- 
2.21.0

[PATCH net-next 2/4] flow_dissector: skip the ICMP dissector for non ICMP packets

[Matteo Croce]

FLOW_DISSECTOR_KEY_ICMP is checked for every packet, not only ICMP ones.
Even if the test overhead is probably negligible, move the
ICMP dissector code under the big 'switch(ip_proto)' so it gets called
only for ICMP packets.

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 net/core/flow_dissector.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index affde70dad47..6443fac65ce8 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -203,6 +203,25 @@ __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().
+ */
+static void __skb_flow_dissect_icmp(const struct sk_buff *skb,
+				    struct flow_dissector *flow_dissector,
+				    void *target_container,
+				    void *data, int thoff, int hlen)
+{
+	struct flow_dissector_key_icmp *key_icmp;
+
+	if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ICMP))
+		return;
+
+	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);
+}
+
 void skb_flow_dissect_meta(const struct sk_buff *skb,
 			   struct flow_dissector *flow_dissector,
 			   void *target_container)
@@ -853,7 +872,6 @@ bool __skb_flow_dissect(const struct net *net,
 	struct flow_dissector_key_basic *key_basic;
 	struct flow_dissector_key_addrs *key_addrs;
 	struct flow_dissector_key_ports *key_ports;
-	struct flow_dissector_key_icmp *key_icmp;
 	struct flow_dissector_key_tags *key_tags;
 	struct flow_dissector_key_vlan *key_vlan;
 	struct bpf_prog *attached = NULL;
@@ -1295,6 +1313,12 @@ bool __skb_flow_dissect(const struct net *net,
 				       data, nhoff, hlen);
 		break;
 
+	case IPPROTO_ICMP:
+	case IPPROTO_ICMPV6:
+		__skb_flow_dissect_icmp(skb, flow_dissector, target_container,
+					data, nhoff, hlen);
+		break;
+
 	default:
 		break;
 	}
@@ -1308,14 +1332,6 @@ bool __skb_flow_dissect(const struct net *net,
 							data, hlen);
 	}
 
-	if (dissector_uses_key(flow_dissector,
-			       FLOW_DISSECTOR_KEY_ICMP)) {
-		key_icmp = skb_flow_dissector_target(flow_dissector,
-						     FLOW_DISSECTOR_KEY_ICMP,
-						     target_container);
-		key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data, hlen);
-	}
-
 	/* Process result of IP proto processing */
 	switch (fdret) {
 	case FLOW_DISSECT_RET_PROTO_AGAIN:
-- 
2.21.0

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

[Matteo Croce]

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;
+	}
+}
+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

[PATCH net-next 4/4] bonding: balance ICMP echoes in layer3+4 mode

[Matteo Croce]

The bonding uses the L4 ports to balance flows between slaves.
As the ICMP protocol has no ports, those packets are sent all to the
same device:

    # tcpdump -qltnni veth0 ip |sed 's/^/0: /' &
    # tcpdump -qltnni veth1 ip |sed 's/^/1: /' &
    # ping -qc1 192.168.0.2
    1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 315, seq 1, length 64
    1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 315, seq 1, length 64
    # ping -qc1 192.168.0.2
    1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 316, seq 1, length 64
    1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 316, seq 1, length 64
    # ping -qc1 192.168.0.2
    1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 317, seq 1, length 64
    1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 317, seq 1, length 64

But some ICMP packets have an Identifier field which is
used to match packets within sessions, let's use this value in the hash
function to balance these packets between bond slaves:

    # ping -qc1 192.168.0.2
    0: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 303, seq 1, length 64
    0: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 303, seq 1, length 64
    # ping -qc1 192.168.0.2
    1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 304, seq 1, length 64
    1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 304, seq 1, length 64

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 drivers/net/bonding/bond_main.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 21d8fcc83c9c..83afb03f4d07 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3267,6 +3267,8 @@ static bool bond_flow_dissect(struct bonding *bond, struct sk_buff *skb,
 		return skb_flow_dissect_flow_keys(skb, fk, 0);
 
 	fk->ports.ports = 0;
+	fk->icmp.icmp = 0;
+	fk->icmp.id = 0;
 	noff = skb_network_offset(skb);
 	if (skb->protocol == htons(ETH_P_IP)) {
 		if (unlikely(!pskb_may_pull(skb, noff + sizeof(*iph))))
@@ -3286,8 +3288,14 @@ static bool bond_flow_dissect(struct bonding *bond, struct sk_buff *skb,
 	} else {
 		return false;
 	}
-	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 && proto >= 0)
-		fk->ports.ports = skb_flow_get_ports(skb, noff, proto);
+	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 && proto >= 0) {
+		if (proto == IPPROTO_ICMP || proto == IPPROTO_ICMPV6)
+			skb_flow_get_icmp_tci(skb, &fk->icmp, skb->data,
+					      skb_transport_offset(skb),
+					      skb_headlen(skb));
+		else
+			fk->ports.ports = skb_flow_get_ports(skb, noff, proto);
+	}
 
 	return true;
 }
@@ -3314,10 +3322,14 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
 		return bond_eth_hash(skb);
 
 	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER23 ||
-	    bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP23)
+	    bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP23) {
 		hash = bond_eth_hash(skb);
-	else
-		hash = (__force u32)flow.ports.ports;
+	} else {
+		if (flow.icmp.id)
+			memcpy(&hash, &flow.icmp, sizeof(hash));
+		else
+			memcpy(&hash, &flow.ports.ports, sizeof(hash));
+	}
 	hash ^= (__force u32)flow_get_u32_dst(&flow) ^
 		(__force u32)flow_get_u32_src(&flow);
 	hash ^= (hash >> 16);
-- 
2.21.0

Re: [PATCH net-next 1/4] flow_dissector: add meaningful comments

[Simon Horman]

On Mon, Oct 21, 2019 at 10:09:45PM +0200, Matteo Croce wrote:
> Documents two piece of code which can't be understood at a glance.
> 
> Signed-off-by: Matteo Croce <mcroce@redhat.com>

Reviewed-by: Simon Horman <simon.horman@netronome.com>

> ---
>  include/net/flow_dissector.h | 1 +
>  net/core/flow_dissector.c    | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index 90bd210be060..7747af3cc500 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -282,6 +282,7 @@ struct flow_keys {
>  	struct flow_dissector_key_vlan cvlan;
>  	struct flow_dissector_key_keyid keyid;
>  	struct flow_dissector_key_ports ports;
> +	/* 'addrs' must be the last member */
>  	struct flow_dissector_key_addrs addrs;
>  };
>  
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 7c09d87d3269..affde70dad47 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -1374,6 +1374,9 @@ static inline size_t flow_keys_hash_length(const struct flow_keys *flow)
>  {
>  	size_t diff = FLOW_KEYS_HASH_OFFSET + sizeof(flow->addrs);
>  	BUILD_BUG_ON((sizeof(*flow) - FLOW_KEYS_HASH_OFFSET) % sizeof(u32));
> +	/* flow.addrs MUST be the last member in struct flow_keys because
> +	 * different L3 protocols have different address length
> +	 */
>  	BUILD_BUG_ON(offsetof(typeof(*flow), addrs) !=
>  		     sizeof(*flow) - sizeof(flow->addrs));
>  
> @@ -1421,6 +1424,9 @@ __be32 flow_get_u32_dst(const struct flow_keys *flow)
>  }
>  EXPORT_SYMBOL(flow_get_u32_dst);
>  
> +/* Sort the source and destination IP (and the ports if the IP are the same),
> + * to have consistent hash within the two directions
> + */
>  static inline void __flow_hash_consistentify(struct flow_keys *keys)
>  {
>  	int addr_diff, i;
> -- 
> 2.21.0
> 

Re: [PATCH net-next 2/4] flow_dissector: skip the ICMP dissector for non ICMP packets

[Simon Horman]

On Mon, Oct 21, 2019 at 10:09:46PM +0200, Matteo Croce wrote:
> FLOW_DISSECTOR_KEY_ICMP is checked for every packet, not only ICMP ones.
> Even if the test overhead is probably negligible, move the
> ICMP dissector code under the big 'switch(ip_proto)' so it gets called
> only for ICMP packets.
> 
> Signed-off-by: Matteo Croce <mcroce@redhat.com>

Reviewed-by: Simon Horman <simon.horman@netronome.com>

> ---
>  net/core/flow_dissector.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index affde70dad47..6443fac65ce8 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -203,6 +203,25 @@ __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().
> + */
> +static void __skb_flow_dissect_icmp(const struct sk_buff *skb,
> +				    struct flow_dissector *flow_dissector,
> +				    void *target_container,
> +				    void *data, int thoff, int hlen)
> +{
> +	struct flow_dissector_key_icmp *key_icmp;
> +
> +	if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ICMP))
> +		return;
> +
> +	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);
> +}
> +
>  void skb_flow_dissect_meta(const struct sk_buff *skb,
>  			   struct flow_dissector *flow_dissector,
>  			   void *target_container)
> @@ -853,7 +872,6 @@ bool __skb_flow_dissect(const struct net *net,
>  	struct flow_dissector_key_basic *key_basic;
>  	struct flow_dissector_key_addrs *key_addrs;
>  	struct flow_dissector_key_ports *key_ports;
> -	struct flow_dissector_key_icmp *key_icmp;
>  	struct flow_dissector_key_tags *key_tags;
>  	struct flow_dissector_key_vlan *key_vlan;
>  	struct bpf_prog *attached = NULL;
> @@ -1295,6 +1313,12 @@ bool __skb_flow_dissect(const struct net *net,
>  				       data, nhoff, hlen);
>  		break;
>  
> +	case IPPROTO_ICMP:
> +	case IPPROTO_ICMPV6:
> +		__skb_flow_dissect_icmp(skb, flow_dissector, target_container,
> +					data, nhoff, hlen);
> +		break;
> +
>  	default:
>  		break;
>  	}
> @@ -1308,14 +1332,6 @@ bool __skb_flow_dissect(const struct net *net,
>  							data, hlen);
>  	}
>  
> -	if (dissector_uses_key(flow_dissector,
> -			       FLOW_DISSECTOR_KEY_ICMP)) {
> -		key_icmp = skb_flow_dissector_target(flow_dissector,
> -						     FLOW_DISSECTOR_KEY_ICMP,
> -						     target_container);
> -		key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data, hlen);
> -	}
> -
>  	/* Process result of IP proto processing */
>  	switch (fdret) {
>  	case FLOW_DISSECT_RET_PROTO_AGAIN:
> -- 
> 2.21.0
> 

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

[Simon Horman]

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
> 

Re: [PATCH net-next 4/4] bonding: balance ICMP echoes in layer3+4 mode

[Simon Horman]

On Mon, Oct 21, 2019 at 10:09:48PM +0200, Matteo Croce wrote:
> The bonding uses the L4 ports to balance flows between slaves.
> As the ICMP protocol has no ports, those packets are sent all to the
> same device:
> 
>     # tcpdump -qltnni veth0 ip |sed 's/^/0: /' &
>     # tcpdump -qltnni veth1 ip |sed 's/^/1: /' &
>     # ping -qc1 192.168.0.2
>     1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 315, seq 1, length 64
>     1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 315, seq 1, length 64
>     # ping -qc1 192.168.0.2
>     1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 316, seq 1, length 64
>     1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 316, seq 1, length 64
>     # ping -qc1 192.168.0.2
>     1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 317, seq 1, length 64
>     1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 317, seq 1, length 64
> 
> But some ICMP packets have an Identifier field which is
> used to match packets within sessions, let's use this value in the hash
> function to balance these packets between bond slaves:
> 
>     # ping -qc1 192.168.0.2
>     0: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 303, seq 1, length 64
>     0: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 303, seq 1, length 64
>     # ping -qc1 192.168.0.2
>     1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 304, seq 1, length 64
>     1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 304, seq 1, length 64
> 
> Signed-off-by: Matteo Croce <mcroce@redhat.com>

I see where this patch is going but it is unclear to me what problem it is
solving. I would expect ICMP traffic to be low volume and thus able to be
handled by a single lower-device of a bond.

...

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

[Matteo Croce]

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

Re: [PATCH net-next 4/4] bonding: balance ICMP echoes in layer3+4 mode

[Matteo Croce]

On Wed, Oct 23, 2019 at 12:01 PM Simon Horman
<simon.horman@netronome.com> wrote:
>
> On Mon, Oct 21, 2019 at 10:09:48PM +0200, Matteo Croce wrote:
> > The bonding uses the L4 ports to balance flows between slaves.
> > As the ICMP protocol has no ports, those packets are sent all to the
> > same device:
> >
> >     # tcpdump -qltnni veth0 ip |sed 's/^/0: /' &
> >     # tcpdump -qltnni veth1 ip |sed 's/^/1: /' &
> >     # ping -qc1 192.168.0.2
> >     1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 315, seq 1, length 64
> >     1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 315, seq 1, length 64
> >     # ping -qc1 192.168.0.2
> >     1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 316, seq 1, length 64
> >     1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 316, seq 1, length 64
> >     # ping -qc1 192.168.0.2
> >     1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 317, seq 1, length 64
> >     1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 317, seq 1, length 64
> >
> > But some ICMP packets have an Identifier field which is
> > used to match packets within sessions, let's use this value in the hash
> > function to balance these packets between bond slaves:
> >
> >     # ping -qc1 192.168.0.2
> >     0: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 303, seq 1, length 64
> >     0: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 303, seq 1, length 64
> >     # ping -qc1 192.168.0.2
> >     1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 304, seq 1, length 64
> >     1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 304, seq 1, length 64
> >
> > Signed-off-by: Matteo Croce <mcroce@redhat.com>
>
> I see where this patch is going but it is unclear to me what problem it is
> solving. I would expect ICMP traffic to be low volume and thus able to be
> handled by a single lower-device of a bond.
>
> ...

Hi,

The problem is not balancing the volume, even if it could increase due
to IoT devices pinging some well known DNS servers to check for
connection.
If a bonding slave is down, people using pings to check for
connectivity could fail to detect a broken link if all the packets are
sent to the alive link.
Anyway, although I didn't measure it, the computational overhead of
this changeset should be minimal, and only affect ICMP packets when
the ICMP dissector is used.

Regards,
-- 
Matteo Croce
per aspera ad upstream

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

[Simon Horman]

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.

Re: [PATCH net-next 4/4] bonding: balance ICMP echoes in layer3+4 mode

[Simon Horman]

On Wed, Oct 23, 2019 at 06:58:16PM +0200, Matteo Croce wrote:
> On Wed, Oct 23, 2019 at 12:01 PM Simon Horman
> <simon.horman@netronome.com> wrote:
> >
> > On Mon, Oct 21, 2019 at 10:09:48PM +0200, Matteo Croce wrote:
> > > The bonding uses the L4 ports to balance flows between slaves.
> > > As the ICMP protocol has no ports, those packets are sent all to the
> > > same device:
> > >
> > >     # tcpdump -qltnni veth0 ip |sed 's/^/0: /' &
> > >     # tcpdump -qltnni veth1 ip |sed 's/^/1: /' &
> > >     # ping -qc1 192.168.0.2
> > >     1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 315, seq 1, length 64
> > >     1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 315, seq 1, length 64
> > >     # ping -qc1 192.168.0.2
> > >     1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 316, seq 1, length 64
> > >     1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 316, seq 1, length 64
> > >     # ping -qc1 192.168.0.2
> > >     1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 317, seq 1, length 64
> > >     1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 317, seq 1, length 64
> > >
> > > But some ICMP packets have an Identifier field which is
> > > used to match packets within sessions, let's use this value in the hash
> > > function to balance these packets between bond slaves:
> > >
> > >     # ping -qc1 192.168.0.2
> > >     0: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 303, seq 1, length 64
> > >     0: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 303, seq 1, length 64
> > >     # ping -qc1 192.168.0.2
> > >     1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 304, seq 1, length 64
> > >     1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 304, seq 1, length 64
> > >
> > > Signed-off-by: Matteo Croce <mcroce@redhat.com>
> >
> > I see where this patch is going but it is unclear to me what problem it is
> > solving. I would expect ICMP traffic to be low volume and thus able to be
> > handled by a single lower-device of a bond.
> >
> > ...
> 
> Hi,
> 
> The problem is not balancing the volume, even if it could increase due
> to IoT devices pinging some well known DNS servers to check for
> connection.
> If a bonding slave is down, people using pings to check for
> connectivity could fail to detect a broken link if all the packets are
> sent to the alive link.
> Anyway, although I didn't measure it, the computational overhead of
> this changeset should be minimal, and only affect ICMP packets when
> the ICMP dissector is used.

So the idea is that by using different id values ping could be used
to probe all lower-devices of a bond? If so then I understand why
you want this and have no particular objection.

Re: [PATCH net-next 0/4] ICMP flow improvements

[David Miller]

From: Matteo Croce <mcroce@redhat.com>
Date: Mon, 21 Oct 2019 22:09:44 +0200

> This series improves the flow inspector handling of ICMP packets:
> The first two patches just add some comments in the code which would have saved
> me a few minutes of time, and refactor a piece of code.
> The third one adds to the flow inspector the capability to extract the
> Identifier field, if present, so echo requests and replies are classified
> as part of the same flow.
> The fourth patch uses the function introduced earlier to the bonding driver,
> so echo replies can be balanced across bonding slaves.

Simon has unaddressed feedback for patch #3, and this has been sitting in that
state for several days.

Please repost this series when everything is sorted, thank you.

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

[Matteo Croce]

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

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

[Simon Horman]

On Fri, Oct 25, 2019 at 02:27:28AM +0200, Matteo Croce wrote:
> On Wed, Oct 23, 2019 at 7:55 PM Simon Horman <simon.horman@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
> 

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

[Matteo Croce]

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.

-- 
Matteo Croce
per aspera ad upstream

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

[Simon Horman]

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.