Not a full review. Comments inline. Best regards, Ilya Maximets.
On 09.01.2019 21:05, Tiago Lam wrote: > Previous commits have added support to the dp_packet API to handle > multi-segmented packets, where data is not stored contiguously in > memory. However, in some cases, it is inevitable and data must be > provided contiguously. Examples of such cases are when performing csums > over the entire packet data, or when write()'ing to a file descriptor > (for a tap interface, for example). For such cases, the dp_packet API > has been extended to provide a way to transform a multi-segmented > DPBUF_DPDK packet into a DPBUF_MALLOC system packet (at the expense of a > copy of memory). If the packet's data is already stored in memory > contigously then there's no need to convert the packet. > > Thus, the main use cases that were assuming that a dp_packet's data is > always held contiguously in memory were changed to make use of the new > "linear functions" in the dp_packet API when there's a need to traverse > the entire's packet data. Per the example above, when the packet's data > needs to be write() to the tap's file descriptor, or when the conntrack > module needs to verify a packet's checksum, the data is now linearized. > > Additionally, the miniflow_extract() function has been modified to check > if the respective packet headers don't span across multiple mbufs. This > requirement is needed to guarantee that callers can assume headers are > always in contiguous memory. > > Signed-off-by: Tiago Lam <[email protected]> > --- > lib/bfd.c | 3 +- > lib/conntrack-private.h | 4 +- > lib/conntrack.c | 43 +++++-- > lib/crc32c.c | 35 ++++-- > lib/crc32c.h | 2 + > lib/dp-packet.c | 18 +++ > lib/dp-packet.h | 267 > ++++++++++++++++++++++++++++++------------ > lib/dpif-netdev.c | 13 +- > lib/dpif-netlink.c | 5 + > lib/dpif.c | 9 ++ > lib/flow.c | 97 ++++++++++++--- > lib/flow.h | 2 +- > lib/mcast-snooping.c | 2 + > lib/netdev-bsd.c | 5 + > lib/netdev-dummy.c | 13 +- > lib/netdev-linux.c | 13 +- > lib/netdev-native-tnl.c | 26 ++-- > lib/odp-execute.c | 12 +- > lib/ovs-lldp.c | 3 +- > lib/packets.c | 85 ++++++++++++-- > lib/packets.h | 7 ++ > ofproto/ofproto-dpif-upcall.c | 20 +++- > ofproto/ofproto-dpif-xlate.c | 32 ++++- > tests/test-rstp.c | 8 +- > tests/test-stp.c | 8 +- > 25 files changed, 579 insertions(+), 153 deletions(-) > [snip] > +/* Linearizes the data on packet 'b', by copying the data into system's > memory. > + * After this the packet is effectively a DPBUF_MALLOC packet. The caller is > + * responsible * for ensuring 'b' needs linearization, by calling > + * dp_packet_is_linear(). > + * > + * This is an expensive operation which should only be performed as a last > + * resort, when multi-segments are under use but data must be accessed > + * linearly. */ > +static inline void > +dp_packet_linearize(struct dp_packet *b) > +{ > + struct rte_mbuf *mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf); > + struct dp_packet *pkt = CONST_CAST(struct dp_packet *, b); > + uint32_t pkt_len = dp_packet_size(pkt); > + struct mbuf_state *mstate = NULL; > + void *dst = xmalloc(pkt_len); > + > + /* Copy packet's data to system's memory */ > + if (!rte_pktmbuf_read(mbuf, 0, pkt_len, dst)) { 'dst' leak. > + return; > + } > + > + /* Free all mbufs except for the first */ > + dp_packet_clear(pkt); > + > + /* Save mbuf's buf_addr to restore later */ > + mstate = xmalloc(sizeof(*mstate)); > + mstate->addr = pkt->mbuf.buf_addr; > + mstate->len = pkt->mbuf.buf_len; > + mstate->off = pkt->mbuf.data_off; > + pkt->mstate = mstate; > + > + /* Tranform DPBUF_DPDK packet into a DPBUF_MALLOC packet */ > + pkt->source = DPBUF_MALLOC; > + pkt->mbuf.buf_addr = dst; > + pkt->mbuf.buf_len = pkt_len; > + pkt->mbuf.data_off = 0; > + dp_packet_set_size(pkt, pkt_len); > +} > #else /* DPDK_NETDEV */ > static inline bool > dp_packet_equal(const struct dp_packet *a, const struct dp_packet *b) > @@ -945,6 +1050,24 @@ dp_packet_has_flow_mark(struct dp_packet *p OVS_UNUSED, > { > return false; > } > + [snip] > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 1564db9..1f1188d 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -5710,6 +5710,11 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, > struct dp_packet *packet_, > .support = dp_netdev_support, > }; > > + /* Gather the whole data for printing the packet (if debug enabled) > */ > + if (!dp_packet_is_linear(packet_)) { > + dp_packet_linearize(packet_); > + } > + > ofpbuf_init(&key, 0); > odp_flow_key_from_flow(&odp_parms, &key); > packet_str = ofp_dp_packet_to_string(packet_); > @@ -5954,6 +5959,7 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd, > bool smc_enable_db; > size_t map_cnt = 0; > bool batch_enable = true; > + int error; > > atomic_read_relaxed(&pmd->dp->smc_enable_db, &smc_enable_db); > atomic_read_relaxed(&pmd->dp->emc_insert_min, &cur_min); > @@ -6001,7 +6007,12 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd, > } > } > > - miniflow_extract(packet, &key->mf); > + error = miniflow_extract(packet, &key->mf); > + if (OVS_UNLIKELY(error)) { > + dp_packet_delete(packet); > + continue; > + } The error checking must be done in all the places, where 'miniflow_extract' called. The 'flow_extract' is the main candidate. Also, we're not parsing the packets in case of partial HW offloading leaving the loop before the 'miniflow_extract' call. This will lead to cases where the bad packet will go for actions execution leading to crash/data corruption. For example in case of HW offloading and the hash() action, packet will be parsed by 'flow_extract' inside OVS_ACTION_ATTR_HASH handler in lib/odp-execute.c module. I think, it's required to check the result in all the places and also change the 'flow_extract' to return the error and check in all the callers. > + > key->len = 0; /* Not computed yet. */ > /* If EMC and SMC disabled skip hash computation */ > if (smc_enable_db == true || cur_min != 0) { [snip] _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
