| Message ID | 20210330223748.399563-1-pctammela@mojatatu.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] 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 30, 2021, at 3:37 PM, Pedro Tammela <pctammela@gmail.com> wrote: > > The current code only checks flags in 'bpf_ringbuf_output()'. > > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> Acked-by: Song Liu <songliubraving@fb.com>
On Tue, Mar 30, 2021 at 3:54 PM Pedro Tammela <pctammela@gmail.com> wrote: > > 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; I think ringbuf design was meant for bpf_ringbuf_submit to never fail. If we do flag validation it probably should be done at the verifier time.
On Tue, Mar 30, 2021 at 4:16 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Mar 30, 2021 at 3:54 PM Pedro Tammela <pctammela@gmail.com> wrote: > > > > 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; > > I think ringbuf design was meant for bpf_ringbuf_submit to never fail. > If we do flag validation it probably should be done at the verifier time. Oops, replied on another version already. But yes, BPF verifier relies on it succeeding. I don't think we can do flags validation at BPF verification time, though, because it is defined as non-const integer and we do have valid cases where we dynamically determine whether to FORCE_WAKEUP or NO_WAKEUP, based on application-driven criteria (e.g., amount of enqueued data).
Em qua., 31 de mar. de 2021 às 04:02, Andrii Nakryiko <andrii.nakryiko@gmail.com> escreveu: > > On Tue, Mar 30, 2021 at 4:16 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Tue, Mar 30, 2021 at 3:54 PM Pedro Tammela <pctammela@gmail.com> wrote: > > > > > > 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; > > > > I think ringbuf design was meant for bpf_ringbuf_submit to never fail. > > If we do flag validation it probably should be done at the verifier time. > > Oops, replied on another version already. But yes, BPF verifier relies > on it succeeding. I don't think we can do flags validation at BPF > verification time, though, because it is defined as non-const integer > and we do have valid cases where we dynamically determine whether to > FORCE_WAKEUP or NO_WAKEUP, based on application-driven criteria (e.g., > amount of enqueued data). Then shouldn't we remove the flags check in 'bpf_ringbuf_output()'?
On Sat, Apr 3, 2021 at 6:29 AM Pedro Tammela <pctammela@gmail.com> wrote: > > Em qua., 31 de mar. de 2021 às 04:02, Andrii Nakryiko > <andrii.nakryiko@gmail.com> escreveu: > > > > On Tue, Mar 30, 2021 at 4:16 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Tue, Mar 30, 2021 at 3:54 PM Pedro Tammela <pctammela@gmail.com> wrote: > > > > > > > > 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; > > > > > > I think ringbuf design was meant for bpf_ringbuf_submit to never fail. > > > If we do flag validation it probably should be done at the verifier time. > > > > Oops, replied on another version already. But yes, BPF verifier relies > > on it succeeding. I don't think we can do flags validation at BPF > > verification time, though, because it is defined as non-const integer > > and we do have valid cases where we dynamically determine whether to > > FORCE_WAKEUP or NO_WAKEUP, based on application-driven criteria (e.g., > > amount of enqueued data). > > Then shouldn't we remove the flags check in 'bpf_ringbuf_output()'? bpf_ringbuf_output() combines reserve + commit operations, so if it performs checks before anything is reserved in ringbuf, it's ok for it to fail and return error. So I don't see any problem there. But once it internally reserves, it always proceeds to complete the commit.
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 100cb2e4c104..38b0b15f99f0 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) + * long 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) + * long 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..a32eefb786f9 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) + * long 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) + * long 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(-)