From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: David Ahern <dsahern@kernel.org>
Cc: davem@davemloft.net, netdev@vger.kernel.org, idosch@mellanox.com,
jiri@mellanox.com, saeedm@mellanox.com,
David Ahern <dsahern@gmail.com>
Subject: Re: [PATCH v2 net-next 05/13] ipv6: Create init helper for fib6_nh
Date: Wed, 27 Mar 2019 15:52:12 -0700 [thread overview]
Message-ID: <20190327225210.kbpivej33a72adwq@ast-mbp> (raw)
In-Reply-To: <20190327182329.18149-6-dsahern@kernel.org>
On Wed, Mar 27, 2019 at 11:23:21AM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Similar to IPv4, consolidate the fib6_nh initialization into a helper.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
> include/net/ip6_fib.h | 4 +
> net/ipv6/route.c | 235 +++++++++++++++++++++++++++-----------------------
> 2 files changed, 131 insertions(+), 108 deletions(-)
>
> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
> index 2acb78a762ee..ce1f81345c8e 100644
> --- a/include/net/ip6_fib.h
> +++ b/include/net/ip6_fib.h
> @@ -444,6 +444,10 @@ static inline struct net_device *fib6_info_nh_dev(const struct fib6_info *f6i)
> return f6i->fib6_nh.nh_dev;
> }
>
> +int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
> + struct fib6_config *cfg, gfp_t gfp_flags,
> + struct netlink_ext_ack *extack);
> +
> static inline
> struct lwtunnel_state *fib6_info_nh_lwt(const struct fib6_info *f6i)
> {
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index b804be3cbf05..e577d2d51b5f 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -2896,16 +2896,133 @@ static int ip6_validate_gw(struct net *net, struct fib6_config *cfg,
> return err;
> }
>
> +int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
> + struct fib6_config *cfg, gfp_t gfp_flags,
> + struct netlink_ext_ack *extack)
> +{
> + struct net_device *dev = NULL;
> + struct inet6_dev *idev = NULL;
> + int addr_type;
> + int err;
> +
> + err = -ENODEV;
> + if (cfg->fc_ifindex) {
> + dev = dev_get_by_index(net, cfg->fc_ifindex);
> + if (!dev)
> + goto out;
> + idev = in6_dev_get(dev);
> + if (!idev)
> + goto out;
> + }
> +
> + if (cfg->fc_flags & RTNH_F_ONLINK) {
> + if (!dev) {
> + NL_SET_ERR_MSG(extack,
> + "Nexthop device required for onlink");
> + goto out;
> + }
> +
> + if (!(dev->flags & IFF_UP)) {
> + NL_SET_ERR_MSG(extack, "Nexthop device is not up");
> + err = -ENETDOWN;
> + goto out;
> + }
> +
> + fib6_nh->nh_flags |= RTNH_F_ONLINK;
> + }
> +
> + if (cfg->fc_encap) {
> + struct lwtunnel_state *lwtstate;
> +
> + err = lwtunnel_build_state(cfg->fc_encap_type,
> + cfg->fc_encap, AF_INET6, cfg,
> + &lwtstate, extack);
> + if (err)
> + goto out;
> +
> + fib6_nh->nh_lwtstate = lwtstate_get(lwtstate);
> + }
> +
> + fib6_nh->nh_weight = 1;
> +
> + /* We cannot add true routes via loopback here,
> + * they would result in kernel looping; promote them to reject routes
> + */
> + addr_type = ipv6_addr_type(&cfg->fc_dst);
> + if ((cfg->fc_flags & RTF_REJECT) ||
> + (dev && (dev->flags & IFF_LOOPBACK) &&
> + !(addr_type & IPV6_ADDR_LOOPBACK) &&
> + !(cfg->fc_flags & RTF_LOCAL))) {
> + /* hold loopback dev/idev if we haven't done so. */
> + if (dev != net->loopback_dev) {
> + if (dev) {
> + dev_put(dev);
> + in6_dev_put(idev);
> + }
> + dev = net->loopback_dev;
> + dev_hold(dev);
> + idev = in6_dev_get(dev);
> + if (!idev) {
> + err = -ENODEV;
> + goto out;
> + }
> + }
> + cfg->fc_flags = RTF_REJECT | RTF_NONEXTHOP;
imo it would be cleaner not to mess with cfg.
Ideally it should be marked 'const'.
> + goto set_dev;
> + }
> +
> + if (cfg->fc_flags & RTF_GATEWAY) {
> + err = ip6_validate_gw(net, cfg, &dev, &idev, extack);
> + if (err)
> + goto out;
> +
> + fib6_nh->nh_gw = cfg->fc_gateway;
> + }
> +
> + err = -ENODEV;
> + if (!dev)
> + goto out;
> +
> + if (idev->cnf.disable_ipv6) {
> + NL_SET_ERR_MSG(extack, "IPv6 is disabled on nexthop device");
> + err = -EACCES;
> + goto out;
> + }
> +
> + if (!(dev->flags & IFF_UP) && !cfg->fc_ignore_dev_down) {
> + NL_SET_ERR_MSG(extack, "Nexthop device is not up");
> + err = -ENETDOWN;
> + goto out;
> + }
> +
> + if (!(cfg->fc_flags & (RTF_LOCAL | RTF_ANYCAST)) &&
> + !netif_carrier_ok(dev))
> + fib6_nh->nh_flags |= RTNH_F_LINKDOWN;
> +
> +set_dev:
> + fib6_nh->nh_dev = dev;
> + err = 0;
> +out:
> + if (idev)
> + in6_dev_put(idev);
> +
> + if (err) {
> + lwtstate_put(fib6_nh->nh_lwtstate);
lwtstate_put() is missing in the error path of existing code.
Is this a bug fix?
Why there is nothing about this in commit log?
> + fib6_nh->nh_lwtstate = NULL;
> + if (dev)
> + dev_put(dev);
> + }
> +
> + return err;
> +}
> +
> static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
> gfp_t gfp_flags,
> struct netlink_ext_ack *extack)
> {
> struct net *net = cfg->fc_nlinfo.nl_net;
> struct fib6_info *rt = NULL;
> - struct net_device *dev = NULL;
> - struct inet6_dev *idev = NULL;
> struct fib6_table *table;
> - int addr_type;
> int err = -EINVAL;
>
> /* RTF_PCPU is an internal flag; can not be set by userspace */
> @@ -2940,30 +3057,6 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
> goto out;
> }
> #endif
> - if (cfg->fc_ifindex) {
> - err = -ENODEV;
> - dev = dev_get_by_index(net, cfg->fc_ifindex);
> - if (!dev)
> - goto out;
> - idev = in6_dev_get(dev);
> - if (!idev)
> - goto out;
> - }
> -
> - if (cfg->fc_flags & RTNH_F_ONLINK) {
> - if (!dev) {
> - NL_SET_ERR_MSG(extack,
> - "Nexthop device required for onlink");
> - err = -ENODEV;
> - goto out;
> - }
> -
> - if (!(dev->flags & IFF_UP)) {
> - NL_SET_ERR_MSG(extack, "Nexthop device is not up");
> - err = -ENETDOWN;
> - goto out;
> - }
> - }
>
> err = -ENOBUFS;
> if (cfg->fc_nlinfo.nlh &&
> @@ -3007,18 +3100,9 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
> cfg->fc_protocol = RTPROT_BOOT;
> rt->fib6_protocol = cfg->fc_protocol;
>
> - addr_type = ipv6_addr_type(&cfg->fc_dst);
> -
> - if (cfg->fc_encap) {
> - struct lwtunnel_state *lwtstate;
> -
> - err = lwtunnel_build_state(cfg->fc_encap_type,
> - cfg->fc_encap, AF_INET6, cfg,
> - &lwtstate, extack);
> - if (err)
> - goto out;
> - rt->fib6_nh.nh_lwtstate = lwtstate_get(lwtstate);
> - }
> + rt->fib6_table = table;
> + rt->fib6_metric = cfg->fc_metric;
> + rt->fib6_type = cfg->fc_type;
>
> ipv6_addr_prefix(&rt->fib6_dst.addr, &cfg->fc_dst, cfg->fc_dst_len);
> rt->fib6_dst.plen = cfg->fc_dst_len;
> @@ -3029,62 +3113,13 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
> ipv6_addr_prefix(&rt->fib6_src.addr, &cfg->fc_src, cfg->fc_src_len);
> rt->fib6_src.plen = cfg->fc_src_len;
> #endif
> -
> - rt->fib6_metric = cfg->fc_metric;
> - rt->fib6_nh.nh_weight = 1;
> -
> - rt->fib6_type = cfg->fc_type;
> -
> - /* We cannot add true routes via loopback here,
> - they would result in kernel looping; promote them to reject routes
> - */
> - if ((cfg->fc_flags & RTF_REJECT) ||
> - (dev && (dev->flags & IFF_LOOPBACK) &&
> - !(addr_type & IPV6_ADDR_LOOPBACK) &&
> - !(cfg->fc_flags & RTF_LOCAL))) {
> - /* hold loopback dev/idev if we haven't done so. */
> - if (dev != net->loopback_dev) {
> - if (dev) {
> - dev_put(dev);
> - in6_dev_put(idev);
> - }
> - dev = net->loopback_dev;
> - dev_hold(dev);
> - idev = in6_dev_get(dev);
> - if (!idev) {
> - err = -ENODEV;
> - goto out;
> - }
> - }
> - rt->fib6_flags = RTF_REJECT|RTF_NONEXTHOP;
> - goto install_route;
> - }
> -
> - if (cfg->fc_flags & RTF_GATEWAY) {
> - err = ip6_validate_gw(net, cfg, &dev, &idev, extack);
> - if (err)
> - goto out;
> -
> - rt->fib6_nh.nh_gw = cfg->fc_gateway;
> - }
> -
> - err = -ENODEV;
> - if (!dev)
> - goto out;
> -
> - if (idev->cnf.disable_ipv6) {
> - NL_SET_ERR_MSG(extack, "IPv6 is disabled on nexthop device");
> - err = -EACCES;
> - goto out;
> - }
> -
> - if (!(dev->flags & IFF_UP) && !cfg->fc_ignore_dev_down) {
> - NL_SET_ERR_MSG(extack, "Nexthop device is not up");
> - err = -ENETDOWN;
> + err = fib6_nh_init(net, &rt->fib6_nh, cfg, gfp_flags, extack);
> + if (err)
> goto out;
> - }
>
> if (!ipv6_addr_any(&cfg->fc_prefsrc)) {
> + struct net_device *dev = fib6_info_nh_dev(rt);
> +
> if (!ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0)) {
> NL_SET_ERR_MSG(extack, "Invalid source address");
> err = -EINVAL;
> @@ -3097,24 +3132,8 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>
> rt->fib6_flags = cfg->fc_flags;
>
> -install_route:
> - if (!(rt->fib6_flags & (RTF_LOCAL | RTF_ANYCAST)) &&
> - !netif_carrier_ok(dev))
> - rt->fib6_nh.nh_flags |= RTNH_F_LINKDOWN;
> - rt->fib6_nh.nh_flags |= (cfg->fc_flags & RTNH_F_ONLINK);
> - rt->fib6_nh.nh_dev = dev;
> - rt->fib6_table = table;
> -
> - if (idev)
> - in6_dev_put(idev);
> -
> return rt;
> out:
> - if (dev)
> - dev_put(dev);
> - if (idev)
> - in6_dev_put(idev);
> -
> fib6_info_release(rt);
> return ERR_PTR(err);
> }
> --
> 2.11.0
>
next prev parent reply other threads:[~2019-03-27 22:52 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-27 18:23 [PATCH v2 net-next 00/13] net: Move fib_nh and fib6_nh to a common struct David Ahern
2019-03-27 18:23 ` [PATCH v2 net-next 01/13] ipv4: Define fib_get_nhs when CONFIG_IP_ROUTE_MULTIPATH is disabled David Ahern
2019-03-27 18:23 ` [PATCH v2 net-next 02/13] ipv4: Move IN_DEV_IGNORE_ROUTES_WITH_LINKDOWN to helper David Ahern
2019-03-27 18:23 ` [PATCH v2 net-next 03/13] ipv4: Create init helper for fib_nh David Ahern
2019-03-27 18:23 ` [PATCH v2 net-next 04/13] ipv4: Create cleanup " David Ahern
2019-03-27 18:23 ` [PATCH v2 net-next 05/13] ipv6: Create init helper for fib6_nh David Ahern
2019-03-27 22:52 ` Alexei Starovoitov [this message]
2019-03-28 0:47 ` David Ahern
2019-03-28 1:30 ` Alexei Starovoitov
2019-03-28 2:05 ` David Ahern
2019-03-28 2:29 ` Alexei Starovoitov
2019-03-28 2:50 ` David Ahern
2019-03-28 3:04 ` Alexei Starovoitov
2019-03-28 2:11 ` David Ahern
2019-03-27 18:23 ` [PATCH v2 net-next 06/13] ipv6: Create cleanup " David Ahern
2019-03-27 18:23 ` [PATCH v2 net-next 07/13] ipv6: Move gateway checks to a fib6_nh setting David Ahern
2019-03-27 18:23 ` [PATCH v2 net-next 08/13] ipv6: Refactor fib6_ignore_linkdown David Ahern
2019-03-27 18:23 ` [PATCH v2 net-next 09/13] ipv6: Change rt6_add_nexthop and rt6_nexthop_info to take fib6_nh David Ahern
2019-03-27 18:23 ` [PATCH v2 net-next 10/13] ipv4: Rename fib_nh entries David Ahern
2019-03-27 18:23 ` [PATCH v2 net-next 11/13] ipv6: Rename fib6_nh entries David Ahern
2019-03-27 18:23 ` [PATCH v2 net-next 12/13] net: Add fib_nh_common and update fib_nh and fib6_nh David Ahern
2019-03-27 18:23 ` [PATCH v2 net-next 13/13] net: Use common nexthop init and release helpers David Ahern
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190327225210.kbpivej33a72adwq@ast-mbp \
--to=alexei.starovoitov@gmail.com \
--cc=davem@davemloft.net \
--cc=dsahern@gmail.com \
--cc=dsahern@kernel.org \
--cc=idosch@mellanox.com \
--cc=jiri@mellanox.com \
--cc=netdev@vger.kernel.org \
--cc=saeedm@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox