Skip to contentSkip to conversation

Kernel panic when using negative priority for HTB class

Bug 60669 In review Stephen Hemminger 4 participants 8 messages Aug 2, 2013 1 message not in the archive

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

Begin forwarded message:

Date: Thu, 1 Aug 2013 02:38:53 -0700
From: "[email protected]" <[email protected]>
To: "[email protected]" <[email protected]>
Subject: [Bug 60669] Kernel panic when using negative priority for HTB class

https://bugzilla.kernel.org/show_bug.cgi?id=60669

_Vi <[email protected]> changed:

What |Removed |Added
----------------------------------------------------------------------------

            Summary|Kernel panic when using HTB |Kernel panic when using
                   |qdisc                       |negative priority for HTB
                   |                            |class

--- Comment #2 from _Vi <[email protected]> ---
Reproducible on 3.11.0-rc3

Looks like negative priority value is essential for the kernel panic.

Signature · 2 lines
You are receiving this mail because:
You are the assignee for the bug.
#1 lore raw
Quoted 6 lines · Stephen Hemminger

--- Comment #2 from _Vi <[email protected]> ---
Reproducible on 3.11.0-rc3

Looks like negative priority value is essential for the kernel panic.

There is a check for prio:

                if ((cl->prio = hopt->prio) >= TC_HTB_NUMPRIO)
		                        cl->prio = TC_HTB_NUMPRIO - 1;

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#3 lore raw

PATCH 1/1 htb: fix sign extension bug

When userspace passes a large priority value
the assignment of the unsigned value hopt->prio
to signed int cl->prio causes cl->prio to become negative and the comparison is with TC_HTB_NUMPRIO is always false.

The result is that HTB crashes by referencing outside the array when processing packets. With this patch the large value wraps around like other values outside the normal range.

See: https://bugzilla.kernel.org/show_bug.cgi?id=60669

Signed-off-by Stephen Hemminger <[email protected]>
net/sched/sch_htb.c +1 -1
@@ -100,7 +100,7 @@ struct htb_class {
 	struct psched_ratecfg	ceil;
 	s64			buffer, cbuffer;/* token bucket depth/rate */
 	s64			mbuffer;	/* max wait time */
-	int			prio;		/* these two are used only by leaves... */
+	u32			prio;		/* these two are used only by leaves... */
 	int			quantum;	/* but stored for parent-to-leaf return */
 
 	struct tcf_proto	*filter_list;	/* class attached filters */
Acked-by on v1 1/1
#4 lore raw

Re: [PATCH net-next] htb: fix sign extension bug

Quoted 16 lines · Stephen Hemminger

When userspace passes a large priority value
the assignment of the unsigned value hopt->prio
to signed int cl->prio causes cl->prio to become negative and the comparison is with TC_HTB_NUMPRIO is always false.

The result is that HTB crashes by referencing outside the array when processing packets. With this patch the large value wraps around like other values outside the normal range.

See: https://bugzilla.kernel.org/show_bug.cgi?id=60669

Signed-off-by Stephen Hemminger <[email protected]>

net/sched/sch_htb.c struct htb_class {

Quoted 9 lines
net/sched/sch_htb.c +1 -2
@@ -100,7 +100,7 @@ struct htb_class {
 	struct psched_ratecfg	ceil;
 	s64			buffer, cbuffer;/* token bucket depth/rate */
 	s64			mbuffer;	/* max wait time */
-	int			prio;		/* these two are used only by leaves... */
+	u32			prio;		/* these two are used only by leaves... */
 	int			quantum;	/* but stored for parent-to-leaf return */
 
 	struct tcf_proto	*filter_list;	/* class attached filters */
--

Thanks Stephen.

Acked-by Eric Dumazet <[email protected]>

It seems appropriate for net tree (and stable)

We probably should report an error from htb_change_class() ?

author
#5 lore raw

Re: [PATCH net-next] htb: fix sign extension bug

Quoted 29 lines · On Fri, 02 Aug 2013 06:36:20 -0700 Eric Dumazet <[email protected]> wrote:
Quoted 16 lines · Stephen Hemminger

When userspace passes a large priority value
the assignment of the unsigned value hopt->prio
to signed int cl->prio causes cl->prio to become negative and the comparison is with TC_HTB_NUMPRIO is always false.

The result is that HTB crashes by referencing outside the array when processing packets. With this patch the large value wraps around like other values outside the normal range.

See: https://bugzilla.kernel.org/show_bug.cgi?id=60669

Signed-off-by Stephen Hemminger <[email protected]>

net/sched/sch_htb.c struct htb_class {

Quoted 9 lines
net/sched/sch_htb.c +1 -2
@@ -100,7 +100,7 @@ struct htb_class {
 	struct psched_ratecfg	ceil;
 	s64			buffer, cbuffer;/* token bucket depth/rate */
 	s64			mbuffer;	/* max wait time */
-	int			prio;		/* these two are used only by leaves... */
+	u32			prio;		/* these two are used only by leaves... */
 	int			quantum;	/* but stored for parent-to-leaf return */
 
 	struct tcf_proto	*filter_list;	/* class attached filters */
--

Thanks Stephen.

Quoted 5 lines
Acked-by Eric Dumazet <[email protected]>

It seems appropriate for net tree (and stable)

We probably should report an error from htb_change_class() ?

There are more signed/unsigned bugs lurking here, and opportunities to shrink the structure. (for example prio could be u8). Overall this code uses int where unsigned should be used.

Examples:
static inline void htb_add_class_to_row(struct htb_sched *q,
struct htb_class *cl, int mask)

{
	q->row_mask[cl->level] |= mask;
	while (mask) {
		int prio = ffz(~mask);
		mask &= ~(1 << prio);
		htb_add_to_id_tree(&q->hlevel[cl->level].hprio[prio].row, cl, prio);
	}
}

Could be:
static void htb_add_class_to_row(struct htb_sched *q,
struct htb_class *cl, u8 mask)

{
	q->row_mask[cl->level] |= mask;
	while (mask) {
		u8 prio = ffz(~mask);
		mask &= ~(1 << prio);
		htb_add_to_id_tree(&q->hlevel[cl->level].hprio[prio].row, cl, prio);
	}
}

Looks like all use of int in HTB should be re-evaluated to see if a smaller unsigned type could be used instead.

maintainer
#6 lore raw

Re: [PATCH net-next] htb: fix sign extension bug

From: Stephen Hemminger <[email protected]>
Date: Fri, 2 Aug 2013 08:24:04 -0700

Quoted 3 lines

There are more signed/unsigned bugs lurking here, and opportunities to shrink the structure. (for example prio could be u8). Overall this code uses int where unsigned should be used.

If any of them can cause an OOPS like this one could, let's get that fixed as soon as possible.

Thanks.

maintainer
#7 lore raw

Re: [PATCH net-next] htb: fix sign extension bug

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Fri, 02 Aug 2013 06:36:20 -0700

Quoted 24 lines · Stephen Hemminger
Quoted 23 lines

When userspace passes a large priority value
the assignment of the unsigned value hopt->prio
to signed int cl->prio causes cl->prio to become negative and the comparison is with TC_HTB_NUMPRIO is always false.

The result is that HTB crashes by referencing outside the array when processing packets. With this patch the large value wraps around like other values outside the normal range.

See: https://bugzilla.kernel.org/show_bug.cgi?id=60669

Signed-off-by Stephen Hemminger <[email protected]>
net/sched/sch_htb.c +1 -1
@@ -100,7 +100,7 @@ struct htb_class {
 	struct psched_ratecfg	ceil;
 	s64			buffer, cbuffer;/* token bucket depth/rate */
 	s64			mbuffer;	/* max wait time */
-	int			prio;		/* these two are used only by leaves... */
+	u32			prio;		/* these two are used only by leaves... */
 	int			quantum;	/* but stored for parent-to-leaf return */
Quoted 8 lines

net/sched/sch_htb.c struct htb_class {

Quoted 2 lines
net/sched/sch_htb.c +0 -1
 	struct tcf_proto	*filter_list;	/* class attached filters */
--

Thanks Stephen.

Acked-by Eric Dumazet <[email protected]>

It seems appropriate for net tree (and stable)

Applied and queued up for -stable, thanks.

author
#8 lore raw
In reply to Cong Wang
Quoted 5 lines · On Fri, 2 Aug 2013 04:50:16 +0000 (UTC) Cong Wang <[email protected]>…
Quoted 4 lines · Stephen Hemminger

--- Comment #2 from _Vi <[email protected]> ---
Reproducible on 3.11.0-rc3

Quoted 8 lines
Quoted 2 lines

Looks like negative priority value is essential for the kernel panic.

There is a check for prio:

                if ((cl->prio = hopt->prio) >= TC_HTB_NUMPRIO)
		                        cl->prio = TC_HTB_NUMPRIO - 1;

This maybe related to or old bugs, 64 bit with current iproute2 the error is caught at the command level.

The issue is that hopt->prio is u32 but cl->prio is int.

Example:
int main()

{
	int x = -1;
	uint32_t prio = x;
	int cl;

	if ((cl = prio) >= TC_HTB_NUMPRIO)
		cl = TC_HTB_NUMPRIO - 1;

	printf("%d %u %d\n", x, prio, cl);
	return 0;
}

-1 4294967295 -1

Signed/unsigned conversions can bite.