| Message ID | 1504086545-7777-7-git-send-email-nikolay@cumulusnetworks.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net/sched: init failure fixes | expand |
On Wed, Aug 30, 2017 at 2:49 AM, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: > It is very unlikely to happen but the backlogs memory allocation > could fail and will free q->flows, but then ->destroy() will free > q->flows too. For correctness remove the first free and let ->destroy > clean up. > > Fixes: 87b60cfacf9f ("net_sched: fix error recovery at qdisc creation") > Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > --- > net/sched/sch_fq_codel.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c > index 337f2d6d81e4..2c0c05f2cc34 100644 > --- a/net/sched/sch_fq_codel.c > +++ b/net/sched/sch_fq_codel.c > @@ -491,10 +491,8 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt) > if (!q->flows) > return -ENOMEM; > q->backlogs = kvzalloc(q->flows_cnt * sizeof(u32), GFP_KERNEL); > - if (!q->backlogs) { > - kvfree(q->flows); > + if (!q->backlogs) > return -ENOMEM; > - } This is fine. Or we can NULL it after kvfree(). I have no preference here. The only difference here is if we still expect ->init() to cleanup its own failure.
On 30/08/17 20:36, Cong Wang wrote: > On Wed, Aug 30, 2017 at 2:49 AM, Nikolay Aleksandrov > <nikolay@cumulusnetworks.com> wrote: >> It is very unlikely to happen but the backlogs memory allocation >> could fail and will free q->flows, but then ->destroy() will free >> q->flows too. For correctness remove the first free and let ->destroy >> clean up. >> >> Fixes: 87b60cfacf9f ("net_sched: fix error recovery at qdisc creation") >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> >> --- >> net/sched/sch_fq_codel.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c >> index 337f2d6d81e4..2c0c05f2cc34 100644 >> --- a/net/sched/sch_fq_codel.c >> +++ b/net/sched/sch_fq_codel.c >> @@ -491,10 +491,8 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt) >> if (!q->flows) >> return -ENOMEM; >> q->backlogs = kvzalloc(q->flows_cnt * sizeof(u32), GFP_KERNEL); >> - if (!q->backlogs) { >> - kvfree(q->flows); >> + if (!q->backlogs) >> return -ENOMEM; >> - } > > This is fine. Or we can NULL it after kvfree(). > > I have no preference here. The only difference here is if we still > expect ->init() to cleanup its own failure. > We don't, that's the point of the changes that lead to these fixes, the way ->destroy() is used by both the default qdisc infra and the normal qdisc add suggest that it should clean up after ->init failure, thus the change.
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c index 337f2d6d81e4..2c0c05f2cc34 100644 --- a/net/sched/sch_fq_codel.c +++ b/net/sched/sch_fq_codel.c @@ -491,10 +491,8 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt) if (!q->flows) return -ENOMEM; q->backlogs = kvzalloc(q->flows_cnt * sizeof(u32), GFP_KERNEL); - if (!q->backlogs) { - kvfree(q->flows); + if (!q->backlogs) return -ENOMEM; - } for (i = 0; i < q->flows_cnt; i++) { struct fq_codel_flow *flow = q->flows + i;
It is very unlikely to happen but the backlogs memory allocation could fail and will free q->flows, but then ->destroy() will free q->flows too. For correctness remove the first free and let ->destroy clean up. Fixes: 87b60cfacf9f ("net_sched: fix error recovery at qdisc creation") Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> --- net/sched/sch_fq_codel.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)