| 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
| ||
|
Message-ID: <CAM0EoMkKmF3mhnHLt6gE2bmpuRGV7=OpzrMrOwtk3TJcDFW2JA@mail.gmail.com>
Date: Tue, 2 Jan 2024 09:06:28 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: Victor Nogueira <victor@...atatu.com>, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, xiyou.wangcong@...il.com,
idosch@...sch.org, mleitner@...hat.com, vladbu@...dia.com, paulb@...dia.com,
pctammela@...atatu.com, netdev@...r.kernel.org, kernel@...atatu.com,
syzbot+84339b9e7330daae4d66@...kaller.appspotmail.com,
syzbot+806b0572c8d06b66b234@...kaller.appspotmail.com,
syzbot+0039110f932d438130f9@...kaller.appspotmail.com
Subject: Re: [PATCH net-next v2 1/1] net/sched: We should only add appropriate
qdiscs blocks to ports' xarray
On Tue, Jan 2, 2024 at 4:59 AM Jiri Pirko <jiri@...nulli.us> wrote:
>
> 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@...atatu.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
>
We need another rule in the doc on nit-picking which states that we
need to make progress at some point. We made many changes to this
patchset based on your suggestions for no other reason other that we
can progress the discussion. This is a patch that fixes a bug of which
there are multiple syzbot reports and consumers of the API(last one
just reported from the MTCP people). There's some sense of urgency to
apply this patch before the original goes into net. More importantly:
This patch fixes the issue and follows the same common check which was
already being done in the committed patchset to check if the qdisc
supports the block set/get operations.
There are about 3 ways to do this check, you objected to the original,
we picked something that works fine, and now you are picking a
different way with tcf_block. I dont see how tcf_block check would
help or solve this problem at all given this is a qdisc issue not a
class issue. What am I missing?
cheers,
jamal
> >
> >Fixes: 913b47d3424e ("net/sched: Introduce tc block netdev tracking infra")
> >Signed-off-by: Victor Nogueira <victor@...atatu.com>
> >Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>
> >Reported-by: Ido Schimmel <idosch@...dia.com>
> >Closes: https://lore.kernel.org/all/ZY1hBb8GFwycfgvd@shredder/
> >Tested-by: Ido Schimmel <idosch@...dia.com>
> >Reported-and-tested-by: syzbot+84339b9e7330daae4d66@...kaller.appspotmail.com
> >Closes: https://lore.kernel.org/all/0000000000007c85f5060dcc3a28@google.com/
> >Reported-and-tested-by: syzbot+806b0572c8d06b66b234@...kaller.appspotmail.com
> >Closes: https://lore.kernel.org/all/00000000000082f2f2060dcc3a92@google.com/
> >Reported-and-tested-by: syzbot+0039110f932d438130f9@...kaller.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
> >
Powered by blists - more mailing lists