On 11/01/2019 11:29, Ilya Maximets wrote: > Not a full review. > Comments inline.
Thanks Ilya, responses in-line. > > 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. ACK. Will fix. > >> + 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. Right, I'll address those use-cases for v14. However, note that performing those checks for some of the cases, like `nxt_resume()`, `check_masked_set_action()`, and the likes in ofproto/ofproto-dpif.c, is pointless at the moment since the packet will never be segmented as it is composed internally. I'd prefer to refrain to catch the errors in those cases. Tiago. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
