* [PATCH net v2] ipv6: add rcu grace period before freeing fib6_node
@ 2017-08-20 0:34 Wei Wang
2017-08-20 4:33 ` Martin KaFai Lau
2017-08-21 2:59 ` David Miller
0 siblings, 2 replies; 4+ messages in thread
From: Wei Wang @ 2017-08-20 0:34 UTC (permalink / raw)
To: David Miller, netdev; +Cc: Eric Dumazet, Martin KaFai Lau, Wei Wang
From: Wei Wang <weiwan@google.com>
We currently keep rt->rt6i_node pointing to the fib6_node for the route.
And some functions make use of this pointer to dereference the fib6_node
from rt structure, e.g. rt6_check(). However, as there is neither
refcount nor rcu taken when dereferencing rt->rt6i_node, it could
potentially cause crashes as rt->rt6i_node could be set to NULL by other
CPUs when doing a route deletion.
This patch introduces an rcu grace period before freeing fib6_node and
makes sure the functions that dereference it takes rcu_read_lock().
Note: there is no "Fixes" tag because this bug was there in a very
early stage.
Signed-off-by: Wei Wang <weiwan@google.com>
Acked-by: Eric Dumazet <edumazet@google.com>
---
v2: removed one extra empty line
include/net/ip6_fib.h | 30 +++++++++++++++++++++++++++++-
net/ipv6/ip6_fib.c | 20 ++++++++++++++++----
net/ipv6/route.c | 14 +++++++++++---
3 files changed, 56 insertions(+), 8 deletions(-)
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 71c1646298ae..977a86e3a8d9 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -72,6 +72,7 @@ struct fib6_node {
__u16 fn_flags;
int fn_sernum;
struct rt6_info *rr_ptr;
+ struct rcu_head rcu;
};
#ifndef CONFIG_IPV6_SUBTREES
@@ -171,13 +172,40 @@ static inline void rt6_update_expires(struct rt6_info *rt0, int timeout)
rt0->rt6i_flags |= RTF_EXPIRES;
}
+/* Function to safely get fn->sernum for passed in rt
+ * and store result in passed in cookie.
+ * Return true if we can get cookie safely
+ * Return false if not
+ */
+static inline bool rt6_get_cookie_safe(const struct rt6_info *rt,
+ u32 *cookie)
+{
+ struct fib6_node *fn;
+ bool status = false;
+
+ rcu_read_lock();
+ fn = rcu_dereference(rt->rt6i_node);
+
+ if (fn) {
+ *cookie = fn->fn_sernum;
+ status = true;
+ }
+
+ rcu_read_unlock();
+ return status;
+}
+
static inline u32 rt6_get_cookie(const struct rt6_info *rt)
{
+ u32 cookie = 0;
+
if (rt->rt6i_flags & RTF_PCPU ||
(unlikely(!list_empty(&rt->rt6i_uncached)) && rt->dst.from))
rt = (struct rt6_info *)(rt->dst.from);
- return rt->rt6i_node ? rt->rt6i_node->fn_sernum : 0;
+ rt6_get_cookie_safe(rt, &cookie);
+
+ return cookie;
}
static inline void ip6_rt_put(struct rt6_info *rt)
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 549aacc3cb2c..a9821c230e4e 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -149,11 +149,23 @@ static struct fib6_node *node_alloc(void)
return fn;
}
-static void node_free(struct fib6_node *fn)
+static void node_free_immediate(struct fib6_node *fn)
+{
+ kmem_cache_free(fib6_node_kmem, fn);
+}
+
+static void node_free_rcu(struct rcu_head *head)
{
+ struct fib6_node *fn = container_of(head, struct fib6_node, rcu);
+
kmem_cache_free(fib6_node_kmem, fn);
}
+static void node_free(struct fib6_node *fn)
+{
+ call_rcu(&fn->rcu, node_free_rcu);
+}
+
void rt6_free_pcpu(struct rt6_info *non_pcpu_rt)
{
int cpu;
@@ -697,9 +709,9 @@ static struct fib6_node *fib6_add_1(struct fib6_node *root,
if (!in || !ln) {
if (in)
- node_free(in);
+ node_free_immediate(in);
if (ln)
- node_free(ln);
+ node_free_immediate(ln);
return ERR_PTR(-ENOMEM);
}
@@ -1138,7 +1150,7 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt,
root, and then (in failure) stale node
in main tree.
*/
- node_free(sfn);
+ node_free_immediate(sfn);
err = PTR_ERR(sn);
goto failure;
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index bec12ae3e6b7..4de2d793c4b8 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1289,7 +1289,9 @@ static void rt6_dst_from_metrics_check(struct rt6_info *rt)
static struct dst_entry *rt6_check(struct rt6_info *rt, u32 cookie)
{
- if (!rt->rt6i_node || (rt->rt6i_node->fn_sernum != cookie))
+ u32 rt_cookie;
+
+ if (!rt6_get_cookie_safe(rt, &rt_cookie) || rt_cookie != cookie)
return NULL;
if (rt6_check_expired(rt))
@@ -1357,8 +1359,14 @@ static void ip6_link_failure(struct sk_buff *skb)
if (rt->rt6i_flags & RTF_CACHE) {
if (dst_hold_safe(&rt->dst))
ip6_del_rt(rt);
- } else if (rt->rt6i_node && (rt->rt6i_flags & RTF_DEFAULT)) {
- rt->rt6i_node->fn_sernum = -1;
+ } else {
+ struct fib6_node *fn;
+
+ rcu_read_lock();
+ fn = rcu_dereference(rt->rt6i_node);
+ if (fn && (rt->rt6i_flags & RTF_DEFAULT))
+ fn->fn_sernum = -1;
+ rcu_read_unlock();
}
}
}
--
2.14.1.480.gb18f417b89-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net v2] ipv6: add rcu grace period before freeing fib6_node
2017-08-20 0:34 [PATCH net v2] ipv6: add rcu grace period before freeing fib6_node Wei Wang
@ 2017-08-20 4:33 ` Martin KaFai Lau
2017-08-21 2:59 ` David Miller
1 sibling, 0 replies; 4+ messages in thread
From: Martin KaFai Lau @ 2017-08-20 4:33 UTC (permalink / raw)
To: Wei Wang; +Cc: David Miller, netdev, Eric Dumazet
On Sat, Aug 19, 2017 at 05:34:08PM -0700, Wei Wang wrote:
> From: Wei Wang <weiwan@google.com>
>
> We currently keep rt->rt6i_node pointing to the fib6_node for the route.
> And some functions make use of this pointer to dereference the fib6_node
> from rt structure, e.g. rt6_check(). However, as there is neither
> refcount nor rcu taken when dereferencing rt->rt6i_node, it could
> potentially cause crashes as rt->rt6i_node could be set to NULL by other
> CPUs when doing a route deletion.
> This patch introduces an rcu grace period before freeing fib6_node and
> makes sure the functions that dereference it takes rcu_read_lock().
>
> Note: there is no "Fixes" tag because this bug was there in a very
> early stage.
>
> Signed-off-by: Wei Wang <weiwan@google.com>
> Acked-by: Eric Dumazet <edumazet@google.com>
Acked-by: Martin KaFai Lau <kafai@fb.com>
> ---
> v2: removed one extra empty line
>
> include/net/ip6_fib.h | 30 +++++++++++++++++++++++++++++-
> net/ipv6/ip6_fib.c | 20 ++++++++++++++++----
> net/ipv6/route.c | 14 +++++++++++---
> 3 files changed, 56 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
> index 71c1646298ae..977a86e3a8d9 100644
> --- a/include/net/ip6_fib.h
> +++ b/include/net/ip6_fib.h
> @@ -72,6 +72,7 @@ struct fib6_node {
> __u16 fn_flags;
> int fn_sernum;
> struct rt6_info *rr_ptr;
> + struct rcu_head rcu;
> };
>
> #ifndef CONFIG_IPV6_SUBTREES
> @@ -171,13 +172,40 @@ static inline void rt6_update_expires(struct rt6_info *rt0, int timeout)
> rt0->rt6i_flags |= RTF_EXPIRES;
> }
>
> +/* Function to safely get fn->sernum for passed in rt
> + * and store result in passed in cookie.
> + * Return true if we can get cookie safely
> + * Return false if not
> + */
> +static inline bool rt6_get_cookie_safe(const struct rt6_info *rt,
> + u32 *cookie)
> +{
> + struct fib6_node *fn;
> + bool status = false;
> +
> + rcu_read_lock();
> + fn = rcu_dereference(rt->rt6i_node);
> +
> + if (fn) {
> + *cookie = fn->fn_sernum;
> + status = true;
> + }
> +
> + rcu_read_unlock();
> + return status;
> +}
> +
> static inline u32 rt6_get_cookie(const struct rt6_info *rt)
> {
> + u32 cookie = 0;
> +
> if (rt->rt6i_flags & RTF_PCPU ||
> (unlikely(!list_empty(&rt->rt6i_uncached)) && rt->dst.from))
> rt = (struct rt6_info *)(rt->dst.from);
>
> - return rt->rt6i_node ? rt->rt6i_node->fn_sernum : 0;
> + rt6_get_cookie_safe(rt, &cookie);
> +
> + return cookie;
> }
>
> static inline void ip6_rt_put(struct rt6_info *rt)
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 549aacc3cb2c..a9821c230e4e 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -149,11 +149,23 @@ static struct fib6_node *node_alloc(void)
> return fn;
> }
>
> -static void node_free(struct fib6_node *fn)
> +static void node_free_immediate(struct fib6_node *fn)
> +{
> + kmem_cache_free(fib6_node_kmem, fn);
> +}
> +
> +static void node_free_rcu(struct rcu_head *head)
> {
> + struct fib6_node *fn = container_of(head, struct fib6_node, rcu);
> +
> kmem_cache_free(fib6_node_kmem, fn);
> }
>
> +static void node_free(struct fib6_node *fn)
> +{
> + call_rcu(&fn->rcu, node_free_rcu);
> +}
> +
> void rt6_free_pcpu(struct rt6_info *non_pcpu_rt)
> {
> int cpu;
> @@ -697,9 +709,9 @@ static struct fib6_node *fib6_add_1(struct fib6_node *root,
>
> if (!in || !ln) {
> if (in)
> - node_free(in);
> + node_free_immediate(in);
> if (ln)
> - node_free(ln);
> + node_free_immediate(ln);
> return ERR_PTR(-ENOMEM);
> }
>
> @@ -1138,7 +1150,7 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt,
> root, and then (in failure) stale node
> in main tree.
> */
> - node_free(sfn);
> + node_free_immediate(sfn);
> err = PTR_ERR(sn);
> goto failure;
> }
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index bec12ae3e6b7..4de2d793c4b8 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1289,7 +1289,9 @@ static void rt6_dst_from_metrics_check(struct rt6_info *rt)
>
> static struct dst_entry *rt6_check(struct rt6_info *rt, u32 cookie)
> {
> - if (!rt->rt6i_node || (rt->rt6i_node->fn_sernum != cookie))
> + u32 rt_cookie;
> +
> + if (!rt6_get_cookie_safe(rt, &rt_cookie) || rt_cookie != cookie)
> return NULL;
>
> if (rt6_check_expired(rt))
> @@ -1357,8 +1359,14 @@ static void ip6_link_failure(struct sk_buff *skb)
> if (rt->rt6i_flags & RTF_CACHE) {
> if (dst_hold_safe(&rt->dst))
> ip6_del_rt(rt);
> - } else if (rt->rt6i_node && (rt->rt6i_flags & RTF_DEFAULT)) {
> - rt->rt6i_node->fn_sernum = -1;
> + } else {
> + struct fib6_node *fn;
> +
> + rcu_read_lock();
> + fn = rcu_dereference(rt->rt6i_node);
> + if (fn && (rt->rt6i_flags & RTF_DEFAULT))
> + fn->fn_sernum = -1;
> + rcu_read_unlock();
> }
> }
> }
> --
> 2.14.1.480.gb18f417b89-goog
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net v2] ipv6: add rcu grace period before freeing fib6_node
2017-08-20 0:34 [PATCH net v2] ipv6: add rcu grace period before freeing fib6_node Wei Wang
2017-08-20 4:33 ` Martin KaFai Lau
@ 2017-08-21 2:59 ` David Miller
2017-08-21 16:19 ` Wei Wang
1 sibling, 1 reply; 4+ messages in thread
From: David Miller @ 2017-08-21 2:59 UTC (permalink / raw)
To: weiwan; +Cc: netdev, edumazet, kafai
From: Wei Wang <weiwan@google.com>
Date: Sat, 19 Aug 2017 17:34:08 -0700
> From: Wei Wang <weiwan@google.com>
>
> We currently keep rt->rt6i_node pointing to the fib6_node for the route.
> And some functions make use of this pointer to dereference the fib6_node
> from rt structure, e.g. rt6_check(). However, as there is neither
> refcount nor rcu taken when dereferencing rt->rt6i_node, it could
> potentially cause crashes as rt->rt6i_node could be set to NULL by other
> CPUs when doing a route deletion.
> This patch introduces an rcu grace period before freeing fib6_node and
> makes sure the functions that dereference it takes rcu_read_lock().
>
> Note: there is no "Fixes" tag because this bug was there in a very
> early stage.
>
> Signed-off-by: Wei Wang <weiwan@google.com>
> Acked-by: Eric Dumazet <edumazet@google.com>
> ---
> v2: removed one extra empty line
Goodness.... where to start.
If this bug has been around forever, why did you make this patch
against net-next instead of net? (I can tell just by looking at
the patch because rt6_free_pcpu() is static in 'net' yet it is
not static in the diff hunk which matches net-next)
And if you made it against net-next, why are you saying "net" in
your subject line instead of "[PATCH net-next v2]"?
Please sort this out properly, and resubmit.
Thank you.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net v2] ipv6: add rcu grace period before freeing fib6_node
2017-08-21 2:59 ` David Miller
@ 2017-08-21 16:19 ` Wei Wang
0 siblings, 0 replies; 4+ messages in thread
From: Wei Wang @ 2017-08-21 16:19 UTC (permalink / raw)
To: David Miller
Cc: Linux Kernel Network Developers, Eric Dumazet, Martin KaFai Lau
> Goodness.... where to start.
>
> If this bug has been around forever, why did you make this patch
> against net-next instead of net? (I can tell just by looking at
> the patch because rt6_free_pcpu() is static in 'net' yet it is
> not static in the diff hunk which matches net-next)
>
> And if you made it against net-next, why are you saying "net" in
> your subject line instead of "[PATCH net-next v2]"?
>
> Please sort this out properly, and resubmit.
>
> Thank you.
It should be applied to "net". I will rebase my change and resubmit.
Sorry for the confusion.
Thanks.
Wei
On Sun, Aug 20, 2017 at 7:59 PM, David Miller <davem@davemloft.net> wrote:
> From: Wei Wang <weiwan@google.com>
> Date: Sat, 19 Aug 2017 17:34:08 -0700
>
>> From: Wei Wang <weiwan@google.com>
>>
>> We currently keep rt->rt6i_node pointing to the fib6_node for the route.
>> And some functions make use of this pointer to dereference the fib6_node
>> from rt structure, e.g. rt6_check(). However, as there is neither
>> refcount nor rcu taken when dereferencing rt->rt6i_node, it could
>> potentially cause crashes as rt->rt6i_node could be set to NULL by other
>> CPUs when doing a route deletion.
>> This patch introduces an rcu grace period before freeing fib6_node and
>> makes sure the functions that dereference it takes rcu_read_lock().
>>
>> Note: there is no "Fixes" tag because this bug was there in a very
>> early stage.
>>
>> Signed-off-by: Wei Wang <weiwan@google.com>
>> Acked-by: Eric Dumazet <edumazet@google.com>
>> ---
>> v2: removed one extra empty line
>
> Goodness.... where to start.
>
> If this bug has been around forever, why did you make this patch
> against net-next instead of net? (I can tell just by looking at
> the patch because rt6_free_pcpu() is static in 'net' yet it is
> not static in the diff hunk which matches net-next)
>
> And if you made it against net-next, why are you saying "net" in
> your subject line instead of "[PATCH net-next v2]"?
>
> Please sort this out properly, and resubmit.
>
> Thank you.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-08-21 16:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-20 0:34 [PATCH net v2] ipv6: add rcu grace period before freeing fib6_node Wei Wang
2017-08-20 4:33 ` Martin KaFai Lau
2017-08-21 2:59 ` David Miller
2017-08-21 16:19 ` Wei Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.