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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iL=Z8TOymdaBJ8WUBh8pXOgp_tKM3KVsQZ05uT3orOj4w@mail.gmail.com>
Date:   Fri, 10 Feb 2023 16:07:42 +0100
From:   Eric Dumazet <edumazet@...gle.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Jacob Keller <jacob.e.keller@...el.com>,
        Gerhard Engleder <gerhard@...leder-embedded.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Tony Nguyen <anthony.l.nguyen@...el.com>,
        intel-wired-lan@...ts.osuosl.org, linux-kernel@...r.kernel.org
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@....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@....com>
> Reviewed-by: Kurt Kanzenbach <kurt@...utronix.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);