Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220610021445.2441579-1-jianhao_xu@smail.nju.edu.cn>
 <3f460707-e267-e749-07fc-c44604cd5713@iogearbox.net> <tencent_29981C021E6150B064C7DBA3@qq.com>
In-Reply-To: <tencent_29981C021E6150B064C7DBA3@qq.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Fri, 10 Jun 2022 01:40:06 -0700
Message-ID: <CANn89iKHfi=kQY1FC=07COJfVX4ROTnGkM_1uKvOfPfdhqt4Ow@mail.gmail.com>
Subject: Re: [PATCH] net: sched: fix potential null pointer deref
To:     Jianhao Xu <jianhao_xu@smail.nju.edu.cn>
Cc:     Daniel Borkmann <daniel@iogearbox.net>, jhs <jhs@mojatatu.com>,
        "xiyou.wangcong" <xiyou.wangcong@gmail.com>,
        jiri <jiri@resnulli.us>, davem <davem@davemloft.net>,
        kuba <kuba@kernel.org>, pabeni <pabeni@redhat.com>,
        netdev <netdev@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jun 10, 2022 at 1:09 AM Jianhao Xu <jianhao_xu@smail.nju.edu.cn> wr=
ote:
>
> Hi,
>
> TBH, We do not have a reproducer. This is found by our static analysis to=
ol. We can not see any clue of the context here of mq_queue_get() to ensure=
 it never returns NULL.
>


All netdev devices have their dev->_tx allocated in netif_alloc_netdev_queu=
es()

There is absolutely no way MQ qdisc could be attached to a device that
has failed netif_alloc_netdev_queues() step.


> I would appreciate it if you could tell me why when you found out it was =
our false positive. It will be helpful for us to improve our tool.


Please do not send patches before you can provide a detailed
explanation of a real bug.

If you need help, post instead a [RFC] with a message explaining how
far you went into your analysis.

A patch should be sent only once you are absolutely sure that there is
a real bug to fix.

Thank you.


>
> Thanks.
> ------------------ Original ------------------
> From:  "Daniel Borkmann"<daniel@iogearbox.net>;
> Date:  Fri, Jun 10, 2022 09:14 AM
> To:  "Jianhao Xu"<jianhao_xu@smail.nju.edu.cn>; "jhs"<jhs@mojatatu.com>; =
"xiyou.wangcong"<xiyou.wangcong@gmail.com>; "jiri"<jiri@resnulli.us>; "dave=
m"<davem@davemloft.net>; "edumazet"<edumazet@google.com>; "kuba"<kuba@kerne=
l.org>; "pabeni"<pabeni@redhat.com>;
> Cc:  "netdev"<netdev@vger.kernel.org>; "linux-kernel"<linux-kernel@vger.k=
ernel.org>;
> Subject:  Re: [PATCH] net: sched: fix potential null pointer deref
>
> Hi Jianhao,
>
> On 6/10/22 4:14 AM, Jianhao Xu wrote:
> > mq_queue_get() may return NULL, a check is needed to avoid using
> > the NULL pointer.
> >
> > Signed-off-by: Jianhao Xu <jianhao_xu@smail.nju.edu.cn>
>
> Do you have a reproducer where this is triggered?
>
> > ---
> >   net/sched/sch_mq.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
> > index 83d2e54bf303..9aca4ca82947 100644
> > --- a/net/sched/sch_mq.c
> > +++ b/net/sched/sch_mq.c
> > @@ -201,6 +201,8 @@ static int mq_graft(struct Qdisc *sch, unsigned lon=
g cl, struct Qdisc *new,
> >   static struct Qdisc *mq_leaf(struct Qdisc *sch, unsigned long cl)
> >   {
> >  struct netdev_queue *dev_queue =3D mq_queue_get(sch, cl);
> > + if (!dev_queue)
> > +return NULL;
> >
> >  return dev_queue->qdisc_sleeping;
> >   }
> > @@ -218,6 +220,8 @@ static int mq_dump_class(struct Qdisc *sch, unsigne=
d long cl,
> >   struct sk_buff *skb, struct tcmsg *tcm)
> >   {
> >  struct netdev_queue *dev_queue =3D mq_queue_get(sch, cl);
> > + if (!dev_queue)
> > +return -1;
> >
> >  tcm->tcm_parent =3D TC_H_ROOT;
> >  tcm->tcm_handle |=3D TC_H_MIN(cl);
> > @@ -229,6 +233,8 @@ static int mq_dump_class_stats(struct Qdisc *sch, u=
nsigned long cl,
> >         struct gnet_dump *d)
> >   {
> >  struct netdev_queue *dev_queue =3D mq_queue_get(sch, cl);
> > + if (!dev_queue)
> > +return -1;
> >
> >  sch =3D dev_queue->qdisc_sleeping;
> >  if (gnet_stats_copy_basic(d, sch->cpu_bstats, &sch->bstats, true) < 0 =
||
> >
>