DPDK patches and discussions
 help / color / mirror / Atom feed
* Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data
@ 2021-12-02  9:35 Dariusz Sosnowski
  2021-12-02 11:38 ` Huichao Cai
  0 siblings, 1 reply; 6+ messages in thread
From: Dariusz Sosnowski @ 2021-12-02  9:35 UTC (permalink / raw)
  To: Huichao Cai; +Cc: konstantin.ananyev, dev

Hi,

On Thu, 2 Dec 2021 10:24:40 +0800, Huichao Cai wrote:
> > Substituting options with NOOP might cause rte_ipv4_fragment_packet to produce more fragments than necessary, since options with copied flag unset will still occupy space in IPv4 header.
> --The "ip_options_fragment" just make a replacement and doesn't change the length of the IPv4 header.So I don't quite understand why it leads to produce more fragments.
If options with copied flag unset are not copied, then IPv4 headers in the fragments (despite 1st fragment) will be shorter. This leaves more byte space for the payload and in effect fragmentation might produce less fragments.

Best regards,
Dariusz Sosnowski



 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data
@ 2022-02-10 12:21 Ananyev, Konstantin
  2022-02-11  2:12 ` Huichao Cai
  0 siblings, 1 reply; 6+ messages in thread
From: Ananyev, Konstantin @ 2022-02-10 12:21 UTC (permalink / raw)
  To: chcchc88; +Cc: dev



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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-11 10:00 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-02  9:35 [PATCH] ip_frag: add IPv4 options fragment and unit test data Dariusz Sosnowski
2021-12-02 11:38 ` Huichao Cai
2021-12-02 12:03   ` Ananyev, Konstantin
2021-12-02 12:11     ` Huichao Cai
2022-02-09  5:50       ` Huichao Cai
2022-02-10 12:21 Ananyev, Konstantin
2022-02-11  2:12 ` Huichao Cai
2022-02-11  9:41   ` Ananyev, Konstantin
2022-02-11 10:00     ` Huichao Cai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).