| Message ID | 20210328161055.257504-2-pctammela@mojatatu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: check flags in 'bpf_ringbuf_discard()' and 'bpf_ringbuf_submit()' | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 12 of 12 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 12009 this patch: 12009 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Blank lines aren't necessary after an open brace '{' WARNING: From:/Signed-off-by: email address mismatch: 'From: Pedro Tammela <pctammela@gmail.com>' != 'Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>' WARNING: please, no space before tabs |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 12497 this patch: 12497 |
| netdev/header_inline | success | Link |
> On Mar 28, 2021, at 9:10 AM, Pedro Tammela <pctammela@gmail.com> wrote: > > The current code only checks flags in 'bpf_ringbuf_output()'. > > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> > --- > include/uapi/linux/bpf.h | 8 ++++---- > kernel/bpf/ringbuf.c | 13 +++++++++++-- > tools/include/uapi/linux/bpf.h | 8 ++++---- > 3 files changed, 19 insertions(+), 10 deletions(-) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 100cb2e4c104..232b5e5dd045 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -4073,7 +4073,7 @@ union bpf_attr { > * Valid pointer with *size* bytes of memory available; NULL, > * otherwise. > * > - * void bpf_ringbuf_submit(void *data, u64 flags) > + * int bpf_ringbuf_submit(void *data, u64 flags) This should be "long" instead of "int". > * Description > * Submit reserved ring buffer sample, pointed to by *data*. > * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification > @@ -4083,9 +4083,9 @@ union bpf_attr { > * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification > * of new data availability is sent unconditionally. > * Return > - * Nothing. Always succeeds. > + * 0 on success, or a negative error in case of failure. > * > - * void bpf_ringbuf_discard(void *data, u64 flags) > + * int bpf_ringbuf_discard(void *data, u64 flags) Ditto. And same for tools/include/uapi/linux/bpf.h > * Description > * Discard reserved ring buffer sample, pointed to by *data*. > * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification > @@ -4095,7 +4095,7 @@ union bpf_attr { > * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification > * of new data availability is sent unconditionally. > * Return > - * Nothing. Always succeeds. > + * 0 on success, or a negative error in case of failure. > * > * u64 bpf_ringbuf_query(void *ringbuf, u64 flags) > * Description > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c > index f25b719ac786..f76dafe2427e 100644 > --- a/kernel/bpf/ringbuf.c > +++ b/kernel/bpf/ringbuf.c > @@ -397,26 +397,35 @@ static void bpf_ringbuf_commit(void *sample, u64 flags, bool discard) > > BPF_CALL_2(bpf_ringbuf_submit, void *, sample, u64, flags) > { > + if (unlikely(flags & ~(BPF_RB_NO_WAKEUP | BPF_RB_FORCE_WAKEUP))) > + return -EINVAL; We can move this check to bpf_ringbuf_commit(). Thanks, Song [...]
Em seg., 29 de mar. de 2021 às 13:10, Song Liu <songliubraving@fb.com> escreveu: > > > > > On Mar 28, 2021, at 9:10 AM, Pedro Tammela <pctammela@gmail.com> wrote: > > > > The current code only checks flags in 'bpf_ringbuf_output()'. > > > > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> > > --- > > include/uapi/linux/bpf.h | 8 ++++---- > > kernel/bpf/ringbuf.c | 13 +++++++++++-- > > tools/include/uapi/linux/bpf.h | 8 ++++---- > > 3 files changed, 19 insertions(+), 10 deletions(-) > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index 100cb2e4c104..232b5e5dd045 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -4073,7 +4073,7 @@ union bpf_attr { > > * Valid pointer with *size* bytes of memory available; NULL, > > * otherwise. > > * > > - * void bpf_ringbuf_submit(void *data, u64 flags) > > + * int bpf_ringbuf_submit(void *data, u64 flags) > > This should be "long" instead of "int". > > > * Description > > * Submit reserved ring buffer sample, pointed to by *data*. > > * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification > > @@ -4083,9 +4083,9 @@ union bpf_attr { > > * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification > > * of new data availability is sent unconditionally. > > * Return > > - * Nothing. Always succeeds. > > + * 0 on success, or a negative error in case of failure. > > * > > - * void bpf_ringbuf_discard(void *data, u64 flags) > > + * int bpf_ringbuf_discard(void *data, u64 flags) > > Ditto. And same for tools/include/uapi/linux/bpf.h > > > * Description > > * Discard reserved ring buffer sample, pointed to by *data*. > > * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification > > @@ -4095,7 +4095,7 @@ union bpf_attr { > > * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification > > * of new data availability is sent unconditionally. > > * Return > > - * Nothing. Always succeeds. > > + * 0 on success, or a negative error in case of failure. > > * > > * u64 bpf_ringbuf_query(void *ringbuf, u64 flags) > > * Description > > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c > > index f25b719ac786..f76dafe2427e 100644 > > --- a/kernel/bpf/ringbuf.c > > +++ b/kernel/bpf/ringbuf.c > > @@ -397,26 +397,35 @@ static void bpf_ringbuf_commit(void *sample, u64 flags, bool discard) > > > > BPF_CALL_2(bpf_ringbuf_submit, void *, sample, u64, flags) > > { > > + if (unlikely(flags & ~(BPF_RB_NO_WAKEUP | BPF_RB_FORCE_WAKEUP))) > > + return -EINVAL; > > We can move this check to bpf_ringbuf_commit(). I don't believe we can because in 'bpf_ringbuf_output()' the flag checking in 'bpf_ringbuf_commit()' is already too late. > > Thanks, > Song > > [...] Pedro
> On Mar 30, 2021, at 7:22 AM, Pedro Tammela <pctammela@gmail.com> wrote: > > Em seg., 29 de mar. de 2021 às 13:10, Song Liu <songliubraving@fb.com> escreveu: >> >> >> >>> On Mar 28, 2021, at 9:10 AM, Pedro Tammela <pctammela@gmail.com> wrote: >>> >>> The current code only checks flags in 'bpf_ringbuf_output()'. >>> >>> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> >>> --- >>> include/uapi/linux/bpf.h | 8 ++++---- >>> kernel/bpf/ringbuf.c | 13 +++++++++++-- >>> tools/include/uapi/linux/bpf.h | 8 ++++---- >>> 3 files changed, 19 insertions(+), 10 deletions(-) >>> >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h >>> index 100cb2e4c104..232b5e5dd045 100644 >>> --- a/include/uapi/linux/bpf.h >>> +++ b/include/uapi/linux/bpf.h >>> @@ -4073,7 +4073,7 @@ union bpf_attr { >>> * Valid pointer with *size* bytes of memory available; NULL, >>> * otherwise. >>> * >>> - * void bpf_ringbuf_submit(void *data, u64 flags) >>> + * int bpf_ringbuf_submit(void *data, u64 flags) >> >> This should be "long" instead of "int". >> >>> * Description >>> * Submit reserved ring buffer sample, pointed to by *data*. >>> * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification >>> @@ -4083,9 +4083,9 @@ union bpf_attr { >>> * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification >>> * of new data availability is sent unconditionally. >>> * Return >>> - * Nothing. Always succeeds. >>> + * 0 on success, or a negative error in case of failure. >>> * >>> - * void bpf_ringbuf_discard(void *data, u64 flags) >>> + * int bpf_ringbuf_discard(void *data, u64 flags) >> >> Ditto. And same for tools/include/uapi/linux/bpf.h >> >>> * Description >>> * Discard reserved ring buffer sample, pointed to by *data*. >>> * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification >>> @@ -4095,7 +4095,7 @@ union bpf_attr { >>> * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification >>> * of new data availability is sent unconditionally. >>> * Return >>> - * Nothing. Always succeeds. >>> + * 0 on success, or a negative error in case of failure. >>> * >>> * u64 bpf_ringbuf_query(void *ringbuf, u64 flags) >>> * Description >>> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c >>> index f25b719ac786..f76dafe2427e 100644 >>> --- a/kernel/bpf/ringbuf.c >>> +++ b/kernel/bpf/ringbuf.c >>> @@ -397,26 +397,35 @@ static void bpf_ringbuf_commit(void *sample, u64 flags, bool discard) >>> >>> BPF_CALL_2(bpf_ringbuf_submit, void *, sample, u64, flags) >>> { >>> + if (unlikely(flags & ~(BPF_RB_NO_WAKEUP | BPF_RB_FORCE_WAKEUP))) >>> + return -EINVAL; >> >> We can move this check to bpf_ringbuf_commit(). > > I don't believe we can because in 'bpf_ringbuf_output()' the flag > checking in 'bpf_ringbuf_commit()' is already > too late. I see. Let's keep it in current functions then. Thanks, Song
On Sun, Mar 28, 2021 at 9:12 AM Pedro Tammela <pctammela@gmail.com> wrote: > > The current code only checks flags in 'bpf_ringbuf_output()'. > > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> > --- > include/uapi/linux/bpf.h | 8 ++++---- > kernel/bpf/ringbuf.c | 13 +++++++++++-- > tools/include/uapi/linux/bpf.h | 8 ++++---- > 3 files changed, 19 insertions(+), 10 deletions(-) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 100cb2e4c104..232b5e5dd045 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -4073,7 +4073,7 @@ union bpf_attr { > * Valid pointer with *size* bytes of memory available; NULL, > * otherwise. > * > - * void bpf_ringbuf_submit(void *data, u64 flags) > + * int bpf_ringbuf_submit(void *data, u64 flags) > * Description > * Submit reserved ring buffer sample, pointed to by *data*. > * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification > @@ -4083,9 +4083,9 @@ union bpf_attr { > * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification > * of new data availability is sent unconditionally. > * Return > - * Nothing. Always succeeds. bpf_ringbuf_submit/bpf_ringbuf_commit has to alway succeed. That's an explicit and strict rule, which BPF verifier relies on. We cannot bail out due to unknown flags, because then ringbuf sample won't ever be submitted and will block all the subsequent samples. > + * 0 on success, or a negative error in case of failure. > * [...]
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 100cb2e4c104..232b5e5dd045 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -4073,7 +4073,7 @@ union bpf_attr { * Valid pointer with *size* bytes of memory available; NULL, * otherwise. * - * void bpf_ringbuf_submit(void *data, u64 flags) + * int bpf_ringbuf_submit(void *data, u64 flags) * Description * Submit reserved ring buffer sample, pointed to by *data*. * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification @@ -4083,9 +4083,9 @@ union bpf_attr { * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification * of new data availability is sent unconditionally. * Return - * Nothing. Always succeeds. + * 0 on success, or a negative error in case of failure. * - * void bpf_ringbuf_discard(void *data, u64 flags) + * int bpf_ringbuf_discard(void *data, u64 flags) * Description * Discard reserved ring buffer sample, pointed to by *data*. * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification @@ -4095,7 +4095,7 @@ union bpf_attr { * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification * of new data availability is sent unconditionally. * Return - * Nothing. Always succeeds. + * 0 on success, or a negative error in case of failure. * * u64 bpf_ringbuf_query(void *ringbuf, u64 flags) * Description diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c index f25b719ac786..f76dafe2427e 100644 --- a/kernel/bpf/ringbuf.c +++ b/kernel/bpf/ringbuf.c @@ -397,26 +397,35 @@ static void bpf_ringbuf_commit(void *sample, u64 flags, bool discard) BPF_CALL_2(bpf_ringbuf_submit, void *, sample, u64, flags) { + if (unlikely(flags & ~(BPF_RB_NO_WAKEUP | BPF_RB_FORCE_WAKEUP))) + return -EINVAL; + bpf_ringbuf_commit(sample, flags, false /* discard */); + return 0; } const struct bpf_func_proto bpf_ringbuf_submit_proto = { .func = bpf_ringbuf_submit, - .ret_type = RET_VOID, + .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_ALLOC_MEM, .arg2_type = ARG_ANYTHING, }; BPF_CALL_2(bpf_ringbuf_discard, void *, sample, u64, flags) { + + if (unlikely(flags & ~(BPF_RB_NO_WAKEUP | BPF_RB_FORCE_WAKEUP))) + return -EINVAL; + bpf_ringbuf_commit(sample, flags, true /* discard */); + return 0; } const struct bpf_func_proto bpf_ringbuf_discard_proto = { .func = bpf_ringbuf_discard, - .ret_type = RET_VOID, + .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_ALLOC_MEM, .arg2_type = ARG_ANYTHING, }; diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 3d6d324184c0..d19c8c2688a2 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -4073,7 +4073,7 @@ union bpf_attr { * Valid pointer with *size* bytes of memory available; NULL, * otherwise. * - * void bpf_ringbuf_submit(void *data, u64 flags) + * int bpf_ringbuf_submit(void *data, u64 flags) * Description * Submit reserved ring buffer sample, pointed to by *data*. * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification @@ -4083,9 +4083,9 @@ union bpf_attr { * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification * of new data availability is sent unconditionally. * Return - * Nothing. Always succeeds. + * 0 on success, or a negative error in case of failure. * - * void bpf_ringbuf_discard(void *data, u64 flags) + * int bpf_ringbuf_discard(void *data, u64 flags) * Description * Discard reserved ring buffer sample, pointed to by *data*. * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification @@ -4095,7 +4095,7 @@ union bpf_attr { * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification * of new data availability is sent unconditionally. * Return - * Nothing. Always succeeds. + * 0 on success, or a negative error in case of failure. * * u64 bpf_ringbuf_query(void *ringbuf, u64 flags) * Description
The current code only checks flags in 'bpf_ringbuf_output()'. Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> --- include/uapi/linux/bpf.h | 8 ++++---- kernel/bpf/ringbuf.c | 13 +++++++++++-- tools/include/uapi/linux/bpf.h | 8 ++++---- 3 files changed, 19 insertions(+), 10 deletions(-)