netdev - Re: [PATCHv2 net 2/2] ipv4: update RTNH_F

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:	Tue, 27 Oct 2015 09:42:25 +0200 (EET)
From:	Julian Anastasov <ja@....bg>
To:	Andy Gospodarek <gospo@...ulusnetworks.com>
cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCHv2 net 2/2] ipv4: update RTNH_F_LINKDOWN flag on UP
 event


	Hello,

On Tue, 27 Oct 2015, Andy Gospodarek wrote:

> I tested this patch and I now see that your reported problem is a result
> of dummy never taking carrier down.  There was a presumption that
> carrier notification would go down when hardware went down (or when the
> logical device backing the hardware went down, but this is clearly not
> always the case.

	It seems not all devices play with the carrier
after IFF_UP is set and we can not rely on NETDEV_CHANGE
to update the flag.

> > +	if (nh_flags & RTNH_F_DEAD) {
> > +		unsigned int flags = dev_get_flags(dev);
> > +
> > +		if (flags & (IFF_RUNNING | IFF_LOWER_UP))
> > +			nh_flags |= RTNH_F_LINKDOWN;
> > +	}
> > +
> >  	prev_fi = NULL;
> >  	hash = fib_devindex_hashfn(dev->ifindex);
> >  	head = &fib_info_devhash[hash];
> 
> Logically this patch makes sense, but I feel as though there may be a
> slightly better option.  Possibly this:
> 
> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index 42778d9..7eb7c40 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -1376,7 +1376,8 @@ int fib_sync_down_dev(struct net_device *dev, unsigned long event)
>  					nexthop_nh->nh_flags |= RTNH_F_DEAD;
>  					/* fall through */
>  				case NETDEV_CHANGE:
> -					nexthop_nh->nh_flags |= RTNH_F_LINKDOWN;
> +					if (!netif_carrier_ok(dev))
> +						nexthop_nh->nh_flags |= RTNH_F_LINKDOWN;

	There is a problem with this approach. Once
the RTNH_F_DEAD flag is set, eg. when last address is removed,
any NETDEV_CHANGE events are ignored in this function.
As result, we may miss the link-down event if we first
remove the addresses, so we will not set RTNH_F_LINKDOWN.

	Also, when device link goes UP we (FIB) can not guess
just based on events what is the actual carrier state
because the NETDEV_CHANGE notification comes only when
IFF_UP is set. So, this check.

	I also attempted to fully recalculate the flag
in fib_sync_up, i.e. with the option not just to clear it
but also to add nexthop_nh->nh_flags |= nh_flags_set logic
but it complicates the code. So, while we always set
RTNH_F_LINKDOWN when DEAD is set, the logic to conditionally
clear RTNH_F_LINKDOWN in fib_sync_up looks the cheapest one.

	Of course, we have a semantic problem when setting
RTNH_F_LINKDOWN on last address removal, i.e. this event
has nothing to do with the link state. But it works because
RTNH_F_LINKDOWN is valid for lookups only when DEAD flag
is not set, so that is why my patch looks this way.

>  					break;
>  				}
>  				dead++;
> @@ -1396,7 +1397,8 @@ int fib_sync_down_dev(struct net_device *dev, unsigned long event)
>  				fi->fib_flags |= RTNH_F_DEAD;
>  				/* fall through */
>  			case NETDEV_CHANGE:
> -				fi->fib_flags |= RTNH_F_LINKDOWN;
> +				if (!netif_carrier_ok(dev))
> +					fi->fib_flags |= RTNH_F_LINKDOWN;
>  				break;
>  			}
>  			ret++;

	I think, we even do not need the RTNH_F_LINKDOWN flag
in fib_flags, currently it is set but never used.

Regards

--
Julian Anastasov <ja@....bg>
--
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