Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> 
> According to RFC791,the options may appear or not in datagrams.
> They must be implemented by all IP modules (host and gateways).
> What is optional is their transmission in any particular datagram,
> not their implementation.So we have to deal with it during the
> fragmenting process.Add some test data for the IPv4 header optional
> field fragmenting.
> 
> Signed-off-by: Huichao Cai <chcchc88@163.com>
> ---

....

> diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
> index 2e7739d..bcafa29 100644
> --- a/lib/ip_frag/rte_ipv4_fragmentation.c
> +++ b/lib/ip_frag/rte_ipv4_fragmentation.c
> @@ -1,4 +1,4 @@
> -/* SPDX-License-Identifier: BSD-3-Clause
> +/* SPDX-License-Identifier: (BSD-3-Clause OR GPL-2.0)
>   * Copyright(c) 2010-2014 Intel Corporation
>   */
> 
> @@ -12,6 +12,13 @@
> 
>  #include "ip_frag_common.h"
> 
> +/* IP options */
> +#define RTE_IPOPT_COPY				0x80
> +#define RTE_IPOPT_CONTROL			0x00
> +#define RTE_IPOPT_END				(0 | RTE_IPOPT_CONTROL)
> +#define RTE_IPOPT_NOOP				(1 | RTE_IPOPT_CONTROL)
> +#define RTE_IPOPT_COPIED(o)			((o) & RTE_IPOPT_COPY)
> +
>  /* Fragment Offset */
>  #define	RTE_IPV4_HDR_DF_SHIFT			14
>  #define	RTE_IPV4_HDR_MF_SHIFT			13
> @@ -41,6 +48,38 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  		rte_pktmbuf_free(mb[i]);
>  }
> 
> +/*
> + *	Options "fragmenting", just fill options not
> + *	allowed in fragments with NOOPs.
> + *	Simple and stupid 8), but the most efficient way.
> + */
> +static inline void ip_options_fragment(struct rte_ipv4_hdr *iph)
> +{
> +	unsigned char *optptr = (unsigned char *)iph +
> +	    sizeof(struct rte_ipv4_hdr);

As a nit, why not 'uint8_t *', to keep style the same through all file? 

> +	int l = (iph->version_ihl & RTE_IPV4_HDR_IHL_MASK) *
> +	    RTE_IPV4_IHL_MULTIPLIER - sizeof(struct rte_ipv4_hdr);

We already done such calculation in rte_ipv4_fragment_packet(),
so can re-use header_len value here.

> +	int optlen;
> +
> +	while (l > 0) {
> +		switch (*optptr) {
> +		case RTE_IPOPT_END:
> +			return;
> +		case RTE_IPOPT_NOOP:
> +			l--;
> +			optptr++;
> +			continue;
> +		}
> +		optlen = optptr[1];
> +		if (optlen < 2 || optlen > l)

Why optlen==1 is not considered as valid one?

> +			return;
> +		if (!RTE_IPOPT_COPIED(*optptr))
> +			memset(optptr, RTE_IPOPT_NOOP, optlen);
> +		l -= optlen;
> +		optptr += optlen;
> +	}
> +}
> +
>  /**
>   * IPv4 fragmentation.
>   *
> @@ -188,6 +227,17 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  		    (uint16_t)out_pkt->pkt_len,
>  		    flag_offset, fragment_offset, more_in_segs);
> 
> +		/*
> +		 * ANK:

What means 'ANK' here? 

> dirty, but effective trick. Upgrade options only if
> +		 * the segment to be fragmented was THE FIRST (otherwise,
> +		 * options are already fixed) and make it ONCE
> +		 * on the initial mbuf, so that all the following fragments
> +		 * will inherit fixed options.
> +		 */
> +		if ((fragment_offset == 0) &&
> +			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0))
> +			ip_options_fragment(in_hdr);
> +

I see two problems with that approach:
- you modify incoming packet's header - which is the change in behaviour,
  and doesn't look right at all.
- you remove not-copied options from all fragments.
  As I can read RFC 791 - first fragment should have a copy of all options present
  in original packet, while other fragments need to have only those that have to be
  copied.  

>  		fragment_offset = (uint16_t)(fragment_offset +
>  		    out_pkt->pkt_len - header_len);
> 
> --
> 1.8.3.1