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

Jesse Gross jesse at nicira.com
Thu Oct 8 20:43:51 UTC 2009



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

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.




More information about the dev mailing list