netdev - Re: [PATCH net-next v6 10/11] net: sched: atomically check-allocate action

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: <vbflg9aaef8.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
Date:   Mon, 13 Aug 2018 10:55:39 +0300
From:   Vlad Buslov <vladbu@...lanox.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Yevgeny Kliteynik <kliteyn@...lanox.com>,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [PATCH net-next v6 10/11] net: sched: atomically check-allocate action


On Fri 10 Aug 2018 at 21:45, Cong Wang <xiyou.wangcong@...il.com> wrote:
> On Fri, Aug 10, 2018 at 3:29 AM Vlad Buslov <vladbu@...lanox.com> wrote:
>>
>> Approach you suggest is valid, but has its own trade-offs:
>>
>> - As you noted, lock granularity becomes coarse-grained due to per-netns
>> scope.
>
> Sure, you acquire idrinfo->lock too, the only difference is how long
> you take it.
>
> The bottleneck of your approach is the same, also you take idrinfo->lock
> twice, so the contention is heavier.
>
>
>>
>> - I am not sure it is possible to call idr_replace() without obtaining
>> idrinfo->lock in this particular case. Concurrent delete of action with
>> same id is possible and, according to idr_replace() description,
>> unlocked execution is not supported for such use-case:
>
> But we can hold its refcnt before releasing idrinfo->lock, so
> idr_replace() can't race with concurrent delete.

Yes, for concurrent delete case I agree. Action is removed from idr only
when last reference is released and, in case of existing action update,
init holds a reference.

What about case when multiple task race to update the same existing
action? I assume idr_replace() can be used for such case, but what would
be the algorithm in case init replaced some other action, and not the
action it actually copied before calling idr_replace()?

>
>
>>
>> - High rate or replace request will generate a lot of unnecessary memory
>> allocations and deallocations.
>>
>
> Yes, this is literally how RCU works, always allocate and copy,
> release upon error.
>
> Also, if this is really a problem, we have SLAB_TYPESAFE_BY_RCU
> too. ;)

Current action update implementation is in-place, so there is no "copy"
stage, besides members of some actions that are RCU-pointers. But I
guess it makes sense if your goal is to refactor all actions to be
updated with RCU.