From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 14D7F459B6;
	Tue, 17 Sep 2024 20:07:51 +0200 (CEST)
Received: from mails.dpdk.org (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id D7BC540280;
	Tue, 17 Sep 2024 20:07:50 +0200 (CEST)
Received: from frasgout.his.huawei.com (frasgout.his.huawei.com
 [185.176.79.56]) by mails.dpdk.org (Postfix) with ESMTP id 07FEC40261
 for <dev@dpdk.org>; Tue, 17 Sep 2024 20:07:50 +0200 (CEST)
Received: from mail.maildlp.com (unknown [172.18.186.31])
 by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4X7V5Q6G4Fz6LCDC;
 Wed, 18 Sep 2024 02:03:38 +0800 (CST)
Received: from frapeml500005.china.huawei.com (unknown [7.182.85.13])
 by mail.maildlp.com (Postfix) with ESMTPS id 6324D140159;
 Wed, 18 Sep 2024 02:07:25 +0800 (CST)
Received: from frapeml500007.china.huawei.com (7.182.85.172) by
 frapeml500005.china.huawei.com (7.182.85.13) with Microsoft SMTP Server
 (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id
 15.1.2507.39; Tue, 17 Sep 2024 20:07:25 +0200
Received: from frapeml500007.china.huawei.com ([7.182.85.172]) by
 frapeml500007.china.huawei.com ([7.182.85.172]) with mapi id 15.01.2507.039;
 Tue, 17 Sep 2024 20:07:25 +0200
From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
To: "vignesh.purushotham.srinivas@ericsson.com"
 <vignesh.purushotham.srinivas@ericsson.com>, "konstantin.v.ananyev@yandex.ru"
 <konstantin.v.ananyev@yandex.ru>
CC: "dev@dpdk.org" <dev@dpdk.org>
Subject: RE: [PATCH] ip_frag: support IPv6 reassembly with extensions
Thread-Topic: [PATCH] ip_frag: support IPv6 reassembly with extensions
Thread-Index: AQHa96p7DJtUreh0vEKAb1S1N5t0lLJcZvLg
Date: Tue, 17 Sep 2024 18:07:25 +0000
Message-ID: <1265a3bd25f3463a898227adbabcb4d6@huawei.com>
References: <20240826112328.3028488-1-vignesh.purushotham.srinivas@ericsson.com>
In-Reply-To: <20240826112328.3028488-1-vignesh.purushotham.srinivas@ericsson.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-originating-ip: [10.206.138.42]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org


> From: Vignesh PS <vignesh.purushotham.srinivas@ericsson.com>
>=20
> Add support to ip_frag library to perform IPv6 reassembly
> when extension headers are present before the fragment
> extension in the packet.
>=20
> Signed-off-by: Vignesh PS <vignesh.purushotham.srinivas@ericsson.com>
> ---
>  .mailmap                          |  1 +
>  lib/ip_frag/ip_frag_common.h      |  2 +
>  lib/ip_frag/ip_reassembly.h       |  2 +
>  lib/ip_frag/rte_ipv6_reassembly.c | 68 +++++++++++++++++++++++++++----
>  4 files changed, 64 insertions(+), 9 deletions(-)
>=20
> diff --git a/.mailmap b/.mailmap
> index 4a508bafad..69b229a5b7 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1548,6 +1548,7 @@ Viacheslav Ovsiienko <viacheslavo@nvidia.com> <viac=
heslavo@mellanox.com>
>  Victor Kaplansky <victork@redhat.com>
>  Victor Raj <victor.raj@intel.com>
>  Vidya Sagar Velumuri <vvelumuri@marvell.com>
> +Vignesh PS <vignesh.purushotham.srinivas@ericsson.com> <vig.vigneshps199=
5@gmail.com>
>  Vignesh Sridhar <vignesh.sridhar@intel.com>
>  Vijayakumar Muthuvel Manickam <mmvijay@gmail.com>
>  Vijaya Mohan Guvva <vijay1054@gmail.com>
> diff --git a/lib/ip_frag/ip_frag_common.h b/lib/ip_frag/ip_frag_common.h
> index 51fc9d47fb..db2665e846 100644
> --- a/lib/ip_frag/ip_frag_common.h
> +++ b/lib/ip_frag/ip_frag_common.h
> @@ -169,6 +169,8 @@ ip_frag_reset(struct ip_frag_pkt *fp, uint64_t tms)
>  	fp->total_size =3D UINT32_MAX;
>  	fp->frag_size =3D 0;
>  	fp->last_idx =3D IP_MIN_FRAG_NUM;
> +	fp->exts_len =3D 0;
> +	fp->next_proto =3D NULL;
>  	fp->frags[IP_LAST_FRAG_IDX] =3D zero_frag;
>  	fp->frags[IP_FIRST_FRAG_IDX] =3D zero_frag;
>  }
> diff --git a/lib/ip_frag/ip_reassembly.h b/lib/ip_frag/ip_reassembly.h
> index 54afed5417..429e74f1b3 100644
> --- a/lib/ip_frag/ip_reassembly.h
> +++ b/lib/ip_frag/ip_reassembly.h
> @@ -54,6 +54,8 @@ struct __rte_cache_aligned ip_frag_pkt {
>  	uint32_t total_size;                   /* expected reassembled size */
>  	uint32_t frag_size;                    /* size of fragments received */
>  	uint32_t last_idx;                     /* index of next entry to fill *=
/
> +	uint32_t exts_len;                     /* length of extension hdrs for =
first fragment */
> +	uint8_t *next_proto;                   /* pointer of the next_proto fie=
ld */
>  	struct ip_frag frags[IP_MAX_FRAG_NUM]; /* fragments */
>  };
>=20
> diff --git a/lib/ip_frag/rte_ipv6_reassembly.c b/lib/ip_frag/rte_ipv6_rea=
ssembly.c
> index 88863a98d1..8decf592a6 100644
> --- a/lib/ip_frag/rte_ipv6_reassembly.c
> +++ b/lib/ip_frag/rte_ipv6_reassembly.c
> @@ -91,19 +91,19 @@ ipv6_frag_reassemble(struct ip_frag_pkt *fp)
>  	/* update ipv6 header for the reassembled datagram */
>  	ip_hdr =3D rte_pktmbuf_mtod_offset(m, struct rte_ipv6_hdr *, m->l2_len)=
;
>=20
> +	payload_len +=3D fp->exts_len;
>  	ip_hdr->payload_len =3D rte_cpu_to_be_16(payload_len);
>=20
>  	/*
>  	 * remove fragmentation header. note that per RFC2460, we need to updat=
e
>  	 * the last non-fragmentable header with the "next header" field to con=
tain
> -	 * type of the first fragmentable header, but we currently don't suppor=
t
> -	 * other headers, so we assume there are no other headers and thus upda=
te
> -	 * the main IPv6 header instead.
> +	 * type of the first fragmentable header.
>  	 */
> -	move_len =3D m->l2_len + m->l3_len - sizeof(*frag_hdr);
> -	frag_hdr =3D (struct rte_ipv6_fragment_ext *) (ip_hdr + 1);
> -	ip_hdr->proto =3D frag_hdr->next_header;
> +	frag_hdr =3D (struct rte_ipv6_fragment_ext *)
> +		((uint8_t *) (ip_hdr + 1) + fp->exts_len);
> +	*fp->next_proto =3D frag_hdr->next_header;
>=20
> +	move_len =3D m->l2_len + m->l3_len - sizeof(*frag_hdr);
>  	ip_frag_memmove(rte_pktmbuf_mtod_offset(m, char *, sizeof(*frag_hdr)),
>  			rte_pktmbuf_mtod(m, char*), move_len);
>=20
> @@ -112,6 +112,39 @@ ipv6_frag_reassemble(struct ip_frag_pkt *fp)
>  	return m;
>  }
>=20
> +/*
> + * Function to crawl through the extension header stack.
> + * This function breaks as soon a the fragment header is
> + * found and returns the total length the traversed exts
> + * and the last extension before the fragment header
> + */
> +static inline uint32_t
> +ip_frag_get_last_exthdr(struct rte_ipv6_hdr *ip_hdr, uint8_t **last_ext)
> +{
> +	uint32_t total_len =3D 0;
> +	uint8_t num_exts =3D 0;
> +	size_t ext_len =3D 0;
> +	*last_ext =3D (uint8_t *)(ip_hdr + 1);
> +	int next_proto =3D ip_hdr->proto;
> +#define MAX_NUM_IPV6_EXTS 8

As a nit - let's keep coding style consistent:
Pls move #define outside the function definition.=20

> +
> +	while (next_proto !=3D IPPROTO_FRAGMENT &&
> +		num_exts < MAX_NUM_IPV6_EXTS &&
> +		(next_proto =3D rte_ipv6_get_next_ext(
> +		*last_ext, next_proto, &ext_len)) >=3D 0) {
> +
> +		total_len +=3D ext_len;
> +
> +		if (next_proto =3D=3D IPPROTO_FRAGMENT)
> +			return total_len;
> +
> +		*last_ext +=3D ext_len;
> +		num_exts++;
> +	}

So if  IPPROTO_FRAGMENT was not found, we just use extension #8 instead?
Shouldn't we return an error in that case,  and probably drop the fragment?

> +	return total_len;
> +}
> +
>  /*
>   * Process new mbuf with fragment of IPV6 datagram.
>   * Incoming mbuf should have its l2_len/l3_len fields setup correctly.
> @@ -139,6 +172,8 @@ rte_ipv6_frag_reassemble_packet(struct rte_ip_frag_tb=
l *tbl,
>  {
>  	struct ip_frag_pkt *fp;
>  	struct ip_frag_key key;
> +	uint8_t *last_ipv6_ext;
> +	uint32_t exts_len;
>  	uint16_t ip_ofs;
>  	int32_t ip_len;
>  	int32_t trim;
> @@ -154,10 +189,10 @@ rte_ipv6_frag_reassemble_packet(struct rte_ip_frag_=
tbl *tbl,
>  	/*
>  	 * as per RFC2460, payload length contains all extension headers
>  	 * as well.
> -	 * since we don't support anything but frag headers,
> -	 * this is what we remove from the payload len.
> +	 * so we remove the extension len from the payload len.
>  	 */
> -	ip_len =3D rte_be_to_cpu_16(ip_hdr->payload_len) - sizeof(*frag_hdr);
> +	exts_len =3D ip_frag_get_last_exthdr(ip_hdr, &last_ipv6_ext);
> +	ip_len =3D rte_be_to_cpu_16(ip_hdr->payload_len) - exts_len - sizeof(*f=
rag_hdr);

Hmm..., as I remember ip_len is what we want to preserve in the packet...
Why we want to remove all previous ext headers here?

>  	trim =3D mb->pkt_len - (ip_len + mb->l3_len + mb->l2_len);
>=20
>  	IP_FRAG_LOG(DEBUG, "%s:%d:\n"
> @@ -201,6 +236,21 @@ rte_ipv6_frag_reassemble_packet(struct rte_ip_frag_t=
bl *tbl,
>  	/* process the fragmented packet. */
>  	mb =3D ip_frag_process(fp, dr, mb, ip_ofs, ip_len,
>  			MORE_FRAGS(frag_hdr->frag_data));

Can you explain why we setting these new fp fields after 'ip_frag_process()=
'?
Ip_frag_process() itself can call reassembly() - if all fragments are alrea=
dy in place.

> +
> +	/* store extension stack info, only for first fragment */
> +	if (ip_ofs =3D=3D 0) {

If we want it for first fragment only, why not invoke ip_frag_get_last_exth=
dr()
only when ip_ofs =3D=3D 0?
=20
> +		/*
> +		 * fp->next_proto points to either the IP's next header
> +		 * or th next header of the extension before the fragment
> +		 * extension
> +		 */
> +		fp->next_proto =3D (uint8_t *)&ip_hdr->proto;
> +		if (exts_len > 0) {
> +			fp->exts_len =3D exts_len;
> +			fp->next_proto =3D last_ipv6_ext;
> +		}
> +	}
> +
>  	ip_frag_inuse(tbl, fp);
>=20
>  	IP_FRAG_LOG(DEBUG, "%s:%d:\n"
> --
> 2.34.1