linux-kernel - Re: [PATCH] net: dev_forward_skb(): Scrub packet's per-netns info only when crossing netns

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

Message-ID: <cf2dee1b-6098-47aa-bb2b-f8a44db904e3@default>
Date:   Thu, 15 Mar 2018 05:14:15 -0700 (PDT)
From:   Liran Alon <liran.alon@...cle.com>
To:     <shmulik.ladkani@...il.com>
Cc:     <netdev@...r.kernel.org>, <daniel@...earbox.net>,
        <davem@...emloft.net>, <linux-kernel@...r.kernel.org>,
        <yuval.shaia@...cle.com>, <idan.brown@...cle.com>
Subject: Re: [PATCH] net: dev_forward_skb(): Scrub packet's per-netns info
 only when crossing netns


----- shmulik.ladkani@...il.com wrote:

> Hi,
> 
> On Tue, 13 Mar 2018 17:07:22 +0200 Liran Alon <liran.alon@...cle.com>
> wrote:
> > Before this commit, dev_forward_skb() always cleared packet's
> > per-network-namespace info. Even if the packet doesn't cross
> > network namespaces.
> > 
> > The comment above dev_forward_skb() describes that this is done
> > because the receiving device may be in another network namespace.
> > However, this case can easily be tested for and therefore we can
> > scrub packet's per-network-namespace info only when receiving
> device
> > is indeed in another network namespace.
> > 
> > Therefore, this commit changes ____dev_forward_skb() to tell
> > skb_scrub_packet() that skb has crossed network-namespace only in
> case
> > transmitting device (skb->dev) network namespace is different then
> > receiving device (dev) network namespace.
> 
> Assuming the premise of this commit is correct, note it may not act
> as
> intended for xnet situation in ipvlan_process_multicast, snip:
> 
> 		nskb->dev = ipvlan->dev;
> 		if (tx_pkt)
> 			ret = dev_forward_skb(ipvlan->dev, nskb);
> 		else
> 			ret = netif_rx(nskb);
> 
> as 'dev' gets already assigned to nskb prior dev_forward_skb (hence
> in
> ____dev_forward_skb both dev and skb->dev are the same).
> Fortunately every ipvlan_multicast_enqueue call is preceded by a
> forced
> scrub; It would be future-proof to not assign nskb->dev in the
> dev_forward_skb case (assign it only in the netif_rx case).

I agree. Nice catch.
skb->dev will later get assigned in eth_type_trans() called from __dev_forward_skb().
I will do this small change in a separate patch before this patch.
(In v2 of this series)

> 
> 
> Regarding the premise of this commit, this "reduces" the
> ipvs/orphan/mark scrubbing in the following *non* xnet situations:
> 
>  1. mac2vlan port xmit to other macvlan ports in Bridge Mode
>  2. similarly for ipvlan
>  3. veth xmit
>  4. l2tp_eth_dev_recv
>  5. bpf redirect/clone_redirect ingress actions
> 
> Regarding l2tp recv, this commit seems to align the srubbing behavior
> with ip tunnels (full scrub only if crossing netns, see
> ip_tunnel_rcv).
> 
> Regarding veth xmit, it does makes sense to preserve the fields if
> not
> crossing netns. This is also the case when one uses tc mirred.
> 
> Regarding bpf redirect, well, it depends on the expectations of each
> bpf
> program.
> I'd argue that preserving the fields (at least the mark field) in the
> *non* xnet makes sense and provides more information and therefore
> more
> capabilities; Alas this might change behavior already being relied
> on.

I now noticed that a similar change was done in the past in
commit 8a83a00b0735 ("net: maintain namespace isolation
between vlan and real device"). Commit changed dev_forward_skb() from always
setting skb->mark=0 to do this only in case we cross netns.

However, a later commit: 59b9997baba5 ("Revert "net: maintain namespace
isolation between vlan and real device") seems to later reverted that change.
But I think that the regression issue mentioned in the revert isn't
related to the change proposed by this commit.
Please correct me if I'm missing something.

> 
> Maybe Daniel can comment on the matter.
> 
> Regards,
> Shmulik