* [PATCH bpf] libbpf: fix race when pinning maps in parallel
@ 2021-07-05 19:09 Martynas Pumputis
2021-07-07 22:38 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: Martynas Pumputis @ 2021-07-05 19:09 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, andrii, m, Joe Stringer
When loading in parallel multiple programs which use the same to-be
pinned map, it is possible that two instances of the loader will call
bpf_object__create_maps() at the same time. If the map doesn't exist
when both instances call bpf_object__reuse_map(), then one of the
instances will fail with EEXIST when calling bpf_map__pin().
Fix the race by retrying creating a map if bpf_map__pin() returns
EEXIST. The fix is similar to the one in iproute2: e4c4685fd6e4 ("bpf:
Fix race condition with map pinning").
Cc: Joe Stringer <joe@wand.net.nz>
Signed-off-by: Martynas Pumputis <m@lambda.lt>
---
tools/lib/bpf/libbpf.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 1e04ce724240..7a31c7c3cd21 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4616,10 +4616,12 @@ bpf_object__create_maps(struct bpf_object *obj)
char *cp, errmsg[STRERR_BUFSIZE];
unsigned int i, j;
int err;
+ bool retried = false;
for (i = 0; i < obj->nr_maps; i++) {
map = &obj->maps[i];
+retry:
if (map->pin_path) {
err = bpf_object__reuse_map(map);
if (err) {
@@ -4660,9 +4662,13 @@ bpf_object__create_maps(struct bpf_object *obj)
if (map->pin_path && !map->pinned) {
err = bpf_map__pin(map, NULL);
if (err) {
+ zclose(map->fd);
+ if (!retried && err == EEXIST) {
+ retried = true;
+ goto retry;
+ }
pr_warn("map '%s': failed to auto-pin at '%s': %d\n",
map->name, map->pin_path, err);
- zclose(map->fd);
goto err_out;
}
}
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf] libbpf: fix race when pinning maps in parallel 2021-07-05 19:09 [PATCH bpf] libbpf: fix race when pinning maps in parallel Martynas Pumputis @ 2021-07-07 22:38 ` Andrii Nakryiko 2021-07-08 15:52 ` Martynas Pumputis 0 siblings, 1 reply; 6+ messages in thread From: Andrii Nakryiko @ 2021-07-07 22:38 UTC (permalink / raw) To: Martynas Pumputis Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Joe Stringer On Mon, Jul 5, 2021 at 12:08 PM Martynas Pumputis <m@lambda.lt> wrote: > > When loading in parallel multiple programs which use the same to-be > pinned map, it is possible that two instances of the loader will call > bpf_object__create_maps() at the same time. If the map doesn't exist > when both instances call bpf_object__reuse_map(), then one of the > instances will fail with EEXIST when calling bpf_map__pin(). > > Fix the race by retrying creating a map if bpf_map__pin() returns > EEXIST. The fix is similar to the one in iproute2: e4c4685fd6e4 ("bpf: > Fix race condition with map pinning"). > > Cc: Joe Stringer <joe@wand.net.nz> > Signed-off-by: Martynas Pumputis <m@lambda.lt> > --- > tools/lib/bpf/libbpf.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 1e04ce724240..7a31c7c3cd21 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -4616,10 +4616,12 @@ bpf_object__create_maps(struct bpf_object *obj) > char *cp, errmsg[STRERR_BUFSIZE]; > unsigned int i, j; > int err; > + bool retried = false; retried has to be reset for each map, so just move it inside the for loop? you can also generalize it to retry_cnt (> 1 attempts) to allow for more extreme cases of multiple loaders fighting very heavily > > for (i = 0; i < obj->nr_maps; i++) { > map = &obj->maps[i]; > > +retry: > if (map->pin_path) { > err = bpf_object__reuse_map(map); > if (err) { > @@ -4660,9 +4662,13 @@ bpf_object__create_maps(struct bpf_object *obj) > if (map->pin_path && !map->pinned) { > err = bpf_map__pin(map, NULL); > if (err) { > + zclose(map->fd); > + if (!retried && err == EEXIST) { so I'm also wondering... should we commit at this point to trying to pin and not attempt to re-create the map? I'm worried that bpf_object__create_map() is not designed and tested to be called multiple times for the same bpf_map, but it's technically possible for it to be called multiple times in this scenario. Check the inner map creation scenario, for example (btw, I think there is a bug in bpf_object__create_map clean up for inner map, care to take a look at that as well?). So unless we want to allow map re-creation if (in a highly unlikely scenario) someone already unpinned the other instance, I'd say we should just bpf_map__pin() here directly, maybe in a short loop to allow for a few attempts. > + retried = true; > + goto retry; > + } > pr_warn("map '%s': failed to auto-pin at '%s': %d\n", > map->name, map->pin_path, err); > - zclose(map->fd); > goto err_out; > } > } > -- > 2.32.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] libbpf: fix race when pinning maps in parallel 2021-07-07 22:38 ` Andrii Nakryiko @ 2021-07-08 15:52 ` Martynas Pumputis 2021-07-08 20:33 ` Andrii Nakryiko 0 siblings, 1 reply; 6+ messages in thread From: Martynas Pumputis @ 2021-07-08 15:52 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Joe Stringer On 7/8/21 12:38 AM, Andrii Nakryiko wrote: > On Mon, Jul 5, 2021 at 12:08 PM Martynas Pumputis <m@lambda.lt> wrote: >> >> When loading in parallel multiple programs which use the same to-be >> pinned map, it is possible that two instances of the loader will call >> bpf_object__create_maps() at the same time. If the map doesn't exist >> when both instances call bpf_object__reuse_map(), then one of the >> instances will fail with EEXIST when calling bpf_map__pin(). >> >> Fix the race by retrying creating a map if bpf_map__pin() returns >> EEXIST. The fix is similar to the one in iproute2: e4c4685fd6e4 ("bpf: >> Fix race condition with map pinning"). >> >> Cc: Joe Stringer <joe@wand.net.nz> >> Signed-off-by: Martynas Pumputis <m@lambda.lt> >> --- >> tools/lib/bpf/libbpf.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index 1e04ce724240..7a31c7c3cd21 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -4616,10 +4616,12 @@ bpf_object__create_maps(struct bpf_object *obj) >> char *cp, errmsg[STRERR_BUFSIZE]; >> unsigned int i, j; >> int err; >> + bool retried = false; > > retried has to be reset for each map, so just move it inside the for > loop? you can also generalize it to retry_cnt (> 1 attempts) to allow > for more extreme cases of multiple loaders fighting very heavily If we move "retried = false" to inside the loop, then there is no need for retry_cnt. Single retry for each map should be enough to resolve the race. In any case, I'm going to move "retried = false", as you suggested. > >> >> for (i = 0; i < obj->nr_maps; i++) { >> map = &obj->maps[i]; >> >> +retry: >> if (map->pin_path) { >> err = bpf_object__reuse_map(map); >> if (err) { >> @@ -4660,9 +4662,13 @@ bpf_object__create_maps(struct bpf_object *obj) >> if (map->pin_path && !map->pinned) { >> err = bpf_map__pin(map, NULL); >> if (err) { >> + zclose(map->fd); >> + if (!retried && err == EEXIST) { > > so I'm also wondering... should we commit at this point to trying to > pin and not attempt to re-create the map? I'm worried that > bpf_object__create_map() is not designed and tested to be called > multiple times for the same bpf_map, but it's technically possible for > it to be called multiple times in this scenario. Check the inner map Good call. I'm going to add "if (retried && map->fd < 0) { return -ENOENT; }" after the "if (map->pinned) { err = bpf_object__reuse_map() ... }" statement. This should prevent from invoking bpf_object__create_map() multiple times. > creation scenario, for example (btw, I think there is a bug in > bpf_object__create_map clean up for inner map, care to take a look at > that as well?). In the case of the inner map, it should be destroyed inside bpf_object__create_map() after a successful BPF_MAP_CREATE. So AFAIU, there should be no need for the cleanup. Or do I miss something? > > So unless we want to allow map re-creation if (in a highly unlikely > scenario) someone already unpinned the other instance, I'd say we > should just bpf_map__pin() here directly, maybe in a short loop to > allow for a few attempts. > >> + retried = true; >> + goto retry; >> + } >> pr_warn("map '%s': failed to auto-pin at '%s': %d\n", >> map->name, map->pin_path, err); >> - zclose(map->fd); >> goto err_out; >> } >> } >> -- >> 2.32.0 >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] libbpf: fix race when pinning maps in parallel 2021-07-08 15:52 ` Martynas Pumputis @ 2021-07-08 20:33 ` Andrii Nakryiko 2021-07-15 10:17 ` Martynas Pumputis 2021-07-22 13:56 ` Martynas Pumputis 0 siblings, 2 replies; 6+ messages in thread From: Andrii Nakryiko @ 2021-07-08 20:33 UTC (permalink / raw) To: Martynas Pumputis Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Joe Stringer On Thu, Jul 8, 2021 at 8:50 AM Martynas Pumputis <m@lambda.lt> wrote: > > > > On 7/8/21 12:38 AM, Andrii Nakryiko wrote: > > On Mon, Jul 5, 2021 at 12:08 PM Martynas Pumputis <m@lambda.lt> wrote: > >> > >> When loading in parallel multiple programs which use the same to-be > >> pinned map, it is possible that two instances of the loader will call > >> bpf_object__create_maps() at the same time. If the map doesn't exist > >> when both instances call bpf_object__reuse_map(), then one of the > >> instances will fail with EEXIST when calling bpf_map__pin(). > >> > >> Fix the race by retrying creating a map if bpf_map__pin() returns > >> EEXIST. The fix is similar to the one in iproute2: e4c4685fd6e4 ("bpf: > >> Fix race condition with map pinning"). > >> > >> Cc: Joe Stringer <joe@wand.net.nz> > >> Signed-off-by: Martynas Pumputis <m@lambda.lt> > >> --- > >> tools/lib/bpf/libbpf.c | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> index 1e04ce724240..7a31c7c3cd21 100644 > >> --- a/tools/lib/bpf/libbpf.c > >> +++ b/tools/lib/bpf/libbpf.c > >> @@ -4616,10 +4616,12 @@ bpf_object__create_maps(struct bpf_object *obj) > >> char *cp, errmsg[STRERR_BUFSIZE]; > >> unsigned int i, j; > >> int err; > >> + bool retried = false; > > > > retried has to be reset for each map, so just move it inside the for > > loop? you can also generalize it to retry_cnt (> 1 attempts) to allow > > for more extreme cases of multiple loaders fighting very heavily > > If we move "retried = false" to inside the loop, then there is no need > for retry_cnt. Single retry for each map should be enough to resolve the > race. In any case, I'm going to move "retried = false", as you suggested. Right, I was originally thinking about the case where already pinned map might get unpinned. But then subsequently rejected the idea of re-creating the map :) So single retry should do. > > > > >> > >> for (i = 0; i < obj->nr_maps; i++) { > >> map = &obj->maps[i]; > >> > >> +retry: > >> if (map->pin_path) { > >> err = bpf_object__reuse_map(map); > >> if (err) { > >> @@ -4660,9 +4662,13 @@ bpf_object__create_maps(struct bpf_object *obj) > >> if (map->pin_path && !map->pinned) { > >> err = bpf_map__pin(map, NULL); > >> if (err) { > >> + zclose(map->fd); > >> + if (!retried && err == EEXIST) { > > > > so I'm also wondering... should we commit at this point to trying to > > pin and not attempt to re-create the map? I'm worried that > > bpf_object__create_map() is not designed and tested to be called > > multiple times for the same bpf_map, but it's technically possible for > > it to be called multiple times in this scenario. Check the inner map > > Good call. I'm going to add "if (retried && map->fd < 0) { return > -ENOENT; }" after the "if (map->pinned) { err = bpf_object__reuse_map() > ... }" statement. This should prevent from invoking > bpf_object__create_map() multiple times. > > > creation scenario, for example (btw, I think there is a bug in > > bpf_object__create_map clean up for inner map, care to take a look at > > that as well?). > > In the case of the inner map, it should be destroyed inside > bpf_object__create_map() after a successful BPF_MAP_CREATE. So AFAIU, > there should be no need for the cleanup. Or do I miss something? But if outer map creation fails, we won't do bpf_map__destroy(map->inner_map);, which is one bug. And then with your retry logic we also don't clean up the internal state of the bpf_map, which is another one. It would be good to add a self-test simulating such situations (e.g., by specifying wrong key_size for outer_map, but correct inner_map definition). Not sure how to reliably simulate this pinning race, though. Can you please add at least the first test case? > > > > > So unless we want to allow map re-creation if (in a highly unlikely > > scenario) someone already unpinned the other instance, I'd say we > > should just bpf_map__pin() here directly, maybe in a short loop to > > allow for a few attempts. > > > >> + retried = true; > >> + goto retry; > >> + } > >> pr_warn("map '%s': failed to auto-pin at '%s': %d\n", > >> map->name, map->pin_path, err); > >> - zclose(map->fd); > >> goto err_out; > >> } > >> } > >> -- > >> 2.32.0 > >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] libbpf: fix race when pinning maps in parallel 2021-07-08 20:33 ` Andrii Nakryiko @ 2021-07-15 10:17 ` Martynas Pumputis 2021-07-22 13:56 ` Martynas Pumputis 1 sibling, 0 replies; 6+ messages in thread From: Martynas Pumputis @ 2021-07-15 10:17 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Joe Stringer On 7/8/21 10:33 PM, Andrii Nakryiko wrote: > On Thu, Jul 8, 2021 at 8:50 AM Martynas Pumputis <m@lambda.lt> wrote: >> >> >> >> On 7/8/21 12:38 AM, Andrii Nakryiko wrote: >>> On Mon, Jul 5, 2021 at 12:08 PM Martynas Pumputis <m@lambda.lt> wrote: >>>> >>>> When loading in parallel multiple programs which use the same to-be >>>> pinned map, it is possible that two instances of the loader will call >>>> bpf_object__create_maps() at the same time. If the map doesn't exist >>>> when both instances call bpf_object__reuse_map(), then one of the >>>> instances will fail with EEXIST when calling bpf_map__pin(). >>>> >>>> Fix the race by retrying creating a map if bpf_map__pin() returns >>>> EEXIST. The fix is similar to the one in iproute2: e4c4685fd6e4 ("bpf: >>>> Fix race condition with map pinning"). >>>> >>>> Cc: Joe Stringer <joe@wand.net.nz> >>>> Signed-off-by: Martynas Pumputis <m@lambda.lt> >>>> --- >>>> tools/lib/bpf/libbpf.c | 8 +++++++- >>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >>>> index 1e04ce724240..7a31c7c3cd21 100644 >>>> --- a/tools/lib/bpf/libbpf.c >>>> +++ b/tools/lib/bpf/libbpf.c >>>> @@ -4616,10 +4616,12 @@ bpf_object__create_maps(struct bpf_object *obj) >>>> char *cp, errmsg[STRERR_BUFSIZE]; >>>> unsigned int i, j; >>>> int err; >>>> + bool retried = false; >>> >>> retried has to be reset for each map, so just move it inside the for >>> loop? you can also generalize it to retry_cnt (> 1 attempts) to allow >>> for more extreme cases of multiple loaders fighting very heavily >> >> If we move "retried = false" to inside the loop, then there is no need >> for retry_cnt. Single retry for each map should be enough to resolve the >> race. In any case, I'm going to move "retried = false", as you suggested. > > Right, I was originally thinking about the case where already pinned > map might get unpinned. But then subsequently rejected the idea of > re-creating the map :) So single retry should do. > >> >>> >>>> >>>> for (i = 0; i < obj->nr_maps; i++) { >>>> map = &obj->maps[i]; >>>> >>>> +retry: >>>> if (map->pin_path) { >>>> err = bpf_object__reuse_map(map); >>>> if (err) { >>>> @@ -4660,9 +4662,13 @@ bpf_object__create_maps(struct bpf_object *obj) >>>> if (map->pin_path && !map->pinned) { >>>> err = bpf_map__pin(map, NULL); >>>> if (err) { >>>> + zclose(map->fd); >>>> + if (!retried && err == EEXIST) { >>> >>> so I'm also wondering... should we commit at this point to trying to >>> pin and not attempt to re-create the map? I'm worried that >>> bpf_object__create_map() is not designed and tested to be called >>> multiple times for the same bpf_map, but it's technically possible for >>> it to be called multiple times in this scenario. Check the inner map >> >> Good call. I'm going to add "if (retried && map->fd < 0) { return >> -ENOENT; }" after the "if (map->pinned) { err = bpf_object__reuse_map() >> ... }" statement. This should prevent from invoking >> bpf_object__create_map() multiple times. >> >>> creation scenario, for example (btw, I think there is a bug in >>> bpf_object__create_map clean up for inner map, care to take a look at >>> that as well?). >> >> In the case of the inner map, it should be destroyed inside >> bpf_object__create_map() after a successful BPF_MAP_CREATE. So AFAIU, >> there should be no need for the cleanup. Or do I miss something? > > But if outer map creation fails, we won't do > bpf_map__destroy(map->inner_map);, which is one bug. And then with > your retry logic we also don't clean up the internal state of the > bpf_map, which is another one. It would be good to add a self-test > simulating such situations (e.g., by specifying wrong key_size for > outer_map, but correct inner_map definition). Not sure how to reliably > simulate this pinning race, though. > > Can you please add at least the first test case? Yep, I've sent the patch with a test case for the first bug. Thanks for explaining. Anyway, regarding the proposed retry, I think the safest approach is to bail before invoking bpf_object__create_map() for the second time (when we retry). This would avoid any issues with idempotence of bpf_object__create_map() and should solve most of the cases (except when a map gets unpinned before the retry, but I expect this to be a very unusual and rare situation). > >> >>> >>> So unless we want to allow map re-creation if (in a highly unlikely >>> scenario) someone already unpinned the other instance, I'd say we >>> should just bpf_map__pin() here directly, maybe in a short loop to >>> allow for a few attempts. >>> >>>> + retried = true; >>>> + goto retry; >>>> + } >>>> pr_warn("map '%s': failed to auto-pin at '%s': %d\n", >>>> map->name, map->pin_path, err); >>>> - zclose(map->fd); >>>> goto err_out; >>>> } >>>> } >>>> -- >>>> 2.32.0 >>>> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] libbpf: fix race when pinning maps in parallel 2021-07-08 20:33 ` Andrii Nakryiko 2021-07-15 10:17 ` Martynas Pumputis @ 2021-07-22 13:56 ` Martynas Pumputis 1 sibling, 0 replies; 6+ messages in thread From: Martynas Pumputis @ 2021-07-22 13:56 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Joe Stringer On 7/8/21 10:33 PM, Andrii Nakryiko wrote: > On Thu, Jul 8, 2021 at 8:50 AM Martynas Pumputis <m@lambda.lt> wrote: >> >> >> >> On 7/8/21 12:38 AM, Andrii Nakryiko wrote: >>> On Mon, Jul 5, 2021 at 12:08 PM Martynas Pumputis <m@lambda.lt> wrote: >>>> >>>> When loading in parallel multiple programs which use the same to-be >>>> pinned map, it is possible that two instances of the loader will call >>>> bpf_object__create_maps() at the same time. If the map doesn't exist >>>> when both instances call bpf_object__reuse_map(), then one of the >>>> instances will fail with EEXIST when calling bpf_map__pin(). >>>> >>>> Fix the race by retrying creating a map if bpf_map__pin() returns >>>> EEXIST. The fix is similar to the one in iproute2: e4c4685fd6e4 ("bpf: >>>> Fix race condition with map pinning"). >>>> >>>> Cc: Joe Stringer <joe@wand.net.nz> >>>> Signed-off-by: Martynas Pumputis <m@lambda.lt> >>>> --- >>>> tools/lib/bpf/libbpf.c | 8 +++++++- >>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >>>> index 1e04ce724240..7a31c7c3cd21 100644 >>>> --- a/tools/lib/bpf/libbpf.c >>>> +++ b/tools/lib/bpf/libbpf.c >>>> @@ -4616,10 +4616,12 @@ bpf_object__create_maps(struct bpf_object *obj) >>>> char *cp, errmsg[STRERR_BUFSIZE]; >>>> unsigned int i, j; >>>> int err; >>>> + bool retried = false; >>> >>> retried has to be reset for each map, so just move it inside the for >>> loop? you can also generalize it to retry_cnt (> 1 attempts) to allow >>> for more extreme cases of multiple loaders fighting very heavily >> >> If we move "retried = false" to inside the loop, then there is no need >> for retry_cnt. Single retry for each map should be enough to resolve the >> race. In any case, I'm going to move "retried = false", as you suggested. > > Right, I was originally thinking about the case where already pinned > map might get unpinned. But then subsequently rejected the idea of > re-creating the map :) So single retry should do. > >> >>> >>>> >>>> for (i = 0; i < obj->nr_maps; i++) { >>>> map = &obj->maps[i]; >>>> >>>> +retry: >>>> if (map->pin_path) { >>>> err = bpf_object__reuse_map(map); >>>> if (err) { >>>> @@ -4660,9 +4662,13 @@ bpf_object__create_maps(struct bpf_object *obj) >>>> if (map->pin_path && !map->pinned) { >>>> err = bpf_map__pin(map, NULL); >>>> if (err) { >>>> + zclose(map->fd); >>>> + if (!retried && err == EEXIST) { >>> >>> so I'm also wondering... should we commit at this point to trying to >>> pin and not attempt to re-create the map? I'm worried that >>> bpf_object__create_map() is not designed and tested to be called >>> multiple times for the same bpf_map, but it's technically possible for >>> it to be called multiple times in this scenario. Check the inner map >> >> Good call. I'm going to add "if (retried && map->fd < 0) { return >> -ENOENT; }" after the "if (map->pinned) { err = bpf_object__reuse_map() >> ... }" statement. This should prevent from invoking >> bpf_object__create_map() multiple times. >> >>> creation scenario, for example (btw, I think there is a bug in >>> bpf_object__create_map clean up for inner map, care to take a look at >>> that as well?). >> >> In the case of the inner map, it should be destroyed inside >> bpf_object__create_map() after a successful BPF_MAP_CREATE. So AFAIU, >> there should be no need for the cleanup. Or do I miss something? > > But if outer map creation fails, we won't do > bpf_map__destroy(map->inner_map);, which is one bug. And then with > your retry logic we also don't clean up the internal state of the > bpf_map, which is another one. It would be good to add a self-test > simulating such situations (e.g., by specifying wrong key_size for > outer_map, but correct inner_map definition). Not sure how to reliably > simulate this pinning race, though. Regarding the second case (i.e., not cleaning up the internal state), I think no additional cleanup is needed with this patch [1] (main diff from prev vsn is that we call bpf_object__map_create() only once). The relevant calls are the following: - bpf_object__create_map(): map->inner_map is destroyed anyway after a successful call, map->fd is closed if pinning fails. - bpf_object__populate_internal_map(): created map elements will be destroyed upon close(map->fd). - init_map_slots(): slots are freed after their initialization. [1]: https://gist.github.com/brb/fff66e47586373fdc1fe39b88175036c > > Can you please add at least the first test case? > >> >>> >>> So unless we want to allow map re-creation if (in a highly unlikely >>> scenario) someone already unpinned the other instance, I'd say we >>> should just bpf_map__pin() here directly, maybe in a short loop to >>> allow for a few attempts. >>> >>>> + retried = true; >>>> + goto retry; >>>> + } >>>> pr_warn("map '%s': failed to auto-pin at '%s': %d\n", >>>> map->name, map->pin_path, err); >>>> - zclose(map->fd); >>>> goto err_out; >>>> } >>>> } >>>> -- >>>> 2.32.0 >>>> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-22 13:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-05 19:09 [PATCH bpf] libbpf: fix race when pinning maps in parallel Martynas Pumputis 2021-07-07 22:38 ` Andrii Nakryiko 2021-07-08 15:52 ` Martynas Pumputis 2021-07-08 20:33 ` Andrii Nakryiko 2021-07-15 10:17 ` Martynas Pumputis 2021-07-22 13:56 ` Martynas Pumputis
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.