On 2019-05-22 11:08 a.m., Vlad Buslov wrote:
On Tue 21 May 2019 at 16:23, Vlad Buslov <vla...@mellanox.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
