[RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

[RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov <nikolay@nvidia.com>

Currently we have different cleanup expectations by different users of
fib6_nh_init:
 1. nh_create_ipv6
 - calls fib6_nh_release manually which does full cleanup

 2. ip6_route_info_create
 - calls fib6_info_release to drop refs to 0 and schedules rcu call
   for fib6_info_destroy_rcu() which also does full cleanup

 3. fib_check_nh_v6_gw
 - doesn't do any cleanup on error, expects fib6_nh_init to clean up
   after itself fully (nhc_pcpu_rth_output per-cpu memory leak on error)

We can alter fib6_nh_init to properly cleanup after itself so
expectations would be the same for everyone and noone would have to do
anything in such case. It is safe because the route is not inserted yet
so the fib6_nh should not be visible at fib6_nh_init point, thus it
should be possible to free up all resources in its error path. The
problems (and leaks) are because it doesn't free all resources in its
error path, the nhc_pcpu_rth_output per-cpu allocation done by
fib_nh_common_init is not cleaned up and it expects its callers to clean
up if an error occurred after it, e.g. the dst per-cpu allocation
might fail. This change allows us to fix the memory leak at 3. and also
to simplify nh_create_ipv6 and remove the special error handling.

Fixes: 717a8f5b2923 ("ipv4: Add fib_check_nh_v6_gw")
Signed-off-by: Nikolay Aleksandrov <nikolay@nvidia.com>
---
Sending as RFC to see what people think. I've tested this patch under
heavy load with replacing/traffic forwarding/nexthop add/del etc.
I've also tested error paths by adding artificial ENOMEM errors in
different fib6_nh_init stages while running kmemleak.

 net/ipv4/nexthop.c |  8 +-------
 net/ipv6/route.c   | 15 +++++++++------
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 5dbd4b5505eb..a7debafe8b90 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -2565,14 +2565,8 @@ static int nh_create_ipv6(struct net *net,  struct nexthop *nh,
 	/* sets nh_dev if successful */
 	err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
 				      extack);
-	if (err) {
-		/* IPv6 is not enabled, don't call fib6_nh_release */
-		if (err == -EAFNOSUPPORT)
-			goto out;
-		ipv6_stub->fib6_nh_release(fib6_nh);
-	} else {
+	if (!err)
 		nh->nh_flags = fib6_nh->fib_nh_flags;
-	}
 out:
 	return err;
 }
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 42d60c76d30a..2107b13cc9ab 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3635,7 +3635,9 @@ int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
 		in6_dev_put(idev);
 
 	if (err) {
-		lwtstate_put(fib6_nh->fib_nh_lws);
+		/* check if we failed after fib_nh_common_init() was called */
+		if (fib6_nh->nh_common.nhc_pcpu_rth_output)
+			fib_nh_common_release(&fib6_nh->nh_common);
 		fib6_nh->fib_nh_lws = NULL;
 		dev_put(dev);
 	}
@@ -3822,7 +3824,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
 	} else {
 		err = fib6_nh_init(net, rt->fib6_nh, cfg, gfp_flags, extack);
 		if (err)
-			goto out;
+			goto out_free;
 
 		fib6_nh = rt->fib6_nh;
 
@@ -3841,7 +3843,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
 		if (!ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0)) {
 			NL_SET_ERR_MSG(extack, "Invalid source address");
 			err = -EINVAL;
-			goto out;
+			goto out_free;
 		}
 		rt->fib6_prefsrc.addr = cfg->fc_prefsrc;
 		rt->fib6_prefsrc.plen = 128;
@@ -3849,12 +3851,13 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
 		rt->fib6_prefsrc.plen = 0;
 
 	return rt;
-out:
-	fib6_info_release(rt);
-	return ERR_PTR(err);
+
 out_free:
 	ip_fib_metrics_put(rt->fib6_metrics);
+	if (rt->nh)
+		nexthop_put(rt->nh);
 	kfree(rt);
+out:
 	return ERR_PTR(err);
 }
 
-- 
2.31.1

Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

[kernel test robot]

Hi Nikolay,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Nikolay-Aleksandrov/net-ipv6-make-fib6_nh_init-properly-clean-after-itself-on-error/20211130-001132
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 2191b1dfef7d45f44b5008d2148676d9f2c82874
config: i386-randconfig-r002-20211128 (https://download.01.org/0day-ci/archive/20211130/202111300529.K2YL6F1D-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/2db1685345b4a1aecadba0a8197c79f5e49da8ec
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nikolay-Aleksandrov/net-ipv6-make-fib6_nh_init-properly-clean-after-itself-on-error/20211130-001132
        git checkout 2db1685345b4a1aecadba0a8197c79f5e49da8ec
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/ipv4/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/ipv4/nexthop.c:2570:1: warning: unused label 'out' [-Wunused-label]
   out:
   ^~~~
   1 warning generated.


vim +/out +2570 net/ipv4/nexthop.c

597cfe4fc3390a David Ahern         2019-05-24  2544  
53010f991a9f5e David Ahern         2019-05-24  2545  static int nh_create_ipv6(struct net *net,  struct nexthop *nh,
53010f991a9f5e David Ahern         2019-05-24  2546  			  struct nh_info *nhi, struct nh_config *cfg,
53010f991a9f5e David Ahern         2019-05-24  2547  			  struct netlink_ext_ack *extack)
53010f991a9f5e David Ahern         2019-05-24  2548  {
53010f991a9f5e David Ahern         2019-05-24  2549  	struct fib6_nh *fib6_nh = &nhi->fib6_nh;
53010f991a9f5e David Ahern         2019-05-24  2550  	struct fib6_config fib6_cfg = {
53010f991a9f5e David Ahern         2019-05-24  2551  		.fc_table = l3mdev_fib_table(cfg->dev),
53010f991a9f5e David Ahern         2019-05-24  2552  		.fc_ifindex = cfg->nh_ifindex,
53010f991a9f5e David Ahern         2019-05-24  2553  		.fc_gateway = cfg->gw.ipv6,
53010f991a9f5e David Ahern         2019-05-24  2554  		.fc_flags = cfg->nh_flags,
9aca491e0dccf8 Ryoga Saito         2021-09-02  2555  		.fc_nlinfo = cfg->nlinfo,
b513bd035f4044 David Ahern         2019-05-24  2556  		.fc_encap = cfg->nh_encap,
b513bd035f4044 David Ahern         2019-05-24  2557  		.fc_encap_type = cfg->nh_encap_type,
38428d68719c45 Roopa Prabhu        2020-05-21  2558  		.fc_is_fdb = cfg->nh_fdb,
53010f991a9f5e David Ahern         2019-05-24  2559  	};
6f43e5252833f3 Colin Ian King      2019-05-30  2560  	int err;
53010f991a9f5e David Ahern         2019-05-24  2561  
53010f991a9f5e David Ahern         2019-05-24  2562  	if (!ipv6_addr_any(&cfg->gw.ipv6))
53010f991a9f5e David Ahern         2019-05-24  2563  		fib6_cfg.fc_flags |= RTF_GATEWAY;
53010f991a9f5e David Ahern         2019-05-24  2564  
53010f991a9f5e David Ahern         2019-05-24  2565  	/* sets nh_dev if successful */
53010f991a9f5e David Ahern         2019-05-24  2566  	err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
53010f991a9f5e David Ahern         2019-05-24  2567  				      extack);
2db1685345b4a1 Nikolay Aleksandrov 2021-11-29  2568  	if (!err)
53010f991a9f5e David Ahern         2019-05-24  2569  		nh->nh_flags = fib6_nh->fib_nh_flags;
1c743127cc54b1 Nikolay Aleksandrov 2021-11-23 @2570  out:
53010f991a9f5e David Ahern         2019-05-24  2571  	return err;
53010f991a9f5e David Ahern         2019-05-24  2572  }
53010f991a9f5e David Ahern         2019-05-24  2573  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4185 bytes --]

Hi Nikolay,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Nikolay-Aleksandrov/net-ipv6-make-fib6_nh_init-properly-clean-after-itself-on-error/20211130-001132
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 2191b1dfef7d45f44b5008d2148676d9f2c82874
config: i386-randconfig-r002-20211128 (https://download.01.org/0day-ci/archive/20211130/202111300529.K2YL6F1D-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/2db1685345b4a1aecadba0a8197c79f5e49da8ec
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nikolay-Aleksandrov/net-ipv6-make-fib6_nh_init-properly-clean-after-itself-on-error/20211130-001132
        git checkout 2db1685345b4a1aecadba0a8197c79f5e49da8ec
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/ipv4/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/ipv4/nexthop.c:2570:1: warning: unused label 'out' [-Wunused-label]
   out:
   ^~~~
   1 warning generated.


vim +/out +2570 net/ipv4/nexthop.c

597cfe4fc3390a David Ahern         2019-05-24  2544  
53010f991a9f5e David Ahern         2019-05-24  2545  static int nh_create_ipv6(struct net *net,  struct nexthop *nh,
53010f991a9f5e David Ahern         2019-05-24  2546  			  struct nh_info *nhi, struct nh_config *cfg,
53010f991a9f5e David Ahern         2019-05-24  2547  			  struct netlink_ext_ack *extack)
53010f991a9f5e David Ahern         2019-05-24  2548  {
53010f991a9f5e David Ahern         2019-05-24  2549  	struct fib6_nh *fib6_nh = &nhi->fib6_nh;
53010f991a9f5e David Ahern         2019-05-24  2550  	struct fib6_config fib6_cfg = {
53010f991a9f5e David Ahern         2019-05-24  2551  		.fc_table = l3mdev_fib_table(cfg->dev),
53010f991a9f5e David Ahern         2019-05-24  2552  		.fc_ifindex = cfg->nh_ifindex,
53010f991a9f5e David Ahern         2019-05-24  2553  		.fc_gateway = cfg->gw.ipv6,
53010f991a9f5e David Ahern         2019-05-24  2554  		.fc_flags = cfg->nh_flags,
9aca491e0dccf8 Ryoga Saito         2021-09-02  2555  		.fc_nlinfo = cfg->nlinfo,
b513bd035f4044 David Ahern         2019-05-24  2556  		.fc_encap = cfg->nh_encap,
b513bd035f4044 David Ahern         2019-05-24  2557  		.fc_encap_type = cfg->nh_encap_type,
38428d68719c45 Roopa Prabhu        2020-05-21  2558  		.fc_is_fdb = cfg->nh_fdb,
53010f991a9f5e David Ahern         2019-05-24  2559  	};
6f43e5252833f3 Colin Ian King      2019-05-30  2560  	int err;
53010f991a9f5e David Ahern         2019-05-24  2561  
53010f991a9f5e David Ahern         2019-05-24  2562  	if (!ipv6_addr_any(&cfg->gw.ipv6))
53010f991a9f5e David Ahern         2019-05-24  2563  		fib6_cfg.fc_flags |= RTF_GATEWAY;
53010f991a9f5e David Ahern         2019-05-24  2564  
53010f991a9f5e David Ahern         2019-05-24  2565  	/* sets nh_dev if successful */
53010f991a9f5e David Ahern         2019-05-24  2566  	err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
53010f991a9f5e David Ahern         2019-05-24  2567  				      extack);
2db1685345b4a1 Nikolay Aleksandrov 2021-11-29  2568  	if (!err)
53010f991a9f5e David Ahern         2019-05-24  2569  		nh->nh_flags = fib6_nh->fib_nh_flags;
1c743127cc54b1 Nikolay Aleksandrov 2021-11-23 @2570  out:
53010f991a9f5e David Ahern         2019-05-24  2571  	return err;
53010f991a9f5e David Ahern         2019-05-24  2572  }
53010f991a9f5e David Ahern         2019-05-24  2573  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

[Ido Schimmel]

On Mon, Nov 29, 2021 at 04:11:51PM +0200, Nikolay Aleksandrov wrote:
> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index 5dbd4b5505eb..a7debafe8b90 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -2565,14 +2565,8 @@ static int nh_create_ipv6(struct net *net,  struct nexthop *nh,
>  	/* sets nh_dev if successful */
>  	err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
>  				      extack);
> -	if (err) {
> -		/* IPv6 is not enabled, don't call fib6_nh_release */
> -		if (err == -EAFNOSUPPORT)
> -			goto out;
> -		ipv6_stub->fib6_nh_release(fib6_nh);
> -	} else {
> +	if (!err)
>  		nh->nh_flags = fib6_nh->fib_nh_flags;
> -	}
>  out:
>  	return err;
>  }

This hunk looks good

> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 42d60c76d30a..2107b13cc9ab 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3635,7 +3635,9 @@ int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
>  		in6_dev_put(idev);
>  
>  	if (err) {
> -		lwtstate_put(fib6_nh->fib_nh_lws);
> +		/* check if we failed after fib_nh_common_init() was called */
> +		if (fib6_nh->nh_common.nhc_pcpu_rth_output)
> +			fib_nh_common_release(&fib6_nh->nh_common);
>  		fib6_nh->fib_nh_lws = NULL;
>  		dev_put(dev);
>  	}

Likewise

> @@ -3822,7 +3824,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>  	} else {
>  		err = fib6_nh_init(net, rt->fib6_nh, cfg, gfp_flags, extack);
>  		if (err)
> -			goto out;
> +			goto out_free;
>  
>  		fib6_nh = rt->fib6_nh;
>  
> @@ -3841,7 +3843,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>  		if (!ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0)) {
>  			NL_SET_ERR_MSG(extack, "Invalid source address");
>  			err = -EINVAL;
> -			goto out;
> +			goto out_free;
>  		}
>  		rt->fib6_prefsrc.addr = cfg->fc_prefsrc;
>  		rt->fib6_prefsrc.plen = 128;
> @@ -3849,12 +3851,13 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>  		rt->fib6_prefsrc.plen = 0;
>  
>  	return rt;
> -out:
> -	fib6_info_release(rt);
> -	return ERR_PTR(err);
> +
>  out_free:
>  	ip_fib_metrics_put(rt->fib6_metrics);
> +	if (rt->nh)
> +		nexthop_put(rt->nh);

Shouldn't this be above ip_fib_metrics_put() given nexthop_get() is
called after ip_fib_metrics_init() ?

Also, shouldn't we call fib6_nh_release() if fib6_nh_init() succeeded
and we failed later?

>  	kfree(rt);
> +out:
>  	return ERR_PTR(err);
>  }
>  
> -- 
> 2.31.1
> 

Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

[Nikolay Aleksandrov]

On 30/11/2021 14:40, Ido Schimmel wrote:
> On Mon, Nov 29, 2021 at 04:11:51PM +0200, Nikolay Aleksandrov wrote:
>> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
>> index 5dbd4b5505eb..a7debafe8b90 100644
>> --- a/net/ipv4/nexthop.c
>> +++ b/net/ipv4/nexthop.c
>> @@ -2565,14 +2565,8 @@ static int nh_create_ipv6(struct net *net,  struct nexthop *nh,
>>  	/* sets nh_dev if successful */
>>  	err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
>>  				      extack);
>> -	if (err) {
>> -		/* IPv6 is not enabled, don't call fib6_nh_release */
>> -		if (err == -EAFNOSUPPORT)
>> -			goto out;
>> -		ipv6_stub->fib6_nh_release(fib6_nh);
>> -	} else {
>> +	if (!err)
>>  		nh->nh_flags = fib6_nh->fib_nh_flags;
>> -	}
>>  out:
>>  	return err;
>>  }
> 
> This hunk looks good
> 
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index 42d60c76d30a..2107b13cc9ab 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -3635,7 +3635,9 @@ int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
>>  		in6_dev_put(idev);
>>  
>>  	if (err) {
>> -		lwtstate_put(fib6_nh->fib_nh_lws);
>> +		/* check if we failed after fib_nh_common_init() was called */
>> +		if (fib6_nh->nh_common.nhc_pcpu_rth_output)
>> +			fib_nh_common_release(&fib6_nh->nh_common);
>>  		fib6_nh->fib_nh_lws = NULL;
>>  		dev_put(dev);
>>  	}
> 
> Likewise
> 
>> @@ -3822,7 +3824,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>  	} else {
>>  		err = fib6_nh_init(net, rt->fib6_nh, cfg, gfp_flags, extack);
>>  		if (err)
>> -			goto out;
>> +			goto out_free;
>>  
>>  		fib6_nh = rt->fib6_nh;
>>  
>> @@ -3841,7 +3843,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>  		if (!ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0)) {
>>  			NL_SET_ERR_MSG(extack, "Invalid source address");
>>  			err = -EINVAL;
>> -			goto out;
>> +			goto out_free;
>>  		}
>>  		rt->fib6_prefsrc.addr = cfg->fc_prefsrc;
>>  		rt->fib6_prefsrc.plen = 128;
>> @@ -3849,12 +3851,13 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>  		rt->fib6_prefsrc.plen = 0;
>>  
>>  	return rt;
>> -out:
>> -	fib6_info_release(rt);
>> -	return ERR_PTR(err);
>> +
>>  out_free:
>>  	ip_fib_metrics_put(rt->fib6_metrics);
>> +	if (rt->nh)
>> +		nexthop_put(rt->nh);
> 
> Shouldn't this be above ip_fib_metrics_put() given nexthop_get() is
> called after ip_fib_metrics_init() ?
> 

yeah, that's ok for symmetry

> Also, shouldn't we call fib6_nh_release() if fib6_nh_init() succeeded
> and we failed later?
> 

Hmm, that's a clear bug. I was only looking at nexthop objects and completely
missed that there's an error possibility after the non-nexthop path.
You're correct, and in fact we have to add another error label specifically for
the non-nexthop case because we shouldn't do it in the nexthop case.

>>  	kfree(rt);
>> +out:
>>  	return ERR_PTR(err);
>>  }
>>  
>> -- 
>> 2.31.1
>>

Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

[David Ahern]

On 11/30/21 5:40 AM, Ido Schimmel wrote:
> On Mon, Nov 29, 2021 at 04:11:51PM +0200, Nikolay Aleksandrov wrote:
>> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
>> index 5dbd4b5505eb..a7debafe8b90 100644
>> --- a/net/ipv4/nexthop.c
>> +++ b/net/ipv4/nexthop.c
>> @@ -2565,14 +2565,8 @@ static int nh_create_ipv6(struct net *net,  struct nexthop *nh,
>>  	/* sets nh_dev if successful */
>>  	err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
>>  				      extack);
>> -	if (err) {
>> -		/* IPv6 is not enabled, don't call fib6_nh_release */
>> -		if (err == -EAFNOSUPPORT)
>> -			goto out;
>> -		ipv6_stub->fib6_nh_release(fib6_nh);
>> -	} else {
>> +	if (!err)
>>  		nh->nh_flags = fib6_nh->fib_nh_flags;
>> -	}
>>  out:
>>  	return err;
>>  }
> 
> This hunk looks good

agreed, but it should be a no-op now so this should be a net-next
cleanup patch.

> 
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index 42d60c76d30a..2107b13cc9ab 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -3635,7 +3635,9 @@ int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
>>  		in6_dev_put(idev);
>>  
>>  	if (err) {
>> -		lwtstate_put(fib6_nh->fib_nh_lws);
>> +		/* check if we failed after fib_nh_common_init() was called */
>> +		if (fib6_nh->nh_common.nhc_pcpu_rth_output)
>> +			fib_nh_common_release(&fib6_nh->nh_common);
>>  		fib6_nh->fib_nh_lws = NULL;
>>  		dev_put(dev);
>>  	}
> 
> Likewise

this is a leak in the current code and should go through -net as a
separate patch.

> 
>> @@ -3822,7 +3824,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>  	} else {
>>  		err = fib6_nh_init(net, rt->fib6_nh, cfg, gfp_flags, extack);
>>  		if (err)
>> -			goto out;
>> +			goto out_free;
>>  
>>  		fib6_nh = rt->fib6_nh;
>>  
>> @@ -3841,7 +3843,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>  		if (!ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0)) {
>>  			NL_SET_ERR_MSG(extack, "Invalid source address");
>>  			err = -EINVAL;
>> -			goto out;
>> +			goto out_free;
>>  		}
>>  		rt->fib6_prefsrc.addr = cfg->fc_prefsrc;
>>  		rt->fib6_prefsrc.plen = 128;
>> @@ -3849,12 +3851,13 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>  		rt->fib6_prefsrc.plen = 0;
>>  
>>  	return rt;
>> -out:
>> -	fib6_info_release(rt);
>> -	return ERR_PTR(err);
>> +
>>  out_free:
>>  	ip_fib_metrics_put(rt->fib6_metrics);
>> +	if (rt->nh)
>> +		nexthop_put(rt->nh);
> 
> Shouldn't this be above ip_fib_metrics_put() given nexthop_get() is
> called after ip_fib_metrics_init() ?
> 
> Also, shouldn't we call fib6_nh_release() if fib6_nh_init() succeeded
> and we failed later?

similarly I think this cleanup is a separate patch.


> 
>>  	kfree(rt);
>> +out:
>>  	return ERR_PTR(err);
>>  }
>>  
>> -- 
>> 2.31.1
>>

Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

[Nikolay Aleksandrov]

On 30/11/2021 18:01, David Ahern wrote:
> On 11/30/21 5:40 AM, Ido Schimmel wrote:
>> On Mon, Nov 29, 2021 at 04:11:51PM +0200, Nikolay Aleksandrov wrote:
>>> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
>>> index 5dbd4b5505eb..a7debafe8b90 100644
>>> --- a/net/ipv4/nexthop.c
>>> +++ b/net/ipv4/nexthop.c
>>> @@ -2565,14 +2565,8 @@ static int nh_create_ipv6(struct net *net,  struct nexthop *nh,
>>>  	/* sets nh_dev if successful */
>>>  	err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
>>>  				      extack);
>>> -	if (err) {
>>> -		/* IPv6 is not enabled, don't call fib6_nh_release */
>>> -		if (err == -EAFNOSUPPORT)
>>> -			goto out;
>>> -		ipv6_stub->fib6_nh_release(fib6_nh);
>>> -	} else {
>>> +	if (!err)
>>>  		nh->nh_flags = fib6_nh->fib_nh_flags;
>>> -	}
>>>  out:
>>>  	return err;
>>>  }
>>
>> This hunk looks good
> 
> agreed, but it should be a no-op now so this should be a net-next
> cleanup patch.
> 

Actually it is needed, it's not a cleanup or noop. If fib6_nh_init fails after fib_nh_common_init
in the per-cpu allocation then fib6_nh->nh_common's pointers will still be there but
freed, so it will lead to double free. We have to NULL them when freeing if we want to avoid that.

>>
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index 42d60c76d30a..2107b13cc9ab 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -3635,7 +3635,9 @@ int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
>>>  		in6_dev_put(idev);
>>>  
>>>  	if (err) {
>>> -		lwtstate_put(fib6_nh->fib_nh_lws);
>>> +		/* check if we failed after fib_nh_common_init() was called */
>>> +		if (fib6_nh->nh_common.nhc_pcpu_rth_output)
>>> +			fib_nh_common_release(&fib6_nh->nh_common);
>>>  		fib6_nh->fib_nh_lws = NULL;
>>>  		dev_put(dev);
>>>  	}
>>
>> Likewise
> 
> this is a leak in the current code and should go through -net as a
> separate patch.
> 

Yep, this is the point of this patch. :)

>>
>>> @@ -3822,7 +3824,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>>  	} else {
>>>  		err = fib6_nh_init(net, rt->fib6_nh, cfg, gfp_flags, extack);
>>>  		if (err)
>>> -			goto out;
>>> +			goto out_free;
>>>  
>>>  		fib6_nh = rt->fib6_nh;
>>>  
>>> @@ -3841,7 +3843,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>>  		if (!ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0)) {
>>>  			NL_SET_ERR_MSG(extack, "Invalid source address");
>>>  			err = -EINVAL;
>>> -			goto out;
>>> +			goto out_free;
>>>  		}
>>>  		rt->fib6_prefsrc.addr = cfg->fc_prefsrc;
>>>  		rt->fib6_prefsrc.plen = 128;
>>> @@ -3849,12 +3851,13 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>>  		rt->fib6_prefsrc.plen = 0;
>>>  
>>>  	return rt;
>>> -out:
>>> -	fib6_info_release(rt);
>>> -	return ERR_PTR(err);
>>> +
>>>  out_free:
>>>  	ip_fib_metrics_put(rt->fib6_metrics);
>>> +	if (rt->nh)
>>> +		nexthop_put(rt->nh);
>>
>> Shouldn't this be above ip_fib_metrics_put() given nexthop_get() is
>> called after ip_fib_metrics_init() ?
>>
>> Also, shouldn't we call fib6_nh_release() if fib6_nh_init() succeeded
>> and we failed later?
> 
> similarly I think this cleanup is a separate patch.
> 

Same thing, fib6_info_destroy_rcu -> fib6_nh_release would double-free the nh_common parts
if fib6_nh_init fails in the per-cpu allocation after fib_nh_common_init.
It is not a cleanup, but a result of the fix. If we want to keep it, we'll have to NULL
the nh_common parts when freeing them in fib_nh_common_release().

> 
>>
>>>  	kfree(rt);
>>> +out:
>>>  	return ERR_PTR(err);
>>>  }
>>>  
>>> -- 
>>> 2.31.1
>>>
> 

Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

[David Ahern]

On 11/30/21 9:45 AM, Nikolay Aleksandrov wrote:
> On 30/11/2021 18:01, David Ahern wrote:
>> On 11/30/21 5:40 AM, Ido Schimmel wrote:
>>> On Mon, Nov 29, 2021 at 04:11:51PM +0200, Nikolay Aleksandrov wrote:
>>>> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
>>>> index 5dbd4b5505eb..a7debafe8b90 100644
>>>> --- a/net/ipv4/nexthop.c
>>>> +++ b/net/ipv4/nexthop.c
>>>> @@ -2565,14 +2565,8 @@ static int nh_create_ipv6(struct net *net,  struct nexthop *nh,
>>>>  	/* sets nh_dev if successful */
>>>>  	err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
>>>>  				      extack);
>>>> -	if (err) {
>>>> -		/* IPv6 is not enabled, don't call fib6_nh_release */
>>>> -		if (err == -EAFNOSUPPORT)
>>>> -			goto out;
>>>> -		ipv6_stub->fib6_nh_release(fib6_nh);
>>>> -	} else {
>>>> +	if (!err)
>>>>  		nh->nh_flags = fib6_nh->fib_nh_flags;
>>>> -	}
>>>>  out:
>>>>  	return err;
>>>>  }
>>>
>>> This hunk looks good
>>
>> agreed, but it should be a no-op now so this should be a net-next
>> cleanup patch.
>>
> 
> Actually it is needed, it's not a cleanup or noop. If fib6_nh_init fails after fib_nh_common_init
> in the per-cpu allocation then fib6_nh->nh_common's pointers will still be there but
> freed, so it will lead to double free. We have to NULL them when freeing if we want to avoid that.

fib6_nh_init should do proper cleanup if it hits an error. Your bug fix
to get nhc_pcpu_rth_output freed should complete that. It can also set
the value to NULL to avoid double free on any code path.


> 
>>>
>>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>>> index 42d60c76d30a..2107b13cc9ab 100644
>>>> --- a/net/ipv6/route.c
>>>> +++ b/net/ipv6/route.c
>>>> @@ -3635,7 +3635,9 @@ int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
>>>>  		in6_dev_put(idev);
>>>>  
>>>>  	if (err) {
>>>> -		lwtstate_put(fib6_nh->fib_nh_lws);
>>>> +		/* check if we failed after fib_nh_common_init() was called */
>>>> +		if (fib6_nh->nh_common.nhc_pcpu_rth_output)
>>>> +			fib_nh_common_release(&fib6_nh->nh_common);
>>>>  		fib6_nh->fib_nh_lws = NULL;
>>>>  		dev_put(dev);
>>>>  	}
>>>
>>> Likewise
>>
>> this is a leak in the current code and should go through -net as a
>> separate patch.
>>
> 
> Yep, this is the point of this patch. :)
> 
>>>
>>>> @@ -3822,7 +3824,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>>>  	} else {
>>>>  		err = fib6_nh_init(net, rt->fib6_nh, cfg, gfp_flags, extack);
>>>>  		if (err)
>>>> -			goto out;
>>>> +			goto out_free;
>>>>  
>>>>  		fib6_nh = rt->fib6_nh;
>>>>  
>>>> @@ -3841,7 +3843,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>>>  		if (!ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0)) {
>>>>  			NL_SET_ERR_MSG(extack, "Invalid source address");
>>>>  			err = -EINVAL;
>>>> -			goto out;
>>>> +			goto out_free;
>>>>  		}
>>>>  		rt->fib6_prefsrc.addr = cfg->fc_prefsrc;
>>>>  		rt->fib6_prefsrc.plen = 128;
>>>> @@ -3849,12 +3851,13 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>>>  		rt->fib6_prefsrc.plen = 0;
>>>>  
>>>>  	return rt;
>>>> -out:
>>>> -	fib6_info_release(rt);
>>>> -	return ERR_PTR(err);
>>>> +
>>>>  out_free:
>>>>  	ip_fib_metrics_put(rt->fib6_metrics);
>>>> +	if (rt->nh)
>>>> +		nexthop_put(rt->nh);
>>>
>>> Shouldn't this be above ip_fib_metrics_put() given nexthop_get() is
>>> called after ip_fib_metrics_init() ?
>>>
>>> Also, shouldn't we call fib6_nh_release() if fib6_nh_init() succeeded
>>> and we failed later?
>>
>> similarly I think this cleanup is a separate patch.
>>
> 
> Same thing, fib6_info_destroy_rcu -> fib6_nh_release would double-free the nh_common parts
> if fib6_nh_init fails in the per-cpu allocation after fib_nh_common_init.
> It is not a cleanup, but a result of the fix. If we want to keep it, we'll have to NULL
> the nh_common parts when freeing them in fib_nh_common_release().

exactly. set it to NULL and make the -net patch as simple as possible

Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

[Nikolay Aleksandrov]

On 30/11/2021 19:18, David Ahern wrote:
> On 11/30/21 9:45 AM, Nikolay Aleksandrov wrote:
>> On 30/11/2021 18:01, David Ahern wrote:
>>> On 11/30/21 5:40 AM, Ido Schimmel wrote:
>>>> On Mon, Nov 29, 2021 at 04:11:51PM +0200, Nikolay Aleksandrov wrote:
>>>>> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
>>>>> index 5dbd4b5505eb..a7debafe8b90 100644
>>>>> --- a/net/ipv4/nexthop.c
>>>>> +++ b/net/ipv4/nexthop.c
>>>>> @@ -2565,14 +2565,8 @@ static int nh_create_ipv6(struct net *net,  struct nexthop *nh,
>>>>>  	/* sets nh_dev if successful */
>>>>>  	err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
>>>>>  				      extack);
>>>>> -	if (err) {
>>>>> -		/* IPv6 is not enabled, don't call fib6_nh_release */
>>>>> -		if (err == -EAFNOSUPPORT)
>>>>> -			goto out;
>>>>> -		ipv6_stub->fib6_nh_release(fib6_nh);
>>>>> -	} else {
>>>>> +	if (!err)
>>>>>  		nh->nh_flags = fib6_nh->fib_nh_flags;
>>>>> -	}
>>>>>  out:
>>>>>  	return err;
>>>>>  }
>>>>
>>>> This hunk looks good
>>>
>>> agreed, but it should be a no-op now so this should be a net-next
>>> cleanup patch.
>>>
>>
>> Actually it is needed, it's not a cleanup or noop. If fib6_nh_init fails after fib_nh_common_init
>> in the per-cpu allocation then fib6_nh->nh_common's pointers will still be there but
>> freed, so it will lead to double free. We have to NULL them when freeing if we want to avoid that.
> 
> fib6_nh_init should do proper cleanup if it hits an error. Your bug fix
> to get nhc_pcpu_rth_output freed should complete that. It can also set
> the value to NULL to avoid double free on any code path.
> 

Indeed, that's another way of achieving the same goal.

> 
>>
>>>>
>>>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>>>> index 42d60c76d30a..2107b13cc9ab 100644
>>>>> --- a/net/ipv6/route.c
>>>>> +++ b/net/ipv6/route.c
>>>>> @@ -3635,7 +3635,9 @@ int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
>>>>>  		in6_dev_put(idev);
>>>>>  
>>>>>  	if (err) {
>>>>> -		lwtstate_put(fib6_nh->fib_nh_lws);
>>>>> +		/* check if we failed after fib_nh_common_init() was called */
>>>>> +		if (fib6_nh->nh_common.nhc_pcpu_rth_output)
>>>>> +			fib_nh_common_release(&fib6_nh->nh_common);
>>>>>  		fib6_nh->fib_nh_lws = NULL;
>>>>>  		dev_put(dev);
>>>>>  	}
>>>>
>>>> Likewise
>>>
>>> this is a leak in the current code and should go through -net as a
>>> separate patch.
>>>
>>
>> Yep, this is the point of this patch. :)
>>
>>>>
>>>>> @@ -3822,7 +3824,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>>>>  	} else {
>>>>>  		err = fib6_nh_init(net, rt->fib6_nh, cfg, gfp_flags, extack);
>>>>>  		if (err)
>>>>> -			goto out;
>>>>> +			goto out_free;
>>>>>  
>>>>>  		fib6_nh = rt->fib6_nh;
>>>>>  
>>>>> @@ -3841,7 +3843,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>>>>  		if (!ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0)) {
>>>>>  			NL_SET_ERR_MSG(extack, "Invalid source address");
>>>>>  			err = -EINVAL;
>>>>> -			goto out;
>>>>> +			goto out_free;
>>>>>  		}
>>>>>  		rt->fib6_prefsrc.addr = cfg->fc_prefsrc;
>>>>>  		rt->fib6_prefsrc.plen = 128;
>>>>> @@ -3849,12 +3851,13 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>>>>  		rt->fib6_prefsrc.plen = 0;
>>>>>  
>>>>>  	return rt;
>>>>> -out:
>>>>> -	fib6_info_release(rt);
>>>>> -	return ERR_PTR(err);
>>>>> +
>>>>>  out_free:
>>>>>  	ip_fib_metrics_put(rt->fib6_metrics);
>>>>> +	if (rt->nh)
>>>>> +		nexthop_put(rt->nh);
>>>>
>>>> Shouldn't this be above ip_fib_metrics_put() given nexthop_get() is
>>>> called after ip_fib_metrics_init() ?
>>>>
>>>> Also, shouldn't we call fib6_nh_release() if fib6_nh_init() succeeded
>>>> and we failed later?
>>>
>>> similarly I think this cleanup is a separate patch.
>>>
>>
>> Same thing, fib6_info_destroy_rcu -> fib6_nh_release would double-free the nh_common parts
>> if fib6_nh_init fails in the per-cpu allocation after fib_nh_common_init.
>> It is not a cleanup, but a result of the fix. If we want to keep it, we'll have to NULL
>> the nh_common parts when freeing them in fib_nh_common_release().
> 
> exactly. set it to NULL and make the -net patch as simple as possible
> 

Sure, of course. I'd prefer to make the code consistent w.r.t expectations regardless of
the release cycle, but I don't have a strong preference. I'll post the fix with
NULLing the nh_common pointers.

Cheers,
 Nik

Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4601 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20211129141151.490533-1-razor@blackwall.org>
References: <20211129141151.490533-1-razor@blackwall.org>
TO: Nikolay Aleksandrov <razor@blackwall.org>

Hi Nikolay,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Nikolay-Aleksandrov/net-ipv6-make-fib6_nh_init-properly-clean-after-itself-on-error/20211130-001132
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 2191b1dfef7d45f44b5008d2148676d9f2c82874
:::::: branch date: 9 hours ago
:::::: commit date: 9 hours ago
config: x86_64-randconfig-s032-20211128 (https://download.01.org/0day-ci/archive/20211130/202111300910.7HWZTGBF-lkp(a)intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/2db1685345b4a1aecadba0a8197c79f5e49da8ec
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nikolay-Aleksandrov/net-ipv6-make-fib6_nh_init-properly-clean-after-itself-on-error/20211130-001132
        git checkout 2db1685345b4a1aecadba0a8197c79f5e49da8ec
        # save the config file to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> net/ipv4/nexthop.c:2570:1: sparse: sparse: unused label 'out'
   net/ipv4/nexthop.c: note: in included file (through include/linux/sysctl.h, include/net/net_namespace.h, include/linux/netdevice.h, ...):
   include/linux/rbtree.h:74:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
   include/linux/rbtree.h:74:9: sparse:    struct rb_node [noderef] __rcu *
   include/linux/rbtree.h:74:9: sparse:    struct rb_node *

vim +/out +2570 net/ipv4/nexthop.c

597cfe4fc3390a David Ahern         2019-05-24  2544  
53010f991a9f5e David Ahern         2019-05-24  2545  static int nh_create_ipv6(struct net *net,  struct nexthop *nh,
53010f991a9f5e David Ahern         2019-05-24  2546  			  struct nh_info *nhi, struct nh_config *cfg,
53010f991a9f5e David Ahern         2019-05-24  2547  			  struct netlink_ext_ack *extack)
53010f991a9f5e David Ahern         2019-05-24  2548  {
53010f991a9f5e David Ahern         2019-05-24  2549  	struct fib6_nh *fib6_nh = &nhi->fib6_nh;
53010f991a9f5e David Ahern         2019-05-24  2550  	struct fib6_config fib6_cfg = {
53010f991a9f5e David Ahern         2019-05-24  2551  		.fc_table = l3mdev_fib_table(cfg->dev),
53010f991a9f5e David Ahern         2019-05-24  2552  		.fc_ifindex = cfg->nh_ifindex,
53010f991a9f5e David Ahern         2019-05-24  2553  		.fc_gateway = cfg->gw.ipv6,
53010f991a9f5e David Ahern         2019-05-24  2554  		.fc_flags = cfg->nh_flags,
9aca491e0dccf8 Ryoga Saito         2021-09-02  2555  		.fc_nlinfo = cfg->nlinfo,
b513bd035f4044 David Ahern         2019-05-24  2556  		.fc_encap = cfg->nh_encap,
b513bd035f4044 David Ahern         2019-05-24  2557  		.fc_encap_type = cfg->nh_encap_type,
38428d68719c45 Roopa Prabhu        2020-05-21  2558  		.fc_is_fdb = cfg->nh_fdb,
53010f991a9f5e David Ahern         2019-05-24  2559  	};
6f43e5252833f3 Colin Ian King      2019-05-30  2560  	int err;
53010f991a9f5e David Ahern         2019-05-24  2561  
53010f991a9f5e David Ahern         2019-05-24  2562  	if (!ipv6_addr_any(&cfg->gw.ipv6))
53010f991a9f5e David Ahern         2019-05-24  2563  		fib6_cfg.fc_flags |= RTF_GATEWAY;
53010f991a9f5e David Ahern         2019-05-24  2564  
53010f991a9f5e David Ahern         2019-05-24  2565  	/* sets nh_dev if successful */
53010f991a9f5e David Ahern         2019-05-24  2566  	err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
53010f991a9f5e David Ahern         2019-05-24  2567  				      extack);
2db1685345b4a1 Nikolay Aleksandrov 2021-11-29  2568  	if (!err)
53010f991a9f5e David Ahern         2019-05-24  2569  		nh->nh_flags = fib6_nh->fib_nh_flags;
1c743127cc54b1 Nikolay Aleksandrov 2021-11-23 @2570  out:
53010f991a9f5e David Ahern         2019-05-24  2571  	return err;
53010f991a9f5e David Ahern         2019-05-24  2572  }
53010f991a9f5e David Ahern         2019-05-24  2573  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4341 bytes --]

Hi Nikolay,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Nikolay-Aleksandrov/net-ipv6-make-fib6_nh_init-properly-clean-after-itself-on-error/20211130-001132
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 2191b1dfef7d45f44b5008d2148676d9f2c82874
config: x86_64-randconfig-s032-20211128 (https://download.01.org/0day-ci/archive/20211130/202111300910.7HWZTGBF-lkp(a)intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
         # apt-get install sparse
         # sparse version: v0.6.4-dirty
         # https://github.com/0day-ci/linux/commit/2db1685345b4a1aecadba0a8197c79f5e49da8ec
         git remote add linux-review https://github.com/0day-ci/linux
         git fetch --no-tags linux-review Nikolay-Aleksandrov/net-ipv6-make-fib6_nh_init-properly-clean-after-itself-on-error/20211130-001132
         git checkout 2db1685345b4a1aecadba0a8197c79f5e49da8ec
         # save the config file to linux build tree
         make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
 >> net/ipv4/nexthop.c:2570:1: sparse: sparse: unused label 'out'
    net/ipv4/nexthop.c: note: in included file (through include/linux/sysctl.h, include/net/net_namespace.h, include/linux/netdevice.h, ...):
    include/linux/rbtree.h:74:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
    include/linux/rbtree.h:74:9: sparse:    struct rb_node [noderef] __rcu *
    include/linux/rbtree.h:74:9: sparse:    struct rb_node *

vim +/out +2570 net/ipv4/nexthop.c

597cfe4fc3390a David Ahern         2019-05-24  2544
53010f991a9f5e David Ahern         2019-05-24  2545  static int nh_create_ipv6(struct net *net,  struct nexthop *nh,
53010f991a9f5e David Ahern         2019-05-24  2546  			  struct nh_info *nhi, struct nh_config *cfg,
53010f991a9f5e David Ahern         2019-05-24  2547  			  struct netlink_ext_ack *extack)
53010f991a9f5e David Ahern         2019-05-24  2548  {
53010f991a9f5e David Ahern         2019-05-24  2549  	struct fib6_nh *fib6_nh = &nhi->fib6_nh;
53010f991a9f5e David Ahern         2019-05-24  2550  	struct fib6_config fib6_cfg = {
53010f991a9f5e David Ahern         2019-05-24  2551  		.fc_table = l3mdev_fib_table(cfg->dev),
53010f991a9f5e David Ahern         2019-05-24  2552  		.fc_ifindex = cfg->nh_ifindex,
53010f991a9f5e David Ahern         2019-05-24  2553  		.fc_gateway = cfg->gw.ipv6,
53010f991a9f5e David Ahern         2019-05-24  2554  		.fc_flags = cfg->nh_flags,
9aca491e0dccf8 Ryoga Saito         2021-09-02  2555  		.fc_nlinfo = cfg->nlinfo,
b513bd035f4044 David Ahern         2019-05-24  2556  		.fc_encap = cfg->nh_encap,
b513bd035f4044 David Ahern         2019-05-24  2557  		.fc_encap_type = cfg->nh_encap_type,
38428d68719c45 Roopa Prabhu        2020-05-21  2558  		.fc_is_fdb = cfg->nh_fdb,
53010f991a9f5e David Ahern         2019-05-24  2559  	};
6f43e5252833f3 Colin Ian King      2019-05-30  2560  	int err;
53010f991a9f5e David Ahern         2019-05-24  2561
53010f991a9f5e David Ahern         2019-05-24  2562  	if (!ipv6_addr_any(&cfg->gw.ipv6))
53010f991a9f5e David Ahern         2019-05-24  2563  		fib6_cfg.fc_flags |= RTF_GATEWAY;
53010f991a9f5e David Ahern         2019-05-24  2564
53010f991a9f5e David Ahern         2019-05-24  2565  	/* sets nh_dev if successful */
53010f991a9f5e David Ahern         2019-05-24  2566  	err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
53010f991a9f5e David Ahern         2019-05-24  2567  				      extack);
2db1685345b4a1 Nikolay Aleksandrov 2021-11-29  2568  	if (!err)
53010f991a9f5e David Ahern         2019-05-24  2569  		nh->nh_flags = fib6_nh->fib_nh_flags;
1c743127cc54b1 Nikolay Aleksandrov 2021-11-23 @2570  out:
53010f991a9f5e David Ahern         2019-05-24  2571  	return err;
53010f991a9f5e David Ahern         2019-05-24  2572  }
53010f991a9f5e David Ahern         2019-05-24  2573

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org