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

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > > Konstantin
> > > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > >
> > > > > On Tue,  8 Jan 2019 14:08:45 +0800
> > > > > Jiayu Hu <jiayu.hu@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.

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.


Med venlig hilsen / kind regards
- Morten Brørup