ip_frag: extend rte_ipv6_frag_get_ipv6_fragment_header()

  Extend rte_ipv6_frag_get_ipv6_fragment_header() to skip over any
other IPv6 extension headers when finding the fragment header.

According to RFC 8200, there is no guarantee that the IPv6
Fragment extension header will come before any other extension
header, even though it is recommended.

Signed-off-by: Cody Doucette <doucette@bu.edu>
Signed-off-by: Qiaobin Fu <qiaobinf@bu.edu>
Reviewed-by: Michel Machado <michel@digirati.com.br>
---
 examples/ip_reassembly/main.c               |  6 ++-
 lib/librte_ip_frag/rte_ip_frag.h            | 41 ++++++++++++++-------
 lib/librte_ip_frag/rte_ip_frag_version.map  |  1 +
 lib/librte_ip_frag/rte_ipv6_fragmentation.c | 38 +++++++++++++++++++
 lib/librte_ip_frag/rte_ipv6_reassembly.c    |  4 +-
 lib/librte_port/rte_port_ras.c              |  6 ++-
 6 files changed, 77 insertions(+), 19 deletions(-)
  

Hi, 

> 
> Extend rte_ipv6_frag_get_ipv6_fragment_header() to skip over any
> other IPv6 extension headers when finding the fragment header.
> 
> According to RFC 8200, there is no guarantee that the IPv6
> Fragment extension header will come before any other extension
> header, even though it is recommended.
> 
> Signed-off-by: Cody Doucette <doucette@bu.edu>
> Signed-off-by: Qiaobin Fu <qiaobinf@bu.edu>
> Reviewed-by: Michel Machado <michel@digirati.com.br>
> ---
>  examples/ip_reassembly/main.c               |  6 ++-
>  lib/librte_ip_frag/rte_ip_frag.h            | 41 ++++++++++++++-------
>  lib/librte_ip_frag/rte_ip_frag_version.map  |  1 +
>  lib/librte_ip_frag/rte_ipv6_fragmentation.c | 38 +++++++++++++++++++
>  lib/librte_ip_frag/rte_ipv6_reassembly.c    |  4 +-
>  lib/librte_port/rte_port_ras.c              |  6 ++-
>  6 files changed, 77 insertions(+), 19 deletions(-)
> 
> diff --git a/examples/ip_reassembly/main.c b/examples/ip_reassembly/main.c
> index 3e8e79c21..13f2b04f1 100644
> --- a/examples/ip_reassembly/main.c
> +++ b/examples/ip_reassembly/main.c
> @@ -367,12 +367,14 @@ reassemble(struct rte_mbuf *m, uint16_t portid, uint32_t queue,
>  		eth_hdr->ether_type = rte_be_to_cpu_16(ETHER_TYPE_IPv4);
>  	} else if (RTE_ETH_IS_IPV6_HDR(m->packet_type)) {
>  		/* if packet is IPv6 */
> -		struct ipv6_extension_fragment *frag_hdr;
> +		const struct ipv6_extension_fragment *frag_hdr;
> +		struct ipv6_extension_fragment frag_hdr_buf;
>  		struct ipv6_hdr *ip_hdr;
> 
>  		ip_hdr = (struct ipv6_hdr *)(eth_hdr + 1);
> 
> -		frag_hdr = rte_ipv6_frag_get_ipv6_fragment_header(ip_hdr);
> +		frag_hdr = rte_ipv6_frag_get_ipv6_fragment_header(m,
> +			ip_hdr, &frag_hdr_buf);
> 
>  		if (frag_hdr != NULL) {
>  			struct rte_mbuf *mo;
> diff --git a/lib/librte_ip_frag/rte_ip_frag.h b/lib/librte_ip_frag/rte_ip_frag.h
> index b3f3f78df..8884c8b31 100644
> --- a/lib/librte_ip_frag/rte_ip_frag.h
> +++ b/lib/librte_ip_frag/rte_ip_frag.h
> @@ -99,6 +99,12 @@ struct rte_ip_frag_tbl {
>  	__extension__ struct ip_frag_pkt pkt[0]; /**< hash table. */
>  };
> 
> +/** IPv6 extension header */
> +struct ipv6_opt_hdr {
> +	uint8_t nexthdr;
> +	uint8_t hdrlen;
> +} __attribute__((packed));
> +
>  /** IPv6 fragment extension header */
>  #define	RTE_IPV6_EHDR_MF_SHIFT			0
>  #define	RTE_IPV6_EHDR_MF_MASK			1
> @@ -208,28 +214,37 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
>  struct rte_mbuf *rte_ipv6_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
>  		struct rte_ip_frag_death_row *dr,
>  		struct rte_mbuf *mb, uint64_t tms, struct ipv6_hdr *ip_hdr,
> -		struct ipv6_extension_fragment *frag_hdr);
> +		const struct ipv6_extension_fragment *frag_hdr);
> +
> +static inline int
> +ipv6_ext_hdr(uint8_t nexthdr)
> +{
> +	/* Find out if nexthdr is an extension header or a protocol. */
> +	return (nexthdr == IPPROTO_HOPOPTS) ||
> +		(nexthdr == IPPROTO_ROUTING) ||
> +		(nexthdr == IPPROTO_FRAGMENT) ||
> +		(nexthdr == IPPROTO_AH) ||
> +		(nexthdr == IPPROTO_NONE) ||
> +		(nexthdr == IPPROTO_DSTOPTS);
> +}
> 
>  /**
>   * Return a pointer to the packet's fragment header, if found.
> - * It only looks at the extension header that's right after the fixed IPv6
> - * header, and doesn't follow the whole chain of extension headers.
>   *
> - * @param hdr
> + * @param pkt
> + *   Pointer to the mbuf of the packet.
> + * @param ip_hdr
>   *   Pointer to the IPv6 header.
> + * @param frag_hdr
> + *   A pointer to the buffer where the fragment header
> + *   will be copied if it is not contiguous in mbuf data.
>   * @return
>   *   Pointer to the IPv6 fragment extension header, or NULL if it's not
>   *   present.
>   */
> -static inline struct ipv6_extension_fragment *
> -rte_ipv6_frag_get_ipv6_fragment_header(struct ipv6_hdr *hdr)
> -{
> -	if (hdr->proto == IPPROTO_FRAGMENT) {
> -		return (struct ipv6_extension_fragment *) ++hdr;
> -	}
> -	else
> -		return NULL;
> -}
> +const struct ipv6_extension_fragment *rte_ipv6_frag_get_ipv6_fragment_header(
> +		struct rte_mbuf *pkt, const struct ipv6_hdr *ip_hdr,
> +		struct ipv6_extension_fragment *frag_hdr);
> 
>  /**
>   * IPv4 fragmentation.
> diff --git a/lib/librte_ip_frag/rte_ip_frag_version.map b/lib/librte_ip_frag/rte_ip_frag_version.map
> index d1acf07cb..98fe4f2d4 100644
> --- a/lib/librte_ip_frag/rte_ip_frag_version.map
> +++ b/lib/librte_ip_frag/rte_ip_frag_version.map
> @@ -8,6 +8,7 @@ DPDK_2.0 {
>  	rte_ipv4_fragment_packet;
>  	rte_ipv6_frag_reassemble_packet;
>  	rte_ipv6_fragment_packet;
> +	rte_ipv6_frag_get_ipv6_fragment_header;
> 
>  	local: *;
>  };
> diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> index 62a7e4e83..bd847dd3d 100644
> --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> @@ -176,3 +176,41 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
> 
>  	return out_pkt_pos;
>  }
> +

Just a generic thought - might be worse to move functions that parse ipv6 header extentions
and related strcutures into rte_net.
I am sure they might be reused by some other code.

> +const struct ipv6_extension_fragment *
> +rte_ipv6_frag_get_ipv6_fragment_header(struct rte_mbuf *pkt,
> +	const struct ipv6_hdr *ip_hdr,
> +	struct ipv6_extension_fragment *frag_hdr)
> +{
> +	size_t offset = sizeof(struct ipv6_hdr);
> +	uint8_t nexthdr = ip_hdr->proto;
> +
> +	while (ipv6_ext_hdr(nexthdr)) {
> +		struct ipv6_opt_hdr opt;
> +		const struct ipv6_opt_hdr *popt = rte_pktmbuf_read(pkt,
> +			offset, sizeof(opt), &opt);

pktmbuf_read() is quite heavy-weight one.
Do we really need it here?
From my perspective - add an assumption that all whole IPv6 header will be inside
one segment seems reasonable enough.
Konstantin 


> +		if (popt == NULL)
> +			return NULL;
> +
> +		switch (nexthdr) {
> +		case IPPROTO_NONE:
> +			return NULL;
> +
> +		case IPPROTO_FRAGMENT:
> +			return rte_pktmbuf_read(pkt, offset,
> +				sizeof(*frag_hdr), frag_hdr);
> +
> +		case IPPROTO_AH:
> +			offset += (popt->hdrlen + 2) << 2;
> +			break;
> +
> +		default:
> +			offset += (popt->hdrlen + 1) << 3;
> +			break;
> +		}
> +
> +		nexthdr = popt->nexthdr;
> +	}
> +
> +	return NULL;
> +}
> diff --git a/lib/librte_ip_frag/rte_ipv6_reassembly.c b/lib/librte_ip_frag/rte_ipv6_reassembly.c
> index db249fe60..b2d67a3f0 100644
> --- a/lib/librte_ip_frag/rte_ipv6_reassembly.c
> +++ b/lib/librte_ip_frag/rte_ipv6_reassembly.c
> @@ -135,8 +135,8 @@ ipv6_frag_reassemble(struct ip_frag_pkt *fp)
>  #define FRAG_OFFSET(x) (rte_cpu_to_be_16(x) >> 3)
>  struct rte_mbuf *
>  rte_ipv6_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
> -		struct rte_ip_frag_death_row *dr, struct rte_mbuf *mb, uint64_t tms,
> -		struct ipv6_hdr *ip_hdr, struct ipv6_extension_fragment *frag_hdr)
> +	struct rte_ip_frag_death_row *dr, struct rte_mbuf *mb, uint64_t tms,
> +	struct ipv6_hdr *ip_hdr, const struct ipv6_extension_fragment *frag_hdr)
>  {
>  	struct ip_frag_pkt *fp;
>  	struct ip_frag_key key;
> diff --git a/lib/librte_port/rte_port_ras.c b/lib/librte_port/rte_port_ras.c
> index c8b2e19bf..28764f744 100644
> --- a/lib/librte_port/rte_port_ras.c
> +++ b/lib/librte_port/rte_port_ras.c
> @@ -184,9 +184,11 @@ process_ipv6(struct rte_port_ring_writer_ras *p, struct rte_mbuf *pkt)
>  	/* Assume there is no ethernet header */
>  	struct ipv6_hdr *pkt_hdr = rte_pktmbuf_mtod(pkt, struct ipv6_hdr *);
> 
> -	struct ipv6_extension_fragment *frag_hdr;
> +	const struct ipv6_extension_fragment *frag_hdr;
> +	struct ipv6_extension_fragment frag_hdr_buf;
>  	uint16_t frag_data = 0;
> -	frag_hdr = rte_ipv6_frag_get_ipv6_fragment_header(pkt_hdr);
> +	frag_hdr = rte_ipv6_frag_get_ipv6_fragment_header(pkt, pkt_hdr,
> +		&frag_hdr_buf);
>  	if (frag_hdr != NULL)
>  		frag_data = rte_be_to_cpu_16(frag_hdr->frag_data);
> 
> --
> 2.17.1
  

Hi,

> Just a generic thought - might be worse to move functions that parse ipv6 header extentions
> and related strcutures into rte_net.
> I am sure they might be reused by some other code.

Sorry, I am misunderstanding. Do you mean it might be better to move
struct ipv6_opt_hdr and ipv6_ext_hdr() into rte_net since they are not
fragmentation specific? That seems fine to me.

> pktmbuf_read() is quite heavy-weight one.
> Do we really need it here?
> From my perspective - add an assumption that all whole IPv6 header will be inside
> one segment seems reasonable enough.

It is my understanding that rte_pktmbuf_read() will almost always just
invoke a light weight rte_pktmbuf_mtod_offset(). It only runs the
heavy weight __rte_pktmbuf_read() in the case that the assumption you
mentioned is broken.

Thanks for looking!
Cody
  

Hi Cody,

> 
> Hi,
> 
> > Just a generic thought - might be worse to move functions that parse ipv6 header extentions
> > and related strcutures into rte_net.
> > I am sure they might be reused by some other code.
> 
> Sorry, I am misunderstanding. Do you mean it might be better to move
> struct ipv6_opt_hdr and ipv6_ext_hdr() into rte_net since they are not
> fragmentation specific? That seems fine to me.

Yes, that's was my thought.

> 
> > pktmbuf_read() is quite heavy-weight one.
> > Do we really need it here?
> > From my perspective - add an assumption that all whole IPv6 header will be inside
> > one segment seems reasonable enough.
> 
> It is my understanding that rte_pktmbuf_read() will almost always just
> invoke a light weight rte_pktmbuf_mtod_offset(). It only runs the
> heavy weight __rte_pktmbuf_read() in the case that the assumption you
> mentioned is broken.

Ah, yes you right.
Konstantin