netdev - Re: [RFC PATCH v2 net-next 0/3] flow_offload: Re-add per-action statistics

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: <b147865f-5224-b66b-2824-8c1c8986900f@mojatatu.com>
Date:   Wed, 22 May 2019 13:24:52 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Vlad Buslov <vladbu@...lanox.com>,
        Cong Wang <xiyou.wangcong@...il.com>
Cc:     Edward Cree <ecree@...arflare.com>, Jiri Pirko <jiri@...nulli.us>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Andy Gospodarek <andy@...yhouse.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Michael Chan <michael.chan@...adcom.com>,
        Vishal Kulkarni <vishal@...lsio.com>,
        Lucas Bates <lucasb@...atatu.com>
Subject: Re: [RFC PATCH v2 net-next 0/3] flow_offload: Re-add per-action
 statistics

On 2019-05-22 11:08 a.m., Vlad Buslov wrote:
> 
> On Tue 21 May 2019 at 16:23, Vlad Buslov <vladbu@...lanox.com> wrote:

> 
> It seems that culprit in this case is tc_action->order field. It is used
> as nla attrtype when dumping actions. Initially it is set according to
> ordering of actions of filter that creates them. However, it is
> overwritten in tca_action_gd() each time action is dumped through action
> API (according to action position in tb array) and when new filter is
> attached to shared action (according to action order on the filter).
> With this we have another way to reproduce the bug:
> 
> sudo tc qdisc add dev lo ingress
> 
> #shared action is the first action (order 1)
> sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \
> match ip dst 127.0.0.8/32 flowid 1:10 \
> action drop index 104 \
> action vlan push id 100 protocol 802.1q
> 
> #shared action is the the second action (order 2)
> sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \
> match ip dst 127.0.0.10/32 flowid 1:10 \
> action vlan push id 101 protocol 802.1q \
> action drop index 104
> 
> # Now action is only visible on one filter
> sudo tc -s filter ls dev lo parent ffff: protocol ip
> 

Ok, thanks for chasing this. A test case i had in mind is to
maybe have 3 actions. Add the drop in the middle for one
and at the begging for another and see if they are visible
with the patch.
If you dont have time I can test maybe AM tommorow.

> The usage of tc_action->order is inherently incorrect for shared actions
> and I don't really understand the reason for using it in first place.
> I'm sending RFC patch that fixes the issue by just using action position
> in tb array for attrtype instead of order field, and it seems to solve
> both issues for me. Please check it out to verify that I'm not breaking
> something. Also, please advise on "fixes" tag since this problem doesn't
> seem to be directly caused by my act API refactoring.
> 

I dont know when this broke then.
Seems to be working correctly on the machine i am right now
(4.4) but broken on 4.19. So somewhere in between things broke.
I dont have other kernels to compare at the moment.

cheers,
jamal