Yes, all frag will be taken out of the batch into ipf. These packet will
eventually have two situations: either they are reassembled successfully and
then refilled the batch, or they are deleted in ipf.
We test it on OVS with DPDK, is okay.
This patch has been running in our environment since July and has been working
fine.
At 2024-12-16 22:46:37, "Aaron Conole" <[email protected]> wrote:
>Aaron Conole <[email protected]> writes:
>
>> fangjiannan <[email protected]> writes:
>>
>>> From: fang <[email protected]>
>>>
>>> Canceling packet copying can better improve the performance of
>>> capturing fragmented packets. In 640d4db, pkt copying was added to fix
>>> the crash, but this bug has been fixed in 7e6b41a, so there is no need
>>> to copy the pkt again.
>>>
>>> Signed-off-by: fangjiannan <[email protected]>
>>> ---
>>
>> I'm not sure about this. Currently, there are multiple ways for a
>> packet to be deleted during processing, and not all of them will
>> sit inline with the ipf handling, nor will IPF be aware of them. IPF
>> was changed in 640d4db to ensure that it will always be the only owner
>> of the packet buffer in question.
>>
>> It's been some time since I looked at the out-of-band release points
>> that IPF could still reference a packet on. Actually, I think they
>> should mostly be eliminated so maybe I'm just carrying FUD from the
>> older IPF design. Did you test this with DPDK ports? And with long
>> running CT connections? Or with frags that would unexpectedly expire?
>> Can you please give some testing details.
>
>I'm going to contradict myself here. I think it's okay. We are assumed
>to take ownership in all cases. If we didn't, there would be a dangling
>uaf as in the case of an unhandled frag we keep it on the batch, and the
>case of reassembled, we refill the batch, and otherwise we will delete.
>In all those cases, the ownership is quite clear. So I think it should
>be fine.
>
>Acked-by: Aaron Conole <[email protected]>
>
>>> lib/ipf.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/lib/ipf.c b/lib/ipf.c
>>> index 2d715f5e9..59e232355 100644
>>> --- a/lib/ipf.c
>>> +++ b/lib/ipf.c
>>> @@ -820,7 +820,7 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list
>>> *ipf_list,
>>> * recommend not setting the mempool number of buffers too low
>>> * and also clamp the number of fragments. */
>>> struct ipf_frag *frag = &ipf_list->frag_list[last_inuse_idx +
>>> 1];
>>> - frag->pkt = dp_packet_clone(pkt);
>>> + frag->pkt = pkt;
>>> frag->start_data_byte = start_data_byte;
>>> frag->end_data_byte = end_data_byte;
>>> ipf_list->last_inuse_idx++;
>>> @@ -959,7 +959,6 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct
>>> dp_packet_batch *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);
>>> } else {
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
