Re: [PATCH] ip_frag: support IPv6 reassembly with extensions

[Vignesh Purushotham Srinivas <vignesh.purushotham.srinivas@ericsson.com>]

-----Original Message-----
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
Date: Tue, 17 Sep 2024 18:07:25 +0000

> > +/*
> > + * 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 = 0;
> > +     uint8_t num_exts = 0;
> > +     size_t ext_len = 0;
> > +     *last_ext = (uint8_t *)(ip_hdr + 1);
> > +     int next_proto = 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.

ACK

> > +
> > +     while (next_proto != IPPROTO_FRAGMENT &&
> > +             num_exts < MAX_NUM_IPV6_EXTS &&
> > +             (next_proto = rte_ipv6_get_next_ext(
> > +             *last_ext, next_proto, &ext_len)) >= 0) {
> > +
> > +             total_len += ext_len;
> > +
> > +             if (next_proto == IPPROTO_FRAGMENT)
> > +                     return total_len;
> > +
> > +             *last_ext += 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?

Hmmm, looks like a bug. Will fix this in the next version

> > +     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_tbl *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 = rte_be_to_cpu_16(ip_hdr->payload_len) -
sizeof(*frag_hdr);
> > +     exts_len = ip_frag_get_last_exthdr(ip_hdr, &last_ipv6_ext);
> > +     ip_len = rte_be_to_cpu_16(ip_hdr->payload_len) - exts_len -
sizeof(*frag_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?

The ip_len for packet is computed again later, after reassembly.
However, here we
compute the ip_len to perform some checks on the packet. And this math
follows
what is mentioned in the RFC.

> >       trim = mb->pkt_len - (ip_len + mb->l3_len + mb->l2_len);
> >
> >       IP_FRAG_LOG(DEBUG, "%s:%d:\n"
> > @@ -201,6 +236,21 @@ rte_ipv6_frag_reassemble_packet(struct
rte_ip_frag_tbl *tbl,
> >       /* process the fragmented packet. */
> >       mb = 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
already in place.

This is a bug, but will fix it in the next version

> > +
> > +     /* store extension stack info, only for first fragment */
> > +     if (ip_ofs == 0) {
> 
> If we want it for first fragment only, why not invoke
ip_frag_get_last_exthdr()
> only when ip_ofs == 0?

No, ip_frag_get_last_exthdr() is called for all fragments to find the
len of
of extensions (if present) to perform length checks but we store this
information
only for the first fragment. After reassembly, this stored information
is used to
restore the extensions from the first fragment. 

> > +             /*
> > +              * 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 = (uint8_t *)&ip_hdr->proto;
> > +             if (exts_len > 0) {
> > +                     fp->exts_len = exts_len;
> > +                     fp->next_proto = last_ipv6_ext;
> > +             }
> > +     }
> > +
> >       ip_frag_inuse(tbl, fp);
> >
> >       IP_FRAG_LOG(DEBUG, "%s:%d:\n"
> > --
> > 2.34.1