* [PATCH net] sch_netem: acquire qdisc lock in netem_change()
@ 2023-06-20 18:44 Eric Dumazet
2023-06-21 11:21 ` Jamal Hadi Salim
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Eric Dumazet @ 2023-06-20 18:44 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: netdev, eric.dumazet, Eric Dumazet, syzbot, Stephen Hemminger,
Jamal Hadi Salim, Cong Wang, Jiri Pirko
syzbot managed to trigger a divide error [1] in netem.
It could happen if q->rate changes while netem_enqueue()
is running, since q->rate is read twice.
It turns out netem_change() always lacked proper synchronization.
[1]
divide error: 0000 [#1] SMP KASAN
CPU: 1 PID: 7867 Comm: syz-executor.1 Not tainted 6.1.30-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/25/2023
RIP: 0010:div64_u64 include/linux/math64.h:69 [inline]
RIP: 0010:packet_time_ns net/sched/sch_netem.c:357 [inline]
RIP: 0010:netem_enqueue+0x2067/0x36d0 net/sched/sch_netem.c:576
Code: 89 e2 48 69 da 00 ca 9a 3b 42 80 3c 28 00 4c 8b a4 24 88 00 00 00 74 0d 4c 89 e7 e8 c3 4f 3b fd 48 8b 4c 24 18 48 89 d8 31 d2 <49> f7 34 24 49 01 c7 4c 8b 64 24 48 4d 01 f7 4c 89 e3 48 c1 eb 03
RSP: 0018:ffffc9000dccea60 EFLAGS: 00010246
RAX: 000001a442624200 RBX: 000001a442624200 RCX: ffff888108a4f000
RDX: 0000000000000000 RSI: 000000000000070d RDI: 000000000000070d
RBP: ffffc9000dcceb90 R08: ffffffff849c5e26 R09: fffffbfff10e1297
R10: 0000000000000000 R11: dffffc0000000001 R12: ffff888108a4f358
R13: dffffc0000000000 R14: 0000001a8cd9a7ec R15: 0000000000000000
FS: 00007fa73fe18700(0000) GS:ffff8881f6b00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fa73fdf7718 CR3: 000000011d36e000 CR4: 0000000000350ee0
Call Trace:
<TASK>
[<ffffffff84714385>] __dev_xmit_skb net/core/dev.c:3931 [inline]
[<ffffffff84714385>] __dev_queue_xmit+0xcf5/0x3370 net/core/dev.c:4290
[<ffffffff84d22df2>] dev_queue_xmit include/linux/netdevice.h:3030 [inline]
[<ffffffff84d22df2>] neigh_hh_output include/net/neighbour.h:531 [inline]
[<ffffffff84d22df2>] neigh_output include/net/neighbour.h:545 [inline]
[<ffffffff84d22df2>] ip_finish_output2+0xb92/0x10d0 net/ipv4/ip_output.c:235
[<ffffffff84d21e63>] __ip_finish_output+0xc3/0x2b0
[<ffffffff84d10a81>] ip_finish_output+0x31/0x2a0 net/ipv4/ip_output.c:323
[<ffffffff84d10f14>] NF_HOOK_COND include/linux/netfilter.h:298 [inline]
[<ffffffff84d10f14>] ip_output+0x224/0x2a0 net/ipv4/ip_output.c:437
[<ffffffff84d123b5>] dst_output include/net/dst.h:444 [inline]
[<ffffffff84d123b5>] ip_local_out net/ipv4/ip_output.c:127 [inline]
[<ffffffff84d123b5>] __ip_queue_xmit+0x1425/0x2000 net/ipv4/ip_output.c:542
[<ffffffff84d12fdc>] ip_queue_xmit+0x4c/0x70 net/ipv4/ip_output.c:556
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
---
net/sched/sch_netem.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 6ef3021e1169a376494d610ac8c3f1c04fb911c5..e79be1b3e74da3c154f7ee23e16cc9e8da8f7106 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -966,6 +966,7 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
if (ret < 0)
return ret;
+ sch_tree_lock(sch);
/* backup q->clg and q->loss_model */
old_clg = q->clg;
old_loss_model = q->loss_model;
@@ -974,7 +975,7 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
ret = get_loss_clg(q, tb[TCA_NETEM_LOSS]);
if (ret) {
q->loss_model = old_loss_model;
- return ret;
+ goto unlock;
}
} else {
q->loss_model = CLG_RANDOM;
@@ -1041,6 +1042,8 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
/* capping jitter to the range acceptable by tabledist() */
q->jitter = min_t(s64, abs(q->jitter), INT_MAX);
+unlock:
+ sch_tree_unlock(sch);
return ret;
get_table_failure:
@@ -1050,7 +1053,8 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
*/
q->clg = old_clg;
q->loss_model = old_loss_model;
- return ret;
+
+ goto unlock;
}
static int netem_init(struct Qdisc *sch, struct nlattr *opt,
--
2.41.0.178.g377b9f9a00-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] sch_netem: acquire qdisc lock in netem_change()
2023-06-20 18:44 [PATCH net] sch_netem: acquire qdisc lock in netem_change() Eric Dumazet
@ 2023-06-21 11:21 ` Jamal Hadi Salim
2023-06-21 14:51 ` Simon Horman
2023-06-22 9:20 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 5+ messages in thread
From: Jamal Hadi Salim @ 2023-06-21 11:21 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Stephen Hemminger, Cong Wang, Jiri Pirko
On Tue, Jun 20, 2023 at 2:44 PM Eric Dumazet <edumazet@google.com> wrote:
>
> syzbot managed to trigger a divide error [1] in netem.
>
> It could happen if q->rate changes while netem_enqueue()
> is running, since q->rate is read twice.
>
> It turns out netem_change() always lacked proper synchronization.
>
> [1]
> divide error: 0000 [#1] SMP KASAN
> CPU: 1 PID: 7867 Comm: syz-executor.1 Not tainted 6.1.30-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/25/2023
> RIP: 0010:div64_u64 include/linux/math64.h:69 [inline]
> RIP: 0010:packet_time_ns net/sched/sch_netem.c:357 [inline]
> RIP: 0010:netem_enqueue+0x2067/0x36d0 net/sched/sch_netem.c:576
> Code: 89 e2 48 69 da 00 ca 9a 3b 42 80 3c 28 00 4c 8b a4 24 88 00 00 00 74 0d 4c 89 e7 e8 c3 4f 3b fd 48 8b 4c 24 18 48 89 d8 31 d2 <49> f7 34 24 49 01 c7 4c 8b 64 24 48 4d 01 f7 4c 89 e3 48 c1 eb 03
> RSP: 0018:ffffc9000dccea60 EFLAGS: 00010246
> RAX: 000001a442624200 RBX: 000001a442624200 RCX: ffff888108a4f000
> RDX: 0000000000000000 RSI: 000000000000070d RDI: 000000000000070d
> RBP: ffffc9000dcceb90 R08: ffffffff849c5e26 R09: fffffbfff10e1297
> R10: 0000000000000000 R11: dffffc0000000001 R12: ffff888108a4f358
> R13: dffffc0000000000 R14: 0000001a8cd9a7ec R15: 0000000000000000
> FS: 00007fa73fe18700(0000) GS:ffff8881f6b00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fa73fdf7718 CR3: 000000011d36e000 CR4: 0000000000350ee0
> Call Trace:
> <TASK>
> [<ffffffff84714385>] __dev_xmit_skb net/core/dev.c:3931 [inline]
> [<ffffffff84714385>] __dev_queue_xmit+0xcf5/0x3370 net/core/dev.c:4290
> [<ffffffff84d22df2>] dev_queue_xmit include/linux/netdevice.h:3030 [inline]
> [<ffffffff84d22df2>] neigh_hh_output include/net/neighbour.h:531 [inline]
> [<ffffffff84d22df2>] neigh_output include/net/neighbour.h:545 [inline]
> [<ffffffff84d22df2>] ip_finish_output2+0xb92/0x10d0 net/ipv4/ip_output.c:235
> [<ffffffff84d21e63>] __ip_finish_output+0xc3/0x2b0
> [<ffffffff84d10a81>] ip_finish_output+0x31/0x2a0 net/ipv4/ip_output.c:323
> [<ffffffff84d10f14>] NF_HOOK_COND include/linux/netfilter.h:298 [inline]
> [<ffffffff84d10f14>] ip_output+0x224/0x2a0 net/ipv4/ip_output.c:437
> [<ffffffff84d123b5>] dst_output include/net/dst.h:444 [inline]
> [<ffffffff84d123b5>] ip_local_out net/ipv4/ip_output.c:127 [inline]
> [<ffffffff84d123b5>] __ip_queue_xmit+0x1425/0x2000 net/ipv4/ip_output.c:542
> [<ffffffff84d12fdc>] ip_queue_xmit+0x4c/0x70 net/ipv4/ip_output.c:556
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
cheers,
jamal
> ---
> net/sched/sch_netem.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index 6ef3021e1169a376494d610ac8c3f1c04fb911c5..e79be1b3e74da3c154f7ee23e16cc9e8da8f7106 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -966,6 +966,7 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
> if (ret < 0)
> return ret;
>
> + sch_tree_lock(sch);
> /* backup q->clg and q->loss_model */
> old_clg = q->clg;
> old_loss_model = q->loss_model;
> @@ -974,7 +975,7 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
> ret = get_loss_clg(q, tb[TCA_NETEM_LOSS]);
> if (ret) {
> q->loss_model = old_loss_model;
> - return ret;
> + goto unlock;
> }
> } else {
> q->loss_model = CLG_RANDOM;
> @@ -1041,6 +1042,8 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
> /* capping jitter to the range acceptable by tabledist() */
> q->jitter = min_t(s64, abs(q->jitter), INT_MAX);
>
> +unlock:
> + sch_tree_unlock(sch);
> return ret;
>
> get_table_failure:
> @@ -1050,7 +1053,8 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
> */
> q->clg = old_clg;
> q->loss_model = old_loss_model;
> - return ret;
> +
> + goto unlock;
> }
>
> static int netem_init(struct Qdisc *sch, struct nlattr *opt,
> --
> 2.41.0.178.g377b9f9a00-goog
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] sch_netem: acquire qdisc lock in netem_change()
2023-06-20 18:44 [PATCH net] sch_netem: acquire qdisc lock in netem_change() Eric Dumazet
2023-06-21 11:21 ` Jamal Hadi Salim
@ 2023-06-21 14:51 ` Simon Horman
2023-06-22 9:20 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 5+ messages in thread
From: Simon Horman @ 2023-06-21 14:51 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Stephen Hemminger, Jamal Hadi Salim,
Cong Wang, Jiri Pirko
On Tue, Jun 20, 2023 at 06:44:25PM +0000, Eric Dumazet wrote:
> syzbot managed to trigger a divide error [1] in netem.
>
> It could happen if q->rate changes while netem_enqueue()
> is running, since q->rate is read twice.
>
> It turns out netem_change() always lacked proper synchronization.
>
> [1]
> divide error: 0000 [#1] SMP KASAN
> CPU: 1 PID: 7867 Comm: syz-executor.1 Not tainted 6.1.30-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/25/2023
> RIP: 0010:div64_u64 include/linux/math64.h:69 [inline]
> RIP: 0010:packet_time_ns net/sched/sch_netem.c:357 [inline]
> RIP: 0010:netem_enqueue+0x2067/0x36d0 net/sched/sch_netem.c:576
> Code: 89 e2 48 69 da 00 ca 9a 3b 42 80 3c 28 00 4c 8b a4 24 88 00 00 00 74 0d 4c 89 e7 e8 c3 4f 3b fd 48 8b 4c 24 18 48 89 d8 31 d2 <49> f7 34 24 49 01 c7 4c 8b 64 24 48 4d 01 f7 4c 89 e3 48 c1 eb 03
> RSP: 0018:ffffc9000dccea60 EFLAGS: 00010246
> RAX: 000001a442624200 RBX: 000001a442624200 RCX: ffff888108a4f000
> RDX: 0000000000000000 RSI: 000000000000070d RDI: 000000000000070d
> RBP: ffffc9000dcceb90 R08: ffffffff849c5e26 R09: fffffbfff10e1297
> R10: 0000000000000000 R11: dffffc0000000001 R12: ffff888108a4f358
> R13: dffffc0000000000 R14: 0000001a8cd9a7ec R15: 0000000000000000
> FS: 00007fa73fe18700(0000) GS:ffff8881f6b00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fa73fdf7718 CR3: 000000011d36e000 CR4: 0000000000350ee0
> Call Trace:
> <TASK>
> [<ffffffff84714385>] __dev_xmit_skb net/core/dev.c:3931 [inline]
> [<ffffffff84714385>] __dev_queue_xmit+0xcf5/0x3370 net/core/dev.c:4290
> [<ffffffff84d22df2>] dev_queue_xmit include/linux/netdevice.h:3030 [inline]
> [<ffffffff84d22df2>] neigh_hh_output include/net/neighbour.h:531 [inline]
> [<ffffffff84d22df2>] neigh_output include/net/neighbour.h:545 [inline]
> [<ffffffff84d22df2>] ip_finish_output2+0xb92/0x10d0 net/ipv4/ip_output.c:235
> [<ffffffff84d21e63>] __ip_finish_output+0xc3/0x2b0
> [<ffffffff84d10a81>] ip_finish_output+0x31/0x2a0 net/ipv4/ip_output.c:323
> [<ffffffff84d10f14>] NF_HOOK_COND include/linux/netfilter.h:298 [inline]
> [<ffffffff84d10f14>] ip_output+0x224/0x2a0 net/ipv4/ip_output.c:437
> [<ffffffff84d123b5>] dst_output include/net/dst.h:444 [inline]
> [<ffffffff84d123b5>] ip_local_out net/ipv4/ip_output.c:127 [inline]
> [<ffffffff84d123b5>] __ip_queue_xmit+0x1425/0x2000 net/ipv4/ip_output.c:542
> [<ffffffff84d12fdc>] ip_queue_xmit+0x4c/0x70 net/ipv4/ip_output.c:556
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> ---
> net/sched/sch_netem.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
Reviewed-by: Simon Horman <simon.horman@corigine.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] sch_netem: acquire qdisc lock in netem_change()
2023-06-20 18:44 [PATCH net] sch_netem: acquire qdisc lock in netem_change() Eric Dumazet
2023-06-21 11:21 ` Jamal Hadi Salim
2023-06-21 14:51 ` Simon Horman
@ 2023-06-22 9:20 ` patchwork-bot+netdevbpf
2023-06-22 16:31 ` Eric Dumazet
2 siblings, 1 reply; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-06-22 9:20 UTC (permalink / raw)
To: Eric Dumazet
Cc: davem, kuba, pabeni, netdev, eric.dumazet, syzkaller, stephen,
jhs, xiyou.wangcong, jiri
Hello:
This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:
On Tue, 20 Jun 2023 18:44:25 +0000 you wrote:
> syzbot managed to trigger a divide error [1] in netem.
>
> It could happen if q->rate changes while netem_enqueue()
> is running, since q->rate is read twice.
>
> It turns out netem_change() always lacked proper synchronization.
>
> [...]
Here is the summary with links:
- [net] sch_netem: acquire qdisc lock in netem_change()
https://git.kernel.org/netdev/net/c/2174a08db80d
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] sch_netem: acquire qdisc lock in netem_change()
2023-06-22 9:20 ` patchwork-bot+netdevbpf
@ 2023-06-22 16:31 ` Eric Dumazet
0 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2023-06-22 16:31 UTC (permalink / raw)
To: patchwork-bot+netdevbpf
Cc: davem, kuba, pabeni, netdev, eric.dumazet, syzkaller, stephen,
jhs, xiyou.wangcong, jiri
On Thu, Jun 22, 2023 at 11:20 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>
> Hello:
>
> This patch was applied to netdev/net.git (main)
> by Paolo Abeni <pabeni@redhat.com>:
>
> On Tue, 20 Jun 2023 18:44:25 +0000 you wrote:
> > syzbot managed to trigger a divide error [1] in netem.
> >
> > It could happen if q->rate changes while netem_enqueue()
> > is running, since q->rate is read twice.
> >
> > It turns out netem_change() always lacked proper synchronization.
> >
> > [...]
>
> Here is the summary with links:
> - [net] sch_netem: acquire qdisc lock in netem_change()
> https://git.kernel.org/netdev/net/c/2174a08db80d
>
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
I missed the fact that get_dist_table() was called from netem_change()
This function uses GFP_KERNEL and also acquires the qdisc lock.
I will send a fix soon.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-06-22 16:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-20 18:44 [PATCH net] sch_netem: acquire qdisc lock in netem_change() Eric Dumazet
2023-06-21 11:21 ` Jamal Hadi Salim
2023-06-21 14:51 ` Simon Horman
2023-06-22 9:20 ` patchwork-bot+netdevbpf
2023-06-22 16:31 ` Eric Dumazet
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).