These patches relax a few verifier requirements around dynptrs.
I was unable to test the patch in 0003 due to unrelated issues compiling the
bpf selftests, but did run an equivalent local test program.
This is the issue I was running into:
progs/cgrp_ls_attach_cgroup.c:17:15: error: use of undeclared identifier 'BPF_MAP_TYPE_CGRP_STORAGE'; did you mean 'BPF_MAP_TYPE_CGROUP_STORAGE'?
__uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
^~~~~~~~~~~~~~~~~~~~~~~~~
BPF_MAP_TYPE_CGROUP_STORAGE
/ssd/kernel/fuse-bpf/tools/testing/selftests/bpf/tools/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint'
#define __uint(name, val) int (*name)[val]
^
/ssd/kernel/fuse-bpf/tools/testing/selftests/bpf/tools/include/vmlinux.h:27892:2: note: 'BPF_MAP_TYPE_CGROUP_STORAGE' declared here
BPF_MAP_TYPE_CGROUP_STORAGE = 19,
^
1 error generated.
Daniel Rosenberg (3):
bpf: verifier: Accept dynptr mem as mem in helpers
bpf: Allow NULL buffers in bpf_dynptr_slice(_rw)
selftests/bpf: Test allowing NULL buffer in dynptr slice
Documentation/bpf/kfuncs.rst | 23 ++++++++++++-
kernel/bpf/helpers.c | 32 ++++++++++++-------
kernel/bpf/verifier.c | 21 ++++++++++++
.../testing/selftests/bpf/prog_tests/dynptr.c | 1 +
.../selftests/bpf/progs/dynptr_success.c | 21 ++++++++++++
5 files changed, 85 insertions(+), 13 deletions(-)
base-commit: 5af607a861d43ffff830fc1890033e579ec44799
--
2.40.0.577.gac1e443424-goog
This allows using memory retrieved from dynptrs with helper functions
that accept ARG_PTR_TO_MEM. For instance, results from bpf_dynptr_data
can be passed along to bpf_strncmp.
Signed-off-by: Daniel Rosenberg <[email protected]>
---
kernel/bpf/verifier.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 56f569811f70..20beab52812a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7164,12 +7164,16 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
* ARG_PTR_TO_MEM + MAYBE_NULL is compatible with PTR_TO_MEM and PTR_TO_MEM + MAYBE_NULL,
* but ARG_PTR_TO_MEM is compatible only with PTR_TO_MEM but NOT with PTR_TO_MEM + MAYBE_NULL
*
+ * ARG_PTR_TO_MEM is compatible with PTR_TO_MEM that is tagged with a dynptr type.
+ *
* Therefore we fold these flags depending on the arg_type before comparison.
*/
if (arg_type & MEM_RDONLY)
type &= ~MEM_RDONLY;
if (arg_type & PTR_MAYBE_NULL)
type &= ~PTR_MAYBE_NULL;
+ if (base_type(arg_type) == ARG_PTR_TO_MEM)
+ type &= ~DYNPTR_TYPE_FLAG_MASK;
if (meta->func_id == BPF_FUNC_kptr_xchg && type & MEM_ALLOC)
type &= ~MEM_ALLOC;
--
2.40.0.577.gac1e443424-goog
bpf_dynptr_slice(_rw) uses a user provided buffer if it can not provide
a pointer to a block of contiguous memory. This buffer is unused in the
case of local dynptrs, and may be unused in other cases as well. There
is no need to require the buffer, as the kfunc can just return NULL if
it was needed and not provided.
This adds another kfunc annotation, __opt, which combines with __sz and
__szk to allow the buffer associated with the size to be NULL. If the
buffer is NULL, the verifier does not check that the buffer is of
sufficient size.
Signed-off-by: Daniel Rosenberg <[email protected]>
---
Documentation/bpf/kfuncs.rst | 23 ++++++++++++++++++++++-
kernel/bpf/helpers.c | 32 ++++++++++++++++++++------------
kernel/bpf/verifier.c | 17 +++++++++++++++++
3 files changed, 59 insertions(+), 13 deletions(-)
diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
index d8a16c4bef7f..69573b511233 100644
--- a/Documentation/bpf/kfuncs.rst
+++ b/Documentation/bpf/kfuncs.rst
@@ -100,7 +100,7 @@ Hence, whenever a constant scalar argument is accepted by a kfunc which is not a
size parameter, and the value of the constant matters for program safety, __k
suffix should be used.
-2.2.2 __uninit Annotation
+2.2.3 __uninit Annotation
-------------------------
This annotation is used to indicate that the argument will be treated as
@@ -117,6 +117,27 @@ Here, the dynptr will be treated as an uninitialized dynptr. Without this
annotation, the verifier will reject the program if the dynptr passed in is
not initialized.
+2.2.4 __opt Annotation
+-------------------------
+
+This annotation is used to indicate that the buffer associated with an __sz or __szk
+argument may be null. If the function is passed a nullptr in place of the buffer,
+the verifier will not check that length is appropriate for the buffer. The kfunc is
+responsible for checking if this buffer is null before using it.
+
+An example is given below::
+
+ __bpf_kfunc void *bpf_dynptr_slice(..., void *buffer__opt, u32 buffer__szk)
+ {
+ ...
+ }
+
+Here, the buffer may be null. If buffer is not null, it at least of size buffer_szk.
+Either way, the returned buffer is either NULL, or of size buffer_szk. Without this
+annotation, the verifier will reject the program if a null pointer is passed in with
+a nonzero size.
+
+
.. _BPF_kfunc_nodef:
2.3 Using an existing kernel function
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 6be16db9f188..f08556fd8b96 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2145,13 +2145,15 @@ __bpf_kfunc struct task_struct *bpf_task_from_pid(s32 pid)
* bpf_dynptr_slice() - Obtain a read-only pointer to the dynptr data.
* @ptr: The dynptr whose data slice to retrieve
* @offset: Offset into the dynptr
- * @buffer: User-provided buffer to copy contents into
- * @buffer__szk: Size (in bytes) of the buffer. This is the length of the
- * requested slice. This must be a constant.
+ * @buffer__opt: User-provided buffer to copy contents into. May be NULL
+ * @buffer__szk: Size (in bytes) of the buffer if present. This is the
+ * length of the requested slice. This must be a constant.
*
* For non-skb and non-xdp type dynptrs, there is no difference between
* bpf_dynptr_slice and bpf_dynptr_data.
*
+ * If buffer__opt is NULL, the call will fail if buffer_opt was needed.
+ *
* If the intention is to write to the data slice, please use
* bpf_dynptr_slice_rdwr.
*
@@ -2168,7 +2170,7 @@ __bpf_kfunc struct task_struct *bpf_task_from_pid(s32 pid)
* direct pointer)
*/
__bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset,
- void *buffer, u32 buffer__szk)
+ void *buffer__opt, u32 buffer__szk)
{
enum bpf_dynptr_type type;
u32 len = buffer__szk;
@@ -2188,15 +2190,19 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
case BPF_DYNPTR_TYPE_RINGBUF:
return ptr->data + ptr->offset + offset;
case BPF_DYNPTR_TYPE_SKB:
- return skb_header_pointer(ptr->data, ptr->offset + offset, len, buffer);
+ if (!buffer__opt)
+ return NULL;
+ return skb_header_pointer(ptr->data, ptr->offset + offset, len, buffer__opt);
case BPF_DYNPTR_TYPE_XDP:
{
void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset + offset, len);
if (xdp_ptr)
return xdp_ptr;
- bpf_xdp_copy_buf(ptr->data, ptr->offset + offset, buffer, len, false);
- return buffer;
+ if (!buffer__opt)
+ return NULL;
+ bpf_xdp_copy_buf(ptr->data, ptr->offset + offset, buffer__opt, len, false);
+ return buffer__opt;
}
default:
WARN_ONCE(true, "unknown dynptr type %d\n", type);
@@ -2208,13 +2214,15 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
* bpf_dynptr_slice_rdwr() - Obtain a writable pointer to the dynptr data.
* @ptr: The dynptr whose data slice to retrieve
* @offset: Offset into the dynptr
- * @buffer: User-provided buffer to copy contents into
- * @buffer__szk: Size (in bytes) of the buffer. This is the length of the
- * requested slice. This must be a constant.
+ * @buffer__opt: User-provided buffer to copy contents into. May be NULL
+ * @buffer__szk: Size (in bytes) of the buffer if present. This is the
+ * length of the requested slice. This must be a constant.
*
* For non-skb and non-xdp type dynptrs, there is no difference between
* bpf_dynptr_slice and bpf_dynptr_data.
*
+ * If buffer__opt is NULL, the call will fail if buffer_opt was needed.
+ *
* The returned pointer is writable and may point to either directly the dynptr
* data at the requested offset or to the buffer if unable to obtain a direct
* data pointer to (example: the requested slice is to the paged area of an skb
@@ -2245,7 +2253,7 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
* direct pointer)
*/
__bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 offset,
- void *buffer, u32 buffer__szk)
+ void *buffer__opt, u32 buffer__szk)
{
if (!ptr->data || bpf_dynptr_is_rdonly(ptr))
return NULL;
@@ -2272,7 +2280,7 @@ __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 o
* will be copied out into the buffer and the user will need to call
* bpf_dynptr_write() to commit changes.
*/
- return bpf_dynptr_slice(ptr, offset, buffer, buffer__szk);
+ return bpf_dynptr_slice(ptr, offset, buffer__opt, buffer__szk);
}
__bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 20beab52812a..b82faef389b1 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9428,6 +9428,19 @@ static bool is_kfunc_arg_const_mem_size(const struct btf *btf,
return __kfunc_param_match_suffix(btf, arg, "__szk");
}
+static bool is_kfunc_arg_optional(const struct btf *btf,
+ const struct btf_param *arg,
+ const struct bpf_reg_state *reg)
+{
+ const struct btf_type *t;
+
+ t = btf_type_skip_modifiers(btf, arg->type, NULL);
+ if (!btf_type_is_ptr(t) || reg->type != SCALAR_VALUE || reg->umax_value > 0)
+ return false;
+
+ return __kfunc_param_match_suffix(btf, arg, "__opt");
+}
+
static bool is_kfunc_arg_constant(const struct btf *btf, const struct btf_param *arg)
{
return __kfunc_param_match_suffix(btf, arg, "__k");
@@ -10539,10 +10552,14 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
break;
case KF_ARG_PTR_TO_MEM_SIZE:
{
+ struct bpf_reg_state *buff_reg = ®s[regno];
+ const struct btf_param *buff_arg = &args[i];
struct bpf_reg_state *size_reg = ®s[regno + 1];
const struct btf_param *size_arg = &args[i + 1];
ret = check_kfunc_mem_size_reg(env, size_reg, regno + 1);
+ if (ret < 0 && is_kfunc_arg_optional(meta->btf, buff_arg, buff_reg))
+ ret = 0;
if (ret < 0) {
verbose(env, "arg#%d arg#%d memory, len pair leads to invalid memory access\n", i, i + 1);
return ret;
--
2.40.0.577.gac1e443424-goog
bpf_dynptr_slice(_rw) no longer requires a buffer for verification. If the
buffer is needed, but not present, the function will return NULL.
Signed-off-by: Daniel Rosenberg <[email protected]>
---
.../testing/selftests/bpf/prog_tests/dynptr.c | 1 +
.../selftests/bpf/progs/dynptr_success.c | 21 +++++++++++++++++++
2 files changed, 22 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
index d176c34a7d2e..db22cad32657 100644
--- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
+++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
@@ -20,6 +20,7 @@ static struct {
{"test_ringbuf", SETUP_SYSCALL_SLEEP},
{"test_skb_readonly", SETUP_SKB_PROG},
{"test_dynptr_skb_data", SETUP_SKB_PROG},
+ {"test_dynptr_skb_nobuff", SETUP_SKB_PROG},
};
static void verify_success(const char *prog_name, enum test_setup_type setup_type)
diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c b/tools/testing/selftests/bpf/progs/dynptr_success.c
index b2fa6c47ecc0..a059ed8d4590 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_success.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
@@ -207,3 +207,24 @@ int test_dynptr_skb_data(struct __sk_buff *skb)
return 1;
}
+
+SEC("?cgroup_skb/egress")
+int test_dynptr_skb_no_buff(struct __sk_buff *skb)
+{
+ struct bpf_dynptr ptr;
+ __u64 *data;
+
+ if (bpf_dynptr_from_skb(skb, 0, &ptr)) {
+ err = 1;
+ return 1;
+ }
+
+ /* This should return NULL. SKB may require a buffer */
+ data = bpf_dynptr_slice(&ptr, 0, NULL, 1);
+ if (data) {
+ err = 2;
+ return 1;
+ }
+
+ return 1;
+}
--
2.40.0.577.gac1e443424-goog
On Wed, Apr 5, 2023 at 5:40 PM Daniel Rosenberg <[email protected]> wrote:
>
> These patches relax a few verifier requirements around dynptrs.
>
> I was unable to test the patch in 0003 due to unrelated issues compiling the
> bpf selftests, but did run an equivalent local test program.
>
> This is the issue I was running into:
> progs/cgrp_ls_attach_cgroup.c:17:15: error: use of undeclared identifier 'BPF_MAP_TYPE_CGRP_STORAGE'; did you mean 'BPF_MAP_TYPE_CGROUP_STORAGE'?
> __uint(type, BPF_MAP_TYPE_CGRP_STORAGE);
> ^~~~~~~~~~~~~~~~~~~~~~~~~
> BPF_MAP_TYPE_CGROUP_STORAGE
> /ssd/kernel/fuse-bpf/tools/testing/selftests/bpf/tools/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint'
> #define __uint(name, val) int (*name)[val]
> ^
> /ssd/kernel/fuse-bpf/tools/testing/selftests/bpf/tools/include/vmlinux.h:27892:2: note: 'BPF_MAP_TYPE_CGROUP_STORAGE' declared here
> BPF_MAP_TYPE_CGROUP_STORAGE = 19,
> ^
> 1 error generated.
It is expected that you build the freshest vmlinux image before
building selftests, because we generate vmlinux.h from it. In your
case we generated vmlinux.h from your system-wide
/sys/kernel/btf/vmlinux BTF information, which doesn't yet have latest
UAPI enums.
>
> Daniel Rosenberg (3):
> bpf: verifier: Accept dynptr mem as mem in helpers
> bpf: Allow NULL buffers in bpf_dynptr_slice(_rw)
> selftests/bpf: Test allowing NULL buffer in dynptr slice
>
> Documentation/bpf/kfuncs.rst | 23 ++++++++++++-
> kernel/bpf/helpers.c | 32 ++++++++++++-------
> kernel/bpf/verifier.c | 21 ++++++++++++
> .../testing/selftests/bpf/prog_tests/dynptr.c | 1 +
> .../selftests/bpf/progs/dynptr_success.c | 21 ++++++++++++
> 5 files changed, 85 insertions(+), 13 deletions(-)
>
>
> base-commit: 5af607a861d43ffff830fc1890033e579ec44799
> --
> 2.40.0.577.gac1e443424-goog
>
On Wed, Apr 5, 2023 at 5:40 PM Daniel Rosenberg <[email protected]> wrote:
>
> This allows using memory retrieved from dynptrs with helper functions
> that accept ARG_PTR_TO_MEM. For instance, results from bpf_dynptr_data
> can be passed along to bpf_strncmp.
>
> Signed-off-by: Daniel Rosenberg <[email protected]>
> ---
I think patches like this should be targeted against bpf-next, so for
next revision please send them against bpf-next tree.
> kernel/bpf/verifier.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 56f569811f70..20beab52812a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7164,12 +7164,16 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> * ARG_PTR_TO_MEM + MAYBE_NULL is compatible with PTR_TO_MEM and PTR_TO_MEM + MAYBE_NULL,
> * but ARG_PTR_TO_MEM is compatible only with PTR_TO_MEM but NOT with PTR_TO_MEM + MAYBE_NULL
> *
> + * ARG_PTR_TO_MEM is compatible with PTR_TO_MEM that is tagged with a dynptr type.
> + *
> * Therefore we fold these flags depending on the arg_type before comparison.
> */
> if (arg_type & MEM_RDONLY)
> type &= ~MEM_RDONLY;
> if (arg_type & PTR_MAYBE_NULL)
> type &= ~PTR_MAYBE_NULL;
> + if (base_type(arg_type) == ARG_PTR_TO_MEM)
> + type &= ~DYNPTR_TYPE_FLAG_MASK;
Something feels off here. Can you paste a bit of verifier log for the
failure you were getting. And let's have a selftest for this situation
as well.
ARG_PTR_TO_MEM shouldn't be qualified with the DYNPTR_TYPE flag, it's
just memory, there is no need to know what type of dynptr it was
derived from. So if that happens, the problem is somewhere else. Let's
root cause and fix that. Having a selftest that demonstrates the
problem will help with that.
>
> if (meta->func_id == BPF_FUNC_kptr_xchg && type & MEM_ALLOC)
> type &= ~MEM_ALLOC;
> --
> 2.40.0.577.gac1e443424-goog
>
On Wed, Apr 5, 2023 at 5:40 PM Daniel Rosenberg <[email protected]> wrote:
>
> bpf_dynptr_slice(_rw) uses a user provided buffer if it can not provide
> a pointer to a block of contiguous memory. This buffer is unused in the
> case of local dynptrs, and may be unused in other cases as well. There
> is no need to require the buffer, as the kfunc can just return NULL if
> it was needed and not provided.
>
> This adds another kfunc annotation, __opt, which combines with __sz and
> __szk to allow the buffer associated with the size to be NULL. If the
> buffer is NULL, the verifier does not check that the buffer is of
> sufficient size.
>
> Signed-off-by: Daniel Rosenberg <[email protected]>
> ---
> Documentation/bpf/kfuncs.rst | 23 ++++++++++++++++++++++-
> kernel/bpf/helpers.c | 32 ++++++++++++++++++++------------
> kernel/bpf/verifier.c | 17 +++++++++++++++++
> 3 files changed, 59 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> index d8a16c4bef7f..69573b511233 100644
> --- a/Documentation/bpf/kfuncs.rst
> +++ b/Documentation/bpf/kfuncs.rst
> @@ -100,7 +100,7 @@ Hence, whenever a constant scalar argument is accepted by a kfunc which is not a
> size parameter, and the value of the constant matters for program safety, __k
> suffix should be used.
>
> -2.2.2 __uninit Annotation
> +2.2.3 __uninit Annotation
> -------------------------
>
> This annotation is used to indicate that the argument will be treated as
> @@ -117,6 +117,27 @@ Here, the dynptr will be treated as an uninitialized dynptr. Without this
> annotation, the verifier will reject the program if the dynptr passed in is
> not initialized.
>
> +2.2.4 __opt Annotation
> +-------------------------
> +
> +This annotation is used to indicate that the buffer associated with an __sz or __szk
> +argument may be null. If the function is passed a nullptr in place of the buffer,
> +the verifier will not check that length is appropriate for the buffer. The kfunc is
> +responsible for checking if this buffer is null before using it.
> +
> +An example is given below::
> +
> + __bpf_kfunc void *bpf_dynptr_slice(..., void *buffer__opt, u32 buffer__szk)
> + {
> + ...
> + }
> +
> +Here, the buffer may be null. If buffer is not null, it at least of size buffer_szk.
> +Either way, the returned buffer is either NULL, or of size buffer_szk. Without this
> +annotation, the verifier will reject the program if a null pointer is passed in with
> +a nonzero size.
> +
> +
> .. _BPF_kfunc_nodef:
>
> 2.3 Using an existing kernel function
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 6be16db9f188..f08556fd8b96 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2145,13 +2145,15 @@ __bpf_kfunc struct task_struct *bpf_task_from_pid(s32 pid)
> * bpf_dynptr_slice() - Obtain a read-only pointer to the dynptr data.
> * @ptr: The dynptr whose data slice to retrieve
> * @offset: Offset into the dynptr
> - * @buffer: User-provided buffer to copy contents into
> - * @buffer__szk: Size (in bytes) of the buffer. This is the length of the
> - * requested slice. This must be a constant.
> + * @buffer__opt: User-provided buffer to copy contents into. May be NULL
> + * @buffer__szk: Size (in bytes) of the buffer if present. This is the
> + * length of the requested slice. This must be a constant.
> *
> * For non-skb and non-xdp type dynptrs, there is no difference between
> * bpf_dynptr_slice and bpf_dynptr_data.
> *
> + * If buffer__opt is NULL, the call will fail if buffer_opt was needed.
> + *
> * If the intention is to write to the data slice, please use
> * bpf_dynptr_slice_rdwr.
> *
> @@ -2168,7 +2170,7 @@ __bpf_kfunc struct task_struct *bpf_task_from_pid(s32 pid)
> * direct pointer)
> */
> __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset,
> - void *buffer, u32 buffer__szk)
> + void *buffer__opt, u32 buffer__szk)
> {
> enum bpf_dynptr_type type;
> u32 len = buffer__szk;
> @@ -2188,15 +2190,19 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
> case BPF_DYNPTR_TYPE_RINGBUF:
> return ptr->data + ptr->offset + offset;
> case BPF_DYNPTR_TYPE_SKB:
> - return skb_header_pointer(ptr->data, ptr->offset + offset, len, buffer);
> + if (!buffer__opt)
> + return NULL;
should we always reject NULL even for SKB/XDP or only when the buffer
*would be* required? If the latter, we could use bpf_dynptr_slice()
with NULL buf to say "only return pointer if no byte copying is
required". As opposed to bpf_dynptr_data(), where I think we always
fail for SKB/XDP, because we are not sure whether users are aware of
this need to copy bytes. Here, users are aware, but chose to prevent
copying.
WDYT?
> + return skb_header_pointer(ptr->data, ptr->offset + offset, len, buffer__opt);
> case BPF_DYNPTR_TYPE_XDP:
> {
> void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset + offset, len);
> if (xdp_ptr)
> return xdp_ptr;
>
> - bpf_xdp_copy_buf(ptr->data, ptr->offset + offset, buffer, len, false);
> - return buffer;
> + if (!buffer__opt)
> + return NULL;
> + bpf_xdp_copy_buf(ptr->data, ptr->offset + offset, buffer__opt, len, false);
> + return buffer__opt;
> }
> default:
> WARN_ONCE(true, "unknown dynptr type %d\n", type);
> @@ -2208,13 +2214,15 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
> * bpf_dynptr_slice_rdwr() - Obtain a writable pointer to the dynptr data.
> * @ptr: The dynptr whose data slice to retrieve
> * @offset: Offset into the dynptr
> - * @buffer: User-provided buffer to copy contents into
> - * @buffer__szk: Size (in bytes) of the buffer. This is the length of the
> - * requested slice. This must be a constant.
> + * @buffer__opt: User-provided buffer to copy contents into. May be NULL
> + * @buffer__szk: Size (in bytes) of the buffer if present. This is the
> + * length of the requested slice. This must be a constant.
> *
> * For non-skb and non-xdp type dynptrs, there is no difference between
> * bpf_dynptr_slice and bpf_dynptr_data.
> *
> + * If buffer__opt is NULL, the call will fail if buffer_opt was needed.
> + *
> * The returned pointer is writable and may point to either directly the dynptr
> * data at the requested offset or to the buffer if unable to obtain a direct
> * data pointer to (example: the requested slice is to the paged area of an skb
> @@ -2245,7 +2253,7 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
> * direct pointer)
> */
> __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 offset,
> - void *buffer, u32 buffer__szk)
> + void *buffer__opt, u32 buffer__szk)
> {
> if (!ptr->data || bpf_dynptr_is_rdonly(ptr))
> return NULL;
> @@ -2272,7 +2280,7 @@ __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 o
> * will be copied out into the buffer and the user will need to call
> * bpf_dynptr_write() to commit changes.
> */
> - return bpf_dynptr_slice(ptr, offset, buffer, buffer__szk);
> + return bpf_dynptr_slice(ptr, offset, buffer__opt, buffer__szk);
> }
>
> __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 20beab52812a..b82faef389b1 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9428,6 +9428,19 @@ static bool is_kfunc_arg_const_mem_size(const struct btf *btf,
> return __kfunc_param_match_suffix(btf, arg, "__szk");
> }
>
> +static bool is_kfunc_arg_optional(const struct btf *btf,
> + const struct btf_param *arg,
> + const struct bpf_reg_state *reg)
> +{
> + const struct btf_type *t;
> +
> + t = btf_type_skip_modifiers(btf, arg->type, NULL);
> + if (!btf_type_is_ptr(t) || reg->type != SCALAR_VALUE || reg->umax_value > 0)
> + return false;
> +
> + return __kfunc_param_match_suffix(btf, arg, "__opt");
> +}
> +
> static bool is_kfunc_arg_constant(const struct btf *btf, const struct btf_param *arg)
> {
> return __kfunc_param_match_suffix(btf, arg, "__k");
> @@ -10539,10 +10552,14 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> break;
> case KF_ARG_PTR_TO_MEM_SIZE:
> {
> + struct bpf_reg_state *buff_reg = ®s[regno];
> + const struct btf_param *buff_arg = &args[i];
> struct bpf_reg_state *size_reg = ®s[regno + 1];
> const struct btf_param *size_arg = &args[i + 1];
>
> ret = check_kfunc_mem_size_reg(env, size_reg, regno + 1);
> + if (ret < 0 && is_kfunc_arg_optional(meta->btf, buff_arg, buff_reg))
> + ret = 0;
would this work correctly if someone passes a non-null buffer with too
small size? Can you please add a test for this use case.
Also, I feel like for cases where we allow a NULL buffer, we need to
explicitly check that the register is a *known* NULL (SCALAR=0
basically). And also in that case the size of the buffer probably
should be enforced to zero, not just be allowed to be any value.
it's scary to just ignore some error, tbh, the number of error
conditions can grow overtime and we'll be masking them with this
is_kfunc_arg_optional() override. Let's be strict and explicit here.
> if (ret < 0) {
> verbose(env, "arg#%d arg#%d memory, len pair leads to invalid memory access\n", i, i + 1);
> return ret;
> --
> 2.40.0.577.gac1e443424-goog
>
On Thu, Apr 6, 2023 at 1:55 PM Andrii Nakryiko
<[email protected]> wrote:
>
> On Wed, Apr 5, 2023 at 5:40 PM Daniel Rosenberg <[email protected]> wrote:
> >
> > This allows using memory retrieved from dynptrs with helper functions
> > that accept ARG_PTR_TO_MEM. For instance, results from bpf_dynptr_data
> > can be passed along to bpf_strncmp.
> >
> > Signed-off-by: Daniel Rosenberg <[email protected]>
> > ---
>
> I think patches like this should be targeted against bpf-next, so for
> next revision please send them against bpf-next tree.
>
> > kernel/bpf/verifier.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 56f569811f70..20beab52812a 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -7164,12 +7164,16 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> > * ARG_PTR_TO_MEM + MAYBE_NULL is compatible with PTR_TO_MEM and PTR_TO_MEM + MAYBE_NULL,
> > * but ARG_PTR_TO_MEM is compatible only with PTR_TO_MEM but NOT with PTR_TO_MEM + MAYBE_NULL
> > *
> > + * ARG_PTR_TO_MEM is compatible with PTR_TO_MEM that is tagged with a dynptr type.
> > + *
> > * Therefore we fold these flags depending on the arg_type before comparison.
> > */
> > if (arg_type & MEM_RDONLY)
> > type &= ~MEM_RDONLY;
> > if (arg_type & PTR_MAYBE_NULL)
> > type &= ~PTR_MAYBE_NULL;
> > + if (base_type(arg_type) == ARG_PTR_TO_MEM)
> > + type &= ~DYNPTR_TYPE_FLAG_MASK;
>
> Something feels off here. Can you paste a bit of verifier log for the
> failure you were getting. And let's have a selftest for this situation
> as well.
>
> ARG_PTR_TO_MEM shouldn't be qualified with the DYNPTR_TYPE flag, it's
> just memory, there is no need to know what type of dynptr it was
> derived from. So if that happens, the problem is somewhere else. Let's
> root cause and fix that. Having a selftest that demonstrates the
> problem will help with that.
+1
All of the DYNPTR_TYPE_FLAG_MASK flags cannot appear in type == reg->type
here.
They are either dynamic flags inside bpf_dynptr_kern->size
or in arg_type.
Like in bpf_dynptr_from_mem_proto.
On Thu, Apr 6, 2023 at 2:09 PM Andrii Nakryiko
<[email protected]> wrote:
>
> should we always reject NULL even for SKB/XDP or only when the buffer
> *would be* required? If the latter, we could use bpf_dynptr_slice()
> with NULL buf to say "only return pointer if no byte copying is
> required". As opposed to bpf_dynptr_data(), where I think we always
> fail for SKB/XDP, because we are not sure whether users are aware of
> this need to copy bytes. Here, users are aware, but chose to prevent
> copying.
>
> WDYT?
>
I think Passing NULL here should signal that you're quite okay with it
failing instead of copying. We could limit this to just local/ringbuf
types, but that seems like an unneeded restriction, particularly if
you're operating on some section of an skb/xdp buffer that you know
will always be contiguous.
I adjusted xdp for that. The adjustment would be similar for skb, I
just didn't do that since it was another layer of indirection deep and
hadn't looked through all of those use cases. Though it should be fine
to just reject when the buffer would have been needed, since all users
currently provide one.
I agree that allowing that same behavior for dnyptr_data would be more
likely to cause confusion. Blocking the copy on dynprt_slice is much
more explicit.
>
> would this work correctly if someone passes a non-null buffer with too
> small size? Can you please add a test for this use case.
>
Yes, that's one of the tests that's missing there. Once I get my build
situation sorted I'll add more tests. The behavior for a non-null
buffer should be unchanged by this patch.
> Also, I feel like for cases where we allow a NULL buffer, we need to
> explicitly check that the register is a *known* NULL (SCALAR=0
> basically). And also in that case the size of the buffer probably
> should be enforced to zero, not just be allowed to be any value.
>
We absolutely should check that the pointer in question is NULL before
ignoring the size check. I think I'm accomplishing that by ignoring
__opt when reg->umax_value > 0 in is_kfunc_arg_optional. Is that the
wrong check? Perhaps I should check var_off == tnum_const(0) instead.
We can't enforce the size being zero in this case because the size is
doing double duty. It's both the length of the requested area of
access into the dnyptr, and the size of the buffer that it might copy
that data into. If we don't provide a buffer, then it doesn't make
sense to check that buffer's size. The size does still apply to the
returned pointer though. Within the kfunc, it just needs to check for
null before copying dynptr data, as well as the regular enforcement of
length against the dynprt/offset.
> it's scary to just ignore some error, tbh, the number of error
> conditions can grow overtime and we'll be masking them with this
> is_kfunc_arg_optional() override. Let's be strict and explicit here.
>
It would probably make more sense to check is_kfunc_arg_optional and
skip the size check altogether. Either way we're just relying on
runtime checks against the dynptr at that point. If the buffer is
known null and optional, we don't care what the relationship between
the buffer and the size is, just that size and the dynptr. __szk
already takes care of it being a constant. This doesn't affect the
return buffer size.
On Thu, Apr 6, 2023 at 1:55 PM Andrii Nakryiko
<[email protected]> wrote:
>
> Something feels off here. Can you paste a bit of verifier log for the
> failure you were getting. And let's have a selftest for this situation
> as well.
>
> ARG_PTR_TO_MEM shouldn't be qualified with the DYNPTR_TYPE flag, it's
> just memory, there is no need to know what type of dynptr it was
> derived from. So if that happens, the problem is somewhere else. Let's
> root cause and fix that. Having a selftest that demonstrates the
> problem will help with that.
>
>
This label is added by dynptr_slice(_rdwr)
if (meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice] ||
meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice_rdwr]) {
enum bpf_type_flag type_flag =
get_dynptr_type_flag(meta.initialized_dynptr.type);
...
regs[BPF_REG_0].type = PTR_TO_MEM | type_flag;
That extra flag was causing the type to be unexpected later on.
I'll add a selftest for this as well.
On Thu, Apr 6, 2023 at 3:25 PM Daniel Rosenberg <[email protected]> wrote:
>
> On Thu, Apr 6, 2023 at 2:09 PM Andrii Nakryiko
> <[email protected]> wrote:
> >
> > should we always reject NULL even for SKB/XDP or only when the buffer
> > *would be* required? If the latter, we could use bpf_dynptr_slice()
> > with NULL buf to say "only return pointer if no byte copying is
> > required". As opposed to bpf_dynptr_data(), where I think we always
> > fail for SKB/XDP, because we are not sure whether users are aware of
> > this need to copy bytes. Here, users are aware, but chose to prevent
> > copying.
> >
> > WDYT?
> >
>
> I think Passing NULL here should signal that you're quite okay with it
> failing instead of copying. We could limit this to just local/ringbuf
> types, but that seems like an unneeded restriction, particularly if
> you're operating on some section of an skb/xdp buffer that you know
> will always be contiguous.
> I adjusted xdp for that. The adjustment would be similar for skb, I
> just didn't do that since it was another layer of indirection deep and
> hadn't looked through all of those use cases. Though it should be fine
> to just reject when the buffer would have been needed, since all users
> currently provide one.
> I agree that allowing that same behavior for dnyptr_data would be more
> likely to cause confusion. Blocking the copy on dynprt_slice is much
> more explicit.
>
> >
> > would this work correctly if someone passes a non-null buffer with too
> > small size? Can you please add a test for this use case.
> >
>
> Yes, that's one of the tests that's missing there. Once I get my build
> situation sorted I'll add more tests. The behavior for a non-null
> buffer should be unchanged by this patch.
cool, sounds good
>
> > Also, I feel like for cases where we allow a NULL buffer, we need to
> > explicitly check that the register is a *known* NULL (SCALAR=0
> > basically). And also in that case the size of the buffer probably
> > should be enforced to zero, not just be allowed to be any value.
> >
>
> We absolutely should check that the pointer in question is NULL before
> ignoring the size check. I think I'm accomplishing that by ignoring
> __opt when reg->umax_value > 0 in is_kfunc_arg_optional. Is that the
> wrong check? Perhaps I should check var_off == tnum_const(0) instead.
yeah, umax_value is probably wrong check, use register_is_null()
but this approach, even if correct, is a bit too subtle. I'd code it explicitly:
- if __opt, then we know it *can be NULL*
- if so, we need to consider two situations
- it is NULL, then don't enforce buffer size
- it is not NULL (or may be not NULL), then enforce buffer size
Basically, conflating check whether argument is marked as opt with
enforcement of all the implied conditions seems very error-prone.
>
> We can't enforce the size being zero in this case because the size is
> doing double duty. It's both the length of the requested area of
> access into the dnyptr, and the size of the buffer that it might copy
yep, completely missed this double use of that constant, ignore my
point about enforcing sz==0 then
> that data into. If we don't provide a buffer, then it doesn't make
> sense to check that buffer's size. The size does still apply to the
> returned pointer though. Within the kfunc, it just needs to check for
yep
> null before copying dynptr data, as well as the regular enforcement of
> length against the dynprt/offset.
>
> > it's scary to just ignore some error, tbh, the number of error
> > conditions can grow overtime and we'll be masking them with this
> > is_kfunc_arg_optional() override. Let's be strict and explicit here.
> >
> It would probably make more sense to check is_kfunc_arg_optional and
> skip the size check altogether. Either way we're just relying on
> runtime checks against the dynptr at that point. If the buffer is
> known null and optional, we don't care what the relationship between
> the buffer and the size is, just that size and the dynptr. __szk
> already takes care of it being a constant. This doesn't affect the
> return buffer size.
yep, I agree about the logic, I'm concerned with the conflated
implementation, as I tried to explain above
>
> It is expected that you build the freshest vmlinux image before
> building selftests, because we generate vmlinux.h from it. In your
> case we generated vmlinux.h from your system-wide
> /sys/kernel/btf/vmlinux BTF information, which doesn't yet have latest
> UAPI enums.
>
I'm still unable to build the selftests. I've got it pointed to a
locally built kernel built using the config/config.x86_64, and have
tried running the vmtest.sh script, and building just the tests via
make. I'm using O= to direct it to the out directory for the kernel
build. I've been hitting various errors when trying this. Confusingly
the error message isn't always the same. Currently from a clean build,
it complains about "linux/atomic.h" not found via #include
"../../../include/linux/filter.h"'s in various files. Other times it's
complained about the various helper functions from bpf_helper_defs.h
being unused.
I'm not sure if I'm invoking the command wrong, or missing
dependencies or something. I got past some earlier issues by updating
clang. Any idea what I'm doing wrong?
On Wed, Apr 26, 2023 at 3:07 PM Daniel Rosenberg <[email protected]> wrote:
>
> >
> > It is expected that you build the freshest vmlinux image before
> > building selftests, because we generate vmlinux.h from it. In your
> > case we generated vmlinux.h from your system-wide
> > /sys/kernel/btf/vmlinux BTF information, which doesn't yet have latest
> > UAPI enums.
> >
> I'm still unable to build the selftests. I've got it pointed to a
> locally built kernel built using the config/config.x86_64, and have
> tried running the vmtest.sh script, and building just the tests via
> make. I'm using O= to direct it to the out directory for the kernel
> build. I've been hitting various errors when trying this. Confusingly
> the error message isn't always the same. Currently from a clean build,
> it complains about "linux/atomic.h" not found via #include
> "../../../include/linux/filter.h"'s in various files. Other times it's
> complained about the various helper functions from bpf_helper_defs.h
> being unused.
>
> I'm not sure if I'm invoking the command wrong, or missing
> dependencies or something. I got past some earlier issues by updating
> clang. Any idea what I'm doing wrong?
Don't know, show the sequence of commands you are running?
I have linux source in ~/linux, and KBUILD_OUTPUT set to
~/linux-build/default. And it only takes this:
$ cd ~/linux
$ make -j90 # build kernel
$ cd tools/testing/selftests/bpf
$ make -j90 # build selftests
And that's it.
On Wed, Apr 26, 2023 at 4:39 PM Andrii Nakryiko
<[email protected]> wrote:
>
> Don't know, show the sequence of commands you are running?
>
> I have linux source in ~/linux, and KBUILD_OUTPUT set to
> ~/linux-build/default. And it only takes this:
>
> $ cd ~/linux
> $ make -j90 # build kernel
> $ cd tools/testing/selftests/bpf
> $ make -j90 # build selftests
>
> And that's it.
I've tried the same, modulo some paths. I'm pretty sure it's version
related at this point.
The current issue I'm seeing is "error: indirect call in function,
which are not supported by eBPF" when using GCC-BPF for
progs/bind4_prog.c
Currently using clang 16.0.0 and gcc 12.2.0-14.
I did manage to get it to build by just commenting out TEST_GEN_PROGS
+= test_progs-bpf_gcc
On Thu, Apr 27, 2023 at 4:36 PM Daniel Rosenberg <[email protected]> wrote:
>
> On Wed, Apr 26, 2023 at 4:39 PM Andrii Nakryiko
> <[email protected]> wrote:
> >
> > Don't know, show the sequence of commands you are running?
> >
> > I have linux source in ~/linux, and KBUILD_OUTPUT set to
> > ~/linux-build/default. And it only takes this:
> >
> > $ cd ~/linux
> > $ make -j90 # build kernel
> > $ cd tools/testing/selftests/bpf
> > $ make -j90 # build selftests
> >
> > And that's it.
>
> I've tried the same, modulo some paths. I'm pretty sure it's version
> related at this point.
> The current issue I'm seeing is "error: indirect call in function,
> which are not supported by eBPF" when using GCC-BPF for
> progs/bind4_prog.c
I don't think GCC-BPF is able to compile selftests properly just yet.
So I guess the problem is that you do have some version of gcc-bpf in
the system and selftest's Makefile tries to build gcc variants of
test_progs? That's bad (I don't have GCC-BPF locally, and everyone
else apparently as well).
So for now just `make BPF_GCC=` ? CC'ing Jose, we should probably
agree on some criteria of "GCC-BPF is really capable of building
selftests" and adjust Makefile to only attempt GCC BPF build in that
case.
>
> Currently using clang 16.0.0 and gcc 12.2.0-14.
> I did manage to get it to build by just commenting out TEST_GEN_PROGS
> += test_progs-bpf_gcc
> On Thu, Apr 27, 2023 at 4:36 PM Daniel Rosenberg <[email protected]> wrote:
>>
>> On Wed, Apr 26, 2023 at 4:39 PM Andrii Nakryiko
>> <[email protected]> wrote:
>> >
>> > Don't know, show the sequence of commands you are running?
>> >
>> > I have linux source in ~/linux, and KBUILD_OUTPUT set to
>> > ~/linux-build/default. And it only takes this:
>> >
>> > $ cd ~/linux
>> > $ make -j90 # build kernel
>> > $ cd tools/testing/selftests/bpf
>> > $ make -j90 # build selftests
>> >
>> > And that's it.
>>
>> I've tried the same, modulo some paths. I'm pretty sure it's version
>> related at this point.
>> The current issue I'm seeing is "error: indirect call in function,
>> which are not supported by eBPF" when using GCC-BPF for
>> progs/bind4_prog.c
>
> I don't think GCC-BPF is able to compile selftests properly just yet.
> So I guess the problem is that you do have some version of gcc-bpf in
> the system and selftest's Makefile tries to build gcc variants of
> test_progs? That's bad (I don't have GCC-BPF locally, and everyone
> else apparently as well).
>
> So for now just `make BPF_GCC=` ? CC'ing Jose, we should probably
> agree on some criteria of "GCC-BPF is really capable of building
> selftests" and adjust Makefile to only attempt GCC BPF build in that
> case.
Being able to run the selftests is our goal at the moment, but we are
not there yet, no.
What about making the kernel build system to emit a visible warning
before it builds the GCC variants of the tests programs? Something like
"this is experimental and will likely fail".
>>
>> Currently using clang 16.0.0 and gcc 12.2.0-14.
>> I did manage to get it to build by just commenting out TEST_GEN_PROGS
>> += test_progs-bpf_gcc
On Fri, Apr 28, 2023 at 2:04 AM Jose E. Marchesi
<[email protected]> wrote:
>
>
> > On Thu, Apr 27, 2023 at 4:36 PM Daniel Rosenberg <[email protected]> wrote:
> >>
> >> On Wed, Apr 26, 2023 at 4:39 PM Andrii Nakryiko
> >> <[email protected]> wrote:
> >> >
> >> > Don't know, show the sequence of commands you are running?
> >> >
> >> > I have linux source in ~/linux, and KBUILD_OUTPUT set to
> >> > ~/linux-build/default. And it only takes this:
> >> >
> >> > $ cd ~/linux
> >> > $ make -j90 # build kernel
> >> > $ cd tools/testing/selftests/bpf
> >> > $ make -j90 # build selftests
> >> >
> >> > And that's it.
> >>
> >> I've tried the same, modulo some paths. I'm pretty sure it's version
> >> related at this point.
> >> The current issue I'm seeing is "error: indirect call in function,
> >> which are not supported by eBPF" when using GCC-BPF for
> >> progs/bind4_prog.c
> >
> > I don't think GCC-BPF is able to compile selftests properly just yet.
> > So I guess the problem is that you do have some version of gcc-bpf in
> > the system and selftest's Makefile tries to build gcc variants of
> > test_progs? That's bad (I don't have GCC-BPF locally, and everyone
> > else apparently as well).
> >
> > So for now just `make BPF_GCC=` ? CC'ing Jose, we should probably
> > agree on some criteria of "GCC-BPF is really capable of building
> > selftests" and adjust Makefile to only attempt GCC BPF build in that
> > case.
>
> Being able to run the selftests is our goal at the moment, but we are
> not there yet, no.
>
> What about making the kernel build system to emit a visible warning
> before it builds the GCC variants of the tests programs? Something like
> "this is experimental and will likely fail".
Given gcc-bpf can't build selftests right now, should we just disable
it until there is a version on which gcc-bpf works? We can make it
such that you can force it to build using gcc-bpf (make USE_GCC_BPF=1
or something).
>
> >>
> >> Currently using clang 16.0.0 and gcc 12.2.0-14.
> >> I did manage to get it to build by just commenting out TEST_GEN_PROGS
> >> += test_progs-bpf_gcc
On Thu, Apr 6, 2023 at 2:09 PM Andrii Nakryiko
<[email protected]> wrote:
>
> would this work correctly if someone passes a non-null buffer with too
> small size? Can you please add a test for this use case.
>
Working on a test case for this, but the test case I wrote fails
without my patches.
I'm just declaring a buffer of size 9 on the stack, and then passing
in bpf_dynptr_slice that buffer, and size 10. That's passing the
verifier just fine. In fact, it loads successfully up to size 16. I'm
guessing that's adjusting for alignment? Still feels very strange. Is
that expected behavior?
On Thu, Apr 6, 2023 at 3:13 PM Alexei Starovoitov
<[email protected]> wrote:
>
> +1
> All of the DYNPTR_TYPE_FLAG_MASK flags cannot appear in type == reg->type
> here.
> They are either dynamic flags inside bpf_dynptr_kern->size
> or in arg_type.
> Like in bpf_dynptr_from_mem_proto.
Looking at this a bit more, I believe this is to enforce packet
restrictions for DYNPTR_TYPE_SKB and DYNPTR_TYPE_XDP. When a helper
function alters a packet, dynptr slices of it are invalidated. If I
remove that annotation entirely, then the invalid_data_slice family of
tests fail. bpf_dynptr_from_mem_proto is fine since that's just local
dynptrs, which don't have any extra limitations.
> On Fri, Apr 28, 2023 at 2:04 AM Jose E. Marchesi
> <[email protected]> wrote:
>>
>>
>> > On Thu, Apr 27, 2023 at 4:36 PM Daniel Rosenberg <[email protected]> wrote:
>> >>
>> >> On Wed, Apr 26, 2023 at 4:39 PM Andrii Nakryiko
>> >> <[email protected]> wrote:
>> >> >
>> >> > Don't know, show the sequence of commands you are running?
>> >> >
>> >> > I have linux source in ~/linux, and KBUILD_OUTPUT set to
>> >> > ~/linux-build/default. And it only takes this:
>> >> >
>> >> > $ cd ~/linux
>> >> > $ make -j90 # build kernel
>> >> > $ cd tools/testing/selftests/bpf
>> >> > $ make -j90 # build selftests
>> >> >
>> >> > And that's it.
>> >>
>> >> I've tried the same, modulo some paths. I'm pretty sure it's version
>> >> related at this point.
>> >> The current issue I'm seeing is "error: indirect call in function,
>> >> which are not supported by eBPF" when using GCC-BPF for
>> >> progs/bind4_prog.c
>> >
>> > I don't think GCC-BPF is able to compile selftests properly just yet.
>> > So I guess the problem is that you do have some version of gcc-bpf in
>> > the system and selftest's Makefile tries to build gcc variants of
>> > test_progs? That's bad (I don't have GCC-BPF locally, and everyone
>> > else apparently as well).
>> >
>> > So for now just `make BPF_GCC=` ? CC'ing Jose, we should probably
>> > agree on some criteria of "GCC-BPF is really capable of building
>> > selftests" and adjust Makefile to only attempt GCC BPF build in that
>> > case.
>>
>> Being able to run the selftests is our goal at the moment, but we are
>> not there yet, no.
>>
>> What about making the kernel build system to emit a visible warning
>> before it builds the GCC variants of the tests programs? Something like
>> "this is experimental and will likely fail".
>
> Given gcc-bpf can't build selftests right now, should we just disable
> it until there is a version on which gcc-bpf works? We can make it
> such that you can force it to build using gcc-bpf (make USE_GCC_BPF=1
> or something).
I think that makes sense.
>>
>> >>
>> >> Currently using clang 16.0.0 and gcc 12.2.0-14.
>> >> I did manage to get it to build by just commenting out TEST_GEN_PROGS
>> >> += test_progs-bpf_gcc
On Fri, Apr 28, 2023 at 6:58 PM Daniel Rosenberg <[email protected]> wrote:
>
> On Thu, Apr 6, 2023 at 2:09 PM Andrii Nakryiko
> <[email protected]> wrote:
> >
> > would this work correctly if someone passes a non-null buffer with too
> > small size? Can you please add a test for this use case.
> >
> Working on a test case for this, but the test case I wrote fails
> without my patches.
> I'm just declaring a buffer of size 9 on the stack, and then passing
> in bpf_dynptr_slice that buffer, and size 10. That's passing the
> verifier just fine. In fact, it loads successfully up to size 16. I'm
> guessing that's adjusting for alignment? Still feels very strange. Is
> that expected behavior?
pointer to stack is trickier (verifier will just mark part of stack as
overwritten with random data), it's best to use map value pointer as a
source of buffer. So try using ARRAY map with small value_size, do
lookup_elem, check for NULL, and pass non-NULL pointer as a buffer.
On Mon, May 1, 2023 at 6:12 PM Daniel Rosenberg <[email protected]> wrote:
>
> On Thu, Apr 6, 2023 at 3:13 PM Alexei Starovoitov
> <[email protected]> wrote:
> >
> > +1
> > All of the DYNPTR_TYPE_FLAG_MASK flags cannot appear in type == reg->type
> > here.
> > They are either dynamic flags inside bpf_dynptr_kern->size
> > or in arg_type.
> > Like in bpf_dynptr_from_mem_proto.
>
> Looking at this a bit more, I believe this is to enforce packet
> restrictions for DYNPTR_TYPE_SKB and DYNPTR_TYPE_XDP. When a helper
> function alters a packet, dynptr slices of it are invalidated. If I
> remove that annotation entirely, then the invalid_data_slice family of
> tests fail. bpf_dynptr_from_mem_proto is fine since that's just local
> dynptrs, which don't have any extra limitations.
Ah, ok, thanks for investigating!