* [PATCH net] ipmr: fix error path when mr_table_alloc fails
@ 2018-06-04 11:55 Sabrina Dubroca
2018-06-04 21:25 ` David Miller
2018-06-05 7:29 ` kbuild test robot
0 siblings, 2 replies; 4+ messages in thread
From: Sabrina Dubroca @ 2018-06-04 11:55 UTC (permalink / raw)
To: netdev
Cc: Sabrina Dubroca, Eric Dumazet, Nikolay Aleksandrov, Yuval Mintz,
Ivan Vecera
commit 0bbbf0e7d0e7 ("ipmr, ip6mr: Unite creation of new mr_table")
refactored ipmr_new_table, so that it now returns NULL when
mr_table_alloc fails. Unfortunately, all callers of ipmr_new_table
expect an ERR_PTR. commit 66fb33254f45 ("ipmr: properly check
rhltable_init() return value") followed suit.
This can result in NULL deref, when ipmr_rules_exit calls
ipmr_free_table with NULL net->ipv4.mrt in the
!CONFIG_IP_MROUTE_MULTIPLE_TABLES version.
This patch makes mr_table_alloc return errors, and changes
ip6mr_new_table and its callers to return/expect error pointers as
well. It also removes the version of mr_table_alloc defined under
!CONFIG_IP_MROUTE_COMMON, since it is never used.
Fixes: 0bbbf0e7d0e7 ("ipmr, ip6mr: Unite creation of new mr_table")
Fixes: 66fb33254f45 ("ipmr: properly check rhltable_init() return value")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
include/linux/mroute_base.h | 10 ----------
net/ipv4/ipmr_base.c | 8 +++++---
net/ipv6/ip6mr.c | 19 +++++++++++++------
3 files changed, 18 insertions(+), 19 deletions(-)
diff --git a/include/linux/mroute_base.h b/include/linux/mroute_base.h
index d617fe45543e..d633f737b3c6 100644
--- a/include/linux/mroute_base.h
+++ b/include/linux/mroute_base.h
@@ -307,16 +307,6 @@ static inline void vif_device_init(struct vif_device *v,
{
}
-static inline void *
-mr_table_alloc(struct net *net, u32 id,
- struct mr_table_ops *ops,
- void (*expire_func)(struct timer_list *t),
- void (*table_set)(struct mr_table *mrt,
- struct net *net))
-{
- return NULL;
-}
-
static inline void *mr_mfc_find_parent(struct mr_table *mrt,
void *hasharg, int parent)
{
diff --git a/net/ipv4/ipmr_base.c b/net/ipv4/ipmr_base.c
index 30221701614c..cafb0506c8c9 100644
--- a/net/ipv4/ipmr_base.c
+++ b/net/ipv4/ipmr_base.c
@@ -35,17 +35,19 @@ mr_table_alloc(struct net *net, u32 id,
struct net *net))
{
struct mr_table *mrt;
+ int err;
mrt = kzalloc(sizeof(*mrt), GFP_KERNEL);
if (!mrt)
- return NULL;
+ return ERR_PTR(-ENOMEM);
mrt->id = id;
write_pnet(&mrt->net, net);
mrt->ops = *ops;
- if (rhltable_init(&mrt->mfc_hash, mrt->ops.rht_params)) {
+ err = rhltable_init(&mrt->mfc_hash, mrt->ops.rht_params);
+ if (err) {
kfree(mrt);
- return NULL;
+ return ERR_PTR(err);
}
INIT_LIST_HEAD(&mrt->mfc_cache_list);
INIT_LIST_HEAD(&mrt->mfc_unres_queue);
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 298fd8b6ed17..f9b801bd00f8 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -227,8 +227,8 @@ static int __net_init ip6mr_rules_init(struct net *net)
INIT_LIST_HEAD(&net->ipv6.mr6_tables);
mrt = ip6mr_new_table(net, RT6_TABLE_DFLT);
- if (!mrt) {
- err = -ENOMEM;
+ if (IS_ERR(mrt)) {
+ err = PTR_ERR(mrt);
goto err1;
}
@@ -301,8 +301,13 @@ static int ip6mr_fib_lookup(struct net *net, struct flowi6 *flp6,
static int __net_init ip6mr_rules_init(struct net *net)
{
- net->ipv6.mrt6 = ip6mr_new_table(net, RT6_TABLE_DFLT);
- return net->ipv6.mrt6 ? 0 : -ENOMEM;
+ struct mr_table *mrt;
+
+ mrt = ip6mr_new_table(net, RT6_TABLE_DFLT);
+ if (IS_ERR(mrt))
+ return PTR_ERR(mrt);
+ net->ipv6.mrt6 = mrt;
+ return 0;
}
static void __net_exit ip6mr_rules_exit(struct net *net)
@@ -1743,6 +1748,7 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, uns
#ifdef CONFIG_IPV6_MROUTE_MULTIPLE_TABLES
case MRT6_TABLE:
{
+ struct mr_table *mrt;
u32 v;
if (optlen != sizeof(u32))
@@ -1757,8 +1763,9 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, uns
rtnl_lock();
ret = 0;
- if (!ip6mr_new_table(net, v))
- ret = -ENOMEM;
+ mrt = ip6mr_new_table(net, v);
+ if (IS_ERR(mrt))
+ ret = PTR_ERR(mrt);
raw6_sk(sk)->ip6mr_table = v;
rtnl_unlock();
return ret;
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] ipmr: fix error path when mr_table_alloc fails
2018-06-04 11:55 [PATCH net] ipmr: fix error path when mr_table_alloc fails Sabrina Dubroca
@ 2018-06-04 21:25 ` David Miller
2018-06-05 10:17 ` Sabrina Dubroca
2018-06-05 7:29 ` kbuild test robot
1 sibling, 1 reply; 4+ messages in thread
From: David Miller @ 2018-06-04 21:25 UTC (permalink / raw)
To: sd; +Cc: netdev, edumazet, nikolay, yuvalm, ivecera
From: Sabrina Dubroca <sd@queasysnail.net>
Date: Mon, 4 Jun 2018 13:55:54 +0200
> commit 0bbbf0e7d0e7 ("ipmr, ip6mr: Unite creation of new mr_table")
> refactored ipmr_new_table, so that it now returns NULL when
> mr_table_alloc fails. Unfortunately, all callers of ipmr_new_table
> expect an ERR_PTR. commit 66fb33254f45 ("ipmr: properly check
> rhltable_init() return value") followed suit.
>
> This can result in NULL deref, when ipmr_rules_exit calls
> ipmr_free_table with NULL net->ipv4.mrt in the
> !CONFIG_IP_MROUTE_MULTIPLE_TABLES version.
>
> This patch makes mr_table_alloc return errors, and changes
> ip6mr_new_table and its callers to return/expect error pointers as
> well. It also removes the version of mr_table_alloc defined under
> !CONFIG_IP_MROUTE_COMMON, since it is never used.
>
> Fixes: 0bbbf0e7d0e7 ("ipmr, ip6mr: Unite creation of new mr_table")
> Fixes: 66fb33254f45 ("ipmr: properly check rhltable_init() return value")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
This adds a new warning with gcc-8.1.1 on Fedora 28
CC [M] net/ipv6/ip6mr.o
In file included from ./arch/x86/include/asm/current.h:5,
from ./include/linux/sched.h:12,
from ./include/linux/uaccess.h:5,
from net/ipv6/ip6mr.c:19:
net/ipv6/ip6mr.c: In function ‘ip6_mroute_setsockopt’:
./include/linux/compiler.h:177:26: warning: ‘mrt’ may be used uninitialized in this function [-Wmaybe-uninitialized]
case 8: *(__u64 *)res = *(volatile __u64 *)p; break; \
^
net/ipv6/ip6mr.c:1752:20: note: ‘mrt’ was declared here
struct mr_table *mrt;
^~~
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] ipmr: fix error path when mr_table_alloc fails
2018-06-04 11:55 [PATCH net] ipmr: fix error path when mr_table_alloc fails Sabrina Dubroca
2018-06-04 21:25 ` David Miller
@ 2018-06-05 7:29 ` kbuild test robot
1 sibling, 0 replies; 4+ messages in thread
From: kbuild test robot @ 2018-06-05 7:29 UTC (permalink / raw)
To: Sabrina Dubroca
Cc: kbuild-all, netdev, Sabrina Dubroca, Eric Dumazet,
Nikolay Aleksandrov, Yuval Mintz, Ivan Vecera
[-- Attachment #1: Type: text/plain, Size: 3404 bytes --]
Hi Sabrina,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net/master]
url: https://github.com/0day-ci/linux/commits/Sabrina-Dubroca/ipmr-fix-error-path-when-mr_table_alloc-fails/20180605-060837
config: x86_64-randconfig-x006-201822 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
In file included from arch/x86/include/asm/current.h:5:0,
from include/linux/sched.h:12,
from include/linux/uaccess.h:5,
from net/ipv6/ip6mr.c:19:
net/ipv6/ip6mr.c: In function 'ip6_mroute_setsockopt':
>> include/linux/compiler.h:177:26: warning: 'mrt' may be used uninitialized in this function [-Wmaybe-uninitialized]
case 8: *(__u64 *)res = *(volatile __u64 *)p; break; \
^
net/ipv6/ip6mr.c:1751:20: note: 'mrt' was declared here
struct mr_table *mrt;
^~~
--
In file included from arch/x86/include/asm/current.h:5:0,
from include/linux/sched.h:12,
from include/linux/uaccess.h:5,
from net//ipv6/ip6mr.c:19:
net//ipv6/ip6mr.c: In function 'ip6_mroute_setsockopt':
>> include/linux/compiler.h:177:26: warning: 'mrt' may be used uninitialized in this function [-Wmaybe-uninitialized]
case 8: *(__u64 *)res = *(volatile __u64 *)p; break; \
^
net//ipv6/ip6mr.c:1751:20: note: 'mrt' was declared here
struct mr_table *mrt;
^~~
vim +/mrt +177 include/linux/compiler.h
230fa253 Christian Borntraeger 2014-11-25 170
d976441f Andrey Ryabinin 2015-10-19 171 #define __READ_ONCE_SIZE \
d976441f Andrey Ryabinin 2015-10-19 172 ({ \
d976441f Andrey Ryabinin 2015-10-19 173 switch (size) { \
d976441f Andrey Ryabinin 2015-10-19 174 case 1: *(__u8 *)res = *(volatile __u8 *)p; break; \
d976441f Andrey Ryabinin 2015-10-19 175 case 2: *(__u16 *)res = *(volatile __u16 *)p; break; \
d976441f Andrey Ryabinin 2015-10-19 176 case 4: *(__u32 *)res = *(volatile __u32 *)p; break; \
d976441f Andrey Ryabinin 2015-10-19 @177 case 8: *(__u64 *)res = *(volatile __u64 *)p; break; \
d976441f Andrey Ryabinin 2015-10-19 178 default: \
d976441f Andrey Ryabinin 2015-10-19 179 barrier(); \
d976441f Andrey Ryabinin 2015-10-19 180 __builtin_memcpy((void *)res, (const void *)p, size); \
d976441f Andrey Ryabinin 2015-10-19 181 barrier(); \
d976441f Andrey Ryabinin 2015-10-19 182 } \
d976441f Andrey Ryabinin 2015-10-19 183 })
d976441f Andrey Ryabinin 2015-10-19 184
:::::: The code at line 177 was first introduced by commit
:::::: d976441f44bc5d48635d081d277aa76556ffbf8b compiler, atomics, kasan: Provide READ_ONCE_NOCHECK()
:::::: TO: Andrey Ryabinin <aryabinin@virtuozzo.com>
:::::: CC: Ingo Molnar <mingo@kernel.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29370 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] ipmr: fix error path when mr_table_alloc fails
2018-06-04 21:25 ` David Miller
@ 2018-06-05 10:17 ` Sabrina Dubroca
0 siblings, 0 replies; 4+ messages in thread
From: Sabrina Dubroca @ 2018-06-05 10:17 UTC (permalink / raw)
To: David Miller; +Cc: netdev, edumazet, nikolay, yuvalm, ivecera
2018-06-04, 17:25:14 -0400, David Miller wrote:
> From: Sabrina Dubroca <sd@queasysnail.net>
> Date: Mon, 4 Jun 2018 13:55:54 +0200
>
> > commit 0bbbf0e7d0e7 ("ipmr, ip6mr: Unite creation of new mr_table")
> > refactored ipmr_new_table, so that it now returns NULL when
> > mr_table_alloc fails. Unfortunately, all callers of ipmr_new_table
> > expect an ERR_PTR. commit 66fb33254f45 ("ipmr: properly check
> > rhltable_init() return value") followed suit.
> >
> > This can result in NULL deref, when ipmr_rules_exit calls
> > ipmr_free_table with NULL net->ipv4.mrt in the
> > !CONFIG_IP_MROUTE_MULTIPLE_TABLES version.
> >
> > This patch makes mr_table_alloc return errors, and changes
> > ip6mr_new_table and its callers to return/expect error pointers as
> > well. It also removes the version of mr_table_alloc defined under
> > !CONFIG_IP_MROUTE_COMMON, since it is never used.
> >
> > Fixes: 0bbbf0e7d0e7 ("ipmr, ip6mr: Unite creation of new mr_table")
> > Fixes: 66fb33254f45 ("ipmr: properly check rhltable_init() return value")
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
>
> This adds a new warning with gcc-8.1.1 on Fedora 28
>
> CC [M] net/ipv6/ip6mr.o
> In file included from ./arch/x86/include/asm/current.h:5,
> from ./include/linux/sched.h:12,
> from ./include/linux/uaccess.h:5,
> from net/ipv6/ip6mr.c:19:
> net/ipv6/ip6mr.c: In function ‘ip6_mroute_setsockopt’:
> ./include/linux/compiler.h:177:26: warning: ‘mrt’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> case 8: *(__u64 *)res = *(volatile __u64 *)p; break; \
> ^
> net/ipv6/ip6mr.c:1752:20: note: ‘mrt’ was declared here
> struct mr_table *mrt;
> ^~~
grmbl, CONFIG_UBSAN disables -Wmaybe-uninitialized. I'll prepare a v2,
sorry.
--
Sabrina
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-06-05 10:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-04 11:55 [PATCH net] ipmr: fix error path when mr_table_alloc fails Sabrina Dubroca
2018-06-04 21:25 ` David Miller
2018-06-05 10:17 ` Sabrina Dubroca
2018-06-05 7:29 ` kbuild test robot
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.