| Message ID | 20221018034718.82389-3-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | fix null pointer access issue in qdisc | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 43 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Oct 17, 2022 at 8:39 PM Zhengchao Shao <shaozhengchao@huawei.com> wrote: > > When the default qdisc is fq_codel, if the qdisc of dev_queue fails to be > inited during mqprio_init(), fq_codel_reset() is invoked to clear > resources. In this case, the flow is NULL, and it will cause gpf issue. > > The process is as follows: > qdisc_create_dflt() > fq_codel_init() > ... > q->flows_cnt = 1024; > ... > q->flows = kvcalloc(...) --->failed, q->flows is NULL > ... > ... > qdisc_put() > ... > fq_codel_reset() > ... > flow = q->flows + i --->q->flows is NULL > > The following is the Call Trace information: > general protection fault, probably for non-canonical address > 0xdffffc0000000001: 0000 [#1] PREEMPT SMP KASAN > KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f] > RIP: 0010:fq_codel_reset+0x14d/0x350 > Call Trace: > <TASK> > qdisc_reset+0xed/0x6f0 > qdisc_destroy+0x82/0x4c0 > qdisc_put+0x9e/0xb0 > qdisc_create_dflt+0x2c3/0x4a0 > mqprio_init+0xa71/0x1760 > qdisc_create+0x3eb/0x1000 > tc_modify_qdisc+0x408/0x1720 > rtnetlink_rcv_msg+0x38e/0xac0 > netlink_rcv_skb+0x12d/0x3a0 > netlink_unicast+0x4a2/0x740 > netlink_sendmsg+0x826/0xcc0 > sock_sendmsg+0xc5/0x100 > ____sys_sendmsg+0x583/0x690 > ___sys_sendmsg+0xe8/0x160 > __sys_sendmsg+0xbf/0x160 > do_syscall_64+0x35/0x80 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > RIP: 0033:0x7fd272b22d04 > </TASK> > > Fixes: 494f5063b86c ("net: sched: fq_codel: remove redundant resource cleanup in fq_codel_init()") > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > --- I vote for a revert, previous code was much cleaner.
On 2022/10/18 12:02, Eric Dumazet wrote: > On Mon, Oct 17, 2022 at 8:39 PM Zhengchao Shao <shaozhengchao@huawei.com> wrote: >> >> When the default qdisc is fq_codel, if the qdisc of dev_queue fails to be >> inited during mqprio_init(), fq_codel_reset() is invoked to clear >> resources. In this case, the flow is NULL, and it will cause gpf issue. >> >> The process is as follows: >> qdisc_create_dflt() >> fq_codel_init() >> ... >> q->flows_cnt = 1024; >> ... >> q->flows = kvcalloc(...) --->failed, q->flows is NULL >> ... >> ... >> qdisc_put() >> ... >> fq_codel_reset() >> ... >> flow = q->flows + i --->q->flows is NULL >> >> The following is the Call Trace information: >> general protection fault, probably for non-canonical address >> 0xdffffc0000000001: 0000 [#1] PREEMPT SMP KASAN >> KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f] >> RIP: 0010:fq_codel_reset+0x14d/0x350 >> Call Trace: >> <TASK> >> qdisc_reset+0xed/0x6f0 >> qdisc_destroy+0x82/0x4c0 >> qdisc_put+0x9e/0xb0 >> qdisc_create_dflt+0x2c3/0x4a0 >> mqprio_init+0xa71/0x1760 >> qdisc_create+0x3eb/0x1000 >> tc_modify_qdisc+0x408/0x1720 >> rtnetlink_rcv_msg+0x38e/0xac0 >> netlink_rcv_skb+0x12d/0x3a0 >> netlink_unicast+0x4a2/0x740 >> netlink_sendmsg+0x826/0xcc0 >> sock_sendmsg+0xc5/0x100 >> ____sys_sendmsg+0x583/0x690 >> ___sys_sendmsg+0xe8/0x160 >> __sys_sendmsg+0xbf/0x160 >> do_syscall_64+0x35/0x80 >> entry_SYSCALL_64_after_hwframe+0x46/0xb0 >> RIP: 0033:0x7fd272b22d04 >> </TASK> >> >> Fixes: 494f5063b86c ("net: sched: fq_codel: remove redundant resource cleanup in fq_codel_init()") >> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> >> --- > > I vote for a revert, previous code was much cleaner. Hi Eric: Thank you for your review. I will revert it in V2. Zhengchao Shao
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c index 99d318b60568..d1e3c8083f2d 100644 --- a/net/sched/sch_fq_codel.c +++ b/net/sched/sch_fq_codel.c @@ -478,23 +478,27 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt, if (opt) { err = fq_codel_change(sch, opt, extack); if (err) - return err; + goto err_out; } err = tcf_block_get(&q->block, &q->filter_list, sch, extack); if (err) - return err; + goto err_out; if (!q->flows) { q->flows = kvcalloc(q->flows_cnt, sizeof(struct fq_codel_flow), GFP_KERNEL); - if (!q->flows) - return -ENOMEM; + if (!q->flows) { + err = -ENOMEM; + goto err_out; + } q->backlogs = kvcalloc(q->flows_cnt, sizeof(u32), GFP_KERNEL); - if (!q->backlogs) - return -ENOMEM; + if (!q->backlogs) { + err = -ENOMEM; + goto err_out; + } for (i = 0; i < q->flows_cnt; i++) { struct fq_codel_flow *flow = q->flows + i; @@ -508,6 +512,10 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt, else sch->flags &= ~TCQ_F_CAN_BYPASS; return 0; + +err_out: + q->flows_cnt = 0; + return err; } static int fq_codel_dump(struct Qdisc *sch, struct sk_buff *skb)
When the default qdisc is fq_codel, if the qdisc of dev_queue fails to be inited during mqprio_init(), fq_codel_reset() is invoked to clear resources. In this case, the flow is NULL, and it will cause gpf issue. The process is as follows: qdisc_create_dflt() fq_codel_init() ... q->flows_cnt = 1024; ... q->flows = kvcalloc(...) --->failed, q->flows is NULL ... ... qdisc_put() ... fq_codel_reset() ... flow = q->flows + i --->q->flows is NULL The following is the Call Trace information: general protection fault, probably for non-canonical address 0xdffffc0000000001: 0000 [#1] PREEMPT SMP KASAN KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f] RIP: 0010:fq_codel_reset+0x14d/0x350 Call Trace: <TASK> qdisc_reset+0xed/0x6f0 qdisc_destroy+0x82/0x4c0 qdisc_put+0x9e/0xb0 qdisc_create_dflt+0x2c3/0x4a0 mqprio_init+0xa71/0x1760 qdisc_create+0x3eb/0x1000 tc_modify_qdisc+0x408/0x1720 rtnetlink_rcv_msg+0x38e/0xac0 netlink_rcv_skb+0x12d/0x3a0 netlink_unicast+0x4a2/0x740 netlink_sendmsg+0x826/0xcc0 sock_sendmsg+0xc5/0x100 ____sys_sendmsg+0x583/0x690 ___sys_sendmsg+0xe8/0x160 __sys_sendmsg+0xbf/0x160 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 RIP: 0033:0x7fd272b22d04 </TASK> Fixes: 494f5063b86c ("net: sched: fq_codel: remove redundant resource cleanup in fq_codel_init()") Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- net/sched/sch_fq_codel.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)