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

[Hu, Jiayu]

> -----Original Message-----
> From: Morten Brørup [mailto:mb at smartsharesystems.com]
> Sent: Tuesday, January 8, 2019 10:50 PM
> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; Hu, Jiayu
> <jiayu.hu at intel.com>; Stephen Hemminger
> <stephen at networkplumber.org>
> Cc: dev at dpdk.org; Bie, Tiwei <tiwei.bie at intel.com>; Richardson, Bruce
> <bruce.richardson at intel.com>; stable at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
> 
> > From: Ananyev, Konstantin [mailto:konstantin.ananyev at intel.com]
> > > From: Morten Brørup [mailto:mb at smartsharesystems.com]
> > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ananyev,
> > > > Konstantin
> > > > > > From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> > > > > >
> > > > > > On Tue,  8 Jan 2019 14:08:45 +0800
> > > > > > Jiayu Hu <jiayu.hu at intel.com> wrote:
> > > > > >
> > > > > > > +	/*
> > > > > > > +	 * Don't process the packet whose Ethernet, IPv4 and TCP
> > > > header
> > > > > > > +	 * lengths are invalid. In addition, if the IPv4 header
> > > > contains
> > > > > > > +	 * Options, the packet shouldn't be processed.
> > > > > > > +	 */
> > > > > > > +	if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->l2_len) ||
> > > > > > > +			ILLEGAL_IPV4_HDRLEN(pkt->l3_len) ||
> > > > > > > +			ILLEGAL_TCP_HDRLEN(pkt->l4_len)))
> > > > > > > +		return -1;
> > > > >
> > > > > In the GRO design, we assume applications give correct
> > > > > MBUF->l2_len/.. for input packets of GRO. Specifically, GRO
> > > > > library assumes applications will set values to MBUF->l2_len/...
> > > > > and guarantee the values are the same as the values in the packet
> > > > > headers. The reason for this assumption is to process header
> > faster.
> > >
> > > > > This is also why I want to add this assumption in the programmer
> > > > > guide.
> > >
> > > +1 to more detailed documentation about assumptions and
> > preconditions.
> > >
> > >
> > > > >
> > > > > The above code is to forbid GRO to process invalid packets, which
> > > > > have invalid packet header lengths, like TCP header length is
> > less
> > > > than
> > > > > 20 bytes.
> > > > >
> > > > > >
> > > > > > I like it when code is as picky as possible when doing
> > > > optimizations because
> > > > > > it reduces possible security riskg.
> > > > > >
> > > > > > To me this looks more confusing and not as careful as doing it
> > > > like:
> > > > > >
> > > > > > 	if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> > > > > > 		return -1;
> > > > > > 	eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> > > > > > 	ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr +
> > ETHER_HDR_LEN);
> > > > > >
> > > > > > 	if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK)
> > << 4)
> > > > > > 		return -1;
> > > > > >
> > > > > > 	if (pkt->l4_len < sizeof(struct tcp_hdr))
> > > > > > 		return -1;
> > > > > >
> > > > > > You should also check for TCP options as well.
> > > > >
> > > > > There are two ways to get ether, ipv4 and tcp headers:
> > > > > 1). Use MBUF->l2_len/l3_len...;
> > > > > 2). Parse packet and ignore MBUF->l2_len/....
> > > > >
> > > > > If we follow the choice 1, we don't need to parse packet and
> > > > > don't need to check if values of MBUF->l2_len/... are correct,
> > > > > since we assume applications will set correct values. If we
> > follow
> > > > > the choice 2, we don't need to care about the values of MBUF-
> > > > >l2_len/...
> > > > >
> > > > > I am a little confused about your code, since it parses packet
> > and
> > > > > checks if the values of MBUF->l2_len/... are correct. If we don't
> > use
> > > > > MBUF->l2_len/... to get ether/ipv4/tcp headers, why should we
> > check
> > > > > the values of MBUF->l2_len/...?
> > > > >
> > > >
> > > > Agree that we don't need both.
> > > > My preference would be to stick with 1).
> > > > In many cases user would have already determined l2/l3/l4 len
> > > > by this stage.
> > > > Konstantin
> > >
> > > Do we have a generic packet header validation library? Otherwise,
> > that would perhaps be a better path. Such a library could probably use
> > > some of the flags from the PMD to determine how much to validate in
> > software.
> >
> > AFAIK - we don't have a generic header parsing library.
> > Yes, it would be good to have such ability, but I think that's out of
> > scope for that patch.
> > BTW, volunteers are welcome :)
> >
> > >
> > > And if it is a documented precondition of the GRO library that m-
> > >l2_len/l3_len... must be set and sensible, perhaps an RTE_ASSERT()
> > could
> > > be considered instead of gracefully returning -1?
> >
> > I suppose that's too extreme.
> > What's wrong with checking input parameters and return an error if they
> > are invalid?
> > Konstantin
> >
> It is extreme, and it was partly meant as a provocation to think deeper
> about it: Do we really need to follow Postel's law
> (https://en.wikipedia.org/wiki/Robustness_principle) in functions where we
> are in full control ourselves?
> 
> Here's what's wrong with it: If each function a packet passes through has to
> parse the packet headers and validate the mbuf parameters from scratch, it
> will have an unnecessary performance cost. It should be done only once in
> the fast path, and subsequent functions should be able to rely on the result
> of that.
> 
> Generally, we should be able to rely on assumptions/preconditions. And
> when a function relies on something, it is a good thing to describe such
> preconditions in the function's documentation.
> 
> From a high level perspective, DPDK Core should not be a bunch of
> completely independent libraries, but a consistent library where its
> functions can rely on preconditions and postconditions of its other
> functions and their intended use in the fast path.

Good point.

> 
> Regarding documentation, it might also be worth mentioning that the m-
> >l2_len/... fields are described as "fields to support TX offloads" in
> rte_mbuf.h. So when RX offload features - such as GRO - rely on these fields,
> perhaps the description in rte_mbuf.h should be updated accordingly.

Yes, both IP reassembly and GRO use those fields. We need to update the description.

Thanks,
Jiayu
> 
> 
> Med venlig hilsen / kind regards
> - Morten Brørup