Messages in this thread Patch in this message | | | From | Eric Dumazet <> | Date | Fri, 10 Feb 2023 16:07:42 +0100 | Subject | Re: [PATCH v2 net-next 10/15] net/sched: make stab available before ops->init() call |
| |
On Tue, Feb 7, 2023 at 2:55 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > Some qdiscs like taprio turn out to be actually pretty reliant on a well > configured stab, to not underestimate the skb transmission time (by > properly accounting for L1 overhead). > > In a future change, taprio will need the stab, if configured by the > user, to be available at ops->init() time. It will become even more > important in upcoming work, when the overhead will be used for the > queueMaxSDU calculation that is passed to an offloading driver. > > However, rcu_assign_pointer(sch->stab, stab) is called right after > ops->init(), making it unavailable, and I don't really see a good reason > for that. > > Move it earlier, which nicely seems to simplify the error handling path > as well.
Well... if you say so :)
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de> >
If TCA_STAB attribute is malformed, we end up calling ->destroy() on a not yet initialized qdisc :/
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.
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);
| |