netdev - Re: [PATCH v15 ] net/veth/XDP: Line-rate packet forwarding in kernel

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, 3 Apr 2018 10:37:02 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     David Ahern <dsahern@...il.com>
Cc:     John Fastabend <john.fastabend@...il.com>,
        "Md. Islam" <mislam4@...t.edu>, netdev@...r.kernel.org,
        David Miller <davem@...emloft.net>, stephen@...workplumber.org,
        agaceph@...il.com, Pavel Emelyanov <xemul@...nvz.org>,
        Eric Dumazet <edumazet@...gle.com>, brouer@...hat.com
Subject: Re: [PATCH v15 ] net/veth/XDP: Line-rate packet forwarding in kernel

On Tue, Apr 03, 2018 at 11:14:00AM -0600, David Ahern wrote:
> On 4/3/18 11:06 AM, Alexei Starovoitov wrote:
> >> For 3 and 4 above I was referring to the route lookup part of it; sorry
> >> for not being clear.
> >>
> >> For example, eth1 is enslaved to bond1 which is in VRF red. The lookup
> >> needs to go to the table associated with the VRF. That is not known by
> >> just looking at eth1. The code exists to walk the upper layers and do
> >> the effective translations, just need to cover those cases.
> >>
> >> The VLAN part of it is a bit more difficult - ingress device for the
> >> lookup should be eth1.100 for example not eth1, and then if eth1.100 is
> >> enslaved to a VRF, ...
> >>
> >> None of it is that complex, just need to walk through the various use
> >> cases and make sure bpf_ipv4_fwd_lookup and bpf_ipv6_fwd_lookup can do
> >> the right thing for these common use cases.
> > I'm a bit lost here. Why this is a concern?
> > 'index' as argument that bpf prog is passing into the helper.
> > The clsbpf program may choose to pass ifindex of the netdev
> > it's attached to or some other one.
> > In your patch you have:
> > +BPF_CALL_3(bpf_ipv4_fwd_lookup, int, index, const struct iphdr *, iph,
> > +	   struct ethhdr *, eth)
> > +{
> > +	struct flowi4 fl4 = {
> > +		.daddr = iph->daddr,
> > +		.saddr = iph->saddr,
> > +		.flowi4_iif = index,
> > +		.flowi4_tos = iph->tos & IPTOS_RT_MASK,
> > +		.flowi4_scope = RT_SCOPE_UNIVERSE,
> > +	};
> > 
> > As you saying there is concern with .flowi4_iif = index line ?
> 
> yes. BPF / XDP programs are installed on the bottom device ... e.g.,
> eth1. The L3 lookup is not necessarily done on that device index.

right, but I still don't see any problem with this helper and vlans.
If xdp program passes incorrect ifindex, it's program's mistake.
If clsbpf attached to vlan passed good ifindex, the lookup will
happen in the correct scope, but even in this case the prog
can pass whatever ifindex it wants.

> 
> > In the above the only thing Daniel and myself pointed out that
> > passing struct iphdr * like this is not safe.
> > We either need size argument which would be a bit cumbersome or
> > extend verifier a little to specify size as part of helper proto,
> > so that verifier can eforce it without having program to pass it.
> > imo that's the only bit missing from that patch to upstream it.
> 
> sure. I did not mean that item 1. was a big deal, just something that
> needed to be fixed.
> 
> > 
> > Also the helper isn't really related to XDP. It should work as-is
> > for clsbpf and xdp programs as far as I can tell.
> > 
> 
> yes.