Re: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks

["Hu, Jiayu" <jiayu.hu@intel.com>]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, January 15, 2019 9:00 AM
> To: Hu, Jiayu <jiayu.hu@intel.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> thomas@monjalon.net; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks
> 
> On Thu, 10 Jan 2019 23:06:08 +0800
> Jiayu Hu <jiayu.hu@intel.com> wrote:
> 
> > +
> > +#define ILLEGAL_ETHER_HDRLEN(len) ((len) != ETHER_HDR_LEN)
> > +#define ILLEGAL_ETHER_VXLAN_HDRLEN(len) \
> > +	((len) != (ETHER_VXLAN_HLEN + ETHER_HDR_LEN))
> > +#define ILLEGAL_IPV4_HDRLEN(len) ((len) != sizeof(struct ipv4_hdr))
> > +#define ILLEGAL_TCP_HDRLEN(len) \
> > +	(((len) < sizeof(struct tcp_hdr)) || ((len) > TCP_MAX_HLEN))
> > +
> 
> Why not inline (which keeps type checking) instead of macro.
> Results in same code.

You mean the inline functions like the following two?
static inline int check_invalid_hdr _tcp4(mbuf) {
	if (unlikely(mbuf->l2_len != ETHER_HDR_LEN ||
			mbuf->l3_len != sizeof(struct ipv4_hdr) ||
			mbuf->l4_len < sizeof(struct tcp_hdr) || mbuf->l4_len > 60))
		return 1;
	return 0;
}
static inline int check_invalid_hdr_vxlan_tcp4(mbuf) {
	if (unlikely(mbuf->outer_l2_len != ETHER_HDR_LEN ||
			mbuf->outer_l3_len != sizeof(struct ipv4_hdr) ||
			mbuf->l2_len != (ETHER_VXLAN_HLEN + ETHER_HDR_LEN) ||
			mbuf->l3_len != sizeof(struct ipv4_hdr) ||
			mbuf->l4_len < sizeof(struct tcp_hdr) || mbuf->l4_len > 60))
		return 1;
	return 0;
}
Or you mean every header length check should be one inline function, like:
check_invalid_ipv4_hdr(), check_invalid_vxlan_hdr() etc. ?

What is the main benefit of inline function for length check here?

> 
> Also, prefer "invalid" instead "ILLEGAL" . There is no government inforcing
> a rule on packet headers.

Thanks. I will change the name.

> 
> Also, what about ipv4 options, or TCP options?
Usually, IPv4 header doesn't have Options. Therefore, the
implementations of TCP and VxLAN GRO don't consider IPv4
Option fields when merge packets. For TCP Options, GRO checks
Option fields when merges packets. So in the above code, the
valid TCP header length is 20~60, and valid IPv4 header length is 20.

> 
> And even VXLAN header check should be more rigorous.  What about not
> allowing
> fragments in IP header for example.

Hmm, this is an assumption that all input packets are not IP fragments, which
is stated in programmer guide. But maybe too many assumptions make it hard
for users to use GRO. I will add this check.

BTW, if the received packets from PMD are IP fragments, their MBUF->packet_type
will include L4 protocols?

> 
> If you are going to do enforcement, be as strict as you can.