Mike Pattrick <[email protected]> writes: > When conntrack is reassembling packet fragments, the same reassembly > context can be shared across multiple threads handling different packets > simultaneously. Once a full packet is assembled, it is added to a packet > batch for processing, in the case where there are multiple different pmd > threads accessing conntrack simultaneously, there is a race condition > where the reassembled packet may be added to an arbitrary batch even if > the current batch is available. > > When this happens, the packet may be handled incorrectly as it is > inserted into a random openflow execution pipeline, instead of the > pipeline for that packets flow. > > This change makes a best effort attempt to try to add the defragmented > packet to the current batch. directly. This should succeed most of the > time. > > Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.") > Reported-at: https://issues.redhat.com/browse/FDP-560 > Signed-off-by: Mike Pattrick <[email protected]> > ---
Acked-by: Paolo Valerio <[email protected]> > lib/ipf.c | 27 ++++++++++++++++++++------- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/lib/ipf.c b/lib/ipf.c > index 3c8960be3..2d715f5e9 100644 > --- a/lib/ipf.c > +++ b/lib/ipf.c > @@ -506,13 +506,15 @@ ipf_reassemble_v6_frags(struct ipf_list *ipf_list) > } > > /* Called when a frag list state transitions to another state. This is > - * triggered by new fragment for the list being received.*/ > -static void > +* triggered by new fragment for the list being received. Returns a > reassembled > +* packet if this fragment has completed one. */ > +static struct reassembled_pkt * > ipf_list_state_transition(struct ipf *ipf, struct ipf_list *ipf_list, > bool ff, bool lf, bool v6) > OVS_REQUIRES(ipf->ipf_lock) > { > enum ipf_list_state curr_state = ipf_list->state; > + struct reassembled_pkt *ret = NULL; > enum ipf_list_state next_state; > switch (curr_state) { > case IPF_LIST_STATE_UNUSED: > @@ -562,12 +564,15 @@ ipf_list_state_transition(struct ipf *ipf, struct > ipf_list *ipf_list, > ipf_reassembled_list_add(&ipf->reassembled_pkt_list, rp); > ipf_expiry_list_remove(ipf_list); > next_state = IPF_LIST_STATE_COMPLETED; > + ret = rp; > } else { > next_state = IPF_LIST_STATE_REASS_FAIL; > } > } > } > ipf_list->state = next_state; > + > + return ret; > } > > /* Some sanity checks are redundant, but prudent, in case code paths for > @@ -799,7 +804,8 @@ ipf_is_frag_duped(const struct ipf_frag *frag_list, int > last_inuse_idx, > static bool > ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list, > struct dp_packet *pkt, uint16_t start_data_byte, > - uint16_t end_data_byte, bool ff, bool lf, bool v6) > + uint16_t end_data_byte, bool ff, bool lf, bool v6, > + struct reassembled_pkt **rp) > OVS_REQUIRES(ipf->ipf_lock) > { > bool duped_frag = ipf_is_frag_duped(ipf_list->frag_list, > @@ -820,7 +826,7 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list > *ipf_list, > ipf_list->last_inuse_idx++; > atomic_count_inc(&ipf->nfrag); > ipf_count(ipf, v6, IPF_NFRAGS_ACCEPTED); > - ipf_list_state_transition(ipf, ipf_list, ff, lf, v6); > + *rp = ipf_list_state_transition(ipf, ipf_list, ff, lf, v6); > } else { > OVS_NOT_REACHED(); > } > @@ -853,7 +859,8 @@ ipf_list_init(struct ipf_list *ipf_list, struct > ipf_list_key *key, > * to a list of fragemnts. */ > static bool > ipf_handle_frag(struct ipf *ipf, struct dp_packet *pkt, ovs_be16 dl_type, > - uint16_t zone, long long now, uint32_t hash_basis) > + uint16_t zone, long long now, uint32_t hash_basis, > + struct reassembled_pkt **rp) > OVS_REQUIRES(ipf->ipf_lock) > { > struct ipf_list_key key; > @@ -922,7 +929,7 @@ ipf_handle_frag(struct ipf *ipf, struct dp_packet *pkt, > ovs_be16 dl_type, > } > > return ipf_process_frag(ipf, ipf_list, pkt, start_data_byte, > - end_data_byte, ff, lf, v6); > + end_data_byte, ff, lf, v6, rp); > } > > /* Filters out fragments from a batch of fragments and adjust the batch. */ > @@ -941,11 +948,17 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct > dp_packet_batch *pb, > || > (dl_type == htons(ETH_TYPE_IPV6) && > ipf_is_valid_v6_frag(ipf, pkt)))) { > + struct reassembled_pkt *rp = NULL; > > ovs_mutex_lock(&ipf->ipf_lock); > - if (!ipf_handle_frag(ipf, pkt, dl_type, zone, now, hash_basis)) { > + if (!ipf_handle_frag(ipf, pkt, dl_type, zone, now, hash_basis, > + &rp)) { > dp_packet_batch_refill(pb, pkt, pb_idx); > } else { > + if (rp && !dp_packet_batch_is_full(pb)) { > + dp_packet_batch_refill(pb, rp->pkt, pb_idx); > + rp->list->reass_execute_ctx = rp->pkt; > + } > dp_packet_delete(pkt); > } > ovs_mutex_unlock(&ipf->ipf_lock); > -- > 2.39.3 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
