* [BUG ?] ipv6: addrconf: Adds a missing in6_ifa_hold()
@ 2017-10-31 3:53 Eric Dumazet
2017-10-31 4:49 ` David Ahern
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2017-10-31 3:53 UTC (permalink / raw)
To: David Ahern; +Cc: netdev
David, I was looking at addrconf_permanent_addr() and wondered
if there is not some problem with it.
It seems we need to increment ifp refcount before calling
ipv6_del_addr()
Could you double check if this patch is needed, I am guessing you have a
test suite exercising this code path ?
Thanks.
PS : Presumably CONFIG_REFCOUNT_FULL=y should have warned you of the
problem.
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 4a96ebbf8eda5f59a6ff88e836d666a404d2bf0d..8a1c846d3df949a4638589f187120db22a3525ba 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3335,6 +3335,7 @@ static void addrconf_permanent_addr(struct net_device *dev)
if ((ifp->flags & IFA_F_PERMANENT) &&
fixup_permanent_addr(idev, ifp) < 0) {
write_unlock_bh(&idev->lock);
+ in6_ifa_hold(ifp);
ipv6_del_addr(ifp);
write_lock_bh(&idev->lock);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [BUG ?] ipv6: addrconf: Adds a missing in6_ifa_hold()
2017-10-31 3:53 [BUG ?] ipv6: addrconf: Adds a missing in6_ifa_hold() Eric Dumazet
@ 2017-10-31 4:49 ` David Ahern
2017-10-31 4:56 ` Eric Dumazet
2017-10-31 5:47 ` [PATCH net] ipv6: addrconf: increment ifp refcount before ipv6_del_addr() Eric Dumazet
0 siblings, 2 replies; 7+ messages in thread
From: David Ahern @ 2017-10-31 4:49 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
On 10/30/17 9:53 PM, Eric Dumazet wrote:
> David, I was looking at addrconf_permanent_addr() and wondered
> if there is not some problem with it.
>
> It seems we need to increment ifp refcount before calling
> ipv6_del_addr()
>
> Could you double check if this patch is needed, I am guessing you have a
> test suite exercising this code path ?
A lot has changed in 20 months since the patch that added the code. For
instance, taking down the 'lo' device no longer affects host routes on
other interfaces. Also, fixup_permanent_addr only fails on memory
allocation. Did you hit this with a test case because I do not have a
general one that causes the memory failure (hard coding a failure for an
address is the only way).
>
> Thanks.
>
> PS : Presumably CONFIG_REFCOUNT_FULL=y should have warned you of the
> problem.
I have not run a debug kernel in a while -- and did not have this option
set. Added it to my debug config.
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 4a96ebbf8eda5f59a6ff88e836d666a404d2bf0d..8a1c846d3df949a4638589f187120db22a3525ba 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -3335,6 +3335,7 @@ static void addrconf_permanent_addr(struct net_device *dev)
> if ((ifp->flags & IFA_F_PERMANENT) &&
> fixup_permanent_addr(idev, ifp) < 0) {
> write_unlock_bh(&idev->lock);
> + in6_ifa_hold(ifp);
> ipv6_del_addr(ifp);
> write_lock_bh(&idev->lock);
>
Yes, forcing a failure here does trigger refcnt warning, but then you
knew that. ;-)
PS. is the following a known failure? I triggered it looking into your
report
[ 170.385741] ======================================================
[ 170.387490] WARNING: possible circular locking dependency detected
[ 170.389214] 4.14.0-rc5+ #338 Not tainted
[ 170.390323] ------------------------------------------------------
[ 170.392017] swapper/0/0 is trying to acquire lock:
[ 170.393408] (slock-AF_INET){+.-.}, at: [<ffffffff8172848b>]
tcp_delack_timer+0x29/0xb1
[ 170.395622]
but task is already holding lock:
[ 170.396943] ((timer)){+.-.}, at: [<ffffffff810f3c53>]
call_timer_fn+0x5/0x36b
[ 170.397912]
which lock already depends on the new lock.
[ 170.398986]
the existing dependency chain (in reverse order) is:
[ 170.399965]
-> #1 ((timer)){+.-.}:
[ 170.400629] lock_acquire+0x154/0x220
[ 170.401198] del_timer_sync+0x47/0xbd
[ 170.401760] inet_csk_reqsk_queue_drop+0x109/0x141
[ 170.402464] inet_csk_complete_hashdance+0x3b/0x68
[ 170.403173] tcp_check_req+0x517/0x5f1
[ 170.403746] tcp_v4_rcv+0x6ad/0xce7
[ 170.404287] ip_local_deliver_finish+0x1d4/0x281
[ 170.404985] ip_local_deliver+0xaf/0xcf
[ 170.405571] ip_rcv_finish+0x632/0x6ff
[ 170.406140] ip_rcv+0x45d/0x4a6
...
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG ?] ipv6: addrconf: Adds a missing in6_ifa_hold()
2017-10-31 4:49 ` David Ahern
@ 2017-10-31 4:56 ` Eric Dumazet
2017-10-31 5:47 ` [PATCH net] ipv6: addrconf: increment ifp refcount before ipv6_del_addr() Eric Dumazet
1 sibling, 0 replies; 7+ messages in thread
From: Eric Dumazet @ 2017-10-31 4:56 UTC (permalink / raw)
To: David Ahern; +Cc: netdev
On Mon, 2017-10-30 at 22:49 -0600, David Ahern wrote:
> On 10/30/17 9:53 PM, Eric Dumazet wrote:
> > David, I was looking at addrconf_permanent_addr() and wondered
> > if there is not some problem with it.
> >
> > It seems we need to increment ifp refcount before calling
> > ipv6_del_addr()
> >
> > Could you double check if this patch is needed, I am guessing you have a
> > test suite exercising this code path ?
>
> A lot has changed in 20 months since the patch that added the code. For
> instance, taking down the 'lo' device no longer affects host routes on
> other interfaces. Also, fixup_permanent_addr only fails on memory
> allocation. Did you hit this with a test case because I do not have a
> general one that causes the memory failure (hard coding a failure for an
> address is the only way).
>
> >
> > Thanks.
> >
> > PS : Presumably CONFIG_REFCOUNT_FULL=y should have warned you of the
> > problem.
>
> I have not run a debug kernel in a while -- and did not have this option
> set. Added it to my debug config.
>
> >
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index 4a96ebbf8eda5f59a6ff88e836d666a404d2bf0d..8a1c846d3df949a4638589f187120db22a3525ba 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -3335,6 +3335,7 @@ static void addrconf_permanent_addr(struct net_device *dev)
> > if ((ifp->flags & IFA_F_PERMANENT) &&
> > fixup_permanent_addr(idev, ifp) < 0) {
> > write_unlock_bh(&idev->lock);
> > + in6_ifa_hold(ifp);
> > ipv6_del_addr(ifp);
> > write_lock_bh(&idev->lock);
> >
>
> Yes, forcing a failure here does trigger refcnt warning, but then you
> knew that. ;-)
>
Okay ;)
>
> PS. is the following a known failure? I triggered it looking into your
> report
>
> [ 170.385741] ======================================================
> [ 170.387490] WARNING: possible circular locking dependency detected
> [ 170.389214] 4.14.0-rc5+ #338 Not tainted
> [ 170.390323] ------------------------------------------------------
> [ 170.392017] swapper/0/0 is trying to acquire lock:
> [ 170.393408] (slock-AF_INET){+.-.}, at: [<ffffffff8172848b>]
> tcp_delack_timer+0x29/0xb1
> [ 170.395622]
> but task is already holding lock:
> [ 170.396943] ((timer)){+.-.}, at: [<ffffffff810f3c53>]
> call_timer_fn+0x5/0x36b
> [ 170.397912]
> which lock already depends on the new lock.
>
> [ 170.398986]
> the existing dependency chain (in reverse order) is:
> [ 170.399965]
> -> #1 ((timer)){+.-.}:
> [ 170.400629] lock_acquire+0x154/0x220
> [ 170.401198] del_timer_sync+0x47/0xbd
> [ 170.401760] inet_csk_reqsk_queue_drop+0x109/0x141
> [ 170.402464] inet_csk_complete_hashdance+0x3b/0x68
> [ 170.403173] tcp_check_req+0x517/0x5f1
> [ 170.403746] tcp_v4_rcv+0x6ad/0xce7
> [ 170.404287] ip_local_deliver_finish+0x1d4/0x281
> [ 170.404985] ip_local_deliver+0xaf/0xcf
> [ 170.405571] ip_rcv_finish+0x632/0x6ff
> [ 170.406140] ip_rcv+0x45d/0x4a6
> ...
Yeah, please look at
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=timers/core&id=52f737c2da40259ac9962170ce608b6fb1b55ee4
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net] ipv6: addrconf: increment ifp refcount before ipv6_del_addr()
2017-10-31 4:49 ` David Ahern
2017-10-31 4:56 ` Eric Dumazet
@ 2017-10-31 5:47 ` Eric Dumazet
2017-10-31 16:09 ` David Ahern
2017-11-01 12:20 ` David Miller
1 sibling, 2 replies; 7+ messages in thread
From: Eric Dumazet @ 2017-10-31 5:47 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, David Miller
From: Eric Dumazet <edumazet@google.com>
In the (unlikely) event fixup_permanent_addr() returns a failure,
addrconf_permanent_addr() calls ipv6_del_addr() without the
mandatory call to in6_ifa_hold(), leading to a refcount error,
spotted by syzkaller :
WARNING: CPU: 1 PID: 3142 at lib/refcount.c:227 refcount_dec+0x4c/0x50
lib/refcount.c:227
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 3142 Comm: ip Not tainted 4.14.0-rc4-next-20171009+ #33
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:16 [inline]
dump_stack+0x194/0x257 lib/dump_stack.c:52
panic+0x1e4/0x41c kernel/panic.c:181
__warn+0x1c4/0x1e0 kernel/panic.c:544
report_bug+0x211/0x2d0 lib/bug.c:183
fixup_bug+0x40/0x90 arch/x86/kernel/traps.c:178
do_trap_no_signal arch/x86/kernel/traps.c:212 [inline]
do_trap+0x260/0x390 arch/x86/kernel/traps.c:261
do_error_trap+0x120/0x390 arch/x86/kernel/traps.c:298
do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:311
invalid_op+0x18/0x20 arch/x86/entry/entry_64.S:905
RIP: 0010:refcount_dec+0x4c/0x50 lib/refcount.c:227
RSP: 0018:ffff8801ca49e680 EFLAGS: 00010286
RAX: 000000000000002c RBX: ffff8801d07cfcdc RCX: 0000000000000000
RDX: 000000000000002c RSI: 1ffff10039493c90 RDI: ffffed0039493cc4
RBP: ffff8801ca49e688 R08: ffff8801ca49dd70 R09: 0000000000000000
R10: ffff8801ca49df58 R11: 0000000000000000 R12: 1ffff10039493cd9
R13: ffff8801ca49e6e8 R14: ffff8801ca49e7e8 R15: ffff8801d07cfcdc
__in6_ifa_put include/net/addrconf.h:369 [inline]
ipv6_del_addr+0x42b/0xb60 net/ipv6/addrconf.c:1208
addrconf_permanent_addr net/ipv6/addrconf.c:3327 [inline]
addrconf_notify+0x1c66/0x2190 net/ipv6/addrconf.c:3393
notifier_call_chain+0x136/0x2c0 kernel/notifier.c:93
__raw_notifier_call_chain kernel/notifier.c:394 [inline]
raw_notifier_call_chain+0x2d/0x40 kernel/notifier.c:401
call_netdevice_notifiers_info+0x32/0x60 net/core/dev.c:1697
call_netdevice_notifiers net/core/dev.c:1715 [inline]
__dev_notify_flags+0x15d/0x430 net/core/dev.c:6843
dev_change_flags+0xf5/0x140 net/core/dev.c:6879
do_setlink+0xa1b/0x38e0 net/core/rtnetlink.c:2113
rtnl_newlink+0xf0d/0x1a40 net/core/rtnetlink.c:2661
rtnetlink_rcv_msg+0x733/0x1090 net/core/rtnetlink.c:4301
netlink_rcv_skb+0x216/0x440 net/netlink/af_netlink.c:2408
rtnetlink_rcv+0x1c/0x20 net/core/rtnetlink.c:4313
netlink_unicast_kernel net/netlink/af_netlink.c:1273 [inline]
netlink_unicast+0x4e8/0x6f0 net/netlink/af_netlink.c:1299
netlink_sendmsg+0xa4a/0xe70 net/netlink/af_netlink.c:1862
sock_sendmsg_nosec net/socket.c:633 [inline]
sock_sendmsg+0xca/0x110 net/socket.c:643
___sys_sendmsg+0x75b/0x8a0 net/socket.c:2049
__sys_sendmsg+0xe5/0x210 net/socket.c:2083
SYSC_sendmsg net/socket.c:2094 [inline]
SyS_sendmsg+0x2d/0x50 net/socket.c:2090
entry_SYSCALL_64_fastpath+0x1f/0xbe
RIP: 0033:0x7fa9174d3320
RSP: 002b:00007ffe302ae9e8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00007ffe302b2ae0 RCX: 00007fa9174d3320
RDX: 0000000000000000 RSI: 00007ffe302aea20 RDI: 0000000000000016
RBP: 0000000000000082 R08: 0000000000000000 R09: 000000000000000f
R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffe302b32a0
R13: 0000000000000000 R14: 00007ffe302b2ab8 R15: 00007ffe302b32b8
Fixes: f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: David Ahern <dsahern@gmail.com>
---
net/ipv6/addrconf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 4a96ebbf8eda5f59a6ff88e836d666a404d2bf0d..8a1c846d3df949a4638589f187120db22a3525ba 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3335,6 +3335,7 @@ static void addrconf_permanent_addr(struct net_device *dev)
if ((ifp->flags & IFA_F_PERMANENT) &&
fixup_permanent_addr(idev, ifp) < 0) {
write_unlock_bh(&idev->lock);
+ in6_ifa_hold(ifp);
ipv6_del_addr(ifp);
write_lock_bh(&idev->lock);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net] ipv6: addrconf: increment ifp refcount before ipv6_del_addr()
2017-10-31 5:47 ` [PATCH net] ipv6: addrconf: increment ifp refcount before ipv6_del_addr() Eric Dumazet
@ 2017-10-31 16:09 ` David Ahern
2017-10-31 16:33 ` Eric Dumazet
2017-11-01 12:20 ` David Miller
1 sibling, 1 reply; 7+ messages in thread
From: David Ahern @ 2017-10-31 16:09 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David Miller
On 10/30/17 11:47 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> In the (unlikely) event fixup_permanent_addr() returns a failure,
> addrconf_permanent_addr() calls ipv6_del_addr() without the
> mandatory call to in6_ifa_hold(), leading to a refcount error,
> spotted by syzkaller :
>
> WARNING: CPU: 1 PID: 3142 at lib/refcount.c:227 refcount_dec+0x4c/0x50
> lib/refcount.c:227
> Kernel panic - not syncing: panic_on_warn set ...
>
...
>
> Fixes: f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: David Ahern <dsahern@gmail.com>
> ---
> net/ipv6/addrconf.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 4a96ebbf8eda5f59a6ff88e836d666a404d2bf0d..8a1c846d3df949a4638589f187120db22a3525ba 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -3335,6 +3335,7 @@ static void addrconf_permanent_addr(struct net_device *dev)
> if ((ifp->flags & IFA_F_PERMANENT) &&
> fixup_permanent_addr(idev, ifp) < 0) {
> write_unlock_bh(&idev->lock);
> + in6_ifa_hold(ifp);
> ipv6_del_addr(ifp);
> write_lock_bh(&idev->lock);
>
>
>
Interestingly, I put the failure sequence into a loop and hit the above
path closing in on 1 million times and I am not seeing a memory leak
with our without this patch.
The above does fix the refcnt and based on other call sites for
ipv6_del_addr is the right thing to do, so...
Acked-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ipv6: addrconf: increment ifp refcount before ipv6_del_addr()
2017-10-31 16:09 ` David Ahern
@ 2017-10-31 16:33 ` Eric Dumazet
0 siblings, 0 replies; 7+ messages in thread
From: Eric Dumazet @ 2017-10-31 16:33 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, David Miller
On Tue, 2017-10-31 at 10:09 -0600, David Ahern wrote:
> Interestingly, I put the failure sequence into a loop and hit the above
> path closing in on 1 million times and I am not seeing a memory leak
> with our without this patch.
>
> The above does fix the refcnt and based on other call sites for
> ipv6_del_addr is the right thing to do, so...
>
> Acked-by: David Ahern <dsahern@gmail.com>
Thanks a lot for testing !
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ipv6: addrconf: increment ifp refcount before ipv6_del_addr()
2017-10-31 5:47 ` [PATCH net] ipv6: addrconf: increment ifp refcount before ipv6_del_addr() Eric Dumazet
2017-10-31 16:09 ` David Ahern
@ 2017-11-01 12:20 ` David Miller
1 sibling, 0 replies; 7+ messages in thread
From: David Miller @ 2017-11-01 12:20 UTC (permalink / raw)
To: eric.dumazet; +Cc: dsahern, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 30 Oct 2017 22:47:09 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> In the (unlikely) event fixup_permanent_addr() returns a failure,
> addrconf_permanent_addr() calls ipv6_del_addr() without the
> mandatory call to in6_ifa_hold(), leading to a refcount error,
> spotted by syzkaller :
...
> Fixes: f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: David Ahern <dsahern@gmail.com>
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-11-01 12:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-31 3:53 [BUG ?] ipv6: addrconf: Adds a missing in6_ifa_hold() Eric Dumazet
2017-10-31 4:49 ` David Ahern
2017-10-31 4:56 ` Eric Dumazet
2017-10-31 5:47 ` [PATCH net] ipv6: addrconf: increment ifp refcount before ipv6_del_addr() Eric Dumazet
2017-10-31 16:09 ` David Ahern
2017-10-31 16:33 ` Eric Dumazet
2017-11-01 12:20 ` David Miller
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.