> On Thu, Dec 21, 2017 at 02:22:43PM +0000, Zoltán Balogh wrote:
> > Xlate_lookup and xlate_lookup_ofproto_() provides in_port and ofproto
> > based on xport determined using flow, which is extracted from packet.
> > The lookup can happen due to recirculation as well. It can happen, that
> > packet_type has been modified during xlate before recirculation is
> > triggered, so the lookup fails or delivers wrong xport.
> > This can be worked around by propagating xport to ctx->xin after the very
> > first lookup and store it in frozen state of the recirculation.
> > So, when lookup is performed due to recirculation, the xport can be
> > retrieved from the frozen state.
> >
> > Signed-off-by: Zoltan Balogh <[email protected]>
> > CC: Jan Scheurich <[email protected]>
>
> Thanks for working on finding and fixing bugs.
>
> Storing a pointer to an xport, then checking later that it points to a
> valid xport, is risky because it opens up the possibility that the
> pointer points to a different xport that just happens to have the same
> address. It's hard to guess how likely that coincidence is, but it
> would be better to avoid it. This is the reason that frozen_state uses
> a random UUID to locate "ofprotos". Probably, that approach would be
> better for xports too.
I see, thank you for the explanation. I'm going to create a new patch using
xport UUIDs.
>
> When xport_in is nonnull but invalid, wouldn't it be better to abort
> than to search for an xport the fallback way?
Yes, that would be better. Especially if UUID is used to get the xport.
>
> What is the reason for the special case for patch ports?
I've performed some tests before created the patch. I had a setup with two
bridges connected with patch ports:
p1 +-------+ +-------+ p2
->-o | | o->-
| br1 o-----o br2 |
+-------+ +-------+
Recirculation can occur in the 2nd bridge as well, for instance when pop_mpls
action is followed by resubmit. In this case a new upcall is performed and
xlate_lookup_ofproto_() and xport_lookup() are invoked again. As I observed,
xport_lookup() returns xport referring to p1 in br1, i.e. the very first port
where the packet was received on. I assume, this is not a bug in OVS, is it?
So, I would like to store this very first port in the frozen state. That's the
reason why I do store xport in ctx.xin->xport_in only if xport is not a patch
port. If it's not a patch port, it can be only the very first input port.
>
> Can you provide a good example of where this is important?
The main goal of this patch is to avoid invocation of xlate_lookup() in case of
recirculation (except recirc due to bond), because it can return a wrong xport.
For instance, if L3 packet with MPLS label is received on a L3 tunnel port and
pop_mpls + resubmit actions are performed, then first packet_type is changed
due to pushing a dummy ethernet header, MPLS label is removed, then resubmit
action is processed. This triggers recirculation, where xport_lookup() fails
due to former change of packet_type as I noted in the commit message.
>
> Thanks,
>
> Ben.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
