[ovs-dev] [PATCH] mirroring: Fix check to avoid outputing to input port.
jesse at nicira.com
Fri Oct 9 19:45:45 UTC 2009
Hearing no further comments, I went ahead and pushed this.
Jesse Gross wrote:
> Ben Pfaff wrote:
>> 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.
> Yes, exactly.
>>> 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.
>> 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.
> Yes, I'm pretty sure that it does. I agree that mirroring to VLAN 0
> is unusual but it doesn't seem like an inherently illegal thing to
> do. In this situation VLAN 0 is only handled differently in the sense
> that it is converted to OFP_VLAN_NONE, which is equivalent here. In
> any case, I traced through what I believe are all possible
> combinations and all of them do the right thing.
> dev mailing list
> dev at openvswitch.org
More information about the dev