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