* [PATCH RFC 0/5] RCU fixes for rcu_assign_pointer() usage
From: Joel Fernandes (Google) @ 2019-02-21 5:49 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ido Schimmel,
Ingo Molnar, moderated list:INTEL ETHERNET DRIVERS,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, Lai Jiangshan,
Martin KaFai Lau, Mathieu Desnoyers, netdev, Paul E. McKenney,
Peter Zijlstra, rcu, Song Liu, Steven Rostedt, xdp-newbies,
Yonghong Song
These patches fix various RCU API usage issues found due to sparse errors as a
result of the recent check to add rcu_check_sparse() to rcu_assign_pointer().
The errors in many cases seem to indicate either an incorrect API usage, or
missing annotations. The annotations added can also help avoid future incorrect
usages and bugs so it is a good idea to do in any case.
These are only build/boot tested and I request for feedback from maintainers
and developers in the various areas the patches touch. Thanks for any feedback!
(There are still errors in rbtree.h but I have kept those for a later time
since fixing them is a bit more involved).
Joel Fernandes (Google) (5):
net: rtnetlink: Fix incorrect RCU API usage
ixgbe: Fix incorrect RCU API usage
sched/cpufreq: Fix incorrect RCU API usage
sched/topology: Annonate RCU pointers properly
rcuwait: Replace rcu_assign_pointer() with WRITE_ONCE
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 4 ++--
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 17 ++++++++++++-----
include/linux/rcuwait.h | 2 +-
kernel/sched/cpufreq.c | 8 ++++++--
kernel/sched/sched.h | 14 +++++++-------
kernel/sched/topology.c | 12 ++++++------
net/core/rtnetlink.c | 4 ++--
7 files changed, 36 insertions(+), 25 deletions(-)
--
2.21.0.rc0.258.g878e2cd30e-goog
^ permalink raw reply
* Re: [PATCH bpf-next v5 1/3] libbpf: add support for using AF_XDP sockets
From: Ye Xiaolong @ 2019-02-21 5:59 UTC (permalink / raw)
To: Magnus Karlsson
Cc: bjorn.topel, ast, daniel, netdev, jakub.kicinski, bjorn.topel,
qi.z.zhang, brouer
In-Reply-To: <1550585838-15630-2-git-send-email-magnus.karlsson@intel.com>
Hi Magnus
On 02/19, Magnus Karlsson wrote:
[snip]
>+static int xsk_setup_xdp_prog(struct xsk_socket *xsk)
>+{
>+ bool prog_attached = false;
>+ __u32 prog_id = 0;
>+ int err;
>+
>+ err = bpf_get_link_xdp_id(xsk->ifindex, &prog_id,
>+ xsk->config.xdp_flags);
>+ if (err)
>+ return err;
>+
>+ if (!prog_id) {
>+ prog_attached = true;
>+ err = xsk_create_bpf_maps(xsk);
>+ if (err)
>+ return err;
>+
>+ err = xsk_load_xdp_prog(xsk);
>+ if (err)
>+ goto out_maps;
>+ } else {
>+ xsk->fd = bpf_prog_get_fd_by_id(prog_id);
I suppose it should be
xsk->prog_fd = bpf_prog_get_fd_by_id(prog_id);
>+ }
>+
>+ err = xsk_update_bpf_maps(xsk, true, xsk->fd);
>+ if (err)
>+ goto out_load;
>+
>+ return 0;
>+
>+out_load:
>+ if (prog_attached)
>+ close(xsk->prog_fd);
>+out_maps:
>+ if (prog_attached)
>+ xsk_delete_bpf_maps(xsk);
>+ return err;
>+}
>+
>+int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
>+ __u32 queue_id, struct xsk_umem *umem,
>+ struct xsk_ring_cons *rx, struct xsk_ring_prod *tx,
>+ const struct xsk_socket_config *usr_config)
>+{
>+ struct sockaddr_xdp sxdp = {};
>+ struct xdp_mmap_offsets off;
>+ struct xsk_socket *xsk;
>+ socklen_t optlen;
>+ void *map;
>+ int err;
>+
>+ if (!umem || !xsk_ptr || !rx || !tx)
>+ return -EFAULT;
>+
>+ if (umem->refcount) {
>+ pr_warning("Error: shared umems not supported by libbpf.\n");
>+ return -EBUSY;
>+ }
>+
>+ xsk = calloc(1, sizeof(*xsk));
>+ if (!xsk)
>+ return -ENOMEM;
>+
>+ if (umem->refcount++ > 0) {
>+ xsk->fd = socket(AF_XDP, SOCK_RAW, 0);
>+ if (xsk->fd < 0) {
>+ err = -errno;
>+ goto out_xsk_alloc;
>+ }
>+ } else {
>+ xsk->fd = umem->fd;
>+ }
>+
>+ xsk->outstanding_tx = 0;
>+ xsk->queue_id = queue_id;
>+ xsk->umem = umem;
>+ xsk->ifindex = if_nametoindex(ifname);
>+ if (!xsk->ifindex) {
>+ err = -errno;
>+ goto out_socket;
>+ }
>+ strncpy(xsk->ifname, ifname, IFNAMSIZ);
>+
>+ xsk_set_xdp_socket_config(&xsk->config, usr_config);
>+
>+ if (rx) {
>+ err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,
>+ &xsk->config.rx_size,
>+ sizeof(xsk->config.rx_size));
>+ if (err) {
>+ err = -errno;
>+ goto out_socket;
>+ }
>+ }
>+ if (tx) {
>+ err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING,
>+ &xsk->config.tx_size,
>+ sizeof(xsk->config.tx_size));
>+ if (err) {
>+ err = -errno;
>+ goto out_socket;
>+ }
>+ }
>+
>+ optlen = sizeof(off);
>+ err = getsockopt(xsk->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen);
>+ if (err) {
>+ err = -errno;
>+ goto out_socket;
>+ }
>+
>+ if (rx) {
>+ map = xsk_mmap(NULL, off.rx.desc +
>+ xsk->config.rx_size * sizeof(struct xdp_desc),
>+ PROT_READ | PROT_WRITE,
>+ MAP_SHARED | MAP_POPULATE,
>+ xsk->fd, XDP_PGOFF_RX_RING);
>+ if (map == MAP_FAILED) {
>+ err = -errno;
>+ goto out_socket;
>+ }
>+
>+ rx->mask = xsk->config.rx_size - 1;
>+ rx->size = xsk->config.rx_size;
>+ rx->producer = map + off.rx.producer;
>+ rx->consumer = map + off.rx.consumer;
>+ rx->ring = map + off.rx.desc;
>+ }
>+ xsk->rx = rx;
>+
>+ if (tx) {
>+ map = xsk_mmap(NULL, off.tx.desc +
>+ xsk->config.tx_size * sizeof(struct xdp_desc),
>+ PROT_READ | PROT_WRITE,
>+ MAP_SHARED | MAP_POPULATE,
>+ xsk->fd, XDP_PGOFF_TX_RING);
>+ if (map == MAP_FAILED) {
>+ err = -errno;
>+ goto out_mmap_rx;
>+ }
>+
>+ tx->mask = xsk->config.tx_size - 1;
>+ tx->size = xsk->config.tx_size;
>+ tx->producer = map + off.tx.producer;
>+ tx->consumer = map + off.tx.consumer;
>+ tx->ring = map + off.tx.desc;
>+ tx->cached_cons = xsk->config.tx_size;
>+ }
>+ xsk->tx = tx;
>+
>+ sxdp.sxdp_family = PF_XDP;
>+ sxdp.sxdp_ifindex = xsk->ifindex;
>+ sxdp.sxdp_queue_id = xsk->queue_id;
>+ sxdp.sxdp_flags = xsk->config.bind_flags;
>+
>+ err = bind(xsk->fd, (struct sockaddr *)&sxdp, sizeof(sxdp));
>+ if (err) {
>+ err = -errno;
>+ goto out_mmap_tx;
>+ }
>+
>+ if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
>+ err = xsk_setup_xdp_prog(xsk);
>+ if (err)
>+ goto out_mmap_tx;
>+ }
>+
>+ *xsk_ptr = xsk;
>+ return 0;
>+
>+out_mmap_tx:
>+ if (tx)
>+ munmap(xsk->tx,
>+ off.tx.desc +
>+ xsk->config.tx_size * sizeof(struct xdp_desc));
>+out_mmap_rx:
>+ if (rx)
>+ munmap(xsk->rx,
>+ off.rx.desc +
>+ xsk->config.rx_size * sizeof(struct xdp_desc));
>+out_socket:
>+ if (--umem->refcount)
>+ close(xsk->fd);
>+out_xsk_alloc:
>+ free(xsk);
>+ return err;
>+}
>+
>+int xsk_umem__delete(struct xsk_umem *umem)
>+{
>+ struct xdp_mmap_offsets off;
>+ socklen_t optlen;
>+ int err;
>+
>+ if (!umem)
>+ return 0;
>+
>+ if (umem->refcount)
>+ return -EBUSY;
>+
>+ optlen = sizeof(off);
>+ err = getsockopt(umem->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen);
>+ if (!err) {
>+ munmap(umem->fill->ring,
>+ off.fr.desc + umem->config.fill_size * sizeof(__u64));
>+ munmap(umem->comp->ring,
>+ off.cr.desc + umem->config.comp_size * sizeof(__u64));
>+ }
>+
>+ close(umem->fd);
>+ free(umem);
>+
>+ return 0;
>+}
>+
>+void xsk_socket__delete(struct xsk_socket *xsk)
>+{
>+ struct xdp_mmap_offsets off;
>+ socklen_t optlen;
>+ int err;
>+
>+ if (!xsk)
>+ return;
>+
>+ (void)xsk_update_bpf_maps(xsk, 0, 0);
>+
>+ optlen = sizeof(off);
>+ err = getsockopt(xsk->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen);
>+ if (!err) {
>+ if (xsk->rx)
>+ munmap(xsk->rx->ring,
>+ off.rx.desc +
>+ xsk->config.rx_size * sizeof(struct xdp_desc));
>+ if (xsk->tx)
>+ munmap(xsk->tx->ring,
>+ off.tx.desc +
>+ xsk->config.tx_size * sizeof(struct xdp_desc));
>+ }
>+
>+ xsk->umem->refcount--;
>+ /* Do not close an fd that also has an associated umem connected
>+ * to it.
>+ */
>+ if (xsk->fd != xsk->umem->fd)
>+ close(xsk->fd);
>+ free(xsk);
>+}
>diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
>new file mode 100644
>index 0000000..a497f00
>--- /dev/null
>+++ b/tools/lib/bpf/xsk.h
>@@ -0,0 +1,203 @@
>+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
>+
>+/*
>+ * AF_XDP user-space access library.
>+ *
>+ * Copyright(c) 2018 - 2019 Intel Corporation.
>+ *
>+ * Author(s): Magnus Karlsson <magnus.karlsson@intel.com>
>+ */
>+
>+#ifndef __LIBBPF_XSK_H
>+#define __LIBBPF_XSK_H
>+
>+#include <stdio.h>
>+#include <stdint.h>
>+#include <linux/if_xdp.h>
>+
>+#include "libbpf.h"
>+
>+#ifdef __cplusplus
>+extern "C" {
>+#endif
>+
>+/* Do not access these members directly. Use the functions below. */
>+#define DEFINE_XSK_RING(name) \
>+struct name { \
>+ __u32 cached_prod; \
>+ __u32 cached_cons; \
>+ __u32 mask; \
>+ __u32 size; \
>+ __u32 *producer; \
>+ __u32 *consumer; \
>+ void *ring; \
>+}
>+
>+DEFINE_XSK_RING(xsk_ring_prod);
>+DEFINE_XSK_RING(xsk_ring_cons);
>+
>+struct xsk_umem;
>+struct xsk_socket;
>+
>+static inline __u64 *xsk_ring_prod__fill_addr(struct xsk_ring_prod *fill,
>+ __u32 idx)
>+{
>+ __u64 *addrs = (__u64 *)fill->ring;
>+
>+ return &addrs[idx & fill->mask];
>+}
>+
>+static inline const __u64 *
>+xsk_ring_cons__comp_addr(const struct xsk_ring_cons *comp, __u32 idx)
>+{
>+ const __u64 *addrs = (const __u64 *)comp->ring;
>+
>+ return &addrs[idx & comp->mask];
>+}
>+
>+static inline struct xdp_desc *xsk_ring_prod__tx_desc(struct xsk_ring_prod *tx,
>+ __u32 idx)
>+{
>+ struct xdp_desc *descs = (struct xdp_desc *)tx->ring;
>+
>+ return &descs[idx & tx->mask];
>+}
>+
>+static inline const struct xdp_desc *
>+xsk_ring_cons__rx_desc(const struct xsk_ring_cons *rx, __u32 idx)
>+{
>+ const struct xdp_desc *descs = (const struct xdp_desc *)rx->ring;
>+
>+ return &descs[idx & rx->mask];
>+}
>+
>+static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb)
>+{
>+ __u32 free_entries = r->cached_cons - r->cached_prod;
>+
>+ if (free_entries >= nb)
>+ return free_entries;
>+
>+ /* Refresh the local tail pointer.
>+ * cached_cons is r->size bigger than the real consumer pointer so
>+ * that this addition can be avoided in the more frequently
>+ * executed code that computs free_entries in the beginning of
>+ * this function. Without this optimization it whould have been
>+ * free_entries = r->cached_prod - r->cached_cons + r->size.
>+ */
>+ r->cached_cons = *r->consumer + r->size;
>+
>+ return r->cached_cons - r->cached_prod;
>+}
>+
>+static inline __u32 xsk_cons_nb_avail(struct xsk_ring_cons *r, __u32 nb)
>+{
>+ __u32 entries = r->cached_prod - r->cached_cons;
>+
>+ if (entries == 0) {
>+ r->cached_prod = *r->producer;
>+ entries = r->cached_prod - r->cached_cons;
>+ }
>+
>+ return (entries > nb) ? nb : entries;
>+}
>+
>+static inline size_t xsk_ring_prod__reserve(struct xsk_ring_prod *prod,
>+ size_t nb, __u32 *idx)
>+{
>+ if (unlikely(xsk_prod_nb_free(prod, nb) < nb))
>+ return 0;
>+
>+ *idx = prod->cached_prod;
>+ prod->cached_prod += nb;
>+
>+ return nb;
>+}
>+
>+static inline void xsk_ring_prod__submit(struct xsk_ring_prod *prod, size_t nb)
>+{
>+ /* Make sure everything has been written to the ring before signalling
>+ * this to the kernel.
>+ */
>+ smp_wmb();
>+
>+ *prod->producer += nb;
>+}
>+
>+static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons,
>+ size_t nb, __u32 *idx)
>+{
>+ size_t entries = xsk_cons_nb_avail(cons, nb);
>+
>+ if (likely(entries > 0)) {
>+ /* Make sure we do not speculatively read the data before
>+ * we have received the packet buffers from the ring.
>+ */
>+ smp_rmb();
>+
>+ *idx = cons->cached_cons;
>+ cons->cached_cons += entries;
>+ }
>+
>+ return entries;
>+}
>+
>+static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb)
>+{
>+ *cons->consumer += nb;
>+}
>+
>+static inline void *xsk_umem__get_data(void *umem_area, __u64 addr)
>+{
>+ return &((char *)umem_area)[addr];
>+}
>+
>+LIBBPF_API int xsk_umem__fd(const struct xsk_umem *umem);
>+LIBBPF_API int xsk_socket__fd(const struct xsk_socket *xsk);
>+
>+#define XSK_RING_CONS__DEFAULT_NUM_DESCS 2048
>+#define XSK_RING_PROD__DEFAULT_NUM_DESCS 2048
>+#define XSK_UMEM__DEFAULT_FRAME_SHIFT 11 /* 2048 bytes */
>+#define XSK_UMEM__DEFAULT_FRAME_SIZE (1 << XSK_UMEM__DEFAULT_FRAME_SHIFT)
>+#define XSK_UMEM__DEFAULT_FRAME_HEADROOM 0
>+
>+struct xsk_umem_config {
>+ __u32 fill_size;
>+ __u32 comp_size;
>+ __u32 frame_size;
>+ __u32 frame_headroom;
>+};
>+
>+/* Flags for the libbpf_flags field. */
>+#define XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD (1 << 0)
>+
>+struct xsk_socket_config {
>+ __u32 rx_size;
>+ __u32 tx_size;
>+ __u32 libbpf_flags;
>+ __u32 xdp_flags;
>+ __u16 bind_flags;
>+};
>+
>+/* Set config to NULL to get the default configuration. */
>+LIBBPF_API int xsk_umem__create(struct xsk_umem **umem,
>+ void *umem_area, __u64 size,
>+ struct xsk_ring_prod *fill,
>+ struct xsk_ring_cons *comp,
>+ const struct xsk_umem_config *config);
>+LIBBPF_API int xsk_socket__create(struct xsk_socket **xsk,
>+ const char *ifname, __u32 queue_id,
>+ struct xsk_umem *umem,
>+ struct xsk_ring_cons *rx,
>+ struct xsk_ring_prod *tx,
>+ const struct xsk_socket_config *config);
>+
>+/* Returns 0 for success and -EBUSY if the umem is still in use. */
>+LIBBPF_API int xsk_umem__delete(struct xsk_umem *umem);
>+LIBBPF_API void xsk_socket__delete(struct xsk_socket *xsk);
>+
>+#ifdef __cplusplus
>+} /* extern "C" */
>+#endif
>+
>+#endif /* __LIBBPF_XSK_H */
>--
>2.7.4
>
^ permalink raw reply
* [PATCH] perf record: Add support for limit perf output file size
From: Jiwei Sun @ 2019-02-21 6:44 UTC (permalink / raw)
To: peterz, mingo, acme, alexander.shishkin, jolsa, namhyung
Cc: ast, daniel, kafai, songliubraving, yhs, linux-kernel, netdev
The patch adds a new option to limit the output file size, then based
on it, we can create a wrapper of the perf command that uses the option
to avoid exhausting the disk space by the unconscious user.
Signed-off-by: Jiwei Sun <jiwei.sun@windriver.com>
---
tools/perf/builtin-record.c | 39 +++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 882285fb9f64..28a03929166d 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -81,6 +81,7 @@ struct record {
bool timestamp_boundary;
struct switch_output switch_output;
unsigned long long samples;
+ unsigned long output_max_size; /* = 0: unlimited */
};
static volatile int auxtrace_record__snapshot_started;
@@ -106,6 +107,12 @@ static bool switch_output_time(struct record *rec)
trigger_is_ready(&switch_output_trigger);
}
+static bool record__output_max_size_exceeded(struct record *rec)
+{
+ return (rec->output_max_size &&
+ rec->bytes_written >= rec->output_max_size);
+}
+
static int record__write(struct record *rec, struct perf_mmap *map __maybe_unused,
void *bf, size_t size)
{
@@ -118,6 +125,9 @@ static int record__write(struct record *rec, struct perf_mmap *map __maybe_unuse
rec->bytes_written += size;
+ if (record__output_max_size_exceeded(rec))
+ raise(SIGTERM);
+
if (switch_output_size(rec))
trigger_hit(&switch_output_trigger);
@@ -1639,6 +1649,33 @@ static int parse_clockid(const struct option *opt, const char *str, int unset)
return -1;
}
+static int parse_output_max_size(const struct option *opt, const char *str,
+ int unset)
+{
+ unsigned long *s = (unsigned long *)opt->value;
+ static struct parse_tag tags_size[] = {
+ { .tag = 'B', .mult = 1 },
+ { .tag = 'K', .mult = 1 << 10 },
+ { .tag = 'M', .mult = 1 << 20 },
+ { .tag = 'G', .mult = 1 << 30 },
+ { .tag = 0 },
+ };
+ unsigned long val;
+
+ if (unset) {
+ *s = 0;
+ return 0;
+ }
+
+ val = parse_tag_value(str, tags_size);
+ if (val != (unsigned long) -1) {
+ *s = val;
+ return 0;
+ }
+
+ return -1;
+}
+
static int record__parse_mmap_pages(const struct option *opt,
const char *str,
int unset __maybe_unused)
@@ -1946,6 +1983,8 @@ static struct option __record_options[] = {
&nr_cblocks_default, "n", "Use <n> control blocks in asynchronous trace writing mode (default: 1, max: 4)",
record__aio_parse),
#endif
+ OPT_CALLBACK(0, "output-max-size", &record.output_max_size,
+ "size", "Output file maximum size", parse_output_max_size),
OPT_END()
};
--
2.20.1
^ permalink raw reply related
* Re: [PATCH bpf-next v5 1/3] libbpf: add support for using AF_XDP sockets
From: Magnus Karlsson @ 2019-02-21 7:09 UTC (permalink / raw)
To: Ye Xiaolong
Cc: Magnus Karlsson, Björn Töpel, ast, Daniel Borkmann,
Network Development, Jakub Kicinski, Björn Töpel,
Zhang, Qi Z, Jesper Dangaard Brouer
In-Reply-To: <20190221055907.GB81905@intel.com>
On Thu, Feb 21, 2019 at 7:06 AM Ye Xiaolong <xiaolong.ye@intel.com> wrote:
>
> Hi Magnus
>
> On 02/19, Magnus Karlsson wrote:
> [snip]
> >+static int xsk_setup_xdp_prog(struct xsk_socket *xsk)
> >+{
> >+ bool prog_attached = false;
> >+ __u32 prog_id = 0;
> >+ int err;
> >+
> >+ err = bpf_get_link_xdp_id(xsk->ifindex, &prog_id,
> >+ xsk->config.xdp_flags);
> >+ if (err)
> >+ return err;
> >+
> >+ if (!prog_id) {
> >+ prog_attached = true;
> >+ err = xsk_create_bpf_maps(xsk);
> >+ if (err)
> >+ return err;
> >+
> >+ err = xsk_load_xdp_prog(xsk);
> >+ if (err)
> >+ goto out_maps;
> >+ } else {
> >+ xsk->fd = bpf_prog_get_fd_by_id(prog_id);
>
> I suppose it should be
>
> xsk->prog_fd = bpf_prog_get_fd_by_id(prog_id);
My bad, that is correct. Will spin a v6.
Thanks: Magnus
> >+ }
> >+
> >+ err = xsk_update_bpf_maps(xsk, true, xsk->fd);
> >+ if (err)
> >+ goto out_load;
> >+
> >+ return 0;
> >+
> >+out_load:
> >+ if (prog_attached)
> >+ close(xsk->prog_fd);
> >+out_maps:
> >+ if (prog_attached)
> >+ xsk_delete_bpf_maps(xsk);
> >+ return err;
> >+}
> >+
> >+int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
> >+ __u32 queue_id, struct xsk_umem *umem,
> >+ struct xsk_ring_cons *rx, struct xsk_ring_prod *tx,
> >+ const struct xsk_socket_config *usr_config)
> >+{
> >+ struct sockaddr_xdp sxdp = {};
> >+ struct xdp_mmap_offsets off;
> >+ struct xsk_socket *xsk;
> >+ socklen_t optlen;
> >+ void *map;
> >+ int err;
> >+
> >+ if (!umem || !xsk_ptr || !rx || !tx)
> >+ return -EFAULT;
> >+
> >+ if (umem->refcount) {
> >+ pr_warning("Error: shared umems not supported by libbpf.\n");
> >+ return -EBUSY;
> >+ }
> >+
> >+ xsk = calloc(1, sizeof(*xsk));
> >+ if (!xsk)
> >+ return -ENOMEM;
> >+
> >+ if (umem->refcount++ > 0) {
> >+ xsk->fd = socket(AF_XDP, SOCK_RAW, 0);
> >+ if (xsk->fd < 0) {
> >+ err = -errno;
> >+ goto out_xsk_alloc;
> >+ }
> >+ } else {
> >+ xsk->fd = umem->fd;
> >+ }
> >+
> >+ xsk->outstanding_tx = 0;
> >+ xsk->queue_id = queue_id;
> >+ xsk->umem = umem;
> >+ xsk->ifindex = if_nametoindex(ifname);
> >+ if (!xsk->ifindex) {
> >+ err = -errno;
> >+ goto out_socket;
> >+ }
> >+ strncpy(xsk->ifname, ifname, IFNAMSIZ);
> >+
> >+ xsk_set_xdp_socket_config(&xsk->config, usr_config);
> >+
> >+ if (rx) {
> >+ err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,
> >+ &xsk->config.rx_size,
> >+ sizeof(xsk->config.rx_size));
> >+ if (err) {
> >+ err = -errno;
> >+ goto out_socket;
> >+ }
> >+ }
> >+ if (tx) {
> >+ err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING,
> >+ &xsk->config.tx_size,
> >+ sizeof(xsk->config.tx_size));
> >+ if (err) {
> >+ err = -errno;
> >+ goto out_socket;
> >+ }
> >+ }
> >+
> >+ optlen = sizeof(off);
> >+ err = getsockopt(xsk->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen);
> >+ if (err) {
> >+ err = -errno;
> >+ goto out_socket;
> >+ }
> >+
> >+ if (rx) {
> >+ map = xsk_mmap(NULL, off.rx.desc +
> >+ xsk->config.rx_size * sizeof(struct xdp_desc),
> >+ PROT_READ | PROT_WRITE,
> >+ MAP_SHARED | MAP_POPULATE,
> >+ xsk->fd, XDP_PGOFF_RX_RING);
> >+ if (map == MAP_FAILED) {
> >+ err = -errno;
> >+ goto out_socket;
> >+ }
> >+
> >+ rx->mask = xsk->config.rx_size - 1;
> >+ rx->size = xsk->config.rx_size;
> >+ rx->producer = map + off.rx.producer;
> >+ rx->consumer = map + off.rx.consumer;
> >+ rx->ring = map + off.rx.desc;
> >+ }
> >+ xsk->rx = rx;
> >+
> >+ if (tx) {
> >+ map = xsk_mmap(NULL, off.tx.desc +
> >+ xsk->config.tx_size * sizeof(struct xdp_desc),
> >+ PROT_READ | PROT_WRITE,
> >+ MAP_SHARED | MAP_POPULATE,
> >+ xsk->fd, XDP_PGOFF_TX_RING);
> >+ if (map == MAP_FAILED) {
> >+ err = -errno;
> >+ goto out_mmap_rx;
> >+ }
> >+
> >+ tx->mask = xsk->config.tx_size - 1;
> >+ tx->size = xsk->config.tx_size;
> >+ tx->producer = map + off.tx.producer;
> >+ tx->consumer = map + off.tx.consumer;
> >+ tx->ring = map + off.tx.desc;
> >+ tx->cached_cons = xsk->config.tx_size;
> >+ }
> >+ xsk->tx = tx;
> >+
> >+ sxdp.sxdp_family = PF_XDP;
> >+ sxdp.sxdp_ifindex = xsk->ifindex;
> >+ sxdp.sxdp_queue_id = xsk->queue_id;
> >+ sxdp.sxdp_flags = xsk->config.bind_flags;
> >+
> >+ err = bind(xsk->fd, (struct sockaddr *)&sxdp, sizeof(sxdp));
> >+ if (err) {
> >+ err = -errno;
> >+ goto out_mmap_tx;
> >+ }
> >+
> >+ if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
> >+ err = xsk_setup_xdp_prog(xsk);
> >+ if (err)
> >+ goto out_mmap_tx;
> >+ }
> >+
> >+ *xsk_ptr = xsk;
> >+ return 0;
> >+
> >+out_mmap_tx:
> >+ if (tx)
> >+ munmap(xsk->tx,
> >+ off.tx.desc +
> >+ xsk->config.tx_size * sizeof(struct xdp_desc));
> >+out_mmap_rx:
> >+ if (rx)
> >+ munmap(xsk->rx,
> >+ off.rx.desc +
> >+ xsk->config.rx_size * sizeof(struct xdp_desc));
> >+out_socket:
> >+ if (--umem->refcount)
> >+ close(xsk->fd);
> >+out_xsk_alloc:
> >+ free(xsk);
> >+ return err;
> >+}
> >+
> >+int xsk_umem__delete(struct xsk_umem *umem)
> >+{
> >+ struct xdp_mmap_offsets off;
> >+ socklen_t optlen;
> >+ int err;
> >+
> >+ if (!umem)
> >+ return 0;
> >+
> >+ if (umem->refcount)
> >+ return -EBUSY;
> >+
> >+ optlen = sizeof(off);
> >+ err = getsockopt(umem->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen);
> >+ if (!err) {
> >+ munmap(umem->fill->ring,
> >+ off.fr.desc + umem->config.fill_size * sizeof(__u64));
> >+ munmap(umem->comp->ring,
> >+ off.cr.desc + umem->config.comp_size * sizeof(__u64));
> >+ }
> >+
> >+ close(umem->fd);
> >+ free(umem);
> >+
> >+ return 0;
> >+}
> >+
> >+void xsk_socket__delete(struct xsk_socket *xsk)
> >+{
> >+ struct xdp_mmap_offsets off;
> >+ socklen_t optlen;
> >+ int err;
> >+
> >+ if (!xsk)
> >+ return;
> >+
> >+ (void)xsk_update_bpf_maps(xsk, 0, 0);
> >+
> >+ optlen = sizeof(off);
> >+ err = getsockopt(xsk->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen);
> >+ if (!err) {
> >+ if (xsk->rx)
> >+ munmap(xsk->rx->ring,
> >+ off.rx.desc +
> >+ xsk->config.rx_size * sizeof(struct xdp_desc));
> >+ if (xsk->tx)
> >+ munmap(xsk->tx->ring,
> >+ off.tx.desc +
> >+ xsk->config.tx_size * sizeof(struct xdp_desc));
> >+ }
> >+
> >+ xsk->umem->refcount--;
> >+ /* Do not close an fd that also has an associated umem connected
> >+ * to it.
> >+ */
> >+ if (xsk->fd != xsk->umem->fd)
> >+ close(xsk->fd);
> >+ free(xsk);
> >+}
> >diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
> >new file mode 100644
> >index 0000000..a497f00
> >--- /dev/null
> >+++ b/tools/lib/bpf/xsk.h
> >@@ -0,0 +1,203 @@
> >+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> >+
> >+/*
> >+ * AF_XDP user-space access library.
> >+ *
> >+ * Copyright(c) 2018 - 2019 Intel Corporation.
> >+ *
> >+ * Author(s): Magnus Karlsson <magnus.karlsson@intel.com>
> >+ */
> >+
> >+#ifndef __LIBBPF_XSK_H
> >+#define __LIBBPF_XSK_H
> >+
> >+#include <stdio.h>
> >+#include <stdint.h>
> >+#include <linux/if_xdp.h>
> >+
> >+#include "libbpf.h"
> >+
> >+#ifdef __cplusplus
> >+extern "C" {
> >+#endif
> >+
> >+/* Do not access these members directly. Use the functions below. */
> >+#define DEFINE_XSK_RING(name) \
> >+struct name { \
> >+ __u32 cached_prod; \
> >+ __u32 cached_cons; \
> >+ __u32 mask; \
> >+ __u32 size; \
> >+ __u32 *producer; \
> >+ __u32 *consumer; \
> >+ void *ring; \
> >+}
> >+
> >+DEFINE_XSK_RING(xsk_ring_prod);
> >+DEFINE_XSK_RING(xsk_ring_cons);
> >+
> >+struct xsk_umem;
> >+struct xsk_socket;
> >+
> >+static inline __u64 *xsk_ring_prod__fill_addr(struct xsk_ring_prod *fill,
> >+ __u32 idx)
> >+{
> >+ __u64 *addrs = (__u64 *)fill->ring;
> >+
> >+ return &addrs[idx & fill->mask];
> >+}
> >+
> >+static inline const __u64 *
> >+xsk_ring_cons__comp_addr(const struct xsk_ring_cons *comp, __u32 idx)
> >+{
> >+ const __u64 *addrs = (const __u64 *)comp->ring;
> >+
> >+ return &addrs[idx & comp->mask];
> >+}
> >+
> >+static inline struct xdp_desc *xsk_ring_prod__tx_desc(struct xsk_ring_prod *tx,
> >+ __u32 idx)
> >+{
> >+ struct xdp_desc *descs = (struct xdp_desc *)tx->ring;
> >+
> >+ return &descs[idx & tx->mask];
> >+}
> >+
> >+static inline const struct xdp_desc *
> >+xsk_ring_cons__rx_desc(const struct xsk_ring_cons *rx, __u32 idx)
> >+{
> >+ const struct xdp_desc *descs = (const struct xdp_desc *)rx->ring;
> >+
> >+ return &descs[idx & rx->mask];
> >+}
> >+
> >+static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb)
> >+{
> >+ __u32 free_entries = r->cached_cons - r->cached_prod;
> >+
> >+ if (free_entries >= nb)
> >+ return free_entries;
> >+
> >+ /* Refresh the local tail pointer.
> >+ * cached_cons is r->size bigger than the real consumer pointer so
> >+ * that this addition can be avoided in the more frequently
> >+ * executed code that computs free_entries in the beginning of
> >+ * this function. Without this optimization it whould have been
> >+ * free_entries = r->cached_prod - r->cached_cons + r->size.
> >+ */
> >+ r->cached_cons = *r->consumer + r->size;
> >+
> >+ return r->cached_cons - r->cached_prod;
> >+}
> >+
> >+static inline __u32 xsk_cons_nb_avail(struct xsk_ring_cons *r, __u32 nb)
> >+{
> >+ __u32 entries = r->cached_prod - r->cached_cons;
> >+
> >+ if (entries == 0) {
> >+ r->cached_prod = *r->producer;
> >+ entries = r->cached_prod - r->cached_cons;
> >+ }
> >+
> >+ return (entries > nb) ? nb : entries;
> >+}
> >+
> >+static inline size_t xsk_ring_prod__reserve(struct xsk_ring_prod *prod,
> >+ size_t nb, __u32 *idx)
> >+{
> >+ if (unlikely(xsk_prod_nb_free(prod, nb) < nb))
> >+ return 0;
> >+
> >+ *idx = prod->cached_prod;
> >+ prod->cached_prod += nb;
> >+
> >+ return nb;
> >+}
> >+
> >+static inline void xsk_ring_prod__submit(struct xsk_ring_prod *prod, size_t nb)
> >+{
> >+ /* Make sure everything has been written to the ring before signalling
> >+ * this to the kernel.
> >+ */
> >+ smp_wmb();
> >+
> >+ *prod->producer += nb;
> >+}
> >+
> >+static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons,
> >+ size_t nb, __u32 *idx)
> >+{
> >+ size_t entries = xsk_cons_nb_avail(cons, nb);
> >+
> >+ if (likely(entries > 0)) {
> >+ /* Make sure we do not speculatively read the data before
> >+ * we have received the packet buffers from the ring.
> >+ */
> >+ smp_rmb();
> >+
> >+ *idx = cons->cached_cons;
> >+ cons->cached_cons += entries;
> >+ }
> >+
> >+ return entries;
> >+}
> >+
> >+static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb)
> >+{
> >+ *cons->consumer += nb;
> >+}
> >+
> >+static inline void *xsk_umem__get_data(void *umem_area, __u64 addr)
> >+{
> >+ return &((char *)umem_area)[addr];
> >+}
> >+
> >+LIBBPF_API int xsk_umem__fd(const struct xsk_umem *umem);
> >+LIBBPF_API int xsk_socket__fd(const struct xsk_socket *xsk);
> >+
> >+#define XSK_RING_CONS__DEFAULT_NUM_DESCS 2048
> >+#define XSK_RING_PROD__DEFAULT_NUM_DESCS 2048
> >+#define XSK_UMEM__DEFAULT_FRAME_SHIFT 11 /* 2048 bytes */
> >+#define XSK_UMEM__DEFAULT_FRAME_SIZE (1 << XSK_UMEM__DEFAULT_FRAME_SHIFT)
> >+#define XSK_UMEM__DEFAULT_FRAME_HEADROOM 0
> >+
> >+struct xsk_umem_config {
> >+ __u32 fill_size;
> >+ __u32 comp_size;
> >+ __u32 frame_size;
> >+ __u32 frame_headroom;
> >+};
> >+
> >+/* Flags for the libbpf_flags field. */
> >+#define XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD (1 << 0)
> >+
> >+struct xsk_socket_config {
> >+ __u32 rx_size;
> >+ __u32 tx_size;
> >+ __u32 libbpf_flags;
> >+ __u32 xdp_flags;
> >+ __u16 bind_flags;
> >+};
> >+
> >+/* Set config to NULL to get the default configuration. */
> >+LIBBPF_API int xsk_umem__create(struct xsk_umem **umem,
> >+ void *umem_area, __u64 size,
> >+ struct xsk_ring_prod *fill,
> >+ struct xsk_ring_cons *comp,
> >+ const struct xsk_umem_config *config);
> >+LIBBPF_API int xsk_socket__create(struct xsk_socket **xsk,
> >+ const char *ifname, __u32 queue_id,
> >+ struct xsk_umem *umem,
> >+ struct xsk_ring_cons *rx,
> >+ struct xsk_ring_prod *tx,
> >+ const struct xsk_socket_config *config);
> >+
> >+/* Returns 0 for success and -EBUSY if the umem is still in use. */
> >+LIBBPF_API int xsk_umem__delete(struct xsk_umem *umem);
> >+LIBBPF_API void xsk_socket__delete(struct xsk_socket *xsk);
> >+
> >+#ifdef __cplusplus
> >+} /* extern "C" */
> >+#endif
> >+
> >+#endif /* __LIBBPF_XSK_H */
> >--
> >2.7.4
> >
^ permalink raw reply
* Re: [PATCH net-next 3/3] nfp: devlink: allow flashing the device via devlink
From: Jiri Pirko @ 2019-02-21 7:00 UTC (permalink / raw)
To: Florian Fainelli
Cc: Jakub Kicinski, davem, netdev, oss-drivers, mkubecek, andrew
In-Reply-To: <befeae18-f80e-6f2f-3e97-28629200cf92@gmail.com>
Thu, Feb 21, 2019 at 03:59:05AM CET, f.fainelli@gmail.com wrote:
>
>
>On 2/19/2019 4:49 PM, Jakub Kicinski wrote:
>> On Tue, 19 Feb 2019 10:19:42 +0100, Jiri Pirko wrote:
>>> Fri, Feb 15, 2019 at 04:44:29PM CET, jakub.kicinski@netronome.com wrote:
>>>> On Fri, 15 Feb 2019 11:15:14 +0100, Jiri Pirko wrote:
>>>>>> static const struct ethtool_ops nfp_net_ethtool_ops = {
>>>>>
>>>>> Why don't you use the compat fallback? I think you should.
>>>>
>>>> You and Michal both asked the same so let me answer the first to ask :)
>>>> - if devlink is built as a module the fallback is not reachable.
>>>
>>> So the fallback is not really good as you can't use it for real drivers
>>> anyway. Odd. Maybe we should compile devlink in without possibility to
>>> have it as module.
>>
>> Ack, I'll make devlink a bool.
>
>Meh how about those poor and memory constrained embedded systems?
>Ideally ethtool should/could have been modular as well, but that ship
>has now sailed.
>
>>
>> I need a little extra time, I forgot that nfp's flower offload still
>> doesn't register all ports (using your port flavour infrastructure).
>>
>
>We have had similar issues with PHYLIB before where we wanted
>net/core/ethtool.c to be able to call into generic PHYLIB functions to
>obtain PHY statistics, an inline helper that de-references the PHY
>device's driver function pointers solved that (look for
>phy_ethtool_get_{strings,sset,stats}) while letting PHYLIB remain modular.
>
>devlink_compat_flash_update() is a bit big to be inlined, but why not?
Others compat functions are going to come.
>
>If we make sure we always provide a devlink_mutex and devlink_list that
>symbols such that this builds wheter CONFIG_DEVLINK=y|m then everything
>else can be determined at runtime whether devlink.ko is loaded or not.
>
>Does that make sense?
>--
>Florian
^ permalink raw reply
* Re: [patch iproute2] devlink: relax dpipe table show dependency on resources
From: Jiri Pirko @ 2019-02-21 7:02 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, dsahern, mlxsw
In-Reply-To: <20190220180117.2ad552d0@shemminger-XPS-13-9360>
Thu, Feb 21, 2019 at 03:01:17AM CET, stephen@networkplumber.org wrote:
>On Wed, 20 Feb 2019 10:21:09 +0100
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> - resource_valid = !!nla_table[DEVLINK_ATTR_DPIPE_TABLE_RESOURCE_ID];
>> + resource_valid = !!nla_table[DEVLINK_ATTR_DPIPE_TABLE_RESOURCE_ID] &&
>> + ctx->resources;
>
>When you add a logical AND operator the !! is no longer needed. The !! convention
>is used to turn a value of not zero (ie not NULL) into a boolean. When you add the
>additional operator the result is already boolean.
Okay. Will send v2.
^ permalink raw reply
* Re: [PATCH net-next 3/3] nfp: devlink: allow flashing the device via devlink
From: Michal Kubecek @ 2019-02-21 7:17 UTC (permalink / raw)
To: Florian Fainelli
Cc: Jakub Kicinski, Jiri Pirko, davem, netdev, oss-drivers, andrew
In-Reply-To: <befeae18-f80e-6f2f-3e97-28629200cf92@gmail.com>
On Wed, Feb 20, 2019 at 06:59:05PM -0800, Florian Fainelli wrote:
> On 2/19/2019 4:49 PM, Jakub Kicinski wrote:
> > On Tue, 19 Feb 2019 10:19:42 +0100, Jiri Pirko wrote:
> >> Fri, Feb 15, 2019 at 04:44:29PM CET, jakub.kicinski@netronome.com wrote:
> >>> On Fri, 15 Feb 2019 11:15:14 +0100, Jiri Pirko wrote:
> >>>>> static const struct ethtool_ops nfp_net_ethtool_ops = {
> >>>>
> >>>> Why don't you use the compat fallback? I think you should.
> >>>
> >>> You and Michal both asked the same so let me answer the first to ask :)
> >>> - if devlink is built as a module the fallback is not reachable.
> >>
> >> So the fallback is not really good as you can't use it for real drivers
> >> anyway. Odd. Maybe we should compile devlink in without possibility to
> >> have it as module.
> >
> > Ack, I'll make devlink a bool.
>
> Meh how about those poor and memory constrained embedded systems?
> Ideally ethtool should/could have been modular as well, but that ship
> has now sailed.
I would certainly like to make the ioctl interface optional once we
reach the end of "phase one", i.e. make ioctl-less ethtool possible.
Looking at the code, I don't see an obvious reason why it couldn't be
modular.
There seem to be only few functions in net/core/ethtool.c which are
called from outside and all seem to be simple helpers not really tied to
the rest of the code, except for netdev_rss_key variable (needed for
/proc/sys/net/core/netdev_rss_key). Some of them could even be inline.
We could always put these into some net/ethtool/stub.c (ethtool-stub.c)
and build unconditionally.
So if keeping the option to have devlink (and ethtool) as a module is
really desirable, I believe it can be done even now (unless I missed
something important).
> We have had similar issues with PHYLIB before where we wanted
> net/core/ethtool.c to be able to call into generic PHYLIB functions to
> obtain PHY statistics, an inline helper that de-references the PHY
> device's driver function pointers solved that (look for
> phy_ethtool_get_{strings,sset,stats}) while letting PHYLIB remain modular.
There is also something similar in netfilter - nf_ct_hook, nfnl_ct_hook
or nf_ipv6_ops.
> devlink_compat_flash_update() is a bit big to be inlined, but why not?
Most of it seems to be the lookup which I'm planning to factor out as
a separate helper. But that would also need to be available to external
code, of course.
Michal
^ permalink raw reply
* Re: [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp
From: Luciano Coelho @ 2019-02-21 7:33 UTC (permalink / raw)
To: Nathan Chancellor, Arnd Bergmann
Cc: Johannes Berg, Emmanuel Grumbach, Intel Linux Wireless,
Kalle Valo, linux-wireless, Networking, Linux Kernel Mailing List,
Nick Desaulniers
In-Reply-To: <20190220175615.GA1312@archlinux-ryzen>
On Wed, 2019-02-20 at 10:56 -0700, Nathan Chancellor wrote:
> On Wed, Feb 20, 2019 at 11:51:34AM +0100, Arnd Bergmann wrote:
> > On Tue, Feb 19, 2019 at 7:22 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-
> > > initiator.c b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-
> > > initiator.c
> > > index e9822a3ec373..92b22250eb7d 100644
> > > --- a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> > > @@ -462,7 +462,7 @@ static void iwl_mvm_debug_range_resp(struct
> > > iwl_mvm *mvm, u8 index,
> > > {
> > > s64 rtt_avg = res->ftm.rtt_avg * 100;
> > >
> > > - do_div(rtt_avg, 6666);
> > > + div64_s64(rtt_avg, 6666);
> >
> > This is wrong: div64_s64 does not modify its argument like
> > do_div(), but
> > it returns the result instead. You also don't want to divide by a
> > 64-bit
> > value when the second argument is a small constant.
> >
> > I think the correct way should be
> >
> > s64 rtt_avg = div_s64(res->ftm.rtt_avg * 100, 6666);
> >
> > If you know that the value is positive, using unsigned types
> > and div_u64() would be slightly faster.
> >
> > Arnd
>
> Thanks for the review and explanation, Arnd.
>
> Luca, could you drop this version so I can resend it?
Sure, please do! I already applied this internally, but I can just fix
it with your new patch and that will be squashed before being sent
upstream, so it will look like your second patch.
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH iproute2] ss: fix compilation under glibc < 2.18
From: Thomas De Schampheleire @ 2019-02-21 7:43 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, Thomas De Schampheleire
In-Reply-To: <20190220175345.34de8462@shemminger-XPS-13-9360>
El jue., 21 feb. 2019 a las 2:53, Stephen Hemminger
(<stephen@networkplumber.org>) escribió:
>
> On Wed, 20 Feb 2019 15:41:51 +0100
> Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
>
> > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> >
> > Commit c759116a0b2b6da8df9687b0a40ac69050132c77 introduced support for
> > AF_VSOCK. This define is only provided since glibc version 2.18, so
> > compilation fails when using older toolchains.
> >
> > Provide the necessary definitions if needed.
> >
> > Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
>
> Not sure why you would want new iproute2 with a 5 year old version of glibc?
> Yes that means update your tool chain.
>
This problem is noticed for an embedded system. It has up-to-date
applications and libraries (built via Buildroot) but the toolchain and
kernel are supplied by the SoC vendor (in this case Marvell, formerly
Cavium Networks). Unfortunately their toolchain is lagging behind and
is still using glibc 2.16.
I could handle this patch locally, but I think there may be other
people in a similar situation which would benefit from an upstream
fix.
Best regards,
Thomas
^ permalink raw reply
* Re: [PATCH] net: dsa: add missing phy address offset
From: Marcel Reichmuth @ 2019-02-21 7:53 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, vivien.didelot@gmail.com,
davem@davemloft.net
In-Reply-To: <20190220193122.GA5302@lunn.ch>
On Wed, Feb 20, 2019 at 08:31:22PM +0100, Andrew Lunn wrote:
> On Wed, Feb 20, 2019 at 11:27:16AM -0800, Florian Fainelli wrote:
> > On 2/20/19 10:15 AM, Marcel Reichmuth wrote:
> >
> > You are supposed to describe the port to PHY mapping using the binding,
> > so for instance:
> >
> > ports {
> > port@0 {
> > reg = <0>;
> > phy-handle = <&phy1>;
> > };
> >
> > };
> >
> > mdio {
> > phy1: phy@1 {
> > reg = <1>;
> > };
> > };
> >
> > etc. is not that working for you?
>
>
> The Espressobin does exactly this:
>
> arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
>
> It also uses the 6341.
>
Thank you very much for your hints. Yes that works indeed too. I
just assumed it was intended to work automatically with the
built-in phys as it does with the other switches I am using.
^ permalink raw reply
* Re: [Bridge] [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries
From: Hangbin Liu @ 2019-02-21 8:01 UTC (permalink / raw)
To: Linus Lüssing
Cc: Nikolay Aleksandrov, netdev, roopa, bridge, davem, yinxu,
Sebastian Gottschall
In-Reply-To: <20181213161027.GC1713@otheros>
Hi Linus,
Sorry, my mail client somehow droped your message and I didn't see your reply.
I find this mail after Nikolay pointed out yesterday.
> Hi and thanks for your reply!
>
> On Fri, Dec 14, 2018 at 10:32:16AM +0800, Ying Xu wrote:
> > I think the scenario mentioned above is abnormal.
>
> Can we agree, that this scenario, if switch A and B were using the
> current bridge code, has issues right now which it did
> not have before that patch?
Yes, I agree. But this "regression" could be fixed by setting up correct
switch configuration. See more explains below.
>
> I also do not quite understand what you mean with "abnormal". Do
> you think that it is unlikely to have two snooping switches and
> general queries with a 0.0.0.0 source?
The "abnormal" means we shouldn't use this kind of configuration on
snooping switch.
We could have general queries with a 0.0.0.0 source, but these queries
are only from proxy Querier, not from real Querier.
Based on RFC 4541,
2.1.1. IGMP Forwarding Rules
b) The arrival port for IGMP Queries (sent by multicast routers)
where the source address is not 0.0.0.0.
The 0.0.0.0 address represents a special case where the switch
is proxying IGMP Queries for faster network convergence, but is
not itself the Querier. The switch does not use its own IP
address (even if it has one), because this would cause the
Queries to be seen as coming from a newly elected Querier. The
0.0.0.0 address is used to indicate that the Query packets are
NOT from a multicast router.
This paragraph specified "0.0.0.0" is only for switch proxying queries.
It should not be used as a IGMP Querier.
"because this would cause the Queries to be seen as coming from a newly
elected Querier" means other address could be elected as a Querier but
"0.0.0.0" should not.
AFAIK, most verdors(Cisco, HW, etc. expect H3C) use none-zero address
as default address on proxying switches. But H3C also respect the
b) section.
>
> Note that with the current bridge code and according to RFC3376
> and RFC2236, as soon as a query with a 0.0.0.0 source is sent somewhere
> in the broadcast domain, it will become the selected querier [*].
Yes, In RFC 3376
6.6.2. Querier Election
IGMPv3 elects a single querier per subnet using the same querier
election mechanism as IGMPv2, namely by IP address. When a router
receives a query with a lower IP address, it sets the Other-Querier-
Present timer to Other Querier Present Interval and ceases to send
queries on the network if it was the previously elected querier.
After its Other-Querier Present timer expires, it should begin
sending General Queries.
It only said we should select lower IP address as Querier, but didn't specify
the "0.0.0.0" address.
While in the later RFC 4541
Abstract
This memo describes the recommendations for Internet Group Management
Protocol (IGMP) and Multicast Listener Discovery (MLD) snooping
switches. These are based on best current practices for IGMPv2, with
further considerations for IGMPv3- and MLDv2-snooping.
And section 2.1.1. IGMP Forwarding Rules(I just pasted above). It did
specify the "0.0.0.0" address issue.
I think we should respect the later and accurater standard, shouldn't we?
>
>
> > The source of query indicats that is a real router or only a switch.(0.0.0.0
> > means switch,non-zero means router).
> > In the scenario above,the switch A was selected to be a querier that means A
> > performs as a router,
> > so switch A should config its query source address to non-zero,and then Host A
> > can recieve the traffic from B.
>
> Even if in the described scenario switch A were configured to use a a non-zero
> source address to become a router, so that switch B would mark the port
> to switch A as a multicast router port, switch A would still loose
> in the querier election right now, as 0.0.0.0 is lower (RFC3376, RFC2236). So
> switch B would then become the selected querier with its 0.0.0.0 source
> and switch A would become silent even though it had a non-zero
> source address.
>
> And then we would have the same issue again, only swapped between
> host+switch A and host+switch B.
>
> Would you agree, does that make sense?
>
Yes, that's why Ying Xu said the topology is "abnormal", or illegal. Because
in a correct configured topology, there should has a Querier with none-zero
IP address.
> Regards, Linus
>
>
> [*]: Looking at br_ip4_multicast_select_querier():
>
> If previously selected querier were 0.0.0.0, it would accept any
> source as a new querier ("!br->ip4_querier.addr.u.ip4"). However,
> if the previously selected querier were non-zero, a query with
> 0.0.0.0 would win, too
> ("ntohl(saddr) <= ntohl(br->ip4_querier.addr.u.ip4)").
In RFC 3810
7.6.2. Querier Election
When a router starts operating on a subnet, by default it
considers itself as being the Querier.
When a router receives a query with a lower IPv6 address than its
own, it sets the Other Querier Present timer to Other Querier Present
Timeout; if it was previously in Querier state, it switches to Non-
Querier state and ceases to send queries on the link.
All MLDv2 queries MUST be sent with the FE80::/64 link-local source
address prefix.
MLDv2 also sepcified the default Querier is itself and electe a lower adddress
as new Querier. And the source address should be link-loacl source address
instead of 0("::"). This is another evidence that we should not use "0.0.0.0"
as a Querier. So I think we should fix the IPv4 election in function
br_ip4_multicast_select_querier().
This looks like a little extreme and may have "regression", but the "regression"
should be fixed by setting up correct router/switch configuration.
What do you think?
Regards
Hangbin
^ permalink raw reply
* [PATCH v2] iwlwifi: mvm: Use div_s64 instead of do_div in iwl_mvm_debug_range_resp
From: Nathan Chancellor @ 2019-02-21 8:06 UTC (permalink / raw)
To: Johannes Berg, Emmanuel Grumbach, Luca Coelho,
Intel Linux Wireless, Kalle Valo
Cc: linux-wireless, netdev, linux-kernel, Nick Desaulniers,
Nathan Chancellor, Arnd Bergmann
In-Reply-To: <20190219182105.19933-1-natechancellor@gmail.com>
Clang warns:
drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c:465:2: warning:
comparison of distinct pointer types ('typeof ((rtt_avg)) *' (aka 'long
long *') and 'uint64_t *' (aka 'unsigned long long *'))
[-Wcompare-distinct-pointer-types]
do_div(rtt_avg, 6666);
^~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
(void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
1 warning generated.
do_div expects an unsigned dividend. Use div_s64, which expects a signed
dividend.
Fixes: 937b10c0de68 ("iwlwifi: mvm: add debug prints for FTM")
Link: https://github.com/ClangBuiltLinux/linux/issues/372
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
v1 -> v2:
* Fix logic (as the return value of div{,64}_s64 must be used), thanks
to Arnd for the review.
drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
index e9822a3ec373..94132cfd1f56 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
@@ -460,9 +460,7 @@ static int iwl_mvm_ftm_range_resp_valid(struct iwl_mvm *mvm, u8 request_id,
static void iwl_mvm_debug_range_resp(struct iwl_mvm *mvm, u8 index,
struct cfg80211_pmsr_result *res)
{
- s64 rtt_avg = res->ftm.rtt_avg * 100;
-
- do_div(rtt_avg, 6666);
+ s64 rtt_avg = div_s64(res->ftm.rtt_avg * 100, 6666);
IWL_DEBUG_INFO(mvm, "entry %d\n", index);
IWL_DEBUG_INFO(mvm, "\tstatus: %d\n", res->status);
--
2.21.0.rc1
^ permalink raw reply related
* [RFC] coallocating struct socket and struct socket_wq
From: Al Viro @ 2019-02-21 8:15 UTC (permalink / raw)
To: netdev
All instances of struct socket are embedded into some
bigger structure - most into struct socket_alloc (with struct inode
following struct socket), some into struct tun_file or struct
tap_queue.
In the last two cases the corresponding struct socket_wq
(the one whose address goes into socket->wq) is in the same
containing structure, right after struct socket.
In case of struct socket_alloc, we allocate struct
socket_wq separately and set socket->wq before anyone sees
either (in sock_alloc_inode()). In sock_destroy_inode()
they are both freed (struct sock_alloc immediately,
struct socket_wq - RCU-delayed).
AFAICS, nothing ever reassigns socket->wq. Could we
simply embed struct socket_wq into struct socket? RCU delay
is not an issue - net/socket.c is non-modular, so call_rcu()-based
variant freeing both together is not horrible. sock_alloc_inode()
would be simplified, tun/tap uses would simply lose the separate
socket_wq members.
The only problem I see here is ____cacheline_aligned_in_smp
we have on struct socket_wq. Could we simply make it the first
field in struct socket? Without lockdep they are reasonably small -
on amd64 socket_wq is 64 bytes, while the rest of struct socket is
48 (and pointer to wq would obviously disappear).
Or is there something subtle I'm missing here? What I
have in mind is something along the lines of
diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index c0b52e48f0e6..7aedc748fbd0 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -519,8 +519,7 @@ static int tap_open(struct inode *inode, struct file *file)
goto err;
}
- RCU_INIT_POINTER(q->sock.wq, &q->wq);
- init_waitqueue_head(&q->wq.wait);
+ init_waitqueue_head(&q->sock.wq.wait);
q->sock.type = SOCK_RAW;
q->sock.state = SS_CONNECTED;
q->sock.file = file;
@@ -578,7 +577,7 @@ static __poll_t tap_poll(struct file *file, poll_table *wait)
goto out;
mask = 0;
- poll_wait(file, &q->wq.wait, wait);
+ poll_wait(file, &q->sock.wq.wait, wait);
if (!ptr_ring_empty(&q->ring))
mask |= EPOLLIN | EPOLLRDNORM;
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index fed298c0cb39..51f023391998 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -169,7 +169,6 @@ struct tun_pcpu_stats {
struct tun_file {
struct sock sk;
struct socket socket;
- struct socket_wq wq;
struct tun_struct __rcu *tun;
struct fasync_struct *fasync;
/* only used for fasnyc */
@@ -2166,7 +2165,7 @@ static void *tun_ring_recv(struct tun_file *tfile, int noblock, int *err)
goto out;
}
- add_wait_queue(&tfile->wq.wait, &wait);
+ add_wait_queue(&tfile->socket.wq.wait, &wait);
current->state = TASK_INTERRUPTIBLE;
while (1) {
@@ -2186,7 +2185,7 @@ static void *tun_ring_recv(struct tun_file *tfile, int noblock, int *err)
}
current->state = TASK_RUNNING;
- remove_wait_queue(&tfile->wq.wait, &wait);
+ remove_wait_queue(&tfile->socket.wq.wait, &wait);
out:
*err = error;
@@ -3409,8 +3408,7 @@ static int tun_chr_open(struct inode *inode, struct file * file)
tfile->flags = 0;
tfile->ifindex = 0;
- init_waitqueue_head(&tfile->wq.wait);
- RCU_INIT_POINTER(tfile->socket.wq, &tfile->wq);
+ init_waitqueue_head(&tfile->socket.wq.wait);
tfile->socket.file = file;
tfile->socket.ops = &tun_socket_ops;
diff --git a/include/linux/if_tap.h b/include/linux/if_tap.h
index 8e66866c11be..915a187cfabd 100644
--- a/include/linux/if_tap.h
+++ b/include/linux/if_tap.h
@@ -62,7 +62,6 @@ struct tap_dev {
struct tap_queue {
struct sock sk;
struct socket sock;
- struct socket_wq wq;
int vnet_hdr_sz;
struct tap_dev __rcu *tap;
struct file *file;
diff --git a/include/linux/net.h b/include/linux/net.h
index e0930678c8bf..e6e0d7858c78 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -108,13 +108,13 @@ struct socket_wq {
* @wq: wait queue for several uses
*/
struct socket {
+ struct socket_wq wq;
socket_state state;
short type;
unsigned long flags;
- struct socket_wq *wq;
struct file *file;
struct sock *sk;
diff --git a/include/net/sock.h b/include/net/sock.h
index 6679f3c120b0..c05c08487900 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1805,7 +1805,7 @@ static inline void sock_graft(struct sock *sk, struct socket *parent)
{
WARN_ON(parent->sk);
write_lock_bh(&sk->sk_callback_lock);
- rcu_assign_pointer(sk->sk_wq, parent->wq);
+ rcu_assign_pointer(sk->sk_wq, &parent->wq);
parent->sk = sk;
sk_set_socket(sk, parent);
sk->sk_uid = SOCK_INODE(parent)->i_uid;
@@ -2089,7 +2089,7 @@ static inline void sock_poll_wait(struct file *filp, struct socket *sock,
poll_table *p)
{
if (!poll_does_not_wait(p)) {
- poll_wait(filp, &sock->wq->wait, p);
+ poll_wait(filp, &sock->wq.wait, p);
/* We need to be sure we are in sync with the
* socket flags modification.
*
diff --git a/net/core/sock.c b/net/core/sock.c
index 71ded4d8025c..d097f981e7d8 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2808,7 +2808,7 @@ void sock_init_data(struct socket *sock, struct sock *sk)
if (sock) {
sk->sk_type = sock->type;
- sk->sk_wq = sock->wq;
+ sk->sk_wq = &sock->wq;
sock->sk = sk;
sk->sk_uid = SOCK_INODE(sock)->i_uid;
} else {
diff --git a/net/socket.c b/net/socket.c
index 643a1648fcc2..03cf4128b3ba 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -239,20 +239,13 @@ static struct kmem_cache *sock_inode_cachep __ro_after_init;
static struct inode *sock_alloc_inode(struct super_block *sb)
{
struct socket_alloc *ei;
- struct socket_wq *wq;
ei = kmem_cache_alloc(sock_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
- wq = kmalloc(sizeof(*wq), GFP_KERNEL);
- if (!wq) {
- kmem_cache_free(sock_inode_cachep, ei);
- return NULL;
- }
- init_waitqueue_head(&wq->wait);
- wq->fasync_list = NULL;
- wq->flags = 0;
- ei->socket.wq = wq;
+ init_waitqueue_head(&ei->socket.wq.wait);
+ ei->socket.wq.fasync_list = NULL;
+ ei->socket.wq.flags = 0;
ei->socket.state = SS_UNCONNECTED;
ei->socket.flags = 0;
@@ -263,15 +256,18 @@ static struct inode *sock_alloc_inode(struct super_block *sb)
return &ei->vfs_inode;
}
-static void sock_destroy_inode(struct inode *inode)
+static void sock_destroy_inode_callback(struct rcu_head *head)
{
- struct socket_alloc *ei;
-
- ei = container_of(inode, struct socket_alloc, vfs_inode);
- kfree_rcu(ei->socket.wq, rcu);
+ struct socket_alloc *ei = container_of(head, struct socket_alloc,
+ vfs_inode.i_rcu);
kmem_cache_free(sock_inode_cachep, ei);
}
+static void sock_destroy_inode(struct inode *inode)
+{
+ call_rcu(&inode->i_rcu, sock_destroy_inode_callback);
+}
+
static void init_once(void *foo)
{
struct socket_alloc *ei = (struct socket_alloc *)foo;
@@ -583,7 +579,7 @@ static void __sock_release(struct socket *sock, struct inode *inode)
module_put(owner);
}
- if (sock->wq->fasync_list)
+ if (sock->wq.fasync_list)
pr_err("%s: fasync list not empty!\n", __func__);
if (!sock->file) {
@@ -1183,7 +1179,7 @@ static int sock_fasync(int fd, struct file *filp, int on)
return -EINVAL;
lock_sock(sk);
- wq = sock->wq;
+ wq = &sock->wq;
fasync_helper(fd, filp, on, &wq->fasync_list);
if (!wq->fasync_list)
^ permalink raw reply related
* pull request (net): ipsec 2019-02-21
From: Steffen Klassert @ 2019-02-21 8:22 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev
1) Don't do TX bytes accounting for the esp trailer when sending
from a request socket as this will result in an out of bounds
memory write. From Martin Willi.
2) Destroy xfrm_state synchronously on net exit path to
avoid nested gc flush callbacks that may trigger a
warning in xfrm6_tunnel_net_exit(). From Cong Wang.
3) Do an unconditionally clone in pfkey_broadcast_one()
to avoid a race when freeing the skb.
From Sean Tranchetti.
4) Fix inbound traffic via XFRM interfaces across network
namespaces. We did the lookup for interfaces and policies
in the wrong namespace. From Tobias Brunner.
Please pull or let me know if there are problems.
Thanks!
The following changes since commit 6fb6e6371f8c463020a41cc0ed1915e140219c3d:
net: dsa: mv88e6xxx: Fix serdes irq setup going recursive (2019-01-27 23:19:19 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git master
for you to fetch changes up to 660899ddf06ae8bb5bbbd0a19418b739375430c5:
xfrm: Fix inbound traffic via XFRM interfaces across network namespaces (2019-02-18 10:58:54 +0100)
----------------------------------------------------------------
Cong Wang (1):
xfrm: destroy xfrm_state synchronously on net exit path
Martin Willi (1):
esp: Skip TX bytes accounting when sending from a request socket
Sean Tranchetti (1):
af_key: unconditionally clone on broadcast
Tobias Brunner (1):
xfrm: Fix inbound traffic via XFRM interfaces across network namespaces
include/net/xfrm.h | 12 +++++++++---
net/ipv4/esp4.c | 2 +-
net/ipv6/esp6.c | 2 +-
net/ipv6/xfrm6_tunnel.c | 2 +-
net/key/af_key.c | 42 ++++++++++++++++--------------------------
net/xfrm/xfrm_interface.c | 4 ++--
net/xfrm/xfrm_policy.c | 4 +++-
net/xfrm/xfrm_state.c | 30 +++++++++++++++++++-----------
net/xfrm/xfrm_user.c | 2 +-
9 files changed, 53 insertions(+), 47 deletions(-)
^ permalink raw reply
* [PATCH 3/4] af_key: unconditionally clone on broadcast
From: Steffen Klassert @ 2019-02-21 8:22 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev
In-Reply-To: <20190221082204.10134-1-steffen.klassert@secunet.com>
From: Sean Tranchetti <stranche@codeaurora.org>
Attempting to avoid cloning the skb when broadcasting by inflating
the refcount with sock_hold/sock_put while under RCU lock is dangerous
and violates RCU principles. It leads to subtle race conditions when
attempting to free the SKB, as we may reference sockets that have
already been freed by the stack.
Unable to handle kernel paging request at virtual address 6b6b6b6b6b6c4b
[006b6b6b6b6b6c4b] address between user and kernel address ranges
Internal error: Oops: 96000004 [#1] PREEMPT SMP
task: fffffff78f65b380 task.stack: ffffff8049a88000
pc : sock_rfree+0x38/0x6c
lr : skb_release_head_state+0x6c/0xcc
Process repro (pid: 7117, stack limit = 0xffffff8049a88000)
Call trace:
sock_rfree+0x38/0x6c
skb_release_head_state+0x6c/0xcc
skb_release_all+0x1c/0x38
__kfree_skb+0x1c/0x30
kfree_skb+0xd0/0xf4
pfkey_broadcast+0x14c/0x18c
pfkey_sendmsg+0x1d8/0x408
sock_sendmsg+0x44/0x60
___sys_sendmsg+0x1d0/0x2a8
__sys_sendmsg+0x64/0xb4
SyS_sendmsg+0x34/0x4c
el0_svc_naked+0x34/0x38
Kernel panic - not syncing: Fatal exception
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Sean Tranchetti <stranche@codeaurora.org>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
net/key/af_key.c | 40 +++++++++++++++-------------------------
1 file changed, 15 insertions(+), 25 deletions(-)
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 637030f43b67..5651c29cb5bd 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -196,30 +196,22 @@ static int pfkey_release(struct socket *sock)
return 0;
}
-static int pfkey_broadcast_one(struct sk_buff *skb, struct sk_buff **skb2,
- gfp_t allocation, struct sock *sk)
+static int pfkey_broadcast_one(struct sk_buff *skb, gfp_t allocation,
+ struct sock *sk)
{
int err = -ENOBUFS;
- sock_hold(sk);
- if (*skb2 == NULL) {
- if (refcount_read(&skb->users) != 1) {
- *skb2 = skb_clone(skb, allocation);
- } else {
- *skb2 = skb;
- refcount_inc(&skb->users);
- }
- }
- if (*skb2 != NULL) {
- if (atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf) {
- skb_set_owner_r(*skb2, sk);
- skb_queue_tail(&sk->sk_receive_queue, *skb2);
- sk->sk_data_ready(sk);
- *skb2 = NULL;
- err = 0;
- }
+ if (atomic_read(&sk->sk_rmem_alloc) > sk->sk_rcvbuf)
+ return err;
+
+ skb = skb_clone(skb, allocation);
+
+ if (skb) {
+ skb_set_owner_r(skb, sk);
+ skb_queue_tail(&sk->sk_receive_queue, skb);
+ sk->sk_data_ready(sk);
+ err = 0;
}
- sock_put(sk);
return err;
}
@@ -234,7 +226,6 @@ static int pfkey_broadcast(struct sk_buff *skb, gfp_t allocation,
{
struct netns_pfkey *net_pfkey = net_generic(net, pfkey_net_id);
struct sock *sk;
- struct sk_buff *skb2 = NULL;
int err = -ESRCH;
/* XXX Do we need something like netlink_overrun? I think
@@ -253,7 +244,7 @@ static int pfkey_broadcast(struct sk_buff *skb, gfp_t allocation,
* socket.
*/
if (pfk->promisc)
- pfkey_broadcast_one(skb, &skb2, GFP_ATOMIC, sk);
+ pfkey_broadcast_one(skb, GFP_ATOMIC, sk);
/* the exact target will be processed later */
if (sk == one_sk)
@@ -268,7 +259,7 @@ static int pfkey_broadcast(struct sk_buff *skb, gfp_t allocation,
continue;
}
- err2 = pfkey_broadcast_one(skb, &skb2, GFP_ATOMIC, sk);
+ err2 = pfkey_broadcast_one(skb, GFP_ATOMIC, sk);
/* Error is cleared after successful sending to at least one
* registered KM */
@@ -278,9 +269,8 @@ static int pfkey_broadcast(struct sk_buff *skb, gfp_t allocation,
rcu_read_unlock();
if (one_sk != NULL)
- err = pfkey_broadcast_one(skb, &skb2, allocation, one_sk);
+ err = pfkey_broadcast_one(skb, allocation, one_sk);
- kfree_skb(skb2);
kfree_skb(skb);
return err;
}
--
2.17.1
^ permalink raw reply related
* [PATCH 1/4] esp: Skip TX bytes accounting when sending from a request socket
From: Steffen Klassert @ 2019-02-21 8:22 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev
In-Reply-To: <20190221082204.10134-1-steffen.klassert@secunet.com>
From: Martin Willi <martin@strongswan.org>
On ESP output, sk_wmem_alloc is incremented for the added padding if a
socket is associated to the skb. When replying with TCP SYNACKs over
IPsec, the associated sk is a casted request socket, only. Increasing
sk_wmem_alloc on a request socket results in a write at an arbitrary
struct offset. In the best case, this produces the following WARNING:
WARNING: CPU: 1 PID: 0 at lib/refcount.c:102 esp_output_head+0x2e4/0x308 [esp4]
refcount_t: addition on 0; use-after-free.
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.0.0-rc3 #2
Hardware name: Marvell Armada 380/385 (Device Tree)
[...]
[<bf0ff354>] (esp_output_head [esp4]) from [<bf1006a4>] (esp_output+0xb8/0x180 [esp4])
[<bf1006a4>] (esp_output [esp4]) from [<c05dee64>] (xfrm_output_resume+0x558/0x664)
[<c05dee64>] (xfrm_output_resume) from [<c05d07b0>] (xfrm4_output+0x44/0xc4)
[<c05d07b0>] (xfrm4_output) from [<c05956bc>] (tcp_v4_send_synack+0xa8/0xe8)
[<c05956bc>] (tcp_v4_send_synack) from [<c0586ad8>] (tcp_conn_request+0x7f4/0x948)
[<c0586ad8>] (tcp_conn_request) from [<c058c404>] (tcp_rcv_state_process+0x2a0/0xe64)
[<c058c404>] (tcp_rcv_state_process) from [<c05958ac>] (tcp_v4_do_rcv+0xf0/0x1f4)
[<c05958ac>] (tcp_v4_do_rcv) from [<c0598a4c>] (tcp_v4_rcv+0xdb8/0xe20)
[<c0598a4c>] (tcp_v4_rcv) from [<c056eb74>] (ip_protocol_deliver_rcu+0x2c/0x2dc)
[<c056eb74>] (ip_protocol_deliver_rcu) from [<c056ee6c>] (ip_local_deliver_finish+0x48/0x54)
[<c056ee6c>] (ip_local_deliver_finish) from [<c056eecc>] (ip_local_deliver+0x54/0xec)
[<c056eecc>] (ip_local_deliver) from [<c056efac>] (ip_rcv+0x48/0xb8)
[<c056efac>] (ip_rcv) from [<c0519c2c>] (__netif_receive_skb_one_core+0x50/0x6c)
[...]
The issue triggers only when not using TCP syncookies, as for syncookies
no socket is associated.
Fixes: cac2661c53f3 ("esp4: Avoid skb_cow_data whenever possible")
Fixes: 03e2a30f6a27 ("esp6: Avoid skb_cow_data whenever possible")
Signed-off-by: Martin Willi <martin@strongswan.org>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
net/ipv4/esp4.c | 2 +-
net/ipv6/esp6.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index 5459f41fc26f..10e809b296ec 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -328,7 +328,7 @@ int esp_output_head(struct xfrm_state *x, struct sk_buff *skb, struct esp_info *
skb->len += tailen;
skb->data_len += tailen;
skb->truesize += tailen;
- if (sk)
+ if (sk && sk_fullsock(sk))
refcount_add(tailen, &sk->sk_wmem_alloc);
goto out;
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index 5afe9f83374d..239d4a65ad6e 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -296,7 +296,7 @@ int esp6_output_head(struct xfrm_state *x, struct sk_buff *skb, struct esp_info
skb->len += tailen;
skb->data_len += tailen;
skb->truesize += tailen;
- if (sk)
+ if (sk && sk_fullsock(sk))
refcount_add(tailen, &sk->sk_wmem_alloc);
goto out;
--
2.17.1
^ permalink raw reply related
* [PATCH 4/4] xfrm: Fix inbound traffic via XFRM interfaces across network namespaces
From: Steffen Klassert @ 2019-02-21 8:22 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev
In-Reply-To: <20190221082204.10134-1-steffen.klassert@secunet.com>
From: Tobias Brunner <tobias@strongswan.org>
After moving an XFRM interface to another namespace it stays associated
with the original namespace (net in `struct xfrm_if` and the list keyed
with `xfrmi_net_id`), allowing processes in the new namespace to use
SAs/policies that were created in the original namespace. For instance,
this allows a keying daemon in one namespace to establish IPsec SAs for
other namespaces without processes there having access to the keys or IKE
credentials.
This worked fine for outbound traffic, however, for inbound traffic the
lookup for the interfaces and the policies used the incorrect namespace
(the one the XFRM interface was moved to).
Fixes: f203b76d7809 ("xfrm: Add virtual xfrm interfaces")
Signed-off-by: Tobias Brunner <tobias@strongswan.org>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
net/xfrm/xfrm_interface.c | 4 ++--
net/xfrm/xfrm_policy.c | 4 +++-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
index 6be8c7df15bb..dbb3c1945b5c 100644
--- a/net/xfrm/xfrm_interface.c
+++ b/net/xfrm/xfrm_interface.c
@@ -76,10 +76,10 @@ static struct xfrm_if *xfrmi_decode_session(struct sk_buff *skb)
int ifindex;
struct xfrm_if *xi;
- if (!skb->dev)
+ if (!secpath_exists(skb) || !skb->dev)
return NULL;
- xfrmn = net_generic(dev_net(skb->dev), xfrmi_net_id);
+ xfrmn = net_generic(xs_net(xfrm_input_state(skb)), xfrmi_net_id);
ifindex = skb->dev->ifindex;
for_each_xfrmi_rcu(xfrmn->xfrmi[0], xi) {
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index ba0a4048c846..8d1a898d0ba5 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3314,8 +3314,10 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
if (ifcb) {
xi = ifcb->decode_session(skb);
- if (xi)
+ if (xi) {
if_id = xi->p.if_id;
+ net = xi->net;
+ }
}
rcu_read_unlock();
--
2.17.1
^ permalink raw reply related
* [PATCH 2/4] xfrm: destroy xfrm_state synchronously on net exit path
From: Steffen Klassert @ 2019-02-21 8:22 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev
In-Reply-To: <20190221082204.10134-1-steffen.klassert@secunet.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
xfrm_state_put() moves struct xfrm_state to the GC list
and schedules the GC work to clean it up. On net exit call
path, xfrm_state_flush() is called to clean up and
xfrm_flush_gc() is called to wait for the GC work to complete
before exit.
However, this doesn't work because one of the ->destructor(),
ipcomp_destroy(), schedules the same GC work again inside
the GC work. It is hard to wait for such a nested async
callback. This is also why syzbot still reports the following
warning:
WARNING: CPU: 1 PID: 33 at net/ipv6/xfrm6_tunnel.c:351 xfrm6_tunnel_net_exit+0x2cb/0x500 net/ipv6/xfrm6_tunnel.c:351
...
ops_exit_list.isra.0+0xb0/0x160 net/core/net_namespace.c:153
cleanup_net+0x51d/0xb10 net/core/net_namespace.c:551
process_one_work+0xd0c/0x1ce0 kernel/workqueue.c:2153
worker_thread+0x143/0x14a0 kernel/workqueue.c:2296
kthread+0x357/0x430 kernel/kthread.c:246
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352
In fact, it is perfectly fine to bypass GC and destroy xfrm_state
synchronously on net exit call path, because it is in process context
and doesn't need a work struct to do any blocking work.
This patch introduces xfrm_state_put_sync() which simply bypasses
GC, and lets its callers to decide whether to use this synchronous
version. On net exit path, xfrm_state_fini() and
xfrm6_tunnel_net_exit() use it. And, as ipcomp_destroy() itself is
blocking, it can use xfrm_state_put_sync() directly too.
Also rename xfrm_state_gc_destroy() to ___xfrm_state_destroy() to
reflect this change.
Fixes: b48c05ab5d32 ("xfrm: Fix warning in xfrm6_tunnel_net_exit.")
Reported-and-tested-by: syzbot+e9aebef558e3ed673934@syzkaller.appspotmail.com
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
include/net/xfrm.h | 12 +++++++++---
net/ipv6/xfrm6_tunnel.c | 2 +-
net/key/af_key.c | 2 +-
net/xfrm/xfrm_state.c | 30 +++++++++++++++++++-----------
net/xfrm/xfrm_user.c | 2 +-
5 files changed, 31 insertions(+), 17 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 7298a53b9702..85386becbaea 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -853,7 +853,7 @@ static inline void xfrm_pols_put(struct xfrm_policy **pols, int npols)
xfrm_pol_put(pols[i]);
}
-void __xfrm_state_destroy(struct xfrm_state *);
+void __xfrm_state_destroy(struct xfrm_state *, bool);
static inline void __xfrm_state_put(struct xfrm_state *x)
{
@@ -863,7 +863,13 @@ static inline void __xfrm_state_put(struct xfrm_state *x)
static inline void xfrm_state_put(struct xfrm_state *x)
{
if (refcount_dec_and_test(&x->refcnt))
- __xfrm_state_destroy(x);
+ __xfrm_state_destroy(x, false);
+}
+
+static inline void xfrm_state_put_sync(struct xfrm_state *x)
+{
+ if (refcount_dec_and_test(&x->refcnt))
+ __xfrm_state_destroy(x, true);
}
static inline void xfrm_state_hold(struct xfrm_state *x)
@@ -1590,7 +1596,7 @@ struct xfrmk_spdinfo {
struct xfrm_state *xfrm_find_acq_byseq(struct net *net, u32 mark, u32 seq);
int xfrm_state_delete(struct xfrm_state *x);
-int xfrm_state_flush(struct net *net, u8 proto, bool task_valid);
+int xfrm_state_flush(struct net *net, u8 proto, bool task_valid, bool sync);
int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_valid);
void xfrm_sad_getinfo(struct net *net, struct xfrmk_sadinfo *si);
void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
diff --git a/net/ipv6/xfrm6_tunnel.c b/net/ipv6/xfrm6_tunnel.c
index f5b4febeaa25..bc65db782bfb 100644
--- a/net/ipv6/xfrm6_tunnel.c
+++ b/net/ipv6/xfrm6_tunnel.c
@@ -344,8 +344,8 @@ static void __net_exit xfrm6_tunnel_net_exit(struct net *net)
struct xfrm6_tunnel_net *xfrm6_tn = xfrm6_tunnel_pernet(net);
unsigned int i;
- xfrm_state_flush(net, IPSEC_PROTO_ANY, false);
xfrm_flush_gc();
+ xfrm_state_flush(net, IPSEC_PROTO_ANY, false, true);
for (i = 0; i < XFRM6_TUNNEL_SPI_BYADDR_HSIZE; i++)
WARN_ON_ONCE(!hlist_empty(&xfrm6_tn->spi_byaddr[i]));
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 655c787f9d54..637030f43b67 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1783,7 +1783,7 @@ static int pfkey_flush(struct sock *sk, struct sk_buff *skb, const struct sadb_m
if (proto == 0)
return -EINVAL;
- err = xfrm_state_flush(net, proto, true);
+ err = xfrm_state_flush(net, proto, true, false);
err2 = unicast_flush_resp(sk, hdr);
if (err || err2) {
if (err == -ESRCH) /* empty table - go quietly */
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 23c92891758a..1bb971f46fc6 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -432,7 +432,7 @@ void xfrm_state_free(struct xfrm_state *x)
}
EXPORT_SYMBOL(xfrm_state_free);
-static void xfrm_state_gc_destroy(struct xfrm_state *x)
+static void ___xfrm_state_destroy(struct xfrm_state *x)
{
tasklet_hrtimer_cancel(&x->mtimer);
del_timer_sync(&x->rtimer);
@@ -474,7 +474,7 @@ static void xfrm_state_gc_task(struct work_struct *work)
synchronize_rcu();
hlist_for_each_entry_safe(x, tmp, &gc_list, gclist)
- xfrm_state_gc_destroy(x);
+ ___xfrm_state_destroy(x);
}
static enum hrtimer_restart xfrm_timer_handler(struct hrtimer *me)
@@ -598,14 +598,19 @@ struct xfrm_state *xfrm_state_alloc(struct net *net)
}
EXPORT_SYMBOL(xfrm_state_alloc);
-void __xfrm_state_destroy(struct xfrm_state *x)
+void __xfrm_state_destroy(struct xfrm_state *x, bool sync)
{
WARN_ON(x->km.state != XFRM_STATE_DEAD);
- spin_lock_bh(&xfrm_state_gc_lock);
- hlist_add_head(&x->gclist, &xfrm_state_gc_list);
- spin_unlock_bh(&xfrm_state_gc_lock);
- schedule_work(&xfrm_state_gc_work);
+ if (sync) {
+ synchronize_rcu();
+ ___xfrm_state_destroy(x);
+ } else {
+ spin_lock_bh(&xfrm_state_gc_lock);
+ hlist_add_head(&x->gclist, &xfrm_state_gc_list);
+ spin_unlock_bh(&xfrm_state_gc_lock);
+ schedule_work(&xfrm_state_gc_work);
+ }
}
EXPORT_SYMBOL(__xfrm_state_destroy);
@@ -708,7 +713,7 @@ xfrm_dev_state_flush_secctx_check(struct net *net, struct net_device *dev, bool
}
#endif
-int xfrm_state_flush(struct net *net, u8 proto, bool task_valid)
+int xfrm_state_flush(struct net *net, u8 proto, bool task_valid, bool sync)
{
int i, err = 0, cnt = 0;
@@ -730,7 +735,10 @@ int xfrm_state_flush(struct net *net, u8 proto, bool task_valid)
err = xfrm_state_delete(x);
xfrm_audit_state_delete(x, err ? 0 : 1,
task_valid);
- xfrm_state_put(x);
+ if (sync)
+ xfrm_state_put_sync(x);
+ else
+ xfrm_state_put(x);
if (!err)
cnt++;
@@ -2215,7 +2223,7 @@ void xfrm_state_delete_tunnel(struct xfrm_state *x)
if (atomic_read(&t->tunnel_users) == 2)
xfrm_state_delete(t);
atomic_dec(&t->tunnel_users);
- xfrm_state_put(t);
+ xfrm_state_put_sync(t);
x->tunnel = NULL;
}
}
@@ -2375,8 +2383,8 @@ void xfrm_state_fini(struct net *net)
unsigned int sz;
flush_work(&net->xfrm.state_hash_work);
- xfrm_state_flush(net, IPSEC_PROTO_ANY, false);
flush_work(&xfrm_state_gc_work);
+ xfrm_state_flush(net, IPSEC_PROTO_ANY, false, true);
WARN_ON(!list_empty(&net->xfrm.state_all));
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index c6d26afcf89d..a131f9ff979e 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1932,7 +1932,7 @@ static int xfrm_flush_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
struct xfrm_usersa_flush *p = nlmsg_data(nlh);
int err;
- err = xfrm_state_flush(net, p->proto, true);
+ err = xfrm_state_flush(net, p->proto, true, false);
if (err) {
if (err == -ESRCH) /* empty table */
return 0;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2] kcm: remove any offset before parsing messages
From: Dominique Martinet @ 2019-02-21 8:22 UTC (permalink / raw)
To: Tom Herbert
Cc: Tom Herbert, David Miller, Doron Roberts-Kedes, Dave Watson,
Linux Kernel Network Developers, LKML
In-Reply-To: <CALx6S35jPN+E7-A4JK9ypAETophtKrJzjd9HPmXowU7RMS=bcA@mail.gmail.com>
Tom Herbert wrote on Wed, Feb 20, 2019:
> > When the client closes the socket, some messages are obviously still "in
> > flight", and the server will recv a POLLERR notification on the csock at
> > some point with many messages left.
> > The documentation says to unattach the csock when you get POLLER. If I
> > do that, the kcm socket will no longer give me any message, so all the
> > messages still in flight at the time are lost.
>
> So basically it sounds like you're interested in supporting TCP
> connections that are half closed. I believe that the error in half
> closed is EPIPE, so if the TCP socket returns that it can be ignored
> and the socket can continue being attached and used to send data.
Did you mean 'can continue being attached and used to receive data'?
I can confirm getsockopt with SO_ERROR gets me EPIPE, but I don't see
how to efficiently ignore EPIPE until POLLIN gets unset -- polling on
both the csock and kcm socket will do many needless wakeups on only the
csock from what I can see, so I'd need some holdoff timer or something.
I guess it's possible though.
> Another possibility is to add some linger semantics to an attached
> socket. For instance, a large message might be sent so that part of
> the messge is queued in TCP and part is queued in the KCM socket.
> Unattach would probably break that message. We probably want to linger
> option similar to SO_LINGER (or maybe just use the option on the TCP
> socket) that means don't complete the detach until any message being
> transmitted on the lower socket has been queued.
That would certainly work, even if non-obvious from a user standpoint.
> > > I'd like to see some retry on ENOMEM before this is merged though, so
> > > while I'm there I'll resend this with a second patch doing that
> > > retry,.. I think just not setting strp->interrupted and not reporting
> > > the error up might be enough? Will have to try either way.
> >
> > I also tried playing with that without much success.
> > I had assumed just not calling strp_parser_err() (which calls the
> > abort_parser cb) would be enough, eventually calling strp_start_timer()
> > like the !len case, but no can do.
>
> I think you need to ignore the ENOMEM and have a timer or other
> callback to retry the operation in the future.
Yes, that's what I had intended to try; basically just break out and
schedule timer as said above.
After a bit more debugging, this part works (__strp_recv() is called
again); but the next packet that is treated properly is rejected because
by the time __strp_recv() was called again a new skb was read and the
length isn't large enough to go all the way into the new packet, so this
test fails:
} else if (len <= (ssize_t)head->len -
skb->len - stm->strp.offset) {
/* Length must be into new skb (and also
* greater than zero)
*/
STRP_STATS_INCR(strp->stats.bad_hdr_len);
strp_parser_err(strp, -EPROTO, desc);
So I need to figure a way to say "call this function again without
reading more data" somehow, or make this check more lax e.g. accept any
len > 0 after a retry maybe...
Removing that branch altogether seems to work at least but I'm not sure
we'd want to?
(grmbl at this slow VM and strparser not being possible to enable as a
module, it takes ages to test)
> > With that said, returning 0 from the parse function also raises POLLERR
> > on the csock and hangs netparser, so things aren't that simple...
>
> Can you point to where this is happening. If the parse_msg callback
> returns 0 that is suppose to indicate that more bytes are needed.
I just blindly returned 0 "from time to time" in the kcm parser
function, but looking at above it was failing on the same check.
This somewhat makes sense for this one to fail here if a new packet was
read, no sane parser function should give a length smaller than what
they require to determine the length.
Thanks,
--
Dominique
^ permalink raw reply
* [PATCH] bpf: test_bpf: turn of preemption in function __run_once
From: Anders Roxell @ 2019-02-21 8:44 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-kernel, Anders Roxell
When running test seccomp_bpf the following splat occurs:
[ RUN ] global.secseccomp_bpf.c:2136:global.detect_seccomp_filter_flags:Expected 22 (22) == (*__errno_location ()) (14)
seccomp_bpf.c:2138:global.detect_seccomp_filter_flags:Failed to detect that an unknown
filter flag (0x8) is unsupported! Does a new flag need to be added to this test?
[ 2155.677841] BUG: assuming atomic context at kernel/seccomp.c:271
[ 2155.689351] in_atomic(): 0, irqs_disabled(): 0, pid: 28540, name: seccomp_bpf
[ 2155.696597] INFO: lockdep is turned off.
[ 2155.700605] CPU: 5 PID: 28540 Comm: seccomp_bpf Tainted: G W 5.0.0-rc7-next-20190220 #1
[ 2155.709972] Hardware name: HiKey Development Board (DT)
[ 2155.715232] Call trace:
[ 2155.717710] dump_backtrace+0x0/0x160
[ 2155.721399] show_stack+0x24/0x30
[ 2155.724742] dump_stack+0xc8/0x114
[ 2155.728172] __cant_sleep+0xf0/0x108
[ 2155.731777] __seccomp_filter+0x8c/0x5c8
[ 2155.735727] __secure_computing+0x4c/0xe8
[ 2155.739767] syscall_trace_enter+0xf8/0x2b8
[ 2155.743982] el0_svc_common+0xf0/0x130
[ 2155.747758] el0_svc_handler+0x38/0x78
[ 2155.751534] el0_svc+0x8/0xc
Rework so that preemption is disabled when we loop over function
'BPF_PROG_RUN(...)'.
Commit 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
highlighted the issue.
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
---
lib/test_bpf.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index f3e570722a7e..0845f635f404 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -6668,12 +6668,14 @@ static int __run_one(const struct bpf_prog *fp, const void *data,
u64 start, finish;
int ret = 0, i;
+ preempt_disable();
start = ktime_get_ns();
for (i = 0; i < runs; i++)
ret = BPF_PROG_RUN(fp, data);
finish = ktime_get_ns();
+ preempt_enable();
*duration = finish - start;
do_div(*duration, runs);
--
2.11.0
^ permalink raw reply related
* Re: Handling an Extra Signal at PHY Reset
From: Paul Kocialkowski @ 2019-02-21 8:50 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, Heiner Kallweit, netdev, Thomas Petazzoni,
Mylène Josserand
In-Reply-To: <20190221014938.GQ14879@lunn.ch>
Hi Andrew,
On Thu, 2019-02-21 at 02:49 +0100, Andrew Lunn wrote:
> On Tue, Feb 19, 2019 at 10:14:20AM +0100, Paul Kocialkowski wrote:
> > Hi,
> >
> > We are dealing with an Ethernet PHY (Marvell 88E1512) that comes with a
> > CONFIG pin that must be connected to one of the other pins of the PHY
> > to configure the LSB of the PHY address as well as I/O voltages (see
> > section 2.18.1 Hardware Configuration of the datasheet). It must be
> > connected "soon after reset" for the PHY to be correctly configured.
>
> Hi Paul
>
> Turns out the datasheet is publicly available.
>
> So you can at run-time configure the voltage. Page 2, register 24, bit
> 13.
>
> So back to my last question. Can you address the PHY without using the
> switch? Even if it has the wrong voltage?
>
> If you can, you could set the correct voltage in the probe() function.
Thanks for looking into our issue :)
I did some more investigating in the meantime, and the hardware logic
actually connects our CONFIG and LED pins when the controlling GPIO is
open-drain.
I can also confirm that it does not prevent contacting the PHY on the
MDIO bus, contrary to what I have stated previously.
So the important step for us to do is to disconnect the CONFIG and LED
pins (at least so we can see our LED blink properly) once the PHY was
reset. But we can't really rely on the fact that the pins were
connected before PHY reset (e.g. U-Boot may have disconnected them
already to use Ethernet) so we still need a way to connect them before
the PHY reset from the MDIO bus core hits, and disconnect them after
that.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH bpf-next v2] bpf, seccomp: fix false positive preemption splat for cbpf->ebpf progs
From: Daniel Borkmann @ 2019-02-21 8:53 UTC (permalink / raw)
To: Kees Cook, Alexei Starovoitov
Cc: Jann Horn, Andy Lutomirski, Alexei Starovoitov,
Network Development
In-Reply-To: <CAGXu5jLHV78-d+_yTKVsgawDAVn7FXZVCcvhtDesXcxhZf_NKg@mail.gmail.com>
On 02/21/2019 06:31 AM, Kees Cook wrote:
> On Wed, Feb 20, 2019 at 8:03 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Wed, Feb 20, 2019 at 3:59 PM Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>>>
>>> On Thu, Feb 21, 2019 at 12:01:35AM +0100, Daniel Borkmann wrote:
>>>> In 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
>>>> a check was added for BPF_PROG_RUN() that for every invocation preemption is
>>>> disabled to not break eBPF assumptions (e.g. per-cpu map). Of course this does
>>>> not count for seccomp because only cBPF -> eBPF is loaded here and it does
>>>> not make use of any functionality that would require this assertion. Fix this
>>>> false positive by adding and using SECCOMP_RUN() variant that does not have
>>>> the cant_sleep(); check.
>>>>
>>>> Fixes: 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
>>>> Reported-by: syzbot+8bf19ee2aa580de7a2a7@syzkaller.appspotmail.com
>>>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>>>> Acked-by: Kees Cook <keescook@chromium.org>
>>>
>>> Applied, Thanks
>>
>> Actually I think it's a wrong approach to go long term.
>> I'm thinking to revert it.
>> I think it's better to disable preemption for duration of
>> seccomp cbpf prog.
>> It's short and there is really no reason for it to be preemptible.
>> When seccomp switches to ebpf we'll have this weird inconsistency.
>> Let's just disable preemption for seccomp as well.
>
> A lot of changes will be needed for seccomp ebpf -- not the least of
> which is convincing me there is a use-case. ;)
>
> But the main issue is that I'm not a huge fan of dropping two
> barriers() across syscall entry. That seems pretty heavy-duty for
> something that is literally not needed right now.
Yeah, I think it's okay to add once actually technically needed. Last
time I looked, if I recall correctly, at least Chrome installs some
heavy duty seccomp programs that go close to prog limit.
Thanks,
Daniel
^ permalink raw reply
* Re: Handling an Extra Signal at PHY Reset
From: Paul Kocialkowski @ 2019-02-21 9:05 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, Heiner Kallweit
Cc: netdev, Thomas Petazzoni, Mylène Josserand
In-Reply-To: <06B5A7DA-55DC-4F58-8078-D44A5230BBEA@gmail.com>
Hi Florian,
On Tue, 2019-02-19 at 08:07 -0800, Florian Fainelli wrote:
>
> On February 19, 2019 1:14:20 AM PST, Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> >
> > We are dealing with an Ethernet PHY (Marvell 88E1512) that comes with a
> > CONFIG pin that must be connected to one of the other pins of the PHY
> > to configure the LSB of the PHY address as well as I/O voltages (see
> > section 2.18.1 Hardware Configuration of the datasheet). It must be
> > connected "soon after reset" for the PHY to be correctly configured.
>
> Even voltage? What guarantees do you have that you are not reducing
> the lifetime of your pads if e.g.: you are configured for 3.3V while
> the other end is 1.8/2.5V? Is there some kind of embedded voltage
> comparator that can be used to help making the right decision?
I'm really not sure about that. We know from the hardware design that
we need 1.8V/2.5V.
Unlike what I stated previously, the two pins are actually connected
when the controlling GPIO is open-drain and what we really need to do
is to disconnect them after reset so that both pins can be used for
their individual functions. But we can't be sure that they were not
already disconnected before by e.g. U-Boot, so we should also make sure
they are connected before issuing reset.
> > We have a switch for connecting the CONFIG pin to the other pin (LED0),
> > which needs to be controlled by Linux. The CONFIG pin seems to be used
> > for a PTP clock the rest of the time.
> >
> > So we are wondering how to properly represent this case, especially on
> > the device-tree side.
> >
> > The trick here is that this step is necessary before the PHY can be
> > discovered on the MDIO bus (and thus the PHY driver selected) so we
> > can't rely on the PHY driver to do this. Basically, it looks like we
> > need to handle this like the reset pin and describe it at the MDIO bus
> > level.
> >
> > Here are some ideas for potential solutions:
> > - Allowing more than a single GPIO to be passed to the MDIO bus' reset-
> > gpios via device-tree and toggling all the passed GPIOs at once;
>
> That would be a mis-representstion of the HW though, since the reset
> line is tied to the PHY package. Making use of the current
> implementation details to put a second reset line does not sound
> great.
Agreed, this is quite a far-fetched solution and in the end, what we
are controlling is not a reset line.
> > - Adding a new optional GPIO for the MDIO bus dedicated to controlling
> > switches for such config switching, perhaps called "config-gpios"
> > (quite a narrow solution);
>
> Indeed, and still has the same design flaw as 1) outline above.
>
> > - Adding a broader power sequence description to the MDIO bus (a bit
> > like it's done with the mmc pwrseq descriptions) which would allow
> > specifying the toggle order/delays of various GPIOs (would probably be
> > the most extensive solution);
>
> That one looks the most compelling and future proof although I do
> wonder how many people would make use of that.
About that one, it turns out that what we need to do has little to do
with power sequencing and more to do with configuration at reset time,
so it doesn't feel like a great fit either.
> > - Adding the extra GPIO control to the MAC description and toggling it
> > through bus->reset (probably the less invasive solution for the core
> > but not very satisfying from the description perspective, since this is
> > definitely not MAC-specific).
> >
> > What do you think about how we could solve this issue?
> > Do you see other options that I missed here?
>
> You could explore having the MDIO bus layer scan its children nodes
> (PHY nodes) and handle properties in there before registering
> devices, so for insurance your PHY DT nodes can have an arbitrary
> number of reset lines, power sequencing properties etc. and the MDIO
> bus layer knowing it's internal implementation does make sure that it
> makes use of these properties in order to make PHY devices
> functional.
>
> Does that make sense? One possible caveat is that the CONFIG pin also
> dictates the address on the bus, so what do we do with the PHY's
> "reg" property, is it it's actual address or is it the desired one
> that we should configure through reset?
That sounds like a good option to me as it would allow keeping the
property on the PHY node. I think having a "config-gpios" property on
the PHY node would work. Then the MDIO core can just walk the children
nodes, set this GPIO before reset, issue PHY reset and then unset the
GPIO.
Maybe we could also allow having the reset-gpios property on the PHY
node? As far as I understand, this property on the MDIO bus was
described as a bus-wide reset, but it seems to be used for PHY reset
instead most of the time (which is not really an accurate description
of the hardware).
I think we should keep the PHY's configured address in the description,
since the PHY will not change its address, but use the one set by the
CONFIG pin from reset.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH RFC 3/5] sched/cpufreq: Fix incorrect RCU API usage
From: Peter Zijlstra @ 2019-02-21 9:18 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ido Schimmel,
Ingo Molnar, moderated list:INTEL ETHERNET DRIVERS,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, Lai Jiangshan,
Martin KaFai Lau, Mathieu Desnoyers, netdev, Paul E. McKenney,
rcu, Song Liu, Steven Rostedt, xdp-newbies, Yonghong Song
In-Reply-To: <20190221054942.132388-4-joel@joelfernandes.org>
On Thu, Feb 21, 2019 at 12:49:40AM -0500, Joel Fernandes (Google) wrote:
> @@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data,
> if (WARN_ON(!data || !func))
> return;
>
> - if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu)))
> + rcu_read_lock();
> + if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) {
> + rcu_read_unlock();
> return;
> + }
> + rcu_read_unlock();
>
> data->func = func;
> rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data);
This doesn't make any kind of sense to me.
^ permalink raw reply
* Re: [PATCH RFC 4/5] sched/topology: Annonate RCU pointers properly
From: Peter Zijlstra @ 2019-02-21 9:19 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ido Schimmel,
Ingo Molnar, moderated list:INTEL ETHERNET DRIVERS,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, Lai Jiangshan,
Martin KaFai Lau, Mathieu Desnoyers, netdev, Paul E. McKenney,
rcu, Song Liu, Steven Rostedt, xdp-newbies, Yonghong Song
In-Reply-To: <20190221054942.132388-5-joel@joelfernandes.org>
On Thu, Feb 21, 2019 at 12:49:41AM -0500, Joel Fernandes (Google) wrote:
> Also replace rcu_assign_pointer call on rq->sd with WRITE_ONCE. This
> should be sufficient for the rq->sd initialization.
> @@ -668,7 +668,7 @@ cpu_attach_domain(struct sched_domain *sd, struct root_domain *rd, int cpu)
>
> rq_attach_root(rq, rd);
> tmp = rq->sd;
> - rcu_assign_pointer(rq->sd, sd);
> + WRITE_ONCE(rq->sd, sd);
> dirty_sched_domain_sysctl(cpu);
> destroy_sched_domains(tmp);
Where did the RELEASE barrier go?
That was a publish operation, now it is not.
^ permalink raw reply
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