netdev - Re: [PATCH net v2 2/3] net: sched: fix action overwrite reference counting

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: <6dd90b61-ad41-6e3a-bab7-1f6da2ed1905@mojatatu.com>
Date:   Thu, 8 Apr 2021 07:59:27 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Cong Wang <xiyou.wangcong@...il.com>,
        Vlad Buslov <vladbu@...dia.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Kumar Kartikeya Dwivedi <memxor@...il.com>,
        David Miller <davem@...emloft.net>,
        Jiri Pirko <jiri@...nulli.us>,
        Jakub Kicinski <kuba@...nel.org>,
        Toke Høiland-Jørgensen 
        <toke@...hat.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Davide Caratti <dcaratti@...hat.com>
Subject: Re: [PATCH net v2 2/3] net: sched: fix action overwrite reference
 counting

On 2021-04-07 7:50 p.m., Cong Wang wrote:
> On Wed, Apr 7, 2021 at 8:36 AM Vlad Buslov <vladbu@...dia.com> wrote:
>>
>> Action init code increments reference counter when it changes an action.
>> This is the desired behavior for cls API which needs to obtain action
>> reference for every classifier that points to action. However, act API just
>> needs to change the action and releases the reference before returning.
>> This sequence breaks when the requested action doesn't exist, which causes
>> act API init code to create new action with specified index, but action is
>> still released before returning and is deleted (unless it was referenced
>> concurrently by cls API).
>>
>> Reproduction:
>>
>> $ sudo tc actions ls action gact
>> $ sudo tc actions change action gact drop index 1
>> $ sudo tc actions ls action gact
>>
> 
> I didn't know 'change' could actually create an action when
> it does not exist. So it sets NLM_F_REPLACE, how could it
> replace a non-existing one? Is this the right behavior or is it too
> late to change even if it is not?

Thats expected behavior for "change" essentially mapping
to classical "SET" i.e.
"create if it doesnt exist, replace if it exists"
i.e NLM_F_CREATE | NLM_F_REPLACE

In retrospect, "replace" should probably have been just NLM_F_REPLACE
"replace if it exists, error otherwise".
Currently there is no distinction between the two.

"Add" is classical "CREATE" i.e "create if doesnt exist, otherwise
error"

It may be feasible to fix "replace" but not sure how many scripts over
the years are now dependent on that behavior.

cheers,
jamal