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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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

>   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).
FWIW xl defaults to writing xenbr0. I don't know what xend does.

Ian.


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