[ovs-dev] [PATCH] mirroring: Fix check to avoid outputing to input port.
jesse at nicira.com
Thu Oct 8 19:58:37 UTC 2009
Ben Pfaff wrote:
> Jesse Gross <jesse at nicira.com> writes:
>> When RSPANing with a bond, ensure that we don't receive a packet on
>> one interface and then output it on the other interface of the bond.
>> Also avoid sending the packet out the same interface it was received
>> on when an interfaced is configured with the VLAN that we are using
>> for RSPAN.
> I had to stare at this for a very long time before I saw how it
> would affect bond behavior. Would you mind breaking it into two
> patches, one for the bond fix and one for the vlan fix, then, to
> make it clear which is which?
Yeah, I knew that it wasn't all that clear but I didn't have a good way
to explain the change. Hopefully the new patch set is more descriptive.
> I don't understand the vlan fix. It appears to be making sure
> that 0 and OFP_VLAN_NONE are treated the same way. But I think
> that we have this property already, because of these invariants
> on the values in the existing test "dst->vlan == vlan":
> * dst->vlan is in 0...4095 (although perhaps we should
> only allow mirroring to nonzero VLANs).
> * vlan is in 0...4095 (see the assignment and test under
> the comment "Figure out what VLAN this packet belongs
> to" in process_flow()).
The key thing is that it is using the vlan tag from the original flow,
instead of the one stored in the vlan parameter. The vlan parameter
contains the vlan after any implicit tagging takes place. For packets
that are sent to output ports with implicit vlans, dst->vlan already has
the tag stripped off. In this case the comparison will fail and packets
will be sent out the input port on the same vlan. Also, I don't believe
that your first invariant is always correct: dst->vlan can also be
OFP_VLAN_NONE (-1), while vlan would be 0 in this situation. This fix
also handles that (probably rare) case.
More information about the dev