netdev - Re: [PATCH v10 12/12] bpf: add sample for xdp forwarding and rewrite

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: <20160803223632.GA42605@ast-mbp.thefacebook.com>
Date:	Wed, 3 Aug 2016 15:36:34 -0700
From:	Alexei Starovoitov <alexei.starovoitov@...il.com>
To:	Tom Herbert <tom@...bertland.com>
Cc:	Brenden Blanco <bblanco@...mgrid.com>,
	"David S. Miller" <davem@...emloft.net>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	Saeed Mahameed <saeedm@....mellanox.co.il>,
	Martin KaFai Lau <kafai@...com>,
	Jesper Dangaard Brouer <brouer@...hat.com>,
	Ari Saha <as754m@....com>, Or Gerlitz <gerlitz.or@...il.com>,
	john fastabend <john.fastabend@...il.com>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Thomas Graf <tgraf@...g.ch>,
	Daniel Borkmann <daniel@...earbox.net>,
	Tariq Toukan <ttoukan.linux@...il.com>,
	Aaron Yue <haoxuany@...com>
Subject: Re: [PATCH v10 12/12] bpf: add sample for xdp forwarding and rewrite

On Wed, Aug 03, 2016 at 12:06:55PM -0700, Tom Herbert wrote:
> On Wed, Aug 3, 2016 at 11:29 AM, Brenden Blanco <bblanco@...mgrid.com> wrote:
> > On Wed, Aug 03, 2016 at 10:29:58AM -0700, Tom Herbert wrote:
> >> On Wed, Aug 3, 2016 at 10:11 AM, Alexei Starovoitov
> >> <alexei.starovoitov@...il.com> wrote:
> >> > On Wed, Aug 03, 2016 at 10:01:54AM -0700, Tom Herbert wrote:
> >> >> On Tue, Jul 19, 2016 at 12:16 PM, Brenden Blanco <bblanco@...mgrid.com> wrote:
> > [...]
> >> >> > +SEC("xdp1")
> >> >> > +int xdp_prog1(struct xdp_md *ctx)
> >> >> > +{
> >> >> > +       void *data_end = (void *)(long)ctx->data_end;
> >> >> > +       void *data = (void *)(long)ctx->data;
> >> >>
> >> >> Brendan,
> >> >>
> >> >> It seems that the cast to long here is done because data_end and data
> >> >> are u32s in xdp_md. So the effect is that we are upcasting a
> >> >> thirty-bit integer into a sixty-four bit pointer (in fact without the
> >> >> cast we see compiler warnings). I don't understand how this can be
> >> >> correct. Can you shed some light on this?
> >> >
> >> > please see:
> >> > http://lists.iovisor.org/pipermail/iovisor-dev/2016-August/000355.html
> >> >
> >> That doesn't explain it. The only thing I can figure is that there is
> >> an implicit assumption somewhere that even though the pointer size may
> >> be 64 bits, only the low order thirty-two bits are relevant in this
> >> environment (i.e. upper bit are always zero for any pointers)-- so
> >> then it would safe store pointers as u32 and to upcast them to void *.
> > No, the actual pointer storage is always void* sized (see struct
> > xdp_buff). The mangling is cosmetic. The verifier converts the
> > underlying bpf load instruction to the right sized operation.
> 
> This is not at all obvious to XDP programmer. The type of ctx
> structure is xdp_md and the definition of that structure in
> uapi/linux/bpf.h says that the fields in the that structure are __u32.
> So when I, as a user naive the inner workings of the verifier, read
> this code it sure looks like we are upcasting a 32 bit value to a 64
> bit value-- that does not seem right at all and the compiler
> apparently concurs my point of view. If the code ends up being correct
> anyway, then the obvious answer to have an explicit cast that points
> out the special nature of this cast. Blindly casting to u32 to long
> for the purposes of assigning to a pointer is only going to confuse
> more people as it has me.

Agree. Would be nice to have few helpers. The question is whether
they belong in bpf.h. Probably not, since they're not kernel abi.
For the same reasons we didn't include instruction building macros
like BPF_ALU64_REG and instead kept them in samples/bpf/libbpf.h
Here probably four static inline functions are needed. Two for __sk_buff
and two for xpd_md. That should make xdp*_kern.c examples a bit easier
to read.