On Tue, 2019-08-27 at 12:29 +0200, Davide Caratti wrote:
> 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_qstats'.
>
> Changes since v1:
> - coding style improvements, thanks to Stefano Brivio
>
> Fixes: 8a53e616de29 ("net: sched: when clearing NOLOCK, clear TCQ_F_CPUSTATS,
> too")
> CC: Paolo Abeni <pab...@redhat.com>
> Reported-by: Li Shuang <shu...@redhat.com>
> Signed-off-by: Davide Caratti <dcara...@redhat.com>
> ---
> net/sched/sch_generic.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 11c03cf4aa74..099797e5409d 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -688,11 +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;
>
> - q->backlog = 0;
> - q->qlen = 0;
> + q = per_cpu_ptr(qdisc->cpu_qstats, i);
> + q->backlog = 0;
> + q->qlen = 0;
> + }
> }
> }
Thanks for fixing this Davide!
Acked-by: Paolo Abeni <pab...@redhat.com>
