Re: [Xen-devel] [PATCH] Prevent vif-bridge from adding user-created taps to a bridge

[Jim Fehlig <jfehlig@xxxxxxxx>]

Ian Campbell wrote:
> On Thu, 2011-10-27 at 06:28 +0100, Jim Fehlig wrote:
>   
>> Jim Fehlig wrote:
>>     
>>> Ian Campbell wrote:
>>>   
>>>       
>>>> On Wed, 2011-10-26 at 00:06 +0100, Jim Fehlig wrote:
>>>>   
>>>>     
>>>>         
>>>>> I previously sent this from my @suse.com mail address without having
>>>>> subscribed it.  Sending again now that I have done so...
>>>>>
>>>>> I received a report that vif-bridge adds any tap interface to a bridge,
>>>>> regardless if xen is running and who created the tap interface.  E.g.
>>>>>
>>>>> # tunctl -p -t tap42
>>>>>
>>>>> will cause vif-bridge to be executed as per the following rule in
>>>>> xen-backend.rules
>>>>>     
>>>>>       
>>>>>           
>>>> Oh dear.
>>>>
>>>>   
>>>>     
>>>>         
>>>>> SUBSYSTEM=="net", KERNEL=="tap*", ACTION=="add",
>>>>> RUN+="/etc/xen/scripts/vif-setup $env{ACTION} type_if=tap"
>>>>>
>>>>> I'm not sure how to improve the rule to prevent execution of vif-setup
>>>>> in this case.  But it seems better to handle it in vif-bridge anyhow, by
>>>>> not connecting the interface to a bridge if there is no corresponding
>>>>> info in xenstore.  Something along the lines of the attached quick
>>>>> patch.  Comments?
>>>>>     
>>>>>       
>>>>>           
>>>> I think overall your change is an improvement, some thoughts:
>>>>
>>>> For a tap device XENBUS_PATH is set in vif-common.sh:
>>>>         elif [ "$type_if" = tap ]; then
>>>>             # Check presence of compulsory args.
>>>>             : ${INTERFACE:?}
>>>>         
>>>>             # Get xenbus_path from device name.
>>>>             # The name is built like that: "tap${domid}.${devid}".
>>>>             dev_=${dev#tap}
>>>>             domid=${dev_%.*}
>>>>             devid=${dev_#*.}
>>>>         
>>>>             XENBUS_PATH="/local/domain/0/backend/vif/$domid/$devid"
>>>>         fi
>>>>
>>>> Could there be false positives from this?
>>>>     
>>>>         
>>> Hmm, yes, I think it is possible.
>>>   
>>>       
>> On second thought, maybe not.  A false positive would mean two tap
>> devices with the same name right?  AFAICT, that's not permitted.
>>     
>
> Oh right, we are given $dev aren't we.
>   
>>>   
>>>       
>>>>  Perhaps we should be more
>>>> aggressively checking for the tapX.Y, where X and Y are integers, format
>>>> as well? (that's not foolproof either though).
>>>>   
>>>>     
>>>>         
>>> Yeah, I don't think that buys us much.
>>>
>>>   
>>>       
>>>> Perhaps the toolstack could write something to xenstore containing the
>>>> literal tap device name which it asked qemu for? Then we can simply read
>>>> it back here, e.g. /libxl/tap/0/tapX.Y -> $XENBUS_PATH (0 being the
>>>> backend domain and the content being the xenbus path so we don't need to
>>>> magic it up).
>>>>     
>>>>         
>> The toolstack already writes something in xenstore, namely
>> $XENBUS_PATH/bridge.
>>     
>
> XENBUS_PATH here is really the vif backend path, not the tap path,
> although they in some way are aliased so in many cases that ok. I was
> just thinking it might be useful to have a backend space for the tap
> device only (since the guest can see the vif backend dir).
>   

So you prefer this approach to solving the problem?

>   
>>   IMO, the problem is in vif-bridge
>>
>> bridge=${bridge:-}
>> bridge=$(xenstore_read_default "$XENBUS_PATH/bridge" "$bridge")
>>
>> if [ -z "$bridge" ]
>> then
>>   bridge=$(brctl show | cut -d "
>> " -f 2 | cut -f 1)
>>
>>   if [ -z "$bridge" ]
>>   then
>>      fatal "Could not find bridge, and none was specified"
>>   fi
>> else
>>   ...
>>
>> If the toolstack hasn't written anything to xenstore, vif-bridge happily
>> connects the tap device to the first bridge it finds.  Shouldn't
>> vif-bridge just exit if no bridge is specified?
>>     
>
> I think that behaviour is historical (which isn't to say it's correct).
>   

Connecting the device to an arbitrary bridge seems dangerous to me. 
What if the bridge is on a sensitive VLAN?

> FWIW xl defaults to writing xenbr0. I don't know what xend does.
>   

xend writes nothing to that node if bridge is not specified in the vif
config :-(.  I suppose that is the reason for the hack in vif-bridge,
which was a bad fix IMO.

Thanks,
Jim
> Ian.
>
>   

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel