From: Stephen Hemminger <stephen@networkplumber•org>
To: Eric Dumazet <eric.dumazet@gmail•com>
Cc: David Miller <davem@davemloft•net>,
Cong Wang <xiyou.wangcong@gmail•com>,
netdev@vger•kernel.org
Subject: Re: [PATCH net-next] htb: fix sign extension bug
Date: Fri, 2 Aug 2013 08:24:04 -0700 [thread overview]
Message-ID: <20130802082404.600bc55c@nehalam.linuxnetplumber.net> (raw)
In-Reply-To: <1375450580.3927.12.camel@edumazet-glaptop>
On Fri, 02 Aug 2013 06:36:20 -0700
Eric Dumazet <eric.dumazet@gmail•com> wrote:
> On Thu, 2013-08-01 at 22:32 -0700, Stephen Hemminger wrote:
> > 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 <stephen@networkplumber•org>
> >
> > --- a/net/sched/sch_htb.c 2013-06-20 09:22:46.489542435 -0700
> > +++ b/net/sched/sch_htb.c 2013-08-01 22:12:43.736307055 -0700
> > @@ -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 <edumazet@google•com>
>
> 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.
next prev parent reply other threads:[~2013-08-02 15:24 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-02 4:12 Fw: [Bug 60669] Kernel panic when using negative priority for HTB class Stephen Hemminger
2013-08-02 4:50 ` Cong Wang
2013-08-02 5:31 ` Stephen Hemminger
[not found] ` <20130801221936.24d4b31e@nehalam.linuxnetplumber.net>
2013-08-02 5:32 ` [PATCH net-next] htb: fix sign extension bug Stephen Hemminger
2013-08-02 13:36 ` Eric Dumazet
2013-08-02 15:24 ` Stephen Hemminger [this message]
2013-08-02 21:53 ` David Miller
2013-08-02 21:52 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130802082404.600bc55c@nehalam.linuxnetplumber.net \
--to=stephen@networkplumber$(echo .)org \
--cc=davem@davemloft$(echo .)net \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=xiyou.wangcong@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox