On 8/16/21 6:21 AM, Tony van der Peet wrote: > When a PACKET_OUT has output port of OFPP_TABLE, and the rule > table includes a meter and this causes the packet to be deleted, > execute with a clone of the packet, restoring the original packet > if it is changed by the execution. > > Add tests to verify the original issue is fixed, and that the fix > doesn't break tunnel processing. > > Reported-by: Tony van der Peet <[email protected]> > Signed-off-by: Ilya Maximets <[email protected]> > Signed-off-by: Tony van der Peet <[email protected]> > --- > lib/dp-packet.h | 13 ++++++++++ > lib/dpif-netdev.c | 19 +++++++++++++- > tests/ofproto-dpif.at | 21 +++++++++++++++ > tests/tunnel-push-pop.at | 56 ++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 108 insertions(+), 1 deletion(-) > > diff --git a/lib/dp-packet.h b/lib/dp-packet.h > index 08d93c277..3dc582fbf 100644 > --- a/lib/dp-packet.h > +++ b/lib/dp-packet.h > @@ -199,6 +199,7 @@ struct dp_packet > *dp_packet_clone_data_with_headroom(const void *, size_t, > void dp_packet_resize(struct dp_packet *b, size_t new_headroom, > size_t new_tailroom); > static inline void dp_packet_delete(struct dp_packet *); > +static inline void dp_packet_swap(struct dp_packet *, struct dp_packet *); > > static inline void *dp_packet_at(const struct dp_packet *, size_t offset, > size_t size); > @@ -256,6 +257,18 @@ dp_packet_delete(struct dp_packet *b) > } > } > > +/* Swaps content of two packets. */ > +static inline void > +dp_packet_swap(struct dp_packet *a, struct dp_packet *b) > +{ > + ovs_assert(a->source == DPBUF_MALLOC || a->source == DPBUF_STUB); > + ovs_assert(b->source == DPBUF_MALLOC || b->source == DPBUF_STUB); > + struct dp_packet c = *a; > + > + *a = *b; > + *b = c; > +} > + > /* If 'b' contains at least 'offset + size' bytes of data, returns a pointer > to > * byte 'offset'. Otherwise, returns a null pointer. */ > static inline void * > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 03f460c7d..4733c3a06 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -4077,7 +4077,10 @@ dpif_netdev_execute(struct dpif *dpif, struct > dpif_execute *execute) > flow_hash_5tuple(execute->flow, 0)); > } > > - dp_packet_batch_init_packet(&pp, execute->packet); > + /* Making a copy because the packet might be stolen during the execution > + * and caller might still need it. */ > + struct dp_packet *packet_clone = dp_packet_clone(execute->packet); > + dp_packet_batch_init_packet(&pp, packet_clone); > dp_netdev_execute_actions(pmd, &pp, false, execute->flow, > execute->actions, execute->actions_len); > dp_netdev_pmd_flush_output_packets(pmd, true); > @@ -4087,6 +4090,20 @@ dpif_netdev_execute(struct dpif *dpif, struct > dpif_execute *execute) > dp_netdev_pmd_unref(pmd); > } > > + if (dp_packet_batch_size(&pp)) { > + /* Packet wasn't dropped during the execution. Swapping content with > + * the original packet, because the caller might expect actions to > + * modify it. */ > + dp_packet_swap(execute->packet, packet_clone); > + dp_packet_delete_batch(&pp, true); > + } else { > + /* Packet was stolen during the execution due to some error. We need > + * to flag that for the caller, so it will not proceed with other > + * actions on this packet. Returning EAGAIN because we just don't > + * know what execlty happened. */ > + return EAGAIN; > + }
Thanks, Tony, for a patch! On a millionth thought about the issue I dropped this 'else' condition, because it doesn't really stop higher layers from execution of subsequent actions, but it's kind of a behavioral change. And it also leads to warnings in a normal case where conntrack steals the packet. With that, applied to master and backported all the way down to 2.12. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
