* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: Linus Torvalds @ 2019-02-23 0:08 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David Miller, Masami Hiramatsu, Steven Rostedt, Andy Lutomirski,
Linux List Kernel Mailing, Ingo Molnar, Andrew Morton, stable,
Changbin Du, Jann Horn, Kees Cook, Andrew Lutomirski,
Daniel Borkmann, Netdev, bpf
In-Reply-To: <20190222235618.dxewmv5dukltaoxl@ast-mbp.dhcp.thefacebook.com>
On Fri, Feb 22, 2019 at 3:56 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> It will preserve existing bpf_probe_read() behavior on x86.
... but that's the worst possible situation.
It appears that people haven't understood that kernel and user
addresses are distinct, and may have written programs that are
fundamentally buggy.
And we _want_ to make it clear that they are buggy on x86-64, exactly
because x86-64 is the one that gets the most testing - by far.
So if x86-64 continues working for buggy programs, then that only
means that those bugs never get fixed.
It would be much better to try to get those things fixed, and make the
x86-64 implementation stricter, exactly so that people end up
_realizing_ that they can't just think "a pointer is a pointer, and
the context doesn't matter".
From a pure functional safety standpoint, I thought bpf already knew
what kind of a pointer it had?
Linus
^ permalink raw reply
* Re: [PATCH net 0/4] udp: a few fixes
From: David Miller @ 2019-02-23 0:09 UTC (permalink / raw)
To: pabeni; +Cc: netdev, sbrivio
In-Reply-To: <cover.1550767274.git.pabeni@redhat.com>
From: Paolo Abeni <pabeni@redhat.com>
Date: Thu, 21 Feb 2019 17:43:56 +0100
> This series includes some UDP-related fixlet. All this stuff has been
> pointed out by the sparse tool. The first two patches are just annotation
> related, while the last 2 cover some very unlikely races.
Series applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: Nadav Amit @ 2019-02-23 0:15 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Jann Horn, Alexei Starovoitov, Steven Rostedt, Linus Torvalds,
Masami Hiramatsu, Linux List Kernel Mailing, Ingo Molnar,
Andrew Morton, Changbin Du, Kees Cook, Daniel Borkmann,
Network Development, bpf@vger.kernel.org, Rick Edgecombe,
Dave Hansen, Peter Zijlstra (Intel), Igor Stoppa
In-Reply-To: <CALCETrU2V5KBA97g3O-yHiUu1acmM_K9b2a5ATmKSqGwdd-+Dw@mail.gmail.com>
> On Feb 22, 2019, at 3:59 PM, Andy Lutomirski <luto@kernel.org> wrote:
>
> On Fri, Feb 22, 2019 at 3:02 PM Jann Horn <jannh@google.com> wrote:
>> On Fri, Feb 22, 2019 at 11:39 PM Nadav Amit <namit@vmware.com> wrote:
>>>> On Feb 22, 2019, at 2:21 PM, Nadav Amit <namit@vmware.com> wrote:
>>>>
>>>>> On Feb 22, 2019, at 2:17 PM, Jann Horn <jannh@google.com> wrote:
>>>>>
>>>>> On Fri, Feb 22, 2019 at 11:08 PM Nadav Amit <namit@vmware.com> wrote:
>>>>>>> On Feb 22, 2019, at 1:43 PM, Jann Horn <jannh@google.com> wrote:
>>>>>>>
>>>>>>> (adding some people from the text_poke series to the thread, removing stable@)
>>>>>>>
>>>>>>> On Fri, Feb 22, 2019 at 8:55 PM Andy Lutomirski <luto@amacapital.net> wrote:
>>>>>>>>> On Feb 22, 2019, at 11:34 AM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>>>>>>>>> On Fri, Feb 22, 2019 at 02:30:26PM -0500, Steven Rostedt wrote:
>>>>>>>>>> On Fri, 22 Feb 2019 11:27:05 -0800
>>>>>>>>>> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>>>> On Fri, Feb 22, 2019 at 09:43:14AM -0800, Linus Torvalds wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Then we should still probably fix up "__probe_kernel_read()" to not
>>>>>>>>>>>> allow user accesses. The easiest way to do that is actually likely to
>>>>>>>>>>>> use the "unsafe_get_user()" functions *without* doing a
>>>>>>>>>>>> uaccess_begin(), which will mean that modern CPU's will simply fault
>>>>>>>>>>>> on a kernel access to user space.
>>>>>>>>>>>
>>>>>>>>>>> On bpf side the bpf_probe_read() helper just calls probe_kernel_read()
>>>>>>>>>>> and users pass both user and kernel addresses into it and expect
>>>>>>>>>>> that the helper will actually try to read from that address.
>>>>>>>>>>>
>>>>>>>>>>> If __probe_kernel_read will suddenly start failing on all user addresses
>>>>>>>>>>> it will break the expectations.
>>>>>>>>>>> How do we solve it in bpf_probe_read?
>>>>>>>>>>> Call probe_kernel_read and if that fails call unsafe_get_user byte-by-byte
>>>>>>>>>>> in the loop?
>>>>>>>>>>> That's doable, but people already complain that bpf_probe_read() is slow
>>>>>>>>>>> and shows up in their perf report.
>>>>>>>>>>
>>>>>>>>>> We're changing kprobes to add a specific flag to say that we want to
>>>>>>>>>> differentiate between kernel or user reads. Can this be done with
>>>>>>>>>> bpf_probe_read()? If it's showing up in perf report, I doubt a single
>>>>>>>>>
>>>>>>>>> so you're saying you will break existing kprobe scripts?
>>>>>>>>> I don't think it's a good idea.
>>>>>>>>> It's not acceptable to break bpf_probe_read uapi.
>>>>>>>>
>>>>>>>> If so, the uapi is wrong: a long-sized number does not reliably identify an address if you don’t separately know whether it’s a user or kernel address. s390x and 4G:4G x86_32 are the notable exceptions. I have lobbied for RISC-V and future x86_64 to join the crowd. I don’t know whether I’ll win this fight, but the uapi will probably have to change for at least s390x.
>>>>>>>>
>>>>>>>> What to do about existing scripts is a different question.
>>>>>>>
>>>>>>> This lack of logical separation between user and kernel addresses
>>>>>>> might interact interestingly with the text_poke series, specifically
>>>>>>> "[PATCH v3 05/20] x86/alternative: Initialize temporary mm for
>>>>>>> patching" (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-6-rick.p.edgecombe%40intel.com%2F&data=02%7C01%7Cnamit%40vmware.com%7Cbab53e52cc5c4ac4419008d69921d1f1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864767879731941&sdata=2tqD7udTCfNbcNLcj5SFpZt8WwK5NwtgaWMKm1Ye1EE%3D&reserved=0)
>>>>>>> and "[PATCH v3 06/20] x86/alternative: Use temporary mm for text
>>>>>>> poking" (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-7-rick.p.edgecombe%40intel.com%2F&data=02%7C01%7Cnamit%40vmware.com%7Cbab53e52cc5c4ac4419008d69921d1f1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864767879731941&sdata=7%2BLShgLKnra6xzSkxdJrCclCacfdE5IdczwScW83nuE%3D&reserved=0),
>>>>>>> right? If someone manages to get a tracing BPF program to trigger in a
>>>>>>> task that has switched to the patching mm, could they use
>>>>>>> bpf_probe_write_user() - which uses probe_kernel_write() after
>>>>>>> checking that KERNEL_DS isn't active and that access_ok() passes - to
>>>>>>> overwrite kernel text that is mapped writable in the patching mm?
>>>>>>
>>>>>> Yes, this is a good point. I guess text_poke() should be defined with
>>>>>> “__kprobes” and open-code memcpy.
>>>>>>
>>>>>> Does it sound reasonable?
>>>>>
>>>>> Doesn't __text_poke() as implemented in the proposed patch use a
>>>>> couple other kernel functions, too? Like switch_mm_irqs_off() and
>>>>> pte_clear() (which can be a call into a separate function on paravirt
>>>>> kernels)?
>>>>
>>>> I will move the pte_clear() to be done after the poking mm was unloaded.
>>>> Give me a few minutes to send a sketch of what I think should be done.
>>>
>>> Err.. You are right, I don’t see an easy way of preventing a kprobe from
>>> being set on switch_mm_irqs_off(), and open-coding this monster is too ugly.
>>>
>>> The reasonable solution seems to me as taking all the relevant pieces of
>>> code (and data) that might be used during text-poking and encapsulating them, so they
>>> will be set in a memory area which cannot be kprobe'd. This can also be
>>> useful to write-protect data structures of code that calls text_poke(),
>>> e.g., static-keys. It can also protect data on that stack that is used
>>> during text_poke() from being overwritten from another core.
>>>
>>> This solution is somewhat similar to Igor Stoppa’s idea of using “enclaves”
>>> when doing write-rarely operations.
>>>
>>> Right now, I think that text_poke() will keep being susceptible to such
>>> an attack, unless you have a better suggestion.
>>
>> A relatively simple approach might be to teach BPF not to run kprobe
>> programs and such in contexts where current->mm isn't the active mm?
>> Maybe using nmi_uaccess_okay(), or something like that? It looks like
>> perf_callchain_user() also already uses that. Except that a lot of
>> this code is x86-specific...
>
> This sounds like exactly the right solution. If you're running from
> some unknown context (like NMI or tracing), then you should check
> nmi_uaccess_okay(). I think we should just promote that to be a
> non-arch-specific function (that returns true by default) and check it
> the relevant bpf_probe_xyz() functions.
I can do that, but notice that switch_mm_irqs_off() writes to
cpu_tlbstate.loaded_mm before it actually writes to CR3. So there are still
a couple of instructions (and the load_new_mm_cr3()) in between that a
kprobe can be set on, no?
I can mark them as non-kprobable.
^ permalink raw reply
* Re: [PATCH V2 3/8] dt-bindings: net: stmmac: add phys config properties
From: Rob Herring @ 2019-02-23 0:16 UTC (permalink / raw)
To: Christophe Roullier
Cc: davem, joabreu, mark.rutland, mcoquelin.stm32, alexandre.torgue,
peppe.cavallaro, linux-stm32, linux-kernel, devicetree,
linux-arm-kernel, netdev, andrew
In-Reply-To: <1550824089-19961-4-git-send-email-christophe.roullier@st.com>
On Fri, Feb 22, 2019 at 09:28:04AM +0100, Christophe Roullier wrote:
> Add properties to support all Phy config
> PHY_MODE (MII,GMII, RMII, RGMII) and in normal, PHY wo crystal (25Mhz),
> PHY wo crystal (50Mhz), No 125Mhz from PHY config.
>
> Signed-off-by: Christophe Roullier <christophe.roullier@st.com>
> ---
> Documentation/devicetree/bindings/net/stm32-dwmac.txt | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> index 1341012..f42dc68 100644
> --- a/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> @@ -24,9 +24,9 @@ Required properties:
> encompases the glue register, and the offset of the control register.
>
> Optional properties:
> -- clock-names: For MPU family "mac-clk-ck" for PHY without quartz
> -- st,int-phyclk (boolean) : valid only where PHY do not have quartz and need to be clock
> - by RCC
You can't just remove properties.
> +- clock-names: For MPU family "eth-ck" for PHY without quartz
> +- st,eth_clk_sel (boolean) : set this property in RGMII PHY when you do not want use 125Mhz
> +- st,eth_ref_clk_sel (boolean) : set this property in RMII mode when you have PHY without crystal 50MHz
s/_/-/
'sel' I assume is short for select, but the naming here and description
don't really tell me what I'm getting.
Rob
^ permalink raw reply
* Re: [PATCH V2 6/8] dt-bindings: net: stmmac: add syscfg clock property
From: Rob Herring @ 2019-02-23 0:19 UTC (permalink / raw)
To: Christophe Roullier
Cc: robh, davem, joabreu, mark.rutland, mcoquelin.stm32,
alexandre.torgue, peppe.cavallaro, linux-stm32, linux-kernel,
devicetree, linux-arm-kernel, netdev, christophe.roullier, andrew
In-Reply-To: <1550824089-19961-7-git-send-email-christophe.roullier@st.com>
On Fri, 22 Feb 2019 09:28:07 +0100, Christophe Roullier wrote:
> Need syscfg clock for MCU family in case bootloader does not
> activate it.
>
> Signed-off-by: Christophe Roullier <christophe.roullier@st.com>
> ---
> Documentation/devicetree/bindings/net/stm32-dwmac.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH net-next v3] ip_tunnel: Add dst_cache support in lwtunnel_state of ip tunnel
From: wenxu @ 2019-02-23 0:32 UTC (permalink / raw)
To: davem, dsahern; +Cc: netdev
From: wenxu <wenxu@ucloud.cn>
The lwtunnel_state is not init the dst_cache Which make the
ip_md_tunnel_xmit can't use the dst_cache. It will lookup
route table every packets.
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
net/core/lwtunnel.c | 3 ++-
net/ipv4/ip_tunnel_core.c | 20 ++++++++++++++++++++
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c
index 0b17175..f6301af 100644
--- a/net/core/lwtunnel.c
+++ b/net/core/lwtunnel.c
@@ -123,17 +123,18 @@ int lwtunnel_build_state(u16 encap_type,
rcu_read_lock();
ops = rcu_dereference(lwtun_encaps[encap_type]);
if (likely(ops && ops->build_state && try_module_get(ops->owner))) {
+ rcu_read_unlock();
found = true;
ret = ops->build_state(encap, family, cfg, lws, extack);
if (ret)
module_put(ops->owner);
}
- rcu_read_unlock();
/* don't rely on -EOPNOTSUPP to detect match as build_state
* handlers could return it
*/
if (!found) {
+ rcu_read_unlock();
NL_SET_ERR_MSG_ATTR(extack, encap,
"LWT encapsulation type not supported");
}
diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
index 9a0e67b..334e77a 100644
--- a/net/ipv4/ip_tunnel_core.c
+++ b/net/ipv4/ip_tunnel_core.c
@@ -252,6 +252,14 @@ static int ip_tun_build_state(struct nlattr *attr,
tun_info = lwt_tun_info(new_state);
+#ifdef CONFIG_DST_CACHE
+ err = dst_cache_init(&tun_info->dst_cache, GFP_KERNEL);
+ if (err) {
+ lwtstate_free(new_state);
+ return err;
+ }
+#endif
+
if (tb[LWTUNNEL_IP_ID])
tun_info->key.tun_id = nla_get_be64(tb[LWTUNNEL_IP_ID]);
@@ -278,6 +286,15 @@ static int ip_tun_build_state(struct nlattr *attr,
return 0;
}
+#ifdef CONFIG_DST_CACHE
+static void ip_tun_destroy_state(struct lwtunnel_state *lwtstate)
+{
+ struct ip_tunnel_info *tun_info = lwt_tun_info(lwtstate);
+
+ dst_cache_destroy(&tun_info->dst_cache);
+}
+#endif
+
static int ip_tun_fill_encap_info(struct sk_buff *skb,
struct lwtunnel_state *lwtstate)
{
@@ -313,6 +330,9 @@ static int ip_tun_cmp_encap(struct lwtunnel_state *a, struct lwtunnel_state *b)
static const struct lwtunnel_encap_ops ip_tun_lwt_ops = {
.build_state = ip_tun_build_state,
+#ifdef CONFIG_DST_CACHE
+ .destroy_state = ip_tun_destroy_state,
+#endif
.fill_encap = ip_tun_fill_encap_info,
.get_encap_size = ip_tun_encap_nlsize,
.cmp_encap = ip_tun_cmp_encap,
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH bpf-next 1/4] bpf: enable program stats
From: Roman Gushchin @ 2019-02-23 0:34 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem@davemloft.net, daniel@iogearbox.net, netdev@vger.kernel.org,
bpf@vger.kernel.org, Kernel Team
In-Reply-To: <20190222233644.1487087-2-ast@kernel.org>
On Fri, Feb 22, 2019 at 03:36:41PM -0800, Alexei Starovoitov wrote:
> JITed BPF programs are indistinguishable from kernel functions, but unlike
> kernel code BPF code can be changed often.
> Typical approach of "perf record" + "perf report" profiling and tunning of
> kernel code works just as well for BPF programs, but kernel code doesn't
> need to be monitored whereas BPF programs do.
> Users load and run large amount of BPF programs.
> These BPF stats allow tools monitor the usage of BPF on the server.
> The monitoring tools will turn sysctl kernel.bpf_stats_enabled
> on and off for few seconds to sample average cost of the programs.
> Aggregated data over hours and days will provide an insight into cost of BPF
> and alarms can trigger in case given program suddenly gets more expensive.
>
> The cost of two sched_clock() per program invocation adds ~20 nsec.
> Fast BPF progs (like selftests/bpf/progs/test_pkt_access.c) will slow down
> from ~40 nsec to ~60 nsec.
> static_key minimizes the cost of the stats collection.
> There is no measurable difference before/after this patch
> with kernel.bpf_stats_enabled=0
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
> include/linux/bpf.h | 7 +++++++
> include/linux/filter.h | 16 +++++++++++++++-
> kernel/bpf/core.c | 13 +++++++++++++
> kernel/bpf/syscall.c | 24 ++++++++++++++++++++++--
> kernel/bpf/verifier.c | 5 +++++
> kernel/sysctl.c | 34 ++++++++++++++++++++++++++++++++++
> 6 files changed, 96 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index de18227b3d95..14260674bc57 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -340,6 +340,11 @@ enum bpf_cgroup_storage_type {
>
> #define MAX_BPF_CGROUP_STORAGE_TYPE __BPF_CGROUP_STORAGE_MAX
>
> +struct bpf_prog_stats {
> + u64 cnt;
> + u64 nsecs;
> +};
> +
> struct bpf_prog_aux {
> atomic_t refcnt;
> u32 used_map_cnt;
> @@ -389,6 +394,7 @@ struct bpf_prog_aux {
> * main prog always has linfo_idx == 0
> */
> u32 linfo_idx;
> + struct bpf_prog_stats __percpu *stats;
> union {
> struct work_struct work;
> struct rcu_head rcu;
> @@ -559,6 +565,7 @@ void bpf_map_area_free(void *base);
> void bpf_map_init_from_attr(struct bpf_map *map, union bpf_attr *attr);
>
> extern int sysctl_unprivileged_bpf_disabled;
> +extern int sysctl_bpf_stats_enabled;
>
> int bpf_map_new_fd(struct bpf_map *map, int flags);
> int bpf_prog_new_fd(struct bpf_prog *prog);
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index f32b3eca5a04..7b14235b6f7d 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -533,7 +533,21 @@ struct sk_filter {
> struct bpf_prog *prog;
> };
>
> -#define BPF_PROG_RUN(filter, ctx) ({ cant_sleep(); (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
> +DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
> +
> +#define BPF_PROG_RUN(prog, ctx) ({ \
> + u32 ret; \
> + cant_sleep(); \
> + if (static_branch_unlikely(&bpf_stats_enabled_key)) { \
> + u64 start = sched_clock(); \
> + ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
> + this_cpu_inc(prog->aux->stats->cnt); \
> + this_cpu_add(prog->aux->stats->nsecs, \
> + sched_clock() - start); \
> + } else { \
> + ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
> + } \
> + ret; })
Can we bump "cnt" unconditionally and gate only the "nsecs" calculation?
Why it might be useful?
If the performance cost is too high to keep the statistics gathering
always enabled, it will be possible to measure the average cost
periodically and extrapolate.
Thanks!
^ permalink raw reply
* pull-request: bpf 2019-02-23
From: Daniel Borkmann @ 2019-02-23 0:36 UTC (permalink / raw)
To: davem; +Cc: daniel, ast, netdev, bpf
Hi David,
The following pull-request contains BPF updates for your *net* tree.
The main changes are:
1) Fix a bug in BPF's LPM deletion logic to match correct prefix
length, from Alban.
2) Fix AF_XDP teardown by not destroying umem prematurely as it
is still needed till all outstanding skbs are freed, from Björn.
3) Fix unkillable BPF_PROG_TEST_RUN under preempt kernel by checking
signal_pending() outside need_resched() condition which is never
triggered there, from Stanislav.
4) Fix two nfp JIT bugs, one in code emission for K-based xor, and
another one to explicitly clear upper bits in alu32, from Jiong.
5) Add bpf list address to maintainers file, from Daniel.
Please consider pulling these changes from:
git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git
Thanks a lot!
----------------------------------------------------------------
The following changes since commit 21d2cb491b9e10bfdf10424673b43cd9eddc2da1:
net/mlx4_en: fix spelling mistake: "quiting" -> "quitting" (2019-02-18 12:06:42 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git
for you to fetch changes up to b4b8bb69c104a9345c528692cde5aa520d885360:
bpf, doc: add bpf list as secondary entry to maintainers file (2019-02-23 01:22:32 +0100)
----------------------------------------------------------------
Alban Crequy (1):
bpf, lpm: fix lookup bug in map_delete_elem
Björn Töpel (1):
Revert "xsk: simplify AF_XDP socket teardown"
Daniel Borkmann (2):
Merge branch 'bpf-nfp-codegen-fixes'
bpf, doc: add bpf list as secondary entry to maintainers file
Jiong Wang (2):
nfp: bpf: fix code-gen bug on BPF_ALU | BPF_XOR | BPF_K
nfp: bpf: fix ALU32 high bits clearance bug
Stanislav Fomichev (1):
bpf/test_run: fix unkillable BPF_PROG_TEST_RUN
MAINTAINERS | 14 ++++++++-
drivers/net/ethernet/netronome/nfp/bpf/jit.c | 17 ++++-------
kernel/bpf/lpm_trie.c | 1 +
net/bpf/test_run.c | 45 +++++++++++++++-------------
net/xdp/xsk.c | 16 +++++++++-
tools/testing/selftests/bpf/test_lpm_map.c | 10 +++++++
6 files changed, 69 insertions(+), 34 deletions(-)
^ permalink raw reply
* [PATCH v4 net-next 0/4] net: dsa: microchip: add MIB counters support
From: Tristram.Ha @ 2019-02-23 0:36 UTC (permalink / raw)
To: Sergio Paracuellos, Andrew Lunn, Florian Fainelli, Pavel Machek
Cc: Tristram Ha, UNGLinuxDriver, netdev
From: Tristram Ha <Tristram.Ha@microchip.com>
This series of patches is to modify the KSZ9477 DSA driver to read MIB
counters periodically to avoid overflow.
The MIB counters should be read only when there is link. Otherwise it is
a waste of time as hardware never increases the counters.
Functions are added to check the port link status so that MIB counters
read call is used efficiently.
v4
- Use readx_poll_timeout
- Fix using mutex in a timer callback function problem
- use dp->slave directly instead of checking whether it is valid
- Add port_cleanup function in a separate patch
- Add a mutex so that changing device variables is safe
v3
- Use netif_carrier_ok instead of checking the phy device pointer
v2
- Create macro similar to readx_poll_timeout to use with switch
- Create ksz_port_cleanup function so that variables like on_ports and
live_ports can be updated inside it
v1
- Use readx_poll_timeout
- Do not clear MIB counters when port is enabled
- Do not advertise 1000 half-duplex mode when port is enabled
- Do not use freeze function as MIB counters may miss counts
Tristram Ha (5):
net: dsa: microchip: prepare PHY for proper advertisement
net: dsa: microchip: add MIB counter reading support
net: dsa: microchip: get port link status
net: dsa: microchip: remove unnecessary include headers
net: dsa: microchip: add port_cleanup function
drivers/net/dsa/microchip/ksz9477.c | 147 ++++++++++++++++++++++-----------
drivers/net/dsa/microchip/ksz_common.c | 142 ++++++++++++++++++++++++++++++-
drivers/net/dsa/microchip/ksz_common.h | 21 ++++-
drivers/net/dsa/microchip/ksz_priv.h | 15 ++--
4 files changed, 270 insertions(+), 55 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH v4 net-next 2/5] net: dsa: microchip: add MIB counter reading support
From: Tristram.Ha @ 2019-02-23 0:36 UTC (permalink / raw)
To: Sergio Paracuellos, Andrew Lunn, Florian Fainelli, Pavel Machek
Cc: Tristram Ha, UNGLinuxDriver, netdev
In-Reply-To: <1550882211-10381-1-git-send-email-Tristram.Ha@microchip.com>
From: Tristram Ha <Tristram.Ha@microchip.com>
Add background MIB counter reading support.
Port MIB counters should only be read when there is link. Otherwise it is
a waste of time as hardware never increases those counters. There are
exceptions as some switches keep track of dropped counts no matter what.
Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/dsa/microchip/ksz9477.c | 123 ++++++++++++++++++++++-----------
drivers/net/dsa/microchip/ksz_common.c | 103 +++++++++++++++++++++++++++
drivers/net/dsa/microchip/ksz_common.h | 18 ++++-
drivers/net/dsa/microchip/ksz_priv.h | 9 ++-
4 files changed, 205 insertions(+), 48 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 4573b6e..94cd385 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -10,6 +10,7 @@
#include <linux/gpio.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/iopoll.h>
#include <linux/platform_data/microchip-ksz.h>
#include <linux/phy.h>
#include <linux/etherdevice.h>
@@ -18,8 +19,8 @@
#include <net/switchdev.h>
#include "ksz_priv.h"
-#include "ksz_common.h"
#include "ksz9477_reg.h"
+#include "ksz_common.h"
static const struct {
int index;
@@ -259,6 +260,75 @@ static int ksz9477_reset_switch(struct ksz_device *dev)
return 0;
}
+static void ksz9477_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
+ u64 *cnt)
+{
+ struct ksz_poll_ctx ctx = {
+ .dev = dev,
+ .port = port,
+ .offset = REG_PORT_MIB_CTRL_STAT__4,
+ };
+ struct ksz_port *p = &dev->ports[port];
+ u32 data;
+ int ret;
+
+ /* retain the flush/freeze bit */
+ data = p->freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
+ data |= MIB_COUNTER_READ;
+ data |= (addr << MIB_COUNTER_INDEX_S);
+ ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, data);
+
+ ret = readx_poll_timeout(ksz_pread32_poll, &ctx, data,
+ !(data & MIB_COUNTER_READ), 10, 1000);
+
+ /* failed to read MIB. get out of loop */
+ if (ret < 0) {
+ dev_dbg(dev->dev, "Failed to get MIB\n");
+ return;
+ }
+
+ /* count resets upon read */
+ ksz_pread32(dev, port, REG_PORT_MIB_DATA, &data);
+ *cnt += data;
+}
+
+static void ksz9477_r_mib_pkt(struct ksz_device *dev, int port, u16 addr,
+ u64 *dropped, u64 *cnt)
+{
+ addr = ksz9477_mib_names[addr].index;
+ ksz9477_r_mib_cnt(dev, port, addr, cnt);
+}
+
+static void ksz9477_freeze_mib(struct ksz_device *dev, int port, bool freeze)
+{
+ u32 val = freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
+ struct ksz_port *p = &dev->ports[port];
+
+ /* enable/disable the port for flush/freeze function */
+ mutex_lock(&p->mib.cnt_mutex);
+ ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, val);
+
+ /* used by MIB counter reading code to know freeze is enabled */
+ p->freeze = freeze;
+ mutex_unlock(&p->mib.cnt_mutex);
+}
+
+static void ksz9477_port_init_cnt(struct ksz_device *dev, int port)
+{
+ struct ksz_port_mib *mib = &dev->ports[port].mib;
+
+ /* flush all enabled port MIB counters */
+ mutex_lock(&mib->cnt_mutex);
+ ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4,
+ MIB_COUNTER_FLUSH_FREEZE);
+ ksz_write8(dev, REG_SW_MAC_CTRL_6, SW_MIB_COUNTER_FLUSH);
+ ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, 0);
+ mutex_unlock(&mib->cnt_mutex);
+
+ mib->cnt_ptr = 0;
+ memset(mib->counters, 0, dev->mib_cnt * sizeof(u64));
+}
+
static enum dsa_tag_protocol ksz9477_get_tag_protocol(struct dsa_switch *ds,
int port)
{
@@ -342,47 +412,6 @@ static void ksz9477_get_strings(struct dsa_switch *ds, int port,
}
}
-static void ksz_get_ethtool_stats(struct dsa_switch *ds, int port,
- uint64_t *buf)
-{
- struct ksz_device *dev = ds->priv;
- int i;
- u32 data;
- int timeout;
-
- mutex_lock(&dev->stats_mutex);
-
- for (i = 0; i < TOTAL_SWITCH_COUNTER_NUM; i++) {
- data = MIB_COUNTER_READ;
- data |= ((ksz9477_mib_names[i].index & 0xFF) <<
- MIB_COUNTER_INDEX_S);
- ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, data);
-
- timeout = 1000;
- do {
- ksz_pread32(dev, port, REG_PORT_MIB_CTRL_STAT__4,
- &data);
- usleep_range(1, 10);
- if (!(data & MIB_COUNTER_READ))
- break;
- } while (timeout-- > 0);
-
- /* failed to read MIB. get out of loop */
- if (!timeout) {
- dev_dbg(dev->dev, "Failed to get MIB\n");
- break;
- }
-
- /* count resets upon read */
- ksz_pread32(dev, port, REG_PORT_MIB_DATA, &data);
-
- dev->mib_value[i] += (uint64_t)data;
- buf[i] = dev->mib_value[i];
- }
-
- mutex_unlock(&dev->stats_mutex);
-}
-
static void ksz9477_cfg_port_member(struct ksz_device *dev, int port,
u8 member)
{
@@ -1151,9 +1180,14 @@ static int ksz9477_setup(struct dsa_switch *ds)
/* queue based egress rate limit */
ksz_cfg(dev, REG_SW_MAC_CTRL_5, SW_OUT_RATE_LIMIT_QUEUE_BASED, true);
+ /* enable global MIB counter freeze function */
+ ksz_cfg(dev, REG_SW_MAC_CTRL_6, SW_MIB_COUNTER_FREEZE, true);
+
/* start switch */
ksz_cfg(dev, REG_SW_OPERATION, SW_START, true);
+ ksz_init_mib_timer(dev);
+
return 0;
}
@@ -1287,6 +1321,7 @@ static int ksz9477_switch_init(struct ksz_device *dev)
if (!dev->ports)
return -ENOMEM;
for (i = 0; i < dev->mib_port_cnt; i++) {
+ mutex_init(&dev->ports[i].mib.cnt_mutex);
dev->ports[i].mib.counters =
devm_kzalloc(dev->dev,
sizeof(u64) *
@@ -1311,6 +1346,10 @@ static void ksz9477_switch_exit(struct ksz_device *dev)
.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
.phy_setup = ksz9477_phy_setup,
.port_setup = ksz9477_port_setup,
+ .r_mib_cnt = ksz9477_r_mib_cnt,
+ .r_mib_pkt = ksz9477_r_mib_pkt,
+ .freeze_mib = ksz9477_freeze_mib,
+ .port_init_cnt = ksz9477_port_init_cnt,
.shutdown = ksz9477_reset_switch,
.detect = ksz9477_switch_detect,
.init = ksz9477_switch_init,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 6f72842..eecfbf3 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -40,6 +40,85 @@ void ksz_update_port_member(struct ksz_device *dev, int port)
}
EXPORT_SYMBOL_GPL(ksz_update_port_member);
+static void port_r_cnt(struct ksz_device *dev, int port)
+{
+ struct ksz_port_mib *mib = &dev->ports[port].mib;
+ u64 *dropped;
+
+ /* Some ports may not have MIB counters before SWITCH_COUNTER_NUM. */
+ while (mib->cnt_ptr < dev->reg_mib_cnt) {
+ dev->dev_ops->r_mib_cnt(dev, port, mib->cnt_ptr,
+ &mib->counters[mib->cnt_ptr]);
+ ++mib->cnt_ptr;
+ }
+
+ /* last one in storage */
+ dropped = &mib->counters[dev->mib_cnt];
+
+ /* Some ports may not have MIB counters after SWITCH_COUNTER_NUM. */
+ while (mib->cnt_ptr < dev->mib_cnt) {
+ dev->dev_ops->r_mib_pkt(dev, port, mib->cnt_ptr,
+ dropped, &mib->counters[mib->cnt_ptr]);
+ ++mib->cnt_ptr;
+ }
+ mib->cnt_ptr = 0;
+}
+
+static void ksz_mib_read_work(struct work_struct *work)
+{
+ struct ksz_device *dev = container_of(work, struct ksz_device,
+ mib_read);
+ struct ksz_port_mib *mib;
+ struct ksz_port *p;
+ int i;
+
+ for (i = 0; i < dev->mib_port_cnt; i++) {
+ p = &dev->ports[i];
+ mib = &p->mib;
+ mutex_lock(&mib->cnt_mutex);
+
+ /* Only read MIB counters when the port is told to do.
+ * If not, read only dropped counters when link is not up.
+ */
+ if (!p->read) {
+ const struct dsa_port *dp = dsa_to_port(dev->ds, i);
+
+ if (!netif_carrier_ok(dp->slave))
+ mib->cnt_ptr = dev->reg_mib_cnt;
+ }
+ port_r_cnt(dev, i);
+ p->read = false;
+ mutex_unlock(&mib->cnt_mutex);
+ }
+}
+
+static void mib_monitor(struct timer_list *t)
+{
+ struct ksz_device *dev = from_timer(dev, t, mib_read_timer);
+
+ mod_timer(&dev->mib_read_timer, jiffies + dev->mib_read_interval);
+ schedule_work(&dev->mib_read);
+}
+
+void ksz_init_mib_timer(struct ksz_device *dev)
+{
+ int i;
+
+ /* Read MIB counters every 30 seconds to avoid overflow. */
+ dev->mib_read_interval = msecs_to_jiffies(30000);
+
+ INIT_WORK(&dev->mib_read, ksz_mib_read_work);
+ timer_setup(&dev->mib_read_timer, mib_monitor, 0);
+
+ for (i = 0; i < dev->mib_port_cnt; i++)
+ dev->dev_ops->port_init_cnt(dev, i);
+
+ /* Start the timer 2 seconds later. */
+ dev->mib_read_timer.expires = jiffies + msecs_to_jiffies(2000);
+ add_timer(&dev->mib_read_timer);
+}
+EXPORT_SYMBOL_GPL(ksz_init_mib_timer);
+
int ksz_phy_read16(struct dsa_switch *ds, int addr, int reg)
{
struct ksz_device *dev = ds->priv;
@@ -72,6 +151,24 @@ int ksz_sset_count(struct dsa_switch *ds, int port, int sset)
}
EXPORT_SYMBOL_GPL(ksz_sset_count);
+void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf)
+{
+ const struct dsa_port *dp = dsa_to_port(ds, port);
+ struct ksz_device *dev = ds->priv;
+ struct ksz_port_mib *mib;
+
+ mib = &dev->ports[port].mib;
+ mutex_lock(&mib->cnt_mutex);
+
+ /* Only read dropped counters if no link. */
+ if (!netif_carrier_ok(dp->slave))
+ mib->cnt_ptr = dev->reg_mib_cnt;
+ port_r_cnt(dev, port);
+ memcpy(buf, mib->counters, dev->mib_cnt * sizeof(u64));
+ mutex_unlock(&mib->cnt_mutex);
+}
+EXPORT_SYMBOL_GPL(ksz_get_ethtool_stats);
+
int ksz_port_bridge_join(struct dsa_switch *ds, int port,
struct net_device *br)
{
@@ -339,6 +436,12 @@ int ksz_switch_register(struct ksz_device *dev,
void ksz_switch_remove(struct ksz_device *dev)
{
+ /* timer started */
+ if (dev->mib_read_timer.expires) {
+ del_timer_sync(&dev->mib_read_timer);
+ flush_work(&dev->mib_read);
+ }
+
dev->dev_ops->exit(dev);
dsa_unregister_switch(dev->ds);
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 2dd832d..f45a553 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -1,19 +1,21 @@
/* SPDX-License-Identifier: GPL-2.0
* Microchip switch driver common header
*
- * Copyright (C) 2017-2018 Microchip Technology Inc.
+ * Copyright (C) 2017-2019 Microchip Technology Inc.
*/
#ifndef __KSZ_COMMON_H
#define __KSZ_COMMON_H
void ksz_update_port_member(struct ksz_device *dev, int port);
+void ksz_init_mib_timer(struct ksz_device *dev);
/* Common DSA access functions */
int ksz_phy_read16(struct dsa_switch *ds, int addr, int reg);
int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val);
int ksz_sset_count(struct dsa_switch *ds, int port, int sset);
+void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf);
int ksz_port_bridge_join(struct dsa_switch *ds, int port,
struct net_device *br);
void ksz_port_bridge_leave(struct dsa_switch *ds, int port,
@@ -211,4 +213,18 @@ static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
ksz_write8(dev, addr, data);
}
+struct ksz_poll_ctx {
+ struct ksz_device *dev;
+ int port;
+ int offset;
+};
+
+static inline u32 ksz_pread32_poll(struct ksz_poll_ctx *ctx)
+{
+ u32 data;
+
+ ksz_pread32(ctx->dev, ctx->port, ctx->offset, &data);
+ return data;
+}
+
#endif
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index 0fdc58b..1d2d98f 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -14,8 +14,6 @@
#include <linux/etherdevice.h>
#include <net/dsa.h>
-#include "ksz9477_reg.h"
-
struct ksz_io_ops;
struct vlan_table {
@@ -23,6 +21,7 @@ struct vlan_table {
};
struct ksz_port_mib {
+ struct mutex cnt_mutex; /* structure access */
u8 cnt_ptr;
u64 *counters;
};
@@ -38,7 +37,8 @@ struct ksz_port {
u32 fiber:1; /* port is fiber */
u32 sgmii:1; /* port is SGMII */
u32 force:1;
- u32 link_just_down:1; /* link just goes down */
+ u32 read:1; /* read MIB counters in background */
+ u32 freeze:1; /* MIB counter freeze is enabled */
struct ksz_port_mib mib;
};
@@ -79,8 +79,6 @@ struct ksz_device {
struct vlan_table *vlan_cache;
- u64 mib_value[TOTAL_SWITCH_COUNTER_NUM];
-
u8 *txbuf;
struct ksz_port *ports;
@@ -153,6 +151,7 @@ struct ksz_dev_ops {
u64 *cnt);
void (*r_mib_pkt)(struct ksz_device *dev, int port, u16 addr,
u64 *dropped, u64 *cnt);
+ void (*freeze_mib)(struct ksz_device *dev, int port, bool freeze);
void (*port_init_cnt)(struct ksz_device *dev, int port);
int (*shutdown)(struct ksz_device *dev);
int (*detect)(struct ksz_device *dev);
--
1.9.1
^ permalink raw reply related
* [PATCH v4 net-next 3/5] net: dsa: microchip: get port link status
From: Tristram.Ha @ 2019-02-23 0:36 UTC (permalink / raw)
To: Sergio Paracuellos, Andrew Lunn, Florian Fainelli, Pavel Machek
Cc: Tristram Ha, UNGLinuxDriver, netdev
In-Reply-To: <1550882211-10381-1-git-send-email-Tristram.Ha@microchip.com>
From: Tristram Ha <Tristram.Ha@microchip.com>
Get port link status to know whether to read MIB counters when the link
is going down.
Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/dsa/microchip/ksz9477.c | 1 +
drivers/net/dsa/microchip/ksz_common.c | 14 ++++++++++++++
drivers/net/dsa/microchip/ksz_common.h | 2 ++
3 files changed, 17 insertions(+)
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 94cd385..1b8267c 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1196,6 +1196,7 @@ static int ksz9477_setup(struct dsa_switch *ds)
.setup = ksz9477_setup,
.phy_read = ksz9477_phy_read16,
.phy_write = ksz9477_phy_write16,
+ .adjust_link = ksz_adjust_link,
.port_enable = ksz_enable_port,
.port_disable = ksz_disable_port,
.get_strings = ksz9477_get_strings,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index eecfbf3..0589fc7 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -140,6 +140,20 @@ int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val)
}
EXPORT_SYMBOL_GPL(ksz_phy_write16);
+void ksz_adjust_link(struct dsa_switch *ds, int port,
+ struct phy_device *phydev)
+{
+ struct ksz_device *dev = ds->priv;
+ struct ksz_port *p = &dev->ports[port];
+
+ /* Read all MIB counters when the link is going down. */
+ if (!phydev->link) {
+ p->read = true;
+ schedule_work(&dev->mib_read);
+ }
+}
+EXPORT_SYMBOL_GPL(ksz_adjust_link);
+
int ksz_sset_count(struct dsa_switch *ds, int port, int sset)
{
struct ksz_device *dev = ds->priv;
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index f45a553..eb29bb0 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -14,6 +14,8 @@
int ksz_phy_read16(struct dsa_switch *ds, int addr, int reg);
int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val);
+void ksz_adjust_link(struct dsa_switch *ds, int port,
+ struct phy_device *phydev);
int ksz_sset_count(struct dsa_switch *ds, int port, int sset);
void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf);
int ksz_port_bridge_join(struct dsa_switch *ds, int port,
--
1.9.1
^ permalink raw reply related
* [PATCH v4 net-next 4/5] net: dsa: microchip: remove unnecessary include headers
From: Tristram.Ha @ 2019-02-23 0:36 UTC (permalink / raw)
To: Sergio Paracuellos, Andrew Lunn, Florian Fainelli, Pavel Machek
Cc: Tristram Ha, UNGLinuxDriver, netdev
In-Reply-To: <1550882211-10381-1-git-send-email-Tristram.Ha@microchip.com>
From: Tristram Ha <Tristram.Ha@microchip.com>
Remove unnecessary header include.
Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/dsa/microchip/ksz9477.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 1b8267c..b1b86b6 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -5,15 +5,11 @@
* Copyright (C) 2017-2019 Microchip Technology Inc.
*/
-#include <linux/delay.h>
-#include <linux/export.h>
-#include <linux/gpio.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/iopoll.h>
#include <linux/platform_data/microchip-ksz.h>
#include <linux/phy.h>
-#include <linux/etherdevice.h>
#include <linux/if_bridge.h>
#include <net/dsa.h>
#include <net/switchdev.h>
--
1.9.1
^ permalink raw reply related
* [PATCH v4 net-next 5/5] net: dsa: microchip: add port_cleanup function
From: Tristram.Ha @ 2019-02-23 0:36 UTC (permalink / raw)
To: Sergio Paracuellos, Andrew Lunn, Florian Fainelli, Pavel Machek
Cc: Tristram Ha, UNGLinuxDriver, netdev
In-Reply-To: <1550882211-10381-1-git-send-email-Tristram.Ha@microchip.com>
From: Tristram Ha <Tristram.Ha@microchip.com>
Add port_cleanup function to reset some device variables when the port is
disabled. Add a mutex to make sure changing those variables is
thread-safe.
Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
drivers/net/dsa/microchip/ksz9477.c | 6 ++++++
drivers/net/dsa/microchip/ksz_common.c | 22 ++++++++++++++++++++++
drivers/net/dsa/microchip/ksz_common.h | 1 +
drivers/net/dsa/microchip/ksz_priv.h | 2 ++
4 files changed, 31 insertions(+)
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index b1b86b6..03de50e 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -450,12 +450,14 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
break;
member = dev->host_mask | p->vid_member;
+ mutex_lock(&dev->dev_mutex);
/* Port is a member of a bridge. */
if (dev->br_member & (1 << port)) {
dev->member |= (1 << port);
member = dev->member;
}
+ mutex_unlock(&dev->dev_mutex);
break;
case BR_STATE_BLOCKING:
data |= PORT_LEARN_DISABLE;
@@ -470,6 +472,7 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
ksz_pwrite8(dev, port, P_STP_CTRL, data);
p->stp_state = state;
+ mutex_lock(&dev->dev_mutex);
if (data & PORT_RX_ENABLE)
dev->rx_ports |= (1 << port);
else
@@ -494,6 +497,7 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
*/
if (forward != dev->member)
ksz_update_port_member(dev, port);
+ mutex_unlock(&dev->dev_mutex);
}
static void ksz9477_flush_dyn_mac_table(struct ksz_device *dev, int port)
@@ -1080,6 +1084,7 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
ksz_pwrite8(dev, port, REG_PORT_XMII_CTRL_1, data8);
p->phydev.duplex = 1;
}
+ mutex_lock(&dev->dev_mutex);
if (cpu_port) {
member = dev->port_mask;
dev->on_ports = dev->host_mask;
@@ -1092,6 +1097,7 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
if (p->phydev.link)
dev->live_ports |= (1 << port);
}
+ mutex_unlock(&dev->dev_mutex);
ksz9477_cfg_port_member(dev, port, member);
/* clear pending interrupts */
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 0589fc7..d89c977 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -20,6 +20,16 @@
#include "ksz_priv.h"
+void ksz_port_cleanup(struct ksz_device *dev, int port)
+{
+ /* Common code for port cleanup. */
+ mutex_lock(&dev->dev_mutex);
+ dev->on_ports &= ~(1 << port);
+ dev->live_ports &= ~(1 << port);
+ mutex_unlock(&dev->dev_mutex);
+}
+EXPORT_SYMBOL_GPL(ksz_port_cleanup);
+
void ksz_update_port_member(struct ksz_device *dev, int port)
{
struct ksz_port *p;
@@ -151,6 +161,13 @@ void ksz_adjust_link(struct dsa_switch *ds, int port,
p->read = true;
schedule_work(&dev->mib_read);
}
+ mutex_lock(&dev->dev_mutex);
+ if (!phydev->link)
+ dev->live_ports &= ~(1 << port);
+ else
+ /* Remember which port is connected and active. */
+ dev->live_ports |= (1 << port) & dev->on_ports;
+ mutex_unlock(&dev->dev_mutex);
}
EXPORT_SYMBOL_GPL(ksz_adjust_link);
@@ -188,7 +205,9 @@ int ksz_port_bridge_join(struct dsa_switch *ds, int port,
{
struct ksz_device *dev = ds->priv;
+ mutex_lock(&dev->dev_mutex);
dev->br_member |= (1 << port);
+ mutex_unlock(&dev->dev_mutex);
/* port_stp_state_set() will be called after to put the port in
* appropriate state so there is no need to do anything.
@@ -203,8 +222,10 @@ void ksz_port_bridge_leave(struct dsa_switch *ds, int port,
{
struct ksz_device *dev = ds->priv;
+ mutex_lock(&dev->dev_mutex);
dev->br_member &= ~(1 << port);
dev->member &= ~(1 << port);
+ mutex_unlock(&dev->dev_mutex);
/* port_stp_state_set() will be called after to put the port in
* forwarding state so there is no need to do anything.
@@ -417,6 +438,7 @@ int ksz_switch_register(struct ksz_device *dev,
gpiod_set_value(dev->reset_gpio, 0);
}
+ mutex_init(&dev->dev_mutex);
mutex_init(&dev->reg_mutex);
mutex_init(&dev->stats_mutex);
mutex_init(&dev->alu_mutex);
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index eb29bb0..0d25bc4 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -7,6 +7,7 @@
#ifndef __KSZ_COMMON_H
#define __KSZ_COMMON_H
+void ksz_port_cleanup(struct ksz_device *dev, int port);
void ksz_update_port_member(struct ksz_device *dev, int port);
void ksz_init_mib_timer(struct ksz_device *dev);
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index 1d2d98f..b52e5ca 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -48,6 +48,7 @@ struct ksz_device {
struct ksz_platform_data *pdata;
const char *name;
+ struct mutex dev_mutex; /* device access */
struct mutex reg_mutex; /* register access */
struct mutex stats_mutex; /* status access */
struct mutex alu_mutex; /* ALU access */
@@ -137,6 +138,7 @@ struct ksz_dev_ops {
void (*flush_dyn_mac_table)(struct ksz_device *dev, int port);
void (*phy_setup)(struct ksz_device *dev, int port,
struct phy_device *phy);
+ void (*port_cleanup)(struct ksz_device *dev, int port);
void (*port_setup)(struct ksz_device *dev, int port, bool cpu_port);
void (*r_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
void (*w_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
--
1.9.1
^ permalink raw reply related
* [PATCH v4 net-next 1/5] net: dsa: microchip: prepare PHY for proper advertisement
From: Tristram.Ha @ 2019-02-23 0:36 UTC (permalink / raw)
To: Sergio Paracuellos, Andrew Lunn, Florian Fainelli, Pavel Machek
Cc: Tristram Ha, UNGLinuxDriver, netdev
In-Reply-To: <1550882211-10381-1-git-send-email-Tristram.Ha@microchip.com>
From: Tristram Ha <Tristram.Ha@microchip.com>
Prepare PHY for proper advertisement as sometimes the PHY in the switch
has its own problems even though it may share the PHY id from regular PHY
but the fixes in the PHY driver do not apply.
Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/dsa/microchip/ksz9477.c | 13 ++++++++++++-
drivers/net/dsa/microchip/ksz_common.c | 3 ++-
drivers/net/dsa/microchip/ksz_priv.h | 4 +++-
3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 674d77e..4573b6e 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -2,7 +2,7 @@
/*
* Microchip KSZ9477 switch driver main logic
*
- * Copyright (C) 2017-2018 Microchip Technology Inc.
+ * Copyright (C) 2017-2019 Microchip Technology Inc.
*/
#include <linux/delay.h>
@@ -966,6 +966,16 @@ static void ksz9477_port_mirror_del(struct dsa_switch *ds, int port,
PORT_MIRROR_SNIFFER, false);
}
+static void ksz9477_phy_setup(struct ksz_device *dev, int port,
+ struct phy_device *phy)
+{
+ if (port < dev->phy_port_cnt) {
+ /* The MAC actually cannot run in 1000 half-duplex mode. */
+ phy_remove_link_mode(phy,
+ ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
+ }
+}
+
static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
{
u8 data8;
@@ -1299,6 +1309,7 @@ static void ksz9477_switch_exit(struct ksz_device *dev)
.get_port_addr = ksz9477_get_port_addr,
.cfg_port_member = ksz9477_cfg_port_member,
.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
+ .phy_setup = ksz9477_phy_setup,
.port_setup = ksz9477_port_setup,
.shutdown = ksz9477_reset_switch,
.detect = ksz9477_switch_detect,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 8a5111f..6f72842 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2,7 +2,7 @@
/*
* Microchip switch driver main logic
*
- * Copyright (C) 2017-2018 Microchip Technology Inc.
+ * Copyright (C) 2017-2019 Microchip Technology Inc.
*/
#include <linux/delay.h>
@@ -238,6 +238,7 @@ int ksz_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
/* setup slave port */
dev->dev_ops->port_setup(dev, port, false);
+ dev->dev_ops->phy_setup(dev, port, phy);
/* port_stp_state_set() will be called after to enable the port so
* there is no need to do anything.
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index 60b4901..0fdc58b 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -2,7 +2,7 @@
*
* Microchip KSZ series switch common definitions
*
- * Copyright (C) 2017-2018 Microchip Technology Inc.
+ * Copyright (C) 2017-2019 Microchip Technology Inc.
*/
#ifndef __KSZ_PRIV_H
@@ -137,6 +137,8 @@ struct ksz_dev_ops {
u32 (*get_port_addr)(int port, int offset);
void (*cfg_port_member)(struct ksz_device *dev, int port, u8 member);
void (*flush_dyn_mac_table)(struct ksz_device *dev, int port);
+ void (*phy_setup)(struct ksz_device *dev, int port,
+ struct phy_device *phy);
void (*port_setup)(struct ksz_device *dev, int port, bool cpu_port);
void (*r_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
void (*w_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie
From: Martin Lau @ 2019-02-23 0:44 UTC (permalink / raw)
To: Lorenz Bauer
Cc: ast@kernel.org, daniel@iogearbox.net, netdev@vger.kernel.org,
linux-api@vger.kernel.org
In-Reply-To: <20190222095057.9442-2-lmb@cloudflare.com>
On Fri, Feb 22, 2019 at 09:50:55AM +0000, Lorenz Bauer wrote:
> Using bpf_sk_lookup_tcp it's possible to ascertain whether a packet belongs
> to a known connection. However, there is one corner case: no sockets are
> created if SYN cookies are active. This means that the final ACK in the
> 3WHS is misclassified.
>
> Using the helper, we can look up the listening socket via bpf_sk_lookup_tcp
> and then check whether a packet is a valid SYN cookie ACK.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
> include/uapi/linux/bpf.h | 18 ++++++++++-
> net/core/filter.c | 68 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 85 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index bcdd2474eee7..bc2af87e9621 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2359,6 +2359,21 @@ union bpf_attr {
> * Return
> * A **struct bpf_tcp_sock** pointer on success, or NULL in
> * case of failure.
> + *
> + * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
> + * Description
> + * Check whether iph and th contain a valid SYN cookie ACK for
> + * the listening socket in sk.
> + *
> + * iph points to the start of the IPv4 or IPv6 header, while
> + * iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
> + *
> + * th points to the start of the TCP header, while th_len contains
> + * sizeof(struct tcphdr).
> + *
> + * Return
> + * 0 if iph and th are a valid SYN cookie ACK, or a negative error
> + * otherwise.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2457,7 +2472,8 @@ union bpf_attr {
> FN(spin_lock), \
> FN(spin_unlock), \
> FN(sk_fullsock), \
> - FN(tcp_sock),
> + FN(tcp_sock), \
> + FN(sk_check_syncookie),
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> * function eBPF program intends to call
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 85749f6ec789..9e68897cc7ed 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5426,6 +5426,70 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
> .arg1_type = ARG_PTR_TO_SOCK_COMMON,
> };
>
> +BPF_CALL_5(bpf_sk_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
s/bpf_sk_check_syncookie/bpf_tcp_check_syncookie/
> + struct tcphdr *, th, u32, th_len)
> +{
> +#if IS_ENABLED(CONFIG_SYN_COOKIES)
nit. "#ifdef CONFIG_SYN_COOKIES" such that it is clear it is a bool kconfig.
> + u32 cookie;
> + int ret;
> +
> + if (unlikely(th_len < sizeof(*th)))
> + return -EINVAL;
> +
> + /* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
> + if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
From the test program in patch 3, the "sk" here is obtained from
bpf_sk_lookup_tcp() which does a sk_to_full_sk() before returning.
AFAICT, meaning bpf_sk_lookup_tcp() will return the listening sk
even if there is a request_sock. Does it make sense to check
syncookie if there is already a request_sock?
> + return -EINVAL;
> +
> + if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
Should tcp_synq_no_recent_overflow(tp) be checked also?
> + return -EINVAL;
> +
> + if (!th->ack || th->rst)
How about th->syn?
> + return -ENOENT;
> +
> + cookie = ntohl(th->ack_seq) - 1;
> +
> + switch (sk->sk_family) {
> + case AF_INET:
> + if (unlikely(iph_len < sizeof(struct iphdr)))
> + return -EINVAL;
> +
> + ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
> + break;
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> + case AF_INET6:
> + if (unlikely(iph_len < sizeof(struct ipv6hdr)))
> + return -EINVAL;
> +
> + ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
> + break;
> +#endif /* CONFIG_IPV6 */
> +
> + default:
> + return -EPROTONOSUPPORT;
> + }
> +
> + if (ret > 0)
> + return 0;
> +
> + return -ENOENT;
> +#else
> + return -ENOTSUP;
> +#endif
> +}
> +
> +static const struct bpf_func_proto bpf_sk_check_syncookie_proto = {
> + .func = bpf_sk_check_syncookie,
> + .gpl_only = true,
> + .pkt_access = true,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_SOCKET,
I think it should be ARG_PTR_TO_TCP_SOCK
> + .arg2_type = ARG_PTR_TO_MEM,
> + .arg3_type = ARG_CONST_SIZE,
> + .arg4_type = ARG_PTR_TO_MEM,
> + .arg5_type = ARG_CONST_SIZE,
> +};
> +
> #endif /* CONFIG_INET */
^ permalink raw reply
* Re: [PATCH v4 net-next 2/5] net: dsa: microchip: add MIB counter reading support
From: Florian Fainelli @ 2019-02-23 0:45 UTC (permalink / raw)
To: Tristram.Ha, Sergio Paracuellos, Andrew Lunn, Pavel Machek
Cc: UNGLinuxDriver, netdev
In-Reply-To: <1550882211-10381-3-git-send-email-Tristram.Ha@microchip.com>
Le 2/22/19 à 4:36 PM, Tristram.Ha@microchip.com a écrit :
> From: Tristram Ha <Tristram.Ha@microchip.com>
>
> Add background MIB counter reading support.
>
> Port MIB counters should only be read when there is link. Otherwise it is
> a waste of time as hardware never increases those counters. There are
> exceptions as some switches keep track of dropped counts no matter what.
>
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Thanks for the quick turn around in addressing feedback, this does look
good now!
--
Florian
^ permalink raw reply
* Re: [PATCH net-next v2] route: Add a new fib_multipath_hash_policy base on cpu id for tunnel packet
From: wenxu @ 2019-02-23 0:45 UTC (permalink / raw)
To: Nikolay Aleksandrov, davem; +Cc: netdev
In-Reply-To: <fd97793f-770c-a541-350c-4f36814a573b@cumulusnetworks.com>
On 2019/2/23 上午12:08, Nikolay Aleksandrov wrote:
> On 22/02/2019 14:50, wenxu wrote:
>> On 2019/2/22 下午6:26, Nikolay Aleksandrov wrote:
>>> On 22/02/2019 11:20, wenxu@ucloud.cn wrote:
>>>> From: wenxu <wenxu@ucloud.cn>
>>>>
>>>> Current fib_multipath_hash_policy can make hash based on the L3 or
>>>> L4. But it only work on the outer IP. So a specific tunnel always
>>>> has the same hash value. But a specific tunnel may contain so many
>>>> inner connections. However there is no good way for tunnel packet.
>>>> A specific tunnel route based on the percpu dst_cache. It will not
>>>> lookup route table for each packet.
>>>>
>>>> This patch provide a based cpu id hash policy. The different
>>>> connection run on different cpu and there will be different hash
>>>> value for percpu dst_cache.
>>>>
>>>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>>>> ---
>>>> net/ipv4/route.c | 6 ++++++
>>>> net/ipv4/sysctl_net_ipv4.c | 2 +-
>>>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>>>
>>> Hi,
>>> When we had the same issue in the bonding, we added a new mode which used
>>> the flow dissector to get the inner headers and hash on them.
>>> I believe that is even easier nowadays, but I think people wanted to
>>> use different hash algorithms as well and last we discussed this we
>>> were talking about yet another bpf use to generate the hash.
>>> Now we got bpf_set_hash() which can be used to achieve this from various
>>> places, if you use that with hash_policy=1 (L4) then skb->hash will
>>> be used and you can achieve any hashing that you desire.
>>>
>>> Cheers,
>>> Nik
>>>
>>>
>> Yes, we can set the skb->hash. But ip tunnel lookup the route table without skb.
>>
>> The most important for performance issue most tunnel send work with dst_cache.
>>
>> The dst_cache is percpu cache. So the number of dst_entry of a specific tunnel is the cpu cores .
>>
>> So It's more better hash based on outer and cpu id.
>>
>>
> I was just hoping for a more generic solution, anyway if this would go forward
> please add documentation for the new option in Documentation/networking/ip-sysctl.txt
> there's a description of fib_multipath_hash_policy. Also how is this supposed to work
> if skb is present & skb->hash is set ? I believe it will be ignored, so it really only
> works for lookups without an skb (or without a generated hash).
>
> A thought (unchecked/untested, may not make much sense, feel free to ignore):
> Why don't you make it mix flowi4_mark in with the hash and add some tunnel option
> which mixes the CPU in that field ?
> That is the new '2' mode will mix flowi4_mark with the hash and if some specific
> tunnel option is set, the tunnel code will mix smp_current_id() with fl4's mark
> before calling ip_route_output(), this sounds more useful to me as users will be
> able to affect the hash calculation via the mark from other places in different
> ways.
>
> Cheers,
> Nik
>
>
Yes, I agree that it need a more generic solution. So It should consider the both percpu cache and not cache situations.
Maye add a fl4.fl4_inner_hash for support this? It always hash according to the inner if it is the tunnel packet(
set the inner_hash by tunnel)
even there is a percpu cache it's also make the connections on the same core with the same hash result.
^ permalink raw reply
* Re: INFO: task hung in rtnetlink_rcv_msg
From: Daniel Borkmann @ 2019-02-23 0:47 UTC (permalink / raw)
To: Jakub Kicinski, Joe Perches
Cc: syzbot, ast, christian, davem, dsahern, hawk, idosch,
john.fastabend, kafai, ktkhai, linux-kernel, netdev, petrm, roopa,
songliubraving, syzkaller-bugs, xdp-newbies, yhs
In-Reply-To: <20190222134511.13eb7a82@cakuba.netronome.com>
On 02/22/2019 10:45 PM, Jakub Kicinski wrote:
> On Fri, 22 Feb 2019 12:14:57 -0800, Joe Perches wrote:
>> On Fri, 2019-02-22 at 12:01 -0800, Jakub Kicinski wrote:
>>> Hi!
>>>
>>> Seems like something funny is going on with get_maintainer.pl since XDP
>>> entry got added. We seem to have been CCed on:
>>
>> I suggest removing the MAINTAINERS line with
>>
>> K: xdp
>>
>> as xdp is a pretty generic file/patch content
>> regex match for the K: type
>>
>> $ git grep --name-only xdp | wc -l
>> 236
>>
>> Rather more files than desired.
>>
>> The N: match is dubious too.
>>
>> It's generally better to have specific lists of
>> maintained file patterns rather than using the
>> K: and N: pattern matches.
>>
>> ---
>> MAINTAINERS | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index d7ad97b235ec..aa703e2cb882 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -16970,7 +16970,6 @@ F: include/net/xdp.h
>> F: kernel/bpf/devmap.c
>> F: kernel/bpf/cpumap.c
>> F: include/trace/events/xdp.h
>> -K: xdp
>> N: xdp
>>
>> XDP SOCKETS (AF_XDP)
>
> Thanks for the explanation, at least now I know why it happens! :)
> I'll leave it to Daniel to decide if we really need it removed,
> so far the false positives weren't overwhelming, just surprising.
No strong opinion. I've seen this K+N pattern in a number of places
in the maintainers file. I'm fine either way if it gets too noisy. :)
^ permalink raw reply
* Re: [PATCH v4 net-next 3/5] net: dsa: microchip: get port link status
From: Florian Fainelli @ 2019-02-23 0:48 UTC (permalink / raw)
To: Tristram.Ha, Sergio Paracuellos, Andrew Lunn, Pavel Machek
Cc: UNGLinuxDriver, netdev
In-Reply-To: <1550882211-10381-4-git-send-email-Tristram.Ha@microchip.com>
Le 2/22/19 à 4:36 PM, Tristram.Ha@microchip.com a écrit :
> From: Tristram Ha <Tristram.Ha@microchip.com>
>
> Get port link status to know whether to read MIB counters when the link
> is going down.
>
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH v4 net-next 4/5] net: dsa: microchip: remove unnecessary include headers
From: Florian Fainelli @ 2019-02-23 0:48 UTC (permalink / raw)
To: Tristram.Ha, Sergio Paracuellos, Andrew Lunn, Pavel Machek
Cc: UNGLinuxDriver, netdev
In-Reply-To: <1550882211-10381-5-git-send-email-Tristram.Ha@microchip.com>
Le 2/22/19 à 4:36 PM, Tristram.Ha@microchip.com a écrit :
> From: Tristram Ha <Tristram.Ha@microchip.com>
>
> Remove unnecessary header include.
>
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH bpf-next 1/4] bpf: enable program stats
From: Daniel Borkmann @ 2019-02-23 1:06 UTC (permalink / raw)
To: Roman Gushchin, Alexei Starovoitov
Cc: davem@davemloft.net, netdev@vger.kernel.org, bpf@vger.kernel.org,
Kernel Team
In-Reply-To: <20190223003434.GA17559@tower.DHCP.thefacebook.com>
On 02/23/2019 01:34 AM, Roman Gushchin wrote:
> On Fri, Feb 22, 2019 at 03:36:41PM -0800, Alexei Starovoitov wrote:
>> JITed BPF programs are indistinguishable from kernel functions, but unlike
>> kernel code BPF code can be changed often.
>> Typical approach of "perf record" + "perf report" profiling and tunning of
>> kernel code works just as well for BPF programs, but kernel code doesn't
>> need to be monitored whereas BPF programs do.
>> Users load and run large amount of BPF programs.
>> These BPF stats allow tools monitor the usage of BPF on the server.
>> The monitoring tools will turn sysctl kernel.bpf_stats_enabled
>> on and off for few seconds to sample average cost of the programs.
>> Aggregated data over hours and days will provide an insight into cost of BPF
>> and alarms can trigger in case given program suddenly gets more expensive.
>>
>> The cost of two sched_clock() per program invocation adds ~20 nsec.
>> Fast BPF progs (like selftests/bpf/progs/test_pkt_access.c) will slow down
>> from ~40 nsec to ~60 nsec.
>> static_key minimizes the cost of the stats collection.
>> There is no measurable difference before/after this patch
>> with kernel.bpf_stats_enabled=0
>>
>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>> ---
>> include/linux/bpf.h | 7 +++++++
>> include/linux/filter.h | 16 +++++++++++++++-
>> kernel/bpf/core.c | 13 +++++++++++++
>> kernel/bpf/syscall.c | 24 ++++++++++++++++++++++--
>> kernel/bpf/verifier.c | 5 +++++
>> kernel/sysctl.c | 34 ++++++++++++++++++++++++++++++++++
>> 6 files changed, 96 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index de18227b3d95..14260674bc57 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -340,6 +340,11 @@ enum bpf_cgroup_storage_type {
>>
>> #define MAX_BPF_CGROUP_STORAGE_TYPE __BPF_CGROUP_STORAGE_MAX
>>
>> +struct bpf_prog_stats {
>> + u64 cnt;
>> + u64 nsecs;
>> +};
>> +
>> struct bpf_prog_aux {
>> atomic_t refcnt;
>> u32 used_map_cnt;
>> @@ -389,6 +394,7 @@ struct bpf_prog_aux {
>> * main prog always has linfo_idx == 0
>> */
>> u32 linfo_idx;
>> + struct bpf_prog_stats __percpu *stats;
>> union {
>> struct work_struct work;
>> struct rcu_head rcu;
>> @@ -559,6 +565,7 @@ void bpf_map_area_free(void *base);
>> void bpf_map_init_from_attr(struct bpf_map *map, union bpf_attr *attr);
>>
>> extern int sysctl_unprivileged_bpf_disabled;
>> +extern int sysctl_bpf_stats_enabled;
>>
>> int bpf_map_new_fd(struct bpf_map *map, int flags);
>> int bpf_prog_new_fd(struct bpf_prog *prog);
>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>> index f32b3eca5a04..7b14235b6f7d 100644
>> --- a/include/linux/filter.h
>> +++ b/include/linux/filter.h
>> @@ -533,7 +533,21 @@ struct sk_filter {
>> struct bpf_prog *prog;
>> };
>>
>> -#define BPF_PROG_RUN(filter, ctx) ({ cant_sleep(); (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
>> +DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
>> +
>> +#define BPF_PROG_RUN(prog, ctx) ({ \
>> + u32 ret; \
>> + cant_sleep(); \
>> + if (static_branch_unlikely(&bpf_stats_enabled_key)) { \
>> + u64 start = sched_clock(); \
>> + ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
>> + this_cpu_inc(prog->aux->stats->cnt); \
>> + this_cpu_add(prog->aux->stats->nsecs, \
>> + sched_clock() - start); \
>> + } else { \
>> + ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
>> + } \
>> + ret; })
>
> Can we bump "cnt" unconditionally and gate only the "nsecs" calculation?
>
> Why it might be useful?
> If the performance cost is too high to keep the statistics gathering
> always enabled, it will be possible to measure the average cost
> periodically and extrapolate.
In general, having some stats and timing info would be useful, but I
guess people might want to customize it in future even more specifically
beyond number of runs + time it takes. One thing that would be super
useful is to have some notion of __attribute__((constructor)) and
__attribute__((destructor)) support in BPF which gets then inlined into
prologue / epilogue of program. E.g. such infrastructure would allow to
mark an skb and measure time it takes through the BPF prog till it hits
an exit point somewhere (without having to explicitly code this into the
program everywhere). Other examples may be histograms or p99 latencies
that might probably be useful. Feels like for monitoring more ways to
program would be nice and to move it into the BPF insns sequence (e.g.
enforced globally or by choice of prog as another option)? Thoughts?
^ permalink raw reply
* [PATCH v2 bpf-next 3/9] bpf: Test bpf_tcp_enter_cwr in test_verifier
From: brakmo @ 2019-02-23 1:06 UTC (permalink / raw)
To: netdev
Cc: Martin Lau, Alexei Starovoitov, Daniel Borkmann, Eric Dumazet,
Kernel Team
In-Reply-To: <20190223010703.678070-1-brakmo@fb.com>
This test ensures the verifier has checked the arg1 of
BPF_FUNC_tcp_enter_cwr is of ARG_PTR_TO_TCP_SOCK type.
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
---
tools/testing/selftests/bpf/verifier/sock.c | 33 +++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/tools/testing/selftests/bpf/verifier/sock.c b/tools/testing/selftests/bpf/verifier/sock.c
index 0ddfdf76aba5..b07a083eeb59 100644
--- a/tools/testing/selftests/bpf/verifier/sock.c
+++ b/tools/testing/selftests/bpf/verifier/sock.c
@@ -382,3 +382,36 @@
.result = REJECT,
.errstr = "type=tcp_sock expected=sock",
},
+{
+ "bpf_tcp_enter_cwr(skb->sk)",
+ .insns = {
+ BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, offsetof(struct __sk_buff, sk)),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_1, 0, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ BPF_EMIT_CALL(BPF_FUNC_tcp_enter_cwr),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
+ .result = REJECT,
+ .errstr = "type=sock_common expected=tcp_sock",
+},
+{
+ "bpf_tcp_enter_cwr(bpf_tcp_sock(skb->sk))",
+ .insns = {
+ BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, offsetof(struct __sk_buff, sk)),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_1, 0, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ BPF_EMIT_CALL(BPF_FUNC_tcp_sock),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_tcp_enter_cwr),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
+ .result = ACCEPT,
+},
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 1/9] bpf: Remove const from get_func_proto
From: brakmo @ 2019-02-23 1:06 UTC (permalink / raw)
To: netdev
Cc: Martin Lau, Alexei Starovoitov, Daniel Borkmann, Eric Dumazet,
Kernel Team
In-Reply-To: <20190223010703.678070-1-brakmo@fb.com>
From: Martin KaFai Lau <kafai@fb.com>
The next patch needs to set a bit in "prog" in
cg_skb_func_proto(). Hence, the "const struct bpf_prog *"
as a second argument will not work.
This patch removes the "const" from get_func_proto and
makes the needed changes to all get_func_proto implementations
to avoid compiler error.
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
---
drivers/media/rc/bpf-lirc.c | 2 +-
include/linux/bpf.h | 2 +-
kernel/bpf/cgroup.c | 2 +-
kernel/trace/bpf_trace.c | 10 +++++-----
net/core/filter.c | 30 +++++++++++++++---------------
5 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
index 390a722e6211..6adb7f734cb9 100644
--- a/drivers/media/rc/bpf-lirc.c
+++ b/drivers/media/rc/bpf-lirc.c
@@ -82,7 +82,7 @@ static const struct bpf_func_proto rc_pointer_rel_proto = {
};
static const struct bpf_func_proto *
-lirc_mode2_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+lirc_mode2_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_rc_repeat:
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index de18227b3d95..d5ba2fc01af3 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -287,7 +287,7 @@ struct bpf_verifier_ops {
/* return eBPF function prototype for verification */
const struct bpf_func_proto *
(*get_func_proto)(enum bpf_func_id func_id,
- const struct bpf_prog *prog);
+ struct bpf_prog *prog);
/* return true if 'size' wide access at offset 'off' within bpf_context
* with 'type' (read or write) is allowed
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 4e807973aa80..0de0f5d98b46 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -701,7 +701,7 @@ int __cgroup_bpf_check_dev_permission(short dev_type, u32 major, u32 minor,
EXPORT_SYMBOL(__cgroup_bpf_check_dev_permission);
static const struct bpf_func_proto *
-cgroup_dev_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+cgroup_dev_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_map_lookup_elem:
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index f1a86a0d881d..0d2f60828d7d 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -561,7 +561,7 @@ static const struct bpf_func_proto bpf_probe_read_str_proto = {
};
static const struct bpf_func_proto *
-tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+tracing_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_map_lookup_elem:
@@ -610,7 +610,7 @@ tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
}
static const struct bpf_func_proto *
-kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+kprobe_prog_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_perf_event_output:
@@ -726,7 +726,7 @@ static const struct bpf_func_proto bpf_get_stack_proto_tp = {
};
static const struct bpf_func_proto *
-tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+tp_prog_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_perf_event_output:
@@ -790,7 +790,7 @@ static const struct bpf_func_proto bpf_perf_prog_read_value_proto = {
};
static const struct bpf_func_proto *
-pe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+pe_prog_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_perf_event_output:
@@ -873,7 +873,7 @@ static const struct bpf_func_proto bpf_get_stack_proto_raw_tp = {
};
static const struct bpf_func_proto *
-raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+raw_tp_prog_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_perf_event_output:
diff --git a/net/core/filter.c b/net/core/filter.c
index 85749f6ec789..97916eedfe69 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5508,7 +5508,7 @@ bpf_base_func_proto(enum bpf_func_id func_id)
}
static const struct bpf_func_proto *
-sock_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+sock_filter_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
/* inet and inet6 sockets are created in a process
@@ -5524,7 +5524,7 @@ sock_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
}
static const struct bpf_func_proto *
-sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+sock_addr_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
/* inet and inet6 sockets are created in a process
@@ -5558,7 +5558,7 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
}
static const struct bpf_func_proto *
-sk_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+sk_filter_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_skb_load_bytes:
@@ -5575,7 +5575,7 @@ sk_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
}
static const struct bpf_func_proto *
-cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+cg_skb_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_get_local_storage:
@@ -5592,7 +5592,7 @@ cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
}
static const struct bpf_func_proto *
-tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+tc_cls_act_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_skb_store_bytes:
@@ -5685,7 +5685,7 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
}
static const struct bpf_func_proto *
-xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+xdp_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_perf_event_output:
@@ -5723,7 +5723,7 @@ const struct bpf_func_proto bpf_sock_map_update_proto __weak;
const struct bpf_func_proto bpf_sock_hash_update_proto __weak;
static const struct bpf_func_proto *
-sock_ops_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+sock_ops_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_setsockopt:
@@ -5751,7 +5751,7 @@ const struct bpf_func_proto bpf_msg_redirect_map_proto __weak;
const struct bpf_func_proto bpf_msg_redirect_hash_proto __weak;
static const struct bpf_func_proto *
-sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+sk_msg_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_msg_redirect_map:
@@ -5777,7 +5777,7 @@ const struct bpf_func_proto bpf_sk_redirect_map_proto __weak;
const struct bpf_func_proto bpf_sk_redirect_hash_proto __weak;
static const struct bpf_func_proto *
-sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+sk_skb_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_skb_store_bytes:
@@ -5812,7 +5812,7 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
}
static const struct bpf_func_proto *
-flow_dissector_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+flow_dissector_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_skb_load_bytes:
@@ -5823,7 +5823,7 @@ flow_dissector_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
}
static const struct bpf_func_proto *
-lwt_out_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+lwt_out_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_skb_load_bytes:
@@ -5850,7 +5850,7 @@ lwt_out_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
}
static const struct bpf_func_proto *
-lwt_in_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+lwt_in_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_lwt_push_encap:
@@ -5861,7 +5861,7 @@ lwt_in_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
}
static const struct bpf_func_proto *
-lwt_xmit_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+lwt_xmit_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_skb_get_tunnel_key:
@@ -5898,7 +5898,7 @@ lwt_xmit_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
}
static const struct bpf_func_proto *
-lwt_seg6local_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+lwt_seg6local_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
{
switch (func_id) {
#if IS_ENABLED(CONFIG_IPV6_SEG6_BPF)
@@ -8124,7 +8124,7 @@ static const struct bpf_func_proto sk_reuseport_load_bytes_relative_proto = {
static const struct bpf_func_proto *
sk_reuseport_func_proto(enum bpf_func_id func_id,
- const struct bpf_prog *prog)
+ struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_sk_select_reuseport:
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 2/9] bpf: Add bpf helper bpf_tcp_enter_cwr
From: brakmo @ 2019-02-23 1:06 UTC (permalink / raw)
To: netdev
Cc: Martin Lau, Alexei Starovoitov, Daniel Borkmann, Eric Dumazet,
Kernel Team
In-Reply-To: <20190223010703.678070-1-brakmo@fb.com>
From: Martin KaFai Lau <kafai@fb.com>
This patch adds a new bpf helper BPF_FUNC_tcp_enter_cwr
"int bpf_tcp_enter_cwr(struct bpf_tcp_sock *tp)".
It is added to BPF_PROG_TYPE_CGROUP_SKB which can be attached
to the egress path where the bpf prog is called by
ip_finish_output() or ip6_finish_output(). The verifier
ensures that the parameter must be a tcp_sock.
This helper makes a tcp_sock enter CWR state. It can be used
by a bpf_prog to manage egress network bandwidth limit per
cgroupv2. A later patch will have a sample program to
show how it can be used to limit bandwidth usage per cgroupv2.
To ensure it is only called from BPF_CGROUP_INET_EGRESS, the
attr->expected_attach_type must be specified as BPF_CGROUP_INET_EGRESS
during load time if the prog uses this new helper.
The newly added prog->enforce_expected_attach_type bit will also be set
if this new helper is used. This bit is for backward compatibility reason
because currently prog->expected_attach_type has been ignored in
BPF_PROG_TYPE_CGROUP_SKB. During attach time,
prog->expected_attach_type is only enforced if the
prog->enforce_expected_attach_type bit is set.
i.e. prog->expected_attach_type is only enforced if this new helper
is used by the prog.
Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
include/linux/bpf.h | 1 +
include/linux/filter.h | 3 ++-
include/uapi/linux/bpf.h | 9 ++++++++-
kernel/bpf/syscall.c | 12 ++++++++++++
kernel/bpf/verifier.c | 4 ++++
net/core/filter.c | 25 +++++++++++++++++++++++++
6 files changed, 52 insertions(+), 2 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d5ba2fc01af3..2d54ba7cf9dd 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -195,6 +195,7 @@ enum bpf_arg_type {
ARG_PTR_TO_SOCKET, /* pointer to bpf_sock */
ARG_PTR_TO_SPIN_LOCK, /* pointer to bpf_spin_lock */
ARG_PTR_TO_SOCK_COMMON, /* pointer to sock_common */
+ ARG_PTR_TO_TCP_SOCK, /* pointer to tcp_sock */
};
/* type of values returned from helper functions */
diff --git a/include/linux/filter.h b/include/linux/filter.h
index f32b3eca5a04..c6e878bdc5a6 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -510,7 +510,8 @@ struct bpf_prog {
blinded:1, /* Was blinded */
is_func:1, /* program is a bpf function */
kprobe_override:1, /* Do we override a kprobe? */
- has_callchain_buf:1; /* callchain buffer allocated? */
+ has_callchain_buf:1, /* callchain buffer allocated? */
+ enforce_expected_attach_type:1; /* Enforce expected_attach_type checking at attach time */
enum bpf_prog_type type; /* Type of BPF program */
enum bpf_attach_type expected_attach_type; /* For some prog types */
u32 len; /* Number of filter blocks */
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bcdd2474eee7..95b5058fa945 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2359,6 +2359,12 @@ union bpf_attr {
* Return
* A **struct bpf_tcp_sock** pointer on success, or NULL in
* case of failure.
+ *
+ * int bpf_tcp_enter_cwr(struct bpf_tcp_sock *tp)
+ * Description
+ * Make a tcp_sock enter CWR state.
+ * Return
+ * 0 on success, or a negative error in case of failure.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2457,7 +2463,8 @@ union bpf_attr {
FN(spin_lock), \
FN(spin_unlock), \
FN(sk_fullsock), \
- FN(tcp_sock),
+ FN(tcp_sock), \
+ FN(tcp_enter_cwr),
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ec7c552af76b..9a478f2875cd 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1482,6 +1482,14 @@ bpf_prog_load_check_attach_type(enum bpf_prog_type prog_type,
default:
return -EINVAL;
}
+ case BPF_PROG_TYPE_CGROUP_SKB:
+ switch (expected_attach_type) {
+ case BPF_CGROUP_INET_INGRESS:
+ case BPF_CGROUP_INET_EGRESS:
+ return 0;
+ default:
+ return -EINVAL;
+ }
default:
return 0;
}
@@ -1725,6 +1733,10 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
case BPF_PROG_TYPE_CGROUP_SOCK:
case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
return attach_type == prog->expected_attach_type ? 0 : -EINVAL;
+ case BPF_PROG_TYPE_CGROUP_SKB:
+ return prog->enforce_expected_attach_type &&
+ prog->expected_attach_type != attach_type ?
+ -EINVAL : 0;
default:
return 0;
}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1b9496c41383..95fb385c6f3c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2424,6 +2424,10 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
return -EFAULT;
}
meta->ptr_id = reg->id;
+ } else if (arg_type == ARG_PTR_TO_TCP_SOCK) {
+ expected_type = PTR_TO_TCP_SOCK;
+ if (type != expected_type)
+ goto err_type;
} else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
if (meta->func_id == BPF_FUNC_spin_lock) {
if (process_spin_lock(env, regno, true))
diff --git a/net/core/filter.c b/net/core/filter.c
index 97916eedfe69..ca57ef25279c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5426,6 +5426,24 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
.arg1_type = ARG_PTR_TO_SOCK_COMMON,
};
+BPF_CALL_1(bpf_tcp_enter_cwr, struct tcp_sock *, tp)
+{
+ struct sock *sk = (struct sock *)tp;
+
+ if (sk->sk_state == TCP_ESTABLISHED) {
+ tcp_enter_cwr(sk);
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static const struct bpf_func_proto bpf_tcp_enter_cwr_proto = {
+ .func = bpf_tcp_enter_cwr,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_TCP_SOCK,
+};
#endif /* CONFIG_INET */
bool bpf_helper_changes_pkt_data(void *func)
@@ -5585,6 +5603,13 @@ cg_skb_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog)
#ifdef CONFIG_INET
case BPF_FUNC_tcp_sock:
return &bpf_tcp_sock_proto;
+ case BPF_FUNC_tcp_enter_cwr:
+ if (prog->expected_attach_type == BPF_CGROUP_INET_EGRESS) {
+ prog->enforce_expected_attach_type = 1;
+ return &bpf_tcp_enter_cwr_proto;
+ } else {
+ return NULL;
+ }
#endif
default:
return sk_filter_func_proto(func_id, prog);
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 0/9] bpf: Network Resource Manager (NRM)
From: brakmo @ 2019-02-23 1:06 UTC (permalink / raw)
To: netdev
Cc: Martin Lau, Alexei Starovoitov, Daniel Borkmann, Eric Dumazet,
Kernel Team
Network Resource Manager is a framework for limiting the bandwidth used
by v2 cgroups. It consists of 4 BPF helpers and a sample BPF program to
limit egress bandwdith as well as a sample user program and script to
simplify NRM testing.
The sample NRM BPF program is not meant to be production quality, it is
provided as proof of concept. A lot more information, including sample
runs in some cases, are provided in the commit messages of the individual
patches.
Two more BPF programs, one to limit ingress and one that limits egress
and uses fq's Earliest Departure Time feature (EDT), will be provided in an
upcomming patchset.
Changes from v1 to v2:
* bpf_tcp_enter_cwr can only be called from a cgroup skb egress BPF
program (otherwise load or attach will fail) where we already hold
the sk lock. Also only applies for ESTABLISHED state.
* bpf_skb_ecn_set_ce uses INET_ECN_set_ce()
* bpf_tcp_check_probe_timer now uses tcp_reset_xmit_timer. Can only be
used by egress cgroup skb programs.
* removed load_cg_skb user program.
* nrm bpf egress program checks packet header in skb to determine
ECN value. Now also works for ECN enabled UDP packets.
Using ECN_ defines instead of integers.
* NRM script test program now uses bpftool instead of load_cg_skb
Martin KaFai Lau (2):
bpf: Remove const from get_func_proto
bpf: Add bpf helper bpf_tcp_enter_cwr
brakmo (7):
bpf: Test bpf_tcp_enter_cwr in test_verifier
bpf: add bpf helper bpf_skb_ecn_set_ce
bpf: Add bpf helper bpf_tcp_check_probe_timer
bpf: sync bpf.h to tools and update bpf_helpers.h
bpf: Sample NRM BPF program to limit egress bw
bpf: User program for testing NRM
bpf: NRM test script
drivers/media/rc/bpf-lirc.c | 2 +-
include/linux/bpf.h | 3 +-
include/linux/filter.h | 3 +-
include/uapi/linux/bpf.h | 27 +-
kernel/bpf/cgroup.c | 2 +-
kernel/bpf/syscall.c | 12 +
kernel/bpf/verifier.c | 4 +
kernel/trace/bpf_trace.c | 10 +-
net/core/filter.c | 101 ++++-
samples/bpf/Makefile | 5 +
samples/bpf/do_nrm_test.sh | 437 +++++++++++++++++++
samples/bpf/nrm.c | 440 ++++++++++++++++++++
samples/bpf/nrm.h | 31 ++
samples/bpf/nrm_kern.h | 137 ++++++
samples/bpf/nrm_out_kern.c | 190 +++++++++
tools/include/uapi/linux/bpf.h | 27 +-
tools/testing/selftests/bpf/bpf_helpers.h | 6 +
tools/testing/selftests/bpf/verifier/sock.c | 33 ++
18 files changed, 1444 insertions(+), 26 deletions(-)
create mode 100755 samples/bpf/do_nrm_test.sh
create mode 100644 samples/bpf/nrm.c
create mode 100644 samples/bpf/nrm.h
create mode 100644 samples/bpf/nrm_kern.h
create mode 100644 samples/bpf/nrm_out_kern.c
--
2.17.1
^ 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