王亮 Liang Wang <[email protected]> writes: > From d5c454bc9c68a6b2b40a2301a21f7337d6086d4c Mon Sep 17 00:00:00 2001 > From: Wang Liang <[email protected]> > Date: Mon, 12 Apr 2021 18:24:21 +0800 > Subject: [PATCH] Fix userspace datapath crash caused by IP fragments > > The ovs userspace datapath will crash on openflow packet-out message > with IP packet largger than MTU. This patch will fix the problem. > When pre-processing the IP fragments, clone the packet if it has > 'dnsteal' flag set. > > Signed-off-by: Wang Liang <[email protected]> > ---
I guess the real issue is that we don't honor the dnsteal flag correctly. It implies that we cannot safely hold a reference to the dp_packet because some other entity (read: the conntrack processing pipeline) will free it. Can you provide a test case that shows this? I've done a little bit of testing, but haven't been able to reproduce. > lib/ipf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/ipf.c b/lib/ipf.c > index c20bcc0b3..43f81706c 100644 > --- a/lib/ipf.c > +++ b/lib/ipf.c > @@ -811,7 +811,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 = pkt; > + frag->pkt = dnsteal ? dp_packet_clone(pkt) : pkt; I think the problem we have is once we do this duplication, I think we'll leak a reference to the packet, because we record the dnsteal value with frag, and that will skip the dp_packet_delete() calls later on during fragment queue release. I think with this change we also need to remove the dnsteal flag and simply always own the packet that comes in. Maybe I missed something? > frag->start_data_byte = start_data_byte; > frag->end_data_byte = end_data_byte; > frag->dnsteal = dnsteal; _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
