netdev - Re: [iproute2-next] tc flower: use right ethertype in icmp/arp parsing

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] [day] [month] [year] [list]

Message-ID: <d3992c0c-3cae-b81d-4caf-a4950ebc2d8b@gmail.com>
Date:   Tue, 27 Oct 2020 08:12:43 -0600
From:   David Ahern <dsahern@...il.com>
To:     Zahari Doychev <zahari.doychev@...ux.com>,
        Simon Horman <simon.horman@...ronome.com>
Cc:     netdev@...r.kernel.org, jhs@...atatu.com
Subject: Re: [iproute2-next] tc flower: use right ethertype in icmp/arp
 parsing

On 10/27/20 3:11 AM, Zahari Doychev wrote:
> On Mon, Oct 26, 2020 at 09:48:24AM +0100, Simon Horman wrote:
>> On Sun, Oct 25, 2020 at 03:18:48PM -0600, David Ahern wrote:
>>> On 10/19/20 5:47 AM, Zahari Doychev wrote:
>>>> Currently the icmp and arp prsing functions are called with inccorect
>>>> ethtype in case of vlan or cvlan filter options. In this case either
>>>> cvlan_ethtype or vlan_ethtype has to be used.
>>>>
>>>> Signed-off-by: Zahari Doychev <zahari.doychev@...ux.com>
>>>> ---
>>>>  tc/f_flower.c | 43 ++++++++++++++++++++++++++-----------------
>>>>  1 file changed, 26 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/tc/f_flower.c b/tc/f_flower.c
>>>> index 00c919fd..dd9f3446 100644
>>>> --- a/tc/f_flower.c
>>>> +++ b/tc/f_flower.c
>>>> @@ -1712,7 +1712,10 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
>>>>  			}
>>>>  		} else if (matches(*argv, "type") == 0) {
>>>>  			NEXT_ARG();
>>>> -			ret = flower_parse_icmp(*argv, eth_type, ip_proto,
>>>> +			ret = flower_parse_icmp(*argv, cvlan_ethtype ?
>>>> +						cvlan_ethtype : vlan_ethtype ?
>>>> +						vlan_ethtype : eth_type,
>>>> +						ip_proto,
>>>
>>> looks correct to me, but would like confirmation of the intent from Simon.
>>
>> Thanks, this appears to be correct to me as ultimately
>> the code wants to operate on ETH_P_IP or ETH_P_IPV6 rather
>> than a VLAN Ether type.
>>
>>> Also, I am not a fan of the readability of that coding style. Rather
>>> than repeat that expression multiple times, make a short helper to
>>> return the relevant eth type and use a temp variable for it. You should
>>> also comment that relevant eth type changes as arguments are parsed.
> 
> I will add the helper and resend.
> 


perhaps it is simpler to have a new local variable that tracks the eth
type to be used in these locations.