* Re: [PATCH v3 bpf-next 10/10] selftests/bpf: cgroup local storage-based network counters
From: Roman Gushchin @ 2018-09-28 10:08 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: netdev, Song Liu, linux-kernel, kernel-team, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20180928085356.56xe7javtd6cdfz6@ast-mbp.dhcp.thefacebook.com>
On Fri, Sep 28, 2018 at 10:53:58AM +0200, Alexei Starovoitov wrote:
> On Wed, Sep 26, 2018 at 12:33:26PM +0100, Roman Gushchin wrote:
> > This commit adds a bpf kselftest, which demonstrates how percpu
> > and shared cgroup local storage can be used for efficient lookup-free
> > network accounting.
> >
> > Cgroup local storage provides generic memory area with a very efficient
> > lookup free access. To avoid expensive atomic operations for each
> > packet, per-cpu cgroup local storage is used. Each packet is initially
> > charged to a per-cpu counter, and only if the counter reaches certain
> > value (32 in this case), the charge is moved into the global atomic
> > counter. This allows to amortize atomic operations, keeping reasonable
> > accuracy.
> >
> > The test also implements a naive network traffic throttling, mostly to
> > demonstrate the possibility of bpf cgroup--based network bandwidth
> > control.
> >
> > Expected output:
> > ./test_netcnt
> > test_netcnt:PASS
> >
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> > Acked-by: Song Liu <songliubraving@fb.com>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > ---
> > tools/testing/selftests/bpf/Makefile | 6 +-
> > tools/testing/selftests/bpf/netcnt_common.h | 23 +++
> > tools/testing/selftests/bpf/netcnt_prog.c | 71 +++++++++
> > tools/testing/selftests/bpf/test_netcnt.c | 153 ++++++++++++++++++++
> > 4 files changed, 251 insertions(+), 2 deletions(-)
> > create mode 100644 tools/testing/selftests/bpf/netcnt_common.h
> > create mode 100644 tools/testing/selftests/bpf/netcnt_prog.c
> > create mode 100644 tools/testing/selftests/bpf/test_netcnt.c
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index fd3851d5c079..5443399dd3a1 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -23,7 +23,8 @@ $(TEST_CUSTOM_PROGS): $(OUTPUT)/%: %.c
> > TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
> > test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
> > test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user \
> > - test_socket_cookie test_cgroup_storage test_select_reuseport
> > + test_socket_cookie test_cgroup_storage test_select_reuseport \
> > + test_netcnt
> >
> > TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test_obj_id.o \
> > test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o \
> > @@ -35,7 +36,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
> > test_get_stack_rawtp.o test_sockmap_kern.o test_sockhash_kern.o \
> > test_lwt_seg6local.o sendmsg4_prog.o sendmsg6_prog.o test_lirc_mode2_kern.o \
> > get_cgroup_id_kern.o socket_cookie_prog.o test_select_reuseport_kern.o \
> > - test_skb_cgroup_id_kern.o bpf_flow.o
> > + test_skb_cgroup_id_kern.o bpf_flow.o netcnt_prog.o
> >
> > # Order correspond to 'make run_tests' order
> > TEST_PROGS := test_kmod.sh \
> > @@ -72,6 +73,7 @@ $(OUTPUT)/test_tcpbpf_user: cgroup_helpers.c
> > $(OUTPUT)/test_progs: trace_helpers.c
> > $(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c
> > $(OUTPUT)/test_cgroup_storage: cgroup_helpers.c
> > +$(OUTPUT)/test_netcnt: cgroup_helpers.c
> >
> > .PHONY: force
> >
> > diff --git a/tools/testing/selftests/bpf/netcnt_common.h b/tools/testing/selftests/bpf/netcnt_common.h
> > new file mode 100644
> > index 000000000000..0e10fc276c2a
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/netcnt_common.h
> > @@ -0,0 +1,23 @@
> > +#ifndef __NETCNT_COMMON_H
> > +#define __NETCNT_COMMON_H
> > +
> > +#include <linux/types.h>
> > +
> > +#define MAX_PERCPU_PACKETS 32
> > +
> > +struct percpu_net_cnt {
> > + __u64 packets;
> > + __u64 bytes;
> > +
> > + __u64 prev_ts;
> > +
> > + __u64 prev_packets;
> > + __u64 prev_bytes;
> > +};
> > +
> > +struct net_cnt {
> > + __u64 packets;
> > + __u64 bytes;
> > +};
> > +
> > +#endif
> > diff --git a/tools/testing/selftests/bpf/netcnt_prog.c b/tools/testing/selftests/bpf/netcnt_prog.c
> > new file mode 100644
> > index 000000000000..1198abca1360
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/netcnt_prog.c
> > @@ -0,0 +1,71 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/bpf.h>
> > +#include <linux/version.h>
> > +
> > +#include "bpf_helpers.h"
> > +#include "netcnt_common.h"
> > +
> > +#define MAX_BPS (3 * 1024 * 1024)
> > +
> > +#define REFRESH_TIME_NS 100000000
> > +#define NS_PER_SEC 1000000000
> > +
> > +struct bpf_map_def SEC("maps") percpu_netcnt = {
> > + .type = BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE,
> > + .key_size = sizeof(struct bpf_cgroup_storage_key),
> > + .value_size = sizeof(struct percpu_net_cnt),
> > +};
> > +
> > +struct bpf_map_def SEC("maps") netcnt = {
> > + .type = BPF_MAP_TYPE_CGROUP_STORAGE,
> > + .key_size = sizeof(struct bpf_cgroup_storage_key),
> > + .value_size = sizeof(struct net_cnt),
> > +};
> > +
> > +SEC("cgroup/skb")
> > +int bpf_nextcnt(struct __sk_buff *skb)
> > +{
> > + struct percpu_net_cnt *percpu_cnt;
> > + char fmt[] = "%d %llu %llu\n";
> > + struct net_cnt *cnt;
> > + __u64 ts, dt;
> > + int ret;
> > +
> > + cnt = bpf_get_local_storage(&netcnt, 0);
> > + percpu_cnt = bpf_get_local_storage(&percpu_netcnt, 0);
> > +
> > + percpu_cnt->packets++;
> > + percpu_cnt->bytes += skb->len;
> > +
> > + if (percpu_cnt->packets > MAX_PERCPU_PACKETS) {
> > + __sync_fetch_and_add(&cnt->packets,
> > + percpu_cnt->packets);
> > + percpu_cnt->packets = 0;
> > +
> > + __sync_fetch_and_add(&cnt->bytes,
> > + percpu_cnt->bytes);
> > + percpu_cnt->bytes = 0;
> > + }
> > +
> > + ts = bpf_ktime_get_ns();
> > + dt = ts - percpu_cnt->prev_ts;
> > +
> > + dt *= MAX_BPS;
> > + dt /= NS_PER_SEC;
> > +
> > + if (cnt->bytes + percpu_cnt->bytes - percpu_cnt->prev_bytes < dt)
> > + ret = 1;
> > + else
> > + ret = 0;
> > +
> > + if (dt > REFRESH_TIME_NS) {
> > + percpu_cnt->prev_ts = ts;
> > + percpu_cnt->prev_packets = cnt->packets;
> > + percpu_cnt->prev_bytes = cnt->bytes;
> > + }
> > +
> > + return !!ret;
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
> > +__u32 _version SEC("version") = LINUX_VERSION_CODE;
> > diff --git a/tools/testing/selftests/bpf/test_netcnt.c b/tools/testing/selftests/bpf/test_netcnt.c
> > new file mode 100644
> > index 000000000000..aa424f8db466
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/test_netcnt.c
> > @@ -0,0 +1,153 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <assert.h>
> > +#include <sys/sysinfo.h>
> > +#include <sys/time.h>
> > +
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf.h>
> > +#include <bpf/libbpf.h>
> > +
> > +#include "cgroup_helpers.h"
> > +#include "bpf_rlimit.h"
> > +#include "netcnt_common.h"
> > +
> > +#define BPF_PROG "./netcnt_prog.o"
> > +#define TEST_CGROUP "/test-network-counters/"
> > +
> > +static int bpf_find_map(const char *test, struct bpf_object *obj,
> > + const char *name)
> > +{
> > + struct bpf_map *map;
> > +
> > + map = bpf_object__find_map_by_name(obj, name);
> > + if (!map) {
> > + printf("%s:FAIL:map '%s' not found\n", test, name);
> > + return -1;
> > + }
> > + return bpf_map__fd(map);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > + struct percpu_net_cnt *percpu_netcnt;
> > + struct bpf_cgroup_storage_key key;
> > + int map_fd, percpu_map_fd;
> > + int error = EXIT_FAILURE;
> > + struct net_cnt netcnt;
> > + struct bpf_object *obj;
> > + int prog_fd, cgroup_fd;
> > + unsigned long packets;
> > + int cpu, nproc;
> > + __u32 prog_cnt;
> > +
> > + nproc = get_nprocs_conf();
> > + percpu_netcnt = malloc(sizeof(*percpu_netcnt) * nproc);
> > + if (!percpu_netcnt) {
> > + printf("Not enough memory for per-cpu area (%d cpus)\n", nproc);
> > + goto err;
> > + }
> > +
> > + if (bpf_prog_load(BPF_PROG, BPF_PROG_TYPE_CGROUP_SKB,
> > + &obj, &prog_fd)) {
> > + printf("Failed to load bpf program\n");
> > + goto out;
> > + }
> > +
> > + if (setup_cgroup_environment()) {
> > + printf("Failed to load bpf program\n");
> > + goto err;
> > + }
> > +
> > + /* Create a cgroup, get fd, and join it */
> > + cgroup_fd = create_and_get_cgroup(TEST_CGROUP);
> > + if (!cgroup_fd) {
> > + printf("Failed to create test cgroup\n");
> > + goto err;
> > + }
> > +
> > + if (join_cgroup(TEST_CGROUP)) {
> > + printf("Failed to join cgroup\n");
> > + goto err;
> > + }
> > +
> > + /* Attach bpf program */
> > + if (bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_INET_EGRESS, 0)) {
> > + printf("Failed to attach bpf program");
> > + goto err;
> > + }
> > +
> > + assert(system("ping localhost -s 500 -c 10000 -f -q > /dev/null") == 0);
> > +
> > + if (bpf_prog_query(cgroup_fd, BPF_CGROUP_INET_EGRESS, 0, NULL, NULL,
> > + &prog_cnt)) {
> > + printf("Failed to query attached programs");
> > + goto err;
> > + }
> > +
> > + map_fd = bpf_find_map(__func__, obj, "netcnt");
> > + if (map_fd < 0) {
> > + printf("Failed to find bpf map with net counters");
> > + goto err;
> > + }
> > +
> > + percpu_map_fd = bpf_find_map(__func__, obj, "percpu_netcnt");
> > + if (percpu_map_fd < 0) {
> > + printf("Failed to find bpf map with percpu net counters");
> > + goto err;
> > + }
> > +
> > + if (bpf_map_get_next_key(map_fd, NULL, &key)) {
> > + printf("Failed to get key in cgroup storage\n");
> > + goto err;
> > + }
> > +
> > + if (bpf_map_lookup_elem(map_fd, &key, &netcnt)) {
> > + printf("Failed to lookup cgroup storage\n");
> > + goto err;
> > + }
> > +
> > + if (bpf_map_lookup_elem(percpu_map_fd, &key, &percpu_netcnt[0])) {
> > + printf("Failed to lookup percpu cgroup storage\n");
> > + goto err;
> > + }
> > +
> > + /* Some packets can be still in per-cpu cache, but not more than
> > + * MAX_PERCPU_PACKETS.
> > + */
> > + packets = netcnt.packets;
> > + for (cpu = 0; cpu < nproc; cpu++) {
> > + if (percpu_netcnt[cpu].packets > 32) {
>
> pls use MAX_PERCPU_PACKETS in the above check.
> could you also double check that if that #define is changed to 1k or so
> the exact "!= 10000" check below still works as expected?
Do you mean adding a new test with a different MAX_PERCPU_PACKETS?
>
> > + printf("Unexpected percpu value: %llu\n",
> > + percpu_netcnt[cpu].packets);
> > + goto err;
>
> > + }
> > +
> > + packets += percpu_netcnt[cpu].packets;
> > + }
> > +
> > + /* No packets should be lost */
> > + if (packets != 10000) {
> > + printf("Unexpected packet count: %lu\n", packets);
> > + goto err;
> > + }
> > +
> > + /* Let's check that bytes counter value is reasonable */
> > + if (netcnt.bytes < packets * 500 || netcnt.bytes > packets * 1500) {
>
> since packet count is accurate why byte count would vary ?
Tbh I'm not sure if the size of the packet here can vary depending
on the environment. Is there a nice way to get the expected size?
^ permalink raw reply
* Re: [PATCH v3 bpf-next 03/10] bpf: introduce per-cpu cgroup local storage
From: Roman Gushchin @ 2018-09-28 10:03 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: netdev, Song Liu, linux-kernel, kernel-team, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20180928084528.i5txkac34pmqvs3p@ast-mbp.dhcp.thefacebook.com>
On Fri, Sep 28, 2018 at 10:45:31AM +0200, Alexei Starovoitov wrote:
> On Wed, Sep 26, 2018 at 12:33:19PM +0100, Roman Gushchin wrote:
> > This commit introduced per-cpu cgroup local storage.
> >
> > Per-cpu cgroup local storage is very similar to simple cgroup storage
> > (let's call it shared), except all the data is per-cpu.
> >
> > The main goal of per-cpu variant is to implement super fast
> > counters (e.g. packet counters), which don't require neither
> > lookups, neither atomic operations.
> >
> > From userspace's point of view, accessing a per-cpu cgroup storage
> > is similar to other per-cpu map types (e.g. per-cpu hashmaps and
> > arrays).
> >
> > Writing to a per-cpu cgroup storage is not atomic, but is performed
> > by copying longs, so some minimal atomicity is here, exactly
> > as with other per-cpu maps.
> >
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > ---
> > include/linux/bpf-cgroup.h | 20 ++++-
> > include/linux/bpf.h | 1 +
> > include/linux/bpf_types.h | 1 +
> > include/uapi/linux/bpf.h | 1 +
> > kernel/bpf/helpers.c | 8 +-
> > kernel/bpf/local_storage.c | 148 ++++++++++++++++++++++++++++++++-----
> > kernel/bpf/syscall.c | 11 ++-
> > kernel/bpf/verifier.c | 15 +++-
> > 8 files changed, 177 insertions(+), 28 deletions(-)
> >
> > diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> > index 7e0c9a1d48b7..588dd5f0bd85 100644
> > --- a/include/linux/bpf-cgroup.h
> > +++ b/include/linux/bpf-cgroup.h
> > @@ -37,7 +37,10 @@ struct bpf_storage_buffer {
> > };
> >
> > struct bpf_cgroup_storage {
> > - struct bpf_storage_buffer *buf;
> > + union {
> > + struct bpf_storage_buffer *buf;
> > + void __percpu *percpu_buf;
> > + };
> > struct bpf_cgroup_storage_map *map;
> > struct bpf_cgroup_storage_key key;
> > struct list_head list;
> > @@ -109,6 +112,9 @@ int __cgroup_bpf_check_dev_permission(short dev_type, u32 major, u32 minor,
> > static inline enum bpf_cgroup_storage_type cgroup_storage_type(
> > struct bpf_map *map)
> > {
> > + if (map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> > + return BPF_CGROUP_STORAGE_PERCPU;
> > +
> > return BPF_CGROUP_STORAGE_SHARED;
> > }
> >
> > @@ -131,6 +137,10 @@ void bpf_cgroup_storage_unlink(struct bpf_cgroup_storage *storage);
> > int bpf_cgroup_storage_assign(struct bpf_prog *prog, struct bpf_map *map);
> > void bpf_cgroup_storage_release(struct bpf_prog *prog, struct bpf_map *map);
> >
> > +int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key, void *value);
> > +int bpf_percpu_cgroup_storage_update(struct bpf_map *map, void *key,
> > + void *value, u64 flags);
> > +
> > /* Wrappers for __cgroup_bpf_run_filter_skb() guarded by cgroup_bpf_enabled. */
> > #define BPF_CGROUP_RUN_PROG_INET_INGRESS(sk, skb) \
> > ({ \
> > @@ -285,6 +295,14 @@ static inline struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(
> > struct bpf_prog *prog, enum bpf_cgroup_storage_type stype) { return 0; }
> > static inline void bpf_cgroup_storage_free(
> > struct bpf_cgroup_storage *storage) {}
> > +static inline int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key,
> > + void *value) {
> > + return 0;
> > +}
> > +static inline int bpf_percpu_cgroup_storage_update(struct bpf_map *map,
> > + void *key, void *value, u64 flags) {
> > + return 0;
> > +}
> >
> > #define cgroup_bpf_enabled (0)
> > #define BPF_CGROUP_PRE_CONNECT_ENABLED(sk) (0)
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index b457fbe7b70b..018299a595c8 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -274,6 +274,7 @@ struct bpf_prog_offload {
> >
> > enum bpf_cgroup_storage_type {
> > BPF_CGROUP_STORAGE_SHARED,
> > + BPF_CGROUP_STORAGE_PERCPU,
> > __BPF_CGROUP_STORAGE_MAX
> > };
> >
> > diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> > index c9bd6fb765b0..5432f4c9f50e 100644
> > --- a/include/linux/bpf_types.h
> > +++ b/include/linux/bpf_types.h
> > @@ -43,6 +43,7 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_ARRAY, cgroup_array_map_ops)
> > #endif
> > #ifdef CONFIG_CGROUP_BPF
> > BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_STORAGE, cgroup_storage_map_ops)
> > +BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE, cgroup_storage_map_ops)
> > #endif
> > BPF_MAP_TYPE(BPF_MAP_TYPE_HASH, htab_map_ops)
> > BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_HASH, htab_percpu_map_ops)
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index aa5ccd2385ed..e2070d819e04 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -127,6 +127,7 @@ enum bpf_map_type {
> > BPF_MAP_TYPE_SOCKHASH,
> > BPF_MAP_TYPE_CGROUP_STORAGE,
> > BPF_MAP_TYPE_REUSEPORT_SOCKARRAY,
> > + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE,
> > };
> >
> > enum bpf_prog_type {
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index e42f8789b7ea..6502115e8f55 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -206,10 +206,16 @@ BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags)
> > */
> > enum bpf_cgroup_storage_type stype = cgroup_storage_type(map);
> > struct bpf_cgroup_storage *storage;
> > + void *ptr;
> >
> > storage = this_cpu_read(bpf_cgroup_storage[stype]);
> >
> > - return (unsigned long)&READ_ONCE(storage->buf)->data[0];
> > + if (stype == BPF_CGROUP_STORAGE_SHARED)
> > + ptr = &READ_ONCE(storage->buf)->data[0];
> > + else
> > + ptr = this_cpu_ptr(storage->percpu_buf);
> > +
> > + return (unsigned long)ptr;
> > }
> >
> > const struct bpf_func_proto bpf_get_local_storage_proto = {
> > diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> > index 6742292fb39e..c739f6dcc3c2 100644
> > --- a/kernel/bpf/local_storage.c
> > +++ b/kernel/bpf/local_storage.c
> > @@ -152,6 +152,71 @@ static int cgroup_storage_update_elem(struct bpf_map *map, void *_key,
> > return 0;
> > }
> >
> > +int bpf_percpu_cgroup_storage_copy(struct bpf_map *_map, void *_key,
> > + void *value)
> > +{
> > + struct bpf_cgroup_storage_map *map = map_to_storage(_map);
> > + struct bpf_cgroup_storage_key *key = _key;
> > + struct bpf_cgroup_storage *storage;
> > + int cpu, off = 0;
> > + u32 size;
> > +
> > + rcu_read_lock();
> > + storage = cgroup_storage_lookup(map, key, false);
> > + if (!storage) {
> > + rcu_read_unlock();
> > + return -ENOENT;
> > + }
> > +
> > + /* per_cpu areas are zero-filled and bpf programs can only
> > + * access 'value_size' of them, so copying rounded areas
> > + * will not leak any kernel data
> > + */
> > + size = round_up(_map->value_size, 8);
> > + for_each_possible_cpu(cpu) {
> > + bpf_long_memcpy(value + off,
> > + per_cpu_ptr(storage->percpu_buf, cpu), size);
> > + off += size;
> > + }
> > + rcu_read_unlock();
> > + return 0;
> > +}
> > +
> > +int bpf_percpu_cgroup_storage_update(struct bpf_map *_map, void *_key,
> > + void *value, u64 map_flags)
> > +{
> > + struct bpf_cgroup_storage_map *map = map_to_storage(_map);
> > + struct bpf_cgroup_storage_key *key = _key;
> > + struct bpf_cgroup_storage *storage;
> > + int cpu, off = 0;
> > + u32 size;
> > +
> > + if (unlikely(map_flags & BPF_EXIST))
> > + return -EINVAL;
>
> that should have been BPF_NOEXIST ?
Yeah, or maybe even better s/&/!= ?
It's probably better to require BPF_EXIST flag to update a cgroup storage?
Agree? If so, let me fix this for both shared and per-cpu versions in
a follow-up patch.
>
> > +
> > + rcu_read_lock();
> > + storage = cgroup_storage_lookup(map, key, false);
> > + if (!storage) {
> > + rcu_read_unlock();
> > + return -ENOENT;
> > + }
> > +
> > + /* the user space will provide round_up(value_size, 8) bytes that
> > + * will be copied into per-cpu area. bpf programs can only access
> > + * value_size of it. During lookup the same extra bytes will be
> > + * returned or zeros which were zero-filled by percpu_alloc,
> > + * so no kernel data leaks possible
> > + */
> > + size = round_up(_map->value_size, 8);
> > + for_each_possible_cpu(cpu) {
> > + bpf_long_memcpy(per_cpu_ptr(storage->percpu_buf, cpu),
> > + value + off, size);
> > + off += size;
> > + }
> > + rcu_read_unlock();
>
> storage_update and storage_copy look essentially the same
> with the only difference that src/dst swap.
> Would it be possible to combine them ?
> Not sure whether #define template would look better.
I'll try to refactor this and next one in a follow-up patch.
Thanks!
^ permalink raw reply
* Re: [PATCH v3 1/2] netfilter: nf_tables: add SECMARK support
From: Pablo Neira Ayuso @ 2018-09-28 9:01 UTC (permalink / raw)
To: Christian Göttsche
Cc: kadlec, fw, davem, netfilter-devel, coreteam, netdev,
linux-kernel, paul, sds, eparis, jmorris, serge, selinux,
linux-security-module
In-Reply-To: <20180923182616.11398-1-cgzones@googlemail.com>
On Sun, Sep 23, 2018 at 08:26:15PM +0200, Christian Göttsche wrote:
> Add the ability to set the security context of packets within the nf_tables framework.
> Add a nft_object for holding security contexts in the kernel and manipulating packets on the wire.
>
> Convert the security context strings at rule addition time to security identifiers.
> This is the same behavior like in xt_SECMARK and offers better performance than computing it per packet.
>
> Set the maximum security context length to 256.
Applied, thanks Christian.
^ permalink raw reply
* Re: [PATCH v3 bpf-next 10/10] selftests/bpf: cgroup local storage-based network counters
From: Alexei Starovoitov @ 2018-09-28 8:53 UTC (permalink / raw)
To: Roman Gushchin
Cc: netdev, Song Liu, linux-kernel, kernel-team, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20180926113326.29069-11-guro@fb.com>
On Wed, Sep 26, 2018 at 12:33:26PM +0100, Roman Gushchin wrote:
> This commit adds a bpf kselftest, which demonstrates how percpu
> and shared cgroup local storage can be used for efficient lookup-free
> network accounting.
>
> Cgroup local storage provides generic memory area with a very efficient
> lookup free access. To avoid expensive atomic operations for each
> packet, per-cpu cgroup local storage is used. Each packet is initially
> charged to a per-cpu counter, and only if the counter reaches certain
> value (32 in this case), the charge is moved into the global atomic
> counter. This allows to amortize atomic operations, keeping reasonable
> accuracy.
>
> The test also implements a naive network traffic throttling, mostly to
> demonstrate the possibility of bpf cgroup--based network bandwidth
> control.
>
> Expected output:
> ./test_netcnt
> test_netcnt:PASS
>
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Acked-by: Song Liu <songliubraving@fb.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> ---
> tools/testing/selftests/bpf/Makefile | 6 +-
> tools/testing/selftests/bpf/netcnt_common.h | 23 +++
> tools/testing/selftests/bpf/netcnt_prog.c | 71 +++++++++
> tools/testing/selftests/bpf/test_netcnt.c | 153 ++++++++++++++++++++
> 4 files changed, 251 insertions(+), 2 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/netcnt_common.h
> create mode 100644 tools/testing/selftests/bpf/netcnt_prog.c
> create mode 100644 tools/testing/selftests/bpf/test_netcnt.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index fd3851d5c079..5443399dd3a1 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -23,7 +23,8 @@ $(TEST_CUSTOM_PROGS): $(OUTPUT)/%: %.c
> TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
> test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
> test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user \
> - test_socket_cookie test_cgroup_storage test_select_reuseport
> + test_socket_cookie test_cgroup_storage test_select_reuseport \
> + test_netcnt
>
> TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test_obj_id.o \
> test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o \
> @@ -35,7 +36,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
> test_get_stack_rawtp.o test_sockmap_kern.o test_sockhash_kern.o \
> test_lwt_seg6local.o sendmsg4_prog.o sendmsg6_prog.o test_lirc_mode2_kern.o \
> get_cgroup_id_kern.o socket_cookie_prog.o test_select_reuseport_kern.o \
> - test_skb_cgroup_id_kern.o bpf_flow.o
> + test_skb_cgroup_id_kern.o bpf_flow.o netcnt_prog.o
>
> # Order correspond to 'make run_tests' order
> TEST_PROGS := test_kmod.sh \
> @@ -72,6 +73,7 @@ $(OUTPUT)/test_tcpbpf_user: cgroup_helpers.c
> $(OUTPUT)/test_progs: trace_helpers.c
> $(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c
> $(OUTPUT)/test_cgroup_storage: cgroup_helpers.c
> +$(OUTPUT)/test_netcnt: cgroup_helpers.c
>
> .PHONY: force
>
> diff --git a/tools/testing/selftests/bpf/netcnt_common.h b/tools/testing/selftests/bpf/netcnt_common.h
> new file mode 100644
> index 000000000000..0e10fc276c2a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/netcnt_common.h
> @@ -0,0 +1,23 @@
> +#ifndef __NETCNT_COMMON_H
> +#define __NETCNT_COMMON_H
> +
> +#include <linux/types.h>
> +
> +#define MAX_PERCPU_PACKETS 32
> +
> +struct percpu_net_cnt {
> + __u64 packets;
> + __u64 bytes;
> +
> + __u64 prev_ts;
> +
> + __u64 prev_packets;
> + __u64 prev_bytes;
> +};
> +
> +struct net_cnt {
> + __u64 packets;
> + __u64 bytes;
> +};
> +
> +#endif
> diff --git a/tools/testing/selftests/bpf/netcnt_prog.c b/tools/testing/selftests/bpf/netcnt_prog.c
> new file mode 100644
> index 000000000000..1198abca1360
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/netcnt_prog.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <linux/version.h>
> +
> +#include "bpf_helpers.h"
> +#include "netcnt_common.h"
> +
> +#define MAX_BPS (3 * 1024 * 1024)
> +
> +#define REFRESH_TIME_NS 100000000
> +#define NS_PER_SEC 1000000000
> +
> +struct bpf_map_def SEC("maps") percpu_netcnt = {
> + .type = BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE,
> + .key_size = sizeof(struct bpf_cgroup_storage_key),
> + .value_size = sizeof(struct percpu_net_cnt),
> +};
> +
> +struct bpf_map_def SEC("maps") netcnt = {
> + .type = BPF_MAP_TYPE_CGROUP_STORAGE,
> + .key_size = sizeof(struct bpf_cgroup_storage_key),
> + .value_size = sizeof(struct net_cnt),
> +};
> +
> +SEC("cgroup/skb")
> +int bpf_nextcnt(struct __sk_buff *skb)
> +{
> + struct percpu_net_cnt *percpu_cnt;
> + char fmt[] = "%d %llu %llu\n";
> + struct net_cnt *cnt;
> + __u64 ts, dt;
> + int ret;
> +
> + cnt = bpf_get_local_storage(&netcnt, 0);
> + percpu_cnt = bpf_get_local_storage(&percpu_netcnt, 0);
> +
> + percpu_cnt->packets++;
> + percpu_cnt->bytes += skb->len;
> +
> + if (percpu_cnt->packets > MAX_PERCPU_PACKETS) {
> + __sync_fetch_and_add(&cnt->packets,
> + percpu_cnt->packets);
> + percpu_cnt->packets = 0;
> +
> + __sync_fetch_and_add(&cnt->bytes,
> + percpu_cnt->bytes);
> + percpu_cnt->bytes = 0;
> + }
> +
> + ts = bpf_ktime_get_ns();
> + dt = ts - percpu_cnt->prev_ts;
> +
> + dt *= MAX_BPS;
> + dt /= NS_PER_SEC;
> +
> + if (cnt->bytes + percpu_cnt->bytes - percpu_cnt->prev_bytes < dt)
> + ret = 1;
> + else
> + ret = 0;
> +
> + if (dt > REFRESH_TIME_NS) {
> + percpu_cnt->prev_ts = ts;
> + percpu_cnt->prev_packets = cnt->packets;
> + percpu_cnt->prev_bytes = cnt->bytes;
> + }
> +
> + return !!ret;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> +__u32 _version SEC("version") = LINUX_VERSION_CODE;
> diff --git a/tools/testing/selftests/bpf/test_netcnt.c b/tools/testing/selftests/bpf/test_netcnt.c
> new file mode 100644
> index 000000000000..aa424f8db466
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_netcnt.c
> @@ -0,0 +1,153 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <assert.h>
> +#include <sys/sysinfo.h>
> +#include <sys/time.h>
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf.h>
> +#include <bpf/libbpf.h>
> +
> +#include "cgroup_helpers.h"
> +#include "bpf_rlimit.h"
> +#include "netcnt_common.h"
> +
> +#define BPF_PROG "./netcnt_prog.o"
> +#define TEST_CGROUP "/test-network-counters/"
> +
> +static int bpf_find_map(const char *test, struct bpf_object *obj,
> + const char *name)
> +{
> + struct bpf_map *map;
> +
> + map = bpf_object__find_map_by_name(obj, name);
> + if (!map) {
> + printf("%s:FAIL:map '%s' not found\n", test, name);
> + return -1;
> + }
> + return bpf_map__fd(map);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + struct percpu_net_cnt *percpu_netcnt;
> + struct bpf_cgroup_storage_key key;
> + int map_fd, percpu_map_fd;
> + int error = EXIT_FAILURE;
> + struct net_cnt netcnt;
> + struct bpf_object *obj;
> + int prog_fd, cgroup_fd;
> + unsigned long packets;
> + int cpu, nproc;
> + __u32 prog_cnt;
> +
> + nproc = get_nprocs_conf();
> + percpu_netcnt = malloc(sizeof(*percpu_netcnt) * nproc);
> + if (!percpu_netcnt) {
> + printf("Not enough memory for per-cpu area (%d cpus)\n", nproc);
> + goto err;
> + }
> +
> + if (bpf_prog_load(BPF_PROG, BPF_PROG_TYPE_CGROUP_SKB,
> + &obj, &prog_fd)) {
> + printf("Failed to load bpf program\n");
> + goto out;
> + }
> +
> + if (setup_cgroup_environment()) {
> + printf("Failed to load bpf program\n");
> + goto err;
> + }
> +
> + /* Create a cgroup, get fd, and join it */
> + cgroup_fd = create_and_get_cgroup(TEST_CGROUP);
> + if (!cgroup_fd) {
> + printf("Failed to create test cgroup\n");
> + goto err;
> + }
> +
> + if (join_cgroup(TEST_CGROUP)) {
> + printf("Failed to join cgroup\n");
> + goto err;
> + }
> +
> + /* Attach bpf program */
> + if (bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_INET_EGRESS, 0)) {
> + printf("Failed to attach bpf program");
> + goto err;
> + }
> +
> + assert(system("ping localhost -s 500 -c 10000 -f -q > /dev/null") == 0);
> +
> + if (bpf_prog_query(cgroup_fd, BPF_CGROUP_INET_EGRESS, 0, NULL, NULL,
> + &prog_cnt)) {
> + printf("Failed to query attached programs");
> + goto err;
> + }
> +
> + map_fd = bpf_find_map(__func__, obj, "netcnt");
> + if (map_fd < 0) {
> + printf("Failed to find bpf map with net counters");
> + goto err;
> + }
> +
> + percpu_map_fd = bpf_find_map(__func__, obj, "percpu_netcnt");
> + if (percpu_map_fd < 0) {
> + printf("Failed to find bpf map with percpu net counters");
> + goto err;
> + }
> +
> + if (bpf_map_get_next_key(map_fd, NULL, &key)) {
> + printf("Failed to get key in cgroup storage\n");
> + goto err;
> + }
> +
> + if (bpf_map_lookup_elem(map_fd, &key, &netcnt)) {
> + printf("Failed to lookup cgroup storage\n");
> + goto err;
> + }
> +
> + if (bpf_map_lookup_elem(percpu_map_fd, &key, &percpu_netcnt[0])) {
> + printf("Failed to lookup percpu cgroup storage\n");
> + goto err;
> + }
> +
> + /* Some packets can be still in per-cpu cache, but not more than
> + * MAX_PERCPU_PACKETS.
> + */
> + packets = netcnt.packets;
> + for (cpu = 0; cpu < nproc; cpu++) {
> + if (percpu_netcnt[cpu].packets > 32) {
pls use MAX_PERCPU_PACKETS in the above check.
could you also double check that if that #define is changed to 1k or so
the exact "!= 10000" check below still works as expected?
> + printf("Unexpected percpu value: %llu\n",
> + percpu_netcnt[cpu].packets);
> + goto err;
> + }
> +
> + packets += percpu_netcnt[cpu].packets;
> + }
> +
> + /* No packets should be lost */
> + if (packets != 10000) {
> + printf("Unexpected packet count: %lu\n", packets);
> + goto err;
> + }
> +
> + /* Let's check that bytes counter value is reasonable */
> + if (netcnt.bytes < packets * 500 || netcnt.bytes > packets * 1500) {
since packet count is accurate why byte count would vary ?
> + printf("Unexpected bytes count: %llu\n", netcnt.bytes);
> + goto err;
> + }
> +
> + error = 0;
> + printf("test_netcnt:PASS\n");
> +
> +err:
> + cleanup_cgroup_environment();
> + free(percpu_netcnt);
> +
> +out:
> + return error;
> +}
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH net-next v6 01/23] asm: simd context helper API
From: Ard Biesheuvel @ 2018-09-28 8:49 UTC (permalink / raw)
To: Jason A. Donenfeld, Joe Perches
Cc: Linux Kernel Mailing List, <netdev@vger.kernel.org>,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, David S. Miller,
Greg Kroah-Hartman, Samuel Neves, Andy Lutomirski,
Thomas Gleixner, linux-arch
In-Reply-To: <CAKv+Gu9e35+AyktmSq9qeNE0LR83_yrEEB3DiQv0bmyArivqRQ@mail.gmail.com>
(+ Joe)
On 28 September 2018 at 10:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 25 September 2018 at 16:56, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> Sometimes it's useful to amortize calls to XSAVE/XRSTOR and the related
>> FPU/SIMD functions over a number of calls, because FPU restoration is
>> quite expensive. This adds a simple header for carrying out this pattern:
>>
>> simd_context_t simd_context;
>>
>> simd_get(&simd_context);
>> while ((item = get_item_from_queue()) != NULL) {
>> encrypt_item(item, simd_context);
>> simd_relax(&simd_context);
>> }
>> simd_put(&simd_context);
>>
>> The relaxation step ensures that we don't trample over preemption, and
>> the get/put API should be a familiar paradigm in the kernel.
>>
>> On the other end, code that actually wants to use SIMD instructions can
>> accept this as a parameter and check it via:
>>
>> void encrypt_item(struct item *item, simd_context_t *simd_context)
>> {
>> if (item->len > LARGE_FOR_SIMD && simd_use(simd_context))
>> wild_simd_code(item);
>> else
>> boring_scalar_code(item);
>> }
>>
>> The actual XSAVE happens during simd_use (and only on the first time),
>> so that if the context is never actually used, no performance penalty is
>> hit.
>>
>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>> Cc: Samuel Neves <sneves@dei.uc.pt>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Greg KH <gregkh@linuxfoundation.org>
>> Cc: linux-arch@vger.kernel.org
>> ---
>> arch/alpha/include/asm/Kbuild | 5 ++-
>> arch/arc/include/asm/Kbuild | 1 +
>> arch/arm/include/asm/simd.h | 63 ++++++++++++++++++++++++++++++
>> arch/arm64/include/asm/simd.h | 51 +++++++++++++++++++++---
>> arch/c6x/include/asm/Kbuild | 3 +-
>> arch/h8300/include/asm/Kbuild | 3 +-
>> arch/hexagon/include/asm/Kbuild | 1 +
>> arch/ia64/include/asm/Kbuild | 1 +
>> arch/m68k/include/asm/Kbuild | 1 +
>> arch/microblaze/include/asm/Kbuild | 1 +
>> arch/mips/include/asm/Kbuild | 1 +
>> arch/nds32/include/asm/Kbuild | 7 ++--
>> arch/nios2/include/asm/Kbuild | 1 +
>> arch/openrisc/include/asm/Kbuild | 7 ++--
>> arch/parisc/include/asm/Kbuild | 1 +
>> arch/powerpc/include/asm/Kbuild | 3 +-
>> arch/riscv/include/asm/Kbuild | 3 +-
>> arch/s390/include/asm/Kbuild | 3 +-
>> arch/sh/include/asm/Kbuild | 1 +
>> arch/sparc/include/asm/Kbuild | 1 +
>> arch/um/include/asm/Kbuild | 3 +-
>> arch/unicore32/include/asm/Kbuild | 1 +
>> arch/x86/include/asm/simd.h | 44 ++++++++++++++++++++-
>> arch/xtensa/include/asm/Kbuild | 1 +
>> include/asm-generic/simd.h | 20 ++++++++++
>> include/linux/simd.h | 28 +++++++++++++
>> 26 files changed, 234 insertions(+), 21 deletions(-)
>> create mode 100644 arch/arm/include/asm/simd.h
>> create mode 100644 include/linux/simd.h
>>
>> diff --git a/arch/alpha/include/asm/Kbuild b/arch/alpha/include/asm/Kbuild
>> index 0580cb8c84b2..07b2c1025d34 100644
>> --- a/arch/alpha/include/asm/Kbuild
>> +++ b/arch/alpha/include/asm/Kbuild
>> @@ -2,14 +2,15 @@
>>
>>
>> generic-y += compat.h
>> +generic-y += current.h
>> generic-y += exec.h
>> generic-y += export.h
>> generic-y += fb.h
>> generic-y += irq_work.h
>> +generic-y += kprobes.h
>> generic-y += mcs_spinlock.h
>> generic-y += mm-arch-hooks.h
>> generic-y += preempt.h
>> generic-y += sections.h
>> +generic-y += simd.h
>> generic-y += trace_clock.h
>> -generic-y += current.h
>> -generic-y += kprobes.h
>
> Given that this patch applies to all architectures at once, it is
> probably better to drop the unrelated reordering hunks to avoid
> conflicts.
>
>> diff --git a/arch/arc/include/asm/Kbuild b/arch/arc/include/asm/Kbuild
>> index feed50ce89fa..a7f4255f1649 100644
>> --- a/arch/arc/include/asm/Kbuild
>> +++ b/arch/arc/include/asm/Kbuild
>> @@ -22,6 +22,7 @@ generic-y += parport.h
>> generic-y += pci.h
>> generic-y += percpu.h
>> generic-y += preempt.h
>> +generic-y += simd.h
>> generic-y += topology.h
>> generic-y += trace_clock.h
>> generic-y += user.h
>> diff --git a/arch/arm/include/asm/simd.h b/arch/arm/include/asm/simd.h
>> new file mode 100644
>> index 000000000000..263950dd69cb
>> --- /dev/null
>> +++ b/arch/arm/include/asm/simd.h
>> @@ -0,0 +1,63 @@
>> +/* SPDX-License-Identifier: GPL-2.0
>> + *
>> + * Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
>> + */
>> +
>> +#include <linux/simd.h>
>> +#ifndef _ASM_SIMD_H
>> +#define _ASM_SIMD_H
>> +
>> +#ifdef CONFIG_KERNEL_MODE_NEON
>> +#include <asm/neon.h>
>> +
>> +static __must_check inline bool may_use_simd(void)
>> +{
>> + return !in_interrupt();
>> +}
>> +
>
> Remember this guy?
>
> https://marc.info/?l=linux-arch&m=149631094625176&w=2
>
> That was never merged, so let's get it right this time.
>
...
>> diff --git a/include/linux/simd.h b/include/linux/simd.h
>> new file mode 100644
>> index 000000000000..33bba21012ff
>> --- /dev/null
>> +++ b/include/linux/simd.h
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: GPL-2.0
>> + *
>> + * Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
>> + */
>> +
>> +#ifndef _SIMD_H
>> +#define _SIMD_H
>> +
>> +typedef enum {
>> + HAVE_NO_SIMD = 1 << 0,
>> + HAVE_FULL_SIMD = 1 << 1,
>> + HAVE_SIMD_IN_USE = 1 << 31
>> +} simd_context_t;
>> +
Oh, and another thing (and I'm surprised checkpatch.pl didn't complain
about it): the use of typedef in new code is strongly discouraged.
This policy predates my involvement, so perhaps Joe can elaborate on
the rationale?
>> +#include <linux/sched.h>
>> +#include <asm/simd.h>
>> +
>> +static inline void simd_relax(simd_context_t *ctx)
>> +{
>> +#ifdef CONFIG_PREEMPT
>> + if ((*ctx & HAVE_SIMD_IN_USE) && need_resched()) {
>> + simd_put(ctx);
>> + simd_get(ctx);
>> + }
>> +#endif
>
> Could we return a bool here indicating whether we rescheduled or not?
> In some cases, we could pass that into the asm code as a 'reload'
> param, allowing repeated loads of key schedules, round constant tables
> or S-boxes to be elided.
>
>> +}
>> +
>> +#endif /* _SIMD_H */
>> --
>> 2.19.0
>>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 03/10] bpf: introduce per-cpu cgroup local storage
From: Alexei Starovoitov @ 2018-09-28 8:45 UTC (permalink / raw)
To: Roman Gushchin
Cc: netdev, Song Liu, linux-kernel, kernel-team, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20180926113326.29069-4-guro@fb.com>
On Wed, Sep 26, 2018 at 12:33:19PM +0100, Roman Gushchin wrote:
> This commit introduced per-cpu cgroup local storage.
>
> Per-cpu cgroup local storage is very similar to simple cgroup storage
> (let's call it shared), except all the data is per-cpu.
>
> The main goal of per-cpu variant is to implement super fast
> counters (e.g. packet counters), which don't require neither
> lookups, neither atomic operations.
>
> From userspace's point of view, accessing a per-cpu cgroup storage
> is similar to other per-cpu map types (e.g. per-cpu hashmaps and
> arrays).
>
> Writing to a per-cpu cgroup storage is not atomic, but is performed
> by copying longs, so some minimal atomicity is here, exactly
> as with other per-cpu maps.
>
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> ---
> include/linux/bpf-cgroup.h | 20 ++++-
> include/linux/bpf.h | 1 +
> include/linux/bpf_types.h | 1 +
> include/uapi/linux/bpf.h | 1 +
> kernel/bpf/helpers.c | 8 +-
> kernel/bpf/local_storage.c | 148 ++++++++++++++++++++++++++++++++-----
> kernel/bpf/syscall.c | 11 ++-
> kernel/bpf/verifier.c | 15 +++-
> 8 files changed, 177 insertions(+), 28 deletions(-)
>
> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> index 7e0c9a1d48b7..588dd5f0bd85 100644
> --- a/include/linux/bpf-cgroup.h
> +++ b/include/linux/bpf-cgroup.h
> @@ -37,7 +37,10 @@ struct bpf_storage_buffer {
> };
>
> struct bpf_cgroup_storage {
> - struct bpf_storage_buffer *buf;
> + union {
> + struct bpf_storage_buffer *buf;
> + void __percpu *percpu_buf;
> + };
> struct bpf_cgroup_storage_map *map;
> struct bpf_cgroup_storage_key key;
> struct list_head list;
> @@ -109,6 +112,9 @@ int __cgroup_bpf_check_dev_permission(short dev_type, u32 major, u32 minor,
> static inline enum bpf_cgroup_storage_type cgroup_storage_type(
> struct bpf_map *map)
> {
> + if (map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> + return BPF_CGROUP_STORAGE_PERCPU;
> +
> return BPF_CGROUP_STORAGE_SHARED;
> }
>
> @@ -131,6 +137,10 @@ void bpf_cgroup_storage_unlink(struct bpf_cgroup_storage *storage);
> int bpf_cgroup_storage_assign(struct bpf_prog *prog, struct bpf_map *map);
> void bpf_cgroup_storage_release(struct bpf_prog *prog, struct bpf_map *map);
>
> +int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key, void *value);
> +int bpf_percpu_cgroup_storage_update(struct bpf_map *map, void *key,
> + void *value, u64 flags);
> +
> /* Wrappers for __cgroup_bpf_run_filter_skb() guarded by cgroup_bpf_enabled. */
> #define BPF_CGROUP_RUN_PROG_INET_INGRESS(sk, skb) \
> ({ \
> @@ -285,6 +295,14 @@ static inline struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(
> struct bpf_prog *prog, enum bpf_cgroup_storage_type stype) { return 0; }
> static inline void bpf_cgroup_storage_free(
> struct bpf_cgroup_storage *storage) {}
> +static inline int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key,
> + void *value) {
> + return 0;
> +}
> +static inline int bpf_percpu_cgroup_storage_update(struct bpf_map *map,
> + void *key, void *value, u64 flags) {
> + return 0;
> +}
>
> #define cgroup_bpf_enabled (0)
> #define BPF_CGROUP_PRE_CONNECT_ENABLED(sk) (0)
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b457fbe7b70b..018299a595c8 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -274,6 +274,7 @@ struct bpf_prog_offload {
>
> enum bpf_cgroup_storage_type {
> BPF_CGROUP_STORAGE_SHARED,
> + BPF_CGROUP_STORAGE_PERCPU,
> __BPF_CGROUP_STORAGE_MAX
> };
>
> diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> index c9bd6fb765b0..5432f4c9f50e 100644
> --- a/include/linux/bpf_types.h
> +++ b/include/linux/bpf_types.h
> @@ -43,6 +43,7 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_ARRAY, cgroup_array_map_ops)
> #endif
> #ifdef CONFIG_CGROUP_BPF
> BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_STORAGE, cgroup_storage_map_ops)
> +BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE, cgroup_storage_map_ops)
> #endif
> BPF_MAP_TYPE(BPF_MAP_TYPE_HASH, htab_map_ops)
> BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_HASH, htab_percpu_map_ops)
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index aa5ccd2385ed..e2070d819e04 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -127,6 +127,7 @@ enum bpf_map_type {
> BPF_MAP_TYPE_SOCKHASH,
> BPF_MAP_TYPE_CGROUP_STORAGE,
> BPF_MAP_TYPE_REUSEPORT_SOCKARRAY,
> + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE,
> };
>
> enum bpf_prog_type {
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index e42f8789b7ea..6502115e8f55 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -206,10 +206,16 @@ BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags)
> */
> enum bpf_cgroup_storage_type stype = cgroup_storage_type(map);
> struct bpf_cgroup_storage *storage;
> + void *ptr;
>
> storage = this_cpu_read(bpf_cgroup_storage[stype]);
>
> - return (unsigned long)&READ_ONCE(storage->buf)->data[0];
> + if (stype == BPF_CGROUP_STORAGE_SHARED)
> + ptr = &READ_ONCE(storage->buf)->data[0];
> + else
> + ptr = this_cpu_ptr(storage->percpu_buf);
> +
> + return (unsigned long)ptr;
> }
>
> const struct bpf_func_proto bpf_get_local_storage_proto = {
> diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> index 6742292fb39e..c739f6dcc3c2 100644
> --- a/kernel/bpf/local_storage.c
> +++ b/kernel/bpf/local_storage.c
> @@ -152,6 +152,71 @@ static int cgroup_storage_update_elem(struct bpf_map *map, void *_key,
> return 0;
> }
>
> +int bpf_percpu_cgroup_storage_copy(struct bpf_map *_map, void *_key,
> + void *value)
> +{
> + struct bpf_cgroup_storage_map *map = map_to_storage(_map);
> + struct bpf_cgroup_storage_key *key = _key;
> + struct bpf_cgroup_storage *storage;
> + int cpu, off = 0;
> + u32 size;
> +
> + rcu_read_lock();
> + storage = cgroup_storage_lookup(map, key, false);
> + if (!storage) {
> + rcu_read_unlock();
> + return -ENOENT;
> + }
> +
> + /* per_cpu areas are zero-filled and bpf programs can only
> + * access 'value_size' of them, so copying rounded areas
> + * will not leak any kernel data
> + */
> + size = round_up(_map->value_size, 8);
> + for_each_possible_cpu(cpu) {
> + bpf_long_memcpy(value + off,
> + per_cpu_ptr(storage->percpu_buf, cpu), size);
> + off += size;
> + }
> + rcu_read_unlock();
> + return 0;
> +}
> +
> +int bpf_percpu_cgroup_storage_update(struct bpf_map *_map, void *_key,
> + void *value, u64 map_flags)
> +{
> + struct bpf_cgroup_storage_map *map = map_to_storage(_map);
> + struct bpf_cgroup_storage_key *key = _key;
> + struct bpf_cgroup_storage *storage;
> + int cpu, off = 0;
> + u32 size;
> +
> + if (unlikely(map_flags & BPF_EXIST))
> + return -EINVAL;
that should have been BPF_NOEXIST ?
> +
> + rcu_read_lock();
> + storage = cgroup_storage_lookup(map, key, false);
> + if (!storage) {
> + rcu_read_unlock();
> + return -ENOENT;
> + }
> +
> + /* the user space will provide round_up(value_size, 8) bytes that
> + * will be copied into per-cpu area. bpf programs can only access
> + * value_size of it. During lookup the same extra bytes will be
> + * returned or zeros which were zero-filled by percpu_alloc,
> + * so no kernel data leaks possible
> + */
> + size = round_up(_map->value_size, 8);
> + for_each_possible_cpu(cpu) {
> + bpf_long_memcpy(per_cpu_ptr(storage->percpu_buf, cpu),
> + value + off, size);
> + off += size;
> + }
> + rcu_read_unlock();
storage_update and storage_copy look essentially the same
with the only difference that src/dst swap.
Would it be possible to combine them ?
Not sure whether #define template would look better.
> + return 0;
> +}
> +
> static int cgroup_storage_get_next_key(struct bpf_map *_map, void *_key,
> void *_next_key)
> {
> @@ -292,55 +357,98 @@ struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(struct bpf_prog *prog,
> {
> struct bpf_cgroup_storage *storage;
> struct bpf_map *map;
> + gfp_t flags;
> + size_t size;
> u32 pages;
>
> map = prog->aux->cgroup_storage[stype];
> if (!map)
> return NULL;
>
> - pages = round_up(sizeof(struct bpf_cgroup_storage) +
> - sizeof(struct bpf_storage_buffer) +
> - map->value_size, PAGE_SIZE) >> PAGE_SHIFT;
> + if (stype == BPF_CGROUP_STORAGE_SHARED) {
> + size = sizeof(struct bpf_storage_buffer) + map->value_size;
> + pages = round_up(sizeof(struct bpf_cgroup_storage) + size,
> + PAGE_SIZE) >> PAGE_SHIFT;
> + } else {
> + size = map->value_size;
> + pages = round_up(round_up(size, 8) * num_possible_cpus(),
> + PAGE_SIZE) >> PAGE_SHIFT;
> + }
> +
> if (bpf_map_charge_memlock(map, pages))
> return ERR_PTR(-EPERM);
>
> storage = kmalloc_node(sizeof(struct bpf_cgroup_storage),
> __GFP_ZERO | GFP_USER, map->numa_node);
> - if (!storage) {
> - bpf_map_uncharge_memlock(map, pages);
> - return ERR_PTR(-ENOMEM);
> - }
> + if (!storage)
> + goto enomem;
>
> - storage->buf = kmalloc_node(sizeof(struct bpf_storage_buffer) +
> - map->value_size, __GFP_ZERO | GFP_USER,
> - map->numa_node);
> - if (!storage->buf) {
> - bpf_map_uncharge_memlock(map, pages);
> - kfree(storage);
> - return ERR_PTR(-ENOMEM);
> + flags = __GFP_ZERO | GFP_USER;
> +
> + if (stype == BPF_CGROUP_STORAGE_SHARED) {
> + storage->buf = kmalloc_node(size, flags, map->numa_node);
> + if (!storage->buf)
> + goto enomem;
> + } else {
> + storage->percpu_buf = __alloc_percpu_gfp(size, 8, flags);
> + if (!storage->percpu_buf)
> + goto enomem;
> }
>
> storage->map = (struct bpf_cgroup_storage_map *)map;
>
> return storage;
> +
> +enomem:
> + bpf_map_uncharge_memlock(map, pages);
> + kfree(storage);
> + return ERR_PTR(-ENOMEM);
> +}
> +
> +static void free_shared_cgroup_storage_rcu(struct rcu_head *rcu)
> +{
> + struct bpf_cgroup_storage *storage =
> + container_of(rcu, struct bpf_cgroup_storage, rcu);
> +
> + kfree(storage->buf);
> + kfree(storage);
> +}
> +
> +static void free_percpu_cgroup_storage_rcu(struct rcu_head *rcu)
> +{
> + struct bpf_cgroup_storage *storage =
> + container_of(rcu, struct bpf_cgroup_storage, rcu);
> +
> + free_percpu(storage->percpu_buf);
> + kfree(storage);
> }
>
> void bpf_cgroup_storage_free(struct bpf_cgroup_storage *storage)
> {
> - u32 pages;
> + enum bpf_cgroup_storage_type stype;
> struct bpf_map *map;
> + u32 pages;
>
> if (!storage)
> return;
>
> map = &storage->map->map;
> - pages = round_up(sizeof(struct bpf_cgroup_storage) +
> - sizeof(struct bpf_storage_buffer) +
> - map->value_size, PAGE_SIZE) >> PAGE_SHIFT;
> + stype = cgroup_storage_type(map);
> + if (stype == BPF_CGROUP_STORAGE_SHARED)
> + pages = round_up(sizeof(struct bpf_cgroup_storage) +
> + sizeof(struct bpf_storage_buffer) +
> + map->value_size, PAGE_SIZE) >> PAGE_SHIFT;
> + else
> + pages = round_up(round_up(map->value_size, 8) *
> + num_possible_cpus(),
> + PAGE_SIZE) >> PAGE_SHIFT;
storage_alloc and storage_free have subttly different math here.
Please combine them into single helper that computes the number of pages.
In the current form it's hard to tell that the math is actually the same.
^ permalink raw reply
* Re: KASAN: global-out-of-bounds Read in __aa_lookupn_ns
From: Dmitry Vyukov @ 2018-09-28 8:39 UTC (permalink / raw)
To: syzbot, John Johansen, James Morris, Serge E. Hallyn,
linux-security-module
Cc: David Miller, LKML, netdev, syzkaller-bugs
In-Reply-To: <CACT4Y+YAbon-m=N-UcEVfGj8DxgRAySQz7qLYQWFxQZqv+3JhA@mail.gmail.com>
On Wed, Sep 26, 2018 at 12:06 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, Sep 26, 2018 at 9:54 AM, syzbot
> <syzbot+71b6643475f707f93fdc@syzkaller.appspotmail.com> wrote:
>> Hello,
>>
>> syzbot found the following crash on:
>>
>> HEAD commit: 02214bfc89c7 Merge tag 'media/v4.19-2' of git://git.kernel..
>> git tree: upstream
>> console output: https://syzkaller.appspot.com/x/log.txt?x=1456f8a1400000
>> kernel config: https://syzkaller.appspot.com/x/.config?x=22a62640793a83c9
>> dashboard link: https://syzkaller.appspot.com/bug?extid=71b6643475f707f93fdc
>> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>>
>> Unfortunately, I don't have any reproducer for this crash yet.
>
> Again misattributed to net. This misattribution should now be fixed by:
> https://github.com/google/syzkaller/commit/db716d6653d073b0abfb51186cd4ac2d5418c9c6
> Adding security/apparmor/policy_ns.c maintainers explicitly.
This is the same as "KASAN: stack-out-of-bounds Read in __aa_lookupn_ns":
https://syzkaller.appspot.com/bug?id=f0d603856d8b3cc9b8e09228f2f548c18ef907ac
right?
>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> Reported-by: syzbot+71b6643475f707f93fdc@syzkaller.appspotmail.com
>>
>> sock_common_setsockopt+0x9a/0xe0 net/core/sock.c:3038
>> __sys_setsockopt+0x1ba/0x3c0 net/socket.c:1902
>> __do_sys_setsockopt net/socket.c:1913 [inline]
>> __se_sys_setsockopt net/socket.c:1910 [inline]
>> __x64_sys_setsockopt+0xbe/0x150 net/socket.c:1910
>> do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
>> ==================================================================
>> BUG: KASAN: global-out-of-bounds in memcmp+0xe3/0x160 lib/string.c:861
>> Read of size 1 at addr ffffffff88000008 by task syz-executor0/10914
>>
>> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> RIP: 0033:0x457579
>> Code: 1d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
>> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff
>> 0f 83 eb b3 fb ff c3 66 2e 0f 1f 84 00 00 00 00
>> RSP: 002b:00007f4c14533c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000036
>> RAX: ffffffffffffffda RBX: 00007f4c14533c90 RCX: 0000000000457579
>> RDX: 0000000000000040 RSI: 0000000000000000 RDI: 0000000000000003
>> RBP: 000000000072bf00 R08: 0000000000000004 R09: 0000000000000000
>> R10: 0000000020000080 R11: 0000000000000246 R12: 00007f4c145346d4
>> R13: 00000000004c3ed9 R14: 00000000004d6260 R15: 0000000000000004
>> CPU: 0 PID: 10914 Comm: syz-executor0 Not tainted 4.19.0-rc5+ #252
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
>> Google 01/01/2011
>> Call Trace:
>> __dump_stack lib/dump_stack.c:77 [inline]
>> dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113
>> print_address_description.cold.8+0x58/0x1ff mm/kasan/report.c:256
>> kasan_report_error mm/kasan/report.c:354 [inline]
>> kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412
>> __asan_report_load1_noabort+0x14/0x20 mm/kasan/report.c:430
>> memcmp+0xe3/0x160 lib/string.c:861
>> strnstr+0x4b/0x70 lib/string.c:934
>> __aa_lookupn_ns+0xc1/0x570 security/apparmor/policy_ns.c:209
>> aa_lookupn_ns+0x88/0x1e0 security/apparmor/policy_ns.c:240
>> aa_fqlookupn_profile+0x1b9/0x1010 security/apparmor/policy.c:468
>> fqlookupn_profile+0x80/0xc0 security/apparmor/label.c:1844
>> aa_label_strn_parse+0xa3a/0x1230 security/apparmor/label.c:1908
>> aa_label_parse+0x42/0x50 security/apparmor/label.c:1943
>> aa_change_profile+0x513/0x3510 security/apparmor/domain.c:1362
>> apparmor_setprocattr+0xa8b/0x1150 security/apparmor/lsm.c:656
>> security_setprocattr+0x66/0xc0 security/security.c:1298
>> proc_pid_attr_write+0x301/0x540 fs/proc/base.c:2555
>> __vfs_write+0x119/0x9f0 fs/read_write.c:485
>> vfs_write+0x1fc/0x560 fs/read_write.c:549
>> ksys_write+0x101/0x260 fs/read_write.c:598
>> __do_sys_write fs/read_write.c:610 [inline]
>> __se_sys_write fs/read_write.c:607 [inline]
>> __x64_sys_write+0x73/0xb0 fs/read_write.c:607
>> do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
>> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> RIP: 0033:0x457579
>> Code: 1d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
>> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff
>> 0f 83 eb b3 fb ff c3 66 2e 0f 1f 84 00 00 00 00
>> RSP: 002b:00007f5a92ec2c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
>> RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000457579
>> RDX: 000000000000002c RSI: 00000000200000c0 RDI: 0000000000000004
>> RBP: 000000000072bf00 R08: 0000000000000000 R09: 0000000000000000
>> R10: 0000000000000000 R11: 0000000000000246 R12: 00007f5a92ec36d4
>> R13: 00000000004c5454 R14: 00000000004d8c78 R15: 00000000ffffffff
>>
>> CPU: 1 PID: 10921 Comm: syz-executor3 Not tainted 4.19.0-rc5+ #252
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
>> Google 01/01/2011
>> The buggy address belongs to the variable:
>> __start_rodata+0x8/0x1000
>> Call Trace:
>> __dump_stack lib/dump_stack.c:77 [inline]
>> dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113
>>
>> Memory state around the buggy address:
>> ffffffff87ffff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> ffffffff87ffff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> fail_dump lib/fault-inject.c:51 [inline]
>> should_fail.cold.4+0xa/0x17 lib/fault-inject.c:149
>>>
>>> ffffffff88000000: 00 fa fa fa fa fa fa fa 00 01 fa fa fa fa fa fa
>>
>> ^
>> ffffffff88000080: 00 00 00 07 fa fa fa fa 00 04 fa fa fa fa fa fa
>> ffffffff88000100: 05 fa fa fa fa fa fa fa 00 00 00 00 05 fa fa fa
>> ==================================================================
>>
>>
>> ---
>> This bug is generated by a bot. It may contain errors.
>> See https://goo.gl/tpsmEJ for more information about syzbot.
>> syzbot engineers can be reached at syzkaller@googlegroups.com.
>>
>> syzbot will keep track of this bug report. See:
>> https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
>> syzbot.
^ permalink raw reply
* Re: [Patch net-next] net_sched: fix an extack message in tcf_block_find()
From: David Ahern @ 2018-09-28 1:51 UTC (permalink / raw)
To: Cong Wang, Eric Dumazet
Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim,
Vlad Buslov
In-Reply-To: <CAM_iQpX0sxzHT4aiaATPDHRnxWb-x1f_WhV9BQ58GNYZ=DUPHA@mail.gmail.com>
On 9/27/18 3:36 PM, Cong Wang wrote:
> On Thu, Sep 27, 2018 at 2:16 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>
>>
>> On 09/27/2018 01:42 PM, Cong Wang wrote:
>>> It is clearly a copy-n-paste.
>>>
>>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>> ---
>>> net/sched/cls_api.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>> index 3de47e99b788..8dd7f8af6d54 100644
>>> --- a/net/sched/cls_api.c
>>> +++ b/net/sched/cls_api.c
>>> @@ -655,7 +655,7 @@ static struct tcf_block *tcf_block_find(struct net *net, struct Qdisc **q,
>>>
>>> *q = qdisc_refcount_inc_nz(*q);
>>> if (!*q) {
>>> - NL_SET_ERR_MSG(extack, "Parent Qdisc doesn't exists");
>>> + NL_SET_ERR_MSG(extack, "Can't increase Qdisc refcount");
>>
>>
>> I am not sure it was a copy-n-paste.
>
>
> Make sure you knew there is an exactly same extack message
> (with a same English grammar error).
>
>
>>
>> Qdisc refcount business is kernel internal.
>
> Yeah, but the extack message is already there, this patch doesn't add
> any new extack. Or you are suggesting we should remove it?
IMO the message grammar should be fixed, but the content is correct --
ie, parent qdisc does not exist.
^ permalink raw reply
* RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
From: Y.b. Lu @ 2018-09-28 8:04 UTC (permalink / raw)
To: Andrew Lunn
Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
netdev@vger.kernel.org, Richard Cochran, David S . Miller,
Ioana Ciocoi Radulescu, Greg Kroah-Hartman
In-Reply-To: <20180927132507.GB23375@lunn.ch>
Hi Andrew,
Thanks a lot for your comments.
Please see my comments inline.
Best regards,
Yangbo Lu
> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, September 27, 2018 9:25 PM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: linux-kernel@vger.kernel.org; devel@driverdev.osuosl.org;
> netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>;
> David S . Miller <davem@davemloft.net>; Ioana Ciocoi Radulescu
> <ruxandra.radulescu@nxp.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>
> Subject: Re: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
>
> On Thu, Sep 27, 2018 at 07:12:27PM +0800, Yangbo Lu wrote:
> > This patch is to move DPAA2 PTP driver out of staging/ since the
> > dpaa2-eth had been moved out.
> >
> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > ---
> > drivers/net/ethernet/freescale/Kconfig | 9 +--------
> > drivers/net/ethernet/freescale/dpaa2/Kconfig | 15
> +++++++++++++++
> > drivers/net/ethernet/freescale/dpaa2/Makefile | 6 ++++--
> > .../ethernet/freescale/dpaa2}/dprtc-cmd.h | 0
> > .../rtc => net/ethernet/freescale/dpaa2}/dprtc.c | 0
> > .../rtc => net/ethernet/freescale/dpaa2}/dprtc.h | 0
> > .../rtc => net/ethernet/freescale/dpaa2}/rtc.c | 0
> > .../rtc => net/ethernet/freescale/dpaa2}/rtc.h | 0
> > drivers/staging/fsl-dpaa2/Kconfig | 8 --------
> > drivers/staging/fsl-dpaa2/Makefile | 1 -
> > drivers/staging/fsl-dpaa2/rtc/Makefile | 7 -------
> > 11 files changed, 20 insertions(+), 26 deletions(-) create mode
> > 100644 drivers/net/ethernet/freescale/dpaa2/Kconfig
> > rename drivers/{staging/fsl-dpaa2/rtc =>
> > net/ethernet/freescale/dpaa2}/dprtc-cmd.h (100%) rename
> > drivers/{staging/fsl-dpaa2/rtc =>
> > net/ethernet/freescale/dpaa2}/dprtc.c (100%) rename
> > drivers/{staging/fsl-dpaa2/rtc =>
> > net/ethernet/freescale/dpaa2}/dprtc.h (100%) rename
> > drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/rtc.c
> > (100%) rename drivers/{staging/fsl-dpaa2/rtc =>
> > net/ethernet/freescale/dpaa2}/rtc.h (100%)
>
> Hi Yangbo
>
> Calling a ptp driver rtc.[ch] seems rather odd. Could you fixup the name,
> change it to ptp.[ch]. Also, some of the function names, and structures,
> rtc_probe->ptp_probe, rtc_remove->ptp_remove, rtc_match_id_table->
> ptp_match_id_table, etc.
[Y.b. Lu] Indeed, it's odd and confusing...
For dpaa2, all hardware resources are allocated and configured through the Management Complex (MC) portals.
MC abstracts most of these resources as DPAA2 objects and exposes ABIs through which they can be configured and controlled.
This ptp timer was named as rtc in MC firmware and APIs as you saw in dprtc.*.
So initially I wrote this driver using rtc as name.
No worries, let me change it in next version.
>
> ptp_dpaa2_adjfreq() probably should return err, not 0.
> ptp_dpaa2_gettime() again does not return the error.
> If fact, it seems like all the main functions ignore errors.
[Y.b. Lu] Will fix the returns in next version.
>
> kzalloc() could be changed to devm_kzalloc() to simplify the cleanup
[Y.b. Lu] Will use devm_kzalloc() in next version.
Can
> ptp_dpaa2_caps be made const?
[Y.b. Lu] Yes. Will change it in next version.
> dpaa2_phc_index does not appear to be used.
[Y.b. Lu] It's used in dpaa2-ethtool.c for .get_ts_info interface of ethtool_ops.
> dev_set_drvdata(dev, NULL); is not needed.
[Y.b. Lu] Will remove it in next version.
> Can rtc_drv be made const?
[Y.b. Lu] Will use const in next version.
> Is rtc.h used by anything other than rtc.c? It seems like it can be removed.
[Y.b. Lu] Let me remove it in next version.
>
> It seems like there is a lot of code in dprtc.c which is unused. rtc.c does nothing
> with interrupts for example. Do you plan to make use of this extra code? Or
> can it be removed leaving just what is needed?
[Y.b. Lu] Currently the ptp/rtc driver is not full-featured. The extra code is being planed to be used.
>
> struct dprtc_cmd_get_irq - Putting pad at the beginning of a struct seems very
> odd. And it is not the only example.
[Y.b. Lu] This should depended on MC firmware and APIs I think. Once the MC improves this, the APIs could be updated to fix this.
>
> Andrew
^ permalink raw reply
* Re: [PATCH net-next v6 00/23] WireGuard: Secure Network Tunnel
From: Ard Biesheuvel @ 2018-09-28 7:52 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Eric Biggers, LKML, Netdev, Linux Crypto Mailing List,
David Miller, Greg Kroah-Hartman
In-Reply-To: <CAHmME9rJ4CewfDEqWh00ZowjWg9TaYpksVSzKSSxoKya-M8_LA@mail.gmail.com>
On 28 September 2018 at 07:46, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Hi Eric,
>
> On Fri, Sep 28, 2018 at 6:55 AM Eric Biggers <ebiggers@kernel.org> wrote:
>> And you still haven't answered my question about adding a new algorithm that is
>> useful to have in both APIs. You're proposing that in most cases the crypto API
>> part will need to go through Herbert while the implementation will need to go
>> through you/Greg, right? Or will you/Greg be taking both? Or will Herbert take
>> both?
>
> If an implementation enters Zinc, it will go through my tree. If it
> enters the crypto API, it will go through Herbert's tree. If there
> wind up being messy tree dependency and merge timing issues, I can
> work this out in the usual way with Herbert. I'll be sure to discuss
> these edge cases with him when we discuss. I think there's a way to
> handle that with minimum friction.
>
I would also strongly prefer that all crypto work is taken through
Herbert's tree, so we have a coherent view of it before it goes
upstream.
>> A documentation file for Zinc is a good idea. Some of the information in your
>> commit messages should be moved there too.
>
> Will do.
>
>> I'm not trying to "politicize" this, but rather determine your criteria for
>> including algorithms in Zinc, which you haven't made clear. Since for the
>> second time you've avoided answering my question about whether you'd allow the
>> SM4 cipher in Zinc, and you designed WireGuard to be "cryptographically
>> opinionated", and you were one of the loudest voices calling for the Speck
>> cipher to be removed, and your justification for Zinc complains about cipher
>> modes from "90s cryptographers", I think it's reasonable for people to wonder
>> whether as the Zinc (i.e. Linux crypto library) maintainer you will restrict the
>> inclusion of crypto algorithms to the ones you prefer, rather than the ones that
>> users need. Note that the kernel is used by people all over the world and needs
>> to support various standards, protocols, and APIs that use different crypto
>> algorithms, not always the ones we'd like; and different users have different
>> preferences. People need to know whether the Linux kernel's crypto library will
>> meet (or be allowed to meet) their crypto needs.
>
> WireGuard is indeed quite opinionated in its primitive choices, but I
> don't think it'd be wise to apply the same design to Zinc. There are
> APIs where the goal is to have a limited set of high-level functions,
> and have those implemented in only a preferred set of primitives. NaCl
> is a good example of this -- functions like "crypto_secretbox" that
> are actually xsalsapoly under the hood. Zinc doesn't intend to become
> an API like those, but rather to provide the actual primitives for use
> cases where that specific primitive is used. Sometimes the kernel is
> in the business of being able to pick its own crypto -- random.c, tcp
> sequence numbers, big_key.c, etc -- but most of the time the kernel is
> in the business of implementing other people's crypto, for specific
> devices/protocols/diskformats. And for those use cases we need not
> some high-level API like NaCl, but rather direct access to the
> primitives that are required to implement those drivers. WireGuard is
> one such example, but so would be Bluetooth, CIFS/SMB, WiFi, and so
> on. We're in the business of writing drivers, after all. So, no, I
> don't think I'd knock down the addition of a primitive because of a
> simple preference for a different primitive, if it was clearly the
> case that the driver requiring it really benefited from having
> accessible via the plain Zinc function calls. Sorry if I hadn't made
> this clear earlier -- I thought Ard had asked more or less the same
> thing about DES and I answered accordingly, but maybe that wasn't made
> clear enough there.
>
>> > For example, check out the avx blocks function. The radix conversion
>> > happens in a few different places throughout. The reason we need it
>> > separately here is because, unlike userspace, it's possible the kernel
>> > code will transition from 2^26 back to 2^64 as a result of the FPU
>> > context changing.
>>
>> IOW, you had to rewrite the x86 assembly algorithm in C and make it use a
>> different Poly1305 context format. That's a major change, where bugs can be
>> introduced -- and at least one was introduced, in fact. I'd appreciate it if
>> you were more accurate in describing your modifications and the potential risks
>> involved.
>
> A different Poly1305 context format? Not at all - it's using the exact
> same context struct as the assembly. And it's making the same
> conversion that the assembly is. There's not something "different"
> happening; that's the whole point.
>
> Also, this is not some process of frightfully transcribing assembly to
> C and hoping it all works out. This is a careful process of reasoning
> about the limbs, proving that the carries are correct, and that the
> arithmetic done in C actually corresponds to what we want. And then
> finally we check that what we've implemented is indeed the same as
> what the assembly implemented. Finally, as I mentioned, hopefully Andy
> will be folding this back into his implementations sometime soon
> anyway.
>
>> > That's a good idea. I can include some discussion about this as well in
>> > the commit message that introduces the glue code, too, I guess? I've
>> > been hesitant to fill these commit messages up even more, given there
>> > are already so many walls of text and whatnot, but if you think that'd
>> > be useful, I'll do that for v7, and also add comments.
>>
>> Please prefer to put information in the code or documentation rather than in
>> commit messages, when appropriate.
>
> Okay, no problem.
>
>> > This is complete and utter garbage, and I find its insinuations insulting
>> > and ridiculous. There is absolutely no lack of honesty and no double
>> > standard being applied whatsoever. Your attempt to cast doubt about the
>> > quality of standards applied and the integrity of the process is wholly
>> > inappropriate. When I tell you that high standards were applied and that
>> > due-diligence was done in developing a particular patch, I mean what I
>> > say.
>>
>> I
>> disagree that my concerns are "complete and utter garbage". And I think that
>> the fact that you prefer to respond by attacking me, rather than committing to
>> be more accurate in your claims and to treat all contributions fairly, is
>> problematic.
>
> I believe you have the sequence of events wrong. I've stated and made
> that there isn't a double standard, that the standards intend to be
> evenly rigorous, and I solicited your feedback on how I could best
> communicate changes in pre-merged patch series; I did the things
> you've said one should do. But your rhetoric then moved to questioning
> my integrity, and I believe that was uncalled for, inappropriate, and
> not a useful contribution to this mostly productive discussion --
> hence garbage. In other words, I provided an acceptable defense, not
> an attack. But can we move past all this schoolyard nonsense? Cut the
> rhetoric, please; it's really quite overwhelming.
>
>> It's great that you found and fixed this
>> bug on your own, and of course that raises my level of confidence in your work.
>
> Good.
>
>> Still, the v6 patchset still includes your claim that "All implementations have
>> been extensively tested and fuzzed"; so that claim was objectively wrong.
>
> I don't think that claim is wrong. The fuzzing and testing that's been
> done has been extensive, and the fuzzing and testing that continues to
> occur is extensive. As mentioned, the bug was fixed pretty much right
> after git-send-email. If you'd like I can try to space out the patch
> submissions by a little bit longer -- that would probably have various
> benefits -- but given that the netdev code is yet to receive a
> thorough review, I think we've got a bit of time before this is
> merged. The faster-paced patch cycles might inadvertently introduce
> things that get fixed immediately after sending then, unfortunately,
> but I don't think this will be the case with the final series that's
> merged. Though I'm incorporating tons and tons of feedback right now,
> I do look forward to the structure of the series settling down a
> little bit and the pace of suggestions decreasing, so that I can focus
> on auditing and verifying again. But given the dynamism and
> interesting insights of the reviews so far, I think the fast pace has
> actually been useful for elucidating the various expectations and
> suggestions of reviewers. It's most certainly improved this patchset
> in terrific ways.
>
>> Well, it doesn't help that you yourself claim that Zinc stands for
>> "Zx2c4's INsane Cryptolib"...
>
> This expansion of the acronym was intended as a totally ridiculous
> joke. I guess it wasn't received well. I'll remove it from the next
> series. Sorry.
>
As I understood from someone who was at your Kernel Recipes talk, you
mentioned that it actually stands for 'zinc is not crypto/' (note the
slash)
That is needlessly divisive and condescending, so if you want to move
past the rhetoric and the schoolyard nonsense, perhaps you can find a
better name for it? Or just put your stuff into crypto/base/,
crypto/core/ or something like that.
^ permalink raw reply
* Re: [PATCH net] net/ncsi: Extend NC-SI Netlink interface to allow user space to send NC-SI command
From: Samuel Mendoza-Jonas @ 2018-09-28 1:26 UTC (permalink / raw)
To: Justin.Lee1, joel
Cc: amithash, vijaykhemka, linux-aspeed, openbmc, sdasari, netdev,
christian
In-Reply-To: <a2f68c5eb785436ea248cb46b73ea246@AUSX13MPS306.AMER.DELL.COM>
On Thu, 2018-09-27 at 21:08 +0000, Justin.Lee1@Dell.com wrote:
> The new command (NCSI_CMD_SEND_CMD) is added to allow user space application
> to send NC-SI command to the network card.
> Also, add a new attribute (NCSI_ATTR_DATA) for transferring request and response.
>
> The work flow is as below.
>
> Request:
> User space application -> Netlink interface (msg)
> -> new Netlink handler - ncsi_send_cmd_nl()
> -> ncsi_xmit_cmd()
> Response:
> Response received - ncsi_rcv_rsp() -> internal response handler - ncsi_rsp_handler_xxx()
> -> ncsi_rsp_handler_netlink()
> -> ncsi_send_netlink_rsp ()
> -> Netlink interface (msg)
> -> user space application
> Command timeout - ncsi_request_timeout() -> ncsi_send_netlink_timeout ()
> -> Netlink interface (msg with zero data length)
> -> user space application
> Error:
> Error detected -> ncsi_send_netlink_err () -> Netlink interface (err msg)
> -> user space application
>
>
> Signed-off-by: Justin Lee <justin.lee1@dell.com>
>
Hi Justin,
Thanks for posting this on the list! The overall design looks good and so
far looks like it should fit relatively well with the other OEM command
patch. I'll try and run some OEM commands against my machine.
Some comments below:
>
> ---
> include/uapi/linux/ncsi.h | 3 +
> net/ncsi/internal.h | 12 ++-
> net/ncsi/ncsi-aen.c | 10 ++-
> net/ncsi/ncsi-cmd.c | 106 ++++++++++++++++--------
> net/ncsi/ncsi-manage.c | 74 ++++++++++++++---
> net/ncsi/ncsi-netlink.c | 199 +++++++++++++++++++++++++++++++++++++++++++++-
> net/ncsi/ncsi-netlink.h | 4 +
> net/ncsi/ncsi-rsp.c | 70 ++++++++++++++--
> 8 files changed, 420 insertions(+), 58 deletions(-)
>
> diff --git a/include/uapi/linux/ncsi.h b/include/uapi/linux/ncsi.h
> index 4c292ec..4992bfc 100644
> --- a/include/uapi/linux/ncsi.h
> +++ b/include/uapi/linux/ncsi.h
> @@ -30,6 +30,7 @@ enum ncsi_nl_commands {
> NCSI_CMD_PKG_INFO,
> NCSI_CMD_SET_INTERFACE,
> NCSI_CMD_CLEAR_INTERFACE,
> + NCSI_CMD_SEND_CMD,
>
> __NCSI_CMD_AFTER_LAST,
> NCSI_CMD_MAX = __NCSI_CMD_AFTER_LAST - 1
> @@ -43,6 +44,7 @@ enum ncsi_nl_commands {
> * @NCSI_ATTR_PACKAGE_LIST: nested array of NCSI_PKG_ATTR attributes
> * @NCSI_ATTR_PACKAGE_ID: package ID
> * @NCSI_ATTR_CHANNEL_ID: channel ID
> + * @NCSI_ATTR_DATA: command payload
> * @NCSI_ATTR_MAX: highest attribute number
> */
> enum ncsi_nl_attrs {
> @@ -51,6 +53,7 @@ enum ncsi_nl_attrs {
> NCSI_ATTR_PACKAGE_LIST,
> NCSI_ATTR_PACKAGE_ID,
> NCSI_ATTR_CHANNEL_ID,
> + NCSI_ATTR_DATA,
>
> __NCSI_ATTR_AFTER_LAST,
> NCSI_ATTR_MAX = __NCSI_ATTR_AFTER_LAST - 1
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index 8055e39..20ce735 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -215,12 +215,17 @@ struct ncsi_request {
> unsigned char id; /* Request ID - 0 to 255 */
> bool used; /* Request that has been assigned */
> unsigned int flags; /* NCSI request property */
> -#define NCSI_REQ_FLAG_EVENT_DRIVEN 1
> +#define NCSI_REQ_FLAG_EVENT_DRIVEN 1
> +#define NCSI_REQ_FLAG_NETLINK_DRIVEN 2
> struct ncsi_dev_priv *ndp; /* Associated NCSI device */
> struct sk_buff *cmd; /* Associated NCSI command packet */
> struct sk_buff *rsp; /* Associated NCSI response packet */
> struct timer_list timer; /* Timer on waiting for response */
> bool enabled; /* Time has been enabled or not */
> +
> + u32 snd_seq; /* netlink sending sequence number */
> + u32 snd_portid; /* netlink portid of sender */
> + struct nlmsghdr nlhdr; /* netlink message header */
> };
>
> enum {
> @@ -301,10 +306,13 @@ struct ncsi_cmd_arg {
> unsigned short payload; /* Command packet payload length */
> unsigned int req_flags; /* NCSI request properties */
> union {
> - unsigned char bytes[16]; /* Command packet specific data */
> + unsigned char bytes[16]; /* Command packet specific data */
> unsigned short words[8];
> unsigned int dwords[4];
> };
> +
> + unsigned char *data; /* Netlink data */
> + struct genl_info *info; /* Netlink information */
> };
>
> extern struct list_head ncsi_dev_list;
> diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> index 25e483e..b5ec193 100644
> --- a/net/ncsi/ncsi-aen.c
> +++ b/net/ncsi/ncsi-aen.c
> @@ -16,6 +16,7 @@
> #include <net/ncsi.h>
> #include <net/net_namespace.h>
> #include <net/sock.h>
> +#include <net/genetlink.h>
>
> #include "internal.h"
> #include "ncsi-pkt.h"
> @@ -73,8 +74,8 @@ static int ncsi_aen_handler_lsc(struct ncsi_dev_priv *ndp,
> ncm->data[2] = data;
> ncm->data[4] = ntohl(lsc->oem_status);
>
> - netdev_dbg(ndp->ndev.dev, "NCSI: LSC AEN - channel %u state %s\n",
> - nc->id, data & 0x1 ? "up" : "down");
> + netdev_dbg(ndp->ndev.dev, "NCSI: LSC AEN - pkg %u ch %u state %s\n",
> + nc->package->id, nc->id, data & 0x1 ? "up" : "down");
There's a few places where you've changed or added some debug statements;
these are good but could probably be in a separate patch since not all of
them are related to the OEM command handling.
>
> chained = !list_empty(&nc->link);
> state = nc->state;
> @@ -148,9 +149,10 @@ static int ncsi_aen_handler_hncdsc(struct ncsi_dev_priv *ndp,
> hncdsc = (struct ncsi_aen_hncdsc_pkt *)h;
> ncm->data[3] = ntohl(hncdsc->status);
> spin_unlock_irqrestore(&nc->lock, flags);
> +
> netdev_dbg(ndp->ndev.dev,
> - "NCSI: host driver %srunning on channel %u\n",
> - ncm->data[3] & 0x1 ? "" : "not ", nc->id);
> + "NCSI: host driver %srunning on pkg %u ch %u\n",
> + ncm->data[3] & 0x1 ? "" : "not ", nc->package->id, nc->id);
>
> return 0;
> }
> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> index 7567ca63..b291297 100644
> --- a/net/ncsi/ncsi-cmd.c
> +++ b/net/ncsi/ncsi-cmd.c
> @@ -17,6 +17,7 @@
> #include <net/ncsi.h>
> #include <net/net_namespace.h>
> #include <net/sock.h>
> +#include <net/genetlink.h>
>
> #include "internal.h"
> #include "ncsi-pkt.h"
> @@ -211,42 +212,75 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
> return 0;
> }
>
> +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> + struct ncsi_cmd_arg *nca)
> +{
> + struct ncsi_cmd_pkt *cmd;
> + unsigned char *dest, *source;
> + unsigned short len;
> +
> + /* struct ncsi_cmd_pkt = minimum length
> + * - frame checksum
> + * - Ethernet header
> + * = 64 - 4 - 14 = 46
> + * minimum payload = 46 - ncsi header - ncsi checksum
> + * = 46 - 16 - 4 = 26
> + */
> + len = nca->payload;
> +
> + /* minimum payload length is 26 bytes to meet minimum packet
> + * length 64
> + */
> + if (len < 26)
> + cmd = skb_put_zero(skb, sizeof(*cmd));
> + else
> + cmd = skb_put_zero(skb, len + sizeof(struct ncsi_pkt_hdr) + 4);
> +
> + dest = (unsigned char *)cmd + sizeof(struct ncsi_pkt_hdr);
> + source = (unsigned char *)nca->data + sizeof(struct ncsi_pkt_hdr);
> + memcpy(dest, source, len);
> +
> + ncsi_cmd_build_header(&cmd->cmd.common, nca);
> +
> + return 0;
> +}
This is quite similar to the other patch which is good, they shouldn't
conflict much.
> +
> static struct ncsi_cmd_handler {
> unsigned char type;
> int payload;
> int (*handler)(struct sk_buff *skb,
> struct ncsi_cmd_arg *nca);
> } ncsi_cmd_handlers[] = {
> - { NCSI_PKT_CMD_CIS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_SP, 4, ncsi_cmd_handler_sp },
> - { NCSI_PKT_CMD_DP, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_EC, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_DC, 4, ncsi_cmd_handler_dc },
> - { NCSI_PKT_CMD_RC, 4, ncsi_cmd_handler_rc },
> - { NCSI_PKT_CMD_ECNT, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_DCNT, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_AE, 8, ncsi_cmd_handler_ae },
> - { NCSI_PKT_CMD_SL, 8, ncsi_cmd_handler_sl },
> - { NCSI_PKT_CMD_GLS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_SVF, 8, ncsi_cmd_handler_svf },
> - { NCSI_PKT_CMD_EV, 4, ncsi_cmd_handler_ev },
> - { NCSI_PKT_CMD_DV, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_SMA, 8, ncsi_cmd_handler_sma },
> - { NCSI_PKT_CMD_EBF, 4, ncsi_cmd_handler_ebf },
> - { NCSI_PKT_CMD_DBF, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_EGMF, 4, ncsi_cmd_handler_egmf },
> - { NCSI_PKT_CMD_DGMF, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_SNFC, 4, ncsi_cmd_handler_snfc },
> - { NCSI_PKT_CMD_GVI, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_GC, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_GP, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_GCPS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_GNS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_GPS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_OEM, 0, NULL },
> - { NCSI_PKT_CMD_PLDM, 0, NULL },
> - { NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
> + { NCSI_PKT_CMD_CIS, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_SP, 4, ncsi_cmd_handler_sp },
> + { NCSI_PKT_CMD_DP, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_EC, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_DC, 4, ncsi_cmd_handler_dc },
> + { NCSI_PKT_CMD_RC, 4, ncsi_cmd_handler_rc },
> + { NCSI_PKT_CMD_ECNT, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_DCNT, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_AE, 8, ncsi_cmd_handler_ae },
> + { NCSI_PKT_CMD_SL, 8, ncsi_cmd_handler_sl },
> + { NCSI_PKT_CMD_GLS, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_SVF, 8, ncsi_cmd_handler_svf },
> + { NCSI_PKT_CMD_EV, 4, ncsi_cmd_handler_ev },
> + { NCSI_PKT_CMD_DV, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_SMA, 8, ncsi_cmd_handler_sma },
> + { NCSI_PKT_CMD_EBF, 4, ncsi_cmd_handler_ebf },
> + { NCSI_PKT_CMD_DBF, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_EGMF, 4, ncsi_cmd_handler_egmf },
> + { NCSI_PKT_CMD_DGMF, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_SNFC, 4, ncsi_cmd_handler_snfc },
> + { NCSI_PKT_CMD_GVI, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_GC, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_GP, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_GCPS, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_GNS, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_GPS, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_OEM, -1, ncsi_cmd_handler_oem },
> + { NCSI_PKT_CMD_PLDM, 0, NULL },
> + { NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
> };
You've changed the whitespace here which has made the diff unnecessarily
large; please just change the single _OEM line and keep the whitespace
the same.
>
> static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca)
> @@ -317,11 +351,20 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
> }
>
> /* Get packet payload length and allocate the request */
> - nca->payload = nch->payload;
> + if (nch->payload >= 0)
> + nca->payload = nch->payload;
> +
> nr = ncsi_alloc_command(nca);
> if (!nr)
> return -ENOMEM;
>
> + /* track netlink information */
> + if (nca->req_flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
> + nr->snd_seq = nca->info->snd_seq;
> + nr->snd_portid = nca->info->snd_portid;
> + nr->nlhdr = *nca->info->nlhdr;
> + }
> +
> /* Prepare the packet */
> nca->id = nr->id;
> ret = nch->handler(nr->cmd, nca);
> @@ -341,6 +384,7 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
> * connection a 1 second delay should be sufficient.
> */
> nr->enabled = true;
> +
> mod_timer(&nr->timer, jiffies + 1 * HZ);
>
> /* Send NCSI packet */
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 0912847..6629103 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -19,6 +19,7 @@
> #include <net/addrconf.h>
> #include <net/ipv6.h>
> #include <net/if_inet6.h>
> +#include <net/genetlink.h>
>
> #include "internal.h"
> #include "ncsi-pkt.h"
> @@ -110,8 +111,9 @@ static void ncsi_channel_monitor(struct timer_list *t)
> case NCSI_CHANNEL_MONITOR_WAIT ... NCSI_CHANNEL_MONITOR_WAIT_MAX:
> break;
> default:
> - netdev_err(ndp->ndev.dev, "NCSI Channel %d timed out!\n",
> - nc->id);
> + netdev_err(ndp->ndev.dev, "NCSI: pkg %u ch %u timed out!\n",
> + np->id, nc->id);
> +
> if (!(ndp->flags & NCSI_DEV_HWA)) {
> ncsi_report_link(ndp, true);
> ndp->flags |= NCSI_DEV_RESHUFFLE;
> @@ -143,6 +145,10 @@ void ncsi_start_channel_monitor(struct ncsi_channel *nc)
> {
> unsigned long flags;
>
> + netdev_dbg(nc->package->ndp->ndev.dev,
> + "NCSI: %s pkg %u ch %u\n",
> + __func__, nc->package->id, nc->id);
> +
> spin_lock_irqsave(&nc->lock, flags);
> WARN_ON_ONCE(nc->monitor.enabled);
> nc->monitor.enabled = true;
> @@ -156,6 +162,10 @@ void ncsi_stop_channel_monitor(struct ncsi_channel *nc)
> {
> unsigned long flags;
>
> + netdev_dbg(nc->package->ndp->ndev.dev,
> + "NCSI: %s pkg %u ch %u\n",
> + __func__, nc->package->id, nc->id);
> +
> spin_lock_irqsave(&nc->lock, flags);
> if (!nc->monitor.enabled) {
> spin_unlock_irqrestore(&nc->lock, flags);
> @@ -406,8 +416,13 @@ static void ncsi_request_timeout(struct timer_list *t)
> {
> struct ncsi_request *nr = from_timer(nr, t, timer);
> struct ncsi_dev_priv *ndp = nr->ndp;
> + struct ncsi_package *np;
> + struct ncsi_channel *nc;
> + struct ncsi_cmd_pkt *cmd;
> unsigned long flags;
>
> + netdev_dbg(ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> /* If the request already had associated response,
> * let the response handler to release it.
> */
> @@ -415,10 +430,23 @@ static void ncsi_request_timeout(struct timer_list *t)
> nr->enabled = false;
> if (nr->rsp || !nr->cmd) {
> spin_unlock_irqrestore(&ndp->lock, flags);
> +
> + netdev_dbg(ndp->ndev.dev, "NCSI: %s - early return\n", __func__);
> +
> return;
> }
> spin_unlock_irqrestore(&ndp->lock, flags);
>
> + if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
> + if (nr->cmd) {
> + /* Find the package */
> + cmd = (struct ncsi_cmd_pkt *)skb_network_header(nr->cmd);
> + ncsi_find_package_and_channel(ndp, cmd->cmd.common.channel,
> + &np, &nc);
> + ncsi_send_netlink_timeout(nr, np, nc);
> + }
> + }
> +
> /* Release the request */
> ncsi_free_request(nr);
> }
> @@ -432,6 +460,10 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
> unsigned long flags;
> int ret;
>
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: %s pkg %u ch %u state %04x\n",
> + __func__, np->id, nc->id, nd->state);
> +
> nca.ndp = ndp;
> nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> switch (nd->state) {
> @@ -647,6 +679,10 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> unsigned long flags;
> int ret;
>
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: %s pkg %u ch %u state %04x\n",
> + __func__, np->id, nc->id, nd->state);
> +
> nca.ndp = ndp;
> nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> switch (nd->state) {
> @@ -788,8 +824,9 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> }
> break;
> case ncsi_dev_state_config_done:
> - netdev_dbg(ndp->ndev.dev, "NCSI: channel %u config done\n",
> - nc->id);
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: pkg %u ch %u config done\n",
> + nc->package->id, nc->id);
> spin_lock_irqsave(&nc->lock, flags);
> if (nc->reconfigure_needed) {
> /* This channel's configuration has been updated
> @@ -815,9 +852,10 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> } else {
> hot_nc = NULL;
> nc->state = NCSI_CHANNEL_INACTIVE;
> +
> netdev_dbg(ndp->ndev.dev,
> - "NCSI: channel %u link down after config\n",
> - nc->id);
> + "NCSI: pkg %u ch %u link down after config\n",
> + nc->package->id, nc->id);
> }
> spin_unlock_irqrestore(&nc->lock, flags);
>
> @@ -853,6 +891,8 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
> force_package = ndp->force_package;
> spin_unlock_irqrestore(&ndp->lock, flags);
>
> + netdev_dbg(ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> /* Force a specific channel whether or not it has link if we have been
> * configured to do so
> */
> @@ -861,8 +901,8 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
> ncm = &found->modes[NCSI_MODE_LINK];
> if (!(ncm->data[2] & 0x1))
> netdev_info(ndp->ndev.dev,
> - "NCSI: Channel %u forced, but it is link down\n",
> - found->id);
> + "NCSI: pkg %u ch %u forced, but it is link down\n",
> + found->package->id, found->id);
> goto out;
> }
>
> @@ -914,6 +954,7 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
> out:
> spin_lock_irqsave(&ndp->lock, flags);
> list_add_tail_rcu(&found->link, &ndp->channel_queue);
> +
> spin_unlock_irqrestore(&ndp->lock, flags);
>
> return ncsi_process_next_channel(ndp);
> @@ -1154,6 +1195,8 @@ static void ncsi_dev_work(struct work_struct *work)
> struct ncsi_dev_priv, work);
> struct ncsi_dev *nd = &ndp->ndev;
>
> + netdev_dbg(ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> switch (nd->state & ncsi_dev_state_major) {
> case ncsi_dev_state_probe:
> ncsi_probe_channel(ndp);
> @@ -1176,6 +1219,8 @@ int ncsi_process_next_channel(struct ncsi_dev_priv *ndp)
> int old_state;
> unsigned long flags;
>
> + netdev_dbg(ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> spin_lock_irqsave(&ndp->lock, flags);
> nc = list_first_or_null_rcu(&ndp->channel_queue,
> struct ncsi_channel, link);
> @@ -1198,14 +1243,14 @@ int ncsi_process_next_channel(struct ncsi_dev_priv *ndp)
> switch (old_state) {
> case NCSI_CHANNEL_INACTIVE:
> ndp->ndev.state = ncsi_dev_state_config;
> - netdev_dbg(ndp->ndev.dev, "NCSI: configuring channel %u\n",
> - nc->id);
> + netdev_dbg(ndp->ndev.dev, "NCSI: configuring pkg %u ch %u\n",
> + nc->package->id, nc->id);
> ncsi_configure_channel(ndp);
> break;
> case NCSI_CHANNEL_ACTIVE:
> ndp->ndev.state = ncsi_dev_state_suspend;
> - netdev_dbg(ndp->ndev.dev, "NCSI: suspending channel %u\n",
> - nc->id);
> + netdev_dbg(ndp->ndev.dev, "NCSI: suspending pkg %u ch %u\n",
> + nc->package->id, nc->id);
> ncsi_suspend_channel(ndp);
> break;
> default:
> @@ -1225,6 +1270,9 @@ int ncsi_process_next_channel(struct ncsi_dev_priv *ndp)
> return ncsi_choose_active_channel(ndp);
> }
>
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: No more channels to process\n");
> +
> ncsi_report_link(ndp, false);
> return -ENODEV;
> }
> @@ -1475,6 +1523,7 @@ struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
> if (list_empty(&ncsi_dev_list))
> register_inet6addr_notifier(&ncsi_inet6addr_notifier);
> #endif
> +
> list_add_tail_rcu(&ndp->node, &ncsi_dev_list);
> spin_unlock_irqrestore(&ncsi_dev_lock, flags);
>
> @@ -1564,6 +1613,7 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)
> if (list_empty(&ncsi_dev_list))
> unregister_inet6addr_notifier(&ncsi_inet6addr_notifier);
> #endif
> +
> spin_unlock_irqrestore(&ncsi_dev_lock, flags);
>
> ncsi_unregister_netlink(nd->dev);
> diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
> index 45f33d6..ab1a959 100644
> --- a/net/ncsi/ncsi-netlink.c
> +++ b/net/ncsi/ncsi-netlink.c
> @@ -20,6 +20,7 @@
> #include <uapi/linux/ncsi.h>
>
> #include "internal.h"
> +#include "ncsi-pkt.h"
> #include "ncsi-netlink.h"
>
> static struct genl_family ncsi_genl_family;
> @@ -29,6 +30,7 @@ static const struct nla_policy ncsi_genl_policy[NCSI_ATTR_MAX + 1] = {
> [NCSI_ATTR_PACKAGE_LIST] = { .type = NLA_NESTED },
> [NCSI_ATTR_PACKAGE_ID] = { .type = NLA_U32 },
> [NCSI_ATTR_CHANNEL_ID] = { .type = NLA_U32 },
> + [NCSI_ATTR_DATA] = { .type = NLA_BINARY, .len = 2048 },
> };
Is there a particular reason for setting len to 2048, or just a decent
buffer?
>
> static struct ncsi_dev_priv *ndp_from_ifindex(struct net *net, u32 ifindex)
> @@ -240,7 +242,7 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
> return 0; /* done */
>
> hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
> - &ncsi_genl_family, NLM_F_MULTI, NCSI_CMD_PKG_INFO);
> + &ncsi_genl_family, NLM_F_MULTI, NCSI_CMD_PKG_INFO);
> if (!hdr) {
> rc = -EMSGSIZE;
> goto err;
> @@ -316,8 +318,8 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info)
> * package
> */
> spin_unlock_irqrestore(&ndp->lock, flags);
> - netdev_info(ndp->ndev.dev, "NCSI: Channel %u does not exist!\n",
> - channel_id);
> + netdev_info(ndp->ndev.dev, "NCSI: pkg %u ch %u does not exist!\n",
> + package_id, channel_id);
> return -ERANGE;
> }
>
> @@ -366,6 +368,191 @@ static int ncsi_clear_interface_nl(struct sk_buff *msg, struct genl_info *info)
> return 0;
> }
>
> +static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
> +{
> + struct ncsi_dev_priv *ndp;
> +
> + struct ncsi_cmd_arg nca;
> + struct ncsi_pkt_hdr *hdr;
> +
> + u32 package_id, channel_id;
> + unsigned char *data;
> + void *head;
> + int len, ret;
> +
> + if (!info || !info->attrs) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (!info->attrs[NCSI_ATTR_IFINDEX]) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (!info->attrs[NCSI_ATTR_PACKAGE_ID]) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (!info->attrs[NCSI_ATTR_CHANNEL_ID]) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
> + nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
> + if (!ndp) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]);
> + channel_id = nla_get_u32(info->attrs[NCSI_ATTR_CHANNEL_ID]);
> +
> + if ((package_id & ~0x07) || (channel_id & ~0x1f)) {
> + ret = -ERANGE;
> + goto out_netlink;
> + }
This is correct but hard to read; we could instead just have
if ((package_id >= 8) ...
which is easier to interpret.
(Probably we should also define the max packages/channels somewhere too)
> +
> + len = nla_len(info->attrs[NCSI_ATTR_DATA]);
> + if (len < sizeof(struct ncsi_pkt_hdr)) {
> + netdev_info(ndp->ndev.dev, "NCSI: no OEM command to send %u\n",
> + package_id);
For statements like these follow the netdev format, eg:
netdev_info(ndp->ndev.dev, "NCSI: no OEM command to send %u\n",
package_id);
> + ret = -EINVAL;
> + goto out_netlink;
> + } else {
> + head = nla_data(info->attrs[NCSI_ATTR_DATA]);
> + data = (unsigned char *)head;
> + }
> +
> + hdr = (struct ncsi_pkt_hdr *)data;
> +
> + nca.ndp = ndp;
> + nca.package = (unsigned char)package_id;
> + nca.channel = (unsigned char)channel_id;
> + nca.type = hdr->type;
> + nca.req_flags = NCSI_REQ_FLAG_NETLINK_DRIVEN;
> + nca.info = info;
> + nca.payload = ntohs(hdr->length);
> + nca.data = data;
> +
> + ret = ncsi_xmit_cmd(&nca);
> +out_netlink:
> + if (ret != 0) {
> + netdev_err(ndp->ndev.dev, "Error %d sending OEM command\n", ret);
> + ncsi_send_netlink_err(ndp->ndev.dev,
> + info->snd_seq, info->snd_portid, info->nlhdr,
> + ret);
> + }
> +out:
> + return ret;
> +}
> +
> +int ncsi_send_netlink_rsp(struct ncsi_request *nr, struct ncsi_package *np, struct ncsi_channel *nc)
> +{
> + struct sk_buff *skb;
> + struct net *net;
> + void *hdr;
> + int rc;
> +
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> + net = dev_net(nr->rsp->dev);
> +
> + skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> + if (!skb)
> + return -ENOMEM;
> +
> + hdr = genlmsg_put(skb, nr->snd_portid, nr->snd_seq,
> + &ncsi_genl_family, 0, NCSI_CMD_SEND_CMD);
> + if (!hdr) {
> + kfree_skb(skb);
> + return -EMSGSIZE;
> + }
> +
> + nla_put_u32(skb, NCSI_ATTR_IFINDEX, nr->rsp->dev->ifindex);
> + if (np)
> + nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID, np->id);
> + if (nc)
> + nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, nc->id);
> + else
> + nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, NCSI_RESERVED_CHANNEL);
> +
> + rc = nla_put(skb, NCSI_ATTR_DATA, nr->rsp->len, (void *)nr->rsp->data);
> + if (rc)
> + goto err;
> +
> + genlmsg_end(skb, hdr);
> + return genlmsg_unicast(net, skb, nr->snd_portid);
> +
> +err:
> + kfree_skb(skb);
> + return rc;
> +}
> +
> +int ncsi_send_netlink_timeout(struct ncsi_request *nr, struct ncsi_package *np, struct ncsi_channel *nc)
> +{
> + struct sk_buff *skb;
> + struct net *net;
> + void *hdr;
> +
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> + skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> + if (!skb)
> + return -ENOMEM;
> +
> + hdr = genlmsg_put(skb, nr->snd_portid, nr->snd_seq,
> + &ncsi_genl_family, 0, NCSI_CMD_SEND_CMD);
> + if (!hdr) {
> + kfree_skb(skb);
> + return -EMSGSIZE;
> + }
> +
> + net = dev_net(nr->cmd->dev);
> +
> + nla_put_u32(skb, NCSI_ATTR_IFINDEX, nr->cmd->dev->ifindex);
> +
> + if (np)
> + nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID, np->id);
> + else
> + nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID,
> + NCSI_PACKAGE_INDEX((((struct ncsi_pkt_hdr *)nr->cmd->data)->channel)));
> +
> + if (nc)
> + nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, nc->id);
> + else
> + nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, NCSI_RESERVED_CHANNEL);
> +
> + genlmsg_end(skb, hdr);
> + return genlmsg_unicast(net, skb, nr->snd_portid);
How does the receiver of this message know it represents a timeout? Just that
it's missing the NCSI_ATTR_DATA attribute?
> +}
> +
> +int ncsi_send_netlink_err(struct net_device *dev, u32 snd_seq, u32 snd_portid, struct nlmsghdr *nlhdr, int err)
> +{
> + struct sk_buff *skb;
> + struct nlmsghdr *nlh;
> + struct nlmsgerr *nle;
> + struct net *net;
> +
> + skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> + if (!skb)
> + return -ENOMEM;
> +
> + net = dev_net(dev);
> +
> + nlh = nlmsg_put(skb, snd_portid, snd_seq,
> + NLMSG_ERROR, sizeof(*nle), 0);
> + nle = (struct nlmsgerr *)nlmsg_data(nlh);
> + nle->error = err;
> + memcpy(&nle->msg, nlhdr, sizeof(*nlh));
> +
> + nlmsg_end(skb, nlh);
> +
> + return nlmsg_unicast(net->genl_sock, skb, snd_portid);
> +}
> +
> static const struct genl_ops ncsi_ops[] = {
> {
> .cmd = NCSI_CMD_PKG_INFO,
> @@ -386,6 +573,12 @@ static const struct genl_ops ncsi_ops[] = {
> .doit = ncsi_clear_interface_nl,
> .flags = GENL_ADMIN_PERM,
> },
> + {
> + .cmd = NCSI_CMD_SEND_CMD,
> + .policy = ncsi_genl_policy,
> + .doit = ncsi_send_cmd_nl,
> + .flags = GENL_ADMIN_PERM,
> + },
> };
>
> static struct genl_family ncsi_genl_family __ro_after_init = {
> diff --git a/net/ncsi/ncsi-netlink.h b/net/ncsi/ncsi-netlink.h
> index 91a5c25..dadaf32 100644
> --- a/net/ncsi/ncsi-netlink.h
> +++ b/net/ncsi/ncsi-netlink.h
> @@ -14,6 +14,10 @@
>
> #include "internal.h"
>
> +int ncsi_send_netlink_rsp(struct ncsi_request *nr, struct ncsi_package *np, struct ncsi_channel *nc);
> +int ncsi_send_netlink_timeout(struct ncsi_request *nr, struct ncsi_package *np, struct ncsi_channel *nc);
> +int ncsi_send_netlink_err(struct net_device *dev, u32 snd_seq, u32 snd_portid, struct nlmsghdr *nlhdr, int err);
> +
> int ncsi_init_netlink(struct net_device *dev);
> int ncsi_unregister_netlink(struct net_device *dev);
>
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 930c1d3..bdf9519 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -16,9 +16,11 @@
> #include <net/ncsi.h>
> #include <net/net_namespace.h>
> #include <net/sock.h>
> +#include <net/genetlink.h>
>
> #include "internal.h"
> #include "ncsi-pkt.h"
> +#include "ncsi-netlink.h"
>
> static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
> unsigned short payload)
> @@ -32,15 +34,22 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
> * before calling this function.
> */
> h = (struct ncsi_rsp_pkt_hdr *)skb_network_header(nr->rsp);
> - if (h->common.revision != NCSI_PKT_REVISION)
> +
> + if (h->common.revision != NCSI_PKT_REVISION) {
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: unsupported header revision\n");
> return -EINVAL;
> - if (ntohs(h->common.length) != payload)
> + }
> + if (ntohs(h->common.length) != payload) {
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: payload length mismatched\n");
> return -EINVAL;
> + }
>
> /* Check on code and reason */
> if (ntohs(h->code) != NCSI_PKT_RSP_C_COMPLETED ||
> - ntohs(h->reason) != NCSI_PKT_RSP_R_NO_ERROR)
> - return -EINVAL;
> + ntohs(h->reason) != NCSI_PKT_RSP_R_NO_ERROR) {
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: non zero response/reason code\n");
> + return -EPERM;
> + }
Why the change to EPERM?
>
> /* Validate checksum, which might be zeroes if the
> * sender doesn't support checksum according to NCSI
> @@ -52,8 +61,11 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
>
> checksum = ncsi_calculate_checksum((unsigned char *)h,
> sizeof(*h) + payload - 4);
> - if (*pchecksum != htonl(checksum))
> +
> + if (*pchecksum != htonl(checksum)) {
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: checksum mismatched\n");
> return -EINVAL;
> + }
>
> return 0;
> }
> @@ -900,6 +912,31 @@ static int ncsi_rsp_handler_gpuuid(struct ncsi_request *nr)
> return 0;
> }
>
> +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
> +{
> + return 0;
> +}
> +
> +static int ncsi_rsp_handler_netlink(struct ncsi_request *nr)
> +{
> + struct ncsi_rsp_pkt *rsp;
> + struct ncsi_dev_priv *ndp = nr->ndp;
> + struct ncsi_package *np;
> + struct ncsi_channel *nc;
> + int ret;
> +
> + /* Find the package */
> + rsp = (struct ncsi_rsp_pkt *)skb_network_header(nr->rsp);
> + ncsi_find_package_and_channel(ndp, rsp->rsp.common.channel,
> + &np, &nc);
> + if (!np)
> + return -ENODEV;
> +
> + ret = ncsi_send_netlink_rsp(nr, np, nc);
> +
> + return ret;
> +}
> +
> static struct ncsi_rsp_handler {
> unsigned char type;
> int payload;
> @@ -932,7 +969,7 @@ static struct ncsi_rsp_handler {
> { NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
> { NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
> { NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
> - { NCSI_PKT_RSP_OEM, 0, NULL },
> + { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
> { NCSI_PKT_RSP_PLDM, 0, NULL },
> { NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
> };
> @@ -950,6 +987,7 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
>
> /* Find the NCSI device */
> nd = ncsi_find_dev(dev);
> +
> ndp = nd ? TO_NCSI_DEV_PRIV(nd) : NULL;
There's a few spots around where you've added or changed whitespace,
please clean these bits up.
> if (!ndp)
> return -ENODEV;
> @@ -1002,6 +1040,15 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
> netdev_warn(ndp->ndev.dev,
> "NCSI: 'bad' packet ignored for type 0x%x\n",
> hdr->type);
> +
> + if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
> + if (ret == -EPERM)
> + goto out_netlink;
> + else
> + ncsi_send_netlink_err(ndp->ndev.dev,
> + nr->snd_seq, nr->snd_portid, &nr->nlhdr,
> + ret);
> + }
More netdev formatting, multi-line statements should be like:
ncsi_send_netlink_err(ndp->ndev.dev,
nr->snd_seq, nr->snd_portid, &nr->nlhdr,
ret);
> goto out;
> }
>
> @@ -1011,6 +1058,17 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
> netdev_err(ndp->ndev.dev,
> "NCSI: Handler for packet type 0x%x returned %d\n",
> hdr->type, ret);
> +
> +out_netlink:
> + if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
> + ret = ncsi_rsp_handler_netlink(nr);
> + if (ret) {
> + netdev_err(ndp->ndev.dev,
> + "NCSI: Netlink handler for packet type 0x%x returned %d\n",
> + hdr->type, ret);
> + }
> + }
> +
> out:
> ncsi_free_request(nr);
> return ret;
^ permalink raw reply
* [PATCH net-next] geneve: fix ttl inherit type
From: Hangbin Liu @ 2018-09-28 1:09 UTC (permalink / raw)
To: netdev
Cc: David Miller, Stephen Hemminger, David Ahern, Phil Sutter,
Hangbin Liu
Phil pointed out that there is a mismatch between vxlan and geneve ttl
inherit. We should define it as a flag and use nla_put_flag to export this
opiton.
Fixes: 52d0d404d39dd ("geneve: add ttl inherit support")
Reported-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
drivers/net/geneve.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 6625fab..09ab2fd 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1100,7 +1100,7 @@ static const struct nla_policy geneve_policy[IFLA_GENEVE_MAX + 1] = {
[IFLA_GENEVE_UDP_CSUM] = { .type = NLA_U8 },
[IFLA_GENEVE_UDP_ZERO_CSUM6_TX] = { .type = NLA_U8 },
[IFLA_GENEVE_UDP_ZERO_CSUM6_RX] = { .type = NLA_U8 },
- [IFLA_GENEVE_TTL_INHERIT] = { .type = NLA_U8 },
+ [IFLA_GENEVE_TTL_INHERIT] = { .type = NLA_FLAG },
};
static int geneve_validate(struct nlattr *tb[], struct nlattr *data[],
@@ -1582,7 +1582,7 @@ static size_t geneve_get_size(const struct net_device *dev)
nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_CSUM */
nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_TX */
nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_RX */
- nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_TTL_INHERIT */
+ nla_total_size(0) + /* IFLA_GENEVE_TTL_INHERIT */
0;
}
@@ -1636,7 +1636,7 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev)
goto nla_put_failure;
#endif
- if (nla_put_u8(skb, IFLA_GENEVE_TTL_INHERIT, ttl_inherit))
+ if (ttl_inherit && nla_put_flag(skb, IFLA_GENEVE_TTL_INHERIT))
goto nla_put_failure;
return 0;
--
2.5.5
^ permalink raw reply related
* [PATCH net] vxlan: use nla_put_flag for ttl inherit
From: Hangbin Liu @ 2018-09-28 1:08 UTC (permalink / raw)
To: netdev
Cc: David Miller, Stephen Hemminger, David Ahern, Phil Sutter,
Hangbin Liu
Phil pointed out that there is a mismatch between vxlan and geneve ttl inherit.
We should define it as a flag and use nla_put_flag to export this opiton.
Fixes: 8fd780698745b ("vxlan: fill ttl inherit info")
Reported-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
drivers/net/vxlan.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 2b8da2b..479dda4 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -3539,7 +3539,7 @@ static size_t vxlan_get_size(const struct net_device *dev)
nla_total_size(sizeof(__u32)) + /* IFLA_VXLAN_LINK */
nla_total_size(sizeof(struct in6_addr)) + /* IFLA_VXLAN_LOCAL{6} */
nla_total_size(sizeof(__u8)) + /* IFLA_VXLAN_TTL */
- nla_total_size(sizeof(__u8)) + /* IFLA_VXLAN_TTL_INHERIT */
+ nla_total_size(0) + /* IFLA_VXLAN_TTL_INHERIT */
nla_total_size(sizeof(__u8)) + /* IFLA_VXLAN_TOS */
nla_total_size(sizeof(__be32)) + /* IFLA_VXLAN_LABEL */
nla_total_size(sizeof(__u8)) + /* IFLA_VXLAN_LEARNING */
@@ -3604,8 +3604,6 @@ static int vxlan_fill_info(struct sk_buff *skb, const struct net_device *dev)
}
if (nla_put_u8(skb, IFLA_VXLAN_TTL, vxlan->cfg.ttl) ||
- nla_put_u8(skb, IFLA_VXLAN_TTL_INHERIT,
- !!(vxlan->cfg.flags & VXLAN_F_TTL_INHERIT)) ||
nla_put_u8(skb, IFLA_VXLAN_TOS, vxlan->cfg.tos) ||
nla_put_be32(skb, IFLA_VXLAN_LABEL, vxlan->cfg.label) ||
nla_put_u8(skb, IFLA_VXLAN_LEARNING,
@@ -3650,6 +3648,10 @@ static int vxlan_fill_info(struct sk_buff *skb, const struct net_device *dev)
nla_put_flag(skb, IFLA_VXLAN_REMCSUM_NOPARTIAL))
goto nla_put_failure;
+ if (vxlan->cfg.flags & VXLAN_F_TTL_INHERIT &&
+ nla_put_flag(skb, IFLA_VXLAN_TTL_INHERIT))
+ goto nla_put_failure;
+
return 0;
nla_put_failure:
--
2.5.5
^ permalink raw reply related
* Re: [PATCH iproute2-next] geneve: fix ttl inherit behavior
From: Hangbin Liu @ 2018-09-28 0:59 UTC (permalink / raw)
To: Phil Sutter, netdev, Stephen Hemminger, David Ahern
In-Reply-To: <20180927090835.GU14666@orbyte.nwl.cc>
On Thu, Sep 27, 2018 at 11:08:36AM +0200, Phil Sutter wrote:
> On Thu, Sep 27, 2018 at 03:27:37PM +0800, Hangbin Liu wrote:
> > Currently when we add geneve with "ttl inherit", we set ttl to 0, which
> > is actually use whatever default value instead of inherit the inner
> > protocol's ttl value.
> >
> > To respect compatibility with old behavior and make a difference between
> > ttl inherit and ttl == 0, we add an attribute IFLA_GENEVE_TTL_INHERIT in
> > kernel commit 52d0d404d39dd ("geneve: add ttl inherit support").
> >
> > Now let's use "ttl inherit" to inherit the inner protocol's ttl, and use
> > "ttl auto" to means "use whatever default value", the same behavior with
> > ttl == 0.
> >
> > Reported-by: Jianlin Shi <jishi@redhat.com>
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>
> Acked-by: Phil Sutter <phil@nwl.cc>
Hi Stephen, David,
Please hold on this path and let me fix the inherit flag issue first.
Thanks
Hangbin
^ permalink raw reply
* Re: [PATCH resend] can: rcar_can: convert to SPDX identifiers
From: Kuninori Morimoto @ 2018-09-28 0:31 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Wolfgang Grandegger, Linux-Renesas, Linux-Net, linux-can
In-Reply-To: <1bfdc10a-41e9-2a1f-b944-a8816186e998@pengutronix.de>
Hi Marc
> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >
> > This patch updates license to use SPDX-License-Identifier
> > instead of verbose license text.
> >
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
>
> Wolfram Sang has already supplied a similar patch, but not for Makefile
> and Kconfig. I've applied your patch for Makefile and Kconfig and
> adjusted the commit message accordingly.
Thank you very much
^ permalink raw reply
* Re: WARN_ON in TLP causing RT throttling
From: Eric Dumazet @ 2018-09-28 0:25 UTC (permalink / raw)
To: stranche, Yuchung Cheng; +Cc: Eric Dumazet, soheil, netdev
In-Reply-To: <f4103fc7d67dff06718bbc5a992170cb@codeaurora.org>
On 09/27/2018 05:16 PM, stranche@codeaurora.org wrote:
> Hi Yuchung,
>
> Based on the dumps we were able to get, it appears that TFO was not used in this case.
> We also tried some local experiments where we dropped incoming SYN packets after already
> successful TFO connections on the receive side to see if TFO would trigger this scenario, but
> have not been able to reproduce it.
>
> One other interesting thing we found is that the socket never sent or received any data. It only
> sent/received the packets for the initial handshake and the outgoing FIN.
Just to make sure : Was this some sort of syzkaller (or other fuzzer) run ?
^ permalink raw reply
* Re: [PATCH] ieee802154: remove a redundant local variable 'i'
From: zhong jiang @ 2018-09-28 6:37 UTC (permalink / raw)
To: Stefan Schmidt; +Cc: davem, alex.aring, netdev, linux-kernel
In-Reply-To: <42999ee5-20c2-5d2c-4ca3-e5bb8f5a3864@datenfreihafen.org>
On 2018/9/27 22:47, Stefan Schmidt wrote:
> Hello.
>
> On 19/09/2018 16:41, zhong jiang wrote:
>> The local variable 'i' is never used after being assigned.
>> hence it should be redundant adn can be removed.
>>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>> ---
>> net/ieee802154/nl802154.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
>> index 99f6c25..5b90151 100644
>> --- a/net/ieee802154/nl802154.c
>> +++ b/net/ieee802154/nl802154.c
>> @@ -445,7 +445,6 @@ static int nl802154_send_wpan_phy(struct cfg802154_registered_device *rdev,
>> {
>> struct nlattr *nl_cmds;
>> void *hdr;
>> - int i;
>>
>> hdr = nl802154hdr_put(msg, portid, seq, flags, cmd);
>> if (!hdr)
>> @@ -508,7 +507,6 @@ static int nl802154_send_wpan_phy(struct cfg802154_registered_device *rdev,
>> if (!nl_cmds)
>> goto nla_put_failure;
>>
>> - i = 0;
>> #define CMD(op, n) \
>> do { \
>> if (rdev->ops->op) { \
>>
> Sorry, but this patch is wrong. The variable i is used in line 513
> inside the CMD() macro. The compiler clearly tells this when running
> with your patch:
>
> net/ieee802154/nl802154.c: In function ‘nl802154_send_wpan_phy’:
> net/ieee802154/nl802154.c:513:4: error: ‘i’ undeclared (first use in
> this function)
>
> I would appreciate if patches sent out would at least be compile tested.
Sorry, I really double check and compile test.
Thanks,
zhong jiang
> regards
> Stefan Schmidt
>
> .
>
^ permalink raw reply
* Re: WARN_ON in TLP causing RT throttling
From: stranche @ 2018-09-28 0:16 UTC (permalink / raw)
To: Yuchung Cheng; +Cc: Eric Dumazet, soheil, netdev
In-Reply-To: <CAK6E8=cMEGMVr+2fpPpoyQ3cqcKb0B1YzzR==bwiP=vQ5oCnmA@mail.gmail.com>
On 2018-09-27 13:14, Yuchung Cheng wrote:
> On Wed, Sep 26, 2018 at 5:09 PM, Eric Dumazet <eric.dumazet@gmail.com>
> wrote:
>>
>>
>>
>> On 09/26/2018 04:46 PM, stranche@codeaurora.org wrote:
>> > Hi Eric,
>> >
>> > Someone recently reported a crash to us on the 4.14.62 kernel where excessive
>> > WARNING prints were spamming the logs and causing watchdog bites. The kernel
>> > does have the following commit by Soheil:
>> > bffd168c3fc5 "tcp: clear tp->packets_out when purging write queue"
>> >
>> > Before this bug we see over 1 second of continuous WARN_ON prints from
>> > tcp_send_loss_probe() like so:
>> >
>> > 7795.530450: <2> tcp_send_loss_probe+0x194/0x1b8
>> > 7795.534833: <2> tcp_write_timer_handler+0xf8/0x1c4
>> > 7795.539492: <2> tcp_write_timer+0x4c/0x74
>> > 7795.543348: <2> call_timer_fn+0xc0/0x1b4
>> > 7795.547113: <2> run_timer_softirq+0x248/0x81c
>> >
>> > Specifically, the prints come from the following check:
>> >
>> > /* Retransmit last segment. */
>> > if (WARN_ON(!skb))
>> > goto rearm_timer;
>> >
>> > Since skb is always NULL, we know there's nothing on the write queue or the
>> > retransmit queue, so we just keep resetting the timer, waiting for more data
>> > to be queued. However, we were able to determine that the TCP socket is in the
>> > TCP_FIN_WAIT1 state, so we will no longer be sending any data and these queues
>> > remain empty.
>> >
>> > Would it be appropriate to stop resetting the TLP timer if we detect that the
>> > connection is starting to close and we have no more data to send the probe with,
>> > or is there some way that this scenario should already be handled?
>> >
>> > Unfortunately, we don't have a reproducer for this crash.
>> >
>>
>> Something is fishy.
>>
>> If there is no skb in the queues, then tp->packets_out should be 0,
>> therefore tcp_rearm_rto() should simply call
>> inet_csk_clear_xmit_timer(sk, ICSK_TIME_RETRANS);
>>
>> I have never seen this report before.
> Do you use Fast Open? I am wondering if its a bug when a TFO server
> closes the socket before the handshake finishes...
>
> Either way, it's pretty safe to just stop TLP if write queue is empty
> for any unexpected reason.
>
>>
Hi Yuchung,
Based on the dumps we were able to get, it appears that TFO was not used
in this case.
We also tried some local experiments where we dropped incoming SYN
packets after already
successful TFO connections on the receive side to see if TFO would
trigger this scenario, but
have not been able to reproduce it.
One other interesting thing we found is that the socket never sent or
received any data. It only
sent/received the packets for the initial handshake and the outgoing
FIN.
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: ethtool tx napi configuration
From: Jason Wang @ 2018-09-27 23:39 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Florian Fainelli, Network Development, David Miller, caleb.raitto,
Michael S. Tsirkin, Jon Olson (Google Drive), Willem de Bruijn
In-Reply-To: <CAF=yD-+hbtz3pXi5EYVsumqMEUE=E+Fq48+mKRj4Th8+jm8dMg@mail.gmail.com>
On 2018年09月27日 21:53, Willem de Bruijn wrote:
> On Thu, Sep 27, 2018 at 4:51 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2018年09月14日 12:46, Willem de Bruijn wrote:
>>>> I'm not sure I get this. If we don't enable tx napi, we tend to delay TX
>>>> interrupt if we found the ring is about to full to avoid interrupt
>>>> storm, so we're probably ok in this case.
>>> I'm only concerned about the transition state when converting from
>>> napi to no-napi when the queue is stopped and tx interrupt disabled.
>>>
>>> With napi mode the interrupt is only disabled if napi is scheduled,
>>> in which case it will eventually reenable the interrupt. But when
>>> switching to no-napi mode in this state no progress will be made.
>>>
>>> But it seems this cannot happen. When converting to no-napi
>>> mode, set_coalesce waits for napi to complete in napi_disable.
>>> So the interrupt should always start enabled when transitioning
>>> into no-napi mode.
>> An update, I meet a hang in napi_disalbe(). But it's hard to be
>> reproduced. I tend to choose a easy way like V1 that only allow the
>> switching when device is down.
> I agree.
>
>> I will post the patch after a vacation. (or you can post if it was
>> urgent for you).
> If you have time to review and add your signed-off-by, I can post it.
> It's a pretty small diff at this point.
>
> But no rush, we can also wait until after your vacation.
Then let me post it after the vacation.
>
> I also need to look at a patch to toggle LRO using ethtool, btw.
Interesting, we've already did something similar during XDP. The
GUEST_TSO_XXX part may need some private flags I believe.
Thanks
^ permalink raw reply
* [PATCHv3 bpf-next 12/12] Documentation: Describe bpf reference tracking
From: Joe Stringer @ 2018-09-27 23:26 UTC (permalink / raw)
To: daniel
Cc: netdev, ast, john.fastabend, tgraf, kafai, nitin.hande,
mauricio.vasquez
In-Reply-To: <20180927232659.14348-1-joe@wand.net.nz>
Document the new pointer types in the verifier and how the pointer ID
tracking works to ensure that references which are taken are later
released.
Signed-off-by: Joe Stringer <joe@wand.net.nz>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
Documentation/networking/filter.txt | 64 +++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/Documentation/networking/filter.txt b/Documentation/networking/filter.txt
index e6b4ebb2b243..4443ce958862 100644
--- a/Documentation/networking/filter.txt
+++ b/Documentation/networking/filter.txt
@@ -1125,6 +1125,14 @@ pointer type. The types of pointers describe their base, as follows:
PTR_TO_STACK Frame pointer.
PTR_TO_PACKET skb->data.
PTR_TO_PACKET_END skb->data + headlen; arithmetic forbidden.
+ PTR_TO_SOCKET Pointer to struct bpf_sock_ops, implicitly refcounted.
+ PTR_TO_SOCKET_OR_NULL
+ Either a pointer to a socket, or NULL; socket lookup
+ returns this type, which becomes a PTR_TO_SOCKET when
+ checked != NULL. PTR_TO_SOCKET is reference-counted,
+ so programs must release the reference through the
+ socket release function before the end of the program.
+ Arithmetic on these pointers is forbidden.
However, a pointer may be offset from this base (as a result of pointer
arithmetic), and this is tracked in two parts: the 'fixed offset' and 'variable
offset'. The former is used when an exactly-known value (e.g. an immediate
@@ -1171,6 +1179,13 @@ over the Ethernet header, then reads IHL and addes (IHL * 4), the resulting
pointer will have a variable offset known to be 4n+2 for some n, so adding the 2
bytes (NET_IP_ALIGN) gives a 4-byte alignment and so word-sized accesses through
that pointer are safe.
+The 'id' field is also used on PTR_TO_SOCKET and PTR_TO_SOCKET_OR_NULL, common
+to all copies of the pointer returned from a socket lookup. This has similar
+behaviour to the handling for PTR_TO_MAP_VALUE_OR_NULL->PTR_TO_MAP_VALUE, but
+it also handles reference tracking for the pointer. PTR_TO_SOCKET implicitly
+represents a reference to the corresponding 'struct sock'. To ensure that the
+reference is not leaked, it is imperative to NULL-check the reference and in
+the non-NULL case, and pass the valid reference to the socket release function.
Direct packet access
--------------------
@@ -1444,6 +1459,55 @@ Error:
8: (7a) *(u64 *)(r0 +0) = 1
R0 invalid mem access 'imm'
+Program that performs a socket lookup then sets the pointer to NULL without
+checking it:
+value:
+ BPF_MOV64_IMM(BPF_REG_2, 0),
+ BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_2, -8),
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_MOV64_IMM(BPF_REG_3, 4),
+ BPF_MOV64_IMM(BPF_REG_4, 0),
+ BPF_MOV64_IMM(BPF_REG_5, 0),
+ BPF_EMIT_CALL(BPF_FUNC_sk_lookup_tcp),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+Error:
+ 0: (b7) r2 = 0
+ 1: (63) *(u32 *)(r10 -8) = r2
+ 2: (bf) r2 = r10
+ 3: (07) r2 += -8
+ 4: (b7) r3 = 4
+ 5: (b7) r4 = 0
+ 6: (b7) r5 = 0
+ 7: (85) call bpf_sk_lookup_tcp#65
+ 8: (b7) r0 = 0
+ 9: (95) exit
+ Unreleased reference id=1, alloc_insn=7
+
+Program that performs a socket lookup but does not NULL-check the returned
+value:
+ BPF_MOV64_IMM(BPF_REG_2, 0),
+ BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_2, -8),
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_MOV64_IMM(BPF_REG_3, 4),
+ BPF_MOV64_IMM(BPF_REG_4, 0),
+ BPF_MOV64_IMM(BPF_REG_5, 0),
+ BPF_EMIT_CALL(BPF_FUNC_sk_lookup_tcp),
+ BPF_EXIT_INSN(),
+Error:
+ 0: (b7) r2 = 0
+ 1: (63) *(u32 *)(r10 -8) = r2
+ 2: (bf) r2 = r10
+ 3: (07) r2 += -8
+ 4: (b7) r3 = 4
+ 5: (b7) r4 = 0
+ 6: (b7) r5 = 0
+ 7: (85) call bpf_sk_lookup_tcp#65
+ 8: (95) exit
+ Unreleased reference id=1, alloc_insn=7
+
Testing
-------
--
2.17.1
^ permalink raw reply related
* [PATCHv3 bpf-next 11/12] selftests/bpf: Add C tests for reference tracking
From: Joe Stringer @ 2018-09-27 23:26 UTC (permalink / raw)
To: daniel
Cc: netdev, ast, john.fastabend, tgraf, kafai, nitin.hande,
mauricio.vasquez
In-Reply-To: <20180927232659.14348-1-joe@wand.net.nz>
Add some tests that demonstrate and test the balanced lookup/free
nature of socket lookup. Section names that start with "fail" represent
programs that are expected to fail verification; all others should
succeed.
Signed-off-by: Joe Stringer <joe@wand.net.nz>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
v3: Rebase against flags arg change of bpf_sk_release()
New tests:
* "fail_use_after_free"
* "fail_modify_sk_pointer"
* "fail_modify_sk_or_null_pointer"
---
tools/testing/selftests/bpf/Makefile | 2 +-
tools/testing/selftests/bpf/test_progs.c | 38 ++++
.../selftests/bpf/test_sk_lookup_kern.c | 180 ++++++++++++++++++
3 files changed, 219 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/test_sk_lookup_kern.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index f802de526f57..1381ab81099c 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -36,7 +36,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
test_get_stack_rawtp.o test_sockmap_kern.o test_sockhash_kern.o \
test_lwt_seg6local.o sendmsg4_prog.o sendmsg6_prog.o test_lirc_mode2_kern.o \
get_cgroup_id_kern.o socket_cookie_prog.o test_select_reuseport_kern.o \
- test_skb_cgroup_id_kern.o bpf_flow.o netcnt_prog.o
+ test_skb_cgroup_id_kern.o bpf_flow.o netcnt_prog.o test_sk_lookup_kern.o
# Order correspond to 'make run_tests' order
TEST_PROGS := test_kmod.sh \
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 63a671803ed6..e8becca9c521 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -1698,6 +1698,43 @@ static void test_task_fd_query_tp(void)
"sys_enter_read");
}
+static void test_reference_tracking()
+{
+ const char *file = "./test_sk_lookup_kern.o";
+ struct bpf_object *obj;
+ struct bpf_program *prog;
+ __u32 duration;
+ int err = 0;
+
+ obj = bpf_object__open(file);
+ if (IS_ERR(obj)) {
+ error_cnt++;
+ return;
+ }
+
+ bpf_object__for_each_program(prog, obj) {
+ const char *title;
+
+ /* Ignore .text sections */
+ title = bpf_program__title(prog, false);
+ if (strstr(title, ".text") != NULL)
+ continue;
+
+ bpf_program__set_type(prog, BPF_PROG_TYPE_SCHED_CLS);
+
+ /* Expect verifier failure if test name has 'fail' */
+ if (strstr(title, "fail") != NULL) {
+ libbpf_set_print(NULL, NULL, NULL);
+ err = !bpf_program__load(prog, "GPL", 0);
+ libbpf_set_print(printf, printf, NULL);
+ } else {
+ err = bpf_program__load(prog, "GPL", 0);
+ }
+ CHECK(err, title, "\n");
+ }
+ bpf_object__close(obj);
+}
+
int main(void)
{
jit_enabled = is_jit_enabled();
@@ -1719,6 +1756,7 @@ int main(void)
test_get_stack_raw_tp();
test_task_fd_query_rawtp();
test_task_fd_query_tp();
+ test_reference_tracking();
printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
diff --git a/tools/testing/selftests/bpf/test_sk_lookup_kern.c b/tools/testing/selftests/bpf/test_sk_lookup_kern.c
new file mode 100644
index 000000000000..b745bdc08c2b
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_sk_lookup_kern.c
@@ -0,0 +1,180 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+// Copyright (c) 2018 Covalent IO, Inc. http://covalent.io
+
+#include <stddef.h>
+#include <stdbool.h>
+#include <string.h>
+#include <linux/bpf.h>
+#include <linux/if_ether.h>
+#include <linux/in.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <linux/pkt_cls.h>
+#include <linux/tcp.h>
+#include <sys/socket.h>
+#include "bpf_helpers.h"
+#include "bpf_endian.h"
+
+int _version SEC("version") = 1;
+char _license[] SEC("license") = "GPL";
+
+/* Fill 'tuple' with L3 info, and attempt to find L4. On fail, return NULL. */
+static struct bpf_sock_tuple *get_tuple(void *data, __u64 nh_off,
+ void *data_end, __u16 eth_proto,
+ bool *ipv4)
+{
+ struct bpf_sock_tuple *result;
+ __u8 proto = 0;
+ __u64 ihl_len;
+
+ if (eth_proto == bpf_htons(ETH_P_IP)) {
+ struct iphdr *iph = (struct iphdr *)(data + nh_off);
+
+ if (iph + 1 > data_end)
+ return NULL;
+ ihl_len = iph->ihl * 4;
+ proto = iph->protocol;
+ *ipv4 = true;
+ result = (struct bpf_sock_tuple *)&iph->saddr;
+ } else if (eth_proto == bpf_htons(ETH_P_IPV6)) {
+ struct ipv6hdr *ip6h = (struct ipv6hdr *)(data + nh_off);
+
+ if (ip6h + 1 > data_end)
+ return NULL;
+ ihl_len = sizeof(*ip6h);
+ proto = ip6h->nexthdr;
+ *ipv4 = true;
+ result = (struct bpf_sock_tuple *)&ip6h->saddr;
+ }
+
+ if (data + nh_off + ihl_len > data_end || proto != IPPROTO_TCP)
+ return NULL;
+
+ return result;
+}
+
+SEC("sk_lookup_success")
+int bpf_sk_lookup_test0(struct __sk_buff *skb)
+{
+ void *data_end = (void *)(long)skb->data_end;
+ void *data = (void *)(long)skb->data;
+ struct ethhdr *eth = (struct ethhdr *)(data);
+ struct bpf_sock_tuple *tuple;
+ struct bpf_sock *sk;
+ size_t tuple_len;
+ bool ipv4;
+
+ if (eth + 1 > data_end)
+ return TC_ACT_SHOT;
+
+ tuple = get_tuple(data, sizeof(*eth), data_end, eth->h_proto, &ipv4);
+ if (!tuple || tuple + sizeof *tuple > data_end)
+ return TC_ACT_SHOT;
+
+ tuple_len = ipv4 ? sizeof(tuple->ipv4) : sizeof(tuple->ipv6);
+ sk = bpf_sk_lookup_tcp(skb, tuple, tuple_len, 0, 0);
+ if (sk)
+ bpf_sk_release(sk);
+ return sk ? TC_ACT_OK : TC_ACT_UNSPEC;
+}
+
+SEC("sk_lookup_success_simple")
+int bpf_sk_lookup_test1(struct __sk_buff *skb)
+{
+ struct bpf_sock_tuple tuple = {};
+ struct bpf_sock *sk;
+
+ sk = bpf_sk_lookup_tcp(skb, &tuple, sizeof(tuple), 0, 0);
+ if (sk)
+ bpf_sk_release(sk);
+ return 0;
+}
+
+SEC("fail_use_after_free")
+int bpf_sk_lookup_uaf(struct __sk_buff *skb)
+{
+ struct bpf_sock_tuple tuple = {};
+ struct bpf_sock *sk;
+ __u32 family = 0;
+
+ sk = bpf_sk_lookup_tcp(skb, &tuple, sizeof(tuple), 0, 0);
+ if (sk) {
+ bpf_sk_release(sk);
+ family = sk->family;
+ }
+ return family;
+}
+
+SEC("fail_modify_sk_pointer")
+int bpf_sk_lookup_modptr(struct __sk_buff *skb)
+{
+ struct bpf_sock_tuple tuple = {};
+ struct bpf_sock *sk;
+ __u32 family;
+
+ sk = bpf_sk_lookup_tcp(skb, &tuple, sizeof(tuple), 0, 0);
+ if (sk) {
+ sk += 1;
+ bpf_sk_release(sk);
+ }
+ return 0;
+}
+
+SEC("fail_modify_sk_or_null_pointer")
+int bpf_sk_lookup_modptr_or_null(struct __sk_buff *skb)
+{
+ struct bpf_sock_tuple tuple = {};
+ struct bpf_sock *sk;
+ __u32 family;
+
+ sk = bpf_sk_lookup_tcp(skb, &tuple, sizeof(tuple), 0, 0);
+ sk += 1;
+ if (sk)
+ bpf_sk_release(sk);
+ return 0;
+}
+
+SEC("fail_no_release")
+int bpf_sk_lookup_test2(struct __sk_buff *skb)
+{
+ struct bpf_sock_tuple tuple = {};
+
+ bpf_sk_lookup_tcp(skb, &tuple, sizeof(tuple), 0, 0);
+ return 0;
+}
+
+SEC("fail_release_twice")
+int bpf_sk_lookup_test3(struct __sk_buff *skb)
+{
+ struct bpf_sock_tuple tuple = {};
+ struct bpf_sock *sk;
+
+ sk = bpf_sk_lookup_tcp(skb, &tuple, sizeof(tuple), 0, 0);
+ bpf_sk_release(sk);
+ bpf_sk_release(sk);
+ return 0;
+}
+
+SEC("fail_release_unchecked")
+int bpf_sk_lookup_test4(struct __sk_buff *skb)
+{
+ struct bpf_sock_tuple tuple = {};
+ struct bpf_sock *sk;
+
+ sk = bpf_sk_lookup_tcp(skb, &tuple, sizeof(tuple), 0, 0);
+ bpf_sk_release(sk);
+ return 0;
+}
+
+void lookup_no_release(struct __sk_buff *skb)
+{
+ struct bpf_sock_tuple tuple = {};
+ bpf_sk_lookup_tcp(skb, &tuple, sizeof(tuple), 0, 0);
+}
+
+SEC("fail_no_release_subcall")
+int bpf_sk_lookup_test5(struct __sk_buff *skb)
+{
+ lookup_no_release(skb);
+ return 0;
+}
--
2.17.1
^ permalink raw reply related
* [PATCHv3 bpf-next 09/12] selftests/bpf: Add tests for reference tracking
From: Joe Stringer @ 2018-09-27 23:26 UTC (permalink / raw)
To: daniel
Cc: netdev, ast, john.fastabend, tgraf, kafai, nitin.hande,
mauricio.vasquez
In-Reply-To: <20180927232659.14348-1-joe@wand.net.nz>
reference tracking: leak potential reference
reference tracking: leak potential reference on stack
reference tracking: leak potential reference on stack 2
reference tracking: zero potential reference
reference tracking: copy and zero potential references
reference tracking: release reference without check
reference tracking: release reference
reference tracking: release reference twice
reference tracking: release reference twice inside branch
reference tracking: alloc, check, free in one subbranch
reference tracking: alloc, check, free in both subbranches
reference tracking in call: free reference in subprog
reference tracking in call: free reference in subprog and outside
reference tracking in call: alloc & leak reference in subprog
reference tracking in call: alloc in subprog, release outside
reference tracking in call: sk_ptr leak into caller stack
reference tracking in call: sk_ptr spill into caller stack
reference tracking: allow LD_ABS
reference tracking: forbid LD_ABS while holding reference
reference tracking: allow LD_IND
reference tracking: forbid LD_IND while holding reference
reference tracking: check reference or tail call
reference tracking: release reference then tail call
reference tracking: leak possible reference over tail call
reference tracking: leak checked reference over tail call
reference tracking: mangle and release sock_or_null
reference tracking: mangle and release sock
reference tracking: access member
reference tracking: write to member
reference tracking: invalid 64-bit access of member
reference tracking: access after release
reference tracking: direct access for lookup
Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
v3: Rebase against bpf_sk_release() flags argument removal.
Removed Alexei's ack since there are many new tests:
* "reference tracking: allow LD_ABS",
* "reference tracking: forbid LD_ABS while holding reference",
* "reference tracking: allow LD_IND",
* "reference tracking: forbid LD_IND while holding reference",
* "reference tracking: check reference or tail call",
* "reference tracking: release reference then tail call",
* "reference tracking: leak possible reference over tail call",
* "reference tracking: leak checked reference over tail call",
* "reference tracking: mangle and release sock_or_null",
* "reference tracking: mangle and release sock",
* "reference tracking: access member",
* "reference tracking: write to member",
* "reference tracking: invalid 64-bit access of member",
* "reference tracking: access after release",
* "reference tracking: direct access for lookup",
---
tools/testing/selftests/bpf/test_verifier.c | 625 ++++++++++++++++++++
1 file changed, 625 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 020b1467e565..9fad54b0bbd0 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -3,6 +3,7 @@
*
* Copyright (c) 2014 PLUMgrid, http://plumgrid.com
* Copyright (c) 2017 Facebook
+ * Copyright (c) 2018 Covalent IO, Inc. http://covalent.io
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of version 2 of the GNU General Public
@@ -178,6 +179,24 @@ static void bpf_fill_rand_ld_dw(struct bpf_test *self)
self->retval = (uint32_t)res;
}
+/* BPF_SK_LOOKUP contains 13 instructions, if you need to fix up maps */
+#define BPF_SK_LOOKUP \
+ /* struct bpf_sock_tuple tuple = {} */ \
+ BPF_MOV64_IMM(BPF_REG_2, 0), \
+ BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_2, -8), \
+ BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -16), \
+ BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -24), \
+ BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -32), \
+ BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -40), \
+ BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -48), \
+ /* sk = sk_lookup_tcp(ctx, &tuple, sizeof tuple, 0, 0) */ \
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), \
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -48), \
+ BPF_MOV64_IMM(BPF_REG_3, sizeof(struct bpf_sock_tuple)), \
+ BPF_MOV64_IMM(BPF_REG_4, 0), \
+ BPF_MOV64_IMM(BPF_REG_5, 0), \
+ BPF_EMIT_CALL(BPF_FUNC_sk_lookup_tcp)
+
static struct bpf_test tests[] = {
{
"add+sub+mul",
@@ -12557,6 +12576,214 @@ static struct bpf_test tests[] = {
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
.result = ACCEPT,
},
+ {
+ "reference tracking: leak potential reference",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0), /* leak reference */
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "Unreleased reference",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: leak potential reference on stack",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -8),
+ BPF_STX_MEM(BPF_DW, BPF_REG_4, BPF_REG_0, 0),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "Unreleased reference",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: leak potential reference on stack 2",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -8),
+ BPF_STX_MEM(BPF_DW, BPF_REG_4, BPF_REG_0, 0),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_ST_MEM(BPF_DW, BPF_REG_4, 0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "Unreleased reference",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: zero potential reference",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_IMM(BPF_REG_0, 0), /* leak reference */
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "Unreleased reference",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: copy and zero potential references",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_MOV64_IMM(BPF_REG_7, 0), /* leak reference */
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "Unreleased reference",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: release reference without check",
+ .insns = {
+ BPF_SK_LOOKUP,
+ /* reference in r0 may be NULL */
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_MOV64_IMM(BPF_REG_2, 0),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "type=sock_or_null expected=sock",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: release reference",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = ACCEPT,
+ },
+ {
+ "reference tracking: release reference 2",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+ BPF_EXIT_INSN(),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = ACCEPT,
+ },
+ {
+ "reference tracking: release reference twice",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "type=inv expected=sock",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: release reference twice inside branch",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3), /* goto end */
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "type=inv expected=sock",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: alloc, check, free in one subbranch",
+ .insns = {
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
+ offsetof(struct __sk_buff, data)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
+ offsetof(struct __sk_buff, data_end)),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 16),
+ /* if (offsetof(skb, mark) > data_len) exit; */
+ BPF_JMP_REG(BPF_JLE, BPF_REG_0, BPF_REG_3, 1),
+ BPF_EXIT_INSN(),
+ BPF_LDX_MEM(BPF_W, BPF_REG_6, BPF_REG_2,
+ offsetof(struct __sk_buff, mark)),
+ BPF_SK_LOOKUP,
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_6, 0, 1), /* mark == 0? */
+ /* Leak reference in R0 */
+ BPF_EXIT_INSN(),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2), /* sk NULL? */
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "Unreleased reference",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: alloc, check, free in both subbranches",
+ .insns = {
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
+ offsetof(struct __sk_buff, data)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
+ offsetof(struct __sk_buff, data_end)),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 16),
+ /* if (offsetof(skb, mark) > data_len) exit; */
+ BPF_JMP_REG(BPF_JLE, BPF_REG_0, BPF_REG_3, 1),
+ BPF_EXIT_INSN(),
+ BPF_LDX_MEM(BPF_W, BPF_REG_6, BPF_REG_2,
+ offsetof(struct __sk_buff, mark)),
+ BPF_SK_LOOKUP,
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_6, 0, 4), /* mark == 0? */
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2), /* sk NULL? */
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2), /* sk NULL? */
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = ACCEPT,
+ },
+ {
+ "reference tracking in call: free reference in subprog",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0), /* unchecked reference */
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+
+ /* subprog 1 */
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_1),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_2, 0, 1),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = ACCEPT,
+ },
{
"pass modified ctx pointer to helper, 1",
.insns = {
@@ -12627,6 +12854,404 @@ static struct bpf_test tests[] = {
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
.result = ACCEPT,
},
+ {
+ "reference tracking in call: free reference in subprog and outside",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0), /* unchecked reference */
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 3),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+
+ /* subprog 1 */
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_1),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_2, 0, 1),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "type=inv expected=sock",
+ .result = REJECT,
+ },
+ {
+ "reference tracking in call: alloc & leak reference in subprog",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -8),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 3),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+
+ /* subprog 1 */
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_4),
+ BPF_SK_LOOKUP,
+ /* spill unchecked sk_ptr into stack of caller */
+ BPF_STX_MEM(BPF_DW, BPF_REG_6, BPF_REG_0, 0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "Unreleased reference",
+ .result = REJECT,
+ },
+ {
+ "reference tracking in call: alloc in subprog, release outside",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_10),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 4),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+
+ /* subprog 1 */
+ BPF_SK_LOOKUP,
+ BPF_EXIT_INSN(), /* return sk */
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .retval = POINTER_VALUE,
+ .result = ACCEPT,
+ },
+ {
+ "reference tracking in call: sk_ptr leak into caller stack",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -8),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+
+ /* subprog 1 */
+ BPF_MOV64_REG(BPF_REG_5, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, -8),
+ BPF_STX_MEM(BPF_DW, BPF_REG_5, BPF_REG_4, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 5),
+ /* spill unchecked sk_ptr into stack of caller */
+ BPF_MOV64_REG(BPF_REG_5, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, -8),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_4, BPF_REG_5, 0),
+ BPF_STX_MEM(BPF_DW, BPF_REG_4, BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+
+ /* subprog 2 */
+ BPF_SK_LOOKUP,
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "Unreleased reference",
+ .result = REJECT,
+ },
+ {
+ "reference tracking in call: sk_ptr spill into caller stack",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -8),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+
+ /* subprog 1 */
+ BPF_MOV64_REG(BPF_REG_5, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, -8),
+ BPF_STX_MEM(BPF_DW, BPF_REG_5, BPF_REG_4, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 8),
+ /* spill unchecked sk_ptr into stack of caller */
+ BPF_MOV64_REG(BPF_REG_5, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, -8),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_4, BPF_REG_5, 0),
+ BPF_STX_MEM(BPF_DW, BPF_REG_4, BPF_REG_0, 0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
+ /* now the sk_ptr is verified, free the reference */
+ BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_4, 0),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+
+ /* subprog 2 */
+ BPF_SK_LOOKUP,
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = ACCEPT,
+ },
+ {
+ "reference tracking: allow LD_ABS",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_1),
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_LD_ABS(BPF_B, 0),
+ BPF_LD_ABS(BPF_H, 0),
+ BPF_LD_ABS(BPF_W, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = ACCEPT,
+ },
+ {
+ "reference tracking: forbid LD_ABS while holding reference",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_1),
+ BPF_SK_LOOKUP,
+ BPF_LD_ABS(BPF_B, 0),
+ BPF_LD_ABS(BPF_H, 0),
+ BPF_LD_ABS(BPF_W, 0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "BPF_LD_[ABS|IND] cannot be mixed with socket references",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: allow LD_IND",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_1),
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_MOV64_IMM(BPF_REG_7, 1),
+ BPF_LD_IND(BPF_W, BPF_REG_7, -0x200000),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_7),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = ACCEPT,
+ .retval = 1,
+ },
+ {
+ "reference tracking: forbid LD_IND while holding reference",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_1),
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_0),
+ BPF_MOV64_IMM(BPF_REG_7, 1),
+ BPF_LD_IND(BPF_W, BPF_REG_7, -0x200000),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_7),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_4),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 1),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "BPF_LD_[ABS|IND] cannot be mixed with socket references",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: check reference or tail call",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_7, BPF_REG_1),
+ BPF_SK_LOOKUP,
+ /* if (sk) bpf_sk_release() */
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_1, 0, 7),
+ /* bpf_tail_call() */
+ BPF_MOV64_IMM(BPF_REG_3, 2),
+ BPF_LD_MAP_FD(BPF_REG_2, 0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_7),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+ BPF_FUNC_tail_call),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_prog1 = { 17 },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = ACCEPT,
+ },
+ {
+ "reference tracking: release reference then tail call",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_7, BPF_REG_1),
+ BPF_SK_LOOKUP,
+ /* if (sk) bpf_sk_release() */
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 1),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ /* bpf_tail_call() */
+ BPF_MOV64_IMM(BPF_REG_3, 2),
+ BPF_LD_MAP_FD(BPF_REG_2, 0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_7),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+ BPF_FUNC_tail_call),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_prog1 = { 18 },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = ACCEPT,
+ },
+ {
+ "reference tracking: leak possible reference over tail call",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_7, BPF_REG_1),
+ /* Look up socket and store in REG_6 */
+ BPF_SK_LOOKUP,
+ /* bpf_tail_call() */
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ BPF_MOV64_IMM(BPF_REG_3, 2),
+ BPF_LD_MAP_FD(BPF_REG_2, 0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_7),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+ BPF_FUNC_tail_call),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ /* if (sk) bpf_sk_release() */
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 1),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_prog1 = { 16 },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "tail_call would lead to reference leak",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: leak checked reference over tail call",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_7, BPF_REG_1),
+ /* Look up socket and store in REG_6 */
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ /* if (!sk) goto end */
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 7),
+ /* bpf_tail_call() */
+ BPF_MOV64_IMM(BPF_REG_3, 0),
+ BPF_LD_MAP_FD(BPF_REG_2, 0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_7),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+ BPF_FUNC_tail_call),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_prog1 = { 17 },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "tail_call would lead to reference leak",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: mangle and release sock_or_null",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, 5),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "R1 pointer arithmetic on sock_or_null prohibited",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: mangle and release sock",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, 5),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "R1 pointer arithmetic on sock prohibited",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: access member",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3),
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_0, 4),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = ACCEPT,
+ },
+ {
+ "reference tracking: write to member",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
+ BPF_LD_IMM64(BPF_REG_2, 0),
+ BPF_STX_MEM(BPF_W, BPF_REG_0, BPF_REG_2, 4),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "cannot write into socket",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: invalid 64-bit access of member",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_0, 0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "invalid bpf_sock access off=0 size=8",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: access after release",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .errstr = "!read_ok",
+ .result = REJECT,
+ },
+ {
+ "reference tracking: direct access for lookup",
+ .insns = {
+ /* Check that the packet is at least 64B long */
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
+ offsetof(struct __sk_buff, data)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
+ offsetof(struct __sk_buff, data_end)),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 64),
+ BPF_JMP_REG(BPF_JGT, BPF_REG_0, BPF_REG_3, 9),
+ /* sk = sk_lookup_tcp(ctx, skb->data, ...) */
+ BPF_MOV64_IMM(BPF_REG_3, sizeof(struct bpf_sock_tuple)),
+ BPF_MOV64_IMM(BPF_REG_4, 0),
+ BPF_MOV64_IMM(BPF_REG_5, 0),
+ BPF_EMIT_CALL(BPF_FUNC_sk_lookup_tcp),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3),
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_0, 4),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = ACCEPT,
+ },
};
static int probe_filter_length(const struct bpf_insn *fp)
--
2.17.1
^ permalink raw reply related
* [PATCHv3 bpf-next 10/12] libbpf: Support loading individual progs
From: Joe Stringer @ 2018-09-27 23:26 UTC (permalink / raw)
To: daniel
Cc: netdev, ast, john.fastabend, tgraf, kafai, nitin.hande,
mauricio.vasquez
In-Reply-To: <20180927232659.14348-1-joe@wand.net.nz>
Allow the individual program load to be invoked. This will help with
testing, where a single ELF may contain several sections, some of which
denote subprograms that are expected to fail verification, along with
some which are expected to pass verification. By allowing programs to be
iterated and individually loaded, each program can be independently
checked against its expected verification result.
Signed-off-by: Joe Stringer <joe@wand.net.nz>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
tools/lib/bpf/libbpf.c | 4 ++--
tools/lib/bpf/libbpf.h | 3 +++
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 425d5ca45c97..9e68fd9fcfca 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -228,7 +228,7 @@ struct bpf_object {
};
#define obj_elf_valid(o) ((o)->efile.elf)
-static void bpf_program__unload(struct bpf_program *prog)
+void bpf_program__unload(struct bpf_program *prog)
{
int i;
@@ -1375,7 +1375,7 @@ load_program(enum bpf_prog_type type, enum bpf_attach_type expected_attach_type,
return ret;
}
-static int
+int
bpf_program__load(struct bpf_program *prog,
char *license, u32 kern_version)
{
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 511c1294dcbf..2ed24d3f80b3 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -128,10 +128,13 @@ void bpf_program__set_ifindex(struct bpf_program *prog, __u32 ifindex);
const char *bpf_program__title(struct bpf_program *prog, bool needs_copy);
+int bpf_program__load(struct bpf_program *prog, char *license,
+ u32 kern_version);
int bpf_program__fd(struct bpf_program *prog);
int bpf_program__pin_instance(struct bpf_program *prog, const char *path,
int instance);
int bpf_program__pin(struct bpf_program *prog, const char *path);
+void bpf_program__unload(struct bpf_program *prog);
struct bpf_insn;
--
2.17.1
^ permalink raw reply related
* [PATCHv3 bpf-next 08/12] selftests/bpf: Generalize dummy program types
From: Joe Stringer @ 2018-09-27 23:26 UTC (permalink / raw)
To: daniel
Cc: netdev, ast, john.fastabend, tgraf, kafai, nitin.hande,
mauricio.vasquez
In-Reply-To: <20180927232659.14348-1-joe@wand.net.nz>
Don't hardcode the dummy program types to SOCKET_FILTER type, as this
prevents testing bpf_tail_call in conjunction with other program types.
Instead, use the program type specified in the test case.
Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
tools/testing/selftests/bpf/test_verifier.c | 31 +++++++++++----------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index a90be44f61e0..020b1467e565 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -12652,18 +12652,18 @@ static int create_map(uint32_t type, uint32_t size_key,
return fd;
}
-static int create_prog_dummy1(void)
+static int create_prog_dummy1(enum bpf_map_type prog_type)
{
struct bpf_insn prog[] = {
BPF_MOV64_IMM(BPF_REG_0, 42),
BPF_EXIT_INSN(),
};
- return bpf_load_program(BPF_PROG_TYPE_SOCKET_FILTER, prog,
+ return bpf_load_program(prog_type, prog,
ARRAY_SIZE(prog), "GPL", 0, NULL, 0);
}
-static int create_prog_dummy2(int mfd, int idx)
+static int create_prog_dummy2(enum bpf_map_type prog_type, int mfd, int idx)
{
struct bpf_insn prog[] = {
BPF_MOV64_IMM(BPF_REG_3, idx),
@@ -12674,11 +12674,12 @@ static int create_prog_dummy2(int mfd, int idx)
BPF_EXIT_INSN(),
};
- return bpf_load_program(BPF_PROG_TYPE_SOCKET_FILTER, prog,
+ return bpf_load_program(prog_type, prog,
ARRAY_SIZE(prog), "GPL", 0, NULL, 0);
}
-static int create_prog_array(uint32_t max_elem, int p1key)
+static int create_prog_array(enum bpf_map_type prog_type, uint32_t max_elem,
+ int p1key)
{
int p2key = 1;
int mfd, p1fd, p2fd;
@@ -12690,8 +12691,8 @@ static int create_prog_array(uint32_t max_elem, int p1key)
return -1;
}
- p1fd = create_prog_dummy1();
- p2fd = create_prog_dummy2(mfd, p2key);
+ p1fd = create_prog_dummy1(prog_type);
+ p2fd = create_prog_dummy2(prog_type, mfd, p2key);
if (p1fd < 0 || p2fd < 0)
goto out;
if (bpf_map_update_elem(mfd, &p1key, &p1fd, BPF_ANY) < 0)
@@ -12748,8 +12749,8 @@ static int create_cgroup_storage(bool percpu)
static char bpf_vlog[UINT_MAX >> 8];
-static void do_test_fixup(struct bpf_test *test, struct bpf_insn *prog,
- int *map_fds)
+static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type,
+ struct bpf_insn *prog, int *map_fds)
{
int *fixup_map1 = test->fixup_map1;
int *fixup_map2 = test->fixup_map2;
@@ -12805,7 +12806,7 @@ static void do_test_fixup(struct bpf_test *test, struct bpf_insn *prog,
}
if (*fixup_prog1) {
- map_fds[4] = create_prog_array(4, 0);
+ map_fds[4] = create_prog_array(prog_type, 4, 0);
do {
prog[*fixup_prog1].imm = map_fds[4];
fixup_prog1++;
@@ -12813,7 +12814,7 @@ static void do_test_fixup(struct bpf_test *test, struct bpf_insn *prog,
}
if (*fixup_prog2) {
- map_fds[5] = create_prog_array(8, 7);
+ map_fds[5] = create_prog_array(prog_type, 8, 7);
do {
prog[*fixup_prog2].imm = map_fds[5];
fixup_prog2++;
@@ -12859,11 +12860,13 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
for (i = 0; i < MAX_NR_MAPS; i++)
map_fds[i] = -1;
- do_test_fixup(test, prog, map_fds);
+ if (!prog_type)
+ prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
+ do_test_fixup(test, prog_type, prog, map_fds);
prog_len = probe_filter_length(prog);
- fd_prog = bpf_verify_program(prog_type ? : BPF_PROG_TYPE_SOCKET_FILTER,
- prog, prog_len, test->flags & F_LOAD_WITH_STRICT_ALIGNMENT,
+ fd_prog = bpf_verify_program(prog_type, prog, prog_len,
+ test->flags & F_LOAD_WITH_STRICT_ALIGNMENT,
"GPL", 0, bpf_vlog, sizeof(bpf_vlog), 1);
expected_ret = unpriv && test->result_unpriv != UNDEF ?
--
2.17.1
^ permalink raw reply related
* [PATCHv3 bpf-next 07/12] bpf: Add helper to retrieve socket in BPF
From: Joe Stringer @ 2018-09-27 23:26 UTC (permalink / raw)
To: daniel
Cc: netdev, ast, john.fastabend, tgraf, kafai, nitin.hande,
mauricio.vasquez
In-Reply-To: <20180927232659.14348-1-joe@wand.net.nz>
This patch adds new BPF helper functions, bpf_sk_lookup_tcp() and
bpf_sk_lookup_udp() which allows BPF programs to find out if there is a
socket listening on this host, and returns a socket pointer which the
BPF program can then access to determine, for instance, whether to
forward or drop traffic. bpf_sk_lookup_xxx() may take a reference on the
socket, so when a BPF program makes use of this function, it must
subsequently pass the returned pointer into the newly added sk_release()
to return the reference.
By way of example, the following pseudocode would filter inbound
connections at XDP if there is no corresponding service listening for
the traffic:
struct bpf_sock_tuple tuple;
struct bpf_sock_ops *sk;
populate_tuple(ctx, &tuple); // Extract the 5tuple from the packet
sk = bpf_sk_lookup_tcp(ctx, &tuple, sizeof tuple, netns, 0);
if (!sk) {
// Couldn't find a socket listening for this traffic. Drop.
return TC_ACT_SHOT;
}
bpf_sk_release(sk);
return TC_ACT_OK;
Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
v2: Rework 'struct bpf_sock_tuple' to allow passing a packet pointer
Limit netns_id field to 32 bits
Fix compile error with CONFIG_IPV6 enabled
Allow direct packet access from helper
v3: Fix release of caller_net when netns is not specified.
Use skb->sk to find caller net when skb->dev is unavailable.
Remove flags argument to sk_release()
Define the semantics of the new helpers more clearly.
---
include/uapi/linux/bpf.h | 93 ++++++++++++-
kernel/bpf/verifier.c | 8 +-
net/core/filter.c | 151 ++++++++++++++++++++++
tools/include/uapi/linux/bpf.h | 93 ++++++++++++-
tools/testing/selftests/bpf/bpf_helpers.h | 12 ++
5 files changed, 354 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index e2070d819e04..f9187b41dff6 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2144,6 +2144,77 @@ union bpf_attr {
* request in the skb.
* Return
* 0 on success, or a negative error in case of failure.
+ *
+ * struct bpf_sock *bpf_sk_lookup_tcp(void *ctx, struct bpf_sock_tuple *tuple, u32 tuple_size, u32 netns, u64 flags)
+ * Description
+ * Look for TCP socket matching *tuple*, optionally in a child
+ * network namespace *netns*. The return value must be checked,
+ * and if non-NULL, released via **bpf_sk_release**\ ().
+ *
+ * The *ctx* should point to the context of the program, such as
+ * the skb or socket (depending on the hook in use). This is used
+ * to determine the base network namespace for the lookup.
+ *
+ * *tuple_size* must be one of:
+ *
+ * **sizeof**\ (*tuple*\ **->ipv4**)
+ * Look for an IPv4 socket.
+ * **sizeof**\ (*tuple*\ **->ipv6**)
+ * Look for an IPv6 socket.
+ *
+ * If the *netns* is zero, then the socket lookup table in the
+ * netns associated with the *ctx* will be used. For the TC hooks,
+ * this in the netns of the device in the skb. For socket hooks,
+ * this in the netns of the socket. If *netns* is non-zero, then
+ * it specifies the ID of the netns relative to the netns
+ * associated with the *ctx*.
+ *
+ * All values for *flags* are reserved for future usage, and must
+ * be left at zero.
+ *
+ * This helper is available only if the kernel was compiled with
+ * **CONFIG_NET** configuration option.
+ * Return
+ * Pointer to *struct bpf_sock*, or NULL in case of failure.
+ *
+ * struct bpf_sock *bpf_sk_lookup_udp(void *ctx, struct bpf_sock_tuple *tuple, u32 tuple_size, u32 netns, u64 flags)
+ * Description
+ * Look for UDP socket matching *tuple*, optionally in a child
+ * network namespace *netns*. The return value must be checked,
+ * and if non-NULL, released via **bpf_sk_release**\ ().
+ *
+ * The *ctx* should point to the context of the program, such as
+ * the skb or socket (depending on the hook in use). This is used
+ * to determine the base network namespace for the lookup.
+ *
+ * *tuple_size* must be one of:
+ *
+ * **sizeof**\ (*tuple*\ **->ipv4**)
+ * Look for an IPv4 socket.
+ * **sizeof**\ (*tuple*\ **->ipv6**)
+ * Look for an IPv6 socket.
+ *
+ * If the *netns* is zero, then the socket lookup table in the
+ * netns associated with the *ctx* will be used. For the TC hooks,
+ * this in the netns of the device in the skb. For socket hooks,
+ * this in the netns of the socket. If *netns* is non-zero, then
+ * it specifies the ID of the netns relative to the netns
+ * associated with the *ctx*.
+ *
+ * All values for *flags* are reserved for future usage, and must
+ * be left at zero.
+ *
+ * This helper is available only if the kernel was compiled with
+ * **CONFIG_NET** configuration option.
+ * Return
+ * Pointer to *struct bpf_sock*, or NULL in case of failure.
+ *
+ * int bpf_sk_release(struct bpf_sock *sk)
+ * Description
+ * Release the reference held by *sock*. *sock* must be a non-NULL
+ * pointer that was returned from bpf_sk_lookup_xxx\ ().
+ * Return
+ * 0 on success, or a negative error in case of failure.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2229,7 +2300,10 @@ union bpf_attr {
FN(get_current_cgroup_id), \
FN(get_local_storage), \
FN(sk_select_reuseport), \
- FN(skb_ancestor_cgroup_id),
+ FN(skb_ancestor_cgroup_id), \
+ FN(sk_lookup_tcp), \
+ FN(sk_lookup_udp), \
+ FN(sk_release),
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
@@ -2399,6 +2473,23 @@ struct bpf_sock {
*/
};
+struct bpf_sock_tuple {
+ union {
+ struct {
+ __be32 saddr;
+ __be32 daddr;
+ __be16 sport;
+ __be16 dport;
+ } ipv4;
+ struct {
+ __be32 saddr[4];
+ __be32 daddr[4];
+ __be16 sport;
+ __be16 dport;
+ } ipv6;
+ };
+};
+
#define XDP_PACKET_HEADROOM 256
/* User return codes for XDP prog type.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index dcc5e8cab537..3f3ae6be6ca3 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -153,6 +153,12 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
* PTR_TO_MAP_VALUE, PTR_TO_SOCKET_OR_NULL becomes PTR_TO_SOCKET when the type
* passes through a NULL-check conditional. For the branch wherein the state is
* changed to CONST_IMM, the verifier releases the reference.
+ *
+ * For each helper function that allocates a reference, such as
+ * bpf_sk_lookup_tcp(), there is a corresponding release function, such as
+ * bpf_sk_release(). When a reference type passes into the release function,
+ * the verifier also releases the reference. If any unchecked or unreleased
+ * reference remains at the end of the program, the verifier rejects it.
*/
/* verifier_state + insn_idx are pushed to stack when branch is encountered */
@@ -300,7 +306,7 @@ static bool arg_type_is_refcounted(enum bpf_arg_type type)
*/
static bool is_release_function(enum bpf_func_id func_id)
{
- return false;
+ return func_id == BPF_FUNC_sk_release;
}
/* string representation of 'enum bpf_reg_type' */
diff --git a/net/core/filter.c b/net/core/filter.c
index 057af3dc9f08..f645655817b6 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -58,13 +58,17 @@
#include <net/busy_poll.h>
#include <net/tcp.h>
#include <net/xfrm.h>
+#include <net/udp.h>
#include <linux/bpf_trace.h>
#include <net/xdp_sock.h>
#include <linux/inetdevice.h>
+#include <net/inet_hashtables.h>
+#include <net/inet6_hashtables.h>
#include <net/ip_fib.h>
#include <net/flow.h>
#include <net/arp.h>
#include <net/ipv6.h>
+#include <net/net_namespace.h>
#include <linux/seg6_local.h>
#include <net/seg6.h>
#include <net/seg6_local.h>
@@ -4813,6 +4817,141 @@ static const struct bpf_func_proto bpf_lwt_seg6_adjust_srh_proto = {
};
#endif /* CONFIG_IPV6_SEG6_BPF */
+struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
+ struct sk_buff *skb, u8 family, u8 proto)
+{
+ int dif = skb->dev->ifindex;
+ bool refcounted = false;
+ struct sock *sk = NULL;
+
+ if (family == AF_INET) {
+ __be32 src4 = tuple->ipv4.saddr;
+ __be32 dst4 = tuple->ipv4.daddr;
+ int sdif = inet_sdif(skb);
+
+ if (proto == IPPROTO_TCP)
+ sk = __inet_lookup(net, &tcp_hashinfo, skb, 0,
+ src4, tuple->ipv4.sport,
+ dst4, tuple->ipv4.dport,
+ dif, sdif, &refcounted);
+ else
+ sk = __udp4_lib_lookup(net, src4, tuple->ipv4.sport,
+ dst4, tuple->ipv4.dport,
+ dif, sdif, &udp_table, skb);
+#if IS_ENABLED(CONFIG_IPV6)
+ } else {
+ struct in6_addr *src6 = (struct in6_addr *)&tuple->ipv6.saddr;
+ struct in6_addr *dst6 = (struct in6_addr *)&tuple->ipv6.daddr;
+ int sdif = inet6_sdif(skb);
+
+ if (proto == IPPROTO_TCP)
+ sk = __inet6_lookup(net, &tcp_hashinfo, skb, 0,
+ src6, tuple->ipv6.sport,
+ dst6, tuple->ipv6.dport,
+ dif, sdif, &refcounted);
+ else
+ sk = __udp6_lib_lookup(net, src6, tuple->ipv6.sport,
+ dst6, tuple->ipv6.dport,
+ dif, sdif, &udp_table, skb);
+#endif
+ }
+
+ if (unlikely(sk && !refcounted && !sock_flag(sk, SOCK_RCU_FREE))) {
+ WARN_ONCE(1, "Found non-RCU, unreferenced socket!");
+ sk = NULL;
+ }
+ return sk;
+}
+
+/* bpf_sk_lookup performs the core lookup for different types of sockets,
+ * taking a reference on the socket if it doesn't have the flag SOCK_RCU_FREE.
+ * Returns the socket as an 'unsigned long' to simplify the casting in the
+ * callers to satisfy BPF_CALL declarations.
+ */
+static unsigned long
+bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
+ u8 proto, u64 netns_id, u64 flags)
+{
+ struct net *caller_net;
+ struct sock *sk = NULL;
+ u8 family = AF_UNSPEC;
+ struct net *net;
+
+ family = len == sizeof(tuple->ipv4) ? AF_INET : AF_INET6;
+ if (unlikely(family == AF_UNSPEC || netns_id > U32_MAX || flags))
+ goto out;
+
+ if (skb->dev)
+ caller_net = dev_net(skb->dev);
+ else
+ caller_net = sock_net(skb->sk);
+ if (netns_id) {
+ net = get_net_ns_by_id(caller_net, netns_id);
+ if (unlikely(!net))
+ goto out;
+ sk = sk_lookup(net, tuple, skb, family, proto);
+ put_net(net);
+ } else {
+ net = caller_net;
+ sk = sk_lookup(net, tuple, skb, family, proto);
+ }
+
+ if (sk)
+ sk = sk_to_full_sk(sk);
+out:
+ return (unsigned long) sk;
+}
+
+BPF_CALL_5(bpf_sk_lookup_tcp, struct sk_buff *, skb,
+ struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
+{
+ return bpf_sk_lookup(skb, tuple, len, IPPROTO_TCP, netns_id, flags);
+}
+
+static const struct bpf_func_proto bpf_sk_lookup_tcp_proto = {
+ .func = bpf_sk_lookup_tcp,
+ .gpl_only = false,
+ .pkt_access = true,
+ .ret_type = RET_PTR_TO_SOCKET_OR_NULL,
+ .arg1_type = ARG_PTR_TO_CTX,
+ .arg2_type = ARG_PTR_TO_MEM,
+ .arg3_type = ARG_CONST_SIZE,
+ .arg4_type = ARG_ANYTHING,
+ .arg5_type = ARG_ANYTHING,
+};
+
+BPF_CALL_5(bpf_sk_lookup_udp, struct sk_buff *, skb,
+ struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
+{
+ return bpf_sk_lookup(skb, tuple, len, IPPROTO_UDP, netns_id, flags);
+}
+
+static const struct bpf_func_proto bpf_sk_lookup_udp_proto = {
+ .func = bpf_sk_lookup_udp,
+ .gpl_only = false,
+ .pkt_access = true,
+ .ret_type = RET_PTR_TO_SOCKET_OR_NULL,
+ .arg1_type = ARG_PTR_TO_CTX,
+ .arg2_type = ARG_PTR_TO_MEM,
+ .arg3_type = ARG_CONST_SIZE,
+ .arg4_type = ARG_ANYTHING,
+ .arg5_type = ARG_ANYTHING,
+};
+
+BPF_CALL_1(bpf_sk_release, struct sock *, sk)
+{
+ if (!sock_flag(sk, SOCK_RCU_FREE))
+ sock_gen_put(sk);
+ return 0;
+}
+
+static const struct bpf_func_proto bpf_sk_release_proto = {
+ .func = bpf_sk_release,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_SOCKET,
+};
+
bool bpf_helper_changes_pkt_data(void *func)
{
if (func == bpf_skb_vlan_push ||
@@ -5019,6 +5158,12 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_skb_ancestor_cgroup_id:
return &bpf_skb_ancestor_cgroup_id_proto;
#endif
+ case BPF_FUNC_sk_lookup_tcp:
+ return &bpf_sk_lookup_tcp_proto;
+ case BPF_FUNC_sk_lookup_udp:
+ return &bpf_sk_lookup_udp_proto;
+ case BPF_FUNC_sk_release:
+ return &bpf_sk_release_proto;
default:
return bpf_base_func_proto(func_id);
}
@@ -5119,6 +5264,12 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sk_redirect_hash_proto;
case BPF_FUNC_get_local_storage:
return &bpf_get_local_storage_proto;
+ case BPF_FUNC_sk_lookup_tcp:
+ return &bpf_sk_lookup_tcp_proto;
+ case BPF_FUNC_sk_lookup_udp:
+ return &bpf_sk_lookup_udp_proto;
+ case BPF_FUNC_sk_release:
+ return &bpf_sk_release_proto;
default:
return bpf_base_func_proto(func_id);
}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index e2070d819e04..f9187b41dff6 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2144,6 +2144,77 @@ union bpf_attr {
* request in the skb.
* Return
* 0 on success, or a negative error in case of failure.
+ *
+ * struct bpf_sock *bpf_sk_lookup_tcp(void *ctx, struct bpf_sock_tuple *tuple, u32 tuple_size, u32 netns, u64 flags)
+ * Description
+ * Look for TCP socket matching *tuple*, optionally in a child
+ * network namespace *netns*. The return value must be checked,
+ * and if non-NULL, released via **bpf_sk_release**\ ().
+ *
+ * The *ctx* should point to the context of the program, such as
+ * the skb or socket (depending on the hook in use). This is used
+ * to determine the base network namespace for the lookup.
+ *
+ * *tuple_size* must be one of:
+ *
+ * **sizeof**\ (*tuple*\ **->ipv4**)
+ * Look for an IPv4 socket.
+ * **sizeof**\ (*tuple*\ **->ipv6**)
+ * Look for an IPv6 socket.
+ *
+ * If the *netns* is zero, then the socket lookup table in the
+ * netns associated with the *ctx* will be used. For the TC hooks,
+ * this in the netns of the device in the skb. For socket hooks,
+ * this in the netns of the socket. If *netns* is non-zero, then
+ * it specifies the ID of the netns relative to the netns
+ * associated with the *ctx*.
+ *
+ * All values for *flags* are reserved for future usage, and must
+ * be left at zero.
+ *
+ * This helper is available only if the kernel was compiled with
+ * **CONFIG_NET** configuration option.
+ * Return
+ * Pointer to *struct bpf_sock*, or NULL in case of failure.
+ *
+ * struct bpf_sock *bpf_sk_lookup_udp(void *ctx, struct bpf_sock_tuple *tuple, u32 tuple_size, u32 netns, u64 flags)
+ * Description
+ * Look for UDP socket matching *tuple*, optionally in a child
+ * network namespace *netns*. The return value must be checked,
+ * and if non-NULL, released via **bpf_sk_release**\ ().
+ *
+ * The *ctx* should point to the context of the program, such as
+ * the skb or socket (depending on the hook in use). This is used
+ * to determine the base network namespace for the lookup.
+ *
+ * *tuple_size* must be one of:
+ *
+ * **sizeof**\ (*tuple*\ **->ipv4**)
+ * Look for an IPv4 socket.
+ * **sizeof**\ (*tuple*\ **->ipv6**)
+ * Look for an IPv6 socket.
+ *
+ * If the *netns* is zero, then the socket lookup table in the
+ * netns associated with the *ctx* will be used. For the TC hooks,
+ * this in the netns of the device in the skb. For socket hooks,
+ * this in the netns of the socket. If *netns* is non-zero, then
+ * it specifies the ID of the netns relative to the netns
+ * associated with the *ctx*.
+ *
+ * All values for *flags* are reserved for future usage, and must
+ * be left at zero.
+ *
+ * This helper is available only if the kernel was compiled with
+ * **CONFIG_NET** configuration option.
+ * Return
+ * Pointer to *struct bpf_sock*, or NULL in case of failure.
+ *
+ * int bpf_sk_release(struct bpf_sock *sk)
+ * Description
+ * Release the reference held by *sock*. *sock* must be a non-NULL
+ * pointer that was returned from bpf_sk_lookup_xxx\ ().
+ * Return
+ * 0 on success, or a negative error in case of failure.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2229,7 +2300,10 @@ union bpf_attr {
FN(get_current_cgroup_id), \
FN(get_local_storage), \
FN(sk_select_reuseport), \
- FN(skb_ancestor_cgroup_id),
+ FN(skb_ancestor_cgroup_id), \
+ FN(sk_lookup_tcp), \
+ FN(sk_lookup_udp), \
+ FN(sk_release),
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
@@ -2399,6 +2473,23 @@ struct bpf_sock {
*/
};
+struct bpf_sock_tuple {
+ union {
+ struct {
+ __be32 saddr;
+ __be32 daddr;
+ __be16 sport;
+ __be16 dport;
+ } ipv4;
+ struct {
+ __be32 saddr[4];
+ __be32 daddr[4];
+ __be16 sport;
+ __be16 dport;
+ } ipv6;
+ };
+};
+
#define XDP_PACKET_HEADROOM 256
/* User return codes for XDP prog type.
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index e4be7730222d..1d407b3494f9 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -143,6 +143,18 @@ static unsigned long long (*bpf_skb_cgroup_id)(void *ctx) =
(void *) BPF_FUNC_skb_cgroup_id;
static unsigned long long (*bpf_skb_ancestor_cgroup_id)(void *ctx, int level) =
(void *) BPF_FUNC_skb_ancestor_cgroup_id;
+static struct bpf_sock *(*bpf_sk_lookup_tcp)(void *ctx,
+ struct bpf_sock_tuple *tuple,
+ int size, unsigned int netns_id,
+ unsigned long long flags) =
+ (void *) BPF_FUNC_sk_lookup_tcp;
+static struct bpf_sock *(*bpf_sk_lookup_udp)(void *ctx,
+ struct bpf_sock_tuple *tuple,
+ int size, unsigned int netns_id,
+ unsigned long long flags) =
+ (void *) BPF_FUNC_sk_lookup_udp;
+static int (*bpf_sk_release)(struct bpf_sock *sk) =
+ (void *) BPF_FUNC_sk_release;
/* llvm builtin functions that eBPF C program may use to
* emit BPF_LD_ABS and BPF_LD_IND instructions
--
2.17.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox