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:

quoted

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 <redacted>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Martin KaFai Lau <redacted>
---
 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