diff mbox series

[v4,net-next,5/9] net/sched: pass netlink extack to mqprio and taprio offload

Message ID 20230403103440.2895683-6-vladimir.oltean@nxp.com
State New
Headers
Series Add tc-mqprio and tc-taprio support for preemptible traffic classes |

Commit Message

Vladimir Oltean April 3, 2023, 10:34 a.m. UTC 
  With the multiplexed ndo_setup_tc() model which lacks a first-class
struct netlink_ext_ack * argument, the only way to pass the netlink
extended ACK message down to the device driver is to embed it within the
offload structure.

Do this for struct tc_mqprio_qopt_offload and struct tc_taprio_qopt_offload.

Since struct tc_taprio_qopt_offload also contains a tc_mqprio_qopt_offload
structure, and since device drivers might effectively reuse their mqprio
implementation for the mqprio portion of taprio, we make taprio set the
extack in both offload structures to point at the same netlink extack
message.

In fact, the taprio handling is a bit more tricky, for 2 reasons.

First is because the offload structure has a longer lifetime than the
extack structure. The driver is supposed to populate the extack
synchronously from ndo_setup_tc() and leave it alone afterwards.
To not have any use-after-free surprises, we zero out the extack pointer
when we leave taprio_enable_offload().

The second reason is because taprio does overwrite the extack message on
ndo_setup_tc() error. We need to switch to the weak form of setting an
extack message, which preserves a potential message set by the driver.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
---
v3->v4: none
v2->v3: patch is new

 include/net/pkt_sched.h |  2 ++
 net/sched/sch_mqprio.c  |  5 ++++-
 net/sched/sch_taprio.c  | 12 ++++++++++--
 3 files changed, 16 insertions(+), 3 deletions(-)

Comments

Jamal Hadi Salim April 7, 2023, 4:09 p.m. UTC | #1 
On Mon, Apr 3, 2023 at 6:35 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> With the multiplexed ndo_setup_tc() model which lacks a first-class
> struct netlink_ext_ack * argument, the only way to pass the netlink
> extended ACK message down to the device driver is to embed it within the
> offload structure.
>
> Do this for struct tc_mqprio_qopt_offload and struct tc_taprio_qopt_offload.
>
> Since struct tc_taprio_qopt_offload also contains a tc_mqprio_qopt_offload
> structure, and since device drivers might effectively reuse their mqprio
> implementation for the mqprio portion of taprio, we make taprio set the
> extack in both offload structures to point at the same netlink extack
> message.
>
> In fact, the taprio handling is a bit more tricky, for 2 reasons.
>
> First is because the offload structure has a longer lifetime than the
> extack structure. The driver is supposed to populate the extack
> synchronously from ndo_setup_tc() and leave it alone afterwards.
> To not have any use-after-free surprises, we zero out the extack pointer
> when we leave taprio_enable_offload().
>
> The second reason is because taprio does overwrite the extack message on
> ndo_setup_tc() error. We need to switch to the weak form of setting an
> extack message, which preserves a potential message set by the driver.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

> ---
> v3->v4: none
> v2->v3: patch is new
>
>  include/net/pkt_sched.h |  2 ++
>  net/sched/sch_mqprio.c  |  5 ++++-
>  net/sched/sch_taprio.c  | 12 ++++++++++--
>  3 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
> index bb0bd69fb655..b43ed4733455 100644
> --- a/include/net/pkt_sched.h
> +++ b/include/net/pkt_sched.h
> @@ -166,6 +166,7 @@ struct tc_mqprio_caps {
>  struct tc_mqprio_qopt_offload {
>         /* struct tc_mqprio_qopt must always be the first element */
>         struct tc_mqprio_qopt qopt;
> +       struct netlink_ext_ack *extack;
>         u16 mode;
>         u16 shaper;
>         u32 flags;
> @@ -193,6 +194,7 @@ struct tc_taprio_sched_entry {
>
>  struct tc_taprio_qopt_offload {
>         struct tc_mqprio_qopt_offload mqprio;
> +       struct netlink_ext_ack *extack;
>         u8 enable;
>         ktime_t base_time;
>         u64 cycle_time;
> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
> index 9ee5a9a9b9e9..5287ff60b3f9 100644
> --- a/net/sched/sch_mqprio.c
> +++ b/net/sched/sch_mqprio.c
> @@ -33,9 +33,12 @@ static int mqprio_enable_offload(struct Qdisc *sch,
>                                  const struct tc_mqprio_qopt *qopt,
>                                  struct netlink_ext_ack *extack)
>  {
> -       struct tc_mqprio_qopt_offload mqprio = {.qopt = *qopt};
>         struct mqprio_sched *priv = qdisc_priv(sch);
>         struct net_device *dev = qdisc_dev(sch);
> +       struct tc_mqprio_qopt_offload mqprio = {
> +               .qopt = *qopt,
> +               .extack = extack,
> +       };
>         int err, i;
>
>         switch (priv->mode) {
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 1f469861eae3..cbad43019172 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1520,7 +1520,9 @@ static int taprio_enable_offload(struct net_device *dev,
>                 return -ENOMEM;
>         }
>         offload->enable = 1;
> +       offload->extack = extack;
>         mqprio_qopt_reconstruct(dev, &offload->mqprio.qopt);
> +       offload->mqprio.extack = extack;
>         taprio_sched_to_offload(dev, sched, offload, &caps);
>
>         for (tc = 0; tc < TC_MAX_QUEUE; tc++)
> @@ -1528,14 +1530,20 @@ static int taprio_enable_offload(struct net_device *dev,
>
>         err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
>         if (err < 0) {
> -               NL_SET_ERR_MSG(extack,
> -                              "Device failed to setup taprio offload");
> +               NL_SET_ERR_MSG_WEAK(extack,
> +                                   "Device failed to setup taprio offload");
>                 goto done;
>         }
>
>         q->offloaded = true;
>
>  done:
> +       /* The offload structure may linger around via a reference taken by the
> +        * device driver, so clear up the netlink extack pointer so that the
> +        * driver isn't tempted to dereference data which stopped being valid
> +        */
> +       offload->extack = NULL;
> +       offload->mqprio.extack = NULL;
>         taprio_offload_free(offload);
>
>         return err;
> --
> 2.34.1
>
diff mbox series

Patch

diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index bb0bd69fb655..b43ed4733455 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -166,6 +166,7 @@  struct tc_mqprio_caps {
 struct tc_mqprio_qopt_offload {
 	/* struct tc_mqprio_qopt must always be the first element */
 	struct tc_mqprio_qopt qopt;
+	struct netlink_ext_ack *extack;
 	u16 mode;
 	u16 shaper;
 	u32 flags;
@@ -193,6 +194,7 @@  struct tc_taprio_sched_entry {
 
 struct tc_taprio_qopt_offload {
 	struct tc_mqprio_qopt_offload mqprio;
+	struct netlink_ext_ack *extack;
 	u8 enable;
 	ktime_t base_time;
 	u64 cycle_time;
diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index 9ee5a9a9b9e9..5287ff60b3f9 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c
@@ -33,9 +33,12 @@  static int mqprio_enable_offload(struct Qdisc *sch,
 				 const struct tc_mqprio_qopt *qopt,
 				 struct netlink_ext_ack *extack)
 {
-	struct tc_mqprio_qopt_offload mqprio = {.qopt = *qopt};
 	struct mqprio_sched *priv = qdisc_priv(sch);
 	struct net_device *dev = qdisc_dev(sch);
+	struct tc_mqprio_qopt_offload mqprio = {
+		.qopt = *qopt,
+		.extack = extack,
+	};
 	int err, i;
 
 	switch (priv->mode) {
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 1f469861eae3..cbad43019172 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1520,7 +1520,9 @@  static int taprio_enable_offload(struct net_device *dev,
 		return -ENOMEM;
 	}
 	offload->enable = 1;
+	offload->extack = extack;
 	mqprio_qopt_reconstruct(dev, &offload->mqprio.qopt);
+	offload->mqprio.extack = extack;
 	taprio_sched_to_offload(dev, sched, offload, &caps);
 
 	for (tc = 0; tc < TC_MAX_QUEUE; tc++)
@@ -1528,14 +1530,20 @@  static int taprio_enable_offload(struct net_device *dev,
 
 	err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
 	if (err < 0) {
-		NL_SET_ERR_MSG(extack,
-			       "Device failed to setup taprio offload");
+		NL_SET_ERR_MSG_WEAK(extack,
+				    "Device failed to setup taprio offload");
 		goto done;
 	}
 
 	q->offloaded = true;
 
 done:
+	/* The offload structure may linger around via a reference taken by the
+	 * device driver, so clear up the netlink extack pointer so that the
+	 * driver isn't tempted to dereference data which stopped being valid
+	 */
+	offload->extack = NULL;
+	offload->mqprio.extack = NULL;
 	taprio_offload_free(offload);
 
 	return err;