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

[Jim Fehlig <jfehlig@suse.com>]

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.

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

I think this is a better approach.  But generally, we don't ask qemu for
a tap device right?  Only when using an emulated NIC afaik.  It seems I
should be able to write the info you suggested to xenstore in
libxl_device_nic_add().  The front and back paths and their contents are
already being created there.

Thanks,
Jim