Re: [PATCH net-next v2 1/1] net/sched: We should only add appropriate qdiscs blocks to ports' xarray

[Jiri Pirko <jiri@resnulli.us>]

The patch subject should briefly describe the nature of the change. Not
what "we" should or should not do.


Sun, Dec 31, 2023 at 06:23:20PM CET, victor@mojatatu.com wrote:
>We should only add qdiscs to the blocks ports' xarray in ingress that
>support ingress_block_set/get or in egress that support
>egress_block_set/get.

Tell the codebase what to do, be imperative. Please read again:
https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes


>
>Fixes: 913b47d3424e ("net/sched: Introduce tc block netdev tracking infra")
>Signed-off-by: Victor Nogueira <victor@mojatatu.com>
>Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
>Reported-by: Ido Schimmel <idosch@nvidia.com>
>Closes: https://lore.kernel.org/all/ZY1hBb8GFwycfgvd@shredder/
>Tested-by: Ido Schimmel <idosch@nvidia.com>
>Reported-and-tested-by: syzbot+84339b9e7330daae4d66@syzkaller.appspotmail.com
>Closes: https://lore.kernel.org/all/0000000000007c85f5060dcc3a28@google.com/
>Reported-and-tested-by: syzbot+806b0572c8d06b66b234@syzkaller.appspotmail.com
>Closes: https://lore.kernel.org/all/00000000000082f2f2060dcc3a92@google.com/
>Reported-and-tested-by: syzbot+0039110f932d438130f9@syzkaller.appspotmail.com
>Closes: https://lore.kernel.org/all/0000000000007fbc8c060dcc3a5c@google.com/
>---
>v1 -> v2:
>
>- Remove newline between fixes tag and Signed-off-by tag
>- Add Ido's Reported-by and Tested-by tags
>- Add syzbot's Reported-and-tested-by tags
>
> net/sched/sch_api.c | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
>diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>index 299086bb6205..426be81276f1 100644
>--- a/net/sched/sch_api.c
>+++ b/net/sched/sch_api.c
>@@ -1187,23 +1187,29 @@ static int qdisc_block_add_dev(struct Qdisc *sch, struct net_device *dev,
> 	struct tcf_block *block;
> 	int err;
> 

Why don't you just check cl_ops->tcf_block ?
In fact, there could be a helper to do it for you either call the op or
return NULL in case it is not defined.


>-	block = cl_ops->tcf_block(sch, TC_H_MIN_INGRESS, NULL);
>-	if (block) {
>-		err = xa_insert(&block->ports, dev->ifindex, dev, GFP_KERNEL);
>-		if (err) {
>-			NL_SET_ERR_MSG(extack,
>-				       "ingress block dev insert failed");
>-			return err;
>+	if (sch->ops->ingress_block_get) {
>+		block = cl_ops->tcf_block(sch, TC_H_MIN_INGRESS, NULL);
>+		if (block) {
>+			err = xa_insert(&block->ports, dev->ifindex, dev,
>+					GFP_KERNEL);
>+			if (err) {
>+				NL_SET_ERR_MSG(extack,
>+					       "ingress block dev insert failed");
>+				return err;
>+			}
> 		}
> 	}
> 
>-	block = cl_ops->tcf_block(sch, TC_H_MIN_EGRESS, NULL);
>-	if (block) {
>-		err = xa_insert(&block->ports, dev->ifindex, dev, GFP_KERNEL);
>-		if (err) {
>-			NL_SET_ERR_MSG(extack,
>-				       "Egress block dev insert failed");
>-			goto err_out;
>+	if (sch->ops->egress_block_get) {
>+		block = cl_ops->tcf_block(sch, TC_H_MIN_EGRESS, NULL);
>+		if (block) {
>+			err = xa_insert(&block->ports, dev->ifindex, dev,
>+					GFP_KERNEL);
>+			if (err) {
>+				NL_SET_ERR_MSG(extack,
>+					       "Egress block dev insert failed");
>+				goto err_out;
>+			}
> 		}
> 	}
> 
>-- 
>2.25.1
>