* [PATCH net] bpf: don't zero out the info struct in bpf_obj_get_info_by_fd()
@ 2017-07-25 22:16 Jakub Kicinski
2017-07-25 22:59 ` Daniel Borkmann
2017-07-27 0:03 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Kicinski @ 2017-07-25 22:16 UTC (permalink / raw)
To: netdev, daniel; +Cc: oss-drivers, alexei.starovoitov, kafai, Jakub Kicinski
The buffer passed to bpf_obj_get_info_by_fd() should be initialized
to zeros. Kernel will enforce that to guarantee we can safely extend
info structures in the future.
Making the bpf_obj_get_info_by_fd() call in libbpf perform the zeroing
is problematic, however, since some members of the info structures
may need to be initialized by the callers (for instance pointers
to buffers to which kernel is to dump translated and jited images).
Remove the zeroing and fix up the in-tree callers before any kernel
has been released with this code.
As Daniel points out this seems to be the intended operation anyway,
since commit 95b9afd3987f ("bpf: Test for bpf ID") is itself setting
the buffer pointers before calling bpf_obj_get_info_by_fd().
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
I have a small patch to add checking if kernel actually populated
the instruction dumps which I will post after this ends up in net-next.
tools/lib/bpf/bpf.c | 1 -
tools/testing/selftests/bpf/test_progs.c | 8 ++++++--
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 412a7c82995a..256f571f2ab5 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -314,7 +314,6 @@ int bpf_obj_get_info_by_fd(int prog_fd, void *info, __u32 *info_len)
int err;
bzero(&attr, sizeof(attr));
- bzero(info, *info_len);
attr.info.bpf_fd = prog_fd;
attr.info.info_len = *info_len;
attr.info.info = ptr_to_u64(info);
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 5855cd3d3d45..1f7dd35551b9 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -340,6 +340,7 @@ static void test_bpf_obj_id(void)
/* Check getting prog info */
info_len = sizeof(struct bpf_prog_info) * 2;
+ bzero(&prog_infos[i], info_len);
prog_infos[i].jited_prog_insns = ptr_to_u64(jited_insns);
prog_infos[i].jited_prog_len = sizeof(jited_insns);
prog_infos[i].xlated_prog_insns = ptr_to_u64(xlated_insns);
@@ -369,6 +370,7 @@ static void test_bpf_obj_id(void)
/* Check getting map info */
info_len = sizeof(struct bpf_map_info) * 2;
+ bzero(&map_infos[i], info_len);
err = bpf_obj_get_info_by_fd(map_fds[i], &map_infos[i],
&info_len);
if (CHECK(err ||
@@ -394,7 +396,7 @@ static void test_bpf_obj_id(void)
nr_id_found = 0;
next_id = 0;
while (!bpf_prog_get_next_id(next_id, &next_id)) {
- struct bpf_prog_info prog_info;
+ struct bpf_prog_info prog_info = {};
int prog_fd;
info_len = sizeof(prog_info);
@@ -418,6 +420,8 @@ static void test_bpf_obj_id(void)
nr_id_found++;
err = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &info_len);
+ prog_infos[i].jited_prog_insns = 0;
+ prog_infos[i].xlated_prog_insns = 0;
CHECK(err || info_len != sizeof(struct bpf_prog_info) ||
memcmp(&prog_info, &prog_infos[i], info_len),
"get-prog-info(next_id->fd)",
@@ -436,7 +440,7 @@ static void test_bpf_obj_id(void)
nr_id_found = 0;
next_id = 0;
while (!bpf_map_get_next_id(next_id, &next_id)) {
- struct bpf_map_info map_info;
+ struct bpf_map_info map_info = {};
int map_fd;
info_len = sizeof(map_info);
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] bpf: don't zero out the info struct in bpf_obj_get_info_by_fd()
2017-07-25 22:16 [PATCH net] bpf: don't zero out the info struct in bpf_obj_get_info_by_fd() Jakub Kicinski
@ 2017-07-25 22:59 ` Daniel Borkmann
2017-07-25 23:15 ` Jakub Kicinski
2017-07-27 0:03 ` David Miller
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2017-07-25 22:59 UTC (permalink / raw)
To: Jakub Kicinski, netdev; +Cc: oss-drivers, alexei.starovoitov, kafai
On 07/26/2017 12:16 AM, Jakub Kicinski wrote:
> The buffer passed to bpf_obj_get_info_by_fd() should be initialized
> to zeros. Kernel will enforce that to guarantee we can safely extend
> info structures in the future.
>
> Making the bpf_obj_get_info_by_fd() call in libbpf perform the zeroing
> is problematic, however, since some members of the info structures
> may need to be initialized by the callers (for instance pointers
> to buffers to which kernel is to dump translated and jited images).
>
> Remove the zeroing and fix up the in-tree callers before any kernel
> has been released with this code.
>
> As Daniel points out this seems to be the intended operation anyway,
> since commit 95b9afd3987f ("bpf: Test for bpf ID") is itself setting
> the buffer pointers before calling bpf_obj_get_info_by_fd().
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> I have a small patch to add checking if kernel actually populated
> the instruction dumps which I will post after this ends up in net-next.
>
> tools/lib/bpf/bpf.c | 1 -
> tools/testing/selftests/bpf/test_progs.c | 8 ++++++--
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 412a7c82995a..256f571f2ab5 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -314,7 +314,6 @@ int bpf_obj_get_info_by_fd(int prog_fd, void *info, __u32 *info_len)
> int err;
>
> bzero(&attr, sizeof(attr));
> - bzero(info, *info_len);
> attr.info.bpf_fd = prog_fd;
> attr.info.info_len = *info_len;
> attr.info.info = ptr_to_u64(info);
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 5855cd3d3d45..1f7dd35551b9 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -340,6 +340,7 @@ static void test_bpf_obj_id(void)
>
> /* Check getting prog info */
> info_len = sizeof(struct bpf_prog_info) * 2;
> + bzero(&prog_infos[i], info_len);
> prog_infos[i].jited_prog_insns = ptr_to_u64(jited_insns);
> prog_infos[i].jited_prog_len = sizeof(jited_insns);
> prog_infos[i].xlated_prog_insns = ptr_to_u64(xlated_insns);
> @@ -369,6 +370,7 @@ static void test_bpf_obj_id(void)
>
> /* Check getting map info */
> info_len = sizeof(struct bpf_map_info) * 2;
> + bzero(&map_infos[i], info_len);
> err = bpf_obj_get_info_by_fd(map_fds[i], &map_infos[i],
> &info_len);
> if (CHECK(err ||
> @@ -394,7 +396,7 @@ static void test_bpf_obj_id(void)
> nr_id_found = 0;
> next_id = 0;
> while (!bpf_prog_get_next_id(next_id, &next_id)) {
> - struct bpf_prog_info prog_info;
> + struct bpf_prog_info prog_info = {};
> int prog_fd;
>
> info_len = sizeof(prog_info);
> @@ -418,6 +420,8 @@ static void test_bpf_obj_id(void)
> nr_id_found++;
>
> err = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &info_len);
> + prog_infos[i].jited_prog_insns = 0;
> + prog_infos[i].xlated_prog_insns = 0;
Can you elaborate why this one above is needed?
> CHECK(err || info_len != sizeof(struct bpf_prog_info) ||
> memcmp(&prog_info, &prog_infos[i], info_len),
> "get-prog-info(next_id->fd)",
> @@ -436,7 +440,7 @@ static void test_bpf_obj_id(void)
> nr_id_found = 0;
> next_id = 0;
> while (!bpf_map_get_next_id(next_id, &next_id)) {
> - struct bpf_map_info map_info;
> + struct bpf_map_info map_info = {};
> int map_fd;
>
> info_len = sizeof(map_info);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] bpf: don't zero out the info struct in bpf_obj_get_info_by_fd()
2017-07-25 22:59 ` Daniel Borkmann
@ 2017-07-25 23:15 ` Jakub Kicinski
2017-07-25 23:20 ` Jakub Kicinski
2017-07-25 23:29 ` Daniel Borkmann
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Kicinski @ 2017-07-25 23:15 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev, oss-drivers, alexei.starovoitov, kafai
On Wed, 26 Jul 2017 00:59:49 +0200, Daniel Borkmann wrote:
> > @@ -418,6 +420,8 @@ static void test_bpf_obj_id(void)
> > nr_id_found++;
> >
> > err = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &info_len);
> > + prog_infos[i].jited_prog_insns = 0;
> > + prog_infos[i].xlated_prog_insns = 0;
>
> Can you elaborate why this one above is needed?
Ah, I removed the comment about it at the last minute. The check below
compares the info we get here with info we got reading the programs in
the earlier loop - using memcmp(). This call, however, doesn't fill in
the pointers for jited and xlated images, so the memcmp() would fail.
It used to work when bpf_obj_get_info_by_fd() was zeroing info, since
the pointers would be cleared by it, and no dump ever returned, it
didn't matter that the call sites differ.
> > CHECK(err || info_len != sizeof(struct bpf_prog_info) ||
> > memcmp(&prog_info, &prog_infos[i], info_len),
> > "get-prog-info(next_id->fd)",
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] bpf: don't zero out the info struct in bpf_obj_get_info_by_fd()
2017-07-25 23:15 ` Jakub Kicinski
@ 2017-07-25 23:20 ` Jakub Kicinski
2017-07-25 23:29 ` Daniel Borkmann
1 sibling, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2017-07-25 23:20 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev, oss-drivers, alexei.starovoitov, kafai
On Tue, 25 Jul 2017 16:15:47 -0700, Jakub Kicinski wrote:
> On Wed, 26 Jul 2017 00:59:49 +0200, Daniel Borkmann wrote:
> > > @@ -418,6 +420,8 @@ static void test_bpf_obj_id(void)
> > > nr_id_found++;
> > >
> > > err = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &info_len);
> > > + prog_infos[i].jited_prog_insns = 0;
> > > + prog_infos[i].xlated_prog_insns = 0;
> >
> > Can you elaborate why this one above is needed?
>
> Ah, I removed the comment about it at the last minute. The check below
> compares the info we get here with info we got reading the programs in
> the earlier loop - using memcmp(). This call, however, doesn't fill in
> the pointers for jited and xlated images, so the memcmp() would fail.
>
> It used to work when bpf_obj_get_info_by_fd() was zeroing info, since
> the pointers would be cleared by it, and no dump ever returned, it
> didn't matter that the call sites differ.
FWIW the comment was this:
+ /* Clear the insns pointers, we're not requesting dumps here.
+ * Otherwise the byte-by-byte comparison below would fail.
+ */
> > > CHECK(err || info_len != sizeof(struct bpf_prog_info) ||
> > > memcmp(&prog_info, &prog_infos[i], info_len),
> > > "get-prog-info(next_id->fd)",
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] bpf: don't zero out the info struct in bpf_obj_get_info_by_fd()
2017-07-25 23:15 ` Jakub Kicinski
2017-07-25 23:20 ` Jakub Kicinski
@ 2017-07-25 23:29 ` Daniel Borkmann
1 sibling, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2017-07-25 23:29 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, oss-drivers, alexei.starovoitov, kafai
On 07/26/2017 01:15 AM, Jakub Kicinski wrote:
> On Wed, 26 Jul 2017 00:59:49 +0200, Daniel Borkmann wrote:
>>> @@ -418,6 +420,8 @@ static void test_bpf_obj_id(void)
>>> nr_id_found++;
>>>
>>> err = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &info_len);
>>> + prog_infos[i].jited_prog_insns = 0;
>>> + prog_infos[i].xlated_prog_insns = 0;
>>
>> Can you elaborate why this one above is needed?
>
> Ah, I removed the comment about it at the last minute. The check below
> compares the info we get here with info we got reading the programs in
> the earlier loop - using memcmp().
Yep, makes sense. I mistook it for 'length' given it is not NULL but 0,
but that is due to __aligned_u64. ;) Anyway, thanks for clarifying.
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] bpf: don't zero out the info struct in bpf_obj_get_info_by_fd()
2017-07-25 22:16 [PATCH net] bpf: don't zero out the info struct in bpf_obj_get_info_by_fd() Jakub Kicinski
2017-07-25 22:59 ` Daniel Borkmann
@ 2017-07-27 0:03 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2017-07-27 0:03 UTC (permalink / raw)
To: jakub.kicinski; +Cc: netdev, daniel, oss-drivers, alexei.starovoitov, kafai
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Tue, 25 Jul 2017 15:16:12 -0700
> The buffer passed to bpf_obj_get_info_by_fd() should be initialized
> to zeros. Kernel will enforce that to guarantee we can safely extend
> info structures in the future.
>
> Making the bpf_obj_get_info_by_fd() call in libbpf perform the zeroing
> is problematic, however, since some members of the info structures
> may need to be initialized by the callers (for instance pointers
> to buffers to which kernel is to dump translated and jited images).
>
> Remove the zeroing and fix up the in-tree callers before any kernel
> has been released with this code.
>
> As Daniel points out this seems to be the intended operation anyway,
> since commit 95b9afd3987f ("bpf: Test for bpf ID") is itself setting
> the buffer pointers before calling bpf_obj_get_info_by_fd().
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Applied, thanks Jakub.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-27 0:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-25 22:16 [PATCH net] bpf: don't zero out the info struct in bpf_obj_get_info_by_fd() Jakub Kicinski
2017-07-25 22:59 ` Daniel Borkmann
2017-07-25 23:15 ` Jakub Kicinski
2017-07-25 23:20 ` Jakub Kicinski
2017-07-25 23:29 ` Daniel Borkmann
2017-07-27 0:03 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).