netdev - Re: [PATCH net] dev_forward_skb: do not scrub skb mark within the same name space

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: <2b01ec85-216a-2785-fd1c-42c38ad30c9d@gmail.com>
Date:   Fri, 25 Jun 2021 11:45:52 +0300
From:   Eyal Birger <eyal.birger@...il.com>
To:     nicolas.dichtel@...nd.com
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        liran.alon@...cle.com, shmulik.ladkani@...il.com,
        daniel@...earbox.net
Subject: Re: [PATCH net] dev_forward_skb: do not scrub skb mark within the
 same name space



On 24/06/2021 18:26, Nicolas Dichtel wrote:
> Hi Eyal,
> 
> Le 24/06/2021 à 14:16, Eyal Birger a écrit :
>> Hi Nicholas,
>>
>> On 24/06/2021 11:05, Nicolas Dichtel wrote:
>>> The goal is to keep the mark during a bpf_redirect(), like it is done for
>>> legacy encapsulation / decapsulation, when there is no x-netns.
>>> This was initially done in commit 213dd74aee76 ("skbuff: Do not scrub skb
>>> mark within the same name space").
>>>
>>> When the call to skb_scrub_packet() was added in dev_forward_skb() (commit
>>> 8b27f27797ca ("skb: allow skb_scrub_packet() to be used by tunnels")), the
>>> second argument (xnet) was set to true to force a call to skb_orphan(). At
>>> this time, the mark was always cleanned up by skb_scrub_packet(), whatever
>>> xnet value was.
>>> This call to skb_orphan() was removed later in commit
>>> 9c4c325252c5 ("skbuff: preserve sock reference when scrubbing the skb.").
>>> But this 'true' stayed here without any real reason.
>>>
>>> Let's correctly set xnet in ____dev_forward_skb(), this function has access
>>> to the previous interface and to the new interface.
>>
>> This change was suggested in the past [1] and I think one of the main concerns
>> was breaking existing callers which assume the mark would be cleared [2].
> Thank you for the pointers!
> 
>>
>> Personally, I think the suggestion made in [3] adding a flag to bpf_redirect()
>> makes a lot of sense for this use case.
> I began with this approach, but actually, as I tried to explain in the commit
> log, this looks more like a bug. This function is called almost everywhere in
> the kernel (except for openvswitch and wireguard) with the xnet argument
> reflecting a netns change. In other words, the behavior is different between
> legacy encapsulation and the one made with ebpf :/
> 

I agree, and was also surprised that ebpf redirection scrubs the mark in 
the same ns - and only on ingress! - so I think keeping the mark should 
have been the default behavior. As noted in the thread though it is not 
clear whether this would break existing deployments both for ebpf and 
veth pairs on the same ns.

Eyal.