| Message ID | 20200916200925.1803161-1-kafai@fb.com |
|---|---|
| State | Deferred |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] bpf: Use hlist_add_head_rcu when linking to sk_storage | expand |
On Wed, Sep 16, 2020 at 01:09:25PM -0700, Martin KaFai Lau wrote: > The sk_storage->list will be traversed by rcu reader in parallel. > Thus, hlist_add_head_rcu() is needed in __selem_link_sk(). This > patch fixes it. > > This part of the code has recently been refactored in bpf-next. > A separate fix will be provided for the bpf-next tree. > > Fixes: 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage") > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- > net/core/bpf_sk_storage.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c > index b988f48153a4..d4d2a56e9d4a 100644 > --- a/net/core/bpf_sk_storage.c > +++ b/net/core/bpf_sk_storage.c > @@ -219,7 +219,7 @@ static void __selem_link_sk(struct bpf_sk_storage *sk_storage, > struct bpf_sk_storage_elem *selem) > { > RCU_INIT_POINTER(selem->sk_storage, sk_storage); > - hlist_add_head(&selem->snode, &sk_storage->list); > + hlist_add_head_rcu(&selem->snode, &sk_storage->list); > } Applying the same, yet very different from git point of view, patch to bpf and bpf-next trees will create a ton of confusion for everyone. I prefer to take this fix (in bpf-next form) into bpf-next only and apply this fix (in bpf form) to 5.9 and stable after the merge window. The code has been around since April 2019 and it wasn't hit in prod, so I don't think there is urgency. Agree?
On Thu, Sep 17, 2020 at 10:47:39AM -0700, Alexei Starovoitov wrote: > On Wed, Sep 16, 2020 at 01:09:25PM -0700, Martin KaFai Lau wrote: > > The sk_storage->list will be traversed by rcu reader in parallel. > > Thus, hlist_add_head_rcu() is needed in __selem_link_sk(). This > > patch fixes it. > > > > This part of the code has recently been refactored in bpf-next. > > A separate fix will be provided for the bpf-next tree. > > > > Fixes: 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage") > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > --- > > net/core/bpf_sk_storage.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c > > index b988f48153a4..d4d2a56e9d4a 100644 > > --- a/net/core/bpf_sk_storage.c > > +++ b/net/core/bpf_sk_storage.c > > @@ -219,7 +219,7 @@ static void __selem_link_sk(struct bpf_sk_storage *sk_storage, > > struct bpf_sk_storage_elem *selem) > > { > > RCU_INIT_POINTER(selem->sk_storage, sk_storage); > > - hlist_add_head(&selem->snode, &sk_storage->list); > > + hlist_add_head_rcu(&selem->snode, &sk_storage->list); > > } > > Applying the same, yet very different from git point of view, patch to > bpf and bpf-next trees will create a ton of confusion for everyone. > I prefer to take this fix (in bpf-next form) into bpf-next only and apply > this fix (in bpf form) to 5.9 and stable after the merge window. > The code has been around since April 2019 and it wasn't hit in prod, > so I don't think there is urgency. > Agree? Yep, agree.
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c index b988f48153a4..d4d2a56e9d4a 100644 --- a/net/core/bpf_sk_storage.c +++ b/net/core/bpf_sk_storage.c @@ -219,7 +219,7 @@ static void __selem_link_sk(struct bpf_sk_storage *sk_storage, struct bpf_sk_storage_elem *selem) { RCU_INIT_POINTER(selem->sk_storage, sk_storage); - hlist_add_head(&selem->snode, &sk_storage->list); + hlist_add_head_rcu(&selem->snode, &sk_storage->list); } static void selem_unlink_map(struct bpf_sk_storage_elem *selem)
The sk_storage->list will be traversed by rcu reader in parallel. Thus, hlist_add_head_rcu() is needed in __selem_link_sk(). This patch fixes it. This part of the code has recently been refactored in bpf-next. A separate fix will be provided for the bpf-next tree. Fixes: 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage") Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- net/core/bpf_sk_storage.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)