* [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 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).