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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 08 Aug 2018 15:06:27 +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 Wed 08 Aug 2018 at 01:20, Cong Wang <xiyou.wangcong@...il.com> wrote:
> On Thu, Jul 5, 2018 at 7:24 AM Vlad Buslov <vladbu@...lanox.com> wrote:
>>
>> Implement function that atomically checks if action exists and either takes
>> reference to it, or allocates idr slot for action index to prevent
>> concurrent allocations of actions with same index. Use EBUSY error pointer
>> to indicate that idr slot is reserved.
>
> A dumb question:
>
> How could "concurrent allocations of actions with same index" happen
> as you already take idrinfo->lock for the whole
> tcf_idr_check_alloc()??

I guess my changelog is not precise enough in this description.
Let look into sequence of events of initialization of new action:
1) tcf_idr_check_alloc() is called by action init.
2) idrinfo->lock is taken.
3) Lookup in idr is performed to determine if action with specified
index already exists.
4) EBUSY pointer is inserted to indicate that id is taken.
5) idrinfo->lock is released.
6) tcf_idr_check_alloc() returns to action init code.
7) New action is allocated and initialized.
8) tcf_idr_insert() is called.
9) idrinfo->lock is taken.
10) EBUSY pointer is substituted with pointer to new action.
11) idrinfo->lock is released.
12) tcf_idr_insert() returns.

So in this case "concurrent allocations of actions with same index"
means not the allocation with same index during tcf_idr_check_alloc(),
but during the period when idrinfo->lock was released(6-8).

>
> For me, it should be only one allocation could succeed, all others
> should fail.

Correct! And this change is made specifically to enforce that rule.

Otherwise, multiple processes could try to create new action with same
id at the same time, and all processes that executed 3, before any
process reached 10, will "succeed" by overwriting each others action in
idr. (and leak memory while doing so)

>
> Maybe you are trying to prevent others treat it like existing one,
> but in that case you can just hold the idinfo->lock for all idr operations.
>
> And more importantly, upper layer is able to tell it is a creation or
> just replace, you don't have to check this in this complicated way.
>
> IOW, all of these complicated code should not exist.

Original code was simpler and didn't involve temporary EBUSY pointer.
This change was made according to Jiri's request. He wanted to have
unified API to be used by all actions and suggested this approach
specifically.