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] [day] [month] [year] [list]

Date:   Mon, 30 Dec 2019 20:36:02 -0800 (PST)
From:   David Miller <davem@...emloft.net>
To:     dcaratti@...hat.com
Cc:     netdev@...r.kernel.org, vladbu@...lanox.com, jhs@...atatu.com,
        xiyou.wangcong@...il.com, jiri@...nulli.us
Subject: Re: [PATCH net] net/sched: add delete_empty() to filters and use
 it in cls_flower

From: Davide Caratti <dcaratti@...hat.com>
Date: Sat, 28 Dec 2019 16:36:58 +0100

> 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>

Applied.