Message ID | 20180720174558.5829-5-guro@fb.com |
---|---|
State | Changes Requested, archived |
Delegated to: | BPF Maintainers |
Headers | show |
Series | bpf: cgroup local storage | expand |
On 07/20/2018 07:45 PM, Roman Gushchin wrote: > If a bpf program is using cgroup local storage, allocate > a bpf_cgroup_storage structure automatically on attaching the program > to a cgroup and save the pointer into the corresponding bpf_prog_list > entry. > Analogically, release the cgroup local storage on detaching > of the bpf program. > > Signed-off-by: Roman Gushchin <guro@fb.com> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Acked-by: Martin KaFai Lau <kafai@fb.com> > --- > include/linux/bpf-cgroup.h | 1 + > kernel/bpf/cgroup.c | 28 ++++++++++++++++++++++++++-- > 2 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h > index 1b1b4e94d77d..f37347331fdb 100644 > --- a/include/linux/bpf-cgroup.h > +++ b/include/linux/bpf-cgroup.h > @@ -42,6 +42,7 @@ struct bpf_cgroup_storage { > struct bpf_prog_list { > struct list_head node; > struct bpf_prog *prog; > + struct bpf_cgroup_storage *storage; > }; > > struct bpf_prog_array; > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c > index badabb0b435c..986ff18ef92e 100644 > --- a/kernel/bpf/cgroup.c > +++ b/kernel/bpf/cgroup.c > @@ -34,6 +34,8 @@ void cgroup_bpf_put(struct cgroup *cgrp) > list_for_each_entry_safe(pl, tmp, progs, node) { > list_del(&pl->node); > bpf_prog_put(pl->prog); > + bpf_cgroup_storage_unlink(pl->storage); > + bpf_cgroup_storage_free(pl->storage); > kfree(pl); > static_branch_dec(&cgroup_bpf_enabled_key); > } > @@ -188,6 +190,7 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog, > { > struct list_head *progs = &cgrp->bpf.progs[type]; > struct bpf_prog *old_prog = NULL; > + struct bpf_cgroup_storage *storage, *old_storage = NULL; > struct cgroup_subsys_state *css; > struct bpf_prog_list *pl; > bool pl_was_allocated; > @@ -210,6 +213,10 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog, > if (prog_list_length(progs) >= BPF_CGROUP_MAX_PROGS) > return -E2BIG; > > + storage = bpf_cgroup_storage_alloc(prog); > + if (IS_ERR(storage)) > + return -ENOMEM; > + > if (flags & BPF_F_ALLOW_MULTI) { > list_for_each_entry(pl, progs, node) > if (pl->prog == prog) > @@ -217,24 +224,33 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog, > return -EINVAL; > > pl = kmalloc(sizeof(*pl), GFP_KERNEL); > - if (!pl) > + if (!pl) { > + bpf_cgroup_storage_free(storage); > return -ENOMEM; > + } Above code is: storage = bpf_cgroup_storage_alloc(prog); if (IS_ERR(storage)) return -ENOMEM; if (flags & BPF_F_ALLOW_MULTI) { list_for_each_entry(pl, progs, node) if (pl->prog == prog) /* disallow attaching the same prog twice */ return -EINVAL; pl = kmalloc(sizeof(*pl), GFP_KERNEL); if (!pl) { bpf_cgroup_storage_free(storage); return -ENOMEM; } Given bpf_cgroup_storage_alloc() only changes the mem and prepares (kmallocs) the local storage buffer, we would leak it above where we attach the same prog twice, no? > + > pl_was_allocated = true; > pl->prog = prog; > + pl->storage = storage; > list_add_tail(&pl->node, progs); > } else { > if (list_empty(progs)) { > pl = kmalloc(sizeof(*pl), GFP_KERNEL); > - if (!pl) > + if (!pl) { > + bpf_cgroup_storage_free(storage); > return -ENOMEM; > + } > pl_was_allocated = true; > list_add_tail(&pl->node, progs); > } else { > pl = list_first_entry(progs, typeof(*pl), node); > old_prog = pl->prog; > + old_storage = pl->storage; > + bpf_cgroup_storage_unlink(old_storage); > pl_was_allocated = false; > } > pl->prog = prog; > + pl->storage = storage; > } > > cgrp->bpf.flags[type] = flags; > @@ -257,10 +273,13 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog, > } > > static_branch_inc(&cgroup_bpf_enabled_key); > + if (old_storage) > + bpf_cgroup_storage_free(old_storage); > if (old_prog) { > bpf_prog_put(old_prog); > static_branch_dec(&cgroup_bpf_enabled_key); > } > + bpf_cgroup_storage_link(storage, cgrp, type); > return 0; > > cleanup: > @@ -276,6 +295,9 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog, > > /* and cleanup the prog list */ > pl->prog = old_prog; > + bpf_cgroup_storage_free(pl->storage); > + pl->storage = old_storage; > + bpf_cgroup_storage_link(old_storage, cgrp, type); > if (pl_was_allocated) { > list_del(&pl->node); > kfree(pl); > @@ -356,6 +378,8 @@ int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog, > > /* now can actually delete it from this cgroup list */ > list_del(&pl->node); > + bpf_cgroup_storage_unlink(pl->storage); > + bpf_cgroup_storage_free(pl->storage); > kfree(pl); > if (list_empty(progs)) > /* last program was detached, reset flags to zero */ >
diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h index 1b1b4e94d77d..f37347331fdb 100644 --- a/include/linux/bpf-cgroup.h +++ b/include/linux/bpf-cgroup.h @@ -42,6 +42,7 @@ struct bpf_cgroup_storage { struct bpf_prog_list { struct list_head node; struct bpf_prog *prog; + struct bpf_cgroup_storage *storage; }; struct bpf_prog_array; diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c index badabb0b435c..986ff18ef92e 100644 --- a/kernel/bpf/cgroup.c +++ b/kernel/bpf/cgroup.c @@ -34,6 +34,8 @@ void cgroup_bpf_put(struct cgroup *cgrp) list_for_each_entry_safe(pl, tmp, progs, node) { list_del(&pl->node); bpf_prog_put(pl->prog); + bpf_cgroup_storage_unlink(pl->storage); + bpf_cgroup_storage_free(pl->storage); kfree(pl); static_branch_dec(&cgroup_bpf_enabled_key); } @@ -188,6 +190,7 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog, { struct list_head *progs = &cgrp->bpf.progs[type]; struct bpf_prog *old_prog = NULL; + struct bpf_cgroup_storage *storage, *old_storage = NULL; struct cgroup_subsys_state *css; struct bpf_prog_list *pl; bool pl_was_allocated; @@ -210,6 +213,10 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog, if (prog_list_length(progs) >= BPF_CGROUP_MAX_PROGS) return -E2BIG; + storage = bpf_cgroup_storage_alloc(prog); + if (IS_ERR(storage)) + return -ENOMEM; + if (flags & BPF_F_ALLOW_MULTI) { list_for_each_entry(pl, progs, node) if (pl->prog == prog) @@ -217,24 +224,33 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog, return -EINVAL; pl = kmalloc(sizeof(*pl), GFP_KERNEL); - if (!pl) + if (!pl) { + bpf_cgroup_storage_free(storage); return -ENOMEM; + } + pl_was_allocated = true; pl->prog = prog; + pl->storage = storage; list_add_tail(&pl->node, progs); } else { if (list_empty(progs)) { pl = kmalloc(sizeof(*pl), GFP_KERNEL); - if (!pl) + if (!pl) { + bpf_cgroup_storage_free(storage); return -ENOMEM; + } pl_was_allocated = true; list_add_tail(&pl->node, progs); } else { pl = list_first_entry(progs, typeof(*pl), node); old_prog = pl->prog; + old_storage = pl->storage; + bpf_cgroup_storage_unlink(old_storage); pl_was_allocated = false; } pl->prog = prog; + pl->storage = storage; } cgrp->bpf.flags[type] = flags; @@ -257,10 +273,13 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog, } static_branch_inc(&cgroup_bpf_enabled_key); + if (old_storage) + bpf_cgroup_storage_free(old_storage); if (old_prog) { bpf_prog_put(old_prog); static_branch_dec(&cgroup_bpf_enabled_key); } + bpf_cgroup_storage_link(storage, cgrp, type); return 0; cleanup: @@ -276,6 +295,9 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog, /* and cleanup the prog list */ pl->prog = old_prog; + bpf_cgroup_storage_free(pl->storage); + pl->storage = old_storage; + bpf_cgroup_storage_link(old_storage, cgrp, type); if (pl_was_allocated) { list_del(&pl->node); kfree(pl); @@ -356,6 +378,8 @@ int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog, /* now can actually delete it from this cgroup list */ list_del(&pl->node); + bpf_cgroup_storage_unlink(pl->storage); + bpf_cgroup_storage_free(pl->storage); kfree(pl); if (list_empty(progs)) /* last program was detached, reset flags to zero */