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

[Jim Fehlig <jfehlig@suse.com>]

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.

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

Thanks,
Jim