diff mbox series

[net] sch_htb: fix refcount leak in htb_parent_to_leaf_offload

Message ID 1622804598-11164-1-git-send-email-wangyunjian@huawei.com (mailing list archive)
State Accepted
Commit 944d671d5faa0d78980a3da5c0f04960ef1ad893
Delegated to: Netdev Maintainers
Headers show
Series [net] sch_htb: fix refcount leak in htb_parent_to_leaf_offload | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/cc_maintainers warning 1 maintainers not CCed: jiri@resnulli.us
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 36 this patch: 36
netdev/kdoc success Errors and warnings before: 36 this patch: 36
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 21 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 36 this patch: 36
netdev/header_inline success Link

Commit Message

wangyunjian June 4, 2021, 11:03 a.m. UTC 
From: Yunjian Wang <wangyunjian@huawei.com>

The commit ae81feb7338c ("sch_htb: fix null pointer dereference
on a null new_q") fixes a NULL pointer dereference bug, but it
is not correct.

Because htb_graft_helper properly handles the case when new_q
is NULL, and after the previous patch by skipping this call
which creates an inconsistency : dev_queue->qdisc will still
point to the old qdisc, but cl->parent->leaf.q will point to
the new one (which will be noop_qdisc, because new_q was NULL).
The code is based on an assumption that these two pointers are
the same, so it can lead to refcount leaks.

The correct fix is to add a NULL pointer check to protect
qdisc_refcount_inc inside htb_parent_to_leaf_offload.

Fixes: ae81feb7338c ("sch_htb: fix null pointer dereference on a null new_q")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Suggested-by: Maxim Mikityanskiy <maximmi@nvidia.com>
---
 net/sched/sch_htb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

patchwork-bot+netdevbpf@kernel.org June 4, 2021, 9:50 p.m. UTC | #1 
Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Fri, 4 Jun 2021 19:03:18 +0800 you wrote:
> From: Yunjian Wang <wangyunjian@huawei.com>
> 
> The commit ae81feb7338c ("sch_htb: fix null pointer dereference
> on a null new_q") fixes a NULL pointer dereference bug, but it
> is not correct.
> 
> Because htb_graft_helper properly handles the case when new_q
> is NULL, and after the previous patch by skipping this call
> which creates an inconsistency : dev_queue->qdisc will still
> point to the old qdisc, but cl->parent->leaf.q will point to
> the new one (which will be noop_qdisc, because new_q was NULL).
> The code is based on an assumption that these two pointers are
> the same, so it can lead to refcount leaks.
> 
> [...]

Here is the summary with links:
  - [net] sch_htb: fix refcount leak in htb_parent_to_leaf_offload
    https://git.kernel.org/netdev/net/c/944d671d5faa

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
diff mbox series

Patch

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 4f9304567dcc..469301a18ff8 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1491,7 +1491,8 @@  static void htb_parent_to_leaf_offload(struct Qdisc *sch,
 	struct Qdisc *old_q;
 
 	/* One ref for cl->leaf.q, the other for dev_queue->qdisc. */
-	qdisc_refcount_inc(new_q);
+	if (new_q)
+		qdisc_refcount_inc(new_q);
 	old_q = htb_graft_helper(dev_queue, new_q);
 	WARN_ON(!(old_q->flags & TCQ_F_BUILTIN));
 }
@@ -1678,10 +1679,9 @@  static int htb_delete(struct Qdisc *sch, unsigned long arg,
 					  cl->parent->common.classid,
 					  NULL);
 		if (q->offload) {
-			if (new_q) {
+			if (new_q)
 				htb_set_lockdep_class_child(new_q);
-				htb_parent_to_leaf_offload(sch, dev_queue, new_q);
-			}
+			htb_parent_to_leaf_offload(sch, dev_queue, new_q);
 		}
 	}