> -----Original Message----- > From: Morten Brørup [mailto:m...@smartsharesystems.com] > Sent: Tuesday, January 8, 2019 10:50 PM > To: Ananyev, Konstantin <konstantin.anan...@intel.com>; Hu, Jiayu > <jiayu...@intel.com>; Stephen Hemminger > <step...@networkplumber.org> > Cc: dev@dpdk.org; Bie, Tiwei <tiwei....@intel.com>; Richardson, Bruce > <bruce.richard...@intel.com>; sta...@dpdk.org > Subject: RE: [dpdk-dev] [PATCH] gro: add missing invalid packet checks > > > From: Ananyev, Konstantin [mailto:konstantin.anan...@intel.com] > > > From: Morten Brørup [mailto:m...@smartsharesystems.com] > > > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Ananyev, > > > > Konstantin > > > > > > From: Stephen Hemminger [mailto:step...@networkplumber.org] > > > > > > > > > > > > On Tue, 8 Jan 2019 14:08:45 +0800 > > > > > > Jiayu Hu <jiayu...@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
