* [PATCH net] net/sched: pfifo_fast: fix wrong dereference when qdisc is reset
@ 2019-08-26 23:15 Davide Caratti
2019-08-27 9:50 ` Stefano Brivio
0 siblings, 1 reply; 3+ messages in thread
From: Davide Caratti @ 2019-08-26 23:15 UTC (permalink / raw)
To: Cong Wang, Jamal Hadi Salim, Jiri Pirko, David S. Miller, netdev
Cc: Paolo Abeni, Li Shuang
Now that 'TCQ_F_CPUSTATS' bit can be cleared, depending on the value of
'TCQ_F_NOLOCK' bit in the parent qdisc, we need to be sure that per-cpu
counters are present when 'reset()' is called for pfifo_fast qdiscs.
Otherwise, the following script:
# tc q a dev lo handle 1: root htb default 100
# tc c a dev lo parent 1: classid 1:100 htb \
> rate 95Mbit ceil 100Mbit burst 64k
[...]
# tc f a dev lo parent 1: protocol arp basic classid 1:100
[...]
# tc q a dev lo parent 1:100 handle 100: pfifo_fast
[...]
# tc q d dev lo root
can generate the following splat:
Unable to handle kernel paging request at virtual address dfff2c01bd148000
Mem abort info:
ESR = 0x96000004
Exception class = DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
Data abort info:
ISV = 0, ISS = 0x00000004
CM = 0, WnR = 0
[dfff2c01bd148000] address between user and kernel address ranges
Internal error: Oops: 96000004 [#1] SMP
[...]
pstate: 80000005 (Nzcv daif -PAN -UAO)
pc : pfifo_fast_reset+0x280/0x4d8
lr : pfifo_fast_reset+0x21c/0x4d8
sp : ffff800d09676fa0
x29: ffff800d09676fa0 x28: ffff200012ee22e4
x27: dfff200000000000 x26: 0000000000000000
x25: ffff800ca0799958 x24: ffff1001940f332b
x23: 0000000000000007 x22: ffff200012ee1ab8
x21: 0000600de8a40000 x20: 0000000000000000
x19: ffff800ca0799900 x18: 0000000000000000
x17: 0000000000000002 x16: 0000000000000000
x15: 0000000000000000 x14: 0000000000000000
x13: 0000000000000000 x12: ffff1001b922e6e2
x11: 1ffff001b922e6e1 x10: 0000000000000000
x9 : 1ffff001b922e6e1 x8 : dfff200000000000
x7 : 0000000000000000 x6 : 0000000000000000
x5 : 1fffe400025dc45c x4 : 1fffe400025dc357
x3 : 00000c01bd148000 x2 : 0000600de8a40000
x1 : 0000000000000007 x0 : 0000600de8a40004
Call trace:
pfifo_fast_reset+0x280/0x4d8
qdisc_reset+0x6c/0x370
htb_reset+0x150/0x3b8 [sch_htb]
qdisc_reset+0x6c/0x370
dev_deactivate_queue.constprop.5+0xe0/0x1a8
dev_deactivate_many+0xd8/0x908
dev_deactivate+0xe4/0x190
qdisc_graft+0x88c/0xbd0
tc_get_qdisc+0x418/0x8a8
rtnetlink_rcv_msg+0x3a8/0xa78
netlink_rcv_skb+0x18c/0x328
rtnetlink_rcv+0x28/0x38
netlink_unicast+0x3c4/0x538
netlink_sendmsg+0x538/0x9a0
sock_sendmsg+0xac/0xf8
___sys_sendmsg+0x53c/0x658
__sys_sendmsg+0xc8/0x140
__arm64_sys_sendmsg+0x74/0xa8
el0_svc_handler+0x164/0x468
el0_svc+0x10/0x14
Code: 910012a0 92400801 d343fc03 11000c21 (38fb6863)
Fix this by testing the value of 'TCQ_F_CPUSTATS' bit in 'qdisc->flags',
before dereferencing 'qdisc->cpu_stats'.
Fixes: 8a53e616de29 ("net: sched: when clearing NOLOCK, clear TCQ_F_CPUSTATS, too")
CC: Paolo Abeni <pabeni@redhat.com>
Reported-by: Li Shuang <shuali@redhat.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/sch_generic.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 11c03cf4aa74..c89b787785a1 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -688,12 +688,14 @@ static void pfifo_fast_reset(struct Qdisc *qdisc)
kfree_skb(skb);
}
- for_each_possible_cpu(i) {
- struct gnet_stats_queue *q = per_cpu_ptr(qdisc->cpu_qstats, i);
+ if (qdisc_is_percpu_stats(qdisc))
+ for_each_possible_cpu(i) {
+ struct gnet_stats_queue *q =
+ per_cpu_ptr(qdisc->cpu_qstats, i);
- q->backlog = 0;
- q->qlen = 0;
- }
+ q->backlog = 0;
+ q->qlen = 0;
+ }
}
static int pfifo_fast_dump(struct Qdisc *qdisc, struct sk_buff *skb)
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] net/sched: pfifo_fast: fix wrong dereference when qdisc is reset
2019-08-26 23:15 [PATCH net] net/sched: pfifo_fast: fix wrong dereference when qdisc is reset Davide Caratti
@ 2019-08-27 9:50 ` Stefano Brivio
2019-08-27 10:15 ` Davide Caratti
0 siblings, 1 reply; 3+ messages in thread
From: Stefano Brivio @ 2019-08-27 9:50 UTC (permalink / raw)
To: Davide Caratti
Cc: Cong Wang, Jamal Hadi Salim, Jiri Pirko, David S. Miller, netdev,
Paolo Abeni, Li Shuang
On Tue, 27 Aug 2019 01:15:16 +0200
Davide Caratti <dcaratti@redhat.com> wrote:
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 11c03cf4aa74..c89b787785a1 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -688,12 +688,14 @@ static void pfifo_fast_reset(struct Qdisc *qdisc)
> kfree_skb(skb);
> }
>
> - for_each_possible_cpu(i) {
> - struct gnet_stats_queue *q = per_cpu_ptr(qdisc->cpu_qstats, i);
> + if (qdisc_is_percpu_stats(qdisc))
This needs curly brackets, as the block has multiple lines (for coding
style only).
> + for_each_possible_cpu(i) {
> + struct gnet_stats_queue *q =
> + per_cpu_ptr(qdisc->cpu_qstats, i);
And you could split declaration and assignment here, it takes two lines
anyway and becomes more readable.
--
Stefano
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] net/sched: pfifo_fast: fix wrong dereference when qdisc is reset
2019-08-27 9:50 ` Stefano Brivio
@ 2019-08-27 10:15 ` Davide Caratti
0 siblings, 0 replies; 3+ messages in thread
From: Davide Caratti @ 2019-08-27 10:15 UTC (permalink / raw)
To: Stefano Brivio
Cc: Cong Wang, Jamal Hadi Salim, Jiri Pirko, David S. Miller, netdev,
Paolo Abeni, Li Shuang
hello Stefano,
thanks for looking at this.
On Tue, 2019-08-27 at 11:50 +0200, Stefano Brivio wrote:
> On Tue, 27 Aug 2019 01:15:16 +0200
> Davide Caratti <dcaratti@redhat.com> wrote:
[...]
> @@ -688,12 +688,14 @@ static void pfifo_fast_reset(struct Qdisc *qdisc)
> > kfree_skb(skb);
> > }
> >
> > - for_each_possible_cpu(i) {
> > - struct gnet_stats_queue *q = per_cpu_ptr(qdisc->cpu_qstats, i);
> > + if (qdisc_is_percpu_stats(qdisc))
>
> This needs curly brackets, as the block has multiple lines (for coding
> style only).
If I well read Documentation/process/coding-style.rst, at the end of
section 3), this rule should apply to loops. But I'm fine with the curly
brace here, if checkpatch doesn't say anything.
I will add it in v2.
> > + for_each_possible_cpu(i) {
> > + struct gnet_stats_queue *q =
> > + per_cpu_ptr(qdisc->cpu_qstats, i);
>
> And you could split declaration and assignment here, it takes two lines
> anyway and becomes more readable.
ok, I will do that in v2.
thanks!
--
davide
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-08-27 10:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-26 23:15 [PATCH net] net/sched: pfifo_fast: fix wrong dereference when qdisc is reset Davide Caratti
2019-08-27 9:50 ` Stefano Brivio
2019-08-27 10:15 ` Davide Caratti
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).