lkml.org 
[lkml]   [2018]   [Jul]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v4 bpf-next 08/14] bpf: introduce the bpf_get_local_storage() helper function
    On Tue, Jul 31, 2018 at 12:34:06PM +0200, Daniel Borkmann wrote:
    > Hi Roman,
    >
    > On 07/27/2018 11:52 PM, Roman Gushchin wrote:
    > > The bpf_get_local_storage() helper function is used
    > > to get a pointer to the bpf local storage from a bpf program.
    > >
    > > It takes a pointer to a storage map and flags as arguments.
    > > Right now it accepts only cgroup storage maps, and flags
    > > argument has to be 0. Further it can be extended to support
    > > other types of local storage: e.g. thread local storage etc.
    > >
    > > 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.h | 2 ++
    > > include/uapi/linux/bpf.h | 13 ++++++++++++-
    > > kernel/bpf/cgroup.c | 2 ++
    > > kernel/bpf/core.c | 1 +
    > > kernel/bpf/helpers.c | 20 ++++++++++++++++++++
    > > kernel/bpf/verifier.c | 18 ++++++++++++++++++
    > > net/core/filter.c | 23 ++++++++++++++++++++++-
    > > 7 files changed, 77 insertions(+), 2 deletions(-)
    > >
    > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
    > > index ca4ac2a39def..cd8790d2c6ed 100644
    > > --- a/include/linux/bpf.h
    > > +++ b/include/linux/bpf.h
    > > @@ -788,6 +788,8 @@ extern const struct bpf_func_proto bpf_sock_map_update_proto;
    > > extern const struct bpf_func_proto bpf_sock_hash_update_proto;
    > > extern const struct bpf_func_proto bpf_get_current_cgroup_id_proto;
    > >
    > > +extern const struct bpf_func_proto bpf_get_local_storage_proto;
    > > +
    > > /* Shared helpers among cBPF and eBPF. */
    > > void bpf_user_rnd_init_once(void);
    > > u64 bpf_user_rnd_u32(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
    > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
    > > index a0aa53148763..495180f229ee 100644
    > > --- a/include/uapi/linux/bpf.h
    > > +++ b/include/uapi/linux/bpf.h
    > > @@ -2081,6 +2081,16 @@ union bpf_attr {
    > > * Return
    > > * A 64-bit integer containing the current cgroup id based
    > > * on the cgroup within which the current task is running.
    > > + *
    > > + * void* get_local_storage(void *map, u64 flags)
    > > + * Description
    > > + * Get the pointer to the local storage area.
    > > + * The type and the size of the local storage is defined
    > > + * by the *map* argument.
    > > + * The *flags* meaning is specific for each map type,
    > > + * and has to be 0 for cgroup local storage.
    > > + * Return
    > > + * Pointer to the local storage area.
    > > */
    >
    > I think it would be crucial to clarify what underlying assumption the
    > program writer can make with regards to concurrent access to this storage.
    >
    > E.g. in this context, can _only_ BPF_XADD be used for counters as otherwise
    > any other type of access may race in parallel, or are we protected by the
    > socket lock and can safely override all data in this buffer via normal stores
    > (e.g. for socket related progs)? What about other types?
    >
    > Right now nothing is mentioned here, but I think it must be clarified to
    > avoid any surprises. E.g. in normal htab case program can at least use the
    > map update there for atomic value updates, but those are disallowed from
    > the cgroup local storage, hence my question. Btw, no need to resend, I can
    > also update the paragraph there.

    Fair enough.

    Mid- to long-term we want to add a per-cpu version of the cgroup local storage,
    and, possible, some locking API. But right now XADD is what should be used.

    I think something like this should work here:

    --

    Depending on the bpf program type, a local storage area can be shared between
    multiple instances of the bpf program, running simultaneously.
    A user should care about the synchronization by himself. For example,
    by using BPF_STX_XADD instruction to alter the shared data.

    --

    Please, feel free to change/add to the text above.

    Also, it might be good to change the example to use STX_XADD:

    index 0597943ce34b..dc83fb2d3f27 100644
    --- a/tools/testing/selftests/bpf/test_cgroup_storage.c
    +++ b/tools/testing/selftests/bpf/test_cgroup_storage.c
    @@ -18,9 +18,9 @@ int main(int argc, char **argv)
    BPF_MOV64_IMM(BPF_REG_2, 0), /* flags, not used */
    BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
    BPF_FUNC_get_local_storage),
    + BPF_MOV64_IMM(BPF_REG_1, 1),
    + BPF_STX_XADD(BPF_DW, BPF_REG_0, BPF_REG_1, 0),
    BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_0, 0),
    - BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, 1),
    - BPF_STX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, 0),
    BPF_ALU64_IMM(BPF_AND, BPF_REG_1, 0x1),
    BPF_MOV64_REG(BPF_REG_0, BPF_REG_1),
    BPF_EXIT_INSN(),

    Thank you!

    Roman

    \
     
     \ /
      Last update: 2018-07-31 19:25    [W:3.399 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site