Re: [net-next 1/7] net/sched: Check for null dev_queue on create flow

[Davide Caratti <dcaratti@redhat.com>]

On Thu, 2017-10-26 at 10:17 -0700, Jeff Kirsher 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, or if the
> dev_queue associated with the parent qdisc is null.
> 
> This patch is in preparation for the next in this series, where
> select_queue() is being added to mqprio and as it may return a null
> dev_queue.
> 
> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
> Tested-by: Henrik Austad <henrik@austad.us>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  net/sched/sch_generic.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

hello,

I didn't notice this when I posted https://www.spinics.net/lists/netdev/ms
g462986.html , about one hour later, targeting Dave's net tree. I saw
similar issues, but in my setup dev_queue was a valid pointer, and dev was
NULL. This made qdisc_alloc() dereference NULL, when accessing dev->
members in the function body before returning the newly allocated qdisc.

So, in my understanding both tests are necessary, but a (very trivial)
conflict will be generated when these two commits will be eventually
merged together.

I like this suggestion from Ivan:

-- >8 --
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index bf8c81e07c70..6bd1ae993326 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 || !dev_queue->dev) {
+               err = !dev_queue ? -EINVAL : -ENOENT;
+               goto errout;
+       }

+       dev = dev_queue->dev;
        p = kzalloc_node(size, GFP_KERNEL,
                         netdev_queue_numa_node_read(dev_queue));

-- 8< --

and I volunteer for sending a v2 of 'net/sched: fix NULL pointer
dereference in qdisc_alloc()' including this, targeting 'net' tree, with
an appropriate tag. WDYT?

regards,
-- 
davide