* Re: [PATCH bpf-next v2 2/8] libbpf: Add a helper for retrieving a prog via index
From: Maciej Fijalkowski @ 2019-01-23 13:41 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: ast, netdev, jakub.kicinski, brouer
In-Reply-To: <91d162e0-3d15-c1d8-1e80-8d0a4f561540@iogearbox.net>
On Wed, 23 Jan 2019 11:41:11 +0100
Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:
> > xdp_redirect_cpu has a 6 different XDP programs that can be attached to
> > network interface. This sample has a option --prognum that allows user
> > for specifying which particular program from a given set will be
> > attached to network interface.
> > In order to make it easier when converting the mentioned sample to
> > libbpf usage, add a function to libbpf that will return program's fd for
> > a given index.
> >
> > Note that there is already a bpf_object__find_prog_by_idx, which could
> > be exported and might be used for that purpose, but it operates on the
> > number of ELF section and here we need an index from a programs array
> > within the bpf_object.
>
> Series in general looks good to me. Few minor comments, mainly in relation
> to the need for libbpf extensions.
>
> Would it not be a better interface to the user to instead choose the prog
> based on section name and then retrieve it via bpf_object__find_program_by_title()
> instead of prognum (which feels less user friendly) at least?
>
I couldn't decide which one from:
* adding a libbpf helper
* changing the xdp_redirect_cpu behaviour
would be more invasive when I was converting this sample to libbpf support.
Your suggestion sounds good, but I'm wondering about the actual implementation.
I suppose that we would choose the program via command line. Some program
section names in this sample are a bit long and it might be irritating for
user to type in for example "xdp_cpu_map5_lb_hash_ip_pairs", no? Or maybe we
can live with this. In case of typo and program being not found it would be
good to print out the section names to choose from in usage().
> > Signed-off-by: Maciej Fijalkowski <maciejromanfijalkowski@gmail.com>
> > Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > ---
> > tools/lib/bpf/libbpf.c | 8 ++++++++
> > tools/lib/bpf/libbpf.h | 3 +++
> > tools/lib/bpf/libbpf.map | 1 +
> > 3 files changed, 12 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index dc838bea403f..21c84d0f6128 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -935,6 +935,14 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
> > return err;
> > }
> >
> > +int
> > +bpf_object__get_prog_fd_by_num(struct bpf_object *obj, int idx)
> > +{
> > + if (idx >= 0 && idx < obj->nr_programs)
> > + return bpf_program__fd(&obj->programs[idx]);
> > + return -ENOENT;
> > +}
> > +
> > static struct bpf_program *
> > bpf_object__find_prog_by_idx(struct bpf_object *obj, int idx)
> > {
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 7f10d36abdde..ca1b381cb3ad 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -95,6 +95,9 @@ LIBBPF_API int bpf_object__btf_fd(const struct bpf_object *obj);
> > LIBBPF_API struct bpf_program *
> > bpf_object__find_program_by_title(struct bpf_object *obj, const char *title);
> >
> > +LIBBPF_API int
> > +bpf_object__get_prog_fd_by_num(struct bpf_object *obj, int idx);
> > +
> > LIBBPF_API struct bpf_object *bpf_object__next(struct bpf_object *prev);
> > #define bpf_object__for_each_safe(pos, tmp) \
> > for ((pos) = bpf_object__next(NULL), \
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 7c59e4f64082..871d2fc07150 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -127,4 +127,5 @@ LIBBPF_0.0.1 {
> > LIBBPF_0.0.2 {
> > global:
> > bpf_object__find_map_fd_by_name;
> > + bpf_object__get_prog_fd_by_num;
> > } LIBBPF_0.0.1;
> >
>
^ permalink raw reply
* Re: [PATCH 1/3] treewide: Lift switch variables out of switches
From: William Kucharski @ 2019-01-23 13:21 UTC (permalink / raw)
To: Jann Horn
Cc: Greg KH, Kees Cook, kernel list, Ard Biesheuvel, Laura Abbott,
Alexander Popov, xen-devel, dri-devel, intel-gfx, intel-wired-lan,
Network Development, linux-usb, linux-fsdevel, Linux-MM, dev,
linux-kbuild, linux-security-module, Kernel Hardening
In-Reply-To: <CAG48ez2vfXkr9dozJiGmze8k49VOXfs=K7M8bv0aQsDDpzrEFQ@mail.gmail.com>
> On Jan 23, 2019, at 5:09 AM, Jann Horn <jannh@google.com> wrote:
>
> AFAICS this only applies to switch statements (because they jump to a
> case and don't execute stuff at the start of the block), not blocks
> after if/while/... .
It bothers me that we are going out of our way to deprecate valid C constructs
in favor of placing the declarations elsewhere.
As current compiler warnings would catch any reference before initialization
usage anyway, it seems like we are letting a compiler warning rather than the
language standard dictate syntax.
Certainly if we want to make it a best practice coding style issue we can, and
then an appropriate note explaining why should be added to
Documentation/process/coding-style.rst.
^ permalink raw reply
* Re: [PATCH bpf-next 3/3] xsk: add sock_diag interface for AF_XDP
From: Daniel Borkmann @ 2019-01-23 13:19 UTC (permalink / raw)
To: bjorn.topel, ast, netdev
Cc: Björn Töpel, magnus.karlsson, magnus.karlsson
In-Reply-To: <20190118130305.11504-4-bjorn.topel@gmail.com>
On 01/18/2019 02:03 PM, bjorn.topel@gmail.com wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
>
> This patch adds the sock_diag interface for querying sockets from user
> space. Tools like iproute2 ss(8) can use this interface to list open
> AF_XDP sockets.
>
> The user-space ABI is defined in linux/xdp_diag.h and includes netlink
> request and response structs. The request can query sockets and the
> response contains socket information about the rings, umems, inode and
> more.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
Series looks good, few minor nits inline:
> ---
> include/uapi/linux/xdp_diag.h | 72 +++++++++++++
> net/xdp/Kconfig | 8 ++
> net/xdp/Makefile | 1 +
> net/xdp/xsk.c | 6 +-
> net/xdp/xsk.h | 12 +++
> net/xdp/xsk_diag.c | 192 ++++++++++++++++++++++++++++++++++
> 6 files changed, 286 insertions(+), 5 deletions(-)
> create mode 100644 include/uapi/linux/xdp_diag.h
> create mode 100644 net/xdp/xsk.h
> create mode 100644 net/xdp/xsk_diag.c
>
> diff --git a/include/uapi/linux/xdp_diag.h b/include/uapi/linux/xdp_diag.h
> new file mode 100644
> index 000000000000..efe8ce281dce
> --- /dev/null
> +++ b/include/uapi/linux/xdp_diag.h
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * xdp_diag: interface for query/monitor XDP sockets
> + * Copyright(c) 2019 Intel Corporation.
> + */
> +
> +#ifndef _LINUX_XDP_DIAG_H
> +#define _LINUX_XDP_DIAG_H
> +
> +#include <linux/types.h>
> +
> +struct xdp_diag_req {
> + __u8 sdiag_family;
> + __u8 sdiag_protocol;
> + __u16 pad;
Presumably this one is for future use? Maybe better as '__u16 :16;' to
avoid compile errors if someone tries to zero 'pad' member manually?
> + __u32 xdiag_ino;
> + __u32 xdiag_show;
> + __u32 xdiag_cookie[2];
> +};
> +
> +struct xdp_diag_msg {
> + __u8 xdiag_family;
> + __u8 xdiag_type;
> + __u32 xdiag_ino;
> + __u32 xdiag_cookie[2];
> +};
> +
> +#define XDP_SHOW_INFO (1 << 0) /* Basic information */
> +#define XDP_SHOW_RING_CFG (1 << 1)
> +#define XDP_SHOW_UMEM (1 << 2)
> +#define XDP_SHOW_MEMINFO (1 << 3)
> +
> +enum {
> + XDP_DIAG_NONE,
> + XDP_DIAG_INFO,
> + XDP_DIAG_UID,
> + XDP_DIAG_RX_RING,
> + XDP_DIAG_TX_RING,
> + XDP_DIAG_UMEM,
> + XDP_DIAG_UMEM_FILL_RING,
> + XDP_DIAG_UMEM_COMPLETION_RING,
> + XDP_DIAG_MEMINFO,
> + __XDP_DIAG_MAX,
> +};
> +
> +#define XDP_DIAG_MAX (__XDP_DIAG_MAX - 1)
> +
> +struct xdp_diag_info {
> + __u32 ifindex;
> + __u32 queue_id;
> +};
> +
> +struct xdp_diag_ring {
> + __u32 entries; /*num descs */
> +};
> +
> +#define XDP_DU_F_ZEROCOPY (1 << 0)
> +
> +struct xdp_diag_umem {
> + __u64 size;
> + __u32 id;
> + __u32 num_pages;
> + __u32 chunk_size;
> + __u32 headroom;
> + __u32 ifindex;
> + __u32 queue_id;
> + __u32 flags;
> + __u32 refs;
> +};
> +
> +
Nit: one newline too much
> +#endif /* _LINUX_XDP_DIAG_H */
> diff --git a/net/xdp/Kconfig b/net/xdp/Kconfig
> index 90e4a7152854..0255b33cff4b 100644
> --- a/net/xdp/Kconfig
> +++ b/net/xdp/Kconfig
> @@ -5,3 +5,11 @@ config XDP_SOCKETS
> help
> XDP sockets allows a channel between XDP programs and
> userspace applications.
> +
> +config XDP_SOCKETS_DIAG
> + tristate "XDP sockets: monitoring interface"
> + depends on XDP_SOCKETS
> + default n
> + help
> + Support for PF_XDP sockets monitoring interface used by the ss tool.
> + If unsure, say Y.
> diff --git a/net/xdp/Makefile b/net/xdp/Makefile
> index 04f073146256..59dbfdf93dca 100644
> --- a/net/xdp/Makefile
> +++ b/net/xdp/Makefile
> @@ -1 +1,2 @@
> obj-$(CONFIG_XDP_SOCKETS) += xsk.o xdp_umem.o xsk_queue.o
> +obj-$(CONFIG_XDP_SOCKETS_DIAG) += xsk_diag.o
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 80ca48cefc42..949d3bbccb2f 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -27,14 +27,10 @@
>
> #include "xsk_queue.h"
> #include "xdp_umem.h"
> +#include "xsk.h"
>
> #define TX_BATCH_SIZE 16
>
> -static struct xdp_sock *xdp_sk(struct sock *sk)
> -{
> - return (struct xdp_sock *)sk;
> -}
> -
> bool xsk_is_setup_for_bpf_map(struct xdp_sock *xs)
> {
> return READ_ONCE(xs->rx) && READ_ONCE(xs->umem) &&
> diff --git a/net/xdp/xsk.h b/net/xdp/xsk.h
> new file mode 100644
> index 000000000000..ba8120610426
> --- /dev/null
> +++ b/net/xdp/xsk.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2019 Intel Corporation. */
> +
> +#ifndef XSK_H_
> +#define XSK_H_
> +
> +static inline struct xdp_sock *xdp_sk(struct sock *sk)
> +{
> + return (struct xdp_sock *)sk;
> +}
> +
> +#endif /* XSK_H_ */
> diff --git a/net/xdp/xsk_diag.c b/net/xdp/xsk_diag.c
> new file mode 100644
> index 000000000000..2585d7a4ac2f
> --- /dev/null
> +++ b/net/xdp/xsk_diag.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* XDP sockets monitoring support
> + *
> + * Copyright(c) 2019 Intel Corporation.
> + *
> + * Author: Björn Töpel <bjorn.topel@intel.com>
> + */
> +
> +#include <linux/module.h>
> +#include <net/xdp_sock.h>
> +#include <linux/xdp_diag.h>
> +#include <linux/sock_diag.h>
> +
> +#include "xsk_queue.h"
> +#include "xsk.h"
> +
> +static int xsk_diag_put_info(const struct xdp_sock *xs, struct sk_buff *nlskb)
> +{
> + struct xdp_diag_info di;
Not a bug, but can we generally zero all the structs on stack such that once
we extend them nothing gets potentially leaked.
> + di.ifindex = xs->dev ? xs->dev->ifindex : 0;
> + di.queue_id = xs->queue_id;
> + return nla_put(nlskb, XDP_DIAG_INFO, sizeof(di), &di);
> +}
> +
> +static int xsk_diag_put_ring(const struct xsk_queue *queue, int nl_type,
> + struct sk_buff *nlskb)
> +{
> + struct xdp_diag_ring dr;
Ditto.
> + dr.entries = queue->nentries;
> + return nla_put(nlskb, nl_type, sizeof(dr), &dr);
> +}
> +
> +static int xsk_diag_put_rings_cfg(const struct xdp_sock *xs,
> + struct sk_buff *nlskb)
> +{
> + int err = 0;
> +
> + if (xs->rx)
> + err = xsk_diag_put_ring(xs->rx, XDP_DIAG_RX_RING, nlskb);
> + if (!err && xs->tx)
> + err = xsk_diag_put_ring(xs->tx, XDP_DIAG_TX_RING, nlskb);
> + return err;
> +}
> +
> +static int xsk_diag_put_umem(const struct xdp_sock *xs, struct sk_buff *nlskb)
> +{
> + struct xdp_umem *umem = xs->umem;
> + struct xdp_diag_umem du;
Ditto
> + int err;
> +
> + if (!umem)
> + return 0;
> +
> + du.id = umem->id;
> + du.size = umem->size;
> + du.num_pages = umem->npgs;
> + du.chunk_size = (__u32)(~umem->chunk_mask + 1);
> + du.headroom = umem->headroom;
> + du.ifindex = umem->dev ? umem->dev->ifindex : 0;
> + du.queue_id = umem->queue_id;
> + du.flags = 0;
> + if (umem->zc)
> + du.flags |= XDP_DU_F_ZEROCOPY;
> + du.refs = refcount_read(&umem->users);
> +
> + err = nla_put(nlskb, XDP_DIAG_UMEM, sizeof(du), &du);
> +
> + if (!err && umem->fq)
> + err = xsk_diag_put_ring(xs->tx, XDP_DIAG_UMEM_FILL_RING, nlskb);
> + if (!err && umem->cq) {
> + err = xsk_diag_put_ring(xs->tx, XDP_DIAG_UMEM_COMPLETION_RING,
> + nlskb);
> + }
> + return err;
> +}
> +
> +static int xsk_diag_fill(struct sock *sk, struct sk_buff *nlskb,
> + struct xdp_diag_req *req,
> + struct user_namespace *user_ns,
> + u32 portid, u32 seq, u32 flags, int sk_ino)
> +{
> + struct xdp_sock *xs = xdp_sk(sk);
> + struct xdp_diag_msg *msg;
> + struct nlmsghdr *nlh;
> +
> + nlh = nlmsg_put(nlskb, portid, seq, SOCK_DIAG_BY_FAMILY, sizeof(*msg),
> + flags);
> + if (!nlh)
> + return -EMSGSIZE;
> +
> + msg = nlmsg_data(nlh);
> + msg->xdiag_family = AF_XDP;
> + msg->xdiag_type = sk->sk_type;
> + msg->xdiag_ino = sk_ino;
> + sock_diag_save_cookie(sk, msg->xdiag_cookie);
Don't we have a hole in struct xdp_diag_msg after xdiag_type? Probably better
to memset everything in the beginning as well.
> + if ((req->xdiag_show & XDP_SHOW_INFO) && xsk_diag_put_info(xs, nlskb))
> + goto out_nlmsg_trim;
> +
> + if ((req->xdiag_show & XDP_SHOW_INFO) &&
> + nla_put_u32(nlskb, XDP_DIAG_UID,
> + from_kuid_munged(user_ns, sock_i_uid(sk))))
> + goto out_nlmsg_trim;
> +
> + if ((req->xdiag_show & XDP_SHOW_RING_CFG) &&
> + xsk_diag_put_rings_cfg(xs, nlskb))
> + goto out_nlmsg_trim;
> +
> + if ((req->xdiag_show & XDP_SHOW_UMEM) &&
> + xsk_diag_put_umem(xs, nlskb))
> + goto out_nlmsg_trim;
> +
> + if ((req->xdiag_show & XDP_SHOW_MEMINFO) &&
> + sock_diag_put_meminfo(sk, nlskb, XDP_DIAG_MEMINFO))
> + goto out_nlmsg_trim;
> +
> + nlmsg_end(nlskb, nlh);
> + return 0;
> +
> +out_nlmsg_trim:
> + nlmsg_cancel(nlskb, nlh);
> + return -EMSGSIZE;
> +}
> +
> +static int xsk_diag_dump(struct sk_buff *nlskb, struct netlink_callback *cb)
> +{
> + struct xdp_diag_req *req = nlmsg_data(cb->nlh);
> + struct net *net = sock_net(nlskb->sk);
> + int num = 0, s_num = cb->args[0];
> + struct sock *sk;
> +
> + mutex_lock(&net->xdp.lock);
> +
> + sk_for_each(sk, &net->xdp.list) {
> + if (!net_eq(sock_net(sk), net))
> + continue;
> + if (num++ < s_num)
> + continue;
> +
> + if (xsk_diag_fill(sk, nlskb, req,
> + sk_user_ns(NETLINK_CB(cb->skb).sk),
> + NETLINK_CB(cb->skb).portid,
> + cb->nlh->nlmsg_seq, NLM_F_MULTI,
> + sock_i_ino(sk)) < 0) {
> + num--;
> + break;
> + }
> + }
> +
> + mutex_unlock(&net->xdp.lock);
> + cb->args[0] = num;
> + return nlskb->len;
> +}
> +
> +static int xsk_diag_handler_dump(struct sk_buff *nlskb, struct nlmsghdr *hdr)
> +{
> + struct netlink_dump_control c = { .dump = xsk_diag_dump };
> + int hdrlen = sizeof(struct xdp_diag_req);
> + struct net *net = sock_net(nlskb->sk);
> + struct xdp_diag_req *req;
> +
> + if (nlmsg_len(hdr) < hdrlen)
> + return -EINVAL;
> +
> + if (!(hdr->nlmsg_flags & NLM_F_DUMP))
> + return -EOPNOTSUPP;
> +
> + req = nlmsg_data(hdr);
> + return netlink_dump_start(net->diag_nlsk, nlskb, hdr, &c);
> +}
> +
> +static const struct sock_diag_handler xsk_diag_handler = {
> + .family = AF_XDP,
> + .dump = xsk_diag_handler_dump,
> +};
> +
> +static int __init xsk_diag_init(void)
> +{
> + return sock_diag_register(&xsk_diag_handler);
> +}
> +
> +static void __exit xsk_diag_exit(void)
> +{
> + sock_diag_unregister(&xsk_diag_handler);
> +}
> +
> +module_init(xsk_diag_init);
> +module_exit(xsk_diag_exit);
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_NET_PF_PROTO_TYPE(PF_NETLINK, NETLINK_SOCK_DIAG, AF_XDP);
>
^ permalink raw reply
* Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct
From: Andrew Lunn @ 2019-01-23 13:15 UTC (permalink / raw)
To: Igor Russkikh; +Cc: David S . Miller, netdev@vger.kernel.org, Nikita Danilov
In-Reply-To: <1bac2c2c-2c60-5043-3b74-0b20138a72c1@aquantia.com>
On Wed, Jan 23, 2019 at 09:49:25AM +0000, Igor Russkikh wrote:
>
> >
> > Hi Igor
> >
> > err = readx_poll_timeout(hw_atl_itr_res_irq_get, self, alt_itr_res,
> > alt_itr_res == 0, 10, 1000);
> >
> > The advantage of using readx_poll_timeout is that it is used by lots
> > of other drivers and works. It is much better to use core
> > infrastructure, then build your own.
>
> Hi Andrew, agreed, but driver have more incompatible places with constructs like
>
> AQ_HW_WAIT_FOR(orig_stats_val !=
> (aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR) &
> BIT(CAPS_HI_STATISTICS)),
> 1U, 10000U);
You can define a little helper:
static u32 aq_hw_read_mpi_state2_addr(struct aq_hw_s *self)
{
return aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR);
}
Then
readx_poll_timeout(u32 aq_hw_read_mpi_state2_addr, self,
stats_val, orig_stats_val != stats_val & BIT(CAPS_HI_STATISTICS));
Given you have the comment:
/* Wait FW to report back */
You think the current code is not very readable. So you could actually
have:
static int sq_fw2_update_stats_fw_wait(aq_hw_s *self, u32 orig_stats_val)
{
u32 stats_val;
return readx_poll_timeout(u32 aq_hw_read_mpi_state2_addr, self,
stats_val,
orig_stats_val != stats_val & BIT(CAPS_HI_STATISTICS),
1, 1000);
}
You see this sort of construct in quite a lot of drivers.
> Found duplicating readl_poll_timeout declaration here:
> https://elixir.bootlin.com/linux/latest/source/drivers/phy/qualcomm/phy-qcom-ufs-i.h#L27
> Not sure what's the reason, but may worth cleaning it up.
Thanks.
Andrew
^ permalink raw reply
* Re: [PATCH ipvs-next] ipvs: avoid indirect calls when calculating checksums
From: Simon Horman @ 2019-01-23 12:50 UTC (permalink / raw)
To: Julian Anastasov
Cc: Matteo Croce, lvs-devel, netdev, netfilter-devel, coreteam,
Wensong Zhang
In-Reply-To: <alpine.LFD.2.21.1901201410480.20940@ja.home.ssi.bg>
On Sun, Jan 20, 2019 at 02:11:31PM +0200, Julian Anastasov wrote:
>
> Hello,
>
> On Sat, 19 Jan 2019, Matteo Croce wrote:
>
> > The function pointer ip_vs_protocol->csum_check is only used in protocol
> > specific code, and never in the generic one.
> > Remove the function pointer from struct ip_vs_protocol and call the
> > checksum functions directly.
> > This reduces the performance impact of the Spectre mitigation, and
> > should give a small improvement even with RETPOLINES disabled.
> >
> > Signed-off-by: Matteo Croce <mcroce@redhat.com>
>
> Looks good to me, thanks!
>
> Acked-by: Julian Anastasov <ja@ssi.bg>
Likewise, Pablo could you consider applying this to nf-next?
Acked-by: Simon Horman <horms@verge.net.au>
>
> > ---
> > include/net/ip_vs.h | 3 ---
> > net/netfilter/ipvs/ip_vs_proto_ah_esp.c | 2 --
> > net/netfilter/ipvs/ip_vs_proto_sctp.c | 8 +++++---
> > net/netfilter/ipvs/ip_vs_proto_tcp.c | 12 +++++++-----
> > net/netfilter/ipvs/ip_vs_proto_udp.c | 12 +++++++-----
> > 5 files changed, 19 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> > index a0d2e0bb9a94..047f9a5ccaad 100644
> > --- a/include/net/ip_vs.h
> > +++ b/include/net/ip_vs.h
> > @@ -453,9 +453,6 @@ struct ip_vs_protocol {
> > int (*dnat_handler)(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > struct ip_vs_conn *cp, struct ip_vs_iphdr *iph);
> >
> > - int (*csum_check)(int af, struct sk_buff *skb,
> > - struct ip_vs_protocol *pp);
> > -
> > const char *(*state_name)(int state);
> >
> > void (*state_transition)(struct ip_vs_conn *cp, int direction,
> > diff --git a/net/netfilter/ipvs/ip_vs_proto_ah_esp.c b/net/netfilter/ipvs/ip_vs_proto_ah_esp.c
> > index 5320d39976e1..480598cb0f05 100644
> > --- a/net/netfilter/ipvs/ip_vs_proto_ah_esp.c
> > +++ b/net/netfilter/ipvs/ip_vs_proto_ah_esp.c
> > @@ -129,7 +129,6 @@ struct ip_vs_protocol ip_vs_protocol_ah = {
> > .conn_out_get = ah_esp_conn_out_get,
> > .snat_handler = NULL,
> > .dnat_handler = NULL,
> > - .csum_check = NULL,
> > .state_transition = NULL,
> > .register_app = NULL,
> > .unregister_app = NULL,
> > @@ -152,7 +151,6 @@ struct ip_vs_protocol ip_vs_protocol_esp = {
> > .conn_out_get = ah_esp_conn_out_get,
> > .snat_handler = NULL,
> > .dnat_handler = NULL,
> > - .csum_check = NULL,
> > .state_transition = NULL,
> > .register_app = NULL,
> > .unregister_app = NULL,
> > diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> > index b0cd7d08f2a7..bc3d1625ecc8 100644
> > --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
> > +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> > @@ -9,6 +9,9 @@
> > #include <net/sctp/checksum.h>
> > #include <net/ip_vs.h>
> >
> > +static int
> > +sctp_csum_check(int af, struct sk_buff *skb, struct ip_vs_protocol *pp);
> > +
> > static int
> > sctp_conn_schedule(struct netns_ipvs *ipvs, int af, struct sk_buff *skb,
> > struct ip_vs_proto_data *pd,
> > @@ -105,7 +108,7 @@ sctp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > int ret;
> >
> > /* Some checks before mangling */
> > - if (pp->csum_check && !pp->csum_check(cp->af, skb, pp))
> > + if (!sctp_csum_check(cp->af, skb, pp))
> > return 0;
> >
> > /* Call application helper if needed */
> > @@ -152,7 +155,7 @@ sctp_dnat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > int ret;
> >
> > /* Some checks before mangling */
> > - if (pp->csum_check && !pp->csum_check(cp->af, skb, pp))
> > + if (!sctp_csum_check(cp->af, skb, pp))
> > return 0;
> >
> > /* Call application helper if needed */
> > @@ -587,7 +590,6 @@ struct ip_vs_protocol ip_vs_protocol_sctp = {
> > .conn_out_get = ip_vs_conn_out_get_proto,
> > .snat_handler = sctp_snat_handler,
> > .dnat_handler = sctp_dnat_handler,
> > - .csum_check = sctp_csum_check,
> > .state_name = sctp_state_name,
> > .state_transition = sctp_state_transition,
> > .app_conn_bind = sctp_app_conn_bind,
> > diff --git a/net/netfilter/ipvs/ip_vs_proto_tcp.c b/net/netfilter/ipvs/ip_vs_proto_tcp.c
> > index 1770fc6ce960..6a275f989085 100644
> > --- a/net/netfilter/ipvs/ip_vs_proto_tcp.c
> > +++ b/net/netfilter/ipvs/ip_vs_proto_tcp.c
> > @@ -31,6 +31,9 @@
> >
> > #include <net/ip_vs.h>
> >
> > +static int
> > +tcp_csum_check(int af, struct sk_buff *skb, struct ip_vs_protocol *pp);
> > +
> > static int
> > tcp_conn_schedule(struct netns_ipvs *ipvs, int af, struct sk_buff *skb,
> > struct ip_vs_proto_data *pd,
> > @@ -166,7 +169,7 @@ tcp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > int ret;
> >
> > /* Some checks before mangling */
> > - if (pp->csum_check && !pp->csum_check(cp->af, skb, pp))
> > + if (!tcp_csum_check(cp->af, skb, pp))
> > return 0;
> >
> > /* Call application helper if needed */
> > @@ -192,7 +195,7 @@ tcp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > tcp_fast_csum_update(cp->af, tcph, &cp->daddr, &cp->vaddr,
> > cp->dport, cp->vport);
> > if (skb->ip_summed == CHECKSUM_COMPLETE)
> > - skb->ip_summed = (cp->app && pp->csum_check) ?
> > + skb->ip_summed = cp->app ?
> > CHECKSUM_UNNECESSARY : CHECKSUM_NONE;
> > } else {
> > /* full checksum calculation */
> > @@ -244,7 +247,7 @@ tcp_dnat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > int ret;
> >
> > /* Some checks before mangling */
> > - if (pp->csum_check && !pp->csum_check(cp->af, skb, pp))
> > + if (!tcp_csum_check(cp->af, skb, pp))
> > return 0;
> >
> > /*
> > @@ -275,7 +278,7 @@ tcp_dnat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > tcp_fast_csum_update(cp->af, tcph, &cp->vaddr, &cp->daddr,
> > cp->vport, cp->dport);
> > if (skb->ip_summed == CHECKSUM_COMPLETE)
> > - skb->ip_summed = (cp->app && pp->csum_check) ?
> > + skb->ip_summed = cp->app ?
> > CHECKSUM_UNNECESSARY : CHECKSUM_NONE;
> > } else {
> > /* full checksum calculation */
> > @@ -736,7 +739,6 @@ struct ip_vs_protocol ip_vs_protocol_tcp = {
> > .conn_out_get = ip_vs_conn_out_get_proto,
> > .snat_handler = tcp_snat_handler,
> > .dnat_handler = tcp_dnat_handler,
> > - .csum_check = tcp_csum_check,
> > .state_name = tcp_state_name,
> > .state_transition = tcp_state_transition,
> > .app_conn_bind = tcp_app_conn_bind,
> > diff --git a/net/netfilter/ipvs/ip_vs_proto_udp.c b/net/netfilter/ipvs/ip_vs_proto_udp.c
> > index 0f53c49025f8..3285718264d5 100644
> > --- a/net/netfilter/ipvs/ip_vs_proto_udp.c
> > +++ b/net/netfilter/ipvs/ip_vs_proto_udp.c
> > @@ -28,6 +28,9 @@
> > #include <net/ip.h>
> > #include <net/ip6_checksum.h>
> >
> > +static int
> > +udp_csum_check(int af, struct sk_buff *skb, struct ip_vs_protocol *pp);
> > +
> > static int
> > udp_conn_schedule(struct netns_ipvs *ipvs, int af, struct sk_buff *skb,
> > struct ip_vs_proto_data *pd,
> > @@ -156,7 +159,7 @@ udp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > int ret;
> >
> > /* Some checks before mangling */
> > - if (pp->csum_check && !pp->csum_check(cp->af, skb, pp))
> > + if (!udp_csum_check(cp->af, skb, pp))
> > return 0;
> >
> > /*
> > @@ -186,7 +189,7 @@ udp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > udp_fast_csum_update(cp->af, udph, &cp->daddr, &cp->vaddr,
> > cp->dport, cp->vport);
> > if (skb->ip_summed == CHECKSUM_COMPLETE)
> > - skb->ip_summed = (cp->app && pp->csum_check) ?
> > + skb->ip_summed = cp->app ?
> > CHECKSUM_UNNECESSARY : CHECKSUM_NONE;
> > } else {
> > /* full checksum calculation */
> > @@ -239,7 +242,7 @@ udp_dnat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > int ret;
> >
> > /* Some checks before mangling */
> > - if (pp->csum_check && !pp->csum_check(cp->af, skb, pp))
> > + if (!udp_csum_check(cp->af, skb, pp))
> > return 0;
> >
> > /*
> > @@ -270,7 +273,7 @@ udp_dnat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > udp_fast_csum_update(cp->af, udph, &cp->vaddr, &cp->daddr,
> > cp->vport, cp->dport);
> > if (skb->ip_summed == CHECKSUM_COMPLETE)
> > - skb->ip_summed = (cp->app && pp->csum_check) ?
> > + skb->ip_summed = cp->app ?
> > CHECKSUM_UNNECESSARY : CHECKSUM_NONE;
> > } else {
> > /* full checksum calculation */
> > @@ -494,7 +497,6 @@ struct ip_vs_protocol ip_vs_protocol_udp = {
> > .conn_out_get = ip_vs_conn_out_get_proto,
> > .snat_handler = udp_snat_handler,
> > .dnat_handler = udp_dnat_handler,
> > - .csum_check = udp_csum_check,
> > .state_transition = udp_state_transition,
> > .state_name = udp_state_name,
> > .register_app = udp_register_app,
> > --
> > 2.20.1
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
>
^ permalink raw reply
* Re: [PATCH ipvs-next] ipvs: use indirect call wrappers
From: Simon Horman @ 2019-01-23 12:47 UTC (permalink / raw)
To: Julian Anastasov, Pablo Neira Ayuso
Cc: Matteo Croce, lvs-devel, netdev, netfilter-devel, coreteam,
Wensong Zhang, Paolo Abeni
In-Reply-To: <alpine.LFD.2.21.1901201411400.20940@ja.home.ssi.bg>
On Sun, Jan 20, 2019 at 02:12:04PM +0200, Julian Anastasov wrote:
>
> Hello,
>
> On Sat, 19 Jan 2019, Matteo Croce wrote:
>
> > Use the new indirect call wrappers in IPVS when calling the TCP or UDP
> > protocol specific functions.
> > This avoids an indirect calls in IPVS, and reduces the performance
> > impact of the Spectre mitigation.
> >
> > Signed-off-by: Matteo Croce <mcroce@redhat.com>
>
> Looks good to me, thanks!
>
> Acked-by: Julian Anastasov <ja@ssi.bg>
Likewise, Pablo could you consider applying this to nf-next?
Acked-by: Simon Horman <horms@verge.net.au>
>
> > ---
> > net/netfilter/ipvs/ip_vs_core.c | 49 +++++++++++++++++++++++-----
> > net/netfilter/ipvs/ip_vs_proto_tcp.c | 3 +-
> > net/netfilter/ipvs/ip_vs_proto_udp.c | 3 +-
> > 3 files changed, 45 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> > index fe9abf3cc10a..e969dad66991 100644
> > --- a/net/netfilter/ipvs/ip_vs_core.c
> > +++ b/net/netfilter/ipvs/ip_vs_core.c
> > @@ -53,6 +53,7 @@
> > #endif
> >
> > #include <net/ip_vs.h>
> > +#include <linux/indirect_call_wrapper.h>
> >
> >
> > EXPORT_SYMBOL(register_ip_vs_scheduler);
> > @@ -70,6 +71,29 @@ EXPORT_SYMBOL(ip_vs_get_debug_level);
> > #endif
> > EXPORT_SYMBOL(ip_vs_new_conn_out);
> >
> > +#ifdef CONFIG_IP_VS_PROTO_TCP
> > +INDIRECT_CALLABLE_DECLARE(int
> > + tcp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > + struct ip_vs_conn *cp, struct ip_vs_iphdr *iph));
> > +#endif
> > +
> > +#ifdef CONFIG_IP_VS_PROTO_UDP
> > +INDIRECT_CALLABLE_DECLARE(int
> > + udp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > + struct ip_vs_conn *cp, struct ip_vs_iphdr *iph));
> > +#endif
> > +
> > +#if defined(CONFIG_IP_VS_PROTO_TCP) && defined(CONFIG_IP_VS_PROTO_UDP)
> > +#define SNAT_CALL(f, ...) \
> > + INDIRECT_CALL_2(f, tcp_snat_handler, udp_snat_handler, __VA_ARGS__)
> > +#elif defined(CONFIG_IP_VS_PROTO_TCP)
> > +#define SNAT_CALL(f, ...) INDIRECT_CALL_1(f, tcp_snat_handler, __VA_ARGS__)
> > +#elif defined(CONFIG_IP_VS_PROTO_UDP)
> > +#define SNAT_CALL(f, ...) INDIRECT_CALL_1(f, udp_snat_handler, __VA_ARGS__)
> > +#else
> > +#define SNAT_CALL(f, ...) f(__VA_ARGS__)
> > +#endif
> > +
> > static unsigned int ip_vs_net_id __read_mostly;
> > /* netns cnt used for uniqueness */
> > static atomic_t ipvs_netns_cnt = ATOMIC_INIT(0);
> > @@ -478,7 +502,9 @@ ip_vs_schedule(struct ip_vs_service *svc, struct sk_buff *skb,
> > */
> > if ((!skb->dev || skb->dev->flags & IFF_LOOPBACK)) {
> > iph->hdr_flags ^= IP_VS_HDR_INVERSE;
> > - cp = pp->conn_in_get(svc->ipvs, svc->af, skb, iph);
> > + cp = INDIRECT_CALL_1(pp->conn_in_get,
> > + ip_vs_conn_in_get_proto, svc->ipvs,
> > + svc->af, skb, iph);
> > iph->hdr_flags ^= IP_VS_HDR_INVERSE;
> >
> > if (cp) {
> > @@ -972,7 +998,8 @@ static int ip_vs_out_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb,
> > ip_vs_fill_iph_skb_icmp(AF_INET, skb, offset, true, &ciph);
> >
> > /* The embedded headers contain source and dest in reverse order */
> > - cp = pp->conn_out_get(ipvs, AF_INET, skb, &ciph);
> > + cp = INDIRECT_CALL_1(pp->conn_out_get, ip_vs_conn_out_get_proto,
> > + ipvs, AF_INET, skb, &ciph);
> > if (!cp)
> > return NF_ACCEPT;
> >
> > @@ -1028,7 +1055,8 @@ static int ip_vs_out_icmp_v6(struct netns_ipvs *ipvs, struct sk_buff *skb,
> > return NF_ACCEPT;
> >
> > /* The embedded headers contain source and dest in reverse order */
> > - cp = pp->conn_out_get(ipvs, AF_INET6, skb, &ciph);
> > + cp = INDIRECT_CALL_1(pp->conn_out_get, ip_vs_conn_out_get_proto,
> > + ipvs, AF_INET6, skb, &ciph);
> > if (!cp)
> > return NF_ACCEPT;
> >
> > @@ -1263,7 +1291,8 @@ handle_response(int af, struct sk_buff *skb, struct ip_vs_proto_data *pd,
> > goto drop;
> >
> > /* mangle the packet */
> > - if (pp->snat_handler && !pp->snat_handler(skb, pp, cp, iph))
> > + if (pp->snat_handler &&
> > + !SNAT_CALL(pp->snat_handler, skb, pp, cp, iph))
> > goto drop;
> >
> > #ifdef CONFIG_IP_VS_IPV6
> > @@ -1389,7 +1418,8 @@ ip_vs_out(struct netns_ipvs *ipvs, unsigned int hooknum, struct sk_buff *skb, in
> > /*
> > * Check if the packet belongs to an existing entry
> > */
> > - cp = pp->conn_out_get(ipvs, af, skb, &iph);
> > + cp = INDIRECT_CALL_1(pp->conn_out_get, ip_vs_conn_out_get_proto,
> > + ipvs, af, skb, &iph);
> >
> > if (likely(cp)) {
> > if (IP_VS_FWD_METHOD(cp) != IP_VS_CONN_F_MASQ)
> > @@ -1644,7 +1674,8 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
> > /* The embedded headers contain source and dest in reverse order.
> > * For IPIP this is error for request, not for reply.
> > */
> > - cp = pp->conn_in_get(ipvs, AF_INET, skb, &ciph);
> > + cp = INDIRECT_CALL_1(pp->conn_in_get, ip_vs_conn_in_get_proto,
> > + ipvs, AF_INET, skb, &ciph);
> >
> > if (!cp) {
> > int v;
> > @@ -1796,7 +1827,8 @@ static int ip_vs_in_icmp_v6(struct netns_ipvs *ipvs, struct sk_buff *skb,
> > /* The embedded headers contain source and dest in reverse order
> > * if not from localhost
> > */
> > - cp = pp->conn_in_get(ipvs, AF_INET6, skb, &ciph);
> > + cp = INDIRECT_CALL_1(pp->conn_in_get, ip_vs_conn_in_get_proto,
> > + ipvs, AF_INET6, skb, &ciph);
> >
> > if (!cp) {
> > int v;
> > @@ -1925,7 +1957,8 @@ ip_vs_in(struct netns_ipvs *ipvs, unsigned int hooknum, struct sk_buff *skb, int
> > /*
> > * Check if the packet belongs to an existing connection entry
> > */
> > - cp = pp->conn_in_get(ipvs, af, skb, &iph);
> > + cp = INDIRECT_CALL_1(pp->conn_in_get, ip_vs_conn_in_get_proto,
> > + ipvs, af, skb, &iph);
> >
> > conn_reuse_mode = sysctl_conn_reuse_mode(ipvs);
> > if (conn_reuse_mode && !iph.fragoffs && is_new_conn(skb, &iph) && cp) {
> > diff --git a/net/netfilter/ipvs/ip_vs_proto_tcp.c b/net/netfilter/ipvs/ip_vs_proto_tcp.c
> > index 6a275f989085..479419759983 100644
> > --- a/net/netfilter/ipvs/ip_vs_proto_tcp.c
> > +++ b/net/netfilter/ipvs/ip_vs_proto_tcp.c
> > @@ -28,6 +28,7 @@
> > #include <net/ip6_checksum.h>
> > #include <linux/netfilter.h>
> > #include <linux/netfilter_ipv4.h>
> > +#include <linux/indirect_call_wrapper.h>
> >
> > #include <net/ip_vs.h>
> >
> > @@ -146,7 +147,7 @@ tcp_partial_csum_update(int af, struct tcphdr *tcph,
> > }
> >
> >
> > -static int
> > +INDIRECT_CALLABLE_SCOPE int
> > tcp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > struct ip_vs_conn *cp, struct ip_vs_iphdr *iph)
> > {
> > diff --git a/net/netfilter/ipvs/ip_vs_proto_udp.c b/net/netfilter/ipvs/ip_vs_proto_udp.c
> > index 3285718264d5..646c384910fb 100644
> > --- a/net/netfilter/ipvs/ip_vs_proto_udp.c
> > +++ b/net/netfilter/ipvs/ip_vs_proto_udp.c
> > @@ -23,6 +23,7 @@
> > #include <linux/netfilter.h>
> > #include <linux/netfilter_ipv4.h>
> > #include <linux/udp.h>
> > +#include <linux/indirect_call_wrapper.h>
> >
> > #include <net/ip_vs.h>
> > #include <net/ip.h>
> > @@ -136,7 +137,7 @@ udp_partial_csum_update(int af, struct udphdr *uhdr,
> > }
> >
> >
> > -static int
> > +INDIRECT_CALLABLE_SCOPE int
> > udp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
> > struct ip_vs_conn *cp, struct ip_vs_iphdr *iph)
> > {
> > --
> > 2.20.1
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
>
^ permalink raw reply
* Re: [PATCH net-next 0/7] Devlink health updates
From: Eran Ben Elisha @ 2019-01-23 12:34 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev@vger.kernel.org, Jiri Pirko, David S. Miller,
Saeed Mahameed, Moshe Shemesh
In-Reply-To: <20190123114452.GE2191@nanopsycho>
On 1/23/2019 1:44 PM, Jiri Pirko wrote:
> Tue, Jan 22, 2019 at 04:57:17PM CET, eranbe@mellanox.com wrote:
>> This patchset fixes some comments that were received for the devlink
>> health series, mostly around the devlink health buffers API.
>>
>> It offers a new devlink<->driver API for passing health dump and diagnose info.
>> As part of this patchset, the new API is developed and integrated into the
>> devlink health and mlx5e TX reporter.
>> Also, added some helpers together with the new API, which reduce the code
>> required by the driver to fill dump and diagnose significantly.
>>
>> Eventually, it also deletes the old API.
>>
>> In addition, it includes some small fixes in the devlink and mlx5e TX reporter.
>
> We are (re-)defining UAPI here. You need to present some examples
> of devlink tool output, both in json and stdout.
Actually we don't really redefining the section, but only having naming
change, DEVLINK_ATTR_HEALTH_BUFFER_OBJECT* to DEVLINK_ATTR_MSG_OBJECT*.
It is pretty much the same in UAPI perspective. All examples from
original patchset are still 100% true.
> Again, much more convenient to be done for the whole patchset, not this
> "fixup-one" :/
The core of the change you have asked for it fully implemented in patch
0001 (and will be kept as is if I would do the re-done procedure). Me
and Moshe had an internal review yesterday and it doesn't look like a
nightmare at all.
Please try to give it a real try as I worked a lot in order to meet the
DD set here, and if you still find it really hard, let me know.
>
>
>>
>>
>> Eran Ben Elisha (7):
>> devlink: Add devlink msg API
>> net/mlx5e: Move driver to use devlink msg API
>> devlink: move devlink health reporter to use devlink msg API
>> devlink: Delete depracated health buffers API
>> devlink: Remove spaces around "=" in the logger print
>> devlink: Fix use-after-free at reporter destroy
>> net/mlx5e: Add RTNL lock to TX recover flow
>>
>> .../mellanox/mlx5/core/en/reporter_tx.c | 124 +---
>> include/net/devlink.h | 79 +--
>> include/trace/events/devlink.h | 2 +-
>> include/uapi/linux/devlink.h | 14 +-
>> net/core/devlink.c | 633 ++++++++----------
>> 5 files changed, 342 insertions(+), 510 deletions(-)
>>
>> --
>> 2.17.1
>>
^ permalink raw reply
* Re: [PATCH net-next] net/mlx4: Mark expected switch fall-through
From: Tariq Toukan @ 2019-01-23 12:29 UTC (permalink / raw)
To: Gustavo A. R. Silva, Tariq Toukan, David S. Miller
Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190123080511.GA32528@embeddedor>
On 1/23/2019 10:05 AM, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> This patch fixes the following warning:
>
> drivers/net/ethernet/mellanox/mlx4/eq.c: In function ‘mlx4_eq_int’:
> drivers/net/ethernet/mellanox/mlx4/mlx4.h:219:5: warning: this statement may fall through [-Wimplicit-fallthrough=]
> if (mlx4_debug_level) \
> ^
> drivers/net/ethernet/mellanox/mlx4/eq.c:558:4: note: in expansion of macro ‘mlx4_dbg’
> mlx4_dbg(dev, "%s: MLX4_EVENT_TYPE_SRQ_LIMIT. srq_no=0x%x, eq 0x%x\n",
> ^~~~~~~~
> drivers/net/ethernet/mellanox/mlx4/eq.c:561:3: note: here
> case MLX4_EVENT_TYPE_SRQ_CATAS_ERROR:
> ^~~~
>
> Warning level 3 was used: -Wimplicit-fallthrough=3
>
> This patch is part of the ongoing efforts to enabling
> -Wimplicit-fallthrough.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> drivers/net/ethernet/mellanox/mlx4/eq.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/eq.c b/drivers/net/ethernet/mellanox/mlx4/eq.c
> index 4953c852c247..2f4201023836 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/eq.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/eq.c
> @@ -558,6 +558,7 @@ static int mlx4_eq_int(struct mlx4_dev *dev, struct mlx4_eq *eq)
> mlx4_dbg(dev, "%s: MLX4_EVENT_TYPE_SRQ_LIMIT. srq_no=0x%x, eq 0x%x\n",
> __func__, be32_to_cpu(eqe->event.srq.srqn),
> eq->eqn);
> + /* fall through */
> case MLX4_EVENT_TYPE_SRQ_CATAS_ERROR:
> if (mlx4_is_master(dev)) {
> /* forward only to slave owning the SRQ */
>
Reviewed-by: Tariq Toukan <tariqt@mellanox.com>
Thanks for your patch.
^ permalink raw reply
* RE: [PATCH 2/3] arm64: dts: renesas: r8a774a1: Add clkp2 clock to CAN nodes
From: Fabrizio Castro @ 2019-01-23 12:18 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Simon Horman, Wolfgang Grandegger, Marc Kleine-Budde, Rob Herring,
Mark Rutland, Michael Turquette, Stephen Boyd, David S. Miller,
Magnus Damm, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-can@vger.kernel.org, netdev@vger.kernel.org,
devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-clk@vger.kernel.org
In-Reply-To: <CAMuHMdVkFJ7Cy-+tNHsOcYaxDj82WjMynuFuPRXa4k=DH2fGCQ@mail.gmail.com>
Hello Geert,
Thank you for your feedback!
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 23 January 2019 12:08
> Subject: Re: [PATCH 2/3] arm64: dts: renesas: r8a774a1: Add clkp2 clock to CAN nodes
>
> Hi Fabrizio,
>
> On Wed, Jan 23, 2019 at 1:01 PM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> > > From: Simon Horman <horms@verge.net.au>
> > > Sent: 23 January 2019 11:38
> > > Subject: Re: [PATCH 2/3] arm64: dts: renesas: r8a774a1: Add clkp2 clock to CAN nodes
> > >
> > > On Wed, Jan 23, 2019 at 10:51:23AM +0100, Simon Horman wrote:
> > > > On Fri, Jan 18, 2019 at 01:13:53PM +0100, Simon Horman wrote:
> > > > > On Thu, Jan 17, 2019 at 02:54:15PM +0000, Fabrizio Castro wrote:
> > > > > > According to the latest information, clkp2 is available on RZ/G2.
> > > > > > Modify CAN0 and CAN1 nodes accordingly.
> > > > > >
> > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > > > Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
>
> > > > Thanks again, applied for v5.1.
> > >
> > > Sorry, I was a little hasty there.
> > >
> > > This patch depends on the presence of R8A774A1_CLK_CANFD which
> > > (rightly) is added in a different patch in this series which is to
> > > go upstream via a different tree.
> > >
> > > I have dropped this patch for now. I think there are two solution to this
> > > problem.
> > >
> > > 1. Provide a version of this patch which uses a numeric index instead of
> > > R8A774A1_CLK_CANFD. And then, once R8A774A1_CLK_CANFD is present in an
> > > RC release provide a patch to switch to using R8A774A1_CLK_CANFD.
> > >
> > > 2. Defer this patch until R8A774A1_CLK_CANFD is present in an RC release.
> >
> > Yeah, my personal preference is solution 2.
>
> Note that solution 2 will probably defer the DT patch to v5.2.
Yeah, my understanding is that even if we chose solution 1 we would still need to be
waiting for v5.2 for the patch to switch to using R8A774A1_CLK_CANFD to appear in
a rc, therefore I think solution 2 is fine.
Thanks,
Fab
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
^ permalink raw reply
* Re: [PATCH 1/3] treewide: Lift switch variables out of switches
From: Ard Biesheuvel @ 2019-01-23 12:12 UTC (permalink / raw)
To: Jann Horn
Cc: Greg KH, Kees Cook, kernel list, Laura Abbott, Alexander Popov,
xen-devel, dri-devel, intel-gfx, intel-wired-lan,
Network Development, linux-usb, linux-fsdevel, Linux-MM, dev,
Linux Kbuild mailing list, linux-security-module,
Kernel Hardening
In-Reply-To: <CAG48ez2vfXkr9dozJiGmze8k49VOXfs=K7M8bv0aQsDDpzrEFQ@mail.gmail.com>
On Wed, 23 Jan 2019 at 13:09, Jann Horn <jannh@google.com> wrote:
>
> On Wed, Jan 23, 2019 at 1:04 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
> > > Variables declared in a switch statement before any case statements
> > > cannot be initialized, so move all instances out of the switches.
> > > After this, future always-initialized stack variables will work
> > > and not throw warnings like this:
> > >
> > > fs/fcntl.c: In function ‘send_sigio_to_task’:
> > > fs/fcntl.c:738:13: warning: statement will never be executed [-Wswitch-unreachable]
> > > siginfo_t si;
> > > ^~
> >
> > That's a pain, so this means we can't have any new variables in { }
> > scope except for at the top of a function?
>
> AFAICS this only applies to switch statements (because they jump to a
> case and don't execute stuff at the start of the block), not blocks
> after if/while/... .
>
I guess that means it may apply to other cases where you do a 'goto'
into the middle of a for() loop, for instance (at the first
iteration), which is also a valid pattern.
Is there any way to tag these assignments so the diagnostic disregards them?
^ permalink raw reply
* Re: [PATCH 1/3] treewide: Lift switch variables out of switches
From: Jann Horn @ 2019-01-23 12:09 UTC (permalink / raw)
To: Greg KH
Cc: Kees Cook, kernel list, Ard Biesheuvel, Laura Abbott,
Alexander Popov, xen-devel, dri-devel, intel-gfx, intel-wired-lan,
Network Development, linux-usb, linux-fsdevel, Linux-MM, dev,
linux-kbuild, linux-security-module, Kernel Hardening
In-Reply-To: <20190123115829.GA31385@kroah.com>
On Wed, Jan 23, 2019 at 1:04 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
> > Variables declared in a switch statement before any case statements
> > cannot be initialized, so move all instances out of the switches.
> > After this, future always-initialized stack variables will work
> > and not throw warnings like this:
> >
> > fs/fcntl.c: In function ‘send_sigio_to_task’:
> > fs/fcntl.c:738:13: warning: statement will never be executed [-Wswitch-unreachable]
> > siginfo_t si;
> > ^~
>
> That's a pain, so this means we can't have any new variables in { }
> scope except for at the top of a function?
AFAICS this only applies to switch statements (because they jump to a
case and don't execute stuff at the start of the block), not blocks
after if/while/... .
> That's going to be a hard thing to keep from happening over time, as
> this is valid C :(
^ permalink raw reply
* Re: [PATCH 2/3] arm64: dts: renesas: r8a774a1: Add clkp2 clock to CAN nodes
From: Geert Uytterhoeven @ 2019-01-23 12:07 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Simon Horman, Wolfgang Grandegger, Marc Kleine-Budde, Rob Herring,
Mark Rutland, Michael Turquette, Stephen Boyd, David S. Miller,
Magnus Damm, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-can@vger.kernel.org, netdev@vger.kernel.org,
devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-clk@vger.kernel.org
In-Reply-To: <TYXPR01MB1775ABDCFD939F91F1053534C0990@TYXPR01MB1775.jpnprd01.prod.outlook.com>
Hi Fabrizio,
On Wed, Jan 23, 2019 at 1:01 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> > From: Simon Horman <horms@verge.net.au>
> > Sent: 23 January 2019 11:38
> > Subject: Re: [PATCH 2/3] arm64: dts: renesas: r8a774a1: Add clkp2 clock to CAN nodes
> >
> > On Wed, Jan 23, 2019 at 10:51:23AM +0100, Simon Horman wrote:
> > > On Fri, Jan 18, 2019 at 01:13:53PM +0100, Simon Horman wrote:
> > > > On Thu, Jan 17, 2019 at 02:54:15PM +0000, Fabrizio Castro wrote:
> > > > > According to the latest information, clkp2 is available on RZ/G2.
> > > > > Modify CAN0 and CAN1 nodes accordingly.
> > > > >
> > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > > Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
> > > Thanks again, applied for v5.1.
> >
> > Sorry, I was a little hasty there.
> >
> > This patch depends on the presence of R8A774A1_CLK_CANFD which
> > (rightly) is added in a different patch in this series which is to
> > go upstream via a different tree.
> >
> > I have dropped this patch for now. I think there are two solution to this
> > problem.
> >
> > 1. Provide a version of this patch which uses a numeric index instead of
> > R8A774A1_CLK_CANFD. And then, once R8A774A1_CLK_CANFD is present in an
> > RC release provide a patch to switch to using R8A774A1_CLK_CANFD.
> >
> > 2. Defer this patch until R8A774A1_CLK_CANFD is present in an RC release.
>
> Yeah, my personal preference is solution 2.
Note that solution 2 will probably defer the DT patch to v5.2.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* RE: [PATCH 2/3] arm64: dts: renesas: r8a774a1: Add clkp2 clock to CAN nodes
From: Fabrizio Castro @ 2019-01-23 12:00 UTC (permalink / raw)
To: Simon Horman
Cc: Wolfgang Grandegger, Marc Kleine-Budde, Rob Herring, Mark Rutland,
Michael Turquette, Stephen Boyd, David S. Miller, Magnus Damm,
Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-can@vger.kernel.org, netdev@vger.kernel.org,
devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-clk@vger.kernel.org
In-Reply-To: <20190123113812.e3nojtxjxywmg4gn@verge.net.au>
Hello Simon,
Thank you for your feedback!
> From: Simon Horman <horms@verge.net.au>
> Sent: 23 January 2019 11:38
> Subject: Re: [PATCH 2/3] arm64: dts: renesas: r8a774a1: Add clkp2 clock to CAN nodes
>
> On Wed, Jan 23, 2019 at 10:51:23AM +0100, Simon Horman wrote:
> > On Fri, Jan 18, 2019 at 01:13:53PM +0100, Simon Horman wrote:
> > > On Thu, Jan 17, 2019 at 02:54:15PM +0000, Fabrizio Castro wrote:
> > > > According to the latest information, clkp2 is available on RZ/G2.
> > > > Modify CAN0 and CAN1 nodes accordingly.
> > > >
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
> > >
> > > Thanks,
> > >
> > > This looks fine to me but I will wait to see if there are other reviews
> > > before applying.
> > >
> > > Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> >
> > Thanks again, applied for v5.1.
>
> Sorry, I was a little hasty there.
>
> This patch depends on the presence of R8A774A1_CLK_CANFD which
> (rightly) is added in a different patch in this series which is to
> go upstream via a different tree.
>
> I have dropped this patch for now. I think there are two solution to this
> problem.
>
> 1. Provide a version of this patch which uses a numeric index instead of
> R8A774A1_CLK_CANFD. And then, once R8A774A1_CLK_CANFD is present in an
> RC release provide a patch to switch to using R8A774A1_CLK_CANFD.
>
> 2. Defer this patch until R8A774A1_CLK_CANFD is present in an RC release.
>
Yeah, my personal preference is solution 2.
Thanks,
Fab
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
^ permalink raw reply
* Re: [PATCH 1/3] treewide: Lift switch variables out of switches
From: Greg KH @ 2019-01-23 11:58 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Ard Biesheuvel, Laura Abbott, Alexander Popov,
xen-devel, dri-devel, intel-gfx, intel-wired-lan, netdev,
linux-usb, linux-fsdevel, linux-mm, dev, linux-kbuild,
linux-security-module, kernel-hardening
In-Reply-To: <20190123110349.35882-2-keescook@chromium.org>
On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
> Variables declared in a switch statement before any case statements
> cannot be initialized, so move all instances out of the switches.
> After this, future always-initialized stack variables will work
> and not throw warnings like this:
>
> fs/fcntl.c: In function ‘send_sigio_to_task’:
> fs/fcntl.c:738:13: warning: statement will never be executed [-Wswitch-unreachable]
> siginfo_t si;
> ^~
That's a pain, so this means we can't have any new variables in { }
scope except for at the top of a function?
That's going to be a hard thing to keep from happening over time, as
this is valid C :(
greg k-h
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: don't hardcode iptables/nc path in test_tcpnotify_user
From: Daniel Borkmann @ 2019-01-23 11:57 UTC (permalink / raw)
To: Stanislav Fomichev, netdev; +Cc: davem, ast
In-Reply-To: <20190117195612.157663-1-sdf@google.com>
On 01/17/2019 08:56 PM, Stanislav Fomichev wrote:
> system() is calling shell which should find the appropriate full path
> via $PATH. On some systems, full path to iptables and/or nc might be
> different that we one we have hardcoded.
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH v2 bpf-next] bpf: allow BPF programs access skb_shared_info->gso_segs field
From: Daniel Borkmann @ 2019-01-23 11:55 UTC (permalink / raw)
To: Martin Lau, Eric Dumazet
Cc: Alexei Starovoitov, netdev, Eric Dumazet, Eddie Hao,
Vlad Dumitrescu, Xiaotian Pei, Yuchung Cheng
In-Reply-To: <20190118184218.wckefzgzepbciurq@kafai-mbp.dhcp.thefacebook.com>
On 01/18/2019 07:42 PM, Martin Lau wrote:
> On Thu, Jan 17, 2019 at 03:31:57PM -0800, Eric Dumazet wrote:
>> This adds the ability to read gso_segs from a BPF program.
>>
>> v2: refined Eddie Hao patch to address Alexei feedback.
>>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Cc: Eddie Hao <eddieh@google.com>
>> Cc: Vlad Dumitrescu <vladum@google.com>
>> Cc: Xiaotian Pei <xiaotian@google.com>
>> Cc: Yuchung Cheng <ycheng@google.com>
>> ---
>> include/uapi/linux/bpf.h | 1 +
>> net/core/filter.c | 21 ++++++++++++
>> tools/include/uapi/linux/bpf.h | 1 +
>> tools/testing/selftests/bpf/test_verifier.c | 36 +++++++++++++++++++++
>> 4 files changed, 59 insertions(+)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 91c43884f295f60a85268ddf0020bf8aa47f8329..2940a9854f6d8e493518ca894e0c9c630ae4ab7a 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -2540,6 +2540,7 @@ struct __sk_buff {
>> __bpf_md_ptr(struct bpf_flow_keys *, flow_keys);
>> __u64 tstamp;
>> __u32 wire_len;
>> + __u32 gso_segs;
>> };
>>
>> struct bpf_tunnel_key {
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 2b3b436ef5457bf44c99780d6dec0b5f403f005c..a6ff5d9a04cf06926ee75cbc523456d12baf25ae 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -6700,6 +6700,27 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
>> target_size));
>> break;
>>
>> + case offsetof(struct __sk_buff, gso_segs):
>> + /* si->dst_reg = skb_shinfo(SKB); */
>> +#ifdef NET_SKBUFF_DATA_USES_OFFSET
>> + *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, head),
>> + si->dst_reg, si->src_reg,
>> + offsetof(struct sk_buff, head));
>> + *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, end),
>> + BPF_REG_TMP, si->src_reg,
>> + offsetof(struct sk_buff, end));
> I am not sure BPF_REG_TMP can be used for non-classic BPF.
> The earlier insn could be using BPF_REG_TMP (which is BPF_REG_2) and
> R2 would become loss after this BPF_LDX_MEM.
Yes, this will indeed corrupt R2 register. BPF_REG_TMP can only be used for
reg mapping out of classic BPF.
> Daniel, can BPF_REG_AX be used here as a tmp?
BPF_REG_AX would work in this case, yes. Neither of the above insns are used
in blinding nor would they collide with current verifier rewrites.
>> + *insn++ = BPF_ALU64_REG(BPF_ADD, si->dst_reg, BPF_REG_TMP);
>> +#else
>> + *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, end),
>> + si->dst_reg, si->src_reg,
>> + offsetof(struct sk_buff, end));
>> +#endif
>> + *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct skb_shared_info, gso_segs),
>> + si->dst_reg, si->dst_reg,
>> + bpf_target_off(struct skb_shared_info,
>> + gso_segs, 2,
>> + target_size));
>> + break;
>> case offsetof(struct __sk_buff, wire_len):
>> BUILD_BUG_ON(FIELD_SIZEOF(struct qdisc_skb_cb, pkt_len) != 4);
>>
>> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
>> index 91c43884f295f60a85268ddf0020bf8aa47f8329..2940a9854f6d8e493518ca894e0c9c630ae4ab7a 100644
>> --- a/tools/include/uapi/linux/bpf.h
>> +++ b/tools/include/uapi/linux/bpf.h
>> @@ -2540,6 +2540,7 @@ struct __sk_buff {
>> __bpf_md_ptr(struct bpf_flow_keys *, flow_keys);
>> __u64 tstamp;
>> __u32 wire_len;
>> + __u32 gso_segs;
>> };
>>
>> struct bpf_tunnel_key {
>> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
>> index 2fd90d4568926d13542783c870507d43a6d6bb64..2c46531044bdf9ec1e4fa47e2c94c9edb0ac3d08 100644
>> --- a/tools/testing/selftests/bpf/test_verifier.c
>> +++ b/tools/testing/selftests/bpf/test_verifier.c
>> @@ -5663,6 +5663,42 @@ static struct bpf_test tests[] = {
>> .result = ACCEPT,
>> .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
>> },
>> + {
>> + "read gso_segs from CGROUP_SKB",
>> + .insns = {
>> + BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_1,
>> + offsetof(struct __sk_buff, gso_segs)),
>> + BPF_MOV64_IMM(BPF_REG_0, 0),
>> + BPF_EXIT_INSN(),
>> + },
>> + .result = ACCEPT,
>> + .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
>> + },
>> + {
>> + "write gso_segs from CGROUP_SKB",
>> + .insns = {
>> + BPF_MOV64_IMM(BPF_REG_0, 0),
>> + BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_0,
>> + offsetof(struct __sk_buff, gso_segs)),
>> + BPF_MOV64_IMM(BPF_REG_0, 0),
>> + BPF_EXIT_INSN(),
>> + },
>> + .result = REJECT,
>> + .result_unpriv = REJECT,
>> + .errstr = "invalid bpf_context access off=164 size=4",
>> + .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
>> + },
>> + {
>> + "read gso_segs from CLS",
>> + .insns = {
>> + BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_1,
>> + offsetof(struct __sk_buff, gso_segs)),
>> + BPF_MOV64_IMM(BPF_REG_0, 0),
>> + BPF_EXIT_INSN(),
>> + },
>> + .result = ACCEPT,
>> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
>> + },
>> {
>> "multiple registers share map_lookup_elem result",
>> .insns = {
>> --
>> 2.20.1.321.g9e740568ce-goog
>>
^ permalink raw reply
* Re: [PATCH net-next 0/7] Devlink health updates
From: Jiri Pirko @ 2019-01-23 11:44 UTC (permalink / raw)
To: Eran Ben Elisha
Cc: netdev, Jiri Pirko, David S. Miller, Saeed Mahameed,
Moshe Shemesh
In-Reply-To: <1548172644-30862-1-git-send-email-eranbe@mellanox.com>
Tue, Jan 22, 2019 at 04:57:17PM CET, eranbe@mellanox.com wrote:
>This patchset fixes some comments that were received for the devlink
>health series, mostly around the devlink health buffers API.
>
>It offers a new devlink<->driver API for passing health dump and diagnose info.
>As part of this patchset, the new API is developed and integrated into the
>devlink health and mlx5e TX reporter.
>Also, added some helpers together with the new API, which reduce the code
>required by the driver to fill dump and diagnose significantly.
>
>Eventually, it also deletes the old API.
>
>In addition, it includes some small fixes in the devlink and mlx5e TX reporter.
We are (re-)defining UAPI here. You need to present some examples
of devlink tool output, both in json and stdout.
Again, much more convenient to be done for the whole patchset, not this
"fixup-one" :/
>
>
>Eran Ben Elisha (7):
> devlink: Add devlink msg API
> net/mlx5e: Move driver to use devlink msg API
> devlink: move devlink health reporter to use devlink msg API
> devlink: Delete depracated health buffers API
> devlink: Remove spaces around "=" in the logger print
> devlink: Fix use-after-free at reporter destroy
> net/mlx5e: Add RTNL lock to TX recover flow
>
> .../mellanox/mlx5/core/en/reporter_tx.c | 124 +---
> include/net/devlink.h | 79 +--
> include/trace/events/devlink.h | 2 +-
> include/uapi/linux/devlink.h | 14 +-
> net/core/devlink.c | 633 ++++++++----------
> 5 files changed, 342 insertions(+), 510 deletions(-)
>
>--
>2.17.1
>
^ permalink raw reply
* [PATCH] netfilter: ipt_CLUSTERIP: fix warning unused variable cn
From: Anders Roxell @ 2019-01-23 11:48 UTC (permalink / raw)
To: pablo, kadlec, fw, davem, kuznet, yoshfuji
Cc: netfilter-devel, coreteam, netdev, linux-kernel, Anders Roxell
When CONFIG_PROC_FS isn't set the variable cn isn't used.
net/ipv4/netfilter/ipt_CLUSTERIP.c: In function ‘clusterip_net_exit’:
net/ipv4/netfilter/ipt_CLUSTERIP.c:849:24: warning: unused variable ‘cn’ [-Wunused-variable]
struct clusterip_net *cn = clusterip_pernet(net);
^~
Rework so the variable 'cn' is declared inside "#ifdef CONFIG_PROC_FS".
Fixes: b12f7bad5ad3 ("netfilter: ipt_CLUSTERIP: remove wrong WARN_ON_ONCE in netns exit routine")
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
---
net/ipv4/netfilter/ipt_CLUSTERIP.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
index b61977db9b7f..2a909e5f9ba0 100644
--- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
+++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
@@ -846,9 +846,9 @@ static int clusterip_net_init(struct net *net)
static void clusterip_net_exit(struct net *net)
{
+#ifdef CONFIG_PROC_FS
struct clusterip_net *cn = clusterip_pernet(net);
-#ifdef CONFIG_PROC_FS
mutex_lock(&cn->mutex);
proc_remove(cn->procdir);
cn->procdir = NULL;
--
2.19.2
^ permalink raw reply related
* Re: [PATCH 2/3] arm64: dts: renesas: r8a774a1: Add clkp2 clock to CAN nodes
From: Simon Horman @ 2019-01-23 11:38 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Wolfgang Grandegger, Marc Kleine-Budde, Rob Herring, Mark Rutland,
Michael Turquette, Stephen Boyd, David S. Miller, Magnus Damm,
Geert Uytterhoeven, Chris Paterson, Biju Das, linux-can, netdev,
devicetree, linux-renesas-soc, linux-clk
In-Reply-To: <20190123095122.l3engeuwx6qr5lzc@verge.net.au>
On Wed, Jan 23, 2019 at 10:51:23AM +0100, Simon Horman wrote:
> On Fri, Jan 18, 2019 at 01:13:53PM +0100, Simon Horman wrote:
> > On Thu, Jan 17, 2019 at 02:54:15PM +0000, Fabrizio Castro wrote:
> > > According to the latest information, clkp2 is available on RZ/G2.
> > > Modify CAN0 and CAN1 nodes accordingly.
> > >
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
> >
> > Thanks,
> >
> > This looks fine to me but I will wait to see if there are other reviews
> > before applying.
> >
> > Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
>
> Thanks again, applied for v5.1.
Sorry, I was a little hasty there.
This patch depends on the presence of R8A774A1_CLK_CANFD which
(rightly) is added in a different patch in this series which is to
go upstream via a different tree.
I have dropped this patch for now. I think there are two solution to this
problem.
1. Provide a version of this patch which uses a numeric index instead of
R8A774A1_CLK_CANFD. And then, once R8A774A1_CLK_CANFD is present in an
RC release provide a patch to switch to using R8A774A1_CLK_CANFD.
2. Defer this patch until R8A774A1_CLK_CANFD is present in an RC release.
^ permalink raw reply
* Re: [PATCH net-next v7 4/8] devlink: Add support for driverinit get value for devlink_port
From: Vasundhara Volam @ 2019-01-23 11:36 UTC (permalink / raw)
To: Jiri Pirko
Cc: David Miller, michael.chan@broadcom.com, Jiri Pirko,
Jakub Kicinski, mkubecek, Netdev
In-Reply-To: <20190123110839.GD2191@nanopsycho>
On Wed, Jan 23, 2019 at 4:47 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Fri, Jan 18, 2019 at 08:09:41AM CET, vasundhara-v.volam@broadcom.com wrote:
> >Add support for "driverinit" configuration mode value for devlink_port
> >configuration parameters. Add devlink_port_param_driverinit_value_get()
> >function to help the driver get the value from devlink_port.
> >
> >Also, move the common code to __devlink_param_driverinit_value_get()
> >to be used by both device and port params.
> >
> >Cc: Jiri Pirko <jiri@mellanox.com>
> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >---
> > include/net/devlink.h | 8 ++++++
> > net/core/devlink.c | 67 ++++++++++++++++++++++++++++++++++++++-------------
> > 2 files changed, 58 insertions(+), 17 deletions(-)
> >
> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >index 98b8a66..09f3f43 100644
> >--- a/include/net/devlink.h
> >+++ b/include/net/devlink.h
> >@@ -838,6 +838,14 @@ static inline bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
> > {
> > }
> >
> >+static inline int
> >+devlink_port_param_driverinit_value_get(struct devlink_port *devlink_port,
> >+ u32 param_id,
> >+ union devlink_param_value *init_val)
> >+{
> >+ return -EOPNOTSUPP;
> >+}
>
>
> You are missing function declaration in case IS_ENABLED(CONFIG_NET_DEVLINK
>
> Otherwise this looks fine.
Thank you for pointing. It is a mistake. I will fix it in next version.
>
>
> >+
> > static inline struct devlink_region *
> > devlink_region_create(struct devlink *devlink,
> > const char *region_name,
>
> [...]
>
^ permalink raw reply
* Re: [PATCH bpf-next v4] libbpf: Show supported ELF section names on when failed to guess a prog/attach type
From: Daniel Borkmann @ 2019-01-23 11:33 UTC (permalink / raw)
To: Taeung Song, Alexei Starovoitov
Cc: netdev, Quentin Monnet, Jakub Kicinski, Andrey Ignatov
In-Reply-To: <20190121130638.7554-1-treeze.taeung@gmail.com>
On 01/21/2019 02:06 PM, Taeung Song wrote:
> We need to let users check their wrong ELF section name
> with proper ELF section names when failed to get a prog/attach type from it.
> Because users can't realize libbpf guess prog/attach types from
> given ELF section names.
> For example, when a 'cgroup' section name of a BPF program is used,
> show available ELF section names(types).
>
> Before:
>
> $ bpftool prog load bpf-prog.o /sys/fs/bpf/prog1
> Error: failed to guess program type based on ELF section name cgroup
>
> After:
>
> libbpf: failed to guess program type based on ELF section name 'cgroup'
> libbpf: supported section(type) names are: socket kprobe/ kretprobe/ classifier action tracepoint/ raw_tracepoint/ xdp perf_event lwt_in lwt_out lwt_xmit lwt_seg6local cgroup_skb/ingress cgroup_skb/egress cgroup/skb cgroup/sock cgroup/post_bind4 cgroup/post_bind6 cgroup/dev sockops sk_skb/stream_parser sk_skb/stream_verdict sk_skb sk_msg lirc_mode2 flow_dissector cgroup/bind4 cgroup/bind6 cgroup/connect4 cgroup/connect6 cgroup/sendmsg4 cgroup/sendmsg6
>
> Cc: Quentin Monnet <quentin.monnet@netronome.com>
> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
> Cc: Andrey Ignatov <rdna@fb.com>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH v2 bpf] bpf: sock recvbuff must be limited by rmem_max in bpf_setsockopt()
From: Daniel Borkmann @ 2019-01-23 11:33 UTC (permalink / raw)
To: Yafang Shao, kafai, brakmo, ast; +Cc: netdev, shaoyafang
In-Reply-To: <1548218239-31223-1-git-send-email-laoar.shao@gmail.com>
On 01/23/2019 05:37 AM, Yafang Shao wrote:
> When sock recvbuff is set by bpf_setsockopt(), the value must by limited
> by rmem_max.
> It is the same with sendbuff.
>
> Fixes: 8c4b4c7e9ff0 ("bpf: Add setsockopt helper function to bpf")
> Acked-by: Martin KaFai Lau <kafai@fb.com>
> Acked-by: Lawrence Brakmo <brakmo@fb.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Applied, thanks!
^ permalink raw reply
* [RFC] Connection Tracking Offload netdev RFC v1.0, part 1/2: command line + implementation
From: Guy Shattah @ 2019-01-23 11:29 UTC (permalink / raw)
To: Guy Shattah, Marcelo Leitner, Aaron Conole, John Hurley,
Simon Horman, Justin Pettit, Gregory Rose, Eelco Chaudron,
Flavio Leitner, Florian Westphal, Jiri Pirko, Rashid Khan,
Sushil Kulkarni, Andy Gospodarek, Roi Dayan, Yossi Kuperman,
Or Gerlitz, Rony Efraim, davem@davemloft.net
Cc: netdev@vger.kernel.org
--------------------------------------------------------------------------
Connection Tracking Offload netdev RFC v1.0 Part 1/2 - TC with Connection Tracking - command line + implementation
--------------------------------------------------------------------------
OVS recirculation ID is to be translated to TC chain, as described in https://www.netdevconf.org/2.2/papers/efraim-extendtctoct-talk.pdf
------------------------------------------------------------------------------------
CT Matches:
------------------------------------------------------------------------------------
The ct match acts on ct_state bits or ct variables which were modified as a result from a connection tracking action.
Some of the information can be extacted directly from struct nf_conn and the rest of the information could be taken by using
conntrack_mt...() [/net/netfilter/xt_conntrack.c]
1. ct_state - a new variable
The ct_state match is used to test result of connection tracking.
The bits are set or unset according to the results of the connection tracking module.
The following Match able ct_state items are supported:
* ±trk - Tracked - Been through the connection tracker
* ±new – a new connection
* ±est - Established connection
* ±dnat - Packet’s source address/port was mangled by NAT.
* ±snat - Packet’s destination address/port was mangled by NAT.
* ±inv - Invalid packet
* ±rel – Related to an existing connection
* ±rpl - Reply: Connection must be established
Example #1: "tc filter add dev eth5 protocol ip parent ffff: chain 100 flower ct_state
+trk +est -dnat action mirred egress redirect dev eth6"
2. three additional integer variables.
These variables, which can be set from within the ct_action, are introduced:
ct_zone - to commit the connection in (u16) Logically separate connection tracking
table/Multi-tenancy
ct_mark - Attach metadata to particular connections (u32)
ct_label – similar to mark (128 bits)
Example #2: "tc filter add dev eth5 protocol ip parent ffff: chain 100 flower
ct_state +trk +est ct_label 10 ct_zone 9 action drop "
Complete list of the flags and their description can be found at:
http://www.openvswitch.org/support/dist-docs/ovs-fields.7.txt
------------------------------------------------------------------------------------
CT actions:
------------------------------------------------------------------------------------
The ct_action action sends packet to ConnTrack ( nf_conntrack_in() method) and then updates ct_state bits according to the result from connection tracking.
[1] CT Action has the following possible arguments:
1. commit: Commit the connection to the connection tracking module which will be
stored beyond the lifetime of packet in the pipeline.
2. force: The force flag may be used in addition to commit flag to effectively terminate
the existing connection and start a new one in the current direction.
3. chain = K (chain is similar to ct 'table' in OVS syntax) : Clone packet to send to
connection tracker. When the connection tracker is finished, resume processing
in chain K for that packet. The original packet continues right after the ct(...) action.
4. Set variable: ct_zone, ct_mark, ct_label (see description above)
Example #3:: "tc .... action ct ct_zone 7 commit ct_label 0x0123456789ABCDEF0000111222"
5. NAT: Specifies the address and port translation for the connection being tracked.
Example #4:
"ct_action nat src 10.0.0.1 10.1.1.0" rewrite source ip+port from the list.
Example #5: "tc ... action ct nat src 10.0.0.1 10.1.1.0" rewrite source ip+port
from the list.
Example #6: "tc ... action ct nat auto" rewrite packets automatically from
saved kernel NAT list
-----
[2] CT action also has 3 new parameters
Three new variables which can be set from within the ct_action.
1. ct_zone: 16 bit
2. ct_mark: 32bit
3. ct_label: 128bit
Example #7: tc..... action ct ct_zone 7 commit ct_label x0123456789ABCDEF0000111222
------
[3] NAT action.
Supporting
(1) specific NAT for source
(2) specific NAT for destination
(3) automatic.
TC, when instructed when and how to do so, will do a NAT translation by using the kernel NAT module.
Resulting in a modified skb returning to the following TC chain for further processing
Example #8: "tc filter add dev eth5 protocol ip parent ffff: chain 100 flower action
ct commit nat src 10.0.0.0 10.0.0.255"
Commit a new connection to Conntrack and replace NAT the source ip address
Example #9: "tc filter add dev eth5 protocol ip parent ffff: chain 100 flower action ct
commit nat auto"
Commit a new connection to Conntrack and replace NAT the source ip address
Additional examples can be found at OVS NAT patch comments:
https://lwn.net/Articles/674868/
[3] match on newly added variables ( ct_zone, ct_mark, ct_label) Example #10: "tc ct_zone 3 ct_mark 0x333 ...."
----------------------------------------
Connection-Tracking action:
----------------------------
TC data path calls Connection Tracking nf_conntrack_in() method with skb which returns connTrack result inside skb->_nfct which is of type struct nf_conn.
Connection-Tracking Match:
----------------------------
connection tracking match can be done using conntrack_mt...() [/net/netfilter/xt_conntrack.c] calls which can be used to match connection tracking information.
Connection-Tracking NAT:
-------------------------------
NAT implementation details are the same as in OVS. As described in:
* https://lwn.net/Articles/674868/
* https://lwn.net/Articles/671459/
* http://www.openvswitch.org/support/ovscon2014/17/1030-conntrack_nat.pdf
Required OVS changes
-------------------------------
1. OVS has to be modified to send Connection-tracking datapath messages to TC 2. OVS datapath has to be enhanced to support enforcement of window-validation
^ permalink raw reply
* [RFC] Connection Tracking Offload netdev RFC v1.0, Part 2/2 - TC with Connection Tracking - hardware offloading and Netfilter changes
From: Guy Shattah @ 2019-01-23 11:29 UTC (permalink / raw)
To: Guy Shattah, Marcelo Leitner, Aaron Conole, John Hurley,
Simon Horman, Justin Pettit, Gregory Rose, Eelco Chaudron,
Flavio Leitner, Florian Westphal, Jiri Pirko, Rashid Khan,
Sushil Kulkarni, Andy Gospodarek, Roi Dayan, Yossi Kuperman,
Or Gerlitz, Rony Efraim, davem@davemloft.net, Pablo Neira Ayuso
Cc: netdev@vger.kernel.org
-------------------------------------------------------------------------
Connection Tracking Offload netdev RFC v1.0 Part 2/2 - TC with Connection Tracking - hardware offloading and Netfilter changes
--------------------------------------------------------------------------
This part continues the discussion of hardware offload.
All details regarding hardware specific implementation and driver were deliberately omitted from this document. as this document should be one fit all vendors
TC command-line interface
--------------------------------
In case of connection tracking hardware-offload TC should reject all skip_software rules. This is done to make
sure sw and hw are always in sync.
It has to be forced by tc in order to prevent cases where there is a chain with
connection tracking (a rule which is offloaded) and the following
chain (which does not contain hardware offload action or match) was not offloaded.
A suggestion:
* On the first time a ct_action or ct_state is used tc would scan
all existing chains and refuse to insert rule if at least one skip_software rule is found.
If everything is OK then raise a flag to mark that all rules from now on must
be both in software and hardware.
* Any further rules will check this flag.
* This flag is clear when all rules in TC are cleared.
I'm certain that we could find more solutions if this one is not sufficient.
Offloading ct_state matches:
--------------------------------
The current offload interface used by TC is ndo_setup_tc().
This interface is used to send a tc chain to the hardware.
The interface is kept the same for commands containing ct_state.
The only difference is in the underlying driver/hardware implementation.
1. It is the duty of the driver to convert 'matches' such as:
ct_zone=XX,ct_mark=XX,,ct_label=XX,ct_state=±trk,±new,±est,±dnat,±snat,±inv,±rel,±rpl
properly to the hardware interface.
2. It is the duty of the driver to make sure that in case of ambiguity, where
hardware is not capable of making a decision on a packet path to send
packet to the software for further processing [see section on fallback to software]
3. please note that Hardware may or may not have the following capabilities:
(a) packet check-sum (layer 3/4)
(b) TCP window-validation
(c) flow counters/stats
(d) other?
Offloading ct_action / changes in tc data-path:
--------------------------------------------------
While on pure software tc connection tracking implementation the packet is sent to connection-tracking
to retrieve result into skb->_nfct and futher matches are used on this struct to make routing decisions,
On tc with offload support there is an additional test:
If packet is part of an established connection a decision is made to offload connection to hardware.
To offload this connection a 'flow offload' infrastructure developed by Pablo is used,
( This infrastructure is enhanced to support connection tracking as described below.[see section on netfilter changes] )
The packet is sent to nft_flow_offload_eval() along with additional data for offload evaluation.
Such additional data is:
* struct _nfct
* from which current flags such as ±rpl/±rel can be used by the driver to set ct_state on
established connection.
* TCP window information
* Any other additional information.
* Originating tc chain id, from which the offload took place.
* flow (connection) id
Possible syncronization issues:
---------------------------------
When connection are offloaded there is a short latency until rule is active in hardware.
This means that packets may arrive to TC after connection has been offloaded.
While this doesn't introduce any issue on UDP+ICMP it might be an issue
on TCP due to TCP window issues. In this case I'd suggest to offload the connection with a
large tcp window and allow the hardware (where window-validation is supported) rescale the window back.
Netfilter changes
---------------------
Once a packet passed through a chain which includes a CT action resulted with 'established' state
TC to send the packet to Netfilter for offload evaluation [ nft_flow_offload_eval()] and then
and flow should be offloaded.
Changes:
1. On Pablo's patches, he had added a new ndo for hardware offload:
https://github.com/orgcandman/linux-next-work/commit/351bd1303c2ae5de0c4512f0cdf7b61508df0777#diff-cf45716f2ff8e67a1727be77c3b894feR1392
However, During discussions in netfilter workshop,(May 2018) Pablo and Jiri Pirko have agreed that both NDOs basically
do the same thing - hardware offload. Hence it would be wiser to use ndo_setup_tc call from netfilter to do the
offload, rather than adding a whole new NDO.
Another suggestion was made it to rename ndo_setup_tc in the future, to reflect that it is not only used from TC.
Please read the following paper from netdev, the paper discusses this solution in depth:
https://www.files.netdevconf.org/d/c5b1c9709ca743d79a63/
2. in case of tc NAT, the NAT header-rewrite has to be a part of the
connection-tracking offload. hence TC will supply Netfilter offload
code not only the chain ID from which the offload took place but
also information on how to do NAT translation.
2. Netfilter to maintain two separate lists of offloaded connections.
Each list is offloaded flows is kept inside: struct nf_flowtable *flowtable
(currently - there is only one list)
2.1 The first list is a list of 'soft' offloaded connections.
In this list Netfilter iterates and is responsible for flow aging and flow termination.
For flow aging it iterates over all the flows.
For flow termination it will be using FIN packets arriving from TC.
2.2 The second list is a list of 'hardware' offloaded connections.
This is a software representation of the psychical list representation in hardware for offloaded connections.
Flows in this list are aged and terminated by the hardware/driver by event/callback.
3. On reaching full HW capacity:
In case where hardware capacity is reached Netfilter should mark the flow
(a new addition inside struct nf_conn or flow offload entry)
and put it on a 'pending list'.
Once the 'offloaded flows list' has new empty spots (released by events)
then it should try to offload again.
Another possibility (maybe in phase two?) is have some kind of smarter LRU algorithm
where some of the connections are offloaded, some are not and there is an algorithm to
decide and switch between hardware and software, based on traffic (stats/counters).
4. Expiration mechanism [in software]:
The current Expiration in Connection Tracking:
Lazy expiration: old connections are removed when new connections arrive.
Relatively long time: over 5 Days for established TCP connection About 180 secs for UDP
Expiration in Netfilter flow offload :
Every once in a while scan all existing offloaded flows and query driver to see if flow has expired [suggested queries to driver in a code that was not submitted upstream:
if (nf_flow_has_expired(flow) ||
nf_flow_is_dying(flow)) {
Suggested expiration algorithm [to be implemented in netfilter flow-offload or the driver]:
Instead of scanning millions of items per one second scan a small number of items per seconds.
Covering all of them twice within the timeout period:
With N offloaded connections - Every K seconds sample N/2k of the flows in the systems.
K= 180 secs for udp/icmp
K= 3600 secs for tcp
Final expiration value either configurable or constant
5. counters - NetFilter has to be enhanced to pull counter (per flow) from hardware
and update stats per connection/flow.
6. megaflows - Netfilter has to be enhanced with API to support deletion of
multiple flows according to the OVS recirculation id/tc chain id.
As dicussed - this will be either a blocking call or async.
7. Fallback - Netfilter has to be enhanced with API to support case where
a flow has to be moved from HW back to SW and then:
(1) never be moved to HW again OR (2) attemp to move back to HW where possible.
Handling OVS rule eviction:
------------------------------
The way OVS offload is being done today is by OVS sending a copy of the OVS-Kernel data-path netlink
messages to TC. OVS then samples counters/stats to see if this flow/connection is still valid
and has not aged. OVS may decide to evict this rule due to aging or due to user manually removing
the openflow rule.
When a rule was removed (a rule containing ct_action command) then a netlink command is sent to TC
TC should then send a command to netfilter offload code (similar to the event mentioned before) to
empty the offloaded connection list.
Regarding counter/stats: for a single chain with a ct_action we need to
pull counters/stats from all the offloaded connections. an accumulative counter.
Fallback to software
-------------------------
While hardware is processing a packet there is a possibility that hardware
reaches a state/flow table which does not contain sufficient information on how to
continue processing.
Such cases may happen when originating flow-table sends a packet to non-existing
target flow table. When hardware capabilities are not sufficient to make a decision,
When there is ambiguity or when packet failed all tests and is sent to software
for further analysis and possibly other cases.
We should note that this may happen in any state of the processing:
It may happen after packet has been decapsulated or after encapsulation.
After header-rewrite (such as NAT) or after some meta-data was placed on the packet
or the connection (ct_label data, for example).
We define two acceptable behaviors:
1. Fallback from a middle of processing to a tc chain which is not the entry one (i.e. chain zero)
the mechanism should be able to fully deliver a packet with all meta-data
to upper layer with exact middle point of processing. to continue processing.
2. Fallback from a middle of processing to a start/entry-point of processing.
this is a private case of #1. Where last flow table is zero.
In this case a replica of the original unmodified packet has to be sent to the start/entry-point of upper-level.
Any other behavior is not acceptable, since software cannot handle cases
where the packet header has been modified without knowning where the process stopped.
In addition to the two described behavior, we also allow a fallback into a middle of
a tc chain.
Why is it required?
In case of a tc rule that does decapsulation/header-rewrite and then connection tracking
it is possible that a packet will go trough decapsulation/header-rewrite and then fail
on connection-tracking in hardware (due to connection not being offloaded yet).
in this case the software (i.e. tc) may receive a malformed packet which does
not match accurately at the tc chain. Hence there is a need to enter the packet
into a middle a chain, after matches were done and all the actions, except for connection tracking.
i.e.; the packet is sent to the equivalent tc chain to do connection-tracking there.
Hence, there is support for fallback into (1) a beginning of tc chain and (2) right into the middle of it before CT.
----
Now lets talk about fallbacks:
1. Fallback from hardware/driver to TC
The driver should be able to restore information (ct_zone/mark/label/last flow table)
into the packet.
We suggest that ct_zone/ct_mark/ct_label/ct_state are restored into struct nf_conn (skb->_nfct)
and the last flow_table is restored into skb->cb (we have space there we could use)
On receiving packet with skb->cb set to certain value tc will start processing
the packet from a chain specific in skb->cb .
2. Fallback from TC to OVS.
There is a need to add a new logic into OVS which supports handling of a packet
from a specific recirculation id/chain/flow table.
Currently OVS-kernel clones the skb and if it fails processing (in kernel)
then the original packet is sent to OVS-userspace.
This cannot work for our case since TC doesn't keep the original copy of the
packet (before header-rewrite/decapsulation/etc) and understandably, the packet
cannot be restored to its original form.
We suggest to do the TC->OVS path in the same manner as Driver->TC path.
a packet is reassembled and restored by the driver and injected into TC
(along with meta-data on skb->cv). If TC lacks the information on how to
proceed the packet is left unchanged and the next stop is OVS which has to be
extended with new logic on how to proceed.
^ permalink raw reply
* Connection Tracking Offload netdev RFC v1.0, Part 2/2 - TC with Connection Tracking - hardware offloading and Netfilter changes
From: Guy Shattah @ 2019-01-23 11:28 UTC (permalink / raw)
To: Marcelo Leitner, Aaron Conole, John Hurley, Simon Horman,
Justin Pettit, Gregory Rose, Eelco Chaudron, Flavio Leitner,
Florian Westphal, Jiri Pirko, Rashid Khan, Sushil Kulkarni,
Andy Gospodarek, Roi Dayan, Yossi Kuperman, Or Gerlitz,
Rony Efraim, davem@davemloft.net, Pablo Neira Ayuso
Cc: netdev@vger.kernel.org
-------------------------------------------------------------------------
Connection Tracking Offload netdev RFC v1.0
Part 2/2 - TC with Connection Tracking - hardware offloading and Netfilter changes
--------------------------------------------------------------------------
This part continues the discussion of hardware offload.
All details regarding hardware specific implementation and driver were deliberately
omitted from this document. as this document should be one fit all vendors
TC command-line interface
--------------------------------
In case of connection tracking hardware-offload TC should reject all skip_software rules. This is done to make
sure sw and hw are always in sync.
It has to be forced by tc in order to prevent cases where there is a chain with
connection tracking (a rule which is offloaded) and the following
chain (which does not contain hardware offload action or match) was not offloaded.
A suggestion:
* On the first time a ct_action or ct_state is used tc would scan
all existing chains and refuse to insert rule if at least one skip_software rule is found.
If everything is OK then raise a flag to mark that all rules from now on must
be both in software and hardware.
* Any further rules will check this flag.
* This flag is clear when all rules in TC are cleared.
I'm certain that we could find more solutions if this one is not sufficient.
Offloading ct_state matches:
--------------------------------
The current offload interface used by TC is ndo_setup_tc().
This interface is used to send a tc chain to the hardware.
The interface is kept the same for commands containing ct_state.
The only difference is in the underlying driver/hardware implementation.
1. It is the duty of the driver to convert 'matches' such as:
ct_zone=XX,ct_mark=XX,,ct_label=XX,ct_state=±trk,±new,±est,±dnat,±snat,±inv,±rel,±rpl
properly to the hardware interface.
2. It is the duty of the driver to make sure that in case of ambiguity, where
hardware is not capable of making a decision on a packet path to send
packet to the software for further processing [see section on fallback to software]
3. please note that Hardware may or may not have the following capabilities:
(a) packet check-sum (layer 3/4)
(b) TCP window-validation
(c) flow counters/stats
(d) other?
Offloading ct_action / changes in tc data-path:
--------------------------------------------------
While on pure software tc connection tracking implementation the packet is sent to connection-tracking
to retrieve result into skb->_nfct and futher matches are used on this struct to make routing decisions,
On tc with offload support there is an additional test:
If packet is part of an established connection a decision is made to offload connection to hardware.
To offload this connection a 'flow offload' infrastructure developed by Pablo is used,
( This infrastructure is enhanced to support connection tracking as described below.[see section on netfilter changes] )
The packet is sent to nft_flow_offload_eval() along with additional data for offload evaluation.
Such additional data is:
* struct _nfct
* from which current flags such as ±rpl/±rel can be used by the driver to set ct_state on
established connection.
* TCP window information
* Any other additional information.
* Originating tc chain id, from which the offload took place.
* flow (connection) id
Possible syncronization issues:
---------------------------------
When connection are offloaded there is a short latency until rule is active in hardware.
This means that packets may arrive to TC after connection has been offloaded.
While this doesn't introduce any issue on UDP+ICMP it might be an issue
on TCP due to TCP window issues. In this case I'd suggest to offload the connection with a
large tcp window and allow the hardware (where window-validation is supported) rescale the window back.
Netfilter changes
---------------------
Once a packet passed through a chain which includes a CT action resulted with 'established' state
TC to send the packet to Netfilter for offload evaluation [ nft_flow_offload_eval()] and then
and flow should be offloaded.
Changes:
1. On Pablo's patches, he had added a new ndo for hardware offload:
https://github.com/orgcandman/linux-next-work/commit/351bd1303c2ae5de0c4512f0cdf7b61508df0777#diff-cf45716f2ff8e67a1727be77c3b894feR1392
However, During discussions in netfilter workshop,(May 2018) Pablo and Jiri Pirko have agreed that both NDOs basically
do the same thing - hardware offload. Hence it would be wiser to use ndo_setup_tc call from netfilter to do the
offload, rather than adding a whole new NDO.
Another suggestion was made it to rename ndo_setup_tc in the future, to reflect that it is not only used from TC.
Please read the following paper from netdev, the paper discusses this solution in depth:
https://www.files.netdevconf.org/d/c5b1c9709ca743d79a63/
2. in case of tc NAT, the NAT header-rewrite has to be a part of the
connection-tracking offload. hence TC will supply Netfilter offload
code not only the chain ID from which the offload took place but
also information on how to do NAT translation.
2. Netfilter to maintain two separate lists of offloaded connections.
Each list is offloaded flows is kept inside: struct nf_flowtable *flowtable
(currently - there is only one list)
2.1 The first list is a list of 'soft' offloaded connections.
In this list Netfilter iterates and is responsible for flow aging and flow termination.
For flow aging it iterates over all the flows.
For flow termination it will be using FIN packets arriving from TC.
2.2 The second list is a list of 'hardware' offloaded connections.
This is a software representation of the psychical list representation in hardware for offloaded connections.
Flows in this list are aged and terminated by the hardware/driver by event/callback.
3. On reaching full HW capacity:
In case where hardware capacity is reached Netfilter should mark the flow
(a new addition inside struct nf_conn or flow offload entry)
and put it on a 'pending list'.
Once the 'offloaded flows list' has new empty spots (released by events)
then it should try to offload again.
Another possibility (maybe in phase two?) is have some kind of smarter LRU algorithm
where some of the connections are offloaded, some are not and there is an algorithm to
decide and switch between hardware and software, based on traffic (stats/counters).
4. Expiration mechanism [in software]:
The current Expiration in Connection Tracking:
Lazy expiration: old connections are removed when new connections arrive.
Relatively long time: over 5 Days for established TCP connection About 180 secs for UDP
Expiration in Netfilter flow offload :
Every once in a while scan all existing offloaded flows and query driver to see if flow has expired [suggested queries to driver in a code that was not submitted upstream:
if (nf_flow_has_expired(flow) ||
nf_flow_is_dying(flow)) {
Suggested expiration algorithm [to be implemented in netfilter flow-offload or the driver]:
Instead of scanning millions of items per one second scan a small number of items per seconds.
Covering all of them twice within the timeout period:
With N offloaded connections - Every K seconds sample N/2k of the flows in the systems.
K= 180 secs for udp/icmp
K= 3600 secs for tcp
Final expiration value either configurable or constant
5. counters - NetFilter has to be enhanced to pull counter (per flow) from hardware
and update stats per connection/flow.
6. megaflows - Netfilter has to be enhanced with API to support deletion of
multiple flows according to the OVS recirculation id/tc chain id.
As dicussed - this will be either a blocking call or async.
7. Fallback - Netfilter has to be enhanced with API to support case where
a flow has to be moved from HW back to SW and then:
(1) never be moved to HW again OR (2) attemp to move back to HW where possible.
Handling OVS rule eviction:
------------------------------
The way OVS offload is being done today is by OVS sending a copy of the OVS-Kernel data-path netlink
messages to TC. OVS then samples counters/stats to see if this flow/connection is still valid
and has not aged. OVS may decide to evict this rule due to aging or due to user manually removing
the openflow rule.
When a rule was removed (a rule containing ct_action command) then a netlink command is sent to TC
TC should then send a command to netfilter offload code (similar to the event mentioned before) to
empty the offloaded connection list.
Regarding counter/stats: for a single chain with a ct_action we need to
pull counters/stats from all the offloaded connections. an accumulative counter.
Fallback to software
-------------------------
While hardware is processing a packet there is a possibility that hardware
reaches a state/flow table which does not contain sufficient information on how to
continue processing.
Such cases may happen when originating flow-table sends a packet to non-existing
target flow table. When hardware capabilities are not sufficient to make a decision,
When there is ambiguity or when packet failed all tests and is sent to software
for further analysis and possibly other cases.
We should note that this may happen in any state of the processing:
It may happen after packet has been decapsulated or after encapsulation.
After header-rewrite (such as NAT) or after some meta-data was placed on the packet
or the connection (ct_label data, for example).
We define two acceptable behaviors:
1. Fallback from a middle of processing to a tc chain which is not the entry one (i.e. chain zero)
the mechanism should be able to fully deliver a packet with all meta-data
to upper layer with exact middle point of processing. to continue processing.
2. Fallback from a middle of processing to a start/entry-point of processing.
this is a private case of #1. Where last flow table is zero.
In this case a replica of the original unmodified packet has to be sent to the start/entry-point of upper-level.
Any other behavior is not acceptable, since software cannot handle cases
where the packet header has been modified without knowning where the process stopped.
In addition to the two described behavior, we also allow a fallback into a middle of
a tc chain.
Why is it required?
In case of a tc rule that does decapsulation/header-rewrite and then connection tracking
it is possible that a packet will go trough decapsulation/header-rewrite and then fail
on connection-tracking in hardware (due to connection not being offloaded yet).
in this case the software (i.e. tc) may receive a malformed packet which does
not match accurately at the tc chain. Hence there is a need to enter the packet
into a middle a chain, after matches were done and all the actions, except for connection tracking.
i.e.; the packet is sent to the equivalent tc chain to do connection-tracking there.
Hence, there is support for fallback into (1) a beginning of tc chain and (2) right into the middle of it before CT.
----
Now lets talk about fallbacks:
1. Fallback from hardware/driver to TC
The driver should be able to restore information (ct_zone/mark/label/last flow table)
into the packet.
We suggest that ct_zone/ct_mark/ct_label/ct_state are restored into struct nf_conn (skb->_nfct)
and the last flow_table is restored into skb->cb (we have space there we could use)
On receiving packet with skb->cb set to certain value tc will start processing
the packet from a chain specific in skb->cb .
2. Fallback from TC to OVS.
There is a need to add a new logic into OVS which supports handling of a packet
from a specific recirculation id/chain/flow table.
Currently OVS-kernel clones the skb and if it fails processing (in kernel)
then the original packet is sent to OVS-userspace.
This cannot work for our case since TC doesn't keep the original copy of the
packet (before header-rewrite/decapsulation/etc) and understandably, the packet
cannot be restored to its original form.
We suggest to do the TC->OVS path in the same manner as Driver->TC path.
a packet is reassembled and restored by the driver and injected into TC
(along with meta-data on skb->cv). If TC lacks the information on how to
proceed the packet is left unchanged and the next stop is OVS which has to be
extended with new logic on how to proceed.
^ 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