[PATCH v2 net-next] net/ipv6: Do not allow route add with a device that is down

[PATCH v2 net-next] net/ipv6: Do not allow route add with a device that is down

[David Ahern]

IPv6 allows routes to be installed when the device is not up (admin up).
Worse, it does not mark it as LINKDOWN. IPv4 does not allow it and really
there is no reason for IPv6 to allow it, so check the flags and deny if
device is admin down.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
v2
- missed setting err to -ENETDOWN (thanks for catching that Roopa)

 net/ipv6/route.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index f85da2f1e729..4e8fab766018 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2734,6 +2734,12 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,
 	if (!dev)
 		goto out;
 
+	err = -ENETDOWN;
+	if (!(dev->flags & IFF_UP)) {
+		NL_SET_ERR_MSG(extack, "Nexthop device is not up");
+		goto out;
+	}
+
 	if (!ipv6_addr_any(&cfg->fc_prefsrc)) {
 		if (!ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0)) {
 			NL_SET_ERR_MSG(extack, "Invalid source address");
-- 
2.11.0

Re: [PATCH v2 net-next] net/ipv6: Do not allow route add with a device that is down

[Stephen Hemminger]

On Wed, 24 Jan 2018 08:29:24 -0800
David Ahern <dsahern@gmail.com> wrote:

> IPv6 allows routes to be installed when the device is not up (admin up).
> Worse, it does not mark it as LINKDOWN. IPv4 does not allow it and really
> there is no reason for IPv6 to allow it, so check the flags and deny if
> device is admin down.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
> v2
> - missed setting err to -ENETDOWN (thanks for catching that Roopa)
> 
>  net/ipv6/route.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index f85da2f1e729..4e8fab766018 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -2734,6 +2734,12 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,
>  	if (!dev)
>  		goto out;
>  
> +	err = -ENETDOWN;
> +	if (!(dev->flags & IFF_UP)) {
> +		NL_SET_ERR_MSG(extack, "Nexthop device is not up");
> +		goto out;
> +	}
> +
>  	if (!ipv6_addr_any(&cfg->fc_prefsrc)) {
>  		if (!ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0)) {
>  			NL_SET_ERR_MSG(extack, "Invalid source address");

This looks like a good idea.

There are two equal ways to check for admin up. Either the dev flags or
look at link state via netif_running().  Maybe the latter would
be better.

Re: [PATCH v2 net-next] net/ipv6: Do not allow route add with a device that is down

[Eric Dumazet]

On Wed, 2018-01-24 at 08:29 -0800, David Ahern wrote:
> IPv6 allows routes to be installed when the device is not up (admin up).
> Worse, it does not mark it as LINKDOWN. IPv4 does not allow it and really
> there is no reason for IPv6 to allow it, so check the flags and deny if
> device is admin down.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
> v2
> - missed setting err to -ENETDOWN (thanks for catching that Roopa)
> 
>  net/ipv6/route.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index f85da2f1e729..4e8fab766018 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -2734,6 +2734,12 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,
>  	if (!dev)
>  		goto out;
>  
> +	err = -ENETDOWN;
> +	if (!(dev->flags & IFF_UP)) {
> +		NL_SET_ERR_MSG(extack, "Nexthop device is not up");
> +		goto out;
> +	}


Note that since you have to take a branch, you can move the 
err = -ENETDOWN inside the branch.

The trick of setting err before doing :

if (cond)
   goto xxxx;

Is only relevant if the goto is naked.

By looking at your patch without more context, it is not clear that the
"err = -ENETDOWN" has no side effect.

Re: [PATCH v2 net-next] net/ipv6: Do not allow route add with a device that is down

[David Ahern]

On 1/24/18 10:23 AM, Eric Dumazet wrote:
> On Wed, 2018-01-24 at 08:29 -0800, David Ahern wrote:
>> IPv6 allows routes to be installed when the device is not up (admin up).
>> Worse, it does not mark it as LINKDOWN. IPv4 does not allow it and really
>> there is no reason for IPv6 to allow it, so check the flags and deny if
>> device is admin down.
>>
>> Signed-off-by: David Ahern <dsahern@gmail.com>
>> ---
>> v2
>> - missed setting err to -ENETDOWN (thanks for catching that Roopa)
>>
>>  net/ipv6/route.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index f85da2f1e729..4e8fab766018 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -2734,6 +2734,12 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,
>>  	if (!dev)
>>  		goto out;
>>  
>> +	err = -ENETDOWN;
>> +	if (!(dev->flags & IFF_UP)) {
>> +		NL_SET_ERR_MSG(extack, "Nexthop device is not up");
>> +		goto out;
>> +	}
> 
> 
> Note that since you have to take a branch, you can move the 
> err = -ENETDOWN inside the branch.
> 
> The trick of setting err before doing :
> 
> if (cond)
>    goto xxxx;
> 
> Is only relevant if the goto is naked.

yes, i was tempted to put it inside the brackets, but recall in the past
some reviewers have strong opinions about it being set first. I can move it.

> 
> By looking at your patch without more context, it is not clear that the
> "err = -ENETDOWN" has no side effect.
> 

It doesn't. only 1 error path after this code and it sets err.

Re: [PATCH v2 net-next] net/ipv6: Do not allow route add with a device that is down

[David Ahern]

On 1/24/18 10:09 AM, Stephen Hemminger wrote:
> On Wed, 24 Jan 2018 08:29:24 -0800
> David Ahern <dsahern@gmail.com> wrote:
> 
>> IPv6 allows routes to be installed when the device is not up (admin up).
>> Worse, it does not mark it as LINKDOWN. IPv4 does not allow it and really
>> there is no reason for IPv6 to allow it, so check the flags and deny if
>> device is admin down.
>>
>> Signed-off-by: David Ahern <dsahern@gmail.com>
>> ---
>> v2
>> - missed setting err to -ENETDOWN (thanks for catching that Roopa)
>>
>>  net/ipv6/route.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index f85da2f1e729..4e8fab766018 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -2734,6 +2734,12 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,
>>  	if (!dev)
>>  		goto out;
>>  
>> +	err = -ENETDOWN;
>> +	if (!(dev->flags & IFF_UP)) {
>> +		NL_SET_ERR_MSG(extack, "Nexthop device is not up");
>> +		goto out;
>> +	}
>> +
>>  	if (!ipv6_addr_any(&cfg->fc_prefsrc)) {
>>  		if (!ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0)) {
>>  			NL_SET_ERR_MSG(extack, "Invalid source address");
> 
> This looks like a good idea.
> 
> There are two equal ways to check for admin up. Either the dev flags or
> look at link state via netif_running().  Maybe the latter would
> be better.
> 

I used dev->flags for consistency with IPv4. Looking at use of
netif_running vs dev->flags in IPv4 and IPv6 code the flag is much more
prevalent.