From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jim Fehlig Subject: Re: [PATCH] Prevent vif-bridge from adding user-created taps to a bridge Date: Wed, 26 Oct 2011 23:28:33 -0600 Message-ID: <4EA8EC01.2010904@suse.com> References: <4EA740EB.7030804@suse.com> <1319614636.16747.39.camel@dagon.hellion.org.uk> <4EA84DBA.6070901@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4EA84DBA.6070901@suse.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Campbell Cc: xen-devel List-Id: xen-devel@lists.xenproject.org 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