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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180315112150.58586758@halley>
Date:   Thu, 15 Mar 2018 11:21:49 +0200
From:   Shmulik Ladkani <shmulik.ladkani@...il.com>
To:     Liran Alon <liran.alon@...cle.com>,
        Daniel Borkmann <daniel@...earbox.net>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, idan.brown@...cle.com,
        Yuval Shaia <yuval.shaia@...cle.com>
Subject: Re: [PATCH] net: dev_forward_skb(): Scrub packet's per-netns info
 only when crossing netns

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).


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.

Maybe Daniel can comment on the matter.

Regards,
Shmulik

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ