netdev - Re: [PATCH net] net/sched: add delete_empty() to filters and use it in cls

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]

Date:   Sun, 29 Dec 2019 13:27:38 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Vlad Buslov <vladbu@...lanox.com>,
        Davide Caratti <dcaratti@...hat.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH net] net/sched: add delete_empty() to filters and use it
 in cls_flower

On 2019-12-29 12:47 p.m., Vlad Buslov wrote:
> 
> On Sat 28 Dec 2019 at 17:36, Davide Caratti <dcaratti@...hat.com> wrote:
>> Revert "net/sched: cls_u32: fix refcount leak in the error path of
>> u32_change()", and fix the u32 refcount leak in a more generic way that
>> preserves the semantic of rule dumping.
>> On tc filters that don't support lockless insertion/removal, there is no
>> need to guard against concurrent insertion when a removal is in progress.
>> Therefore, for most of them we can avoid a full walk() when deleting, and
>> just decrease the refcount, like it was done on older Linux kernels.
>> This fixes situations where walk() was wrongly detecting a non-empty
>> filter, like it happened with cls_u32 in the error path of change(), thus
>> leading to failures in the following tdc selftests:
>>
>>   6aa7: (filter, u32) Add/Replace u32 with source match and invalid indev
>>   6658: (filter, u32) Add/Replace u32 with custom hash table and invalid handle
>>   74c2: (filter, u32) Add/Replace u32 filter with invalid hash table id
>>
>> On cls_flower, and on (future) lockless filters, this check is necessary:
>> move all the check_empty() logic in a callback so that each filter
>> can have its own implementation. For cls_flower, it's sufficient to check
>> if no IDRs have been allocated.
>>
>> This reverts commit 275c44aa194b7159d1191817b20e076f55f0e620.
>>
>> Changes since v1:
>>   - document the need for delete_empty() when TCF_PROTO_OPS_DOIT_UNLOCKED
>>     is used, thanks to Vlad Buslov
>>   - implement delete_empty() without doing fl_walk(), thanks to Vlad Buslov
>>   - squash revert and new fix in a single patch, to be nice with bisect
>>     tests that run tdc on u32 filter, thanks to Dave Miller
>>
>> Fixes: 275c44aa194b ("net/sched: cls_u32: fix refcount leak in the error path of u32_change()")
>> Fixes: 6676d5e416ee ("net: sched: set dedicated tcf_walker flag when tp is empty")
>> Suggested-by: Jamal Hadi Salim <jhs@...atatu.com>
>> Suggested-by: Vlad Buslov <vladbu@...lanox.com>
>> Signed-off-by: Davide Caratti <dcaratti@...hat.com>
>> ---
> 
> Thanks again, Davide!
> 
> Reviewed-by: Vlad Buslov <vladbu@...lanox.com>

Tested-by: Jamal Hadi Salim <jhs@...atatu.com>
Acked-by: Jamal Hadi Salim <jhs@...atatu.com>

cheers,
jamal