On Wed, Mar 27, 2019 at 11:23:21AM -0700, David Ahern wrote:
> From: David Ahern <dsah...@gmail.com>
>
> Similar to IPv4, consolidate the fib6_nh initialization into a helper.
>
> Signed-off-by: David Ahern <dsah...@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
>
