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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 13 Apr 2022 13:44:56 +0200 From: Jiri Pirko <jiri@...nulli.us> To: Boris Sukholitko <boris.sukholitko@...adcom.com> Cc: netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>, "Gustavo A . R . Silva" <gustavoars@...nel.org>, Vladimir Oltean <vladimir.oltean@....com>, Eric Dumazet <edumazet@...gle.com>, zhang kai <zhangkaiheb@....com>, Yoshiki Komachi <komachi.yoshiki@...il.com>, Ilya Lifshits <ilya.lifshits@...adcom.com> Subject: Re: [PATCH net-next v2 5/5] net/sched: flower: Consider the number of tags for vlan filters Wed, Apr 13, 2022 at 10:14:17AM CEST, boris.sukholitko@...adcom.com wrote: >On Tue, Apr 12, 2022 at 04:17:01PM +0200, Jiri Pirko wrote: >> Tue, Apr 12, 2022 at 03:16:10PM CEST, boris.sukholitko@...adcom.com wrote: >> >On Tue, Apr 12, 2022 at 02:12:15PM +0200, Jiri Pirko wrote: >> >> Tue, Apr 12, 2022 at 01:40:49PM CEST, boris.sukholitko@...adcom.com wrote: >> >> >On Tue, Apr 12, 2022 at 01:09:35PM +0200, Jiri Pirko wrote: >> >> >> Tue, Apr 12, 2022 at 12:02:36PM CEST, boris.sukholitko@...adcom.com wrote: >> >> >> >Currently the existence of vlan filters is conditional on the vlan >> >> >> >protocol being matched in the tc rule. I.e. the following rule: >> >> >> > >> >> >> >tc filter add dev eth1 ingress flower vlan_prio 5 >> >> >> > >> >> >> >is illegal because we lack protocol 802.1q in the rule. >> >> >> > >> >> >> >Having the num_of_vlans filter configured removes this restriction. The >> >> >> >following rule becomes ok: >> >> >> > >> >> >> >tc filter add dev eth1 ingress flower num_of_vlans 1 vlan_prio 5 >> >> >> > >> >> >> >because we know that the packet is single tagged. >> >> >> > >> >> >> >We achieve the above by having is_vlan_key helper look at the number of >> >> >> >> >> >> Sorry to be a nitpicker, but who's "we"? When I read the patch >> >> >> description, I need to understand clearly what the patch is doing, which >> >> >> is not this case. You suppose to command the codebase what to do. >> >> >> I fail to see that :/ >> >> >> >> >> >> >> >> > >> >> >What do you think of the following description? The description consists >> >> >of two parts: the first provides motivation for the patch, the second is >> >> >the way the motivation is implemented. I've judiciously edited out the >> >> >"we"-word. :) >> >> > >> >> ><description> >> >> > >> >> >Currently the existence of vlan filters is conditional on the vlan >> >> >protocol being matched in the tc rule. I.e. the following rule: >> >> > >> >> >tc filter add dev eth1 ingress flower vlan_prio 5 >> >> > >> >> >is illegal because vlan protocol (e.g. 802.1q) does not appear in the rule. >> >> > >> >> >Having the num_of_vlans filter configured removes this restriction. The >> >> >following rule becomes ok: >> >> > >> >> >tc filter add dev eth1 ingress flower num_of_vlans 1 vlan_prio 5 >> >> >> >> So this is what this patch allows? >> > >> >Yes. >> > >> >> You are talking about it as it is >> >> already possible with the code before this patch being applied. >> >> >> > >> >Sorry for the confusion. In the updated description I try to make the >> >distinction much clearer. >> > >> >> >> >> > >> >> >because having num_of_vlans==1 implies that the packet is single tagged. >> >> > >> >> >To make the above possible, is_vlan_key helper is changed to look at the >> >> >number of vlans in addition to the vlan ethertype. >> >> >> >> What "is changed"? You should tell the codebase what to do, what toadd, >> >> remove or change. If you did that, it would be very clear to the reader >> >> what the patch is supposed to do. >> >> >> > >> >The "changed" refers to the code of is_vlan_key function which is >> >changed by this patch. Please see the updated description. >> > >> >> >> >> > >> >> >Outer tag vlan filters (e.g. vlan_prio) require the number of vlan tags >> >> >be greater than 0. Inner filters (e.g. cvlan_prio) require the number of >> >> >vlan tags be greater than 1. >> >> >> >> Again, unclear what this describes, if the current code before the patch >> >> or the state after this patch. >> >> >> > >> >What about the following: >> > >> ><description> >> > >> >Before this commit the existence of vlan filters was conditional on the vlan >> >protocol being matched in the tc rule. For example, the following rule: >> > >> >tc filter add dev eth1 ingress flower vlan_prio 5 >> > >> >was illegal because vlan protocol (e.g. 802.1q) does not appear in the rule. >> > >> >This commit removes the above restriction. Having the num_of_vlans >> >> Say rather just "Remove the above restriction. ..." >> >> >> >filter configured allows further matching on vlan attributes. The >> >following rule is ok now: >> > >> >tc filter add dev eth1 ingress flower num_of_vlans 1 vlan_prio 5 >> > >> >because having num_of_vlans==1 implies that the packet is single tagged. >> > >> >To do this, this commit changes is_vlan_key helper to look at the number >> >> "Change the is_vlan_key helper to look..." >> >> Don't talk about "this commit". >> > >OK. The following incorporates both of the above suggestions: > ><description> > >Before this commit the existence of vlan filters was conditional on the vlan >protocol being matched in the tc rule. For example, the following rule: > >tc filter add dev eth1 ingress flower vlan_prio 5 > >was illegal because vlan protocol (e.g. 802.1q) does not appear in the rule. > >Remove the above restriction by looking at the num_of_vlans filter to >allow further matching on vlan attributes. The following rule is ok now: What's "now"? > >tc filter add dev eth1 ingress flower num_of_vlans 1 vlan_prio 5 > >because having num_of_vlans==1 implies that the packet is single tagged. > >Change is_vlan_key helper to look at the number of vlans in addition to >the vlan ethertype. Outer (e.g. vlan_prio) and inner (e.g. cvlan_prio) >tag vlan filters require the number of vlan tags to be greater then 0 >and 1 accordingly. I don't get this last sentence. "filters require". Do you do the change or are you stating what's in before the patch? > >As a result of is_vlan_key change, the ethertype may be set to 0 when >matching on the number of vlans. Update fl_set_key_vlan to avoid setting >key, mask vlan_tpid for the 0 ethertype. > ></description> > >Thanks, >Boris.
Powered by blists - more mailing lists