netdev - Re: [Patch net] net_sched: refetch skb protocol for each filter

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]

Date:   Tue, 15 Jan 2019 10:59:02 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Martin Olsson <martin.olsson+netdev@...torsecurity.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Lucas Bates <lucasb@...atatu.com>
Subject: Re: [Patch net] net_sched: refetch skb protocol for each filter

On 2019-01-13 4:08 p.m., Cong Wang wrote:
> On Sat, Jan 12, 2019 at 7:41 AM Jamal Hadi Salim <jhs@...atatu.com> wrote:
>>
>> On 2019-01-12 7:23 a.m., Jamal Hadi Salim wrote:
>>
>>> Do we have a test case for a setup like this in tdc?
>>> i.e incoming tagged and then vlan popped by action?
>>> Otherwise a test with IFE which resets the ethertype
>>> would be sufficient i.e just something that will messup
>>> with skb->protocol.
>>
>> And here is a slightly complex test with IFE.
>> Wanted to show both reclassify and continue in play
>> as well as something that change skb->protocol.
>>
> 
> I don't know why you need a complex one, Martin's
> test case is pretty simple (as I already sent to you).
> 

Martin example had a "reclassify" with the vlan pop.
Your example was showing a "continue"
Note: "Reclassify" is typical how we have explained
to users to deal with skb->protocol changes.

I wanted to demonstrate that if i have a rule that followed that
rule  (as Martin did) with something other than vlan it would work:

match u32 protocol someethertype .... action change vl reclassify
match u32 protocol ip ... action continue
match u32 protocol ip ... action blah

infact that works before your patch. Basically it was an exercise
to reduce _the variables_ from knowing that reclassify/continue
do/nt work in presence of changing skb->protocol.
IMO that example demonstrated that the current skb->protocol
logic works and the vlan action maybe the guilty party.

> Also, you can add two printk()'s around the skb_vlan_pop()
> in tcf_vlan_act() to see the difference of tc_skb_protocol()
> return values before and after. I tried, it clearly shows
> ETH_P_8021Q and ETH_P_IP.
> 
> Of course, it could be tc_skb_protocol() which is wrong,
> as skb->protocol stays same.
> 
> This patch is always correct despite of tc_skb_protocol():
> 
> 1. If tc_skb_protocol() is wrong, this patch fixes nothing,
> and harms nothing.
> 
> 2. If tc_skb_protocol() is correct, this patch fixes a bug.
> 
> Changing tc_skb_protocol() is much more risky than this
> patch. You fixed a very similar bug before:
> 
> commit 619fe32640b4b01f370574d50344ae0f62689816
> Author: Jamal Hadi Salim <jhs@...atatu.com>
> Date:   Thu Feb 18 07:38:04 2016 -0500
> 
>      net_sched fix: reclassification needs to consider ether protocol changes
> 
> which also implies tc_skb_protocol() is correct.

I agree with your rationale.
Lets have Lucas give this a run and we can move
forward.

cheers,
jamal