Message ID | 20220806074019.2756957-2-houtao@huaweicloud.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | BPF |
Headers | show |
Series | fixes for bpf map iterator | expand |
On 8/6/22 12:40 AM, Hou Tao wrote: > From: Hou Tao <houtao1@huawei.com> > > During bpf(BPF_LINK_CREATE) for BPF_TRACE_ITER, bpf_iter_attach_map() > has already acquired a map uref, but the uref may be released by > bpf_link_release() during th reading of map iterator. some wording issue: bpf_iter_attach_map() acquires a map uref, and the uref may be released before or in the middle of iterating map elements. For example, the uref could be released in bpf_iter_detach_map() as part of bpf_link_release(), or could be released in bpf_map_put_with_uref() as part of bpf_map_release(). > > Alternative fix is acquiring an extra bpf_link reference just like > a pinned map iterator does, but it introduces unnecessary dependency > on bpf_link instead of bpf_map. > > So choose another fix: acquiring an extra map uref in .init_seq_private > for array map iterator. > > Fixes: d3cc2ab546ad ("bpf: Implement bpf iterator for array maps") > Signed-off-by: Hou Tao <houtao1@huawei.com> Acked-by: Yonghong Song <yhs@fb.com> > --- > kernel/bpf/arraymap.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c > index d3e734bf8056..bf6898bb7cb8 100644 > --- a/kernel/bpf/arraymap.c > +++ b/kernel/bpf/arraymap.c > @@ -649,6 +649,12 @@ static int bpf_iter_init_array_map(void *priv_data, > seq_info->percpu_value_buf = value_buf; > } > > + /* > + * During bpf(BPF_LINK_CREATE), bpf_iter_attach_map() has already > + * acquired a map uref, but the uref may be released by > + * bpf_link_release(), so acquire an extra map uref for iterator. > + */ > + bpf_map_inc_with_uref(map); > seq_info->map = map; > return 0; > } > @@ -657,6 +663,7 @@ static void bpf_iter_fini_array_map(void *priv_data) > { > struct bpf_iter_seq_array_map_info *seq_info = priv_data; > > + bpf_map_put_with_uref(seq_info->map); > kfree(seq_info->percpu_value_buf); > } >
Hi, On 8/8/2022 10:53 PM, Yonghong Song wrote: > > > On 8/6/22 12:40 AM, Hou Tao wrote: >> From: Hou Tao <houtao1@huawei.com> >> >> During bpf(BPF_LINK_CREATE) for BPF_TRACE_ITER, bpf_iter_attach_map() >> has already acquired a map uref, but the uref may be released by >> bpf_link_release() during th reading of map iterator. > > some wording issue: > bpf_iter_attach_map() acquires a map uref, and the uref may be released > before or in the middle of iterating map elements. For example, the uref > could be released in bpf_iter_detach_map() as part of > bpf_link_release(), or could be released in bpf_map_put_with_uref() > as part of bpf_map_release(). Thanks, it is much better than the original commit message. Will update in v2. Regards Tao > >> >> Alternative fix is acquiring an extra bpf_link reference just like >> a pinned map iterator does, but it introduces unnecessary dependency >> on bpf_link instead of bpf_map. >> >> So choose another fix: acquiring an extra map uref in .init_seq_private >> for array map iterator. >> >> Fixes: d3cc2ab546ad ("bpf: Implement bpf iterator for array maps") >> Signed-off-by: Hou Tao <houtao1@huawei.com> > > Acked-by: Yonghong Song <yhs@fb.com> > >> --- >> kernel/bpf/arraymap.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c >> index d3e734bf8056..bf6898bb7cb8 100644 >> --- a/kernel/bpf/arraymap.c >> +++ b/kernel/bpf/arraymap.c >> @@ -649,6 +649,12 @@ static int bpf_iter_init_array_map(void *priv_data, >> seq_info->percpu_value_buf = value_buf; >> } >> + /* >> + * During bpf(BPF_LINK_CREATE), bpf_iter_attach_map() has already >> + * acquired a map uref, but the uref may be released by >> + * bpf_link_release(), so acquire an extra map uref for iterator. >> + */ >> + bpf_map_inc_with_uref(map); >> seq_info->map = map; >> return 0; >> } >> @@ -657,6 +663,7 @@ static void bpf_iter_fini_array_map(void *priv_data) >> { >> struct bpf_iter_seq_array_map_info *seq_info = priv_data; >> + bpf_map_put_with_uref(seq_info->map); >> kfree(seq_info->percpu_value_buf); >> } >>
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c index d3e734bf8056..bf6898bb7cb8 100644 --- a/kernel/bpf/arraymap.c +++ b/kernel/bpf/arraymap.c @@ -649,6 +649,12 @@ static int bpf_iter_init_array_map(void *priv_data, seq_info->percpu_value_buf = value_buf; } + /* + * During bpf(BPF_LINK_CREATE), bpf_iter_attach_map() has already + * acquired a map uref, but the uref may be released by + * bpf_link_release(), so acquire an extra map uref for iterator. + */ + bpf_map_inc_with_uref(map); seq_info->map = map; return 0; } @@ -657,6 +663,7 @@ static void bpf_iter_fini_array_map(void *priv_data) { struct bpf_iter_seq_array_map_info *seq_info = priv_data; + bpf_map_put_with_uref(seq_info->map); kfree(seq_info->percpu_value_buf); }