Re: [PATCH v2 net-next 10/15] net/sched: make stab available before ops->init() call

From: Eric Dumazet <edumazet@google.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Jamal Hadi Salim <jhs@mojatatu.com>,
	Cong Wang <xiyou.wangcong@gmail.com>,
	Jiri Pirko <jiri@resnulli.us>,
	Vinicius Costa Gomes <vinicius.gomes@intel.com>,
	Kurt Kanzenbach <kurt@linutronix.de>,
	Jacob Keller <jacob.e.keller@intel.com>,
	Gerhard Engleder <gerhard@engleder-embedded.com>,
	Jesse Brandeburg <jesse.brandeburg@intel.com>,
	Tony Nguyen <anthony.l.nguyen@intel.com>,
	intel-wired-lan@lists.osuosl.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 net-next 10/15] net/sched: make stab available before ops->init() call
Date: Fri, 10 Feb 2023 16:07:42 +0100	[thread overview]
Message-ID: <CANn89iL=Z8TOymdaBJ8WUBh8pXOgp_tKM3KVsQZ05uT3orOj4w@mail.gmail.com> (raw)
In-Reply-To: <20230207135440.1482856-11-vladimir.oltean@nxp.com>

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);
