[Patch net-next] net_sched: properly check for empty skb array on error path

[Patch net-next] net_sched: properly check for empty skb array on error path

[Cong Wang]

First, the check of &q->ring.queue against NULL is wrong, it
is always false. We should check the value rather than the address.

Secondly, we need the same check in pfifo_fast_reset() too,
as both ->reset() and ->destroy() are called in qdisc_destroy().

Fixes: c5ad119fb6c0 ("net: sched: pfifo_fast use skb_array")
Reported-by: syzbot <syzkaller@googlegroups.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/sch_generic.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 981c08fe810b..876fab2604b8 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -659,6 +659,12 @@ static void pfifo_fast_reset(struct Qdisc *qdisc)
 		struct skb_array *q = band2list(priv, band);
 		struct sk_buff *skb;
 
+		/* NULL ring is possible if destroy path is due to a failed
+		 * skb_array_init() in pfifo_fast_init() case.
+		 */
+		if (!q->ring.queue)
+			continue;
+
 		while ((skb = skb_array_consume_bh(q)) != NULL)
 			kfree_skb(skb);
 	}
@@ -719,7 +725,7 @@ static void pfifo_fast_destroy(struct Qdisc *sch)
 		/* NULL ring is possible if destroy path is due to a failed
 		 * skb_array_init() in pfifo_fast_init() case.
 		 */
-		if (!&q->ring.queue)
+		if (!q->ring.queue)
 			continue;
 		/* Destroy ring but no need to kfree_skb because a call to
 		 * pfifo_fast_reset() has already done that work.
-- 
2.13.0

Re: [Patch net-next] net_sched: properly check for empty skb array on error path

[John Fastabend]

On 12/18/2017 02:34 PM, Cong Wang wrote:
> First, the check of &q->ring.queue against NULL is wrong, it
> is always false. We should check the value rather than the address.
> 

Thanks.

> Secondly, we need the same check in pfifo_fast_reset() too,
> as both ->reset() and ->destroy() are called in qdisc_destroy().
> 

not that it hurts to have the check here, but if init fails
in qdisc_create it seems only ->destroy() is called without
a ->reset().

Is there another path for init() to fail that I'm missing.

> Fixes: c5ad119fb6c0 ("net: sched: pfifo_fast use skb_array")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  net/sched/sch_generic.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 

Re: [Patch net-next] net_sched: properly check for empty skb array on error path

[Cong Wang]

On Mon, Dec 18, 2017 at 5:25 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> On 12/18/2017 02:34 PM, Cong Wang wrote:
>> First, the check of &q->ring.queue against NULL is wrong, it
>> is always false. We should check the value rather than the address.
>>
>
> Thanks.
>
>> Secondly, we need the same check in pfifo_fast_reset() too,
>> as both ->reset() and ->destroy() are called in qdisc_destroy().
>>
>
> not that it hurts to have the check here, but if init fails
> in qdisc_create it seems only ->destroy() is called without
> a ->reset().
>
> Is there another path for init() to fail that I'm missing.

Pretty sure ->reset() is called in qdisc_destroy() and also before
->destroy():


void qdisc_destroy(struct Qdisc *qdisc)
{
        const struct Qdisc_ops  *ops = qdisc->ops;
        struct sk_buff *skb, *tmp;

        if (qdisc->flags & TCQ_F_BUILTIN ||
            !refcount_dec_and_test(&qdisc->refcnt))
                return;

#ifdef CONFIG_NET_SCHED
        qdisc_hash_del(qdisc);

        qdisc_put_stab(rtnl_dereference(qdisc->stab));
#endif
        gen_kill_estimator(&qdisc->rate_est);
        if (ops->reset)
                ops->reset(qdisc);
        if (ops->destroy)
                ops->destroy(qdisc);

Re: [Patch net-next] net_sched: properly check for empty skb array on error path

[John Fastabend]

On 12/18/2017 06:20 PM, Cong Wang wrote:
> On Mon, Dec 18, 2017 at 5:25 PM, John Fastabend
> <john.fastabend@gmail.com> wrote:
>> On 12/18/2017 02:34 PM, Cong Wang wrote:
>>> First, the check of &q->ring.queue against NULL is wrong, it
>>> is always false. We should check the value rather than the address.
>>>
>>
>> Thanks.
>>
>>> Secondly, we need the same check in pfifo_fast_reset() too,
>>> as both ->reset() and ->destroy() are called in qdisc_destroy().
>>>
>>
>> not that it hurts to have the check here, but if init fails
>> in qdisc_create it seems only ->destroy() is called without
>> a ->reset().
>>
>> Is there another path for init() to fail that I'm missing.
> 
> Pretty sure ->reset() is called in qdisc_destroy() and also before
> ->destroy():
> 

Except, the failed init path does not call qdisc_destroy.

static struct Qdisc *qdisc_create(struct net_device *dev,
[...]

        if (ops->init) {
                err = ops->init(sch, tca[TCA_OPTIONS]);
                if (err != 0)
                        goto err_out5;
        }
[...]

err_out5:
        /* ops->init() failed, we call ->destroy() like qdisc_create_dflt() */
        if (ops->destroy)
                ops->destroy(sch);
err_out3:
        dev_put(dev);
        kfree((char *) sch - sch->padded);
err_out2:
        module_put(ops->owner);
err_out:
        *errp = err;
        return NULL;
[...]



> 
> void qdisc_destroy(struct Qdisc *qdisc)
> {
>         const struct Qdisc_ops  *ops = qdisc->ops;
>         struct sk_buff *skb, *tmp;
> 
>         if (qdisc->flags & TCQ_F_BUILTIN ||
>             !refcount_dec_and_test(&qdisc->refcnt))
>                 return;
> 
> #ifdef CONFIG_NET_SCHED
>         qdisc_hash_del(qdisc);
> 
>         qdisc_put_stab(rtnl_dereference(qdisc->stab));
> #endif
>         gen_kill_estimator(&qdisc->rate_est);
>         if (ops->reset)
>                 ops->reset(qdisc);
>         if (ops->destroy)
>                 ops->destroy(qdisc);
> 

Re: [Patch net-next] net_sched: properly check for empty skb array on error path

[Cong Wang]

On Mon, Dec 18, 2017 at 7:58 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> On 12/18/2017 06:20 PM, Cong Wang wrote:
>> On Mon, Dec 18, 2017 at 5:25 PM, John Fastabend
>> <john.fastabend@gmail.com> wrote:
>>> On 12/18/2017 02:34 PM, Cong Wang wrote:
>>>> First, the check of &q->ring.queue against NULL is wrong, it
>>>> is always false. We should check the value rather than the address.
>>>>
>>>
>>> Thanks.
>>>
>>>> Secondly, we need the same check in pfifo_fast_reset() too,
>>>> as both ->reset() and ->destroy() are called in qdisc_destroy().
>>>>
>>>
>>> not that it hurts to have the check here, but if init fails
>>> in qdisc_create it seems only ->destroy() is called without
>>> a ->reset().
>>>
>>> Is there another path for init() to fail that I'm missing.
>>
>> Pretty sure ->reset() is called in qdisc_destroy() and also before
>> ->destroy():
>>
>
> Except, the failed init path does not call qdisc_destroy.
>
> static struct Qdisc *qdisc_create(struct net_device *dev,
> [...]
>
>         if (ops->init) {
>                 err = ops->init(sch, tca[TCA_OPTIONS]);
>                 if (err != 0)
>                         goto err_out5;
>         }
> [...]
>
> err_out5:
>         /* ops->init() failed, we call ->destroy() like qdisc_create_dflt() */
>         if (ops->destroy)
>                 ops->destroy(sch);

Didn't I say qdisc_destroy() rather than ->destroy()? :-)


struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,
                                const struct Qdisc_ops *ops,
                                unsigned int parentid)
{
        struct Qdisc *sch;

        if (!try_module_get(ops->owner))
                return NULL;

        sch = qdisc_alloc(dev_queue, ops);
        if (IS_ERR(sch)) {
                module_put(ops->owner);
                return NULL;
        }
        sch->parent = parentid;

        if (!ops->init || ops->init(sch, NULL) == 0)
                return sch;

        qdisc_destroy(sch);
        return NULL;
}

Re: [Patch net-next] net_sched: properly check for empty skb array on error path

[John Fastabend]

On 12/18/2017 08:31 PM, Cong Wang wrote:
> On Mon, Dec 18, 2017 at 7:58 PM, John Fastabend
> <john.fastabend@gmail.com> wrote:
>> On 12/18/2017 06:20 PM, Cong Wang wrote:
>>> On Mon, Dec 18, 2017 at 5:25 PM, John Fastabend
>>> <john.fastabend@gmail.com> wrote:
>>>> On 12/18/2017 02:34 PM, Cong Wang wrote:
>>>>> First, the check of &q->ring.queue against NULL is wrong, it
>>>>> is always false. We should check the value rather than the address.
>>>>>
>>>>
>>>> Thanks.
>>>>
>>>>> Secondly, we need the same check in pfifo_fast_reset() too,
>>>>> as both ->reset() and ->destroy() are called in qdisc_destroy().
>>>>>
>>>>
>>>> not that it hurts to have the check here, but if init fails
>>>> in qdisc_create it seems only ->destroy() is called without
>>>> a ->reset().
>>>>
>>>> Is there another path for init() to fail that I'm missing.
>>>
>>> Pretty sure ->reset() is called in qdisc_destroy() and also before
>>> ->destroy():
>>>
>>
>> Except, the failed init path does not call qdisc_destroy.
>>
>> static struct Qdisc *qdisc_create(struct net_device *dev,
>> [...]
>>
>>         if (ops->init) {
>>                 err = ops->init(sch, tca[TCA_OPTIONS]);
>>                 if (err != 0)
>>                         goto err_out5;
>>         }
>> [...]
>>
>> err_out5:
>>         /* ops->init() failed, we call ->destroy() like qdisc_create_dflt() */
>>         if (ops->destroy)
>>                 ops->destroy(sch);
> 
> Didn't I say qdisc_destroy() rather than ->destroy()? :-)
> 

Yep, thanks for the fix.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [Patch net-next] net_sched: properly check for empty skb array on error path

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 18 Dec 2017 14:34:26 -0800

> First, the check of &q->ring.queue against NULL is wrong, it
> is always false. We should check the value rather than the address.
> 
> Secondly, we need the same check in pfifo_fast_reset() too,
> as both ->reset() and ->destroy() are called in qdisc_destroy().
> 
> Fixes: c5ad119fb6c0 ("net: sched: pfifo_fast use skb_array")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied, thanks Cong.