* Re: [RFC 0/3] Adding config get/set to devlink
From: Steve Lin @ 2017-10-12 20:12 UTC (permalink / raw)
To: Florian Fainelli
Cc: David Miller, Jiri Pirko, Roopa Prabhu, netdev, Jiri Pirko,
Michael Chan, linux-pci, John Linville, Andy Gospodarek
In-Reply-To: <21ab4a5d-0b6a-7976-7bf0-acd334f2613f@gmail.com>
On Thu, Oct 12, 2017 at 3:20 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 10/12/2017 12:06 PM, David Miller wrote:
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Date: Thu, 12 Oct 2017 08:43:59 -0700
>>
>>> Once we move ethtool (or however we name its successor) over to
>>> netlink there is an opportunity for accessing objects that do and do
>>> not have a netdevice representor today (e.g: management ports on
>>> switches) with the same interface, and devlink could be used for
>>> that.
>>
>> That is an interesting angle for including this in devlink.
>>
>> I'm not so sure what to do about this.
>>
>> One suggestion is that devlink is used for getting ethtool stats for
>> objects lacking netdev representor's, and a new genetlink family is
>> used for netdev based ethtool.
>
> Right, I was also thinking along those lines that we we would have a new
> generic netlink family for ethtool to support ethtool over netlink.
>
>>
>> I think it's important that we don't expand the scope of devlink
>> beyond what it was originally designed for.
>
> It seems to me like devlink is well defined in what it is not for: it is
> not meant to be used for any object that is/has a net_device, but it is
> not well defined for what it can offer to these non network devices. For
> instance, we have a tremendous amount of operations that are extremely
> specific to its single user(s) such as mlx5 and mlxsw.
>
> For instance, I am not sure how the buffer reservation scheme can be
> generalized, and this is always the tricky part with a single user
> facility in that you try to generalize the best you can based on the HW
> you know. This is not a criticism or meant to be anything negative, this
> just happens to be the case, and we did not have anything better.
>
> So maybe the first thing is to clarify what devlink operations can and
> should be and what they are absolutely not allowed to cover. We should
> also clarify whether a generic set/get that Steven is proposing is
> something that we tolerate, or whether there should be specific function
> pointers implemented for each attribute, which would be more in line
> with what has been done thus far.
Hi Florian,
Some of this is subjective, of course, but just to clarify, it did
seem like implementing a new devlink_op function pointer per attribute
might be more consistent with what's been done so far. But for code
reuse purposes - i.e. to avoid replicating essentially the same
function for each of the 30+ config attributes - I elected to just
implement a single generic get and set devlink_op.
Steve
^ permalink raw reply
* RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver
From: Yuval Mintz @ 2017-10-12 20:21 UTC (permalink / raw)
To: Yunsheng Lin, davem@davemloft.net
Cc: huangdaode@hisilicon.com, xuwei5@hisilicon.com,
liguozhu@hisilicon.com, Yisen.Zhuang@huawei.com,
gabriele.paoloni@huawei.com, john.garry@huawei.com,
linuxarm@huawei.com, yisen.zhuang@huawei.com,
salil.mehta@huawei.com, lipeng321@huawei.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1507775912-22402-1-git-send-email-linyunsheng@huawei.com>
> This patchset adds a new hardware offload type in mqprio before adding
> mqprio hardware offload support in hns3 driver.
I think one of the biggest issues in tying this to DCB configuration is the
non-immediate [and possibly non persistent] configuration.
Scenario #1:
User is configuring mqprio offloaded with 3 TCs while device is in willing mode.
Would you expect the driver to immediately respond with a success or instead
delay the return until the DCBx negotiation is complete and the operational
num of TCs is actually 3?
Scenario #2:
Assume user explicitly offloaded mqprio with 3 TCs, but now DCB configuration
has changed on the peer side and 4 TCs is the new negotiated operational value.
Your current driver logic would change the number of TCs underneath the user
configuration [and it would actually probably work due to mqprio being a crappy
qdisc]. But was that the user actual intention?
[I think the likely answer in this scenario is 'yes' since the alternative is no better.
But I still thought it was worth mentioning]
Cheers,
Yuval
>
> Yunsheng Lin (2):
> mqprio: Add a new hardware offload type in mqprio
> net: hns3: Add mqprio hardware offload support in hns3 driver
>
> drivers/net/ethernet/hisilicon/hns3/hnae3.h | 1 +
> .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 23 +++++++++++
> .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46 ++++++++++++++-
> -------
> include/uapi/linux/pkt_sched.h | 1 +
> 4 files changed, 55 insertions(+), 16 deletions(-)
>
> --
> 1.9.1
^ permalink raw reply
* Re: [net-next V7 PATCH 1/5] bpf: introduce new bpf cpu map type BPF_MAP_TYPE_CPUMAP
From: Edward Cree @ 2017-10-12 20:35 UTC (permalink / raw)
To: Jesper Dangaard Brouer, netdev
Cc: jakub.kicinski, Michael S. Tsirkin, pavel.odintsov, Jason Wang,
mchan, John Fastabend, peter.waskiewicz.jr, ast, Daniel Borkmann,
Alexei Starovoitov, Andy Gospodarek
In-Reply-To: <150781120970.9409.4248519763583438653.stgit@firesoul>
On 12/10/17 13:26, Jesper Dangaard Brouer wrote:
> The 'cpumap' is primary used as a backend map for XDP BPF helper
s/primary/primarily.
> call bpf_redirect_map() and XDP_REDIRECT action, like 'devmap'.
>
> This patch implement the main part of the map. It is not connected to
> the XDP redirect system yet, and no SKB allocation are done yet.
>
> The main concern in this patch is to ensure the datapath can run
> without any locking. This adds complexity to the setup and tear-down
> procedure, which assumptions are extra carefully documented in the
> code comments.
>
> V2:
> - make sure array isn't larger than NR_CPUS
> - make sure CPUs added is a valid possible CPU
>
> V3: fix nitpicks from Jakub Kicinski <kubakici@wp.pl>
>
> V5:
> - Restrict map allocation to root / CAP_SYS_ADMIN
> - WARN_ON_ONCE if queue is not empty on tear-down
> - Return -EPERM on memlock limit instead of -ENOMEM
> - Error code in __cpu_map_entry_alloc() also handle ptr_ring_cleanup()
> - Moved cpu_map_enqueue() to next patch
>
> V6: all notice by Daniel Borkmann
> - Fix err return code in cpu_map_alloc() introduced in V5
> - Move cpu_possible() check after max_entries boundary check
> - Forbid usage initially in check_map_func_compatibility()
>
> V7:
> - Fix alloc error path spotted by Daniel Borkmann
> - Did stress test adding+removing CPUs from the map concurrently
> - Fixed refcnt issue on cpu_map_entry, kthread started too soon
> - Make sure packets are flushed during tear-down, involved use of
> rcu_barrier() and kthread_run only exit after queue is empty
> - Fix alloc error path in __cpu_map_entry_alloc() for ptr_ring
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> include/linux/bpf_types.h | 1
> include/uapi/linux/bpf.h | 1
> kernel/bpf/Makefile | 1
> kernel/bpf/cpumap.c | 561 ++++++++++++++++++++++++++++++++++++++++
> kernel/bpf/syscall.c | 8 -
> kernel/bpf/verifier.c | 5
> tools/include/uapi/linux/bpf.h | 1
> 7 files changed, 577 insertions(+), 1 deletion(-)
> create mode 100644 kernel/bpf/cpumap.c
>
> diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> index 6f1a567667b8..814c1081a4a9 100644
> --- a/include/linux/bpf_types.h
> +++ b/include/linux/bpf_types.h
> @@ -41,4 +41,5 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP, dev_map_ops)
> #ifdef CONFIG_STREAM_PARSER
> BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKMAP, sock_map_ops)
> #endif
> +BPF_MAP_TYPE(BPF_MAP_TYPE_CPUMAP, cpu_map_ops)
> #endif
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 6db9e1d679cd..4303fb6c3817 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -112,6 +112,7 @@ enum bpf_map_type {
> BPF_MAP_TYPE_HASH_OF_MAPS,
> BPF_MAP_TYPE_DEVMAP,
> BPF_MAP_TYPE_SOCKMAP,
> + BPF_MAP_TYPE_CPUMAP,
> };
>
> enum bpf_prog_type {
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index 897daa005b23..dba0bd33a43c 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -4,6 +4,7 @@ obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o tnum.o
> obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o map_in_map.o
> ifeq ($(CONFIG_NET),y)
> obj-$(CONFIG_BPF_SYSCALL) += devmap.o
> +obj-$(CONFIG_BPF_SYSCALL) += cpumap.o
> ifeq ($(CONFIG_STREAM_PARSER),y)
> obj-$(CONFIG_BPF_SYSCALL) += sockmap.o
> endif
> diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> new file mode 100644
> index 000000000000..34db22afcca2
> --- /dev/null
> +++ b/kernel/bpf/cpumap.c
> @@ -0,0 +1,561 @@
> +/* bpf/cpumap.c
> + *
> + * Copyright (c) 2017 Jesper Dangaard Brouer, Red Hat Inc.
> + * Released under terms in GPL version 2. See COPYING.
> + */
> +
> +/* The 'cpumap' is primary used as a backend map for XDP BPF helper
Again, s/primary/primarily.
> + * call bpf_redirect_map() and XDP_REDIRECT action, like 'devmap'.
> + *
> + * Unlike devmap which redirect XDP frames out another NIC device,
> + * this map type redirect raw XDP frames to another CPU. The remote
Also I think both of these 'redirect' should be 'redirects', just a
grammatical nit pick ;)
> + * CPU will do SKB-allocation and call the normal network stack.
> + *
> + * This is a scalability and isolation mechanism, that allow
> + * separating the early driver network XDP layer, from the rest of the
> + * netstack, and assigning dedicated CPUs for this stage. This
> + * basically allows for 10G wirespeed pre-filtering via bpf.
> + */
> +#include <linux/bpf.h>
> +#include <linux/filter.h>
> +#include <linux/ptr_ring.h>
> +
> +#include <linux/sched.h>
> +#include <linux/workqueue.h>
> +#include <linux/kthread.h>
> +#include <linux/capability.h>
> +
> +/* General idea: XDP packets getting XDP redirected to another CPU,
> + * will maximum be stored/queued for one driver ->poll() call. It is
> + * guaranteed that setting flush bit and flush operation happen on
> + * same CPU. Thus, cpu_map_flush operation can deduct via this_cpu_ptr()
> + * which queue in bpf_cpu_map_entry contains packets.
> + */
> +
> +#define CPU_MAP_BULK_SIZE 8 /* 8 == one cacheline on 64-bit archs */
> +struct xdp_bulk_queue {
> + void *q[CPU_MAP_BULK_SIZE];
> + unsigned int count;
> +};
I realise it's a bit late to say this on a v7, but it might be better to
use a linked-list (list_heads) here instead of an array. Then, the
struct xdp_pkt you store in the packet headroom could contain the
list_head, there's no arbitrary bulking limit, and the flush just has
to link the newly-created elements into the receiving CPU's list.
Is there an obvious reason why this wouldn't work / can't perform as
well, or should I try it and benchmark it?
-Ed
^ permalink raw reply
* Re: [PATCH net-next 0/2] Fix IFE meta modules loading
From: Roman Mashak @ 2017-10-12 20:37 UTC (permalink / raw)
To: David Miller; +Cc: netdev, jhs
In-Reply-To: <20171012.122318.1151617277989957064.davem@davemloft.net>
David Miller <davem@davemloft.net> writes:
> From: Roman Mashak <mrv@mojatatu.com>
> Date: Wed, 11 Oct 2017 10:50:28 -0400
>
>> Adjust module alias names of IFE meta modules and fix the bug that
>> prevented auto-loading IFE modules in run-time.
>
> Anyone using the existing alises will be broken by these changes
> no?
Actually aliases never worked, the bug existed since the day act_meta_*
modules have been introduced. I suspect everyone compiles them in-kernel
rather then as modules.
> Is it possible to define multiple aliases? That would be a way to
> get the more desirable names whilst not breaking existing users.
^ permalink raw reply
* Re: [PATCH net-next 01/12] bpf: verifier: set reg_type on context accesses in second pass
From: Daniel Borkmann @ 2017-10-12 20:43 UTC (permalink / raw)
To: Jakub Kicinski, netdev; +Cc: oss-drivers, alexei.starovoitov
In-Reply-To: <20171012173418.4029-2-jakub.kicinski@netronome.com>
On 10/12/2017 07:34 PM, Jakub Kicinski wrote:
> Use a simplified is_valid_access() callback when verifier
> is used for program analysis by non-host JITs. This allows
> us to teach the verifier about packet start and packet end
> offsets for direct packet access.
>
> We can extend the callback as needed but for most packet
> processing needs there isn't much more the offloads may
> require.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> ---
> CC: alexei.starovoitov@gmail.com
> CC: daniel@iogearbox.net
>
> kernel/bpf/verifier.c | 43 +++++++++++++++++++++++++++++++++++++------
> 1 file changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2cdbcc4f8f6b..9755279d94cb 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -813,6 +813,36 @@ static int check_packet_access(struct bpf_verifier_env *env, u32 regno, int off,
> return err;
> }
>
> +static bool analyzer_is_valid_access(struct bpf_verifier_env *env, int off,
> + struct bpf_insn_access_aux *info)
> +{
> + switch (env->prog->type) {
> + case BPF_PROG_TYPE_XDP:
> + switch (off) {
> + case offsetof(struct xdp_buff, data):
> + info->reg_type = PTR_TO_PACKET;
> + return true;
> + case offsetof(struct xdp_buff, data_end):
> + info->reg_type = PTR_TO_PACKET_END;
> + return true;
> + }
> + return false;
> + case BPF_PROG_TYPE_SCHED_CLS:
> + switch (off) {
> + case offsetof(struct sk_buff, data):
> + info->reg_type = PTR_TO_PACKET;
> + return true;
> + case offsetof(struct sk_buff, cb) +
> + offsetof(struct bpf_skb_data_end, data_end):
> + info->reg_type = PTR_TO_PACKET_END;
> + return true;
> + }
> + return false;
> + default:
> + return false;
> + }
> +}
> +
> /* check access to 'struct bpf_context' fields. Supports fixed offsets only */
> static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, int off, int size,
> enum bpf_access_type t, enum bpf_reg_type *reg_type)
> @@ -821,12 +851,13 @@ static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, int off,
> .reg_type = *reg_type,
> };
>
> - /* for analyzer ctx accesses are already validated and converted */
> - if (env->analyzer_ops)
> - return 0;
> -
> - if (env->prog->aux->ops->is_valid_access &&
> - env->prog->aux->ops->is_valid_access(off, size, t, &info)) {
> + if (env->analyzer_ops) {
> + if (analyzer_is_valid_access(env, off, &info)) {
> + *reg_type = info.reg_type;
Is there some specific issue with the is_valid_access() callbacks that you
need to do this (I couldn't parse that out of the commit message)? It would
be nice to keep the reg_type setting in one place, meaning the callbacks
themselves, so we wouldn't need to maintain this in multiple places.
> + return 0;
> + }
> + } else if (env->prog->aux->ops->is_valid_access &&
> + env->prog->aux->ops->is_valid_access(off, size, t, &info)) {
> /* A non zero info.ctx_field_size indicates that this field is a
> * candidate for later verifier transformation to load the whole
> * field and then apply a mask when accessed with a narrower
>
^ permalink raw reply
* [PATCH] Add -target to clang switches while cross compiling.
From: Abhijit Ayarekar @ 2017-10-12 20:45 UTC (permalink / raw)
To: ast; +Cc: daniel, netdev, linux-kernel, Abhijit Ayarekar
Latest llvm update excludes assembly instructions.
As a result __ASM_SYSREGS_H define is not required.
-target switch includes appropriate target specific files.
Tested on x86 and arm64 with llvm with git revision
commit df6ca162269f9d756f8742bf4b658dcf690e3eb5
Author: Yonghong Song <yhs@fb.com>
Date: Thu Sep 28 02:46:11 2017 +0000
bpf: add new insns for bswap_to_le and negation
Signed-off-by: Abhijit Ayarekar <abhijit.ayarekar@caviumnetworks.com>
---
samples/bpf/Makefile | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index ebc2ad6..81f9fcd 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -180,6 +180,7 @@ CLANG ?= clang
# Detect that we're cross compiling and use the cross compiler
ifdef CROSS_COMPILE
HOSTCC = $(CROSS_COMPILE)gcc
+CLANG_ARCH_ARGS = -target $(ARCH)
endif
# Trick to allow make to be run from this directory
@@ -229,9 +230,9 @@ $(obj)/tracex5_kern.o: $(obj)/syscall_nrs.h
$(obj)/%.o: $(src)/%.c
$(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) -I$(obj) \
-I$(srctree)/tools/testing/selftests/bpf/ \
- -D__KERNEL__ -D__ASM_SYSREG_H -Wno-unused-value -Wno-pointer-sign \
+ -D__KERNEL__ -Wno-unused-value -Wno-pointer-sign \
-D__TARGET_ARCH_$(ARCH) -Wno-compare-distinct-pointer-types \
-Wno-gnu-variable-sized-type-not-at-end \
-Wno-address-of-packed-member -Wno-tautological-compare \
- -Wno-unknown-warning-option \
+ -Wno-unknown-warning-option $(CLANG_ARCH_ARGS) \
-O2 -emit-llvm -c $< -o -| $(LLC) -march=bpf -filetype=obj -o $@
--
2.7.4
^ permalink raw reply related
* [PATCH net-next v5 0/5] bpf: security: New file mode and LSM hooks for eBPF object permission control
From: Chenbo Feng @ 2017-10-12 20:55 UTC (permalink / raw)
To: netdev
Cc: Jeffrey Vander Stoep, Alexei Starovoitov, lorenzo,
Daniel Borkmann, Stephen Smalley, James Morris, Paul Moore,
Chenbo Feng
From: Chenbo Feng <fengc@google.com>
Much like files and sockets, eBPF objects are accessed, controlled, and
shared via a file descriptor (FD). Unlike files and sockets, the
existing mechanism for eBPF object access control is very limited.
Currently there are two options for granting accessing to eBPF
operations: grant access to all processes, or only CAP_SYS_ADMIN
processes. The CAP_SYS_ADMIN-only mode is not ideal because most users
do not have this capability and granting a user CAP_SYS_ADMIN grants too
many other security-sensitive permissions. It also unnecessarily allows
all CAP_SYS_ADMIN processes access to eBPF functionality. Allowing all
processes to access to eBPF objects is also undesirable since it has
potential to allow unprivileged processes to consume kernel memory, and
opens up attack surface to the kernel.
Adding LSM hooks maintains the status quo for systems which do not use
an LSM, preserving compatibility with userspace, while allowing security
modules to choose how best to handle permissions on eBPF objects. Here
is a possible use case for the lsm hooks with selinux module:
The network-control daemon (netd) creates and loads an eBPF object for
network packet filtering and analysis. It passes the object FD to an
unprivileged network monitor app (netmonitor), which is not allowed to
create, modify or load eBPF objects, but is allowed to read the traffic
stats from the map.
Selinux could use these hooks to grant the following permissions:
allow netd self:bpf_map { create read write};
allow netmonitor netd:fd use;
allow netmonitor netd:bpf_map read;
In this patch series, A file mode is added to bpf map to store the
accessing mode. With this file mode flags, the map can be obtained read
only, write only or read and write. With the help of this file mode,
several security hooks can be added to the eBPF syscall implementations
to do permissions checks. These LSM hooks are mainly focused on checking
the process privileges before it obtains the fd for a specific bpf
object. No matter from a file location or from a eBPF id. Besides that,
a general check hook is also implemented at the start of bpf syscalls so
that each security module can have their own implementation on the reset
of bpf object related functionalities.
In order to store the ownership and security information about eBPF
maps, a security field pointer is added to the struct bpf_map. And the
last two patch set are implementation of selinux check on these hooks
introduced, plus an additional check when eBPF object is passed between
processes using unix socket as well as binder IPC.
Change since V1:
- Whitelist the new bpf flags in the map allocate check.
- Added bpf selftest for the new flags.
- Added two new security hooks for copying the security information from
the bpf object security struct to file security struct
- Simplified the checking action when bpf fd is passed between processes.
Change since V2:
- Fixed the line break problem for map flags check
- Fixed the typo in selinux check of file mode.
- Merge bpf_map and bpf_prog into one selinux class
- Added bpf_type and bpf_sid into file security struct to store the
security information when generate fd.
- Add the hook to bpf_map_new_fd and bpf_prog_new_fd.
Change since V3:
- Return the actual error from security check instead of -EPERM
- Move the hooks into anon_inode_getfd() to avoid get file again after
bpf object file is installed with fd.
- Removed the bpf_sid field inside file_scerity_struct to reduce the
cache size.
Change since V4:
- Rename bpf av prog_use to prog_run to distinguish from fd_use.
- Remove the bpf_type field inside file_scerity_struct and use bpf fops
to indentify bpf object instead.
Chenbo Feng (5):
bpf: Add file mode configuration into bpf maps
bpf: Add tests for eBPF file mode
security: bpf: Add LSM hooks for bpf object related syscall
selinux: bpf: Add selinux check for eBPF syscall operations
selinux: bpf: Add addtional check for bpf object file receive
include/linux/bpf.h | 15 ++-
include/linux/lsm_hooks.h | 54 +++++++++++
include/linux/security.h | 45 +++++++++
include/uapi/linux/bpf.h | 6 ++
kernel/bpf/arraymap.c | 6 +-
kernel/bpf/devmap.c | 5 +-
kernel/bpf/hashtab.c | 5 +-
kernel/bpf/inode.c | 15 ++-
kernel/bpf/lpm_trie.c | 3 +-
kernel/bpf/sockmap.c | 5 +-
kernel/bpf/stackmap.c | 5 +-
kernel/bpf/syscall.c | 118 ++++++++++++++++++++---
security/security.c | 32 +++++++
security/selinux/hooks.c | 160 ++++++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 2 +
security/selinux/include/objsec.h | 4 +
tools/testing/selftests/bpf/test_maps.c | 48 ++++++++++
17 files changed, 502 insertions(+), 26 deletions(-)
--
2.15.0.rc0.271.g36b669edcc-goog
^ permalink raw reply
* [PATCH net-next v5 1/5] bpf: Add file mode configuration into bpf maps
From: Chenbo Feng @ 2017-10-12 20:55 UTC (permalink / raw)
To: netdev
Cc: Jeffrey Vander Stoep, Alexei Starovoitov, lorenzo,
Daniel Borkmann, Stephen Smalley, James Morris, Paul Moore,
Chenbo Feng
In-Reply-To: <20171012205510.36028-1-chenbofeng.kernel@gmail.com>
From: Chenbo Feng <fengc@google.com>
Introduce the map read/write flags to the eBPF syscalls that returns the
map fd. The flags is used to set up the file mode when construct a new
file descriptor for bpf maps. To not break the backward capability, the
f_flags is set to O_RDWR if the flag passed by syscall is 0. Otherwise
it should be O_RDONLY or O_WRONLY. When the userspace want to modify or
read the map content, it will check the file mode to see if it is
allowed to make the change.
Signed-off-by: Chenbo Feng <fengc@google.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
include/linux/bpf.h | 6 ++--
include/uapi/linux/bpf.h | 6 ++++
kernel/bpf/arraymap.c | 6 +++-
kernel/bpf/devmap.c | 5 ++-
kernel/bpf/hashtab.c | 5 +--
kernel/bpf/inode.c | 15 ++++++---
kernel/bpf/lpm_trie.c | 3 +-
kernel/bpf/sockmap.c | 5 ++-
kernel/bpf/stackmap.c | 5 ++-
kernel/bpf/syscall.c | 80 +++++++++++++++++++++++++++++++++++++++++++-----
10 files changed, 114 insertions(+), 22 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index bc7da2ddfcaf..0e9ca2555d7f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -308,11 +308,11 @@ void bpf_map_area_free(void *base);
extern int sysctl_unprivileged_bpf_disabled;
-int bpf_map_new_fd(struct bpf_map *map);
+int bpf_map_new_fd(struct bpf_map *map, int flags);
int bpf_prog_new_fd(struct bpf_prog *prog);
int bpf_obj_pin_user(u32 ufd, const char __user *pathname);
-int bpf_obj_get_user(const char __user *pathname);
+int bpf_obj_get_user(const char __user *pathname, int flags);
int bpf_percpu_hash_copy(struct bpf_map *map, void *key, void *value);
int bpf_percpu_array_copy(struct bpf_map *map, void *key, void *value);
@@ -331,6 +331,8 @@ int bpf_fd_htab_map_update_elem(struct bpf_map *map, struct file *map_file,
void *key, void *value, u64 map_flags);
int bpf_fd_htab_map_lookup_elem(struct bpf_map *map, void *key, u32 *value);
+int bpf_get_file_flag(int flags);
+
/* memcpy that is used with 8-byte aligned pointers, power-of-8 size and
* forced to use 'long' read/writes to try to atomically copy long counters.
* Best-effort only. No barriers here, since it _will_ race with concurrent
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6db9e1d679cd..9cb50a228c39 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -217,6 +217,10 @@ enum bpf_attach_type {
#define BPF_OBJ_NAME_LEN 16U
+/* Flags for accessing BPF object */
+#define BPF_F_RDONLY (1U << 3)
+#define BPF_F_WRONLY (1U << 4)
+
union bpf_attr {
struct { /* anonymous struct used by BPF_MAP_CREATE command */
__u32 map_type; /* one of enum bpf_map_type */
@@ -259,6 +263,7 @@ union bpf_attr {
struct { /* anonymous struct used by BPF_OBJ_* commands */
__aligned_u64 pathname;
__u32 bpf_fd;
+ __u32 file_flags;
};
struct { /* anonymous struct used by BPF_PROG_ATTACH/DETACH commands */
@@ -286,6 +291,7 @@ union bpf_attr {
__u32 map_id;
};
__u32 next_id;
+ __u32 open_flags;
};
struct { /* anonymous struct used by BPF_OBJ_GET_INFO_BY_FD */
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 68d866628be0..988c04c91e10 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -19,6 +19,9 @@
#include "map_in_map.h"
+#define ARRAY_CREATE_FLAG_MASK \
+ (BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
+
static void bpf_array_free_percpu(struct bpf_array *array)
{
int i;
@@ -56,7 +59,8 @@ static struct bpf_map *array_map_alloc(union bpf_attr *attr)
/* check sanity of attributes */
if (attr->max_entries == 0 || attr->key_size != 4 ||
- attr->value_size == 0 || attr->map_flags & ~BPF_F_NUMA_NODE ||
+ attr->value_size == 0 ||
+ attr->map_flags & ~ARRAY_CREATE_FLAG_MASK ||
(percpu && numa_node != NUMA_NO_NODE))
return ERR_PTR(-EINVAL);
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index e093d9a2c4dd..e5d3de7cff2e 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -50,6 +50,9 @@
#include <linux/bpf.h>
#include <linux/filter.h>
+#define DEV_CREATE_FLAG_MASK \
+ (BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
+
struct bpf_dtab_netdev {
struct net_device *dev;
struct bpf_dtab *dtab;
@@ -80,7 +83,7 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
/* check sanity of attributes */
if (attr->max_entries == 0 || attr->key_size != 4 ||
- attr->value_size != 4 || attr->map_flags & ~BPF_F_NUMA_NODE)
+ attr->value_size != 4 || attr->map_flags & ~DEV_CREATE_FLAG_MASK)
return ERR_PTR(-EINVAL);
dtab = kzalloc(sizeof(*dtab), GFP_USER);
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index 431126f31ea3..919955236e63 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -18,8 +18,9 @@
#include "bpf_lru_list.h"
#include "map_in_map.h"
-#define HTAB_CREATE_FLAG_MASK \
- (BPF_F_NO_PREALLOC | BPF_F_NO_COMMON_LRU | BPF_F_NUMA_NODE)
+#define HTAB_CREATE_FLAG_MASK \
+ (BPF_F_NO_PREALLOC | BPF_F_NO_COMMON_LRU | BPF_F_NUMA_NODE | \
+ BPF_F_RDONLY | BPF_F_WRONLY)
struct bucket {
struct hlist_nulls_head head;
diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index e833ed914358..7d8c6dd8dd5d 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c
@@ -295,7 +295,7 @@ int bpf_obj_pin_user(u32 ufd, const char __user *pathname)
}
static void *bpf_obj_do_get(const struct filename *pathname,
- enum bpf_type *type)
+ enum bpf_type *type, int flags)
{
struct inode *inode;
struct path path;
@@ -307,7 +307,7 @@ static void *bpf_obj_do_get(const struct filename *pathname,
return ERR_PTR(ret);
inode = d_backing_inode(path.dentry);
- ret = inode_permission(inode, MAY_WRITE);
+ ret = inode_permission(inode, ACC_MODE(flags));
if (ret)
goto out;
@@ -326,18 +326,23 @@ static void *bpf_obj_do_get(const struct filename *pathname,
return ERR_PTR(ret);
}
-int bpf_obj_get_user(const char __user *pathname)
+int bpf_obj_get_user(const char __user *pathname, int flags)
{
enum bpf_type type = BPF_TYPE_UNSPEC;
struct filename *pname;
int ret = -ENOENT;
+ int f_flags;
void *raw;
+ f_flags = bpf_get_file_flag(flags);
+ if (f_flags < 0)
+ return f_flags;
+
pname = getname(pathname);
if (IS_ERR(pname))
return PTR_ERR(pname);
- raw = bpf_obj_do_get(pname, &type);
+ raw = bpf_obj_do_get(pname, &type, f_flags);
if (IS_ERR(raw)) {
ret = PTR_ERR(raw);
goto out;
@@ -346,7 +351,7 @@ int bpf_obj_get_user(const char __user *pathname)
if (type == BPF_TYPE_PROG)
ret = bpf_prog_new_fd(raw);
else if (type == BPF_TYPE_MAP)
- ret = bpf_map_new_fd(raw);
+ ret = bpf_map_new_fd(raw, f_flags);
else
goto out;
diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
index 34d8a690ea05..885e45479680 100644
--- a/kernel/bpf/lpm_trie.c
+++ b/kernel/bpf/lpm_trie.c
@@ -495,7 +495,8 @@ static int trie_delete_elem(struct bpf_map *map, void *_key)
#define LPM_KEY_SIZE_MAX LPM_KEY_SIZE(LPM_DATA_SIZE_MAX)
#define LPM_KEY_SIZE_MIN LPM_KEY_SIZE(LPM_DATA_SIZE_MIN)
-#define LPM_CREATE_FLAG_MASK (BPF_F_NO_PREALLOC | BPF_F_NUMA_NODE)
+#define LPM_CREATE_FLAG_MASK (BPF_F_NO_PREALLOC | BPF_F_NUMA_NODE | \
+ BPF_F_RDONLY | BPF_F_WRONLY)
static struct bpf_map *trie_alloc(union bpf_attr *attr)
{
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index a298d6666698..86ec846f2d5e 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -40,6 +40,9 @@
#include <linux/list.h>
#include <net/strparser.h>
+#define SOCK_CREATE_FLAG_MASK \
+ (BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
+
struct bpf_stab {
struct bpf_map map;
struct sock **sock_map;
@@ -489,7 +492,7 @@ static struct bpf_map *sock_map_alloc(union bpf_attr *attr)
/* check sanity of attributes */
if (attr->max_entries == 0 || attr->key_size != 4 ||
- attr->value_size != 4 || attr->map_flags & ~BPF_F_NUMA_NODE)
+ attr->value_size != 4 || attr->map_flags & ~SOCK_CREATE_FLAG_MASK)
return ERR_PTR(-EINVAL);
if (attr->value_size > KMALLOC_MAX_SIZE)
diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index 135be433e9a0..a15bc636cc98 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -11,6 +11,9 @@
#include <linux/perf_event.h>
#include "percpu_freelist.h"
+#define STACK_CREATE_FLAG_MASK \
+ (BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
+
struct stack_map_bucket {
struct pcpu_freelist_node fnode;
u32 hash;
@@ -60,7 +63,7 @@ static struct bpf_map *stack_map_alloc(union bpf_attr *attr)
if (!capable(CAP_SYS_ADMIN))
return ERR_PTR(-EPERM);
- if (attr->map_flags & ~BPF_F_NUMA_NODE)
+ if (attr->map_flags & ~STACK_CREATE_FLAG_MASK)
return ERR_PTR(-EINVAL);
/* check sanity of attributes */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index d124e702e040..b02582ead9a4 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -294,17 +294,48 @@ static void bpf_map_show_fdinfo(struct seq_file *m, struct file *filp)
}
#endif
+static ssize_t bpf_dummy_read(struct file *filp, char __user *buf, size_t siz,
+ loff_t *ppos)
+{
+ /* We need this handler such that alloc_file() enables
+ * f_mode with FMODE_CAN_READ.
+ */
+ return -EINVAL;
+}
+
+static ssize_t bpf_dummy_write(struct file *filp, const char __user *buf,
+ size_t siz, loff_t *ppos)
+{
+ /* We need this handler such that alloc_file() enables
+ * f_mode with FMODE_CAN_WRITE.
+ */
+ return -EINVAL;
+}
+
static const struct file_operations bpf_map_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = bpf_map_show_fdinfo,
#endif
.release = bpf_map_release,
+ .read = bpf_dummy_read,
+ .write = bpf_dummy_write,
};
-int bpf_map_new_fd(struct bpf_map *map)
+int bpf_map_new_fd(struct bpf_map *map, int flags)
{
return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
- O_RDWR | O_CLOEXEC);
+ flags | O_CLOEXEC);
+}
+
+int bpf_get_file_flag(int flags)
+{
+ if ((flags & BPF_F_RDONLY) && (flags & BPF_F_WRONLY))
+ return -EINVAL;
+ if (flags & BPF_F_RDONLY)
+ return O_RDONLY;
+ if (flags & BPF_F_WRONLY)
+ return O_WRONLY;
+ return O_RDWR;
}
/* helper macro to check that unused fields 'union bpf_attr' are zero */
@@ -344,12 +375,17 @@ static int map_create(union bpf_attr *attr)
{
int numa_node = bpf_map_attr_numa_node(attr);
struct bpf_map *map;
+ int f_flags;
int err;
err = CHECK_ATTR(BPF_MAP_CREATE);
if (err)
return -EINVAL;
+ f_flags = bpf_get_file_flag(attr->map_flags);
+ if (f_flags < 0)
+ return f_flags;
+
if (numa_node != NUMA_NO_NODE &&
((unsigned int)numa_node >= nr_node_ids ||
!node_online(numa_node)))
@@ -375,7 +411,7 @@ static int map_create(union bpf_attr *attr)
if (err)
goto free_map;
- err = bpf_map_new_fd(map);
+ err = bpf_map_new_fd(map, f_flags);
if (err < 0) {
/* failed to allocate fd.
* bpf_map_put() is needed because the above
@@ -490,6 +526,11 @@ static int map_lookup_elem(union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
+ if (!(f.file->f_mode & FMODE_CAN_READ)) {
+ err = -EPERM;
+ goto err_put;
+ }
+
key = memdup_user(ukey, map->key_size);
if (IS_ERR(key)) {
err = PTR_ERR(key);
@@ -570,6 +611,11 @@ static int map_update_elem(union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
+ if (!(f.file->f_mode & FMODE_CAN_WRITE)) {
+ err = -EPERM;
+ goto err_put;
+ }
+
key = memdup_user(ukey, map->key_size);
if (IS_ERR(key)) {
err = PTR_ERR(key);
@@ -653,6 +699,11 @@ static int map_delete_elem(union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
+ if (!(f.file->f_mode & FMODE_CAN_WRITE)) {
+ err = -EPERM;
+ goto err_put;
+ }
+
key = memdup_user(ukey, map->key_size);
if (IS_ERR(key)) {
err = PTR_ERR(key);
@@ -696,6 +747,11 @@ static int map_get_next_key(union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
+ if (!(f.file->f_mode & FMODE_CAN_READ)) {
+ err = -EPERM;
+ goto err_put;
+ }
+
if (ukey) {
key = memdup_user(ukey, map->key_size);
if (IS_ERR(key)) {
@@ -902,6 +958,8 @@ static const struct file_operations bpf_prog_fops = {
.show_fdinfo = bpf_prog_show_fdinfo,
#endif
.release = bpf_prog_release,
+ .read = bpf_dummy_read,
+ .write = bpf_dummy_write,
};
int bpf_prog_new_fd(struct bpf_prog *prog)
@@ -1111,11 +1169,11 @@ static int bpf_prog_load(union bpf_attr *attr)
return err;
}
-#define BPF_OBJ_LAST_FIELD bpf_fd
+#define BPF_OBJ_LAST_FIELD file_flags
static int bpf_obj_pin(const union bpf_attr *attr)
{
- if (CHECK_ATTR(BPF_OBJ))
+ if (CHECK_ATTR(BPF_OBJ) || attr->file_flags != 0)
return -EINVAL;
return bpf_obj_pin_user(attr->bpf_fd, u64_to_user_ptr(attr->pathname));
@@ -1126,7 +1184,8 @@ static int bpf_obj_get(const union bpf_attr *attr)
if (CHECK_ATTR(BPF_OBJ) || attr->bpf_fd != 0)
return -EINVAL;
- return bpf_obj_get_user(u64_to_user_ptr(attr->pathname));
+ return bpf_obj_get_user(u64_to_user_ptr(attr->pathname),
+ attr->file_flags);
}
#ifdef CONFIG_CGROUP_BPF
@@ -1386,12 +1445,13 @@ static int bpf_prog_get_fd_by_id(const union bpf_attr *attr)
return fd;
}
-#define BPF_MAP_GET_FD_BY_ID_LAST_FIELD map_id
+#define BPF_MAP_GET_FD_BY_ID_LAST_FIELD open_flags
static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
{
struct bpf_map *map;
u32 id = attr->map_id;
+ int f_flags;
int fd;
if (CHECK_ATTR(BPF_MAP_GET_FD_BY_ID))
@@ -1400,6 +1460,10 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
+ f_flags = bpf_get_file_flag(attr->open_flags);
+ if (f_flags < 0)
+ return f_flags;
+
spin_lock_bh(&map_idr_lock);
map = idr_find(&map_idr, id);
if (map)
@@ -1411,7 +1475,7 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
- fd = bpf_map_new_fd(map);
+ fd = bpf_map_new_fd(map, f_flags);
if (fd < 0)
bpf_map_put(map);
--
2.15.0.rc0.271.g36b669edcc-goog
^ permalink raw reply related
* [PATCH net-next v5 2/5] bpf: Add tests for eBPF file mode
From: Chenbo Feng @ 2017-10-12 20:55 UTC (permalink / raw)
To: netdev
Cc: Jeffrey Vander Stoep, Alexei Starovoitov, lorenzo,
Daniel Borkmann, Stephen Smalley, James Morris, Paul Moore,
Chenbo Feng
In-Reply-To: <20171012205510.36028-1-chenbofeng.kernel@gmail.com>
From: Chenbo Feng <fengc@google.com>
Two related tests are added into bpf selftest to test read only map and
write only map. The tests verified the read only and write only flags
are working on hash maps.
Signed-off-by: Chenbo Feng <fengc@google.com>
---
tools/testing/selftests/bpf/test_maps.c | 48 +++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index fe3a443a1102..896f23cfe918 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -1033,6 +1033,51 @@ static void test_map_parallel(void)
assert(bpf_map_get_next_key(fd, &key, &key) == -1 && errno == ENOENT);
}
+static void test_map_rdonly(void)
+{
+ int i, fd, key = 0, value = 0;
+
+ fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value),
+ MAP_SIZE, map_flags | BPF_F_RDONLY);
+ if (fd < 0) {
+ printf("Failed to create map for read only test '%s'!\n",
+ strerror(errno));
+ exit(1);
+ }
+
+ key = 1;
+ value = 1234;
+ /* Insert key=1 element. */
+ assert(bpf_map_update_elem(fd, &key, &value, BPF_ANY) == -1 &&
+ errno == EPERM);
+
+ /* Check that key=2 is not found. */
+ assert(bpf_map_lookup_elem(fd, &key, &value) == -1 && errno == ENOENT);
+ assert(bpf_map_get_next_key(fd, &key, &value) == -1 && errno == ENOENT);
+}
+
+static void test_map_wronly(void)
+{
+ int i, fd, key = 0, value = 0;
+
+ fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value),
+ MAP_SIZE, map_flags | BPF_F_WRONLY);
+ if (fd < 0) {
+ printf("Failed to create map for read only test '%s'!\n",
+ strerror(errno));
+ exit(1);
+ }
+
+ key = 1;
+ value = 1234;
+ /* Insert key=1 element. */
+ assert(bpf_map_update_elem(fd, &key, &value, BPF_ANY) == 0)
+
+ /* Check that key=2 is not found. */
+ assert(bpf_map_lookup_elem(fd, &key, &value) == -1 && errno == EPERM);
+ assert(bpf_map_get_next_key(fd, &key, &value) == -1 && errno == EPERM);
+}
+
static void run_all_tests(void)
{
test_hashmap(0, NULL);
@@ -1050,6 +1095,9 @@ static void run_all_tests(void)
test_map_large();
test_map_parallel();
test_map_stress();
+
+ test_map_rdonly();
+ test_map_wronly();
}
int main(void)
--
2.15.0.rc0.271.g36b669edcc-goog
^ permalink raw reply related
* [PATCH net-next v5 3/5] security: bpf: Add LSM hooks for bpf object related syscall
From: Chenbo Feng @ 2017-10-12 20:55 UTC (permalink / raw)
To: netdev
Cc: Jeffrey Vander Stoep, Alexei Starovoitov, lorenzo,
Daniel Borkmann, Stephen Smalley, James Morris, Paul Moore,
Chenbo Feng
In-Reply-To: <20171012205510.36028-1-chenbofeng.kernel@gmail.com>
From: Chenbo Feng <fengc@google.com>
Introduce several LSM hooks for the syscalls that will allow the
userspace to access to eBPF object such as eBPF programs and eBPF maps.
The security check is aimed to enforce a per object security protection
for eBPF object so only processes with the right priviliges can
read/write to a specific map or use a specific eBPF program. Besides
that, a general security hook is added before the multiplexer of bpf
syscall to check the cmd and the attribute used for the command. The
actual security module can decide which command need to be checked and
how the cmd should be checked.
Signed-off-by: Chenbo Feng <fengc@google.com>
---
include/linux/bpf.h | 6 ++++++
include/linux/lsm_hooks.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++
include/linux/security.h | 45 +++++++++++++++++++++++++++++++++++++++
kernel/bpf/syscall.c | 34 +++++++++++++++++++++++++++--
security/security.c | 32 ++++++++++++++++++++++++++++
5 files changed, 169 insertions(+), 2 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 0e9ca2555d7f..225740688ab7 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -57,6 +57,9 @@ struct bpf_map {
atomic_t usercnt;
struct bpf_map *inner_map_meta;
char name[BPF_OBJ_NAME_LEN];
+#ifdef CONFIG_SECURITY
+ void *security;
+#endif
};
/* function argument constraints */
@@ -190,6 +193,9 @@ struct bpf_prog_aux {
struct user_struct *user;
u64 load_time; /* ns since boottime */
char name[BPF_OBJ_NAME_LEN];
+#ifdef CONFIG_SECURITY
+ void *security;
+#endif
union {
struct work_struct work;
struct rcu_head rcu;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index c9258124e417..7161d8e7ee79 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1351,6 +1351,40 @@
* @inode we wish to get the security context of.
* @ctx is a pointer in which to place the allocated security context.
* @ctxlen points to the place to put the length of @ctx.
+ *
+ * Security hooks for using the eBPF maps and programs functionalities through
+ * eBPF syscalls.
+ *
+ * @bpf:
+ * Do a initial check for all bpf syscalls after the attribute is copied
+ * into the kernel. The actual security module can implement their own
+ * rules to check the specific cmd they need.
+ *
+ * @bpf_map:
+ * Do a check when the kernel generate and return a file descriptor for
+ * eBPF maps.
+ *
+ * @map: bpf map that we want to access
+ * @mask: the access flags
+ *
+ * @bpf_prog:
+ * Do a check when the kernel generate and return a file descriptor for
+ * eBPF programs.
+ *
+ * @prog: bpf prog that userspace want to use.
+ *
+ * @bpf_map_alloc_security:
+ * Initialize the security field inside bpf map.
+ *
+ * @bpf_map_free_security:
+ * Clean up the security information stored inside bpf map.
+ *
+ * @bpf_prog_alloc_security:
+ * Initialize the security field inside bpf program.
+ *
+ * @bpf_prog_free_security:
+ * Clean up the security information stored inside bpf prog.
+ *
*/
union security_list_options {
int (*binder_set_context_mgr)(struct task_struct *mgr);
@@ -1682,6 +1716,17 @@ union security_list_options {
struct audit_context *actx);
void (*audit_rule_free)(void *lsmrule);
#endif /* CONFIG_AUDIT */
+
+#ifdef CONFIG_BPF_SYSCALL
+ int (*bpf)(int cmd, union bpf_attr *attr,
+ unsigned int size);
+ int (*bpf_map)(struct bpf_map *map, fmode_t fmode);
+ int (*bpf_prog)(struct bpf_prog *prog);
+ int (*bpf_map_alloc_security)(struct bpf_map *map);
+ void (*bpf_map_free_security)(struct bpf_map *map);
+ int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
+ void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
+#endif /* CONFIG_BPF_SYSCALL */
};
struct security_hook_heads {
@@ -1901,6 +1946,15 @@ struct security_hook_heads {
struct list_head audit_rule_match;
struct list_head audit_rule_free;
#endif /* CONFIG_AUDIT */
+#ifdef CONFIG_BPF_SYSCALL
+ struct list_head bpf;
+ struct list_head bpf_map;
+ struct list_head bpf_prog;
+ struct list_head bpf_map_alloc_security;
+ struct list_head bpf_map_free_security;
+ struct list_head bpf_prog_alloc_security;
+ struct list_head bpf_prog_free_security;
+#endif /* CONFIG_BPF_SYSCALL */
} __randomize_layout;
/*
diff --git a/include/linux/security.h b/include/linux/security.h
index ce6265960d6c..18800b0911e5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -31,6 +31,7 @@
#include <linux/string.h>
#include <linux/mm.h>
#include <linux/fs.h>
+#include <linux/bpf.h>
struct linux_binprm;
struct cred;
@@ -1730,6 +1731,50 @@ static inline void securityfs_remove(struct dentry *dentry)
#endif
+#ifdef CONFIG_BPF_SYSCALL
+#ifdef CONFIG_SECURITY
+extern int security_bpf(int cmd, union bpf_attr *attr, unsigned int size);
+extern int security_bpf_map(struct bpf_map *map, fmode_t fmode);
+extern int security_bpf_prog(struct bpf_prog *prog);
+extern int security_bpf_map_alloc(struct bpf_map *map);
+extern void security_bpf_map_free(struct bpf_map *map);
+extern int security_bpf_prog_alloc(struct bpf_prog_aux *aux);
+extern void security_bpf_prog_free(struct bpf_prog_aux *aux);
+#else
+static inline int security_bpf(int cmd, union bpf_attr *attr,
+ unsigned int size)
+{
+ return 0;
+}
+
+static inline int security_bpf_map(struct bpf_map *map, fmode_t fmode)
+{
+ return 0;
+}
+
+static inline int security_bpf_prog(struct bpf_prog *prog)
+{
+ return 0;
+}
+
+static inline int security_bpf_map_alloc(struct bpf_map *map)
+{
+ return 0;
+}
+
+static inline void security_bpf_map_free(struct bpf_map *map)
+{ }
+
+static inline int security_bpf_prog_alloc(struct bpf_prog_aux *aux)
+{
+ return 0;
+}
+
+static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
+{ }
+#endif /* CONFIG_SECURITY */
+#endif /* CONFIG_BPF_SYSCALL */
+
#ifdef CONFIG_SECURITY
static inline char *alloc_secdata(void)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index b02582ead9a4..d3e152e282d8 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -210,6 +210,7 @@ static void bpf_map_free_deferred(struct work_struct *work)
struct bpf_map *map = container_of(work, struct bpf_map, work);
bpf_map_uncharge_memlock(map);
+ security_bpf_map_free(map);
/* implementation dependent freeing */
map->ops->map_free(map);
}
@@ -323,6 +324,12 @@ static const struct file_operations bpf_map_fops = {
int bpf_map_new_fd(struct bpf_map *map, int flags)
{
+ int ret;
+
+ ret = security_bpf_map(map, OPEN_FMODE(flags));
+ if (ret < 0)
+ return ret;
+
return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
flags | O_CLOEXEC);
}
@@ -403,10 +410,14 @@ static int map_create(union bpf_attr *attr)
atomic_set(&map->refcnt, 1);
atomic_set(&map->usercnt, 1);
- err = bpf_map_charge_memlock(map);
+ err = security_bpf_map_alloc(map);
if (err)
goto free_map_nouncharge;
+ err = bpf_map_charge_memlock(map);
+ if (err)
+ goto free_map_sec;
+
err = bpf_map_alloc_id(map);
if (err)
goto free_map;
@@ -428,6 +439,8 @@ static int map_create(union bpf_attr *attr)
free_map:
bpf_map_uncharge_memlock(map);
+free_map_sec:
+ security_bpf_map_free(map);
free_map_nouncharge:
map->ops->map_free(map);
return err;
@@ -906,6 +919,7 @@ static void __bpf_prog_put_rcu(struct rcu_head *rcu)
free_used_maps(aux);
bpf_prog_uncharge_memlock(aux->prog);
+ security_bpf_prog_free(aux);
bpf_prog_free(aux->prog);
}
@@ -964,6 +978,12 @@ static const struct file_operations bpf_prog_fops = {
int bpf_prog_new_fd(struct bpf_prog *prog)
{
+ int ret;
+
+ ret = security_bpf_prog(prog);
+ if (ret < 0)
+ return ret;
+
return anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
O_RDWR | O_CLOEXEC);
}
@@ -1103,10 +1123,14 @@ static int bpf_prog_load(union bpf_attr *attr)
if (!prog)
return -ENOMEM;
- err = bpf_prog_charge_memlock(prog);
+ err = security_bpf_prog_alloc(prog->aux);
if (err)
goto free_prog_nouncharge;
+ err = bpf_prog_charge_memlock(prog);
+ if (err)
+ goto free_prog_sec;
+
prog->len = attr->insn_cnt;
err = -EFAULT;
@@ -1164,6 +1188,8 @@ static int bpf_prog_load(union bpf_attr *attr)
free_used_maps(prog->aux);
free_prog:
bpf_prog_uncharge_memlock(prog);
+free_prog_sec:
+ security_bpf_prog_free(prog->aux);
free_prog_nouncharge:
bpf_prog_free(prog);
return err;
@@ -1630,6 +1656,10 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
if (copy_from_user(&attr, uattr, size) != 0)
return -EFAULT;
+ err = security_bpf(cmd, &attr, size);
+ if (err < 0)
+ return err;
+
switch (cmd) {
case BPF_MAP_CREATE:
err = map_create(&attr);
diff --git a/security/security.c b/security/security.c
index 4bf0f571b4ef..1cd8526cb0b7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -12,6 +12,7 @@
* (at your option) any later version.
*/
+#include <linux/bpf.h>
#include <linux/capability.h>
#include <linux/dcache.h>
#include <linux/module.h>
@@ -1703,3 +1704,34 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule,
actx);
}
#endif /* CONFIG_AUDIT */
+
+#ifdef CONFIG_BPF_SYSCALL
+int security_bpf(int cmd, union bpf_attr *attr, unsigned int size)
+{
+ return call_int_hook(bpf, 0, cmd, attr, size);
+}
+int security_bpf_map(struct bpf_map *map, fmode_t fmode)
+{
+ return call_int_hook(bpf_map, 0, map, fmode);
+}
+int security_bpf_prog(struct bpf_prog *prog)
+{
+ return call_int_hook(bpf_prog, 0, prog);
+}
+int security_bpf_map_alloc(struct bpf_map *map)
+{
+ return call_int_hook(bpf_map_alloc_security, 0, map);
+}
+int security_bpf_prog_alloc(struct bpf_prog_aux *aux)
+{
+ return call_int_hook(bpf_prog_alloc_security, 0, aux);
+}
+void security_bpf_map_free(struct bpf_map *map)
+{
+ call_void_hook(bpf_map_free_security, map);
+}
+void security_bpf_prog_free(struct bpf_prog_aux *aux)
+{
+ call_void_hook(bpf_prog_free_security, aux);
+}
+#endif /* CONFIG_BPF_SYSCALL */
--
2.15.0.rc0.271.g36b669edcc-goog
^ permalink raw reply related
* [PATCH net-next v5 4/5] selinux: bpf: Add selinux check for eBPF syscall operations
From: Chenbo Feng @ 2017-10-12 20:55 UTC (permalink / raw)
To: netdev
Cc: Jeffrey Vander Stoep, Alexei Starovoitov, lorenzo,
Daniel Borkmann, Stephen Smalley, James Morris, Paul Moore,
Chenbo Feng
In-Reply-To: <20171012205510.36028-1-chenbofeng.kernel@gmail.com>
From: Chenbo Feng <fengc@google.com>
Implement the actual checks introduced to eBPF related syscalls. This
implementation use the security field inside bpf object to store a sid that
identify the bpf object. And when processes try to access the object,
selinux will check if processes have the right privileges. The creation
of eBPF object are also checked at the general bpf check hook and new
cmd introduced to eBPF domain can also be checked there.
Signed-off-by: Chenbo Feng <fengc@google.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
security/selinux/hooks.c | 111 ++++++++++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 2 +
security/selinux/include/objsec.h | 4 ++
3 files changed, 117 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f5d304736852..12cf7de8cbed 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -85,6 +85,7 @@
#include <linux/export.h>
#include <linux/msg.h>
#include <linux/shm.h>
+#include <linux/bpf.h>
#include "avc.h"
#include "objsec.h"
@@ -6252,6 +6253,106 @@ static void selinux_ib_free_security(void *ib_sec)
}
#endif
+#ifdef CONFIG_BPF_SYSCALL
+static int selinux_bpf(int cmd, union bpf_attr *attr,
+ unsigned int size)
+{
+ u32 sid = current_sid();
+ int ret;
+
+ switch (cmd) {
+ case BPF_MAP_CREATE:
+ ret = avc_has_perm(sid, sid, SECCLASS_BPF, BPF__MAP_CREATE,
+ NULL);
+ break;
+ case BPF_PROG_LOAD:
+ ret = avc_has_perm(sid, sid, SECCLASS_BPF, BPF__PROG_LOAD,
+ NULL);
+ break;
+ default:
+ ret = 0;
+ break;
+ }
+
+ return ret;
+}
+
+static u32 bpf_map_fmode_to_av(fmode_t fmode)
+{
+ u32 av = 0;
+
+ if (fmode & FMODE_READ)
+ av |= BPF__MAP_READ;
+ if (fmode & FMODE_WRITE)
+ av |= BPF__MAP_WRITE;
+ return av;
+}
+
+static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
+{
+ u32 sid = current_sid();
+ struct bpf_security_struct *bpfsec;
+
+ bpfsec = map->security;
+ return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF,
+ bpf_map_fmode_to_av(fmode), NULL);
+}
+
+static int selinux_bpf_prog(struct bpf_prog *prog)
+{
+ u32 sid = current_sid();
+ struct bpf_security_struct *bpfsec;
+
+ bpfsec = prog->aux->security;
+ return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF,
+ BPF__PROG_RUN, NULL);
+}
+
+static int selinux_bpf_map_alloc(struct bpf_map *map)
+{
+ struct bpf_security_struct *bpfsec;
+
+ bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
+ if (!bpfsec)
+ return -ENOMEM;
+
+ bpfsec->sid = current_sid();
+ map->security = bpfsec;
+
+ return 0;
+}
+
+static void selinux_bpf_map_free(struct bpf_map *map)
+{
+ struct bpf_security_struct *bpfsec = map->security;
+
+ map->security = NULL;
+ kfree(bpfsec);
+}
+
+static int selinux_bpf_prog_alloc(struct bpf_prog_aux *aux)
+{
+ struct bpf_security_struct *bpfsec;
+
+ bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
+ if (!bpfsec)
+ return -ENOMEM;
+
+ bpfsec->sid = current_sid();
+ aux->security = bpfsec;
+
+ return 0;
+}
+
+static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
+{
+ struct bpf_security_struct *bpfsec = aux->security;
+
+ aux->security = NULL;
+ kfree(bpfsec);
+}
+#endif
+
static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
@@ -6471,6 +6572,16 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(audit_rule_match, selinux_audit_rule_match),
LSM_HOOK_INIT(audit_rule_free, selinux_audit_rule_free),
#endif
+
+#ifdef CONFIG_BPF_SYSCALL
+ LSM_HOOK_INIT(bpf, selinux_bpf),
+ LSM_HOOK_INIT(bpf_map, selinux_bpf_map),
+ LSM_HOOK_INIT(bpf_prog, selinux_bpf_prog),
+ LSM_HOOK_INIT(bpf_map_alloc_security, selinux_bpf_map_alloc),
+ LSM_HOOK_INIT(bpf_prog_alloc_security, selinux_bpf_prog_alloc),
+ LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
+ LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
+#endif
};
static __init int selinux_init(void)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 35ffb29a69cb..0a7023b5f000 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -237,6 +237,8 @@ struct security_class_mapping secclass_map[] = {
{ "access", NULL } },
{ "infiniband_endport",
{ "manage_subnet", NULL } },
+ { "bpf",
+ {"map_create", "map_read", "map_write", "prog_load", "prog_run"} },
{ NULL }
};
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 1649cd18eb0b..3d54468ce334 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -150,6 +150,10 @@ struct pkey_security_struct {
u32 sid; /* SID of pkey */
};
+struct bpf_security_struct {
+ u32 sid; /*SID of bpf obj creater*/
+};
+
extern unsigned int selinux_checkreqprot;
#endif /* _SELINUX_OBJSEC_H_ */
--
2.15.0.rc0.271.g36b669edcc-goog
^ permalink raw reply related
* [PATCH net-next v5 5/5] selinux: bpf: Add addtional check for bpf object file receive
From: Chenbo Feng @ 2017-10-12 20:55 UTC (permalink / raw)
To: netdev
Cc: Jeffrey Vander Stoep, Alexei Starovoitov, lorenzo,
Daniel Borkmann, Stephen Smalley, James Morris, Paul Moore,
Chenbo Feng
In-Reply-To: <20171012205510.36028-1-chenbofeng.kernel@gmail.com>
From: Chenbo Feng <fengc@google.com>
Introduce a bpf object related check when sending and receiving files
through unix domain socket as well as binder. It checks if the receiving
process have privilege to read/write the bpf map or use the bpf program.
This check is necessary because the bpf maps and programs are using a
anonymous inode as their shared inode so the normal way of checking the
files and sockets when passing between processes cannot work properly on
eBPF object. This check only works when the BPF_SYSCALL is configured.
Signed-off-by: Chenbo Feng <fengc@google.com>
---
include/linux/bpf.h | 3 +++
kernel/bpf/syscall.c | 4 ++--
security/selinux/hooks.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 225740688ab7..81d6c01b8825 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -285,6 +285,9 @@ int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
#ifdef CONFIG_BPF_SYSCALL
DECLARE_PER_CPU(int, bpf_prog_active);
+extern const struct file_operations bpf_map_fops;
+extern const struct file_operations bpf_prog_fops;
+
#define BPF_PROG_TYPE(_id, _ops) \
extern const struct bpf_verifier_ops _ops;
#define BPF_MAP_TYPE(_id, _ops) \
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index d3e152e282d8..8bdb98aa7f34 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp, const char __user *buf,
return -EINVAL;
}
-static const struct file_operations bpf_map_fops = {
+const struct file_operations bpf_map_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = bpf_map_show_fdinfo,
#endif
@@ -967,7 +967,7 @@ static void bpf_prog_show_fdinfo(struct seq_file *m, struct file *filp)
}
#endif
-static const struct file_operations bpf_prog_fops = {
+const struct file_operations bpf_prog_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = bpf_prog_show_fdinfo,
#endif
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 12cf7de8cbed..ef7e5c1de640 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1815,6 +1815,10 @@ static inline int file_path_has_perm(const struct cred *cred,
return inode_has_perm(cred, file_inode(file), av, &ad);
}
+#ifdef CONFIG_BPF_SYSCALL
+static int bpf_fd_pass(struct file *file, u32 sid);
+#endif
+
/* Check whether a task can use an open file descriptor to
access an inode in a given way. Check access to the
descriptor itself, and then use dentry_has_perm to
@@ -1845,6 +1849,12 @@ static int file_has_perm(const struct cred *cred,
goto out;
}
+#ifdef CONFIG_BPF_SYSCALL
+ rc = bpf_fd_pass(file, cred_sid(cred));
+ if (rc)
+ return rc;
+#endif
+
/* av is zero if only checking access to the descriptor. */
rc = 0;
if (av)
@@ -2165,6 +2175,12 @@ static int selinux_binder_transfer_file(struct task_struct *from,
return rc;
}
+#ifdef CONFIG_BPF_SYSCALL
+ rc = bpf_fd_pass(file, sid);
+ if (rc)
+ return rc;
+#endif
+
if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
return 0;
@@ -6288,6 +6304,39 @@ static u32 bpf_map_fmode_to_av(fmode_t fmode)
return av;
}
+/* This function will check the file pass through unix socket or binder to see
+ * if it is a bpf related object. And apply correspinding checks on the bpf
+ * object based on the type. The bpf maps and programs, not like other files and
+ * socket, are using a shared anonymous inode inside the kernel as their inode.
+ * So checking that inode cannot identify if the process have privilege to
+ * access the bpf object and that's why we have to add this additional check in
+ * selinux_file_receive and selinux_binder_transfer_files.
+ */
+static int bpf_fd_pass(struct file *file, u32 sid)
+{
+ struct bpf_security_struct *bpfsec;
+ struct bpf_prog *prog;
+ struct bpf_map *map;
+ int ret;
+
+ if (file->f_op == &bpf_map_fops) {
+ map = file->private_data;
+ bpfsec = map->security;
+ ret = avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_MAP,
+ bpf_map_fmode_to_av(file->f_mode), NULL);
+ if (ret)
+ return ret;
+ } else if (file->f_op == &bpf_prog_fops) {
+ prog = file->private_data;
+ bpfsec = prog->aux->security;
+ ret = avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_PROG,
+ BPF__PROG_RUN, NULL);
+ if (ret)
+ return ret;
+ }
+ return 0;
+}
+
static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
{
u32 sid = current_sid();
--
2.15.0.rc0.271.g36b669edcc-goog
^ permalink raw reply related
* Re: [PATCH net-next 01/12] bpf: verifier: set reg_type on context accesses in second pass
From: Jakub Kicinski @ 2017-10-12 20:56 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev, oss-drivers, alexei.starovoitov
In-Reply-To: <59DFD3DE.9050907@iogearbox.net>
On Thu, 12 Oct 2017 22:43:10 +0200, Daniel Borkmann wrote:
> On 10/12/2017 07:34 PM, Jakub Kicinski wrote:
> > Use a simplified is_valid_access() callback when verifier
> > is used for program analysis by non-host JITs. This allows
> > us to teach the verifier about packet start and packet end
> > offsets for direct packet access.
> >
> > We can extend the callback as needed but for most packet
> > processing needs there isn't much more the offloads may
> > require.
> >
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Reviewed-by: Simon Horman <simon.horman@netronome.com>
> > ---
> > CC: alexei.starovoitov@gmail.com
> > CC: daniel@iogearbox.net
> >
> > kernel/bpf/verifier.c | 43 +++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 37 insertions(+), 6 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 2cdbcc4f8f6b..9755279d94cb 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -813,6 +813,36 @@ static int check_packet_access(struct bpf_verifier_env *env, u32 regno, int off,
> > return err;
> > }
> >
> > +static bool analyzer_is_valid_access(struct bpf_verifier_env *env, int off,
> > + struct bpf_insn_access_aux *info)
> > +{
> > + switch (env->prog->type) {
> > + case BPF_PROG_TYPE_XDP:
> > + switch (off) {
> > + case offsetof(struct xdp_buff, data):
> > + info->reg_type = PTR_TO_PACKET;
> > + return true;
> > + case offsetof(struct xdp_buff, data_end):
> > + info->reg_type = PTR_TO_PACKET_END;
> > + return true;
> > + }
> > + return false;
> > + case BPF_PROG_TYPE_SCHED_CLS:
> > + switch (off) {
> > + case offsetof(struct sk_buff, data):
> > + info->reg_type = PTR_TO_PACKET;
> > + return true;
> > + case offsetof(struct sk_buff, cb) +
> > + offsetof(struct bpf_skb_data_end, data_end):
> > + info->reg_type = PTR_TO_PACKET_END;
> > + return true;
> > + }
> > + return false;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > /* check access to 'struct bpf_context' fields. Supports fixed offsets only */
> > static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, int off, int size,
> > enum bpf_access_type t, enum bpf_reg_type *reg_type)
> > @@ -821,12 +851,13 @@ static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, int off,
> > .reg_type = *reg_type,
> > };
> >
> > - /* for analyzer ctx accesses are already validated and converted */
> > - if (env->analyzer_ops)
> > - return 0;
> > -
> > - if (env->prog->aux->ops->is_valid_access &&
> > - env->prog->aux->ops->is_valid_access(off, size, t, &info)) {
> > + if (env->analyzer_ops) {
> > + if (analyzer_is_valid_access(env, off, &info)) {
> > + *reg_type = info.reg_type;
>
> Is there some specific issue with the is_valid_access() callbacks that you
> need to do this (I couldn't parse that out of the commit message)?
Do you mean why not just call is_valid_access()? The offsets are
translated, so is_valid_access() will use user space __sk_buff's
offsets while we have the kernel's sk_buff offsets here...
> It would be nice to keep the reg_type setting in one place, meaning
> the callbacks themselves, so we wouldn't need to maintain this in
> multiple places.
Hm.. I though this was the smallest and simplest change. I could
translate the offsets but that seems wobbly. Or try to consolidate the
call into the same if () branch? Not sure..
As a bonus info I discovered there is a bug in -net with how things are
converted. We allow arithmetic on context pointers but then only
look at the insn.off in the converter... I'm working on a fix.
^ permalink raw reply
* Re: [net-next V7 PATCH 3/5] bpf: cpumap xdp_buff to skb conversion and allocation
From: Edward Cree @ 2017-10-12 21:13 UTC (permalink / raw)
To: Jesper Dangaard Brouer, netdev
Cc: jakub.kicinski, Michael S. Tsirkin, pavel.odintsov, Jason Wang,
mchan, John Fastabend, peter.waskiewicz.jr, ast, Daniel Borkmann,
Alexei Starovoitov, Andy Gospodarek
In-Reply-To: <150781121989.9409.14542782810603037426.stgit@firesoul>
On 12/10/17 13:26, Jesper Dangaard Brouer wrote:
> This patch makes cpumap functional, by adding SKB allocation and
> invoking the network stack on the dequeuing CPU.
>
> For constructing the SKB on the remote CPU, the xdp_buff in converted
> into a struct xdp_pkt, and it mapped into the top headroom of the
> packet, to avoid allocating separate mem. For now, struct xdp_pkt is
> just a cpumap internal data structure, with info carried between
> enqueue to dequeue.
<snip>
> +struct sk_buff *cpu_map_build_skb(struct bpf_cpu_map_entry *rcpu,
> + struct xdp_pkt *xdp_pkt)
> +{
> + unsigned int frame_size;
> + void *pkt_data_start;
> + struct sk_buff *skb;
> +
> + /* build_skb need to place skb_shared_info after SKB end, and
> + * also want to know the memory "truesize". Thus, need to
> + * know the memory frame size backing xdp_buff.
> + *
> + * XDP was designed to have PAGE_SIZE frames, but this
> + * assumption is not longer true with ixgbe and i40e. It
> + * would be preferred to set frame_size to 2048 or 4096
> + * depending on the driver.
> + * frame_size = 2048;
> + * frame_len = frame_size - sizeof(*xdp_pkt);
> + *
> + * Instead, with info avail, skb_shared_info in placed after
> + * packet len. This, unfortunately fakes the truesize.
> + * Another disadvantage of this approach, the skb_shared_info
> + * is not at a fixed memory location, with mixed length
> + * packets, which is bad for cache-line hotness.
> + */
> + frame_size = SKB_DATA_ALIGN(xdp_pkt->len) + xdp_pkt->headroom +
> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +
> + pkt_data_start = xdp_pkt->data - xdp_pkt->headroom;
> + skb = build_skb(pkt_data_start, frame_size);
> + if (!skb)
> + return NULL;
> +
> + skb_reserve(skb, xdp_pkt->headroom);
> + __skb_put(skb, xdp_pkt->len);
> + if (xdp_pkt->metasize)
> + skb_metadata_set(skb, xdp_pkt->metasize);
> +
> + /* Essential SKB info: protocol and skb->dev */
> + skb->protocol = eth_type_trans(skb, xdp_pkt->dev_rx);
> +
> + /* Optional SKB info, currently missing:
> + * - HW checksum info (skb->ip_summed)
> + * - HW RX hash (skb_set_hash)
> + * - RX ring dev queue index (skb_record_rx_queue)
> + */
One possibility for dealing with these and related issues — also things
like the proper way to free an xdp_buff if SKB creation fails, which
might not be page_frag_free() for some drivers with unusual recycle ring
implementations — is to have a new ndo for 'receiving' an xdp_pkt from a
cpumap redirect.
Since you're always receiving from the same driver that enqueued it, even
the structure of the metadata stored in the top of the packet page
doesn't have to be standardised; instead, each driver can put there just
whatever happens to be needed for its ndo_xdp_rx routine. (Though there
would probably be standard enqueue and dequeue functions that the
'common-case' drivers could use.)
In some cases, the driver could even just leave in the page the packet
prefix it got from the NIC, rather than reading it and then writing an
interpreted version back, thus minimising the number of packet-page
cachelines the 'bottom half' RX function has to touch (it would still
need to write in anything it got from the RX event, of course).
It shouldn't be much work as many driver RX routines are already
structured this way — sfc, for instance, has a split into efx_rx_packet()
and __efx_rx_packet(), as a software pipeline for prefetching.
-Ed
^ permalink raw reply
* [PATCH] atm: fore200e: mark expected switch fall-throughs
From: Gustavo A. R. Silva @ 2017-10-12 21:11 UTC (permalink / raw)
To: Chas Williams
Cc: linux-atm-general, netdev, linux-kernel, Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
drivers/atm/fore200e.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/atm/fore200e.c b/drivers/atm/fore200e.c
index f8b7e86..126855e 100644
--- a/drivers/atm/fore200e.c
+++ b/drivers/atm/fore200e.c
@@ -358,26 +358,33 @@ fore200e_shutdown(struct fore200e* fore200e)
case FORE200E_STATE_COMPLETE:
kfree(fore200e->stats);
+ /* fall through */
case FORE200E_STATE_IRQ:
free_irq(fore200e->irq, fore200e->atm_dev);
+ /* fall through */
case FORE200E_STATE_ALLOC_BUF:
fore200e_free_rx_buf(fore200e);
+ /* fall through */
case FORE200E_STATE_INIT_BSQ:
fore200e_uninit_bs_queue(fore200e);
+ /* fall through */
case FORE200E_STATE_INIT_RXQ:
fore200e->bus->dma_chunk_free(fore200e, &fore200e->host_rxq.status);
fore200e->bus->dma_chunk_free(fore200e, &fore200e->host_rxq.rpd);
+ /* fall through */
case FORE200E_STATE_INIT_TXQ:
fore200e->bus->dma_chunk_free(fore200e, &fore200e->host_txq.status);
fore200e->bus->dma_chunk_free(fore200e, &fore200e->host_txq.tpd);
+ /* fall through */
case FORE200E_STATE_INIT_CMDQ:
fore200e->bus->dma_chunk_free(fore200e, &fore200e->host_cmdq.status);
+ /* fall through */
case FORE200E_STATE_INITIALIZE:
/* nothing to do for that state */
@@ -390,6 +397,7 @@ fore200e_shutdown(struct fore200e* fore200e)
case FORE200E_STATE_MAP:
fore200e->bus->unmap(fore200e);
+ /* fall through */
case FORE200E_STATE_CONFIGURE:
/* nothing to do for that state */
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next 01/12] bpf: verifier: set reg_type on context accesses in second pass
From: Daniel Borkmann @ 2017-10-12 21:33 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, oss-drivers, alexei.starovoitov
In-Reply-To: <20171012135632.1fa408bc@cakuba.netronome.com>
On 10/12/2017 10:56 PM, Jakub Kicinski wrote:
> On Thu, 12 Oct 2017 22:43:10 +0200, Daniel Borkmann wrote:
[...]
>> It would be nice to keep the reg_type setting in one place, meaning
>> the callbacks themselves, so we wouldn't need to maintain this in
>> multiple places.
>
> Hm.. I though this was the smallest and simplest change. I could
> translate the offsets but that seems wobbly. Or try to consolidate the
> call into the same if () branch? Not sure..
Different callbacks for post-verification would be good at min as it
would allow to keep all the context access info in one place for a
given type at least.
> As a bonus info I discovered there is a bug in -net with how things are
> converted. We allow arithmetic on context pointers but then only
> look at the insn.off in the converter... I'm working on a fix.
Ohh well, good catch, indeed! :( Can you also add coverage to the
bpf selftests for this?
Thanks,
Daniel
^ permalink raw reply
* Re: [RFC 0/3] Adding config get/set to devlink
From: Michal Kubecek @ 2017-10-12 21:36 UTC (permalink / raw)
To: Florian Fainelli
Cc: David Miller, jiri, roopa, steven.lin1, netdev, jiri,
michael.chan, linux-pci, linville, gospo
In-Reply-To: <21ab4a5d-0b6a-7976-7bf0-acd334f2613f@gmail.com>
On Thu, Oct 12, 2017 at 12:20:07PM -0700, Florian Fainelli wrote:
> On 10/12/2017 12:06 PM, David Miller wrote:
> >
> > One suggestion is that devlink is used for getting ethtool stats for
> > objects lacking netdev representor's, and a new genetlink family is
> > used for netdev based ethtool.
>
> Right, I was also thinking along those lines that we we would have a new
> generic netlink family for ethtool to support ethtool over netlink.
This is what I plan to work on on next SUSE Hackweek in November. But
I'm, of course, open to suggestions and I don't insist on this approach.
Michal Kubecek
^ permalink raw reply
* Re: [patch net-next 00/34] net: sched: allow qdiscs to share filter block instances
From: David Ahern @ 2017-10-12 21:37 UTC (permalink / raw)
To: Jiri Pirko, netdev
Cc: davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
matanb, leonro, idosch, jakub.kicinski, ast, daniel, simon.horman,
pieter.jansenvanvuuren, john.hurley, edumazet, alexander.h.duyck,
john.fastabend, willemb
In-Reply-To: <20171012171823.1431-1-jiri@resnulli.us>
On 10/12/17 11:17 AM, Jiri Pirko wrote:
> So back to the example. First, we create 2 qdiscs. Both will share
> block number 22. "22" is just an identification. If we don't pass any
> block number, a new one will be generated by kernel:
>
> $ tc qdisc add dev ens7 ingress block 22
> ^^^^^^^^
> $ tc qdisc add dev ens8 ingress block 22
> ^^^^^^^^
>
> Now if we list the qdiscs, we will see the block index in the output:
>
> $ tc qdisc
> qdisc ingress ffff: dev ens7 parent ffff:fff1 block 22
> qdisc ingress ffff: dev ens8 parent ffff:fff1 block 22
>
> Now we can add filter to any of qdiscs sharing the same block:
>
> $ tc filter add dev ens7 parent ffff: protocol ip pref 25 flower dst_ip 192.168.0.0/16 action drop
>
>
> We will see the same output if we list filters for ens7 and ens8, including stats:
>
> $ tc -s filter show dev ens7 ingress
> filter protocol ip pref 25 flower chain 0
> filter protocol ip pref 25 flower chain 0 handle 0x1
> eth_type ipv4
> dst_ip 192.168.0.0/16
> not_in_hw
> action order 1: gact action drop
> random type none pass val 0
> index 1 ref 1 bind 1 installed 39 sec used 2 sec
> Action statistics:
> Sent 3108 bytes 37 pkt (dropped 37, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
>
> $ tc -s filter show dev ens8 ingress
> filter protocol ip pref 25 flower chain 0
> filter protocol ip pref 25 flower chain 0 handle 0x1
> eth_type ipv4
> dst_ip 192.168.0.0/16
> not_in_hw
> action order 1: gact action drop
> random type none pass val 0
> index 1 ref 1 bind 1 installed 40 sec used 3 sec
> Action statistics:
> Sent 3108 bytes 37 pkt (dropped 37, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
This seems like really odd semantics to me ... a filter added to one
device shows up on another.
Why not make the shared block a standalone object that is configured
through its own set of commands and then referenced by both devices?
^ permalink raw reply
* Re: [PATCH net-next 01/12] bpf: verifier: set reg_type on context accesses in second pass
From: Jakub Kicinski @ 2017-10-12 21:39 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev, oss-drivers, alexei.starovoitov
In-Reply-To: <59DFDFA1.3020809@iogearbox.net>
On Thu, 12 Oct 2017 23:33:21 +0200, Daniel Borkmann wrote:
> On 10/12/2017 10:56 PM, Jakub Kicinski wrote:
> > On Thu, 12 Oct 2017 22:43:10 +0200, Daniel Borkmann wrote:
> [...]
> >> It would be nice to keep the reg_type setting in one place, meaning
> >> the callbacks themselves, so we wouldn't need to maintain this in
> >> multiple places.
> >
> > Hm.. I though this was the smallest and simplest change. I could
> > translate the offsets but that seems wobbly. Or try to consolidate the
> > call into the same if () branch? Not sure..
>
> Different callbacks for post-verification would be good at min as it
> would allow to keep all the context access info in one place for a
> given type at least.
Sorry to be clear - you're suggesting adding a new callback to struct
bpf_verifier_ops, or swapping the struct bpf_verifier_ops for a
special post-verification one?
> > As a bonus info I discovered there is a bug in -net with how things are
> > converted. We allow arithmetic on context pointers but then only
> > look at the insn.off in the converter... I'm working on a fix.
>
> Ohh well, good catch, indeed! :( Can you also add coverage to the
> bpf selftests for this?
Will do!
^ permalink raw reply
* Re: Ethtool question
From: Ben Greear @ 2017-10-12 21:45 UTC (permalink / raw)
To: David Miller, linville; +Cc: netdev
In-Reply-To: <20171011.134919.321292333200236097.davem@davemloft.net>
On 10/11/2017 01:49 PM, David Miller wrote:
> From: "John W. Linville" <linville@tuxdriver.com>
> Date: Wed, 11 Oct 2017 16:44:07 -0400
>
>> On Wed, Oct 11, 2017 at 09:51:56AM -0700, Ben Greear wrote:
>>> I noticed today that setting some ethtool settings to the same value
>>> returns an error code. I would think this should silently return
>>> success instead? Makes it easier to call it from scripts this way:
>>>
>>> [root@lf0313-6477 lanforge]# ethtool -L eth3 combined 1
>>> combined unmodified, ignoring
>>> no channel parameters changed, aborting
>>> current values: tx 0 rx 0 other 1 combined 1
>>> [root@lf0313-6477 lanforge]# echo $?
>>> 1
>>
>> I just had this discussion a couple of months ago with someone. My
>> initial feeling was like you, a no-op is not a failure. But someone
>> convinced me otherwise...I will now endeavour to remember who that
>> was and how they convinced me...
>>
>> Anyone else have input here?
>
> I guess this usually happens when drivers don't support changing the
> settings at all. So they just make their ethtool operation for the
> 'set' always return an error.
>
> We could have a generic ethtool helper that does "get" and then if the
> "set" request is identical just return zero.
>
> But from another perspective, the error returned from the "set" in this
> situation also indicates to the user that the driver does not support
> the "set" operation which has value and meaning in and of itself. And
> we'd lose that with the given suggestion.
In my case, the driver (igb) does support the set, my program just made the same
ethtool call several times and it fails after the initial change (that actually
changes something), as best as I can figure.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [patch net-next 06/34] net: core: use dev->ingress_queue instead of tp->q
From: Daniel Borkmann @ 2017-10-12 21:45 UTC (permalink / raw)
To: Jiri Pirko, netdev
Cc: davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
matanb, leonro, idosch, jakub.kicinski, ast, simon.horman,
pieter.jansenvanvuuren, john.hurley, edumazet, dsahern,
alexander.h.duyck, john.fastabend, willemb
In-Reply-To: <20171012171823.1431-7-jiri@resnulli.us>
On 10/12/2017 07:17 PM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> In sch_handle_egress and sch_handle_ingress, don't use tp->q and use
> dev->ingress_queue which stores the same pointer instead.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
> net/core/dev.c | 21 +++++++++++++++------
> 1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index fcddccb..cb9e5e5 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3273,14 +3273,18 @@ EXPORT_SYMBOL(dev_loopback_xmit);
> static struct sk_buff *
> sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
> {
> + struct netdev_queue *netdev_queue =
> + rcu_dereference_bh(dev->ingress_queue);
> struct tcf_proto *cl = rcu_dereference_bh(dev->egress_cl_list);
> struct tcf_result cl_res;
> + struct Qdisc *q;
>
> - if (!cl)
> + if (!cl || !netdev_queue)
> return skb;
> + q = netdev_queue->qdisc;
NAK, no additional overhead in the software fast-path of
sch_handle_{ingress,egress}() like this. There are users out there
that use tc in software only, so performance is critical here.
> /* qdisc_skb_cb(skb)->pkt_len was already set by the caller. */
> - qdisc_bstats_cpu_update(cl->q, skb);
> + qdisc_bstats_cpu_update(q, skb);
>
> switch (tcf_classify(skb, cl, &cl_res, false)) {
> case TC_ACT_OK:
> @@ -3288,7 +3292,7 @@ sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
> skb->tc_index = TC_H_MIN(cl_res.classid);
> break;
> case TC_ACT_SHOT:
> - qdisc_qstats_cpu_drop(cl->q);
> + qdisc_qstats_cpu_drop(q);
> *ret = NET_XMIT_DROP;
> kfree_skb(skb);
> return NULL;
> @@ -4188,16 +4192,21 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
> struct net_device *orig_dev)
> {
> #ifdef CONFIG_NET_CLS_ACT
> + struct netdev_queue *netdev_queue =
> + rcu_dereference_bh(skb->dev->ingress_queue);
> struct tcf_proto *cl = rcu_dereference_bh(skb->dev->ingress_cl_list);
> struct tcf_result cl_res;
> + struct Qdisc *q;
>
> /* If there's at least one ingress present somewhere (so
> * we get here via enabled static key), remaining devices
> * that are not configured with an ingress qdisc will bail
> * out here.
> */
> - if (!cl)
> + if (!cl || !netdev_queue)
> return skb;
> + q = netdev_queue->qdisc;
> +
> if (*pt_prev) {
> *ret = deliver_skb(skb, *pt_prev, orig_dev);
> *pt_prev = NULL;
> @@ -4205,7 +4214,7 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
>
> qdisc_skb_cb(skb)->pkt_len = skb->len;
> skb->tc_at_ingress = 1;
> - qdisc_bstats_cpu_update(cl->q, skb);
> + qdisc_bstats_cpu_update(q, skb);
>
> switch (tcf_classify(skb, cl, &cl_res, false)) {
> case TC_ACT_OK:
> @@ -4213,7 +4222,7 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
> skb->tc_index = TC_H_MIN(cl_res.classid);
> break;
> case TC_ACT_SHOT:
> - qdisc_qstats_cpu_drop(cl->q);
> + qdisc_qstats_cpu_drop(q);
> kfree_skb(skb);
> return NULL;
> case TC_ACT_STOLEN:
>
^ permalink raw reply
* Re: [PATCH net-next 01/12] bpf: verifier: set reg_type on context accesses in second pass
From: Daniel Borkmann @ 2017-10-12 21:46 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, oss-drivers, alexei.starovoitov
In-Reply-To: <20171012143950.58c1b601@cakuba.netronome.com>
On 10/12/2017 11:39 PM, Jakub Kicinski wrote:
> On Thu, 12 Oct 2017 23:33:21 +0200, Daniel Borkmann wrote:
>> On 10/12/2017 10:56 PM, Jakub Kicinski wrote:
>>> On Thu, 12 Oct 2017 22:43:10 +0200, Daniel Borkmann wrote:
>> [...]
>>>> It would be nice to keep the reg_type setting in one place, meaning
>>>> the callbacks themselves, so we wouldn't need to maintain this in
>>>> multiple places.
>>>
>>> Hm.. I though this was the smallest and simplest change. I could
>>> translate the offsets but that seems wobbly. Or try to consolidate the
>>> call into the same if () branch? Not sure..
>>
>> Different callbacks for post-verification would be good at min as it
>> would allow to keep all the context access info in one place for a
>> given type at least.
>
> Sorry to be clear - you're suggesting adding a new callback to struct
> bpf_verifier_ops, or swapping the struct bpf_verifier_ops for a
> special post-verification one?
Either way is fine by me.
>>> As a bonus info I discovered there is a bug in -net with how things are
>>> converted. We allow arithmetic on context pointers but then only
>>> look at the insn.off in the converter... I'm working on a fix.
>>
>> Ohh well, good catch, indeed! :( Can you also add coverage to the
>> bpf selftests for this?
>
> Will do!
Thanks,
Daniel
^ permalink raw reply
* Re: [RFC 0/3] Adding config get/set to devlink
From: Roopa Prabhu @ 2017-10-12 21:53 UTC (permalink / raw)
To: Florian Fainelli
Cc: David Miller, Jiří Pírko, Steve Lin,
netdev@vger.kernel.org, Jiri Pirko, Michael Chan, linux-pci,
John W. Linville, Andy Gospodarek
In-Reply-To: <21ab4a5d-0b6a-7976-7bf0-acd334f2613f@gmail.com>
On Thu, Oct 12, 2017 at 12:20 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 10/12/2017 12:06 PM, David Miller wrote:
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Date: Thu, 12 Oct 2017 08:43:59 -0700
>>
>>> Once we move ethtool (or however we name its successor) over to
>>> netlink there is an opportunity for accessing objects that do and do
>>> not have a netdevice representor today (e.g: management ports on
>>> switches) with the same interface, and devlink could be used for
>>> that.
>>
>> That is an interesting angle for including this in devlink.
>>
>> I'm not so sure what to do about this.
>>
>> One suggestion is that devlink is used for getting ethtool stats for
>> objects lacking netdev representor's, and a new genetlink family is
>> used for netdev based ethtool.
>
> Right, I was also thinking along those lines that we we would have a new
> generic netlink family for ethtool to support ethtool over netlink.
new api is fine by me. The reason for suggesting devlink was because
some of the devlink
port_* ops are close to ethtool ops that can operate on a port/netdev.
eg split_port could be a netdev operation
unless you want to split before the netdev is created.
There are some ops in devlink which are global hw parameters and not
specific to a port, those fit perfectly with
devlinks original goal.
>
>>
>> I think it's important that we don't expand the scope of devlink
>> beyond what it was originally designed for.
>
> It seems to me like devlink is well defined in what it is not for: it is
> not meant to be used for any object that is/has a net_device, but it is
> not well defined for what it can offer to these non network devices. For
> instance, we have a tremendous amount of operations that are extremely
> specific to its single user(s) such as mlx5 and mlxsw.
>
> For instance, I am not sure how the buffer reservation scheme can be
> generalized, and this is always the tricky part with a single user
> facility in that you try to generalize the best you can based on the HW
> you know. This is not a criticism or meant to be anything negative, this
> just happens to be the case, and we did not have anything better.
>
> So maybe the first thing is to clarify what devlink operations can and
> should be and what they are absolutely not allowed to cover. We should
> also clarify whether a generic set/get that Steven is proposing is
> something that we tolerate, or whether there should be specific function
> pointers implemented for each attribute, which would be more in line
> with what has been done thus far.
> --
> Florian
^ permalink raw reply
* Re: Ethtool question
From: Roopa Prabhu @ 2017-10-12 22:00 UTC (permalink / raw)
To: Ben Greear; +Cc: David Miller, John W. Linville, netdev@vger.kernel.org
In-Reply-To: <59DFE275.3050805@candelatech.com>
On Thu, Oct 12, 2017 at 2:45 PM, Ben Greear <greearb@candelatech.com> wrote:
> On 10/11/2017 01:49 PM, David Miller wrote:
>>
>> From: "John W. Linville" <linville@tuxdriver.com>
>> Date: Wed, 11 Oct 2017 16:44:07 -0400
>>
>>> On Wed, Oct 11, 2017 at 09:51:56AM -0700, Ben Greear wrote:
>>>>
>>>> I noticed today that setting some ethtool settings to the same value
>>>> returns an error code. I would think this should silently return
>>>> success instead? Makes it easier to call it from scripts this way:
>>>>
>>>> [root@lf0313-6477 lanforge]# ethtool -L eth3 combined 1
>>>> combined unmodified, ignoring
>>>> no channel parameters changed, aborting
>>>> current values: tx 0 rx 0 other 1 combined 1
>>>> [root@lf0313-6477 lanforge]# echo $?
>>>> 1
>>>
>>>
>>> I just had this discussion a couple of months ago with someone. My
>>> initial feeling was like you, a no-op is not a failure. But someone
>>> convinced me otherwise...I will now endeavour to remember who that
>>> was and how they convinced me...
>>>
>>> Anyone else have input here?
>>
>>
>> I guess this usually happens when drivers don't support changing the
>> settings at all. So they just make their ethtool operation for the
>> 'set' always return an error.
>>
>> We could have a generic ethtool helper that does "get" and then if the
>> "set" request is identical just return zero.
>>
>> But from another perspective, the error returned from the "set" in this
>> situation also indicates to the user that the driver does not support
>> the "set" operation which has value and meaning in and of itself. And
>> we'd lose that with the given suggestion.
>
>
> In my case, the driver (igb) does support the set, my program just made the
> same
> ethtool call several times and it fails after the initial change (that
> actually
> changes something), as best as I can figure.
This error is returned by ethtool user-space. It does a get, check and
then set if user has requested changes.
^ permalink raw reply
* Re: [net-next PATCH] mqprio: Reserve last 32 classid values for HW traffic classes and misc IDs
From: Jesus Sanchez-Palencia @ 2017-10-12 21:56 UTC (permalink / raw)
To: Alexander Duyck, jiri, amritha.nambiar, vinicius.gomes, netdev,
jhs, davem
In-Reply-To: <20171012182658.14632.9010.stgit@localhost.localdomain>
Hi Alex,
On 10/12/2017 11:38 AM, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> This patch makes a slight tweak to mqprio in order to bring the
> classid values used back in line with what is used for mq. The general idea
> is to reserve values :ffe0 - :ffef to identify hardware traffic classes
> normally reported via dev->num_tc. By doing this we can maintain a
> consistent behavior with mq for classid where :1 - :ffdf will represent a
> physical qdisc mapped onto a Tx queue represented by classid - 1, and the
> traffic classes will be mapped onto a known subset of classid values
> reserved for our virtual qdiscs.
>
> Note I reserved the range from :fff0 - :ffff since this way we might be
> able to reuse these classid values with clsact and ingress which would mean
> that for mq, mqprio, ingress, and clsact we should be able to maintain a
> similar classid layout.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
>
> So I thought I would put this out here as a first step towards trying to
> address some of Jiri's concerns about wanting to have a consistent
> userspace API.
>
> The plan is to follow this up with patches to ingress and clsact to look at
> exposing a set of virtual qdiscs similar to what we already have for the HW
> traffic classes in mqprio, although I won't bother with the ability to dump
> class stats since they don't actually enqueue anything.
>
> include/uapi/linux/pkt_sched.h | 1 +
> net/sched/sch_mqprio.c | 79 +++++++++++++++++++++++-----------------
> 2 files changed, 47 insertions(+), 33 deletions(-)
>
> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 099bf5528fed..174f1cf7e7f9 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -74,6 +74,7 @@ struct tc_estimator {
> #define TC_H_INGRESS (0xFFFFFFF1U)
> #define TC_H_CLSACT TC_H_INGRESS
>
> +#define TC_H_MIN_PRIORITY 0xFFE0U
> #define TC_H_MIN_INGRESS 0xFFF2U
> #define TC_H_MIN_EGRESS 0xFFF3U
>
> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
> index 6bcdfe6e7b63..a61ef119a556 100644
> --- a/net/sched/sch_mqprio.c
> +++ b/net/sched/sch_mqprio.c
> @@ -115,6 +115,10 @@ static int mqprio_init(struct Qdisc *sch, struct nlattr *opt)
> if (!netif_is_multiqueue(dev))
> return -EOPNOTSUPP;
>
> + /* make certain can allocate enough classids to handle queues */
> + if (dev->num_tx_queues >= TC_H_MIN_PRIORITY)
> + return -ENOMEM;
> +
> if (!opt || nla_len(opt) < sizeof(*qopt))
> return -EINVAL;
>
> @@ -193,7 +197,7 @@ static struct netdev_queue *mqprio_queue_get(struct Qdisc *sch,
> unsigned long cl)
> {
> struct net_device *dev = qdisc_dev(sch);
> - unsigned long ntx = cl - 1 - netdev_get_num_tc(dev);
> + unsigned long ntx = cl - 1;
>
> if (ntx >= dev->num_tx_queues)
> return NULL;
> @@ -282,38 +286,35 @@ static unsigned long mqprio_find(struct Qdisc *sch, u32 classid)
> struct net_device *dev = qdisc_dev(sch);
> unsigned int ntx = TC_H_MIN(classid);
>
> - if (ntx > dev->num_tx_queues + netdev_get_num_tc(dev))
> - return 0;
> - return ntx;
> + /* There are essentially two regions here that have valid classid
> + * values. The first region will have a classid value of 1 through
> + * num_tx_queues. All of these are backed by actual Qdiscs.
> + */
> + if (ntx < TC_H_MIN_PRIORITY)
> + return (ntx <= dev->num_tx_queues) ? ntx : 0;
> +
> + /* The second region represents the hardware traffic classes. These
> + * are represented by classid values of TC_H_MIN_PRIORITY through
> + * TC_H_MIN_PRIORITY + netdev_get_num_tc - 1
> + */
> + return ((ntx - TC_H_MIN_PRIORITY) < netdev_get_num_tc(dev)) ? ntx : 0;
> }
>
> static int mqprio_dump_class(struct Qdisc *sch, unsigned long cl,
> struct sk_buff *skb, struct tcmsg *tcm)
> {
> - struct net_device *dev = qdisc_dev(sch);
> + if (cl < TC_H_MIN_PRIORITY) {
> + struct netdev_queue *dev_queue = mqprio_queue_get(sch, cl);
> + struct net_device *dev = qdisc_dev(sch);
> + int tc = netdev_txq_to_tc(dev, cl - 1);
>
> - if (cl <= netdev_get_num_tc(dev)) {
> + tcm->tcm_parent = (tc < 0) ? 0 :
> + TC_H_MAKE(TC_H_MAJ(sch->handle),
> + TC_H_MIN(tc + TC_H_MIN_PRIORITY));
> + tcm->tcm_info = dev_queue->qdisc_sleeping->handle;
> + } else {
> tcm->tcm_parent = TC_H_ROOT;
> tcm->tcm_info = 0;
> - } else {
> - int i;
> - struct netdev_queue *dev_queue;
> -
> - dev_queue = mqprio_queue_get(sch, cl);
> - tcm->tcm_parent = 0;
> - for (i = 0; i < netdev_get_num_tc(dev); i++) {
> - struct netdev_tc_txq tc = dev->tc_to_txq[i];
> - int q_idx = cl - netdev_get_num_tc(dev);
> -
> - if (q_idx > tc.offset &&
> - q_idx <= tc.offset + tc.count) {
> - tcm->tcm_parent =
> - TC_H_MAKE(TC_H_MAJ(sch->handle),
> - TC_H_MIN(i + 1));
> - break;
> - }
> - }
> - tcm->tcm_info = dev_queue->qdisc_sleeping->handle;
> }
> tcm->tcm_handle |= TC_H_MIN(cl);
> return 0;
> @@ -324,15 +325,14 @@ static int mqprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
> __releases(d->lock)
> __acquires(d->lock)
> {
> - struct net_device *dev = qdisc_dev(sch);
> -
> - if (cl <= netdev_get_num_tc(dev)) {
> + if (cl >= TC_H_MIN_PRIORITY) {
> int i;
> __u32 qlen = 0;
> struct Qdisc *qdisc;
> struct gnet_stats_queue qstats = {0};
> struct gnet_stats_basic_packed bstats = {0};
> - struct netdev_tc_txq tc = dev->tc_to_txq[cl - 1];
> + struct net_device *dev = qdisc_dev(sch);
> + struct netdev_tc_txq tc = dev->tc_to_txq[cl & TC_BITMASK];
>
> /* Drop lock here it will be reclaimed before touching
> * statistics this is required because the d->lock we
> @@ -385,12 +385,25 @@ static void mqprio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
>
> /* Walk hierarchy with a virtual class per tc */
> arg->count = arg->skip;
> - for (ntx = arg->skip;
> - ntx < dev->num_tx_queues + netdev_get_num_tc(dev);
> - ntx++) {
> + for (ntx = arg->skip; ntx < netdev_get_num_tc(dev); ntx++) {
> + if (arg->fn(sch, ntx + TC_H_MIN_PRIORITY, arg) < 0) {
> + arg->stop = 1;
> + return;
> + }
> + arg->count++;
> + }
> +
> + /* Pad the values and skip over unused traffic classes */
> + if (ntx < TC_MAX_QUEUE) {
> + arg->count = TC_MAX_QUEUE;
> + ntx = TC_MAX_QUEUE;
> + }
> +
> + /* Reset offset, sort out remaining per-queue qdiscs */
> + for (ntx -= TC_MAX_QUEUE; ntx < dev->num_tx_queues; ntx++) {
> if (arg->fn(sch, ntx + 1, arg) < 0) {
> arg->stop = 1;
> - break;
> + return;
> }
> arg->count++;
> }
>
Tested-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
Looks good.
thanks,
Jesus
^ 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