| Message ID | 20171003234435.8979-3-vinicius.gomes@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On Tue, 2017-10-03 at 16:44 -0700, Vinicius Costa Gomes wrote: > From: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com> > > In qdisc_alloc() the dev_queue pointer was used without any checks being > performed. If qdisc_create() gets a null dev_queue pointer, it just > passes it along to qdisc_alloc(), leading to a crash. That happens if a > root qdisc implements select_queue() and returns a null dev_queue > pointer for an "invalid handle", for example. > > One way to reproduce that is: > > 1) Setup mqprio > $ tc qdisc replace dev enp3s0 parent root mqprio num_tc 3 \ > map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 queues 1@0 1@1 2@2 hw 0 > > 2) Replace the first inner qdisc > $ tc qdisc replace dev enp3s0 parent 8001:1 pfifo_fast > > This will lead to the following crash: When was this bug added ? If this is a consequence of your prior patch (1/4), then this must come before it. No need to add a stack trace for a not existing bug. Instead, explain in the changelog that it is a prep work. We try to not break the tree on purpose, so that future bisection will not hit a point where the kernel crashes.
Hi, On 10/03/2017 08:47 PM, Eric Dumazet wrote: > On Tue, 2017-10-03 at 16:44 -0700, Vinicius Costa Gomes wrote: >> From: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com> >> >> In qdisc_alloc() the dev_queue pointer was used without any checks being >> performed. If qdisc_create() gets a null dev_queue pointer, it just >> passes it along to qdisc_alloc(), leading to a crash. That happens if a >> root qdisc implements select_queue() and returns a null dev_queue >> pointer for an "invalid handle", for example. >> >> One way to reproduce that is: >> >> 1) Setup mqprio >> $ tc qdisc replace dev enp3s0 parent root mqprio num_tc 3 \ >> map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 queues 1@0 1@1 2@2 hw 0 >> >> 2) Replace the first inner qdisc >> $ tc qdisc replace dev enp3s0 parent 8001:1 pfifo_fast >> >> This will lead to the following crash: > > When was this bug added ? > > If this is a consequence of your prior patch (1/4), then this must come > before it. > > No need to add a stack trace for a not existing bug. > Instead, explain in the changelog that it is a prep work. > > We try to not break the tree on purpose, so that future bisection will > not hit a point where the kernel crashes. Sure, that makes absolute sense. It'll be fixed in our v5 as you've suggested. Thanks, Jesus
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index a0a198768aad..de2408f1ccd3 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -603,8 +603,14 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue, struct Qdisc *sch; unsigned int size = QDISC_ALIGN(sizeof(*sch)) + ops->priv_size; int err = -ENOBUFS; - struct net_device *dev = dev_queue->dev; + struct net_device *dev; + + if (!dev_queue) { + err = -EINVAL; + goto errout; + } + dev = dev_queue->dev; p = kzalloc_node(size, GFP_KERNEL, netdev_queue_numa_node_read(dev_queue));