netdev - Re: [patch net-next 0/4] net: allow to change carrier from userspace

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121214171345.GC1652@minipsycho.brq.redhat.com>
Date:	Fri, 14 Dec 2012 18:13:45 +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 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?

Jiri

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