From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Ahern Subject: Re: [PATCH v2 net-next] net/ipv6: Do not allow route add with a device that is down Date: Wed, 24 Jan 2018 10:29:46 -0700 Message-ID: References: <20180124162924.6984-1-dsahern@gmail.com> <1516814614.3715.24.camel@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: yoshfuji@linux-ipv6.org, idosch@mellanox.com, roopa@cumulusnetworks.com To: Eric Dumazet , netdev@vger.kernel.org Return-path: Received: from mail-pf0-f195.google.com ([209.85.192.195]:40366 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964790AbeAXR3t (ORCPT ); Wed, 24 Jan 2018 12:29:49 -0500 Received: by mail-pf0-f195.google.com with SMTP id i66so3605947pfd.7 for ; Wed, 24 Jan 2018 09:29:48 -0800 (PST) In-Reply-To: <1516814614.3715.24.camel@gmail.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 >> --- >> 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.