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?
>
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Reported-at: https://issues.redhat.com/browse/FDP-560
> Signed-off-by: Mike Pattrick <[email protected]>
> ---
> 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.
> + 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
