* [PATCH net 1/2] sch_htb: Fix select_queue for non-offload mode
2021-03-11 14:42 [PATCH net 0/2] Bugfixes for HTB Maxim Mikityanskiy
@ 2021-03-11 14:42 ` Maxim Mikityanskiy
2021-03-11 14:42 ` [PATCH net 2/2] sch_htb: Fix offload cleanup in htb_destroy on htb_init failure Maxim Mikityanskiy
2021-03-12 1:36 ` [PATCH net 0/2] Bugfixes for HTB Cong Wang
2 siblings, 0 replies; 4+ messages in thread
From: Maxim Mikityanskiy @ 2021-03-11 14:42 UTC (permalink / raw)
To: Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: David S. Miller, Jakub Kicinski, Tariq Toukan, Eric Dumazet,
netdev, Maxim Mikityanskiy, syzbot+b53a709f04722ca12a3c
htb_select_queue assumes it's always the offload mode, and it ends up in
calling ndo_setup_tc without any checks. It may lead to a NULL pointer
dereference if ndo_setup_tc is not implemented, or to an error returned
from the driver, which will prevent attaching qdiscs to HTB classes in
the non-offload mode.
This commit fixes the bug by adding the missing check to
htb_select_queue. In the non-offload mode it will return sch->dev_queue,
mimicking tc_modify_qdisc's behavior for the case where select_queue is
not implemented.
Reported-by: syzbot+b53a709f04722ca12a3c@syzkaller.appspotmail.com
Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
---
net/sched/sch_htb.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index dff3adf5a915..b23203159996 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1340,8 +1340,12 @@ htb_select_queue(struct Qdisc *sch, struct tcmsg *tcm)
{
struct net_device *dev = qdisc_dev(sch);
struct tc_htb_qopt_offload offload_opt;
+ struct htb_sched *q = qdisc_priv(sch);
int err;
+ if (!q->offload)
+ return sch->dev_queue;
+
offload_opt = (struct tc_htb_qopt_offload) {
.command = TC_HTB_LEAF_QUERY_QUEUE,
.classid = TC_H_MIN(tcm->tcm_parent),
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH net 2/2] sch_htb: Fix offload cleanup in htb_destroy on htb_init failure
2021-03-11 14:42 [PATCH net 0/2] Bugfixes for HTB Maxim Mikityanskiy
2021-03-11 14:42 ` [PATCH net 1/2] sch_htb: Fix select_queue for non-offload mode Maxim Mikityanskiy
@ 2021-03-11 14:42 ` Maxim Mikityanskiy
2021-03-12 1:36 ` [PATCH net 0/2] Bugfixes for HTB Cong Wang
2 siblings, 0 replies; 4+ messages in thread
From: Maxim Mikityanskiy @ 2021-03-11 14:42 UTC (permalink / raw)
To: Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: David S. Miller, Jakub Kicinski, Tariq Toukan, Eric Dumazet,
netdev, Maxim Mikityanskiy, syzbot+b53a709f04722ca12a3c
htb_init may fail to do the offload if it's not supported or if a
runtime error happens when allocating direct qdiscs. In those cases
TC_HTB_CREATE command is not sent to the driver, however, htb_destroy
gets called anyway and attempts to send TC_HTB_DESTROY.
It shouldn't happen, because the driver didn't receive TC_HTB_CREATE,
and also because the driver may not support ndo_setup_tc at all, while
q->offload is true, and htb_destroy mistakenly thinks the offload is
supported. Trying to call ndo_setup_tc in the latter case will lead to a
NULL pointer dereference.
This commit fixes the issues with htb_destroy by deferring assignment of
q->offload until after the TC_HTB_CREATE command. The necessary cleanup
of the offload entities is already done in htb_init.
Reported-by: syzbot+b53a709f04722ca12a3c@syzkaller.appspotmail.com
Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
---
net/sched/sch_htb.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index b23203159996..62e12cb41a3e 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1020,6 +1020,7 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt,
struct nlattr *tb[TCA_HTB_MAX + 1];
struct tc_htb_glob *gopt;
unsigned int ntx;
+ bool offload;
int err;
qdisc_watchdog_init(&q->watchdog, sch);
@@ -1044,9 +1045,9 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt,
if (gopt->version != HTB_VER >> 16)
return -EINVAL;
- q->offload = nla_get_flag(tb[TCA_HTB_OFFLOAD]);
+ offload = nla_get_flag(tb[TCA_HTB_OFFLOAD]);
- if (q->offload) {
+ if (offload) {
if (sch->parent != TC_H_ROOT)
return -EOPNOTSUPP;
@@ -1076,7 +1077,7 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt,
q->rate2quantum = 1;
q->defcls = gopt->defcls;
- if (!q->offload)
+ if (!offload)
return 0;
for (ntx = 0; ntx < q->num_direct_qdiscs; ntx++) {
@@ -1107,12 +1108,14 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt,
if (err)
goto err_free_qdiscs;
+ /* Defer this assignment, so that htb_destroy skips offload-related
+ * parts (especially calling ndo_setup_tc) on errors.
+ */
+ q->offload = true;
+
return 0;
err_free_qdiscs:
- /* TC_HTB_CREATE call failed, avoid any further calls to the driver. */
- q->offload = false;
-
for (ntx = 0; ntx < q->num_direct_qdiscs && q->direct_qdiscs[ntx];
ntx++)
qdisc_put(q->direct_qdiscs[ntx]);
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net 0/2] Bugfixes for HTB
2021-03-11 14:42 [PATCH net 0/2] Bugfixes for HTB Maxim Mikityanskiy
2021-03-11 14:42 ` [PATCH net 1/2] sch_htb: Fix select_queue for non-offload mode Maxim Mikityanskiy
2021-03-11 14:42 ` [PATCH net 2/2] sch_htb: Fix offload cleanup in htb_destroy on htb_init failure Maxim Mikityanskiy
@ 2021-03-12 1:36 ` Cong Wang
2 siblings, 0 replies; 4+ messages in thread
From: Cong Wang @ 2021-03-12 1:36 UTC (permalink / raw)
To: Maxim Mikityanskiy
Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski,
Tariq Toukan, Eric Dumazet, Linux Kernel Network Developers
On Thu, Mar 11, 2021 at 6:42 AM Maxim Mikityanskiy <maximmi@nvidia.com> wrote:
>
> The HTB offload feature introduced a few bugs in HTB. One affects the
> non-offload mode, preventing attaching qdiscs to HTB classes, and the
> other affects the error flow, when the netdev doesn't support the
> offload, but it was requested. This short series fixes them.
Both look good to me.
Acked-by: Cong Wang <cong.wang@bytedance.com>
Thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread