bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling
@ 2021-01-12  9:14 Gilad Reti
  2021-01-12  9:26 ` [PATCH bpf 2/2] selftests/bpf: add verifier test for PTR_TO_MEM spill Gilad Reti
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Gilad Reti @ 2021-01-12  9:14 UTC (permalink / raw)
  To: bpf
  Cc: gilad.reti, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, netdev, linux-kernel

Add support for pointer to mem register spilling, to allow the verifier
to track pointer to valid memory addresses. Such pointers are returned
for example by a successful call of the bpf_ringbuf_reserve helper.

This patch was suggested as a solution by Yonghong Song.

The patch was partially contibuted by CyberArk Software, Inc.

Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier
support for it")
Signed-off-by: Gilad Reti <gilad.reti@gmail.com>
---
 kernel/bpf/verifier.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 17270b8404f1..36af69fac591 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2217,6 +2217,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
 	case PTR_TO_RDWR_BUF:
 	case PTR_TO_RDWR_BUF_OR_NULL:
 	case PTR_TO_PERCPU_BTF_ID:
+	case PTR_TO_MEM:
+	case PTR_TO_MEM_OR_NULL:
 		return true;
 	default:
 		return false;
-- 
2.27.0


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

* [PATCH bpf 2/2] selftests/bpf: add verifier test for PTR_TO_MEM spill
  2021-01-12  9:14 [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling Gilad Reti
@ 2021-01-12  9:26 ` Gilad Reti
  2021-01-12 13:57 ` [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling KP Singh
  2021-01-12 19:46 ` Andrii Nakryiko
  2 siblings, 0 replies; 7+ messages in thread
From: Gilad Reti @ 2021-01-12  9:26 UTC (permalink / raw)
  To: bpf
  Cc: gilad.reti, Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, linux-kselftest, netdev, linux-kernel

Add test to check that the verifier is able to recognize spilling of
PTR_TO_MEM registers.

The patch was partially contibuted by CyberArk Software, Inc.

Signed-off-by: Gilad Reti <gilad.reti@gmail.com>
---
 tools/testing/selftests/bpf/test_verifier.c   | 12 +++++++-
 .../selftests/bpf/verifier/spill_fill.c       | 30 +++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 777a81404fdb..f8569f04064b 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -50,7 +50,7 @@
 #define MAX_INSNS	BPF_MAXINSNS
 #define MAX_TEST_INSNS	1000000
 #define MAX_FIXUPS	8
-#define MAX_NR_MAPS	20
+#define MAX_NR_MAPS	21
 #define MAX_TEST_RUNS	8
 #define POINTER_VALUE	0xcafe4all
 #define TEST_DATA_LEN	64
@@ -87,6 +87,7 @@ struct bpf_test {
 	int fixup_sk_storage_map[MAX_FIXUPS];
 	int fixup_map_event_output[MAX_FIXUPS];
 	int fixup_map_reuseport_array[MAX_FIXUPS];
+	int fixup_map_ringbuf[MAX_FIXUPS];
 	const char *errstr;
 	const char *errstr_unpriv;
 	uint32_t insn_processed;
@@ -640,6 +641,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
 	int *fixup_sk_storage_map = test->fixup_sk_storage_map;
 	int *fixup_map_event_output = test->fixup_map_event_output;
 	int *fixup_map_reuseport_array = test->fixup_map_reuseport_array;
+	int *fixup_map_ringbuf = test->fixup_map_ringbuf;
 
 	if (test->fill_helper) {
 		test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
@@ -817,6 +819,14 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
 			fixup_map_reuseport_array++;
 		} while (*fixup_map_reuseport_array);
 	}
+	if (*fixup_map_ringbuf) {
+		map_fds[20] = create_map(BPF_MAP_TYPE_RINGBUF, 0,
+					   0, 4096);
+		do {
+			prog[*fixup_map_ringbuf].imm = map_fds[20];
+			fixup_map_ringbuf++;
+		} while (*fixup_map_ringbuf);
+	}
 }
 
 struct libcap {
diff --git a/tools/testing/selftests/bpf/verifier/spill_fill.c b/tools/testing/selftests/bpf/verifier/spill_fill.c
index 45d43bf82f26..1833b6c730dd 100644
--- a/tools/testing/selftests/bpf/verifier/spill_fill.c
+++ b/tools/testing/selftests/bpf/verifier/spill_fill.c
@@ -28,6 +28,36 @@
 	.result = ACCEPT,
 	.result_unpriv = ACCEPT,
 },
+{
+	"check valid spill/fill, ptr to mem",
+	.insns = {
+	/* reserve 8 byte ringbuf memory */
+	BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+	BPF_LD_MAP_FD(BPF_REG_1, 0),
+	BPF_MOV64_IMM(BPF_REG_2, 8),
+	BPF_MOV64_IMM(BPF_REG_3, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_reserve),
+	/* store a pointer to the reserved memory in R6 */
+	BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+	/* check whether the reservation was successful */
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),
+	/* spill R6(mem) into the stack */
+	BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_6, -8),
+	/* fill it back in R7 */
+	BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_10, -8),
+	/* should be able to access *(R7) = 0 */
+	BPF_ST_MEM(BPF_DW, BPF_REG_7, 0, 0),
+	/* submit the reserved rungbuf memory */
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_7),
+	BPF_MOV64_IMM(BPF_REG_2, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_submit),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.fixup_map_ringbuf = { 1 },
+	.result = ACCEPT,
+	.result_unpriv = ACCEPT,
+},
 {
 	"check corrupted spill/fill",
 	.insns = {
-- 
2.27.0


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

* Re: [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling
  2021-01-12  9:14 [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling Gilad Reti
  2021-01-12  9:26 ` [PATCH bpf 2/2] selftests/bpf: add verifier test for PTR_TO_MEM spill Gilad Reti
@ 2021-01-12 13:57 ` KP Singh
  2021-01-12 14:23   ` Gilad Reti
  2021-01-12 19:46 ` Andrii Nakryiko
  2 siblings, 1 reply; 7+ messages in thread
From: KP Singh @ 2021-01-12 13:57 UTC (permalink / raw)
  To: Gilad Reti
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Networking, open list

On Tue, Jan 12, 2021 at 10:14 AM Gilad Reti <gilad.reti@gmail.com> wrote:
>
> Add support for pointer to mem register spilling, to allow the verifier
> to track pointer to valid memory addresses. Such pointers are returned

nit: pointers

> for example by a successful call of the bpf_ringbuf_reserve helper.
>
> This patch was suggested as a solution by Yonghong Song.

You can use the "Suggested-by:" tag for this.

>
> The patch was partially contibuted by CyberArk Software, Inc.

nit: typo *contributed

Also, I was wondering if "partially" here means someone collaborated with you
on the patch? And, in that case:

"Co-developed-by:" would be a better tag here.

Acked-by: KP Singh <kpsingh@kernel.org>


>
> Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier
> support for it")
> Signed-off-by: Gilad Reti <gilad.reti@gmail.com>
> ---
>  kernel/bpf/verifier.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 17270b8404f1..36af69fac591 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2217,6 +2217,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
>         case PTR_TO_RDWR_BUF:
>         case PTR_TO_RDWR_BUF_OR_NULL:
>         case PTR_TO_PERCPU_BTF_ID:
> +       case PTR_TO_MEM:
> +       case PTR_TO_MEM_OR_NULL:
>                 return true;
>         default:
>                 return false;
> --
> 2.27.0
>

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

* Re: [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling
  2021-01-12 13:57 ` [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling KP Singh
@ 2021-01-12 14:23   ` Gilad Reti
  2021-01-12 15:01     ` KP Singh
  0 siblings, 1 reply; 7+ messages in thread
From: Gilad Reti @ 2021-01-12 14:23 UTC (permalink / raw)
  To: KP Singh
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Networking, open list

On Tue, Jan 12, 2021 at 3:57 PM KP Singh <kpsingh@kernel.org> wrote:
>
> On Tue, Jan 12, 2021 at 10:14 AM Gilad Reti <gilad.reti@gmail.com> wrote:
> >
> > Add support for pointer to mem register spilling, to allow the verifier
> > to track pointer to valid memory addresses. Such pointers are returned
>
> nit: pointers

Thanks

>
> > for example by a successful call of the bpf_ringbuf_reserve helper.
> >
> > This patch was suggested as a solution by Yonghong Song.
>
> You can use the "Suggested-by:" tag for this.

Thanks

>
> >
> > The patch was partially contibuted by CyberArk Software, Inc.
>
> nit: typo *contributed

Thanks. Should I submit a v2 of the patch to correct all of those?

>
> Also, I was wondering if "partially" here means someone collaborated with you
> on the patch? And, in that case:
>
> "Co-developed-by:" would be a better tag here.

No, I did it alone. I mentioned CyberArk since I work there and did some of the
coding during my daily work, so they deserve credit.

>
> Acked-by: KP Singh <kpsingh@kernel.org>
>
>
> >
> > Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier
> > support for it")
> > Signed-off-by: Gilad Reti <gilad.reti@gmail.com>
> > ---
> >  kernel/bpf/verifier.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 17270b8404f1..36af69fac591 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -2217,6 +2217,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
> >         case PTR_TO_RDWR_BUF:
> >         case PTR_TO_RDWR_BUF_OR_NULL:
> >         case PTR_TO_PERCPU_BTF_ID:
> > +       case PTR_TO_MEM:
> > +       case PTR_TO_MEM_OR_NULL:
> >                 return true;
> >         default:
> >                 return false;
> > --
> > 2.27.0
> >

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

* Re: [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling
  2021-01-12 14:23   ` Gilad Reti
@ 2021-01-12 15:01     ` KP Singh
  0 siblings, 0 replies; 7+ messages in thread
From: KP Singh @ 2021-01-12 15:01 UTC (permalink / raw)
  To: Gilad Reti
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Networking, open list

On Tue, Jan 12, 2021 at 3:24 PM Gilad Reti <gilad.reti@gmail.com> wrote:
>
> On Tue, Jan 12, 2021 at 3:57 PM KP Singh <kpsingh@kernel.org> wrote:
> >
> > On Tue, Jan 12, 2021 at 10:14 AM Gilad Reti <gilad.reti@gmail.com> wrote:
> > >
> > > Add support for pointer to mem register spilling, to allow the verifier
> > > to track pointer to valid memory addresses. Such pointers are returned
> >
> > nit: pointers
>
> Thanks
>
> >
> > > for example by a successful call of the bpf_ringbuf_reserve helper.
> > >
> > > This patch was suggested as a solution by Yonghong Song.
> >
> > You can use the "Suggested-by:" tag for this.
>
> Thanks
>
> >
> > >
> > > The patch was partially contibuted by CyberArk Software, Inc.
> >
> > nit: typo *contributed
>
> Thanks. Should I submit a v2 of the patch to correct all of those?

I think it would be nice to do another revision
which also addresses the comments on the other patch.


>
> >
> > Also, I was wondering if "partially" here means someone collaborated with you
> > on the patch? And, in that case:
> >
> > "Co-developed-by:" would be a better tag here.
>
> No, I did it alone. I mentioned CyberArk since I work there and did some of the
> coding during my daily work, so they deserve credit.
>
> >
> > Acked-by: KP Singh <kpsingh@kernel.org>
> >
> >
> > >
> > > Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier
> > > support for it")
> > > Signed-off-by: Gilad Reti <gilad.reti@gmail.com>
> > > ---
> > >  kernel/bpf/verifier.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 17270b8404f1..36af69fac591 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -2217,6 +2217,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
> > >         case PTR_TO_RDWR_BUF:
> > >         case PTR_TO_RDWR_BUF_OR_NULL:
> > >         case PTR_TO_PERCPU_BTF_ID:
> > > +       case PTR_TO_MEM:
> > > +       case PTR_TO_MEM_OR_NULL:
> > >                 return true;
> > >         default:
> > >                 return false;
> > > --
> > > 2.27.0
> > >

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

* Re: [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling
  2021-01-12  9:14 [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling Gilad Reti
  2021-01-12  9:26 ` [PATCH bpf 2/2] selftests/bpf: add verifier test for PTR_TO_MEM spill Gilad Reti
  2021-01-12 13:57 ` [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling KP Singh
@ 2021-01-12 19:46 ` Andrii Nakryiko
  2021-01-12 20:21   ` Daniel Borkmann
  2 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2021-01-12 19:46 UTC (permalink / raw)
  To: Gilad Reti
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Networking, open list

On Tue, Jan 12, 2021 at 1:14 AM Gilad Reti <gilad.reti@gmail.com> wrote:
>
> Add support for pointer to mem register spilling, to allow the verifier
> to track pointer to valid memory addresses. Such pointers are returned
> for example by a successful call of the bpf_ringbuf_reserve helper.
>
> This patch was suggested as a solution by Yonghong Song.
>
> The patch was partially contibuted by CyberArk Software, Inc.
>
> Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier
> support for it")

Surprised no one mentioned this yet. Fixes tag should always be on a
single unwrapped line, however long it is, please fix.


> Signed-off-by: Gilad Reti <gilad.reti@gmail.com>
> ---
>  kernel/bpf/verifier.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 17270b8404f1..36af69fac591 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2217,6 +2217,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
>         case PTR_TO_RDWR_BUF:
>         case PTR_TO_RDWR_BUF_OR_NULL:
>         case PTR_TO_PERCPU_BTF_ID:
> +       case PTR_TO_MEM:
> +       case PTR_TO_MEM_OR_NULL:
>                 return true;
>         default:
>                 return false;
> --
> 2.27.0
>

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

* Re: [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling
  2021-01-12 19:46 ` Andrii Nakryiko
@ 2021-01-12 20:21   ` Daniel Borkmann
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Borkmann @ 2021-01-12 20:21 UTC (permalink / raw)
  To: Andrii Nakryiko, Gilad Reti
  Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Martin KaFai Lau,
	Song Liu, Yonghong Song, John Fastabend, KP Singh, Networking,
	open list

On 1/12/21 8:46 PM, Andrii Nakryiko wrote:
> On Tue, Jan 12, 2021 at 1:14 AM Gilad Reti <gilad.reti@gmail.com> wrote:
>>
>> Add support for pointer to mem register spilling, to allow the verifier
>> to track pointer to valid memory addresses. Such pointers are returned
>> for example by a successful call of the bpf_ringbuf_reserve helper.
>>
>> This patch was suggested as a solution by Yonghong Song.
>>
>> The patch was partially contibuted by CyberArk Software, Inc.
>>
>> Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier
>> support for it")
> 
> Surprised no one mentioned this yet. Fixes tag should always be on a
> single unwrapped line, however long it is, please fix.

Especially for first-time contributors there is usually some luxury that we
would have fixed this up on the fly while applying. ;) But given a v2 is going
to be sent anyway it's good to point it out for future reference, agree.

Thanks,
Daniel

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

end of thread, other threads:[~2021-01-12 21:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-12  9:14 [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling Gilad Reti
2021-01-12  9:26 ` [PATCH bpf 2/2] selftests/bpf: add verifier test for PTR_TO_MEM spill Gilad Reti
2021-01-12 13:57 ` [PATCH bpf 1/2] bpf: support PTR_TO_MEM{,_OR_NULL} register spilling KP Singh
2021-01-12 14:23   ` Gilad Reti
2021-01-12 15:01     ` KP Singh
2021-01-12 19:46 ` Andrii Nakryiko
2021-01-12 20:21   ` Daniel Borkmann

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).