Message ID | 20170930002657.15291-3-vinicius.gomes@intel.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Series | TSN: Add qdisc based config interface for CBS | expand |
On Fri, Sep 29, 2017 at 5:26 PM, Vinicius Costa Gomes <vinicius.gomes@intel.com> 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. Does it make sense to let mqprio_select_queue() always return non-NULL? At least mq_select_queue() returns queue #0 as a fallback.
Hi, On 09/30/2017 05:22 PM, Cong Wang wrote: > On Fri, Sep 29, 2017 at 5:26 PM, Vinicius Costa Gomes > <vinicius.gomes@intel.com> 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. > > Does it make sense to let mqprio_select_queue() always return > non-NULL? > > At least mq_select_queue() returns queue #0 as a fallback. I had seen that, but my understanding was that for mqprio the inner qdiscs are always related to one of the Tx netdev_queue per design. Returning any other queue as a fallback seemed like going against that to me. I'd rather keep this function as the patch is proposing, thus either returning the correct netdev_queue for a given handle, or NULL as a way to flag that something was 'wrong' with it. Returning queue #0 is misleading in that sense, imo. What do you think? Regards, 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));