| Message ID | OF7A403C69.2C35464E-ON48257D7B.002E24AA-48257D7B.002F2A62@zte.com.cn |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 10/24/2014 01:34 AM, wang.bo116@zte.com.cn wrote: > [...] > > -------------------------------------------------------------------------------- > > This patch fix this problem, base on linux 3.18-rc-1: > > Signed-off-by: Wang Bo <wang.bo116@zte.com.cn> > Tested-by: Ma Chenggong <ma.chenggong@zte.com.cn> > diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c > index 42f72f1..a0c90e7 100755 > --- a/net/sched/sch_mq.c > +++ b/net/sched/sch_mq.c > @@ -33,6 +33,7 @@ static void mq_destroy(struct Qdisc *sch) > for (ntx = 0; ntx < dev->num_tx_queues && priv->qdiscs[ntx]; ntx++) > qdisc_destroy(priv->qdiscs[ntx]); > kfree(priv->qdiscs); > + priv->qdiscs = NULL; > } > > static int mq_init(struct Qdisc *sch, struct nlattr *opt) > Acked-by: John Fastabend <john.r.fastabend@intel.com> Patch looks fine, another way to fix this would be drop the mq_destroy() call in the error path. I'm not convinced one is any better than the other but maybe some other folks have opinions, it seems a bit wrong to call mq_destroy twice so in that sense it may be a bit nicer to drop the mq_destroy(). Also same bug in mqprio do you want to submit a patch for that qdisc as well? Otherwise I can. Thanks!
On Fri, Oct 24, 2014 at 10:49 AM, John Fastabend <john.fastabend@gmail.com> wrote: > > Patch looks fine, another way to fix this would be drop the > mq_destroy() call in the error path. I'm not convinced one > is any better than the other but maybe some other folks have > opinions, it seems a bit wrong to call mq_destroy twice so in > that sense it may be a bit nicer to drop the mq_destroy(). Dropping mq_destroy() in error path is indeed better, because upper layer does cleanup intentionally. Look at what other qdisc's do. :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 24, 2014 at 11:13 AM, Cong Wang <cwang@twopensource.com> wrote: > On Fri, Oct 24, 2014 at 10:49 AM, John Fastabend > <john.fastabend@gmail.com> wrote: >> >> Patch looks fine, another way to fix this would be drop the >> mq_destroy() call in the error path. I'm not convinced one >> is any better than the other but maybe some other folks have >> opinions, it seems a bit wrong to call mq_destroy twice so in >> that sense it may be a bit nicer to drop the mq_destroy(). > > Dropping mq_destroy() in error path is indeed better, > because upper layer does cleanup intentionally. > Look at what other qdisc's do. :) Anyway, I am fixing other qdisc's now, will leave mq and mq_prio to you. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 24, 2014 at 11:13:56AM -0700, Cong Wang wrote: > On Fri, Oct 24, 2014 at 10:49 AM, John Fastabend > <john.fastabend@gmail.com> wrote: > > > > Patch looks fine, another way to fix this would be drop the > > mq_destroy() call in the error path. I'm not convinced one > > is any better than the other but maybe some other folks have > > opinions, it seems a bit wrong to call mq_destroy twice so in > > that sense it may be a bit nicer to drop the mq_destroy(). > > Dropping mq_destroy() in error path is indeed better, > because upper layer does cleanup intentionally. > Look at what other qdisc's do. :) I would argue that the qdisc_destroy() call in qdisc_create_dflt() is wrong, it should instead free the qdisc and release the module reference manually as done in qdisc_create(). qdisc_destroy() should only be called for fully initialized qdiscs. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 24, 2014 at 12:14 PM, Patrick McHardy <kaber@trash.net> wrote: > On Fri, Oct 24, 2014 at 11:13:56AM -0700, Cong Wang wrote: >> On Fri, Oct 24, 2014 at 10:49 AM, John Fastabend >> <john.fastabend@gmail.com> wrote: >> > >> > Patch looks fine, another way to fix this would be drop the >> > mq_destroy() call in the error path. I'm not convinced one >> > is any better than the other but maybe some other folks have >> > opinions, it seems a bit wrong to call mq_destroy twice so in >> > that sense it may be a bit nicer to drop the mq_destroy(). >> >> Dropping mq_destroy() in error path is indeed better, >> because upper layer does cleanup intentionally. >> Look at what other qdisc's do. :) > > I would argue that the qdisc_destroy() call in qdisc_create_dflt() > is wrong, it should instead free the qdisc and release the module > reference manually as done in qdisc_create(). > > qdisc_destroy() should only be called for fully initialized qdiscs. Probably, but at least ->destroy() should be called, looking at those calling qdisc_watchdog_init(), they are supposed to call qdisc_watchdog_cancel() when >init() fails after that. ->destroy() is supposed to be able to clean up even partially initialized qdisc's. So, for qdisc_create_dflt() we should probably just call ->destroy(). Reading the code again, seems it is inconsistent with qdisc_create(), where ->destroy() is skipped when ->init() fails. Hmm, we have a bigger problem here. I am working on a patch now. Thanks for pointing this out. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 24, 2014 at 01:52:00PM -0700, Cong Wang wrote: > On Fri, Oct 24, 2014 at 12:14 PM, Patrick McHardy <kaber@trash.net> wrote: > > On Fri, Oct 24, 2014 at 11:13:56AM -0700, Cong Wang wrote: > >> On Fri, Oct 24, 2014 at 10:49 AM, John Fastabend > >> <john.fastabend@gmail.com> wrote: > >> > > >> > Patch looks fine, another way to fix this would be drop the > >> > mq_destroy() call in the error path. I'm not convinced one > >> > is any better than the other but maybe some other folks have > >> > opinions, it seems a bit wrong to call mq_destroy twice so in > >> > that sense it may be a bit nicer to drop the mq_destroy(). > >> > >> Dropping mq_destroy() in error path is indeed better, > >> because upper layer does cleanup intentionally. > >> Look at what other qdisc's do. :) > > > > I would argue that the qdisc_destroy() call in qdisc_create_dflt() > > is wrong, it should instead free the qdisc and release the module > > reference manually as done in qdisc_create(). > > > > qdisc_destroy() should only be called for fully initialized qdiscs. > > Probably, but at least ->destroy() should be called, looking at > those calling qdisc_watchdog_init(), they are supposed to call > qdisc_watchdog_cancel() when >init() fails after that. In which cases does it actually fail after that? Usually this is called once initialization is complete. > ->destroy() is supposed to be able to clean up even partially > initialized qdisc's. So, for qdisc_create_dflt() we should probably > just call ->destroy(). No, why do you think that? ->init() is supposed to clean up after itself if it fails, both qdisc_destroy() and ->destroy are only supposed to be called after ->init() succeeded. Its simply symetrical, as everywhere else in the kernel. If a sub-init funtion fails, it should clean up and return an error. We don't destroy things we've never successfully initialized, they're supposed to clean up after themselves. > Reading the code again, seems it is inconsistent with qdisc_create(), > where ->destroy() is skipped when ->init() fails. Hmm, we have > a bigger problem here. > > I am working on a patch now. Thanks for pointing this out. No, that is not inconsistent, it is consistent with what we're doing everywhere else in the kernel, what the qdisc layer has always done and how qdiscs expect it. Where are you seeing a problem now? Its a simple fix: qdisc_create_dflt() should behave similar to qdisc_create(). If any ->init() functions than don't properly clean up on error, that's an additional bug. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 24, 2014 at 2:45 PM, Patrick McHardy <kaber@trash.net> wrote: > On Fri, Oct 24, 2014 at 01:52:00PM -0700, Cong Wang wrote: >> On Fri, Oct 24, 2014 at 12:14 PM, Patrick McHardy <kaber@trash.net> wrote: >> > On Fri, Oct 24, 2014 at 11:13:56AM -0700, Cong Wang wrote: >> >> On Fri, Oct 24, 2014 at 10:49 AM, John Fastabend >> >> <john.fastabend@gmail.com> wrote: >> >> > >> >> > Patch looks fine, another way to fix this would be drop the >> >> > mq_destroy() call in the error path. I'm not convinced one >> >> > is any better than the other but maybe some other folks have >> >> > opinions, it seems a bit wrong to call mq_destroy twice so in >> >> > that sense it may be a bit nicer to drop the mq_destroy(). >> >> >> >> Dropping mq_destroy() in error path is indeed better, >> >> because upper layer does cleanup intentionally. >> >> Look at what other qdisc's do. :) >> > >> > I would argue that the qdisc_destroy() call in qdisc_create_dflt() >> > is wrong, it should instead free the qdisc and release the module >> > reference manually as done in qdisc_create(). >> > >> > qdisc_destroy() should only be called for fully initialized qdiscs. >> >> Probably, but at least ->destroy() should be called, looking at >> those calling qdisc_watchdog_init(), they are supposed to call >> qdisc_watchdog_cancel() when >init() fails after that. > > In which cases does it actually fail after that? Usually this is > called once initialization is complete. How about tbf_change() in tbf_init()? If tbf_change() fails, watchdog is still there if we don't call ->destroy(). Yes, I know the timer is started, the point is we do miss something clean up, even trivial. tbf is not the only one who calls xxx_change() in xxx_init(). > >> ->destroy() is supposed to be able to clean up even partially >> initialized qdisc's. So, for qdisc_create_dflt() we should probably >> just call ->destroy(). > > No, why do you think that? ->init() is supposed to clean up after > itself if it fails, both qdisc_destroy() and ->destroy are only > supposed to be called after ->init() succeeded. Why? This would end up with calling xxx_destroy() in every failure path in ->init(). Moving it up to caller makes the code cleaner and smaller. > > Its simply symetrical, as everywhere else in the kernel. If a sub-init > funtion fails, it should clean up and return an error. We don't > destroy things we've never successfully initialized, they're supposed > to clean up after themselves. Most (if not all) ->destroy() are able to clean partially initialized qdisc, I don't see why it could be a problem here. We don't have to keep with other kernel subsystem as long as it makes sense, net_sched subsystem is pretty much self-contained. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 24, 2014 at 03:17:41PM -0700, Cong Wang wrote: > On Fri, Oct 24, 2014 at 2:45 PM, Patrick McHardy <kaber@trash.net> wrote: > >> > > >> > I would argue that the qdisc_destroy() call in qdisc_create_dflt() > >> > is wrong, it should instead free the qdisc and release the module > >> > reference manually as done in qdisc_create(). > >> > > >> > qdisc_destroy() should only be called for fully initialized qdiscs. > >> > >> Probably, but at least ->destroy() should be called, looking at > >> those calling qdisc_watchdog_init(), they are supposed to call > >> qdisc_watchdog_cancel() when >init() fails after that. > > > > In which cases does it actually fail after that? Usually this is > > called once initialization is complete. > > How about tbf_change() in tbf_init()? If tbf_change() fails, > watchdog is still there if we don't call ->destroy(). Yes, > I know the timer is started, the point is we do miss something > clean up, even trivial. > > tbf is not the only one who calls xxx_change() in xxx_init(). Then these are bugs. On failure we exit with the same state as we entered, there's nothing new about that. This in fact is *no* bug though since qdisc_watchdog_init() merely initializes the timer and doesn't require cleanup. > > Its simply symetrical, as everywhere else in the kernel. If a sub-init > > funtion fails, it should clean up and return an error. We don't > > destroy things we've never successfully initialized, they're supposed > > to clean up after themselves. > > Most (if not all) ->destroy() are able to clean partially initialized qdisc, > I don't see why it could be a problem here. > > We don't have to keep with other kernel subsystem as long as it makes > sense, net_sched subsystem is pretty much self-contained. Its about having a sane API. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 24, 2014 at 5:33 PM, Patrick McHardy <kaber@trash.net> wrote: > > Its about having a sane API. I don't see why calling ->destroy() on failure is not sane in qdisc case. I never want to argue general case. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 24, 2014 at 05:57:59PM -0700, Cong Wang wrote: > On Fri, Oct 24, 2014 at 5:33 PM, Patrick McHardy <kaber@trash.net> wrote: > > > > Its about having a sane API. > > I don't see why calling ->destroy() on failure is not sane in qdisc case. > I never want to argue general case. Because it makes things more complicated. You need to keep track of what was actually initialized since you can't assume a consistent state in ->destroy() anymore. If ->init() fails, it knows where it failed, ->destroy() can't know that. Look at htb_destroy() for an example. It starts with cancel_work_sync(&q->work); Was that actually initialized and can be cancled? You don't know. Next comes qdisc_watchdog_cancel(&q->watchdog); Same here, if the error happened before it was initialized, crash. These are just the first two lines. You get the problem. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c index 42f72f1..a0c90e7 100755 --- a/net/sched/sch_mq.c +++ b/net/sched/sch_mq.c @@ -33,6 +33,7 @@ static void mq_destroy(struct Qdisc *sch) for (ntx = 0; ntx < dev->num_tx_queues && priv->qdiscs[ntx]; ntx++) qdisc_destroy(priv->qdiscs[ntx]); kfree(priv->qdiscs); + priv->qdiscs = NULL; } static int mq_init(struct Qdisc *sch, struct nlattr *opt)