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

[Yonghong Song <yhs@fb.com>]

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.

>   		ret = -ERANGE;
>   	}
>   
>