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

[Cong Wang]

On Tue, Jun 27, 2017 at 5:54 PM, Gao Feng  wrote:
> At 2017-06-28 01:49:50, "Eric Dumazet"  wrote:
>>On Tue, 2017-06-27 at 10:08 -0700, Cong Wang wrote:
>>> On Tue, Jun 27, 2017 at 9:50 AM, Eric Dumazet  
>>> wrote:
>>> > On Tue, 2017-06-27 at 09:30 -0700, Cong Wang wrote:
>>> >> On Mon, Jun 26, 2017 at 6:35 PM,   wrote:
>>> >> > From: Gao Feng 
>>> >> >
>>> >> > 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 
>>> >>
>>> >> Looks good,
>>> >>
>>> >> Acked-by: Cong Wang 
>>> >>
>>> >> 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.

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

[Eric Dumazet]

On Tue, 2017-06-27 at 10:08 -0700, Cong Wang wrote:
> On Tue, Jun 27, 2017 at 9:50 AM, Eric Dumazet  wrote:
> > On Tue, 2017-06-27 at 09:30 -0700, Cong Wang wrote:
> >> On Mon, Jun 26, 2017 at 6:35 PM,   wrote:
> >> > From: Gao Feng 
> >> >
> >> > 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 
> >>
> >> Looks good,
> >>
> >> Acked-by: Cong Wang 
> >>
> >> 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.

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

[Cong Wang]

On Tue, Jun 27, 2017 at 9:50 AM, Eric Dumazet  wrote:
> On Tue, 2017-06-27 at 09:30 -0700, Cong Wang wrote:
>> On Mon, Jun 26, 2017 at 6:35 PM,   wrote:
>> > From: Gao Feng 
>> >
>> > 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 
>>
>> Looks good,
>>
>> Acked-by: Cong Wang 
>>
>> 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.

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

[Eric Dumazet]

On Tue, 2017-06-27 at 09:30 -0700, Cong Wang wrote:
> On Mon, Jun 26, 2017 at 6:35 PM,   wrote:
> > From: Gao Feng 
> >
> > 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 
> 
> Looks good,
> 
> Acked-by: Cong Wang 
> 
> 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.

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

[Cong Wang]

On Mon, Jun 26, 2017 at 6:35 PM,   wrote:
> From: Gao Feng 
>
> 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 

Looks good,

Acked-by: Cong Wang 

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

Thanks.