| Message ID | 49F1DE6F.1030606@trash.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Suppose that someone has eth0, eth0.2 and eth0.5 admin UP. Then, sets eth0.5 admin DOWN. Later they set eth0 down and up for some reason. Would eth0.5 now go admin UP? If so, I think that is not a good idea and could possibly be considered a security issue. I think instead there should be a new flag for VLANs that is 'preferred-admin-state'. To be UP, both the underlying device and the preferred state must be UP. That should allow bouncing eth0 w/out affecting the eventual admin state of the VLANs on eth0 in the example above. For what it's worth, it seems that other virtual devices, such as VIFS on a wifi radio, might need the same sort of behaviour. Thanks, Ben
Ben Greear <greearb@candelatech.com> wrote: >Suppose that someone has eth0, eth0.2 and eth0.5 admin UP. > >Then, sets eth0.5 admin DOWN. > >Later they set eth0 down and up for some reason. > >Would eth0.5 now go admin UP? Yes, with the patch applied, it does. >If so, I think that is not a good idea and could possibly be considered >a security issue. I think instead there should be a new flag for VLANs >that is 'preferred-admin-state'. To be UP, both the underlying device >and the preferred state must be UP. That should allow bouncing eth0 >w/out affecting the eventual admin state of the VLANs on eth0 in >the example above. I dunno if it's a security issue, but I'd agree it's wrong. I looked, and I suspect that a couple of judiciously placed "if (dev->flags & IFF_UP)" bits ought to sort things out by simply not copying the carrier state to the upper level VLAN device if that device is down. Unless somebody works this up over the weekend, I'll work that out on Monday. When the vlan device (eth0.5 in the above example) later is set up, it'll do the right thing for its own carrier state. >For what it's worth, it seems that other virtual devices, such as VIFS >on a wifi radio, might need the same sort of behaviour. Would they actually need a flag, or could it be worked out just by checking their IFF_UP-ness? -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Patrick McHardy <kaber@trash.net> Date: Fri, 24 Apr 2009 17:44:47 +0200 > vlan: update vlan carrier state for admin up/down > > Currently, the VLAN event handler does not adjust the VLAN > device's carrier state when the real device or the VLAN device is set > administratively up or down. > > The following patch adds a transfer of operating state from the > real device to the VLAN device when the real device is administratively > set up or down, and sets the carrier state up or down during init, open > and close of the VLAN device. > > This permits observers above the VLAN device that care about the > carrier state (bonding's link monitor, for example) to receive updates > for administrative changes by more closely mimicing the behavior of real > devices. > > Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> > Signed-off-by: Patrick McHardy <kaber@trash.net> I've applied this to net-2.6, thanks! I realize there are some more issues to deal with wrt. the comments Ben made. And I eagerly await patches for that :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jay Vosburgh wrote: >> If so, I think that is not a good idea and could possibly be considered >> a security issue. I think instead there should be a new flag for VLANs >> that is 'preferred-admin-state'. To be UP, both the underlying device >> and the preferred state must be UP. That should allow bouncing eth0 >> w/out affecting the eventual admin state of the VLANs on eth0 in >> the example above. >> > > I dunno if it's a security issue, but I'd agree it's wrong. I > looked, and I suspect that a couple of judiciously placed "if > (dev->flags & IFF_UP)" bits ought to sort things out by simply not > copying the carrier state to the upper level VLAN device if that device > is down. Unless somebody works this up over the weekend, I'll work that > out on Monday. > That may be fine. I suppose you'll also be ignoring admin state changes for the underlying devices in the VLAN code? Ie, if eth0 goes admin down, you won't change the eth0.5 vlan to be admin down? > Would they actually need a flag, or could it be worked out just > by checking their IFF_UP-ness? > If no admin state is propagated at all, I think you can skip any extra flags. But, if you want to propagate admin state (as the kernel does now with VLANs), then I don't see how you can get by without another flag. Thanks, Ben
Ben Greear <greearb@candelatech.com> wrote: >Jay Vosburgh wrote: >>> If so, I think that is not a good idea and could possibly be considered >>> a security issue. I think instead there should be a new flag for VLANs >>> that is 'preferred-admin-state'. To be UP, both the underlying device >>> and the preferred state must be UP. That should allow bouncing eth0 >>> w/out affecting the eventual admin state of the VLANs on eth0 in >>> the example above. >>> >> >> I dunno if it's a security issue, but I'd agree it's wrong. I >> looked, and I suspect that a couple of judiciously placed "if >> (dev->flags & IFF_UP)" bits ought to sort things out by simply not >> copying the carrier state to the upper level VLAN device if that device >> is down. Unless somebody works this up over the weekend, I'll work that >> out on Monday. >> >That may be fine. I suppose you'll also be ignoring admin state changes >for the >underlying devices in the VLAN code? Ie, if eth0 goes admin down, you won't >change the eth0.5 vlan to be admin down? Ok, I did a bit of testing on this, and I don't believe the "update vlan carrier state" patch I did changed anything in this regard. Without the patch, given a configuration of eth0 with eth0.600 and eth0.700 stacked above, the sequence "ifconfig eth0.700 down; ifconfig eth0 down; ifconfig eth0 up" results in eth0.700 being up at the end, and both eth0.600 and eth0.700 being down when eth0 is down (up and down here referring to IFF_UP, not carrier state). The VLAN event handler (vlan_device_event) deliberately sets all VLAN devices to match the real device when the real device goes up or down. I still agree that this is a bit counterintuitive, but the patch doesn't change the VLAN behavior in this regard. With the patch, the same thing happens, but the carrier state also changes to match (without the patch, the VLAN interfaces remain carrier up the whole time). I'm reluctant to mess with this behavior without knowing why it works this way; there may be a good reason for it that I'm not aware of. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jay Vosburgh <fubar@us.ibm.com> Date: Mon, 27 Apr 2009 16:55:58 -0700 > I'm reluctant to mess with this behavior without knowing why it > works this way; there may be a good reason for it that I'm not aware > of. To be honest I think it's just that this is one huge dark corner of behavior for many virtual devices, rather than any of it being intentional. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > From: Jay Vosburgh <fubar@us.ibm.com> > Date: Mon, 27 Apr 2009 16:55:58 -0700 > >> I'm reluctant to mess with this behavior without knowing why it >> works this way; there may be a good reason for it that I'm not aware >> of. > > To be honest I think it's just that this is one huge dark > corner of behavior for many virtual devices, rather than > any of it being intentional. For what it's worth, I've been using a patch that disables the behaviour in vlan that brings up/down the interface based on the underlying device for several years w/out noticeable problems. However, I may just be getting lucky. Ben
David Miller wrote: > From: Jay Vosburgh <fubar@us.ibm.com> > Date: Mon, 27 Apr 2009 16:55:58 -0700 > >> I'm reluctant to mess with this behavior without knowing why it >> works this way; there may be a good reason for it that I'm not aware >> of. > > To be honest I think it's just that this is one huge dark > corner of behavior for many virtual devices, rather than > any of it being intentional. Indeed. In case of VLANs it has always been this way and I'm reluctant to change the default behaviour since people might be relying on this to flush their routes or something similar. For new drivers I'd say they should always use operstate. For VLAN the safest way would be to add a flag to disable this behaviour. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
commit fcfdf9ecbaf759b658a9722af2862e595f2dec6b
Author: Jay Vosburgh <fubar@us.ibm.com>
Date: Fri Apr 24 17:22:08 2009 +0200
vlan: update vlan carrier state for admin up/down
Currently, the VLAN event handler does not adjust the VLAN
device's carrier state when the real device or the VLAN device is set
administratively up or down.
The following patch adds a transfer of operating state from the
real device to the VLAN device when the real device is administratively
set up or down, and sets the carrier state up or down during init, open
and close of the VLAN device.
This permits observers above the VLAN device that care about the
carrier state (bonding's link monitor, for example) to receive updates
for administrative changes by more closely mimicing the behavior of real
devices.
Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
Signed-off-by: Patrick McHardy <kaber@trash.net>
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 2b7390e..d1e1054 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -492,6 +492,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
continue;
dev_change_flags(vlandev, flgs & ~IFF_UP);
+ vlan_transfer_operstate(dev, vlandev);
}
break;
@@ -507,6 +508,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
continue;
dev_change_flags(vlandev, flgs | IFF_UP);
+ vlan_transfer_operstate(dev, vlandev);
}
break;
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 1b34135..2ce6658 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -462,6 +462,7 @@ static int vlan_dev_open(struct net_device *dev)
if (vlan->flags & VLAN_FLAG_GVRP)
vlan_gvrp_request_join(dev);
+ netif_carrier_on(dev);
return 0;
clear_allmulti:
@@ -471,6 +472,7 @@ del_unicast:
if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
dev_unicast_delete(real_dev, dev->dev_addr, ETH_ALEN);
out:
+ netif_carrier_off(dev);
return err;
}
@@ -492,6 +494,7 @@ static int vlan_dev_stop(struct net_device *dev)
if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len);
+ netif_carrier_off(dev);
return 0;
}
@@ -612,6 +615,8 @@ static int vlan_dev_init(struct net_device *dev)
struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
int subclass = 0;
+ netif_carrier_off(dev);
+
/* IFF_BROADCAST|IFF_MULTICAST; ??? */
dev->flags = real_dev->flags & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI);
dev->iflink = real_dev->ifindex;