* [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem.
@ 2021-12-20 20:12 Hao Luo
2021-12-21 4:28 ` Yonghong Song
0 siblings, 1 reply; 6+ messages in thread
From: Hao Luo @ 2021-12-20 20:12 UTC (permalink / raw)
To: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann
Cc: Martin KaFai Lau, Song Liu, Yonghong Song, KP Singh, bpf, Hao Luo
The second parameter of bpf_d_path() can only accept writable
memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not
be passed into bpf_d_path for modification. This patch adds
a selftest to verify this behavior.
Signed-off-by: Hao Luo <haoluo@google.com>
---
.../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++-
.../bpf/progs/test_d_path_check_rdonly_mem.c | 30 +++++++++++++++++++
2 files changed, 51 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
index 0a577a248d34..f8d8c5a5dfba 100644
--- a/tools/testing/selftests/bpf/prog_tests/d_path.c
+++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
@@ -9,6 +9,7 @@
#define MAX_FILES 7
#include "test_d_path.skel.h"
+#include "test_d_path_check_rdonly_mem.skel.h"
static int duration;
@@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid)
return ret;
}
-void test_d_path(void)
+static void test_d_path_basic(void)
{
struct test_d_path__bss *bss;
struct test_d_path *skel;
@@ -155,3 +156,22 @@ void test_d_path(void)
cleanup:
test_d_path__destroy(skel);
}
+
+static void test_d_path_check_rdonly_mem(void)
+{
+ struct test_d_path_check_rdonly_mem *skel;
+
+ skel = test_d_path_check_rdonly_mem__open_and_load();
+ ASSERT_ERR_PTR(skel, "unexpected load of a prog using d_path to write rdonly_mem\n");
+
+ test_d_path_check_rdonly_mem__destroy(skel);
+}
+
+void test_d_path(void)
+{
+ if (test__start_subtest("basic"))
+ test_d_path_basic();
+
+ if (test__start_subtest("check_rdonly_mem"))
+ test_d_path_check_rdonly_mem();
+}
diff --git a/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
new file mode 100644
index 000000000000..c7a9655d5850
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Google */
+
+#include "vmlinux.h"
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+extern const int bpf_prog_active __ksym;
+
+SEC("fentry/security_inode_getattr")
+int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat,
+ __u32 request_mask, unsigned int query_flags)
+{
+ char *active;
+ __u32 cpu;
+
+ cpu = bpf_get_smp_processor_id();
+ active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu);
+ if (active) {
+ /* FAIL here! 'active' is a rdonly_mem. bpf helpers that
+ * update its arguments can not write into it.
+ */
+ bpf_d_path(path, active, sizeof(int));
+ }
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.34.1.307.g9b7440fafd-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem.
2021-12-20 20:12 [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem Hao Luo
@ 2021-12-21 4:28 ` Yonghong Song
2021-12-21 20:16 ` Hao Luo
0 siblings, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2021-12-21 4:28 UTC (permalink / raw)
To: Hao Luo, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann
Cc: Martin KaFai Lau, Song Liu, KP Singh, bpf
On 12/20/21 12:12 PM, Hao Luo wrote:
> The second parameter of bpf_d_path() can only accept writable
> memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not
> be passed into bpf_d_path for modification. This patch adds
> a selftest to verify this behavior.
>
> Signed-off-by: Hao Luo <haoluo@google.com>
> ---
> .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++-
> .../bpf/progs/test_d_path_check_rdonly_mem.c | 30 +++++++++++++++++++
> 2 files changed, 51 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
> index 0a577a248d34..f8d8c5a5dfba 100644
> --- a/tools/testing/selftests/bpf/prog_tests/d_path.c
> +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
> @@ -9,6 +9,7 @@
> #define MAX_FILES 7
>
> #include "test_d_path.skel.h"
> +#include "test_d_path_check_rdonly_mem.skel.h"
>
> static int duration;
>
> @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid)
> return ret;
> }
>
> -void test_d_path(void)
> +static void test_d_path_basic(void)
> {
> struct test_d_path__bss *bss;
> struct test_d_path *skel;
> @@ -155,3 +156,22 @@ void test_d_path(void)
> cleanup:
> test_d_path__destroy(skel);
> }
> +
> +static void test_d_path_check_rdonly_mem(void)
> +{
> + struct test_d_path_check_rdonly_mem *skel;
> +
> + skel = test_d_path_check_rdonly_mem__open_and_load();
> + ASSERT_ERR_PTR(skel, "unexpected load of a prog using d_path to write rdonly_mem\n");
> +
> + test_d_path_check_rdonly_mem__destroy(skel);
You shouldn't call test_d_path_check_rdonly_mem__destroy(skel) if skel
is an ERR_PTR. Maybe
if (!ASSERT_ERR_PTR(...))
test_d_path_check_rdonly_mem__destroy(skel);
> +}
> +
> +void test_d_path(void)
> +{
> + if (test__start_subtest("basic"))
> + test_d_path_basic();
> +
> + if (test__start_subtest("check_rdonly_mem"))
> + test_d_path_check_rdonly_mem();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> new file mode 100644
> index 000000000000..c7a9655d5850
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Google */
> +
> +#include "vmlinux.h"
> +
> +#include "vmlinux.h"
duplicated vmlinux.h.
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +extern const int bpf_prog_active __ksym;
> +
> +SEC("fentry/security_inode_getattr")
> +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat,
> + __u32 request_mask, unsigned int query_flags)
> +{
> + char *active;
int *active?
It may not matter since the program is rejected by the kernel but
with making it conforms to kernel definition we have one less thing
to worry about the verification.
> + __u32 cpu;
> +
> + cpu = bpf_get_smp_processor_id();
> + active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu);
int *
> + if (active) {
> + /* FAIL here! 'active' is a rdonly_mem. bpf helpers that
'active' points to readonly memory.
> + * update its arguments can not write into it.
> + */
> + bpf_d_path(path, active, sizeof(int));
> + }
> + return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem.
2021-12-21 4:28 ` Yonghong Song
@ 2021-12-21 20:16 ` Hao Luo
2021-12-21 22:29 ` Yonghong Song
2021-12-22 0:24 ` Andrii Nakryiko
0 siblings, 2 replies; 6+ messages in thread
From: Hao Luo @ 2021-12-21 20:16 UTC (permalink / raw)
To: Yonghong Song
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Song Liu, KP Singh, bpf
On Mon, Dec 20, 2021 at 8:28 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 12/20/21 12:12 PM, Hao Luo wrote:
> > The second parameter of bpf_d_path() can only accept writable
> > memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not
> > be passed into bpf_d_path for modification. This patch adds
> > a selftest to verify this behavior.
> >
> > Signed-off-by: Hao Luo <haoluo@google.com>
> > ---
> > .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++-
> > .../bpf/progs/test_d_path_check_rdonly_mem.c | 30 +++++++++++++++++++
> > 2 files changed, 51 insertions(+), 1 deletion(-)
> > create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > index 0a577a248d34..f8d8c5a5dfba 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/d_path.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > @@ -9,6 +9,7 @@
> > #define MAX_FILES 7
> >
> > #include "test_d_path.skel.h"
> > +#include "test_d_path_check_rdonly_mem.skel.h"
> >
> > static int duration;
> >
> > @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid)
> > return ret;
> > }
> >
> > -void test_d_path(void)
> > +static void test_d_path_basic(void)
> > {
> > struct test_d_path__bss *bss;
> > struct test_d_path *skel;
> > @@ -155,3 +156,22 @@ void test_d_path(void)
> > cleanup:
> > test_d_path__destroy(skel);
> > }
> > +
> > +static void test_d_path_check_rdonly_mem(void)
> > +{
> > + struct test_d_path_check_rdonly_mem *skel;
> > +
> > + skel = test_d_path_check_rdonly_mem__open_and_load();
> > + ASSERT_ERR_PTR(skel, "unexpected load of a prog using d_path to write rdonly_mem\n");
> > +
> > + test_d_path_check_rdonly_mem__destroy(skel);
>
> You shouldn't call test_d_path_check_rdonly_mem__destroy(skel) if skel
> is an ERR_PTR. Maybe
> if (!ASSERT_ERR_PTR(...))
> test_d_path_check_rdonly_mem__destroy(skel);
>
Ack. Will change that.
I don't know if it's only me: I find it confusing when figuring out
what ASSERT_ERR_PTR(ptr) returns. Is the returned value 'ptr'? or 'ptr
!= NULL'? or 'err != 0'? I used to think ASSERT-like function/macro
returns nothing.
I noticed that xxx__destroy has a check for NULL, so I put the destroy
function unconditionally.
> > +}
> > +
> > +void test_d_path(void)
> > +{
> > + if (test__start_subtest("basic"))
> > + test_d_path_basic();
> > +
> > + if (test__start_subtest("check_rdonly_mem"))
> > + test_d_path_check_rdonly_mem();
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> > new file mode 100644
> > index 000000000000..c7a9655d5850
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> > @@ -0,0 +1,30 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2021 Google */
> > +
> > +#include "vmlinux.h"
> > +
> > +#include "vmlinux.h"
>
> duplicated vmlinux.h.
>
Thanks. Will fix that.
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +extern const int bpf_prog_active __ksym;
> > +
> > +SEC("fentry/security_inode_getattr")
> > +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat,
> > + __u32 request_mask, unsigned int query_flags)
> > +{
> > + char *active;
>
> int *active?
> It may not matter since the program is rejected by the kernel but
> with making it conforms to kernel definition we have one less thing
> to worry about the verification.
>
Because bpf_d_path() accepts 'char *' instead of 'int *', I need to
cast 'active' to 'char *' somewhere, otherwise the compiler will issue
a warning. To combine with your comment, maybe the following:
int *active;
active = (int *)bpf_per_cpu_ptr(...);
...
bpf_d_path(path, (char *)active, sizeof(int));
> > + __u32 cpu;
> > +
> > + cpu = bpf_get_smp_processor_id();
> > + active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu);
>
> int *
>
> > + if (active) {
> > + /* FAIL here! 'active' is a rdonly_mem. bpf helpers that
>
> 'active' points to readonly memory.
>
Ack.
> > + * update its arguments can not write into it.
> > + */
> > + bpf_d_path(path, active, sizeof(int));
> > + }
> > + return 0;
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem.
2021-12-21 20:16 ` Hao Luo
@ 2021-12-21 22:29 ` Yonghong Song
2021-12-22 0:24 ` Andrii Nakryiko
1 sibling, 0 replies; 6+ messages in thread
From: Yonghong Song @ 2021-12-21 22:29 UTC (permalink / raw)
To: Hao Luo
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Song Liu, KP Singh, bpf
On 12/21/21 12:16 PM, Hao Luo wrote:
> On Mon, Dec 20, 2021 at 8:28 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 12/20/21 12:12 PM, Hao Luo wrote:
>>> The second parameter of bpf_d_path() can only accept writable
>>> memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not
>>> be passed into bpf_d_path for modification. This patch adds
>>> a selftest to verify this behavior.
>>>
>>> Signed-off-by: Hao Luo <haoluo@google.com>
>>> ---
>>> .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++-
>>> .../bpf/progs/test_d_path_check_rdonly_mem.c | 30 +++++++++++++++++++
>>> 2 files changed, 51 insertions(+), 1 deletion(-)
>>> create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
>>> index 0a577a248d34..f8d8c5a5dfba 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/d_path.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
>>> @@ -9,6 +9,7 @@
>>> #define MAX_FILES 7
>>>
>>> #include "test_d_path.skel.h"
>>> +#include "test_d_path_check_rdonly_mem.skel.h"
>>>
>>> static int duration;
>>>
>>> @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid)
>>> return ret;
>>> }
>>>
[...]
>>> +
>>> +extern const int bpf_prog_active __ksym;
>>> +
>>> +SEC("fentry/security_inode_getattr")
>>> +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat,
>>> + __u32 request_mask, unsigned int query_flags)
>>> +{
>>> + char *active;
>>
>> int *active?
>> It may not matter since the program is rejected by the kernel but
>> with making it conforms to kernel definition we have one less thing
>> to worry about the verification.
>>
>
> Because bpf_d_path() accepts 'char *' instead of 'int *', I need to
> cast 'active' to 'char *' somewhere, otherwise the compiler will issue
> a warning. To combine with your comment, maybe the following:
>
> int *active;
> active = (int *)bpf_per_cpu_ptr(...);
> ...
> bpf_d_path(path, (char *)active, sizeof(int));
This is fine. Thanks!
>
>>> + __u32 cpu;
>>> +
>>> + cpu = bpf_get_smp_processor_id();
>>> + active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu);
>>
>> int *
>>
>>> + if (active) {
>>> + /* FAIL here! 'active' is a rdonly_mem. bpf helpers that
>>
>> 'active' points to readonly memory.
>>
>
> Ack.
>
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem.
2021-12-21 20:16 ` Hao Luo
2021-12-21 22:29 ` Yonghong Song
@ 2021-12-22 0:24 ` Andrii Nakryiko
2021-12-22 1:05 ` Hao Luo
1 sibling, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2021-12-22 0:24 UTC (permalink / raw)
To: Hao Luo
Cc: Yonghong Song, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Song Liu, KP Singh, bpf
On Tue, Dec 21, 2021 at 12:16 PM Hao Luo <haoluo@google.com> wrote:
>
> On Mon, Dec 20, 2021 at 8:28 PM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 12/20/21 12:12 PM, Hao Luo wrote:
> > > The second parameter of bpf_d_path() can only accept writable
> > > memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not
> > > be passed into bpf_d_path for modification. This patch adds
> > > a selftest to verify this behavior.
> > >
> > > Signed-off-by: Hao Luo <haoluo@google.com>
> > > ---
> > > .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++-
> > > .../bpf/progs/test_d_path_check_rdonly_mem.c | 30 +++++++++++++++++++
> > > 2 files changed, 51 insertions(+), 1 deletion(-)
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > > index 0a577a248d34..f8d8c5a5dfba 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/d_path.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > > @@ -9,6 +9,7 @@
> > > #define MAX_FILES 7
> > >
> > > #include "test_d_path.skel.h"
> > > +#include "test_d_path_check_rdonly_mem.skel.h"
> > >
> > > static int duration;
> > >
> > > @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid)
> > > return ret;
> > > }
> > >
> > > -void test_d_path(void)
> > > +static void test_d_path_basic(void)
> > > {
> > > struct test_d_path__bss *bss;
> > > struct test_d_path *skel;
> > > @@ -155,3 +156,22 @@ void test_d_path(void)
> > > cleanup:
> > > test_d_path__destroy(skel);
> > > }
> > > +
> > > +static void test_d_path_check_rdonly_mem(void)
> > > +{
> > > + struct test_d_path_check_rdonly_mem *skel;
> > > +
> > > + skel = test_d_path_check_rdonly_mem__open_and_load();
> > > + ASSERT_ERR_PTR(skel, "unexpected load of a prog using d_path to write rdonly_mem\n");
> > > +
> > > + test_d_path_check_rdonly_mem__destroy(skel);
> >
> > You shouldn't call test_d_path_check_rdonly_mem__destroy(skel) if skel
> > is an ERR_PTR. Maybe
> > if (!ASSERT_ERR_PTR(...))
> > test_d_path_check_rdonly_mem__destroy(skel);
> >
>
> Ack. Will change that.
no need, __destroy() handles NULLs and ERR_PTR just fine, the way you
wrote it is totally correct (that's a deliberate nice feature of
libbpf's "destructor" APIs)
>
> I don't know if it's only me: I find it confusing when figuring out
> what ASSERT_ERR_PTR(ptr) returns. Is the returned value 'ptr'? or 'ptr
> != NULL'? or 'err != 0'? I used to think ASSERT-like function/macro
> returns nothing.
>
You haven't looked at many other selftests, I presume. All the
ASSERT_xxx() macros return true/false depending whether the assertion
holds or not. ASSERT_ERR_PTR() checks that ptr *is* erroneous (which
is NULL and ERR_PTR). If it's not, it returns false. So
if (!ASSERT_ERR_PTR(ptr, "short_descriptor"))
/* do something if assertion failed */
is a common pattern.
Note also "short_descriptor", it's not supposed to be a long
descriptive sentences, it's sort of a "codename" of the particular
check. It's not illegal to use space-separated sentence, but better to
keep it short and identifier-like.
> I noticed that xxx__destroy has a check for NULL, so I put the destroy
> function unconditionally.
>
> > > +}
> > > +
> > > +void test_d_path(void)
> > > +{
> > > + if (test__start_subtest("basic"))
> > > + test_d_path_basic();
> > > +
> > > + if (test__start_subtest("check_rdonly_mem"))
> > > + test_d_path_check_rdonly_mem();
> > > +}
> > > diff --git a/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> > > new file mode 100644
> > > index 000000000000..c7a9655d5850
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> > > @@ -0,0 +1,30 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/* Copyright (c) 2021 Google */
> > > +
> > > +#include "vmlinux.h"
> > > +
> > > +#include "vmlinux.h"
> >
> > duplicated vmlinux.h.
> >
>
> Thanks. Will fix that.
>
> > > +#include <bpf/bpf_helpers.h>
> > > +#include <bpf/bpf_tracing.h>
> > > +
> > > +extern const int bpf_prog_active __ksym;
> > > +
> > > +SEC("fentry/security_inode_getattr")
> > > +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat,
> > > + __u32 request_mask, unsigned int query_flags)
> > > +{
> > > + char *active;
> >
> > int *active?
> > It may not matter since the program is rejected by the kernel but
> > with making it conforms to kernel definition we have one less thing
> > to worry about the verification.
> >
>
> Because bpf_d_path() accepts 'char *' instead of 'int *', I need to
> cast 'active' to 'char *' somewhere, otherwise the compiler will issue
> a warning. To combine with your comment, maybe the following:
>
> int *active;
> active = (int *)bpf_per_cpu_ptr(...);
> ...
> bpf_d_path(path, (char *)active, sizeof(int));
>
why not `void *`?
> > > + __u32 cpu;
> > > +
> > > + cpu = bpf_get_smp_processor_id();
> > > + active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu);
> >
> > int *
> >
> > > + if (active) {
> > > + /* FAIL here! 'active' is a rdonly_mem. bpf helpers that
> >
> > 'active' points to readonly memory.
> >
>
> Ack.
>
> > > + * update its arguments can not write into it.
> > > + */
> > > + bpf_d_path(path, active, sizeof(int));
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +char _license[] SEC("license") = "GPL";
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem.
2021-12-22 0:24 ` Andrii Nakryiko
@ 2021-12-22 1:05 ` Hao Luo
0 siblings, 0 replies; 6+ messages in thread
From: Hao Luo @ 2021-12-22 1:05 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Yonghong Song, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Song Liu, KP Singh, bpf
On Tue, Dec 21, 2021 at 4:24 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Dec 21, 2021 at 12:16 PM Hao Luo <haoluo@google.com> wrote:
> >
> > On Mon, Dec 20, 2021 at 8:28 PM Yonghong Song <yhs@fb.com> wrote:
> > >
> > >
> > >
> > > On 12/20/21 12:12 PM, Hao Luo wrote:
> > > > The second parameter of bpf_d_path() can only accept writable
> > > > memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not
> > > > be passed into bpf_d_path for modification. This patch adds
> > > > a selftest to verify this behavior.
> > > >
> > > > Signed-off-by: Hao Luo <haoluo@google.com>
> > > > ---
> > > > .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++-
> > > > .../bpf/progs/test_d_path_check_rdonly_mem.c | 30 +++++++++++++++++++
> > > > 2 files changed, 51 insertions(+), 1 deletion(-)
> > > > create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > > > index 0a577a248d34..f8d8c5a5dfba 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/d_path.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > > > @@ -9,6 +9,7 @@
> > > > #define MAX_FILES 7
> > > >
> > > > #include "test_d_path.skel.h"
> > > > +#include "test_d_path_check_rdonly_mem.skel.h"
> > > >
> > > > static int duration;
> > > >
> > > > @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid)
> > > > return ret;
> > > > }
> > > >
> > > > -void test_d_path(void)
> > > > +static void test_d_path_basic(void)
> > > > {
> > > > struct test_d_path__bss *bss;
> > > > struct test_d_path *skel;
> > > > @@ -155,3 +156,22 @@ void test_d_path(void)
> > > > cleanup:
> > > > test_d_path__destroy(skel);
> > > > }
> > > > +
> > > > +static void test_d_path_check_rdonly_mem(void)
> > > > +{
> > > > + struct test_d_path_check_rdonly_mem *skel;
> > > > +
> > > > + skel = test_d_path_check_rdonly_mem__open_and_load();
> > > > + ASSERT_ERR_PTR(skel, "unexpected load of a prog using d_path to write rdonly_mem\n");
> > > > +
> > > > + test_d_path_check_rdonly_mem__destroy(skel);
> > >
> > > You shouldn't call test_d_path_check_rdonly_mem__destroy(skel) if skel
> > > is an ERR_PTR. Maybe
> > > if (!ASSERT_ERR_PTR(...))
> > > test_d_path_check_rdonly_mem__destroy(skel);
> > >
> >
> > Ack. Will change that.
>
> no need, __destroy() handles NULLs and ERR_PTR just fine, the way you
> wrote it is totally correct (that's a deliberate nice feature of
> libbpf's "destructor" APIs)
>
Yep. That's also my understanding.
> >
> > I don't know if it's only me: I find it confusing when figuring out
> > what ASSERT_ERR_PTR(ptr) returns. Is the returned value 'ptr'? or 'ptr
> > != NULL'? or 'err != 0'? I used to think ASSERT-like function/macro
> > returns nothing.
> >
>
> You haven't looked at many other selftests, I presume. All the
> ASSERT_xxx() macros return true/false depending whether the assertion
> holds or not. ASSERT_ERR_PTR() checks that ptr *is* erroneous (which
> is NULL and ERR_PTR). If it's not, it returns false. So
>
> if (!ASSERT_ERR_PTR(ptr, "short_descriptor"))
> /* do something if assertion failed */
>
> is a common pattern.
>
> Note also "short_descriptor", it's not supposed to be a long
> descriptive sentences, it's sort of a "codename" of the particular
> check. It's not illegal to use space-separated sentence, but better to
> keep it short and identifier-like.
>
I see. Thanks for the explanation.
> > I noticed that xxx__destroy has a check for NULL, so I put the destroy
> > function unconditionally.
> >
> > > > +}
> > > > +
> > > > +void test_d_path(void)
> > > > +{
> > > > + if (test__start_subtest("basic"))
> > > > + test_d_path_basic();
> > > > +
> > > > + if (test__start_subtest("check_rdonly_mem"))
> > > > + test_d_path_check_rdonly_mem();
> > > > +}
> > > > diff --git a/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> > > > new file mode 100644
> > > > index 000000000000..c7a9655d5850
> > > > --- /dev/null
> > > > +++ b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> > > > @@ -0,0 +1,30 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/* Copyright (c) 2021 Google */
> > > > +
> > > > +#include "vmlinux.h"
> > > > +
> > > > +#include "vmlinux.h"
> > >
> > > duplicated vmlinux.h.
> > >
> >
> > Thanks. Will fix that.
> >
> > > > +#include <bpf/bpf_helpers.h>
> > > > +#include <bpf/bpf_tracing.h>
> > > > +
> > > > +extern const int bpf_prog_active __ksym;
> > > > +
> > > > +SEC("fentry/security_inode_getattr")
> > > > +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat,
> > > > + __u32 request_mask, unsigned int query_flags)
> > > > +{
> > > > + char *active;
> > >
> > > int *active?
> > > It may not matter since the program is rejected by the kernel but
> > > with making it conforms to kernel definition we have one less thing
> > > to worry about the verification.
> > >
> >
> > Because bpf_d_path() accepts 'char *' instead of 'int *', I need to
> > cast 'active' to 'char *' somewhere, otherwise the compiler will issue
> > a warning. To combine with your comment, maybe the following:
> >
> > int *active;
> > active = (int *)bpf_per_cpu_ptr(...);
> > ...
> > bpf_d_path(path, (char *)active, sizeof(int));
> >
>
> why not `void *`?
>
'void *' works. Just haven't thought about that.
> > > > + __u32 cpu;
> > > > +
> > > > + cpu = bpf_get_smp_processor_id();
> > > > + active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu);
> > >
> > > int *
> > >
> > > > + if (active) {
> > > > + /* FAIL here! 'active' is a rdonly_mem. bpf helpers that
> > >
> > > 'active' points to readonly memory.
> > >
> >
> > Ack.
> >
> > > > + * update its arguments can not write into it.
> > > > + */
> > > > + bpf_d_path(path, active, sizeof(int));
> > > > + }
> > > > + return 0;
> > > > +}
> > > > +
> > > > +char _license[] SEC("license") = "GPL";
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-22 1:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-20 20:12 [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem Hao Luo
2021-12-21 4:28 ` Yonghong Song
2021-12-21 20:16 ` Hao Luo
2021-12-21 22:29 ` Yonghong Song
2021-12-22 0:24 ` Andrii Nakryiko
2021-12-22 1:05 ` Hao Luo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.