* [PATCH] Prevent vif-bridge from adding user-created taps to a bridge
@ 2011-10-25 23:06 Jim Fehlig
2011-10-26 7:37 ` Ian Campbell
0 siblings, 1 reply; 7+ messages in thread
From: Jim Fehlig @ 2011-10-25 23:06 UTC (permalink / raw)
To: xen-devel
[-- Attachment #1: Type: text/plain, Size: 800 bytes --]
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
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?
Thanks!
Jim
[-- Attachment #2: vif-bridge.patch --]
[-- Type: text/x-patch, Size: 943 bytes --]
# HG changeset patch
# User Jim Fehlig <jfehlig@suse.com>
# Date 1319581952 21600
# Node ID 74da2a3a1db1476d627f42e4a99e9e720cc6774d
# Parent 6c583d35d76dda2236c81d9437ff9d57ab02c006
Prevent vif-bridge from adding user-created tap interfaces to a bridge
Exit vif-bridge script if there is no device info in xenstore, preventing
it from adding user-created taps to bridges.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
diff -r 6c583d35d76d -r 74da2a3a1db1 tools/hotplug/Linux/vif-bridge
--- a/tools/hotplug/Linux/vif-bridge Thu Oct 20 15:36:01 2011 +0100
+++ b/tools/hotplug/Linux/vif-bridge Tue Oct 25 16:32:32 2011 -0600
@@ -31,6 +31,13 @@
dir=$(dirname "$0")
. "$dir/vif-common.sh"
+
+domu=$(xenstore_read_default "$XENBUS_PATH/domain" "")
+if [ -z "$domu" ]
+then
+ log debug "No device details in $XENBUS_PATH, exiting."
+ exit 0
+fi
bridge=${bridge:-}
bridge=$(xenstore_read_default "$XENBUS_PATH/bridge" "$bridge")
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Prevent vif-bridge from adding user-created taps to a bridge 2011-10-25 23:06 [PATCH] Prevent vif-bridge from adding user-created taps to a bridge Jim Fehlig @ 2011-10-26 7:37 ` Ian Campbell 2011-10-26 18:13 ` Jim Fehlig 0 siblings, 1 reply; 7+ messages in thread From: Ian Campbell @ 2011-10-26 7:37 UTC (permalink / raw) To: Jim Fehlig; +Cc: xen-devel 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? 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). 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). Ian. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Prevent vif-bridge from adding user-created taps to a bridge 2011-10-26 7:37 ` Ian Campbell @ 2011-10-26 18:13 ` Jim Fehlig 2011-10-27 5:28 ` Jim Fehlig 0 siblings, 1 reply; 7+ messages in thread From: Jim Fehlig @ 2011-10-26 18:13 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Prevent vif-bridge from adding user-created taps to a bridge 2011-10-26 18:13 ` Jim Fehlig @ 2011-10-27 5:28 ` Jim Fehlig 2011-10-27 9:02 ` Ian Campbell 0 siblings, 1 reply; 7+ messages in thread From: Jim Fehlig @ 2011-10-27 5:28 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Prevent vif-bridge from adding user-created taps to a bridge 2011-10-27 5:28 ` Jim Fehlig @ 2011-10-27 9:02 ` Ian Campbell 2011-10-27 15:13 ` Jim Fehlig 0 siblings, 1 reply; 7+ messages in thread From: Ian Campbell @ 2011-10-27 9:02 UTC (permalink / raw) To: Jim Fehlig; +Cc: xen-devel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Prevent vif-bridge from adding user-created taps to a bridge 2011-10-27 9:02 ` Ian Campbell @ 2011-10-27 15:13 ` Jim Fehlig 2011-10-27 15:16 ` Ian Campbell 0 siblings, 1 reply; 7+ messages in thread From: Jim Fehlig @ 2011-10-27 15:13 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel 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. > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Prevent vif-bridge from adding user-created taps to a bridge 2011-10-27 15:13 ` Jim Fehlig @ 2011-10-27 15:16 ` Ian Campbell 0 siblings, 0 replies; 7+ messages in thread From: Ian Campbell @ 2011-10-27 15:16 UTC (permalink / raw) To: Jim Fehlig; +Cc: xen-devel On Thu, 2011-10-27 at 16:13 +0100, Jim Fehlig wrote: > > 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? It's probably the right thing to do long term but your initial patch seems like a reasonable enough fix right now. Ian. > > > > >> 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. > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-10-27 15:16 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-25 23:06 [PATCH] Prevent vif-bridge from adding user-created taps to a bridge Jim Fehlig 2011-10-26 7:37 ` Ian Campbell 2011-10-26 18:13 ` Jim Fehlig 2011-10-27 5:28 ` Jim Fehlig 2011-10-27 9:02 ` Ian Campbell 2011-10-27 15:13 ` Jim Fehlig 2011-10-27 15:16 ` Ian Campbell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.