netdev - Re: [PATCH net-next v3] net: sched: don't disable bh when accessing action idr

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]

Message-ID: <vbfsh6hhu4d.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
Date:   Thu, 24 May 2018 19:54:26 +0300
From:   Vlad Buslov <vladbu@...lanox.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Jiri Pirko <jiri@...nulli.us>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        David Miller <davem@...emloft.net>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v3] net: sched: don't disable bh when accessing action idr


On Wed 23 May 2018 at 23:14, Cong Wang <xiyou.wangcong@...il.com> wrote:
> On Wed, May 23, 2018 at 1:52 AM, Vlad Buslov <vladbu@...lanox.com> wrote:
>> Initial net_device implementation used ingress_lock spinlock to synchronize
>> ingress path of device. This lock was used in both process and bh context.
>> In some code paths action map lock was obtained while holding ingress_lock.
>> Commit e1e992e52faa ("[NET_SCHED] protect action config/dump from irqs")
>> modified actions to always disable bh, while using action map lock, in
>> order to prevent deadlock on ingress_lock in softirq. This lock was removed
>> in commit 555353cfa1ae ("netdev: The ingress_lock member is no longer
>> needed.").
>>
>> Another reason to disable bh was filters delete code, that released actions
>> in rcu callback. This code was changed to release actions from workqueue
>> context in patch set "net_sched: close the race between call_rcu() and
>> cleanup_net()".
>>
>> With these changes it is no longer necessary to continue disable bh while
>> accessing action map.
>>
>> Replace all action idr spinlock usage with regular calls that do not
>> disable bh.
>
> Looks much better now!
>
> I _guess_ we perhaps can even get rid of this spinlock since most of
> the callers hold RTNL lock, not sure about the dump() path where
> RTNL might be removed recently.

Actually, this change is a result of discussion in code review of my
patch set that removes RTNL dependency from TC rules update path.

>
> Anyway,
>
> Acked-by: Cong Wang <xiyou.wangcong@...il.com>

Thank you for reviewing my code!