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

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: <c5b1da91-924f-43f8-8fba-6295d4a77d13@gmail.com>
Date: Thu, 4 Jan 2024 10:49:31 -0800
From: Kui-Feng Lee <sinquersw@...il.com>
To: Victor Nogueira <victor@...atatu.com>, jhs@...atatu.com,
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, xiyou.wangcong@...il.com, jiri@...nulli.us
Cc: 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 1/4/24 10:06, Kui-Feng Lee wrote:
> 
> 
> On 12/31/23 09:23, Victor Nogueira 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.
>>
>> 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;
>> -    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;
>> +            }
>>           }
>>       }
> 
> Hi Vector,
> 
> Thank you for fixing this issue!
> Could you also add a test case to avoid regression in future?
> We have BPF test cases that fails for this issue. However,
> not everyone run BPF selftest for netdev changes.
> It would be better to have a test case for net as well.
> 

The following links are about the errors of bpf selftest. FYI!

  - 
https://github.com/kernel-patches/bpf/actions/runs/7401181881/job/20136944224
  - 
https://lore.kernel.org/netdev/ce8d3e55-b8bc-409c-ace9-5cf1c4f7c88e@gmail.com/