* [PATCH net 0/2] Bugfixes for HTB
@ 2021-03-11 14:42 Maxim Mikityanskiy
2021-03-11 14:42 ` [PATCH net 1/2] sch_htb: Fix select_queue for non-offload mode Maxim Mikityanskiy
` (2 more replies)
0 siblings, 3 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
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.
Maxim Mikityanskiy (2):
sch_htb: Fix select_queue for non-offload mode
sch_htb: Fix offload cleanup in htb_destroy on htb_init failure
net/sched/sch_htb.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* [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
end of thread, other threads:[~2021-03-12 1:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 ` [PATCH net 0/2] Bugfixes for HTB Cong Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.