[PATCH bpf-next v2] bpf: check flags in 'bpf_ringbuf_discard()' and 'bpf_ringbuf_submit()'

The current code only checks flags in 'bpf_ringbuf_output()'.

Signed-off-by: Pedro Tammela <redacted>
---
 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..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

-- 
2.25.1

On Mar 30, 2021, at 3:37 PM, Pedro Tammela [off-list ref] wrote:

The current code only checks flags in 'bpf_ringbuf_output()'.

Signed-off-by: Pedro Tammela <redacted>

Acked-by: Song Liu <redacted>

On Tue, Mar 30, 2021 at 3:54 PM Pedro Tammela [off-list ref] 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
[off-list ref] wrote:

On Tue, Mar 30, 2021 at 3:54 PM Pedro Tammela [off-list ref] wrote:

quoted

 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
[off-list ref] escreveu:

On Tue, Mar 30, 2021 at 4:16 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Tue, Mar 30, 2021 at 3:54 PM Pedro Tammela [off-list ref] wrote:

quoted

 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 [off-list ref] wrote:

Em qua., 31 de mar. de 2021 às 04:02, Andrii Nakryiko
[off-list ref] escreveu:

quoted

On Tue, Mar 30, 2021 at 4:16 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Tue, Mar 30, 2021 at 3:54 PM Pedro Tammela [off-list ref] wrote:

quoted

 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.