diff mbox series

[net,v2,1/3] net: ipv6: add fib6_nh_release_dsts stub

Message ID 20211122151514.2813935-2-razor@blackwall.org (mailing list archive)
State Accepted
Commit 8837cbbf854246f5f4d565f21e6baa945d37aded
Delegated to: Netdev Maintainers
Headers show
Series net: nexthop: fix refcount issues when replacing groups | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2098 this patch: 2098
netdev/cc_maintainers fail 1 blamed authors not CCed: dsahern@kernel.org; 2 maintainers not CCed: yoshfuji@linux-ipv6.org dsahern@kernel.org
netdev/build_clang success Errors and warnings before: 306 this patch: 306
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 2224 this patch: 2224
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 46 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Nikolay Aleksandrov Nov. 22, 2021, 3:15 p.m. UTC 
From: Nikolay Aleksandrov <nikolay@nvidia.com>

We need a way to release a fib6_nh's per-cpu dsts when replacing
nexthops otherwise we can end up with stale per-cpu dsts which hold net
device references, so add a new IPv6 stub called fib6_nh_release_dsts.
It must be used after an RCU grace period, so no new dsts can be created
through a group's nexthop entry.
Similar to fib6_nh_release it shouldn't be used if fib6_nh_init has failed
so it doesn't need a dummy stub when IPv6 is not enabled.

Fixes: 7bf4796dd099 ("nexthops: add support for replace")
Signed-off-by: Nikolay Aleksandrov <nikolay@nvidia.com>
---
v2: no changes

 include/net/ip6_fib.h    |  1 +
 include/net/ipv6_stubs.h |  1 +
 net/ipv6/af_inet6.c      |  1 +
 net/ipv6/route.c         | 19 +++++++++++++++++++
 4 files changed, 22 insertions(+)

Comments

David Ahern Nov. 28, 2021, 7:21 p.m. UTC | #1 
On 11/22/21 8:15 AM, Nikolay Aleksandrov wrote:
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 3ae25b8ffbd6..42d60c76d30a 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3680,6 +3680,25 @@ void fib6_nh_release(struct fib6_nh *fib6_nh)
>  	fib_nh_common_release(&fib6_nh->nh_common);
>  }
>  
> +void fib6_nh_release_dsts(struct fib6_nh *fib6_nh)
> +{
> +	int cpu;
> +
> +	if (!fib6_nh->rt6i_pcpu)
> +		return;
> +
> +	for_each_possible_cpu(cpu) {
> +		struct rt6_info *pcpu_rt, **ppcpu_rt;
> +
> +		ppcpu_rt = per_cpu_ptr(fib6_nh->rt6i_pcpu, cpu);
> +		pcpu_rt = xchg(ppcpu_rt, NULL);
> +		if (pcpu_rt) {
> +			dst_dev_put(&pcpu_rt->dst);
> +			dst_release(&pcpu_rt->dst);
> +		}
> +	}
> +}
> +

this duplicates fib6_nh_release. Can you send a follow on to have it use
this new function?
Nikolay Aleksandrov Nov. 28, 2021, 8:14 p.m. UTC | #2 
On 28/11/2021 21:21, David Ahern wrote:
> On 11/22/21 8:15 AM, Nikolay Aleksandrov wrote:
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index 3ae25b8ffbd6..42d60c76d30a 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -3680,6 +3680,25 @@ void fib6_nh_release(struct fib6_nh *fib6_nh)
>>  	fib_nh_common_release(&fib6_nh->nh_common);
>>  }
>>  
>> +void fib6_nh_release_dsts(struct fib6_nh *fib6_nh)
>> +{
>> +	int cpu;
>> +
>> +	if (!fib6_nh->rt6i_pcpu)
>> +		return;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		struct rt6_info *pcpu_rt, **ppcpu_rt;
>> +
>> +		ppcpu_rt = per_cpu_ptr(fib6_nh->rt6i_pcpu, cpu);
>> +		pcpu_rt = xchg(ppcpu_rt, NULL);
>> +		if (pcpu_rt) {
>> +			dst_dev_put(&pcpu_rt->dst);
>> +			dst_release(&pcpu_rt->dst);
>> +		}
>> +	}
>> +}
>> +
> 
> this duplicates fib6_nh_release. Can you send a follow on to have it use
> this new function?
> 

It duplicates a part of it but in a safe way because the fib6_nh could still be visible,
while fib6_nh_release does it in a way that assumes it's not. I could re-use
this helper in fib6_nh_release though, since it doesn't matter how the entries are
freed there. I'm guessing that is what you meant?
I'll take care of that and of the few possible optimizations for nexthop in net-next.

Thanks,
 Nik
David Ahern Nov. 28, 2021, 9:35 p.m. UTC | #3 
On 11/28/21 1:14 PM, Nikolay Aleksandrov wrote:
> It duplicates a part of it but in a safe way because the fib6_nh could still be visible,
> while fib6_nh_release does it in a way that assumes it's not. I could re-use
> this helper in fib6_nh_release though, since it doesn't matter how the entries are
> freed there. I'm guessing that is what you meant?

yes, that is what I meant - it duplicates a chunk of the code in
fib6_nh_release.

> I'll take care of that and of the few possible optimizations for nexthop in net-next.

thanks!
diff mbox series

Patch

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index c412dde4d67d..83b8070d1cc9 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -485,6 +485,7 @@  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);
 void fib6_nh_release(struct fib6_nh *fib6_nh);
+void fib6_nh_release_dsts(struct fib6_nh *fib6_nh);
 
 int call_fib6_entry_notifiers(struct net *net,
 			      enum fib_event_type event_type,
diff --git a/include/net/ipv6_stubs.h b/include/net/ipv6_stubs.h
index afbce90c4480..45e0339be6fa 100644
--- a/include/net/ipv6_stubs.h
+++ b/include/net/ipv6_stubs.h
@@ -47,6 +47,7 @@  struct ipv6_stub {
 			    struct fib6_config *cfg, gfp_t gfp_flags,
 			    struct netlink_ext_ack *extack);
 	void (*fib6_nh_release)(struct fib6_nh *fib6_nh);
+	void (*fib6_nh_release_dsts)(struct fib6_nh *fib6_nh);
 	void (*fib6_update_sernum)(struct net *net, struct fib6_info *rt);
 	int (*ip6_del_rt)(struct net *net, struct fib6_info *rt, bool skip_notify);
 	void (*fib6_rt_update)(struct net *net, struct fib6_info *rt,
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 0c4da163535a..dab4a047590b 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -1026,6 +1026,7 @@  static const struct ipv6_stub ipv6_stub_impl = {
 	.ip6_mtu_from_fib6 = ip6_mtu_from_fib6,
 	.fib6_nh_init	   = fib6_nh_init,
 	.fib6_nh_release   = fib6_nh_release,
+	.fib6_nh_release_dsts = fib6_nh_release_dsts,
 	.fib6_update_sernum = fib6_update_sernum_stub,
 	.fib6_rt_update	   = fib6_rt_update,
 	.ip6_del_rt	   = ip6_del_rt,
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 3ae25b8ffbd6..42d60c76d30a 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3680,6 +3680,25 @@  void fib6_nh_release(struct fib6_nh *fib6_nh)
 	fib_nh_common_release(&fib6_nh->nh_common);
 }
 
+void fib6_nh_release_dsts(struct fib6_nh *fib6_nh)
+{
+	int cpu;
+
+	if (!fib6_nh->rt6i_pcpu)
+		return;
+
+	for_each_possible_cpu(cpu) {
+		struct rt6_info *pcpu_rt, **ppcpu_rt;
+
+		ppcpu_rt = per_cpu_ptr(fib6_nh->rt6i_pcpu, cpu);
+		pcpu_rt = xchg(ppcpu_rt, NULL);
+		if (pcpu_rt) {
+			dst_dev_put(&pcpu_rt->dst);
+			dst_release(&pcpu_rt->dst);
+		}
+	}
+}
+
 static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
 					      gfp_t gfp_flags,
 					      struct netlink_ext_ack *extack)