Re: [PATCH 1/1] selftests/bpf: add cls_redirect classifier

[Alexei Starovoitov]

On Sat, May 02, 2020 at 01:48:51AM +0200, Daniel Borkmann wrote:
> On 4/27/20 11:45 AM, Lorenz Bauer wrote:
> > On Sun, 26 Apr 2020 at 18:33, Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> [...]
> > > > +/* Linux packet pointers are either aligned to NET_IP_ALIGN (aka 2 bytes),
> > > > + * or not aligned if the arch supports efficient unaligned access.
> > > > + *
> > > > + * Since the verifier ensures that eBPF packet accesses follow these rules,
> > > > + * we can tell LLVM to emit code as if we always had a larger alignment.
> > > > + * It will yell at us if we end up on a platform where this is not valid.
> > > > + */
> > > > +typedef uint8_t *net_ptr __attribute__((align_value(8)));
> > > 
> > > Wow. I didn't know about this attribute.
> > > I wonder whether it can help Daniel's memcpy hack.
> > 
> > Yes, I think so.
> 
> Just for some more context [0]. I think the problem is a bit more complex in
> general. Generally, _any_ kind of pointer to some data (except for the stack)
> is currently treated as byte-by-byte copy from __builtin_memcpy() and other
> similarly available __builtin_*() helpers on BPF backend since the backend
> cannot make any assumptions about the data's alignment and whether unaligned
> access from the underlying arch is ok & efficient (the latter the verifier
> does judge for us however). So it's definitely not just limited to xdp->data.
> There is also the issue that while access to any non-stack data can be
> unaligned, access to the stack however cannot. I've discussed a while back
> with Yonghong about potential solutions. One would be to add a small patch
> to the BPF backend to enable __builtin_*() helpers to allow for unaligned
> access which could then be opt-ed in e.g. via -mattr from llc for the case
> when we know that the compiled program only runs on archs with efficient
> unaligned access anyway. However, this still potentially breaks with the BPF
> stack for the case when objects are, for example, larger than size 8 but with
> a natural alignment smaller than 8 where __builtin_memcpy() would then decide
> to emit dw-typed load/stores. But for these cases could then be annotated via
> __aligned(8) on stack. So this is basically what we do right now as a generic
> workaround in Cilium [0], meaning, our own memcpy/memset with optimal number
> of instructions and __aligned(8) where needed; most of the time this __aligned(8)
> is not needed, so it's really just a few places, and we also have a cocci
> scripts to catch these during development if needed. Anyway, real thing would
> be to allow the BPF stack for unaligned access as well and then BPF backend
> could nicely solve this in a native way w/o any workarounds, but that is tbd.
> 
> Thanks,
> Daniel
> 
>   [0] https://github.com/cilium/cilium/blob/master/bpf/include/bpf/builtins.h

Daniel,
do you mind adding such memcpy to libbpf ?