Messages in this thread | | | Date | Fri, 10 Feb 2023 17:20:58 +0200 | From | Vladimir Oltean <> | Subject | Re: [PATCH v2 net-next 10/15] net/sched: make stab available before ops->init() call |
| |
On Fri, Feb 10, 2023 at 04:07:42PM +0100, Eric Dumazet wrote: > On Tue, Feb 7, 2023 at 2:55 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > Move it earlier, which nicely seems to simplify the error handling path > > as well. > > Well... if you say so :)
:)
> If TCA_STAB attribute is malformed, we end up calling ->destroy() on a > not yet initialized qdisc :/
Right. Sorry, I didn't pay enough attention, and the old structure with "err_out4" having a "goto err_out3" confused me. Because I was trying to match the old teardown path with the new (linear) one, I was trying to keep the order between qdisc_put_stab() and ops->destroy(). But I forgot that I need to *reverse* it, since I reversed their order in the setup path :-/
> I am going to send the following fix, unless someone disagrees. > > (Moving qdisc_put_stab() _after_ ops->destroy(sch) is not strictly > needed for a fix, > but undo should be done in reverse steps for clarity.
Right, and it's a net-next patch, so a larger fix which brings the code into shape should be fine.
> > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c > index e9780631b5b58202068e20c42ccf1197eac2194c..aba789c30a2eb50d339b8a888495b794825e1775 > 100644 > --- a/net/sched/sch_api.c > +++ b/net/sched/sch_api.c > @@ -1286,7 +1286,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev, > stab = qdisc_get_stab(tca[TCA_STAB], extack); > if (IS_ERR(stab)) { > err = PTR_ERR(stab); > - goto err_out4; > + goto err_out3; > } > rcu_assign_pointer(sch->stab, stab); > } > @@ -1294,14 +1294,14 @@ static struct Qdisc *qdisc_create(struct > net_device *dev, > if (ops->init) { > err = ops->init(sch, tca[TCA_OPTIONS], extack); > if (err != 0) > - goto err_out5; > + goto err_out4; > } > > if (tca[TCA_RATE]) { > err = -EOPNOTSUPP; > if (sch->flags & TCQ_F_MQROOT) { > NL_SET_ERR_MSG(extack, "Cannot attach rate > estimator to a multi-queue root qdisc"); > - goto err_out5; > + goto err_out4; > } > > err = gen_new_estimator(&sch->bstats, > @@ -1312,7 +1312,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev, > tca[TCA_RATE]); > if (err) { > NL_SET_ERR_MSG(extack, "Failed to generate new > estimator"); > - goto err_out5; > + goto err_out4; > } > } > > @@ -1321,12 +1321,13 @@ static struct Qdisc *qdisc_create(struct > net_device *dev, > > return sch; > > -err_out5: > - qdisc_put_stab(rtnl_dereference(sch->stab)); > err_out4: > - /* ops->init() failed, we call ->destroy() like qdisc_create_dflt() */ > + /* Even if ops->init() failed, we call ops->destroy() > + * like qdisc_create_dflt(). > + */ > if (ops->destroy) > ops->destroy(sch); > + qdisc_put_stab(rtnl_dereference(sch->stab)); > err_out3: > netdev_put(dev, &sch->dev_tracker); > qdisc_free(sch);
I applied the changes from this patch manually, and the result looks good. Thanks (and sorry)!
| |