* [dpdk-stable] [dpdk-dev] [PATCH] bpf: fix to allow ptr stack program type
@ 2019-08-09 16:29 jerinj
2019-08-12 8:49 ` Ananyev, Konstantin
0 siblings, 1 reply; 6+ messages in thread
From: jerinj @ 2019-08-09 16:29 UTC (permalink / raw)
To: dev, Konstantin Ananyev; +Cc: thomas, Jerin Jacob, stable
From: Jerin Jacob <jerinj@marvell.com>
bpf_validate does not allow to execute
RTE_BPF_ARG_PTR_STACK for no reason.
Fix it by enhancing the prog_arg.type check.
Fixes: 6e12ec4c4d6d ("bpf: add more checks")
Cc: stable@dpdk.org
Signed-off-by: Jerin Jacob <jerinj@marvell.com>
---
lib/librte_bpf/bpf_validate.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
index 0cf41fa27..c75777b6d 100644
--- a/lib/librte_bpf/bpf_validate.c
+++ b/lib/librte_bpf/bpf_validate.c
@@ -2216,6 +2216,7 @@ bpf_validate(struct rte_bpf *bpf)
/* check input argument type, don't allow mbuf ptr on 32-bit */
if (bpf->prm.prog_arg.type != RTE_BPF_ARG_RAW &&
+ bpf->prm.prog_arg.type != RTE_BPF_ARG_PTR_STACK &&
bpf->prm.prog_arg.type != RTE_BPF_ARG_PTR &&
(sizeof(uint64_t) != sizeof(uintptr_t) ||
bpf->prm.prog_arg.type != RTE_BPF_ARG_PTR_MBUF)) {
--
2.22.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-stable] [dpdk-dev] [PATCH] bpf: fix to allow ptr stack program type
2019-08-09 16:29 [dpdk-stable] [dpdk-dev] [PATCH] bpf: fix to allow ptr stack program type jerinj
@ 2019-08-12 8:49 ` Ananyev, Konstantin
2019-08-12 10:34 ` Jerin Jacob Kollanukkaran
0 siblings, 1 reply; 6+ messages in thread
From: Ananyev, Konstantin @ 2019-08-12 8:49 UTC (permalink / raw)
To: jerinj, dev; +Cc: thomas, stable
Hi Jerin,
>
> bpf_validate does not allow to execute
> RTE_BPF_ARG_PTR_STACK for no reason.
I believe there is a reason,
ARG_PTR_STACK is reserved for memory within BPF program internal stack only.
User that calls BPF program should never pass parameter with that type.
If the user allocates parameter for bpf function on the stack, he can
still use RTE_BPF_ARG_PTR for it.
Konstantin
> Fix it by enhancing the prog_arg.type check.
>
> Fixes: 6e12ec4c4d6d ("bpf: add more checks")
> Cc: stable@dpdk.org
>
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> ---
> lib/librte_bpf/bpf_validate.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
> index 0cf41fa27..c75777b6d 100644
> --- a/lib/librte_bpf/bpf_validate.c
> +++ b/lib/librte_bpf/bpf_validate.c
> @@ -2216,6 +2216,7 @@ bpf_validate(struct rte_bpf *bpf)
>
> /* check input argument type, don't allow mbuf ptr on 32-bit */
> if (bpf->prm.prog_arg.type != RTE_BPF_ARG_RAW &&
> + bpf->prm.prog_arg.type != RTE_BPF_ARG_PTR_STACK &&
> bpf->prm.prog_arg.type != RTE_BPF_ARG_PTR &&
> (sizeof(uint64_t) != sizeof(uintptr_t) ||
> bpf->prm.prog_arg.type != RTE_BPF_ARG_PTR_MBUF)) {
> --
> 2.22.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-stable] [dpdk-dev] [PATCH] bpf: fix to allow ptr stack program type
2019-08-12 8:49 ` Ananyev, Konstantin
@ 2019-08-12 10:34 ` Jerin Jacob Kollanukkaran
2019-08-12 11:37 ` Ananyev, Konstantin
0 siblings, 1 reply; 6+ messages in thread
From: Jerin Jacob Kollanukkaran @ 2019-08-12 10:34 UTC (permalink / raw)
To: Ananyev, Konstantin, dev; +Cc: thomas, stable
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Monday, August 12, 2019 2:20 PM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; stable@dpdk.org
> Subject: [EXT] RE: [dpdk-dev] [PATCH] bpf: fix to allow ptr stack program
> type
>
> ----------------------------------------------------------------------
> Hi Jerin,
Hi Konstantin,
>
> >
> > bpf_validate does not allow to execute RTE_BPF_ARG_PTR_STACK for no
> > reason.
>
> I believe there is a reason,
> ARG_PTR_STACK is reserved for memory within BPF program internal stack
> only.
> User that calls BPF program should never pass parameter with that type.
OK.
Shouldn't we remove that from public header file
(lib/librte_bpf/rte_bpf.h) then ?
> If the user allocates parameter for bpf function on the stack, he can still use
> RTE_BPF_ARG_PTR for it.
I see the _stack_ is only allocated under RTE_BPF_ARG_PTR_STACK checks in bpf_validate.c.
Can you check? I agree that stack should be allocated for RTE_BPF_ARG_PTR as well.
I am writing the arm64 JIT support now, I see always stack of size of 0. I did not spend much
Time on the generic piece of ebpf code(Focusing only on JIT side now).
Can you share more detail the stack allocation scheme, Is validate code parse the eBPF opcode and
Figure out the stack depth it by its own and pass to JIT function where Arch code can allocate
enough stack.
> Konstantin
>
> > Fix it by enhancing the prog_arg.type check.
> >
> > Fixes: 6e12ec4c4d6d ("bpf: add more checks")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> > ---
> > lib/librte_bpf/bpf_validate.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/librte_bpf/bpf_validate.c
> > b/lib/librte_bpf/bpf_validate.c index 0cf41fa27..c75777b6d 100644
> > --- a/lib/librte_bpf/bpf_validate.c
> > +++ b/lib/librte_bpf/bpf_validate.c
> > @@ -2216,6 +2216,7 @@ bpf_validate(struct rte_bpf *bpf)
> >
> > /* check input argument type, don't allow mbuf ptr on 32-bit */
> > if (bpf->prm.prog_arg.type != RTE_BPF_ARG_RAW &&
> > + bpf->prm.prog_arg.type !=
> RTE_BPF_ARG_PTR_STACK &&
> > bpf->prm.prog_arg.type != RTE_BPF_ARG_PTR &&
> > (sizeof(uint64_t) != sizeof(uintptr_t) ||
> > bpf->prm.prog_arg.type !=
> RTE_BPF_ARG_PTR_MBUF)) {
> > --
> > 2.22.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-stable] [dpdk-dev] [PATCH] bpf: fix to allow ptr stack program type
2019-08-12 10:34 ` Jerin Jacob Kollanukkaran
@ 2019-08-12 11:37 ` Ananyev, Konstantin
2019-08-13 3:31 ` Jerin Jacob Kollanukkaran
0 siblings, 1 reply; 6+ messages in thread
From: Ananyev, Konstantin @ 2019-08-12 11:37 UTC (permalink / raw)
To: Jerin Jacob Kollanukkaran, dev; +Cc: thomas, stable
> > Hi Jerin,
>
> Hi Konstantin,
>
> >
> > >
> > > bpf_validate does not allow to execute RTE_BPF_ARG_PTR_STACK for no
> > > reason.
> >
> > I believe there is a reason,
> > ARG_PTR_STACK is reserved for memory within BPF program internal stack
> > only.
> > User that calls BPF program should never pass parameter with that type.
>
> OK.
> Shouldn't we remove that from public header file
> (lib/librte_bpf/rte_bpf.h) then ?
Probably... or might be just put extra comments to mark it as internal?
The reason I left it here, so we can add new public values for enum here,
before RTE_BPF_ARG_PTR_STACK.
Of course in theory we can use for RTE_BPF_ARG_PTR_STACK some reserved
value instead.
>
> > If the user allocates parameter for bpf function on the stack, he can still use
> > RTE_BPF_ARG_PTR for it.
>
> I see the _stack_ is only allocated under RTE_BPF_ARG_PTR_STACK checks in bpf_validate.c.
> Can you check? I agree that stack should be allocated for RTE_BPF_ARG_PTR as well.
Not sure I understand your query here...
Each BPF program can use up to MAX_BPF_STACK_SIZE bytes for stack.
Though to avoid JIT to allocate unused space for the stack, in bpf_validate.c
we figure out does given BPF program really allocate things on the stack and if yes,
how many bytes is needed.
This info is stored in rte_bpf.stack_sz and can be used later by the JIT.
Let say for x86 jit is used in emit_prolog().
>
> I am writing the arm64 JIT support now, I see always stack of size of 0. I did not spend much
> Time on the generic piece of ebpf code(Focusing only on JIT side now).
>
> Can you share more detail the stack allocation scheme, Is validate code parse the eBPF opcode and
> Figure out the stack depth it by its own and pass to JIT function where Arch code can allocate
> enough stack.
Yep, see above.
Konstantin
>
>
> >
> > > Fix it by enhancing the prog_arg.type check.
> > >
> > > Fixes: 6e12ec4c4d6d ("bpf: add more checks")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> > > ---
> > > lib/librte_bpf/bpf_validate.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/lib/librte_bpf/bpf_validate.c
> > > b/lib/librte_bpf/bpf_validate.c index 0cf41fa27..c75777b6d 100644
> > > --- a/lib/librte_bpf/bpf_validate.c
> > > +++ b/lib/librte_bpf/bpf_validate.c
> > > @@ -2216,6 +2216,7 @@ bpf_validate(struct rte_bpf *bpf)
> > >
> > > /* check input argument type, don't allow mbuf ptr on 32-bit */
> > > if (bpf->prm.prog_arg.type != RTE_BPF_ARG_RAW &&
> > > + bpf->prm.prog_arg.type !=
> > RTE_BPF_ARG_PTR_STACK &&
> > > bpf->prm.prog_arg.type != RTE_BPF_ARG_PTR &&
> > > (sizeof(uint64_t) != sizeof(uintptr_t) ||
> > > bpf->prm.prog_arg.type !=
> > RTE_BPF_ARG_PTR_MBUF)) {
> > > --
> > > 2.22.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-stable] [dpdk-dev] [PATCH] bpf: fix to allow ptr stack program type
2019-08-12 11:37 ` Ananyev, Konstantin
@ 2019-08-13 3:31 ` Jerin Jacob Kollanukkaran
2019-08-13 6:50 ` Ananyev, Konstantin
0 siblings, 1 reply; 6+ messages in thread
From: Jerin Jacob Kollanukkaran @ 2019-08-13 3:31 UTC (permalink / raw)
To: Ananyev, Konstantin, dev; +Cc: thomas, stable
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Monday, August 12, 2019 5:08 PM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; stable@dpdk.org
> Subject: [EXT] RE: [dpdk-dev] [PATCH] bpf: fix to allow ptr stack program
> type
>
> External Email
>
> ----------------------------------------------------------------------
>
> > > Hi Jerin,
> >
> > Hi Konstantin,
> >
> > >
> > > >
> > > > bpf_validate does not allow to execute RTE_BPF_ARG_PTR_STACK for
> > > > no reason.
> > >
> > > I believe there is a reason,
> > > ARG_PTR_STACK is reserved for memory within BPF program internal
> > > stack only.
> > > User that calls BPF program should never pass parameter with that type.
> >
> > OK.
> > Shouldn't we remove that from public header file
> > (lib/librte_bpf/rte_bpf.h) then ?
>
> Probably... or might be just put extra comments to mark it as internal?
> The reason I left it here, so we can add new public values for enum here,
> before RTE_BPF_ARG_PTR_STACK.
> Of course in theory we can use for RTE_BPF_ARG_PTR_STACK some
> reserved value instead.
>
> >
> > > If the user allocates parameter for bpf function on the stack, he
> > > can still use RTE_BPF_ARG_PTR for it.
> >
> > I see the _stack_ is only allocated under RTE_BPF_ARG_PTR_STACK checks
> in bpf_validate.c.
> > Can you check? I agree that stack should be allocated for
> RTE_BPF_ARG_PTR as well.
>
> Not sure I understand your query here...
> Each BPF program can use up to MAX_BPF_STACK_SIZE bytes for stack.
> Though to avoid JIT to allocate unused space for the stack, in bpf_validate.c
> we figure out does given BPF program really allocate things on the stack and
> if yes, how many bytes is needed.
> This info is stored in rte_bpf.stack_sz and can be used later by the JIT.
> Let say for x86 jit is used in emit_prolog().
I thought, stack will be allocated only when user gives
RTE_BPF_ARG_PTR_STACK.
I tested following program with RTE_BPF_ARG_PTR. It allocates stacks
Properly. So everything is good.
stdw [r10-64], 0xab
mov r0, 0
exit
I will modify this patch to following to avoid any confusion to user:
1) Change RTE_BPF_ARG_PTR_STACK to RTE_BPF_ARG_RESERVED in public header file
2) In the implementation #define RTE_BPF_ARG_RESERVED BPF_ARG_PTR_STACK
Is it OK?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] bpf: fix to allow ptr stack program type
2019-08-13 3:31 ` Jerin Jacob Kollanukkaran
@ 2019-08-13 6:50 ` Ananyev, Konstantin
0 siblings, 0 replies; 6+ messages in thread
From: Ananyev, Konstantin @ 2019-08-13 6:50 UTC (permalink / raw)
To: Jerin Jacob Kollanukkaran, dev; +Cc: thomas, stable
Hi Jerin,
> > > > >
> > > > > bpf_validate does not allow to execute RTE_BPF_ARG_PTR_STACK for
> > > > > no reason.
> > > >
> > > > I believe there is a reason,
> > > > ARG_PTR_STACK is reserved for memory within BPF program internal
> > > > stack only.
> > > > User that calls BPF program should never pass parameter with that type.
> > >
> > > OK.
> > > Shouldn't we remove that from public header file
> > > (lib/librte_bpf/rte_bpf.h) then ?
> >
> > Probably... or might be just put extra comments to mark it as internal?
> > The reason I left it here, so we can add new public values for enum here,
> > before RTE_BPF_ARG_PTR_STACK.
> > Of course in theory we can use for RTE_BPF_ARG_PTR_STACK some
> > reserved value instead.
> >
> > >
> > > > If the user allocates parameter for bpf function on the stack, he
> > > > can still use RTE_BPF_ARG_PTR for it.
> > >
> > > I see the _stack_ is only allocated under RTE_BPF_ARG_PTR_STACK checks
> > in bpf_validate.c.
> > > Can you check? I agree that stack should be allocated for
> > RTE_BPF_ARG_PTR as well.
> >
> > Not sure I understand your query here...
> > Each BPF program can use up to MAX_BPF_STACK_SIZE bytes for stack.
> > Though to avoid JIT to allocate unused space for the stack, in bpf_validate.c
> > we figure out does given BPF program really allocate things on the stack and
> > if yes, how many bytes is needed.
> > This info is stored in rte_bpf.stack_sz and can be used later by the JIT.
> > Let say for x86 jit is used in emit_prolog().
>
> I thought, stack will be allocated only when user gives
> RTE_BPF_ARG_PTR_STACK.
> I tested following program with RTE_BPF_ARG_PTR. It allocates stacks
> Properly. So everything is good.
>
> stdw [r10-64], 0xab
> mov r0, 0
> exit
>
> I will modify this patch to following to avoid any confusion to user:
> 1) Change RTE_BPF_ARG_PTR_STACK to RTE_BPF_ARG_RESERVED in public header file
> 2) In the implementation #define RTE_BPF_ARG_RESERVED BPF_ARG_PTR_STACK
>
> Is it OK?
Yes, sounds like a good approach to me.
Thanks
Konstantin
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-13 6:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-09 16:29 [dpdk-stable] [dpdk-dev] [PATCH] bpf: fix to allow ptr stack program type jerinj
2019-08-12 8:49 ` Ananyev, Konstantin
2019-08-12 10:34 ` Jerin Jacob Kollanukkaran
2019-08-12 11:37 ` Ananyev, Konstantin
2019-08-13 3:31 ` Jerin Jacob Kollanukkaran
2019-08-13 6:50 ` Ananyev, Konstantin
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).