| Message ID | 1375726729.4457.45.camel@edumazet-glaptop |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 05 Aug 2013 11:18:49 -0700 > From: Eric Dumazet <edumazet@google.com> > > AddressSanitizer [1] dynamic checker pointed a potential > out of bound access in leaf_walk_rcu() > > We could allocate one more slot in tnode_new() to leave the prefetch() > in-place but it looks not worth the pain. > > Bug added in commit 82cfbb008572b ("[IPV4] fib_trie: iterator recode") > > [1] : > https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel > > Reported-by: Andrey Konovalov <andreyknvl@google.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> I question the validity of the prefetch anyways, even without the out-of-bounds concerns. Applied and queued up for -stable, thanks Eric. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 05 Aug 2013 11:18:49 -0700 Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > AddressSanitizer [1] dynamic checker pointed a potential > out of bound access in leaf_walk_rcu() > > We could allocate one more slot in tnode_new() to leave the prefetch() > in-place but it looks not worth the pain. > > Bug added in commit 82cfbb008572b ("[IPV4] fib_trie: iterator recode") > > [1] : > https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel > > Reported-by: Andrey Konovalov <andreyknvl@google.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Dmitry Vyukov <dvyukov@google.com> Isn't prefetch supposed to always be safe, even out of bounds; even prefetch(NULL). Although I really doubt prefetch helps in in this code anyway. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-08-05 at 15:41 -0700, Stephen Hemminger wrote: > On Mon, 05 Aug 2013 11:18:49 -0700 > Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > From: Eric Dumazet <edumazet@google.com> > > > > AddressSanitizer [1] dynamic checker pointed a potential > > out of bound access in leaf_walk_rcu() > > > > We could allocate one more slot in tnode_new() to leave the prefetch() > > in-place but it looks not worth the pain. > > > > Bug added in commit 82cfbb008572b ("[IPV4] fib_trie: iterator recode") > > > > [1] : > > https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel > > > > Reported-by: Andrey Konovalov <andreyknvl@google.com> > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Cc: Dmitry Vyukov <dvyukov@google.com> > > Isn't prefetch supposed to always be safe, even out of bounds; even prefetch(NULL). > Although I really doubt prefetch helps in in this code anyway. prefetch(...) was not the problem here. The problem was X = array[N] with N being >= size(array) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c index 108a1e9c..3df6d3e 100644 --- a/net/ipv4/fib_trie.c +++ b/net/ipv4/fib_trie.c @@ -71,7 +71,6 @@ #include <linux/init.h> #include <linux/list.h> #include <linux/slab.h> -#include <linux/prefetch.h> #include <linux/export.h> #include <net/net_namespace.h> #include <net/ip.h> @@ -1761,10 +1760,8 @@ static struct leaf *leaf_walk_rcu(struct tnode *p, struct rt_trie_node *c) if (!c) continue; - if (IS_LEAF(c)) { - prefetch(rcu_dereference_rtnl(p->child[idx])); + if (IS_LEAF(c)) return (struct leaf *) c; - } /* Rescan start scanning in new node */ p = (struct tnode *) c;