On Thu, May 16, 2024 at 8:35 AM Simon Horman <ho...@ovn.org> wrote:
>
> Hi Mike,
>
> On Wed, May 15, 2024 at 12:24:33PM -0400, Mike Pattrick wrote:
> > 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, this is most likely the batch that added it in the
> > first place, but that isn't a guarantee.
> >
> > The packets in these batches should be segregated by network protocol
> > versuib (ipv4 vs ipv6) for conntrack defragmentation to function
>
> nit: version
>
> > appropriately. However, there are conditions where we would add a
> > reassembled packet of one type to a batch of another.
> >
> > This change introduces checks to make sure that reassembled or expired
> > fragments are only added to packet batches of the same type. It also
> > makes a best effort attempt to make sure the defragmented packet is
> > inserted into the current batch.
>
> Would it make any sense to separate these changes into separate patches?
Can do!
>
> >
> > Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> > Reported-at: https://issues.redhat.com/browse/FDP-560
> > Signed-off-by: Mike Pattrick <m...@redhat.com>
> > ---
> > Note: This solution is far from perfect, ipf.c can still insert packets
> > into more or less arbitrary batches but this bug fix is needed to avoid a
> > memory overrun and should insert packets into the proper batch in the
> > common case. I'm working on a more correct solution but it changes how
> > fragments are fundimentally handled, and couldn't be considered a bug fix.
>
> FWIIW, I'm ok with changes that move things to a better, even if not ideal,
> state.
>
> ...
>
> > @@ -943,9 +952,14 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct
> > dp_packet_batch *pb,
> > ipf_is_valid_v6_frag(ipf, pkt)))) {
> >
> > 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)) {
>
> The conditions under which rp are set are complex and buried
> inside the call-chain under ipf_handle_frag(). I am concerned
> that there are cases where it may be used unset here. Or that
> the complexity allows for such cases to be inadvertently added
> later.
>
> Could we make this more robust, f.e. by making sure rp is
> always initialised when ipf_handle_frag returns by setting
> it to NULL towards the top of that function.
Agreed that it's overly complex. I'll change this to initialize this
in ipf_extract_frags_from_batch(), the functions in between
ipf_list_state_transition and ipf_extract_frags_from_batch shouldn't
really touch or care about this value.
-M
>
> > + 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);
>
> ...
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
