Re: [PATCH v3 bpf-next] selftests/bpf: Use the last page in test_snprintf_btf on s390

[Ilya Leoshkevich <iii@linux.ibm.com>]

On Fri, 2021-02-26 at 19:47 -0800, Yonghong Song wrote:
> 
> 
> On 2/26/21 11:09 AM, Ilya Leoshkevich wrote:
> > test_snprintf_btf fails on s390, because NULL points to a readable
> > struct lowcore there. Fix by using the last page instead.
> > 
> > Error message example:
> > 
> >      printing 0000000000000000 should generate error, got (361)
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> > 
> > v1: 
> > https://lore.kernel.org/bpf/20210226135923.114211-1-iii@linux.ibm.com/
> > v1 -> v2: Yonghong suggested to add the pointer value to the error
> >            message.
> >            I've noticed that I've been passing BADPTR as flags,
> > therefore
> >            the fix worked only by accident. Put it into p.ptr where
> > it
> >            belongs.
> > 
> > v2: 
> > https://lore.kernel.org/bpf/20210226182014.115347-1-iii@linux.ibm.com/
> > v2 -> v3: Heiko mentioned that using _REGION1_SIZE is not future-
> > proof.
> >            We had a private discussion and came to the conclusion
> > that
> >            the the last page is good enough.
> 
> Heiko, could you ack the patch if it is okay? Thanks!
> 
> > 
> >   .../testing/selftests/bpf/progs/netif_receive_skb.c | 13
> > ++++++++++---
> >   1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/progs/netif_receive_skb.c
> > b/tools/testing/selftests/bpf/progs/netif_receive_skb.c
> > index 6b670039ea67..c3669967067e 100644
> > --- a/tools/testing/selftests/bpf/progs/netif_receive_skb.c
> > +++ b/tools/testing/selftests/bpf/progs/netif_receive_skb.c
> > @@ -16,6 +16,13 @@ bool skip = false;
> >   #define STRSIZE                       2048
> >   #define EXPECTED_STRSIZE      256
> >   
> > +#if defined(bpf_target_s390)
> > +/* NULL points to a readable struct lowcore on s390, so take the
> > last page */
> > +#define BADPTR                 ((void *)0xFFFFFFFFFFFFF000ULL)
> > +#else
> > +#define BADPTR                 0
> > +#endif
> > +
> >   #ifndef ARRAY_SIZE
> >   #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> >   #endif
> > @@ -113,11 +120,11 @@ int BPF_PROG(trace_netif_receive_skb, struct
> > sk_buff *skb)
> >         }
> >   
> >         /* Check invalid ptr value */
> > -       p.ptr = 0;
> > +       p.ptr = BADPTR;
> >         __ret = bpf_snprintf_btf(str, STRSIZE, &p, sizeof(p), 0);
> >         if (__ret >= 0) {
> > -               bpf_printk("printing NULL should generate error,
> > got (%d)",
> > -                          __ret);
> > +               bpf_printk("printing %p should generate error, got
> > (%d)",
> > +                          BADPTR, __ret);
> 
>  From https://www.kernel.org/doc/Documentation/printk-formats.txt:
> 
> Pointers printed without a specifier extension (i.e unadorned %p) are
> hashed to give a unique identifier without leaking kernel addresses
> to user
> space. On 64 bit machines the first 32 bits are zeroed. If you
> _really_
> want the address see %px below.
> 
> I think it is okay to use %px here.

I don't think bpf_trace_printk supports it, but I'll use %llx instead.

[...]