* [PATCH 0/3] bpf_wq followup series
@ 2024-04-25 13:59 Benjamin Tissoires
2024-04-25 13:59 ` [PATCH 1/3] bpf: do not walk twice the map on free Benjamin Tissoires
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Benjamin Tissoires @ 2024-04-25 13:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan
Cc: bpf, linux-kernel, linux-kselftest, Benjamin Tissoires
Few patches that should have been there from day 1.
Anyway, they are coming now.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
Benjamin Tissoires (3):
bpf: do not walk twice the map on free
bpf: do not walk twice the hash map on free
selftests/bpf: drop an unused local variable
kernel/bpf/arraymap.c | 15 ++++++++-------
kernel/bpf/hashtab.c | 16 +++++-----------
tools/testing/selftests/bpf/prog_tests/wq.c | 2 --
3 files changed, 13 insertions(+), 20 deletions(-)
---
base-commit: 52578f7f53ff8fe3a8f6f3bc8b5956615c07a16e
change-id: 20240425-bpf-next-2114350587e3
Best regards,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/3] bpf: do not walk twice the map on free
2024-04-25 13:59 [PATCH 0/3] bpf_wq followup series Benjamin Tissoires
@ 2024-04-25 13:59 ` Benjamin Tissoires
2024-04-25 13:59 ` [PATCH 2/3] bpf: do not walk twice the hash " Benjamin Tissoires
2024-04-25 13:59 ` [PATCH 3/3] selftests/bpf: drop an unused local variable Benjamin Tissoires
2 siblings, 0 replies; 7+ messages in thread
From: Benjamin Tissoires @ 2024-04-25 13:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan
Cc: bpf, linux-kernel, linux-kselftest, Benjamin Tissoires
If someone stores both a timer and a workqueue in a map, on free we
would walk it twice.
Add a check in array_map_free_timers_wq and free the timers
and workqueues if they are present.
Fixes: 246331e3f1ea ("bpf: allow struct bpf_wq to be embedded in arraymaps and hashmaps")
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
kernel/bpf/arraymap.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 580d07b15471..feabc0193852 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -436,13 +436,14 @@ static void array_map_free_timers_wq(struct bpf_map *map)
/* We don't reset or free fields other than timer and workqueue
* on uref dropping to zero.
*/
- if (btf_record_has_field(map->record, BPF_TIMER))
- for (i = 0; i < array->map.max_entries; i++)
- bpf_obj_free_timer(map->record, array_map_elem_ptr(array, i));
-
- if (btf_record_has_field(map->record, BPF_WORKQUEUE))
- for (i = 0; i < array->map.max_entries; i++)
- bpf_obj_free_workqueue(map->record, array_map_elem_ptr(array, i));
+ if (btf_record_has_field(map->record, BPF_TIMER | BPF_WORKQUEUE)) {
+ for (i = 0; i < array->map.max_entries; i++) {
+ if (btf_record_has_field(map->record, BPF_TIMER))
+ bpf_obj_free_timer(map->record, array_map_elem_ptr(array, i));
+ if (btf_record_has_field(map->record, BPF_WORKQUEUE))
+ bpf_obj_free_workqueue(map->record, array_map_elem_ptr(array, i));
+ }
+ }
}
/* Called when map->refcnt goes to zero, either from workqueue or from syscall */
--
2.44.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] bpf: do not walk twice the hash map on free
2024-04-25 13:59 [PATCH 0/3] bpf_wq followup series Benjamin Tissoires
2024-04-25 13:59 ` [PATCH 1/3] bpf: do not walk twice the map on free Benjamin Tissoires
@ 2024-04-25 13:59 ` Benjamin Tissoires
2024-04-25 19:48 ` Alexei Starovoitov
2024-04-25 23:09 ` kernel test robot
2024-04-25 13:59 ` [PATCH 3/3] selftests/bpf: drop an unused local variable Benjamin Tissoires
2 siblings, 2 replies; 7+ messages in thread
From: Benjamin Tissoires @ 2024-04-25 13:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan
Cc: bpf, linux-kernel, linux-kselftest, Benjamin Tissoires
If someone stores both a timer and a workqueue in a hash map, on free, we
would walk it twice.
Add a check in htab_free_malloced_timers_or_wq and free the timers
and workqueues if they are present.
Fixes: 246331e3f1ea ("bpf: allow struct bpf_wq to be embedded in arraymaps and hashmaps")
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
kernel/bpf/hashtab.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index 0179183c543a..20162ae741e9 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -1515,7 +1515,7 @@ static void delete_all_elements(struct bpf_htab *htab)
migrate_enable();
}
-static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab, bool is_timer)
+static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab)
{
int i;
@@ -1527,10 +1527,10 @@ static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab, bool is_timer
hlist_nulls_for_each_entry(l, n, head, hash_node) {
/* We only free timer on uref dropping to zero */
- if (is_timer)
+ if (btf_record_has_field(htab->map.record, BPF_TIMER))
bpf_obj_free_timer(htab->map.record,
l->key + round_up(htab->map.key_size, 8));
- else
+ if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE))
bpf_obj_free_workqueue(htab->map.record,
l->key + round_up(htab->map.key_size, 8));
}
@@ -1544,18 +1544,12 @@ static void htab_map_free_timers_and_wq(struct bpf_map *map)
struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
/* We only free timer and workqueue on uref dropping to zero */
- if (btf_record_has_field(htab->map.record, BPF_TIMER)) {
+ if (btf_record_has_field(htab->map.record, BPF_TIMER | BPF_WORKQUEUE)) {
if (!htab_is_prealloc(htab))
- htab_free_malloced_timers_or_wq(htab, true);
+ htab_free_malloced_timers_or_wq(htab);
else
htab_free_prealloced_timers(htab);
}
- if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE)) {
- if (!htab_is_prealloc(htab))
- htab_free_malloced_timers_or_wq(htab, false);
- else
- htab_free_prealloced_wq(htab);
- }
}
/* Called when map->refcnt goes to zero, either from workqueue or from syscall */
--
2.44.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] selftests/bpf: drop an unused local variable
2024-04-25 13:59 [PATCH 0/3] bpf_wq followup series Benjamin Tissoires
2024-04-25 13:59 ` [PATCH 1/3] bpf: do not walk twice the map on free Benjamin Tissoires
2024-04-25 13:59 ` [PATCH 2/3] bpf: do not walk twice the hash " Benjamin Tissoires
@ 2024-04-25 13:59 ` Benjamin Tissoires
2 siblings, 0 replies; 7+ messages in thread
From: Benjamin Tissoires @ 2024-04-25 13:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan
Cc: bpf, linux-kernel, linux-kselftest, Benjamin Tissoires
Some copy/paste leftover, this is never used
Fixes: e3d9eac99afd ("selftests/bpf: wq: add bpf_wq_init() checks")
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
tools/testing/selftests/bpf/prog_tests/wq.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/wq.c b/tools/testing/selftests/bpf/prog_tests/wq.c
index c4bacd3160e1..99e438fe12ac 100644
--- a/tools/testing/selftests/bpf/prog_tests/wq.c
+++ b/tools/testing/selftests/bpf/prog_tests/wq.c
@@ -36,7 +36,5 @@ void serial_test_wq(void)
void serial_test_failures_wq(void)
{
- LIBBPF_OPTS(bpf_test_run_opts, topts);
-
RUN_TESTS(wq_failures);
}
--
2.44.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] bpf: do not walk twice the hash map on free
2024-04-25 13:59 ` [PATCH 2/3] bpf: do not walk twice the hash " Benjamin Tissoires
@ 2024-04-25 19:48 ` Alexei Starovoitov
2024-04-30 9:20 ` Benjamin Tissoires
2024-04-25 23:09 ` kernel test robot
1 sibling, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2024-04-25 19:48 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, bpf, LKML,
open list:KERNEL SELFTEST FRAMEWORK
On Thu, Apr 25, 2024 at 6:59 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> If someone stores both a timer and a workqueue in a hash map, on free, we
> would walk it twice.
> Add a check in htab_free_malloced_timers_or_wq and free the timers
> and workqueues if they are present.
>
> Fixes: 246331e3f1ea ("bpf: allow struct bpf_wq to be embedded in arraymaps and hashmaps")
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> kernel/bpf/hashtab.c | 16 +++++-----------
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 0179183c543a..20162ae741e9 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -1515,7 +1515,7 @@ static void delete_all_elements(struct bpf_htab *htab)
> migrate_enable();
> }
>
> -static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab, bool is_timer)
> +static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab)
> {
> int i;
>
> @@ -1527,10 +1527,10 @@ static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab, bool is_timer
>
> hlist_nulls_for_each_entry(l, n, head, hash_node) {
> /* We only free timer on uref dropping to zero */
> - if (is_timer)
> + if (btf_record_has_field(htab->map.record, BPF_TIMER))
> bpf_obj_free_timer(htab->map.record,
> l->key + round_up(htab->map.key_size, 8));
> - else
> + if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE))
> bpf_obj_free_workqueue(htab->map.record,
> l->key + round_up(htab->map.key_size, 8));
> }
> @@ -1544,18 +1544,12 @@ static void htab_map_free_timers_and_wq(struct bpf_map *map)
> struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
>
> /* We only free timer and workqueue on uref dropping to zero */
> - if (btf_record_has_field(htab->map.record, BPF_TIMER)) {
> + if (btf_record_has_field(htab->map.record, BPF_TIMER | BPF_WORKQUEUE)) {
> if (!htab_is_prealloc(htab))
> - htab_free_malloced_timers_or_wq(htab, true);
> + htab_free_malloced_timers_or_wq(htab);
> else
> htab_free_prealloced_timers(htab);
> }
> - if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE)) {
> - if (!htab_is_prealloc(htab))
> - htab_free_malloced_timers_or_wq(htab, false);
> - else
> - htab_free_prealloced_wq(htab);
This looks wrong.
htab_free_prealloced_wq() is now unused as compiler says:
../kernel/bpf/hashtab.c:243:13: warning: ‘htab_free_prealloced_wq’
defined but not used [-Wunused-function]
243 | static void htab_free_prealloced_wq(struct bpf_htab *htab)
| ^~~~~~~~~~~~~~~~~~~~~~~
and prealloced maps with wq leak wq-s.
pw-bot: cr
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] bpf: do not walk twice the hash map on free
2024-04-25 13:59 ` [PATCH 2/3] bpf: do not walk twice the hash " Benjamin Tissoires
2024-04-25 19:48 ` Alexei Starovoitov
@ 2024-04-25 23:09 ` kernel test robot
1 sibling, 0 replies; 7+ messages in thread
From: kernel test robot @ 2024-04-25 23:09 UTC (permalink / raw)
To: Benjamin Tissoires, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan
Cc: oe-kbuild-all, bpf, linux-kernel, linux-kselftest, Benjamin Tissoires
Hi Benjamin,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 52578f7f53ff8fe3a8f6f3bc8b5956615c07a16e]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Tissoires/bpf-do-not-walk-twice-the-map-on-free/20240425-220322
base: 52578f7f53ff8fe3a8f6f3bc8b5956615c07a16e
patch link: https://lore.kernel.org/r/20240425-bpf-next-v1-2-1d8330e6c643%40kernel.org
patch subject: [PATCH 2/3] bpf: do not walk twice the hash map on free
config: arc-randconfig-002-20240426 (https://download.01.org/0day-ci/archive/20240426/202404260653.ULrCGrp2-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240426/202404260653.ULrCGrp2-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404260653.ULrCGrp2-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> kernel/bpf/hashtab.c:243:13: warning: 'htab_free_prealloced_wq' defined but not used [-Wunused-function]
243 | static void htab_free_prealloced_wq(struct bpf_htab *htab)
| ^~~~~~~~~~~~~~~~~~~~~~~
vim +/htab_free_prealloced_wq +243 kernel/bpf/hashtab.c
68134668c17f31 Alexei Starovoitov 2021-07-14 242
246331e3f1eac9 Benjamin Tissoires 2024-04-20 @243 static void htab_free_prealloced_wq(struct bpf_htab *htab)
246331e3f1eac9 Benjamin Tissoires 2024-04-20 244 {
246331e3f1eac9 Benjamin Tissoires 2024-04-20 245 u32 num_entries = htab->map.max_entries;
246331e3f1eac9 Benjamin Tissoires 2024-04-20 246 int i;
246331e3f1eac9 Benjamin Tissoires 2024-04-20 247
246331e3f1eac9 Benjamin Tissoires 2024-04-20 248 if (!btf_record_has_field(htab->map.record, BPF_WORKQUEUE))
246331e3f1eac9 Benjamin Tissoires 2024-04-20 249 return;
246331e3f1eac9 Benjamin Tissoires 2024-04-20 250 if (htab_has_extra_elems(htab))
246331e3f1eac9 Benjamin Tissoires 2024-04-20 251 num_entries += num_possible_cpus();
246331e3f1eac9 Benjamin Tissoires 2024-04-20 252
246331e3f1eac9 Benjamin Tissoires 2024-04-20 253 for (i = 0; i < num_entries; i++) {
246331e3f1eac9 Benjamin Tissoires 2024-04-20 254 struct htab_elem *elem;
246331e3f1eac9 Benjamin Tissoires 2024-04-20 255
246331e3f1eac9 Benjamin Tissoires 2024-04-20 256 elem = get_htab_elem(htab, i);
246331e3f1eac9 Benjamin Tissoires 2024-04-20 257 bpf_obj_free_workqueue(htab->map.record,
246331e3f1eac9 Benjamin Tissoires 2024-04-20 258 elem->key + round_up(htab->map.key_size, 8));
246331e3f1eac9 Benjamin Tissoires 2024-04-20 259 cond_resched();
246331e3f1eac9 Benjamin Tissoires 2024-04-20 260 }
246331e3f1eac9 Benjamin Tissoires 2024-04-20 261 }
246331e3f1eac9 Benjamin Tissoires 2024-04-20 262
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] bpf: do not walk twice the hash map on free
2024-04-25 19:48 ` Alexei Starovoitov
@ 2024-04-30 9:20 ` Benjamin Tissoires
0 siblings, 0 replies; 7+ messages in thread
From: Benjamin Tissoires @ 2024-04-30 9:20 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, bpf, LKML,
open list:KERNEL SELFTEST FRAMEWORK
On Apr 25 2024, Alexei Starovoitov wrote:
> On Thu, Apr 25, 2024 at 6:59 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> >
> > If someone stores both a timer and a workqueue in a hash map, on free, we
> > would walk it twice.
> > Add a check in htab_free_malloced_timers_or_wq and free the timers
> > and workqueues if they are present.
> >
> > Fixes: 246331e3f1ea ("bpf: allow struct bpf_wq to be embedded in arraymaps and hashmaps")
> > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> > ---
> > kernel/bpf/hashtab.c | 16 +++++-----------
> > 1 file changed, 5 insertions(+), 11 deletions(-)
> >
> > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> > index 0179183c543a..20162ae741e9 100644
> > --- a/kernel/bpf/hashtab.c
> > +++ b/kernel/bpf/hashtab.c
> > @@ -1515,7 +1515,7 @@ static void delete_all_elements(struct bpf_htab *htab)
> > migrate_enable();
> > }
> >
> > -static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab, bool is_timer)
> > +static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab)
> > {
> > int i;
> >
> > @@ -1527,10 +1527,10 @@ static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab, bool is_timer
> >
> > hlist_nulls_for_each_entry(l, n, head, hash_node) {
> > /* We only free timer on uref dropping to zero */
> > - if (is_timer)
> > + if (btf_record_has_field(htab->map.record, BPF_TIMER))
> > bpf_obj_free_timer(htab->map.record,
> > l->key + round_up(htab->map.key_size, 8));
> > - else
> > + if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE))
> > bpf_obj_free_workqueue(htab->map.record,
> > l->key + round_up(htab->map.key_size, 8));
> > }
> > @@ -1544,18 +1544,12 @@ static void htab_map_free_timers_and_wq(struct bpf_map *map)
> > struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
> >
> > /* We only free timer and workqueue on uref dropping to zero */
> > - if (btf_record_has_field(htab->map.record, BPF_TIMER)) {
> > + if (btf_record_has_field(htab->map.record, BPF_TIMER | BPF_WORKQUEUE)) {
> > if (!htab_is_prealloc(htab))
> > - htab_free_malloced_timers_or_wq(htab, true);
> > + htab_free_malloced_timers_or_wq(htab);
> > else
> > htab_free_prealloced_timers(htab);
> > }
> > - if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE)) {
> > - if (!htab_is_prealloc(htab))
> > - htab_free_malloced_timers_or_wq(htab, false);
> > - else
> > - htab_free_prealloced_wq(htab);
>
> This looks wrong.
> htab_free_prealloced_wq() is now unused as compiler says:
> ../kernel/bpf/hashtab.c:243:13: warning: ‘htab_free_prealloced_wq’
> defined but not used [-Wunused-function]
> 243 | static void htab_free_prealloced_wq(struct bpf_htab *htab)
> | ^~~~~~~~~~~~~~~~~~~~~~~
>
> and prealloced maps with wq leak wq-s.
oops, you are right. Sending a v2 right away (sorry for the delay).
Cheers,
Benjamin
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-04-30 9:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-25 13:59 [PATCH 0/3] bpf_wq followup series Benjamin Tissoires
2024-04-25 13:59 ` [PATCH 1/3] bpf: do not walk twice the map on free Benjamin Tissoires
2024-04-25 13:59 ` [PATCH 2/3] bpf: do not walk twice the hash " Benjamin Tissoires
2024-04-25 19:48 ` Alexei Starovoitov
2024-04-30 9:20 ` Benjamin Tissoires
2024-04-25 23:09 ` kernel test robot
2024-04-25 13:59 ` [PATCH 3/3] selftests/bpf: drop an unused local variable Benjamin Tissoires
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).