| Message ID | 20200226130345.209469-11-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Add trampoline and dispatcher to /proc/kallsyms | expand |
On Wed, Feb 26, 2020 at 5:06 AM Jiri Olsa <jolsa@kernel.org> wrote: > > When bpf_prog is removed from kallsyms it's on the way > out to be removed, so we don't care about lnode state. > > However the bpf_ksym_del will be used also by bpf_trampoline > and bpf_dispatcher objects, which stay allocated even when > they are not in kallsyms list, hence the lnode re-init. > > The list_del_rcu commentary states that we need to call > synchronize_rcu, before we can change/re-init the list_head > pointers. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> Acked-by: Song Liu <songliubraving@fb.com>
On Wed, Feb 26, 2020 at 02:03:37PM +0100, Jiri Olsa wrote: > When bpf_prog is removed from kallsyms it's on the way > out to be removed, so we don't care about lnode state. > > However the bpf_ksym_del will be used also by bpf_trampoline > and bpf_dispatcher objects, which stay allocated even when > they are not in kallsyms list, hence the lnode re-init. > > The list_del_rcu commentary states that we need to call > synchronize_rcu, before we can change/re-init the list_head > pointers. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > kernel/bpf/core.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > index c95424fc53de..1af2109b45c7 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -672,6 +672,13 @@ void bpf_ksym_del(struct bpf_ksym *ksym) > spin_lock_bh(&bpf_lock); > __bpf_ksym_del(ksym); > spin_unlock_bh(&bpf_lock); > + > + /* > + * As explained in list_del_rcu, We must call synchronize_rcu > + * before changing list_head pointers. > + */ > + synchronize_rcu(); > + INIT_LIST_HEAD_RCU(&ksym->lnode); I don't understand what this is for. The comment made it even more confusing. What kind of ksym reuse are you expecting? Looking at trampoline and dispatcher patches I think cnt == 0 condition is unnecessary. Just add them to ksym at creation time and remove from ksym at destroy. Both are executable code sections. Though RIP should never point into them while there are no progs I think it's better to keep them in ksym always. Imagine sw race conditions in destruction. CPU bugs. What not. In patch 3 the name bpf_get_prog_addr_region(const struct bpf_prog *prog) became wrong and 'const' pointer makes it even more misleading. The function is not getting prog addr. It's setting ksym's addr. I think it should be called: bpf_ksym_set_addr(struct bpf_ksym *ksym); __always_inline should be removed too. Similar in patch 4: static void bpf_get_prog_name(const struct bpf_prog *prog) also is wrong for the same reasons. It probably should be: static void bpf_ksym_set_name(struct bpf_ksym *ksym); I'm still not confortable with patch 15 sorting bit. next = rb_next(&ksym->tnode.node[0]); if (next) is too tricky for me. I cannot wrap my head yet. Since user space doesn't rely on sorted order could you drop it? Do patches 16-18 strongly depend on patches 1-15 ? We can take them via bpf-next tree. No problem. Just need Arnaldo's ack. Overall looks great. All around important work. Please address above and respin. I would like to land it soon.
On Thu, Feb 27, 2020 at 11:50:36AM -0800, Alexei Starovoitov wrote: > On Wed, Feb 26, 2020 at 02:03:37PM +0100, Jiri Olsa wrote: > > When bpf_prog is removed from kallsyms it's on the way > > out to be removed, so we don't care about lnode state. > > > > However the bpf_ksym_del will be used also by bpf_trampoline > > and bpf_dispatcher objects, which stay allocated even when > > they are not in kallsyms list, hence the lnode re-init. > > > > The list_del_rcu commentary states that we need to call > > synchronize_rcu, before we can change/re-init the list_head > > pointers. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > kernel/bpf/core.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > > index c95424fc53de..1af2109b45c7 100644 > > --- a/kernel/bpf/core.c > > +++ b/kernel/bpf/core.c > > @@ -672,6 +672,13 @@ void bpf_ksym_del(struct bpf_ksym *ksym) > > spin_lock_bh(&bpf_lock); > > __bpf_ksym_del(ksym); > > spin_unlock_bh(&bpf_lock); > > + > > + /* > > + * As explained in list_del_rcu, We must call synchronize_rcu > > + * before changing list_head pointers. > > + */ > > + synchronize_rcu(); > > + INIT_LIST_HEAD_RCU(&ksym->lnode); > > I don't understand what this is for. > The comment made it even more confusing. > What kind of ksym reuse are you expecting? > > Looking at trampoline and dispatcher patches I think cnt == 0 > condition is unnecessary. Just add them to ksym at creation time > and remove from ksym at destroy. Both are executable code sections. > Though RIP should never point into them while there are no progs > I think it's better to keep them in ksym always. > Imagine sw race conditions in destruction. CPU bugs. What not. aah ok, that should also solve your first question, because the code above won't be needed anymore I wish I read this comment before I prepared elabored ascii/code picture of why the code above is needed ;-)) > > In patch 3 the name > bpf_get_prog_addr_region(const struct bpf_prog *prog) > became wrong and 'const' pointer makes it even more misleading. > The function is not getting prog addr. It's setting ksym's addr. > I think it should be called: > bpf_ksym_set_addr(struct bpf_ksym *ksym); > __always_inline should be removed too. ok, will change > > Similar in patch 4: > static void bpf_get_prog_name(const struct bpf_prog *prog) > also is wrong for the same reasons. > It probably should be: > static void bpf_ksym_set_name(struct bpf_ksym *ksym); ok > > I'm still not confortable with patch 15 sorting bit. > next = rb_next(&ksym->tnode.node[0]); > if (next) > is too tricky for me. I cannot wrap my head yet. > Since user space doesn't rely on sorted order could you drop it? yes, as I said I only added it because I liked how simple it turned out to be > > Do patches 16-18 strongly depend on patches 1-15 ? > We can take them via bpf-next tree. No problem. Just need Arnaldo's ack. actualy there're some changes on the list from this week, that touch the same code, so we might need to take them through Arnaldo's code I'l double check > > Overall looks great. All around important work. > Please address above and respin. I would like to land it soon. > will respin soon, thanks for review jirka
Em Thu, Feb 27, 2020 at 11:50:36AM -0800, Alexei Starovoitov escreveu: > On Wed, Feb 26, 2020 at 02:03:37PM +0100, Jiri Olsa wrote: > > When bpf_prog is removed from kallsyms it's on the way > > out to be removed, so we don't care about lnode state. > > > > However the bpf_ksym_del will be used also by bpf_trampoline > > and bpf_dispatcher objects, which stay allocated even when > > they are not in kallsyms list, hence the lnode re-init. > > > > The list_del_rcu commentary states that we need to call > > synchronize_rcu, before we can change/re-init the list_head > > pointers. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > kernel/bpf/core.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > > index c95424fc53de..1af2109b45c7 100644 > > --- a/kernel/bpf/core.c > > +++ b/kernel/bpf/core.c > > @@ -672,6 +672,13 @@ void bpf_ksym_del(struct bpf_ksym *ksym) > > spin_lock_bh(&bpf_lock); > > __bpf_ksym_del(ksym); > > spin_unlock_bh(&bpf_lock); > > + > > + /* > > + * As explained in list_del_rcu, We must call synchronize_rcu > > + * before changing list_head pointers. > > + */ > > + synchronize_rcu(); > > + INIT_LIST_HEAD_RCU(&ksym->lnode); > > I don't understand what this is for. > The comment made it even more confusing. > What kind of ksym reuse are you expecting? > > Looking at trampoline and dispatcher patches I think cnt == 0 > condition is unnecessary. Just add them to ksym at creation time > and remove from ksym at destroy. Both are executable code sections. > Though RIP should never point into them while there are no progs > I think it's better to keep them in ksym always. > Imagine sw race conditions in destruction. CPU bugs. What not. > > In patch 3 the name > bpf_get_prog_addr_region(const struct bpf_prog *prog) > became wrong and 'const' pointer makes it even more misleading. > The function is not getting prog addr. It's setting ksym's addr. > I think it should be called: > bpf_ksym_set_addr(struct bpf_ksym *ksym); > __always_inline should be removed too. > > Similar in patch 4: > static void bpf_get_prog_name(const struct bpf_prog *prog) > also is wrong for the same reasons. > It probably should be: > static void bpf_ksym_set_name(struct bpf_ksym *ksym); > > I'm still not confortable with patch 15 sorting bit. > next = rb_next(&ksym->tnode.node[0]); > if (next) > is too tricky for me. I cannot wrap my head yet. > Since user space doesn't rely on sorted order could you drop it? > > Do patches 16-18 strongly depend on patches 1-15 ? > We can take them via bpf-next tree. No problem. Just need Arnaldo's ack. No problems, sent the acks, we can sort out problems later, but from the top of my mind I can't antecipate any, - Arnaldo > Overall looks great. All around important work. > Please address above and respin. I would like to land it soon.
Em Fri, Feb 28, 2020 at 01:17:08PM +0100, Jiri Olsa escreveu: > On Thu, Feb 27, 2020 at 11:50:36AM -0800, Alexei Starovoitov wrote: > > On Wed, Feb 26, 2020 at 02:03:37PM +0100, Jiri Olsa wrote: > > Do patches 16-18 strongly depend on patches 1-15 ? > > We can take them via bpf-next tree. No problem. Just need Arnaldo's ack. > > actualy there're some changes on the list from this week, that touch > the same code, so we might need to take them through Arnaldo's code Ravi's patches, yeah, will push them via perf/urgent now, since those are fixes, but I guess those won't clash... > I'l double check Please > > > > Overall looks great. All around important work. > > Please address above and respin. I would like to land it soon. > > > > will respin soon, thanks for review > > jirka
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index c95424fc53de..1af2109b45c7 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -672,6 +672,13 @@ void bpf_ksym_del(struct bpf_ksym *ksym) spin_lock_bh(&bpf_lock); __bpf_ksym_del(ksym); spin_unlock_bh(&bpf_lock); + + /* + * As explained in list_del_rcu, We must call synchronize_rcu + * before changing list_head pointers. + */ + synchronize_rcu(); + INIT_LIST_HEAD_RCU(&ksym->lnode); } static bool bpf_prog_kallsyms_candidate(const struct bpf_prog *fp)
When bpf_prog is removed from kallsyms it's on the way out to be removed, so we don't care about lnode state. However the bpf_ksym_del will be used also by bpf_trampoline and bpf_dispatcher objects, which stay allocated even when they are not in kallsyms list, hence the lnode re-init. The list_del_rcu commentary states that we need to call synchronize_rcu, before we can change/re-init the list_head pointers. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- kernel/bpf/core.c | 7 +++++++ 1 file changed, 7 insertions(+)