* [PATCH net] net: sched: red: avoid hashing NULL child
@ 2018-05-18 12:51 Paolo Abeni
2018-05-18 14:22 ` Jiri Kosina
2018-05-18 17:53 ` David Miller
0 siblings, 2 replies; 3+ messages in thread
From: Paolo Abeni @ 2018-05-18 12:51 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Hangbin Liu, Jiri Kosina
Hangbin reported an Oops triggered by the syzkaller qdisc rules:
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] SMP KASAN PTI
Modules linked in: sch_red
CPU: 0 PID: 28699 Comm: syz-executor5 Not tainted 4.17.0-rc4.kcov #1
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
RIP: 0010:qdisc_hash_add+0x26/0xa0
RSP: 0018:ffff8800589cf470 EFLAGS: 00010203
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffffff824ad971
RDX: 0000000000000007 RSI: ffffc9000ce9f000 RDI: 000000000000003c
RBP: 0000000000000001 R08: ffffed000b139ea2 R09: ffff8800589cf4f0
R10: ffff8800589cf50f R11: ffffed000b139ea2 R12: ffff880054019fc0
R13: ffff880054019fb4 R14: ffff88005c0af600 R15: ffff880054019fb0
FS: 00007fa6edcb1700(0000) GS:ffff88005ce00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000740 CR3: 000000000fc16000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
red_change+0x2d2/0xed0 [sch_red]
qdisc_create+0x57e/0xef0
tc_modify_qdisc+0x47f/0x14e0
rtnetlink_rcv_msg+0x6a8/0x920
netlink_rcv_skb+0x2a2/0x3c0
netlink_unicast+0x511/0x740
netlink_sendmsg+0x825/0xc30
sock_sendmsg+0xc5/0x100
___sys_sendmsg+0x778/0x8e0
__sys_sendmsg+0xf5/0x1b0
do_syscall_64+0xbd/0x3b0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x450869
RSP: 002b:00007fa6edcb0c48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00007fa6edcb16b4 RCX: 0000000000450869
RDX: 0000000000000000 RSI: 00000000200000c0 RDI: 0000000000000013
RBP: 000000000072bea0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
R13: 0000000000008778 R14: 0000000000702838 R15: 00007fa6edcb1700
Code: e9 0b fe ff ff 0f 1f 44 00 00 55 53 48 89 fb 89 f5 e8 3f 07 f3 fe 48 8d 7b 3c 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 04 84 d2 75 51
RIP: qdisc_hash_add+0x26/0xa0 RSP: ffff8800589cf470
When a red qdisc is updated with a 0 limit, the child qdisc is left
unmodified, no additional scheduler is created in red_change(),
the 'child' local variable is rightfully NULL and must not add it
to the hash table.
This change addresses the above issue moving qdisc_hash_add() right
after the child qdisc creation. It additionally removes unneeded checks
for noop_qdisc.
Reported-by: Hangbin Liu <liuhangbin@gmail.com>
Fixes: 49b499718fa1 ("net: sched: make default fifo qdiscs appear in the dump")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/sched/sch_red.c | 5 +++--
net/sched/sch_tbf.c | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index 16644b3d2362..56c181c3feeb 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -222,10 +222,11 @@ static int red_change(struct Qdisc *sch, struct nlattr *opt,
extack);
if (IS_ERR(child))
return PTR_ERR(child);
- }
- if (child != &noop_qdisc)
+ /* child is fifo, no need to check for noop_qdisc */
qdisc_hash_add(child, true);
+ }
+
sch_tree_lock(sch);
q->flags = ctl->flags;
q->limit = ctl->limit;
diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index 03225a8df973..6f74a426f159 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -383,6 +383,9 @@ static int tbf_change(struct Qdisc *sch, struct nlattr *opt,
err = PTR_ERR(child);
goto done;
}
+
+ /* child is fifo, no need to check for noop_qdisc */
+ qdisc_hash_add(child, true);
}
sch_tree_lock(sch);
@@ -391,8 +394,6 @@ static int tbf_change(struct Qdisc *sch, struct nlattr *opt,
q->qdisc->qstats.backlog);
qdisc_destroy(q->qdisc);
q->qdisc = child;
- if (child != &noop_qdisc)
- qdisc_hash_add(child, true);
}
q->limit = qopt->limit;
if (tb[TCA_TBF_PBURST])
--
2.14.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] net: sched: red: avoid hashing NULL child
2018-05-18 12:51 [PATCH net] net: sched: red: avoid hashing NULL child Paolo Abeni
@ 2018-05-18 14:22 ` Jiri Kosina
2018-05-18 17:53 ` David Miller
1 sibling, 0 replies; 3+ messages in thread
From: Jiri Kosina @ 2018-05-18 14:22 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Hangbin Liu
On Fri, 18 May 2018, Paolo Abeni wrote:
> Hangbin reported an Oops triggered by the syzkaller qdisc rules:
>
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: 0000 [#1] SMP KASAN PTI
> Modules linked in: sch_red
> CPU: 0 PID: 28699 Comm: syz-executor5 Not tainted 4.17.0-rc4.kcov #1
> Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> RIP: 0010:qdisc_hash_add+0x26/0xa0
> RSP: 0018:ffff8800589cf470 EFLAGS: 00010203
> RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffffff824ad971
> RDX: 0000000000000007 RSI: ffffc9000ce9f000 RDI: 000000000000003c
> RBP: 0000000000000001 R08: ffffed000b139ea2 R09: ffff8800589cf4f0
> R10: ffff8800589cf50f R11: ffffed000b139ea2 R12: ffff880054019fc0
> R13: ffff880054019fb4 R14: ffff88005c0af600 R15: ffff880054019fb0
> FS: 00007fa6edcb1700(0000) GS:ffff88005ce00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000020000740 CR3: 000000000fc16000 CR4: 00000000000006f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> red_change+0x2d2/0xed0 [sch_red]
> qdisc_create+0x57e/0xef0
> tc_modify_qdisc+0x47f/0x14e0
> rtnetlink_rcv_msg+0x6a8/0x920
> netlink_rcv_skb+0x2a2/0x3c0
> netlink_unicast+0x511/0x740
> netlink_sendmsg+0x825/0xc30
> sock_sendmsg+0xc5/0x100
> ___sys_sendmsg+0x778/0x8e0
> __sys_sendmsg+0xf5/0x1b0
> do_syscall_64+0xbd/0x3b0
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x450869
> RSP: 002b:00007fa6edcb0c48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: 00007fa6edcb16b4 RCX: 0000000000450869
> RDX: 0000000000000000 RSI: 00000000200000c0 RDI: 0000000000000013
> RBP: 000000000072bea0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
> R13: 0000000000008778 R14: 0000000000702838 R15: 00007fa6edcb1700
> Code: e9 0b fe ff ff 0f 1f 44 00 00 55 53 48 89 fb 89 f5 e8 3f 07 f3 fe 48 8d 7b 3c 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 04 84 d2 75 51
> RIP: qdisc_hash_add+0x26/0xa0 RSP: ffff8800589cf470
>
> When a red qdisc is updated with a 0 limit, the child qdisc is left
> unmodified, no additional scheduler is created in red_change(),
> the 'child' local variable is rightfully NULL and must not add it
> to the hash table.
>
> This change addresses the above issue moving qdisc_hash_add() right
> after the child qdisc creation. It additionally removes unneeded checks
> for noop_qdisc.
>
> Reported-by: Hangbin Liu <liuhangbin@gmail.com>
> Fixes: 49b499718fa1 ("net: sched: make default fifo qdiscs appear in the dump")
Acked-by: Jiri Kosina <jkosina@suse.cz>
Thanks for fixing this Paolo.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] net: sched: red: avoid hashing NULL child
2018-05-18 12:51 [PATCH net] net: sched: red: avoid hashing NULL child Paolo Abeni
2018-05-18 14:22 ` Jiri Kosina
@ 2018-05-18 17:53 ` David Miller
1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2018-05-18 17:53 UTC (permalink / raw)
To: pabeni; +Cc: netdev, jhs, xiyou.wangcong, jiri, liuhangbin, jkosina
From: Paolo Abeni <pabeni@redhat.com>
Date: Fri, 18 May 2018 14:51:44 +0200
> Hangbin reported an Oops triggered by the syzkaller qdisc rules:
...
> When a red qdisc is updated with a 0 limit, the child qdisc is left
> unmodified, no additional scheduler is created in red_change(),
> the 'child' local variable is rightfully NULL and must not add it
> to the hash table.
>
> This change addresses the above issue moving qdisc_hash_add() right
> after the child qdisc creation. It additionally removes unneeded checks
> for noop_qdisc.
>
> Reported-by: Hangbin Liu <liuhangbin@gmail.com>
> Fixes: 49b499718fa1 ("net: sched: make default fifo qdiscs appear in the dump")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Applied and queued up for -stable, thanks Paolo.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-05-18 17:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-18 12:51 [PATCH net] net: sched: red: avoid hashing NULL child Paolo Abeni
2018-05-18 14:22 ` Jiri Kosina
2018-05-18 17:53 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).