[PATCH] net_sched: drr: check for NULL pointer in drr_dequeue

[PATCH] net_sched: drr: check for NULL pointer in drr_dequeue

[Bernie Harris]

There are cases where qdisc_dequeue_peeked can return NULL, and the result
is dereferenced later on in the function.

Similarly to the other qdisc dequeue functions, check whether the skb
pointer is NULL and if it is, goto out.

Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz>
---
 net/sched/sch_drr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index f26bdea..8086c3d 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -403,6 +403,8 @@ static struct sk_buff *drr_dequeue(struct Qdisc *sch)
 		if (len <= cl->deficit) {
 			cl->deficit -= len;
 			skb = qdisc_dequeue_peeked(cl->qdisc);
+			if (skb == NULL)
+				goto out;
 			if (cl->qdisc->q.qlen == 0)
 				list_del(&cl->alist);
 
-- 
2.7.0

Re: [PATCH] net_sched: drr: check for NULL pointer in drr_dequeue

[Cong Wang]

On Mon, Jan 25, 2016 at 7:51 PM, Bernie Harris
<bernie.harris@alliedtelesis.co.nz> wrote:
> There are cases where qdisc_dequeue_peeked can return NULL, and the result
> is dereferenced later on in the function.
>
> Similarly to the other qdisc dequeue functions, check whether the skb
> pointer is NULL and if it is, goto out.
>
> Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz>

Looks good to me, just one minor thing: an unlikely() can be used for this case.


> ---
>  net/sched/sch_drr.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
> index f26bdea..8086c3d 100644
> --- a/net/sched/sch_drr.c
> +++ b/net/sched/sch_drr.c
> @@ -403,6 +403,8 @@ static struct sk_buff *drr_dequeue(struct Qdisc *sch)
>                 if (len <= cl->deficit) {
>                         cl->deficit -= len;
>                         skb = qdisc_dequeue_peeked(cl->qdisc);
> +                       if (skb == NULL)
> +                               goto out;
>                         if (cl->qdisc->q.qlen == 0)
>                                 list_del(&cl->alist);
>
> --
> 2.7.0
>

Re: [PATCH] net_sched: drr: check for NULL pointer in drr_dequeue

[Bernie Harris]

Thank you. I will add unlikely() and re-submit the patch.

________________________________________
From: Cong Wang <xiyou.wangcong@gmail.com>
Sent: 27 January 2016 07:03
To: Bernie Harris
Cc: Linux Kernel Network Developers; David Miller; Jamal Hadi Salim; stable@vger.kernel.org
Subject: Re: [PATCH] net_sched: drr: check for NULL pointer in drr_dequeue

On Mon, Jan 25, 2016 at 7:51 PM, Bernie Harris
<bernie.harris@alliedtelesis.co.nz> wrote:
> There are cases where qdisc_dequeue_peeked can return NULL, and the result
> is dereferenced later on in the function.
>
> Similarly to the other qdisc dequeue functions, check whether the skb
> pointer is NULL and if it is, goto out.
>
> Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz>

Looks good to me, just one minor thing: an unlikely() can be used for this case.


> ---
>  net/sched/sch_drr.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
> index f26bdea..8086c3d 100644
> --- a/net/sched/sch_drr.c
> +++ b/net/sched/sch_drr.c
> @@ -403,6 +403,8 @@ static struct sk_buff *drr_dequeue(struct Qdisc *sch)
>                 if (len <= cl->deficit) {
>                         cl->deficit -= len;
>                         skb = qdisc_dequeue_peeked(cl->qdisc);
> +                       if (skb == NULL)
> +                               goto out;
>                         if (cl->qdisc->q.qlen == 0)
>                                 list_del(&cl->alist);
>
> --
> 2.7.0
>

[PATCH] net_sched: drr: check for NULL pointer in drr_dequeue

[Bernie Harris]

There are cases where qdisc_dequeue_peeked can return NULL, and the result
is dereferenced later on in the function.

Similarly to the other qdisc dequeue functions, check whether the skb
pointer is NULL and if it is, goto out.

Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz>
---
 net/sched/sch_drr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index f26bdea..a1cd778 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -403,6 +403,8 @@ static struct sk_buff *drr_dequeue(struct Qdisc *sch)
 		if (len <= cl->deficit) {
 			cl->deficit -= len;
 			skb = qdisc_dequeue_peeked(cl->qdisc);
+			if (unlikely(skb == NULL))
+				goto out;
 			if (cl->qdisc->q.qlen == 0)
 				list_del(&cl->alist);
 
-- 
2.7.0

Re: [PATCH] net_sched: drr: check for NULL pointer in drr_dequeue

[Sergei Shtylyov]

Hello.

On 1/28/2016 6:30 AM, Bernie Harris wrote:

> There are cases where qdisc_dequeue_peeked can return NULL, and the result
> is dereferenced later on in the function.
>
> Similarly to the other qdisc dequeue functions, check whether the skb
> pointer is NULL and if it is, goto out.
>
> Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz>
> ---
>   net/sched/sch_drr.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
> index f26bdea..a1cd778 100644
> --- a/net/sched/sch_drr.c
> +++ b/net/sched/sch_drr.c
> @@ -403,6 +403,8 @@ static struct sk_buff *drr_dequeue(struct Qdisc *sch)
>   		if (len <= cl->deficit) {
>   			cl->deficit -= len;
>   			skb = qdisc_dequeue_peeked(cl->qdisc);
> +			if (unlikely(skb == NULL))

    !skb is preferred in the networking code. I think scripts/checkpatch.pl 
should've warned you.

[...]

MBR, Sergei

Re: [PATCH] net_sched: drr: check for NULL pointer in drr_dequeue

[Cong Wang]

On Wed, Jan 27, 2016 at 7:30 PM, Bernie Harris
<bernie.harris@alliedtelesis.co.nz> wrote:
> There are cases where qdisc_dequeue_peeked can return NULL, and the result
> is dereferenced later on in the function.
>
> Similarly to the other qdisc dequeue functions, check whether the skb
> pointer is NULL and if it is, goto out.
>
> Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz>

Reviewed-by: Cong Wang <xiyou.wangcong@gmail.com>

Re: [PATCH] net_sched: drr: check for NULL pointer in drr_dequeue

[David Miller]

From: Bernie Harris <bernie.harris@alliedtelesis.co.nz>
Date: Thu, 28 Jan 2016 16:30:51 +1300

> There are cases where qdisc_dequeue_peeked can return NULL, and the result
> is dereferenced later on in the function.
> 
> Similarly to the other qdisc dequeue functions, check whether the skb
> pointer is NULL and if it is, goto out.
> 
> Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz>

Applied, thanks.