On Tue, Jun 23, 2020 at 11:24 PM CEST, Martin KaFai Lau wrote: > On Tue, Jun 23, 2020 at 10:59:37PM +0200, Jakub Sitnicki wrote: >> On Tue, Jun 23, 2020 at 09:33 PM CEST, Martin KaFai Lau wrote: >> > On Tue, Jun 23, 2020 at 12:34:58PM +0200, Jakub Sitnicki wrote: >> > >> > [ ... ] >> > >> >> @@ -93,8 +108,16 @@ static int bpf_netns_link_update_prog(struct bpf_link >> >> *link, >> >> goto out_unlock; >> >> } >> >> >> >> + run_array = rcu_dereference_protected(net->bpf.run_array[type], >> >> + >> >> lockdep_is_held(&netns_bpf_mutex)); >> >> + if (run_array) >> >> + ret = bpf_prog_array_replace_item(run_array, link->prog, >> >> new_prog); >> >> + else >> > When will this happen? >> >> This will never happen, unless there is a bug. As long as there is a >> link attached, run_array should never be detached (null). Because it can >> be handled gracefully, we fail the bpf(LINK_UPDATE) syscall. >> >> Your question makes me think that perhaps it should trigger a warning, >> with WARN_ON_ONCE, to signal clearly to the reader that this is an >> unexpected state. >> >> WDYT? > Thanks for confirming and the explanation. > > If it will never happen, I would skip the "if (run_array)". That > will help the code reading in the future. > > I would not WARN also.
Best code is no code :-) I realized that bpf_prog_array_replace_item() cannot fail either, unless there is a bug how we compile the prog_array. So I plan to remove that error check as well. Thanks for feedback.
