sch_htb: Fix inconsistency when leaf qdisc creation fails

PATCH Applied Maxim Mikityanskiy 4 participants 4 messages Aug 26, 2021 – Aug 30, 2021

Subsystems (5)

Networking Network drivers Traffic control (tc) Mellanox mlx5 core +1 more subsystems

lore

Share & tools

lore

Maxim Mikityanskiy

author

2021-08-26 11:54 UTC #0 lore raw

PATCH 1/1 sch_htb: Fix inconsistency when leaf qdisc creation fails

In HTB offload mode, qdiscs of leaf classes are grafted to netdev queues. sch_htb expects the dev_queue field of these qdiscs to point to the corresponding queues. However, qdisc creation may fail, and in that case noop_qdisc is used instead. Its dev_queue doesn't point to the right queue, so sch_htb can lose track of used netdev queues, which will cause internal inconsistencies.

This commit fixes this bug by keeping track of the netdev queue inside struct htb_class. All reads of cl->leaf.q->dev_queue are replaced by the new field, the two values are synced on writes, and WARNs are added to assert equality of the two values.

The driver API has changed: when TC_HTB_LEAF_DEL needs to move a queue, the driver used to pass the old and new queue IDs to sch_htb. Now that there is a new field (offload_queue) in struct htb_class that needs to be updated on this operation, the driver will pass the old class ID to sch_htb instead (it already knows the new class ID).

Fixes d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")

Signed-off-by Maxim Mikityanskiy <[email protected]>

Reviewed-by Tariq Toukan <[email protected]>

---
 .../net/ethernet/mellanox/mlx5/core/en/qos.c  | 15 ++-
 .../net/ethernet/mellanox/mlx5/core/en/qos.h  |  4 +-
 .../net/ethernet/mellanox/mlx5/core/en_main.c |  3 +-
 include/net/pkt_cls.h                         |  3 +-
 net/sched/sch_htb.c                           | 97 ++++++++++++-------
 5 files changed, 72 insertions(+), 50 deletions(-)

drivers/net/ethernet/mellanox/mlx5/core/en/qos.c +6 -9

index 5efe3278b0f6..1fd8baf19829 100644
@@ -733,8 +733,8 @@ static void mlx5e_reset_qdisc(struct net_device *dev, u16 qid)
 	spin_unlock_bh(qdisc_lock(qdisc));
 }
 
-int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 classid, u16 *old_qid,
-		       u16 *new_qid, struct netlink_ext_ack *extack)
+int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 *classid,
+		       struct netlink_ext_ack *extack)
 {
 	struct mlx5e_qos_node *node;
 	struct netdev_queue *txq;

@@ -742,11 +742,9 @@ int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 classid, u16 *old_qid,
 	bool opened;
 	int err;
 
-	qos_dbg(priv->mdev, "TC_HTB_LEAF_DEL classid %04x\n", classid);
-
-	*old_qid = *new_qid = 0;
+	qos_dbg(priv->mdev, "TC_HTB_LEAF_DEL classid %04x\n", *classid);
 
-	node = mlx5e_sw_node_find(priv, classid);
+	node = mlx5e_sw_node_find(priv, *classid);
 	if (!node)
 		return -ENOENT;

@@ -764,7 +762,7 @@ int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 classid, u16 *old_qid,
 	err = mlx5_qos_destroy_node(priv->mdev, node->hw_id);
 	if (err) /* Not fatal. */
 		qos_warn(priv->mdev, "Failed to destroy leaf node %u (class %04x), err = %d\n",
-			 node->hw_id, classid, err);
+			 node->hw_id, *classid, err);
 
 	mlx5e_sw_node_delete(priv, node);

@@ -826,8 +824,7 @@ int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 classid, u16 *old_qid,
 	if (opened)
 		mlx5e_reactivate_qos_sq(priv, moved_qid, txq);
 
-	*old_qid = mlx5e_qid_from_qos(&priv->channels, moved_qid);
-	*new_qid = mlx5e_qid_from_qos(&priv->channels, qid);
+	*classid = node->classid;
 	return 0;
 }

drivers/net/ethernet/mellanox/mlx5/core/en/qos.h +2 -2

index 5af7991fcd19..757682b7c0e0 100644
@@ -34,8 +34,8 @@ int mlx5e_htb_leaf_alloc_queue(struct mlx5e_priv *priv, u16 classid,
 			       struct netlink_ext_ack *extack);
 int mlx5e_htb_leaf_to_inner(struct mlx5e_priv *priv, u16 classid, u16 child_classid,
 			    u64 rate, u64 ceil, struct netlink_ext_ack *extack);
-int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 classid, u16 *old_qid,
-		       u16 *new_qid, struct netlink_ext_ack *extack);
+int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 *classid,
+		       struct netlink_ext_ack *extack);
 int mlx5e_htb_leaf_del_last(struct mlx5e_priv *priv, u16 classid, bool force,
 			    struct netlink_ext_ack *extack);
 int mlx5e_htb_node_modify(struct mlx5e_priv *priv, u16 classid, u64 rate, u64 ceil,

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +1 -2

index 24f919ef9b8e..1a111af6ef7f 100644
@@ -3445,8 +3445,7 @@ static int mlx5e_setup_tc_htb(struct mlx5e_priv *priv, struct tc_htb_qopt_offloa
 		return mlx5e_htb_leaf_to_inner(priv, htb->parent_classid, htb->classid,
 					       htb->rate, htb->ceil, htb->extack);
 	case TC_HTB_LEAF_DEL:
-		return mlx5e_htb_leaf_del(priv, htb->classid, &htb->moved_qid, &htb->qid,
-					  htb->extack);
+		return mlx5e_htb_leaf_del(priv, &htb->classid, htb->extack);
 	case TC_HTB_LEAF_DEL_LAST:
 	case TC_HTB_LEAF_DEL_LAST_FORCE:
 		return mlx5e_htb_leaf_del_last(priv, htb->classid,

include/net/pkt_cls.h +1 -2

index 298a8d10168b..fb34b66aefa7 100644
@@ -824,10 +824,9 @@ enum tc_htb_command {
 struct tc_htb_qopt_offload {
 	struct netlink_ext_ack *extack;
 	enum tc_htb_command command;
-	u16 classid;
 	u32 parent_classid;
+	u16 classid;
 	u16 qid;
-	u16 moved_qid;
 	u64 rate;
 	u64 ceil;
 };

net/sched/sch_htb.c +62 ~~-35~~

170 lines not on this page · raw message

Signature · 1 line

2.25.1

Eric Dumazet

2021-08-26 16:11 UTC #1 lore raw

Quoted 21 lines · Maxim Mikityanskiy

In HTB offload mode, qdiscs of leaf classes are grafted to netdev queues. sch_htb expects the dev_queue field of these qdiscs to point to the corresponding queues. However, qdisc creation may fail, and in that case noop_qdisc is used instead. Its dev_queue doesn't point to the right queue, so sch_htb can lose track of used netdev queues, which will cause internal inconsistencies.
This commit fixes this bug by keeping track of the netdev queue inside struct htb_class. All reads of cl->leaf.q->dev_queue are replaced by the new field, the two values are synced on writes, and WARNs are added to assert equality of the two values.
The driver API has changed: when TC_HTB_LEAF_DEL needs to move a queue, the driver used to pass the old and new queue IDs to sch_htb. Now that there is a new field (offload_queue) in struct htb_class that needs to be updated on this operation, the driver will pass the old class ID to sch_htb instead (it already knows the new class ID).
Fixes d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
Signed-off-by Maxim Mikityanskiy <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>

Quoted 7 lines

---
 .../net/ethernet/mellanox/mlx5/core/en/qos.c  | 15 ++-
 .../net/ethernet/mellanox/mlx5/core/en/qos.h  |  4 +-
 .../net/ethernet/mellanox/mlx5/core/en_main.c |  3 +-
 include/net/pkt_cls.h                         |  3 +-
 net/sched/sch_htb.c                           | 97 ++++++++++++-------
 5 files changed, 72 insertions(+), 50 deletions(-)

Having one patch touching net/sched and one driver looks odd.

I guess it is not possible to split it ?

Saeed Mahameed

maintainer

2021-08-26 17:42 UTC #2 lore raw

Quoted 43 lines · Eric Dumazet

Quoted 31 lines · Maxim Mikityanskiy
In HTB offload mode, qdiscs of leaf classes are grafted to netdev queues. sch_htb expects the dev_queue field of these qdiscs to point to
the corresponding queues. However, qdisc creation may fail, and in that
case noop_qdisc is used instead. Its dev_queue doesn't point to the right queue, so sch_htb can lose track of used netdev queues, which will
cause internal inconsistencies.
This commit fixes this bug by keeping track of the netdev queue inside
struct htb_class. All reads of cl->leaf.q->dev_queue are replaced by the
new field, the two values are synced on writes, and WARNs are added to
assert equality of the two values.
The driver API has changed: when TC_HTB_LEAF_DEL needs to move a queue,
the driver used to pass the old and new queue IDs to sch_htb. Now that
there is a new field (offload_queue) in struct htb_class that needs to
be updated on this operation, the driver will pass the old class ID to
sch_htb instead (it already knows the new class ID).
Fixes d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
Signed-off-by Maxim Mikityanskiy <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>
Quoted 8 lines
---
.../net/ethernet/mellanox/mlx5/core/en/qos.c | 15 ++- .../net/ethernet/mellanox/mlx5/core/en/qos.h | 4 +- .../net/ethernet/mellanox/mlx5/core/en_main.c | 3 +- include/net/pkt_cls.h | 3 +- net/sched/sch_htb.c | 97 ++++++++++++---
----
 5 files changed, 72 insertions(+), 50 deletions(-)

Quoted 4 lines

Having one patch touching net/sched and one driver looks odd.
I guess it is not possible to split it ?

not really possible since API for the driver got changed struct tc_htb_qopt_offload: removed moved_qid field.
we can minimize the driver code just to make it build, then implement the proper support in a later patch, but what's the point, the driver changes are pretty minimal.

[email protected]

CI Applied

2021-08-30 23:50 UTC #3 lore raw

In reply to Maxim Mikityanskiy

This patch was applied to netdev/net-next.git (refs/heads/master):

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

Quoted 8 lines · On Thu, 26 Aug 2021 14:54:25 +0300 you wrote:

In HTB offload mode, qdiscs of leaf classes are grafted to netdev queues. sch_htb expects the dev_queue field of these qdiscs to point to the corresponding queues. However, qdisc creation may fail, and in that case noop_qdisc is used instead. Its dev_queue doesn't point to the right queue, so sch_htb can lose track of used netdev queues, which will cause internal inconsistencies.
[...]

Here is the summary with links:
- [net] sch_htb: Fix inconsistency when leaf qdisc creation fails

    https://git.kernel.org/netdev/net-next/c/ca49bfd90a9d

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html