bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 bpf-next] selftests/bpf: Use the last page in test_snprintf_btf on s390
@ 2021-02-26 19:09 Ilya Leoshkevich
  2021-02-27  3:47 ` Yonghong Song
  0 siblings, 1 reply; 4+ messages in thread
From: Ilya Leoshkevich @ 2021-02-26 19:09 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Yonghong Song, Heiko Carstens
  Cc: bpf, Vasily Gorbik, Ilya Leoshkevich

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.

 .../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);
 		ret = -ERANGE;
 	}
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v3 bpf-next] selftests/bpf: Use the last page in test_snprintf_btf on s390
  2021-02-26 19:09 [PATCH v3 bpf-next] selftests/bpf: Use the last page in test_snprintf_btf on s390 Ilya Leoshkevich
@ 2021-02-27  3:47 ` Yonghong Song
  2021-02-27  5:13   ` Ilya Leoshkevich
  2021-02-28  8:02   ` Heiko Carstens
  0 siblings, 2 replies; 4+ messages in thread
From: Yonghong Song @ 2021-02-27  3:47 UTC (permalink / raw)
  To: Ilya Leoshkevich, Alexei Starovoitov, Daniel Borkmann, Heiko Carstens
  Cc: bpf, Vasily Gorbik



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;
>   	}
>   
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3 bpf-next] selftests/bpf: Use the last page in test_snprintf_btf on s390
  2021-02-27  3:47 ` Yonghong Song
@ 2021-02-27  5:13   ` Ilya Leoshkevich
  2021-02-28  8:02   ` Heiko Carstens
  1 sibling, 0 replies; 4+ messages in thread
From: Ilya Leoshkevich @ 2021-02-27  5:13 UTC (permalink / raw)
  To: Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Heiko Carstens
  Cc: bpf, Vasily Gorbik

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.

[...]


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3 bpf-next] selftests/bpf: Use the last page in test_snprintf_btf on s390
  2021-02-27  3:47 ` Yonghong Song
  2021-02-27  5:13   ` Ilya Leoshkevich
@ 2021-02-28  8:02   ` Heiko Carstens
  1 sibling, 0 replies; 4+ messages in thread
From: Heiko Carstens @ 2021-02-28  8:02 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Ilya Leoshkevich, Alexei Starovoitov, Daniel Borkmann, bpf,
	Vasily Gorbik

On Fri, Feb 26, 2021 at 07:47:02PM -0800, Yonghong Song 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>
> > ---
> > 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!

Yes, sure.

Acked-by: Heiko Carstens <hca@linux.ibm.com>

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-02-28  8:03 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-26 19:09 [PATCH v3 bpf-next] selftests/bpf: Use the last page in test_snprintf_btf on s390 Ilya Leoshkevich
2021-02-27  3:47 ` Yonghong Song
2021-02-27  5:13   ` Ilya Leoshkevich
2021-02-28  8:02   ` Heiko Carstens

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).