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

Jesse Gross 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.
>>>     
>>
>> 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.
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org




More information about the dev mailing list