lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 14 Dec 2012 18:35:00 +0100
From:	Jiri Pirko <jiri@...nulli.us>
To:	Stephen Hemminger <shemminger@...tta.com>
Cc:	Flavio Leitner <fbl@...hat.com>,
	John Fastabend <john.fastabend@...il.com>,
	netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
	bhutchings@...arflare.com, mirqus@...il.com,
	greearb@...delatech.com
Subject: Re: [patch net-next 0/4] net: allow to change carrier from userspace

Fri, Dec 14, 2012 at 06:23:43PM CET, shemminger@...tta.com wrote:
>On Fri, 14 Dec 2012 18:13:45 +0100
>Jiri Pirko <jiri@...nulli.us> wrote:
>
>> Fri, Dec 14, 2012 at 05:59:18PM CET, shemminger@...tta.com wrote:
>> >On Fri, 14 Dec 2012 17:35:32 +0100
>> >Jiri Pirko <jiri@...nulli.us> wrote:
>> >
>> >> Fri, Dec 14, 2012 at 05:12:01PM CET, shemminger@...tta.com wrote:
>> >> >On Fri, 14 Dec 2012 15:41:34 +0100
>> >> >Jiri Pirko <jiri@...nulli.us> wrote:
>> >> >
>> >> >> Thu, Dec 13, 2012 at 07:20:51PM CET, shemminger@...tta.com wrote:
>> >> >> >On Thu, 13 Dec 2012 16:17:33 -0200
>> >> >> >Flavio Leitner <fbl@...hat.com> wrote:
>> >> >> >
>> >> >> >> On Thu, 13 Dec 2012 10:09:33 -0800
>> >> >> >> Stephen Hemminger <shemminger@...tta.com> wrote:
>> >> >> >> 
>> >> >> >> > On Thu, 13 Dec 2012 15:54:23 -0200
>> >> >> >> > Flavio Leitner <fbl@...hat.com> wrote:
>> >> >> >> > 
>> >> >> >> > > I am saying this because people are used to and there are scripts out
>> >> >> >> > > there using something like:
>> >> >> >> > > # ethtool <iface> | grep 'Link'
>> >> >> >> > > to react an interface failure.
>> >> >> >> > 
>> >> >> >> > Then the script is broken. It is asking about hardware state.
>> >> >> >> 
>> >> >> >> I was talking about the team master interface, so it makes sense
>> >> >> >> to check its 'hardware' state. Just think on 'bond0' interface
>> >> >> >> with no slaves. It should report Link detected: no.
>> >> >> >> 
>> >> >> >> See bond_release(), what happens if bond->slave_cnt == 0, for instance.
>> >> >> >> 
>> >> >> >
>> >> >> >I was thinking more that ethtool operation for reporting link on
>> >> >> >the team device should use the proper check rather than just using netif_carrier_ok(),
>> >> >> >the team ethtool operation for get_link should be check IFF_RUNNING flag
>> >> >> >in dev->flags which is controlled by operstate transistions.
>> >> >> 
>> >> >> I admit I'm bit confused now.
>> >> >> 
>> >> >> For example in bridge code:
>> >> >> in br_add_if() - netif_carrier_ok() is checked and by the value it is
>> >> >> decided if br_stp_enable_port() is called or not. Wouldn't it make more
>> >> >> sense to check IFF_RUNNING (or netif_oper_up()) here?
>> >> >> 
>> >> >> The reason I'm asing is that if team device is in bridge, carrier is
>> >> >> always ON and I'm fiddling with IF_OPER_UP and IF_OPER_DORMANT from
>> >> >> userspace, in current code, bridge wouldn't know the difference...
>> >> >> 
>> >> >> There are more exmaples of similar usage of netif_carrier_ok() in
>> >> >> bridge (called on ports), bonding (called on slaves), team code (called on ports).
>> >> >
>> >> >Yes the bridge should be fixed to work with user controlled devices.
>> >> 
>> >> Okay. I'll try to figure out some patchset over the weekend.
>> >> 
>> >> Thanks.
>> >> 
>> >
>> >Something like this seems needed.
>> >
>> >--- a/net/bridge/br_if.c	2012-10-25 09:11:15.627272524 -0700
>> >+++ b/net/bridge/br_if.c	2012-12-14 08:58:14.329847361 -0800
>> >@@ -66,14 +66,14 @@ void br_port_carrier_check(struct net_br
>> > 	struct net_device *dev = p->dev;
>> > 	struct net_bridge *br = p->br;
>> > 
>> >-	if (netif_running(dev) && netif_carrier_ok(dev))
>> >+	if (netif_running(dev) && netif_oper_up(dev))
>> > 		p->path_cost = port_cost(dev);
>> > 
>> > 	if (!netif_running(br->dev))
>> > 		return;
>> > 
>> > 	spin_lock_bh(&br->lock);
>> >-	if (netif_running(dev) && netif_carrier_ok(dev)) {
>> >+	if (netif_running(dev) && netif_oper_up(dev))
>> > 		if (p->state == BR_STATE_DISABLED)
>> > 			br_stp_enable_port(p);
>> > 	} else {
>> >--- a/net/bridge/br_notify.c	2012-10-25 09:11:15.631272484 -0700
>> >+++ b/net/bridge/br_notify.c	2012-12-14 08:57:36.954222724 -0800
>> >@@ -82,7 +82,7 @@ static int br_device_event(struct notifi
>> > 		break;
>> > 
>> > 	case NETDEV_UP:
>> >-		if (netif_carrier_ok(dev) && (br->dev->flags & IFF_UP)) {
>> >+		if (netif_running(br->dev) && netif_oper_up(dev)) {
>> > 			spin_lock_bh(&br->lock);
>> > 			br_stp_enable_port(p);
>> > 			spin_unlock_bh(&br->lock);
>> 
>> 
>> Yes. I have this already in my queue. I just spotted a problem though.
>> 
>> Lets say teamd sets operstate of the team device by values IF_OPER_UP
>> and IF_OPER_DORMANT depending on teamd states of ports.
>> What if one would like to use 802.1X supplicant on the same device?
>> That would not be possible.
>> 
>> This proves that the layering would not be correct. It look like the
>> carrier userspace set would be the correct thing to do after all...
>> 
>> What do you think?
>
>That is tough, you have two applications conflicting over control of
>the same state on the same device.


I do not think so. The thing is, they both do something different. Teamd
should care of the low level link and set that up accordingly.
802.1X supplicant is higher. This implies they should not use the same
interface, ergo IMHO Teamd should be able to set carrier directly.

And that is what is provided by this patchset.

ps: I think that the use of 802.1X supplicant over team device is legit
setup.

>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists