[ovs-dev] [PATCH] mirroring: Fix check to avoid outputing to input port.

Ben Pfaff blp at nicira.com
Thu Oct 8 20:27:49 UTC 2009


Jesse Gross <jesse at nicira.com> writes:

> Ben Pfaff wrote:
>> 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.  

OK, so the current situation looks like this.  A packet comes in
on an implicitly tagged interface: flow->dl_vlan is
htons(OFP_VLAN_NONE), vlan is 123.  A mirror targets VLAN 123, so
we try to send it back out the port it came in on.  dst->vlan is
OFP_VLAN_NONE, vlan is 123, so we incorrectly fail to drop it.
Your flow_vlan would be OFP_VLAN_NONE, which would cause it to be
dropped, correctly.  OK, I understand now, that makes sense.

> 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.

OK.

Does your patch correctly handle mirroring to VLAN 0?  (That is
an unusual thing to do, and perhaps we should disallow it.)  I am
lost in a maze of confusing little variables at this point.




More information about the dev mailing list