[net PATCH] net: sched: Fix one possible panic when no destroy callback

From: Gao Feng <redacted>

When qdisc fail to init, qdisc_create would invoke the destroy callback
to cleanup. But there is no check if the callback exists really. So it
would cause the panic if there is no real destroy callback like these
qdisc codel, pfifo, pfifo_fast, and so on.

Now add one the check for destroy to avoid the possible panic.

Signed-off-by: Gao Feng <redacted>
---
 net/sched/sch_api.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index e88342f..cfdbfa1 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c

@@ -1019,7 +1019,8 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
 		return sch;
 	}
 	/* ops->init() failed, we call ->destroy() like qdisc_create_dflt() */
-	ops->destroy(sch);
+	if (ops->destroy)
+		ops->destroy(sch);
 err_out3:
 	dev_put(dev);
 	kfree((char *) sch - sch->padded);

-- 
1.9.1

On Mon, Jun 26, 2017 at 6:35 PM,  [off-list ref] wrote:

From: Gao Feng <redacted>

When qdisc fail to init, qdisc_create would invoke the destroy callback
to cleanup. But there is no check if the callback exists really. So it
would cause the panic if there is no real destroy callback like these
qdisc codel, pfifo, pfifo_fast, and so on.

Now add one the check for destroy to avoid the possible panic.

Signed-off-by: Gao Feng <redacted>

Looks good,

Acked-by: Cong Wang <redacted>

This is introduced by commit 87b60cfacf9f17cf71933c6e33b.
Please add proper Fixes tag next time.

Thanks.

On Tue, 2017-06-27 at 09:30 -0700, Cong Wang wrote:

On Mon, Jun 26, 2017 at 6:35 PM,  [off-list ref] wrote:

quoted

From: Gao Feng <redacted>

When qdisc fail to init, qdisc_create would invoke the destroy callback
to cleanup. But there is no check if the callback exists really. So it
would cause the panic if there is no real destroy callback like these
qdisc codel, pfifo, pfifo_fast, and so on.

Now add one the check for destroy to avoid the possible panic.

Signed-off-by: Gao Feng <redacted>

Looks good,

Acked-by: Cong Wang <redacted>

This is introduced by commit 87b60cfacf9f17cf71933c6e33b.
Please add proper Fixes tag next time.

Given that pfifo, pfifo_fast or codel can not fail their init,
I do not see this patch as a net candidate, and the Fixes: tag seems not
needed.

Gao, have you really hit a bug, or is this patch some kind of cleanup or
prep work ?

If yes, please properly identify which packet scheduler had an issue.

Thanks.

On Tue, Jun 27, 2017 at 9:50 AM, Eric Dumazet [off-list ref] wrote:

On Tue, 2017-06-27 at 09:30 -0700, Cong Wang wrote:

quoted

On Mon, Jun 26, 2017 at 6:35 PM,  [off-list ref] wrote:

quoted

From: Gao Feng <redacted>

When qdisc fail to init, qdisc_create would invoke the destroy callback
to cleanup. But there is no check if the callback exists really. So it
would cause the panic if there is no real destroy callback like these
qdisc codel, pfifo, pfifo_fast, and so on.

Now add one the check for destroy to avoid the possible panic.

Signed-off-by: Gao Feng <redacted>

Looks good,

Acked-by: Cong Wang <redacted>

This is introduced by commit 87b60cfacf9f17cf71933c6e33b.
Please add proper Fixes tag next time.

Given that pfifo, pfifo_fast or codel can not fail their init,

How about codel_init() -> codel_change() -> nla_parse_nested() ?

I do not see this patch as a net candidate, and the Fixes: tag seems not
needed.

True, but we add Fixes tag to net-next candidates too.

On Tue, 2017-06-27 at 10:08 -0700, Cong Wang wrote:

On Tue, Jun 27, 2017 at 9:50 AM, Eric Dumazet [off-list ref] wrote:

quoted

On Tue, 2017-06-27 at 09:30 -0700, Cong Wang wrote:

quoted

On Mon, Jun 26, 2017 at 6:35 PM,  [off-list ref] wrote:

quoted

From: Gao Feng <redacted>

When qdisc fail to init, qdisc_create would invoke the destroy callback
to cleanup. But there is no check if the callback exists really. So it
would cause the panic if there is no real destroy callback like these
qdisc codel, pfifo, pfifo_fast, and so on.

Now add one the check for destroy to avoid the possible panic.

Signed-off-by: Gao Feng <redacted>

Looks good,

Acked-by: Cong Wang <redacted>

This is introduced by commit 87b60cfacf9f17cf71933c6e33b.
Please add proper Fixes tag next time.

Given that pfifo, pfifo_fast or codel can not fail their init,

How about codel_init() -> codel_change() -> nla_parse_nested() ?

Yeah, with a malicious user space then (iproute2/tc is fine), codel
could be problematic.

pfifo and pfifo_fast can definitely not hit this bug.

changelog needs a bit of attention, even if the bug is real.

Thanks.

At 2017-06-28 01:49:50, "Eric Dumazet" [off-list ref] wrote:

On Tue, 2017-06-27 at 10:08 -0700, Cong Wang wrote:

quoted

On Tue, Jun 27, 2017 at 9:50 AM, Eric Dumazet [off-list ref] wrote:

quoted

On Tue, 2017-06-27 at 09:30 -0700, Cong Wang wrote:

quoted

On Mon, Jun 26, 2017 at 6:35 PM,  [off-list ref] wrote:

quoted

From: Gao Feng <redacted>

When qdisc fail to init, qdisc_create would invoke the destroy callback
to cleanup. But there is no check if the callback exists really. So it
would cause the panic if there is no real destroy callback like these
qdisc codel, pfifo, pfifo_fast, and so on.

Now add one the check for destroy to avoid the possible panic.

Signed-off-by: Gao Feng <redacted>

Looks good,

Acked-by: Cong Wang <redacted>

This is introduced by commit 87b60cfacf9f17cf71933c6e33b.
Please add proper Fixes tag next time.

OK. Actually I didn't know it is introduced by this commit before :)
Need I send an update patch again ?

quoted

quoted

Given that pfifo, pfifo_fast or codel can not fail their init,

How about codel_init() -> codel_change() -> nla_parse_nested() ?

Yeah, with a malicious user space then (iproute2/tc is fine), codel
could be problematic.

pfifo and pfifo_fast can definitely not hit this bug.

changelog needs a bit of attention, even if the bug is real.

Thanks.

Yes, the codel could fail to init, and the fifo/pfifo could failed When "nla_len(opt) < sizeof(*ctl)".

Best Regards
Feng

On Tue, Jun 27, 2017 at 5:54 PM, Gao Feng [off-list ref] wrote:

At 2017-06-28 01:49:50, "Eric Dumazet" [off-list ref] wrote:

quoted

On Tue, 2017-06-27 at 10:08 -0700, Cong Wang wrote:

quoted

On Tue, Jun 27, 2017 at 9:50 AM, Eric Dumazet [off-list ref] wrote:

quoted

On Tue, 2017-06-27 at 09:30 -0700, Cong Wang wrote:

quoted

On Mon, Jun 26, 2017 at 6:35 PM,  [off-list ref] wrote:

quoted

From: Gao Feng <redacted>

When qdisc fail to init, qdisc_create would invoke the destroy callback
to cleanup. But there is no check if the callback exists really. So it
would cause the panic if there is no real destroy callback like these
qdisc codel, pfifo, pfifo_fast, and so on.

Now add one the check for destroy to avoid the possible panic.

Signed-off-by: Gao Feng <redacted>

Looks good,

Acked-by: Cong Wang <redacted>

This is introduced by commit 87b60cfacf9f17cf71933c6e33b.
Please add proper Fixes tag next time.

OK. Actually I didn't know it is introduced by this commit before :)
Need I send an update patch again ?

Yes please, update the changelog as Eric suggested and also
add Fixes.