* [PATCH net] ipv4: Pass original device to ip_rcv_finish_core
From: David Ahern @ 2019-02-25 21:55 UTC (permalink / raw)
To: davem; +Cc: netdev, David Ahern, Edward Cree
From: David Ahern <dsahern@gmail.com>
ip_route_input_rcu expects the original ingress device (e.g., for
proper multicast handling). The skb->dev can be changed by l3mdev_ip_rcv,
so dev needs to be saved prior to calling it. This was the behavior prior
to the listify changes.
Fixes: 5fa12739a53d0 ("net: ipv4: listify ip_rcv_finish")
Cc: Edward Cree <ecree@solarflare.com>
Signed-off-by: David Ahern <dsahern@gmail.com>
---
net/ipv4/ip_input.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index 51d8efba6de2..1f4737b77067 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -307,11 +307,10 @@ static inline bool ip_rcv_options(struct sk_buff *skb)
}
static int ip_rcv_finish_core(struct net *net, struct sock *sk,
- struct sk_buff *skb)
+ struct sk_buff *skb, struct net_device *dev)
{
const struct iphdr *iph = ip_hdr(skb);
int (*edemux)(struct sk_buff *skb);
- struct net_device *dev = skb->dev;
struct rtable *rt;
int err;
@@ -400,6 +399,7 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
static int ip_rcv_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
{
+ struct net_device *dev = skb->dev;
int ret;
/* if ingress device is enslaved to an L3 master device pass the
@@ -409,7 +409,7 @@ static int ip_rcv_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
if (!skb)
return NET_RX_SUCCESS;
- ret = ip_rcv_finish_core(net, sk, skb);
+ ret = ip_rcv_finish_core(net, sk, skb, dev);
if (ret != NET_RX_DROP)
ret = dst_input(skb);
return ret;
@@ -545,6 +545,7 @@ static void ip_list_rcv_finish(struct net *net, struct sock *sk,
INIT_LIST_HEAD(&sublist);
list_for_each_entry_safe(skb, next, head, list) {
+ struct net_device *dev = skb->dev;
struct dst_entry *dst;
skb_list_del_init(skb);
@@ -554,7 +555,7 @@ static void ip_list_rcv_finish(struct net *net, struct sock *sk,
skb = l3mdev_ip_rcv(skb);
if (!skb)
continue;
- if (ip_rcv_finish_core(net, sk, skb) == NET_RX_DROP)
+ if (ip_rcv_finish_core(net, sk, skb, dev) == NET_RX_DROP)
continue;
dst = skb_dst(skb);
--
2.11.0
^ permalink raw reply related
* [PATCH] rtnetlink: Synchronze net in rtnl_unregister()
From: Dmitry Safonov @ 2019-02-25 21:27 UTC (permalink / raw)
To: linux-kernel
Cc: 0x7f454c46, Dmitry Safonov, David S. Miller, Florian Westphal,
Hannes Frederic Sowa, netdev
rtnl_unregister() unsets handler from table, which is protected
by rtnl_lock or RCU. At this moment only dump handlers access the table
with rcu_lock(). Every other user accesses under rtnl.
Callers may expect that rtnl_unregister() prevents any further handlers
calls, alike rtnl_unregister_all(). And they do expect it.
I've looked on in-tree caller uses:
br_mdb: safe, but in err-path br_netlink_init()
fib_rules: safe - err-path is very early in __init
ip6mr: safe - following unregister_pernet_subsys() calls internally rcu_barrier()
qrtr: safe - following sock_unregister() calls internally synchronize_rcu()
While it's possible to document that rtnl_unregister() requires
synchronize_net() afterwards - unlike rtnl_unregister_all(), I believe
the module exit is very much slow-path.
Issue seems to be very theoretical and unlikely, so I'm not Cc'ing
stable tree.
Fixes: 6853dd488119 ("rtnetlink: protect handler table with rcu")
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Florian Westphal <fw@strlen.de>
Cc: "Hannes Frederic Sowa" <hannes@stressinduktion.org>
Cc: netdev@vger.kernel.org
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
net/core/rtnetlink.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 5ea1bed08ede..3db70da4f951 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -308,7 +308,9 @@ int rtnl_unregister(int protocol, int msgtype)
rcu_assign_pointer(tab[msgindex], NULL);
rtnl_unlock();
- kfree_rcu(link, rcu);
+ synchronize_net();
+
+ kfree(link);
return 0;
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2] bpf: test_bpf: turn of preemption in function __run_once
From: Daniel Borkmann @ 2019-02-25 21:28 UTC (permalink / raw)
To: Anders Roxell, ast; +Cc: netdev, linux-kernel, Arnd Bergmann
In-Reply-To: <20190222084652.1326-1-anders.roxell@linaro.org>
On 02/22/2019 09:46 AM, Anders Roxell wrote:
> When running BPF test suite the following splat occurs:
>
> [ 415.930950] test_bpf: #0 TAX jited:0
> [ 415.931067] BUG: assuming atomic context at lib/test_bpf.c:6674
> [ 415.946169] in_atomic(): 0, irqs_disabled(): 0, pid: 11556, name: modprobe
> [ 415.953176] INFO: lockdep is turned off.
> [ 415.957207] CPU: 1 PID: 11556 Comm: modprobe Tainted: G W 5.0.0-rc7-next-20190220 #1
> [ 415.966328] Hardware name: HiKey Development Board (DT)
> [ 415.971592] Call trace:
> [ 415.974069] dump_backtrace+0x0/0x160
> [ 415.977761] show_stack+0x24/0x30
> [ 415.981104] dump_stack+0xc8/0x114
> [ 415.984534] __cant_sleep+0xf0/0x108
> [ 415.988145] test_bpf_init+0x5e0/0x1000 [test_bpf]
> [ 415.992971] do_one_initcall+0x90/0x428
> [ 415.996837] do_init_module+0x60/0x1e4
> [ 416.000614] load_module+0x1de0/0x1f50
> [ 416.004391] __se_sys_finit_module+0xc8/0xe0
> [ 416.008691] __arm64_sys_finit_module+0x24/0x30
> [ 416.013255] el0_svc_common+0x78/0x130
> [ 416.017031] el0_svc_handler+0x38/0x78
> [ 416.020806] el0_svc+0x8/0xc
>
> Rework so that preemption is disabled when we loop over function
> 'BPF_PROG_RUN(...)'.
> Commit 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> highlighted the issue.
(Technically not an issue in the test suite itself, but good to have same
guarantees everywhere in BPF_PROG_RUN().)
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH bpf-next] bpf/test_run: fix unkillable BPF_PROG_TEST_RUN for flow dissector
From: Daniel Borkmann @ 2019-02-25 21:36 UTC (permalink / raw)
To: Stanislav Fomichev, netdev; +Cc: davem, ast, syzbot
In-Reply-To: <20190219185417.145696-1-sdf@google.com>
On 02/19/2019 07:54 PM, Stanislav Fomichev wrote:
> Syzbot found out that running BPF_PROG_TEST_RUN with repeat=0xffffffff
> makes process unkillable. The problem is that when CONFIG_PREEMPT is
> enabled, we never see need_resched() return true. This is due to the
> fact that preempt_enable() (which we do in bpf_test_run_one on each
> iteration) now handles resched if it's needed.
>
> Let's disable preemption for the whole run, not per test. In this case
> we can properly see whether resched is needed.
> Let's also properly return -EINTR to the userspace in case of a signal
> interrupt.
>
> This is a follow up for a recently fixed issue in bpf_test_run, see
> commit df1a2cb7c74b ("bpf/test_run: fix unkillable
> BPF_PROG_TEST_RUN").
>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
Applied, thanks! Would be nice to consolidate at least some of these bits
so it's not duplicated, perhaps __always_inline might help for function args
in avoiding indirect calls (iirc, perf's rb handling uses this heavily).
> ---
> net/bpf/test_run.c | 26 ++++++++++++++++++++------
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 2c5172b33209..619655db8d9e 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -293,31 +293,45 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
> if (!repeat)
> repeat = 1;
>
> + rcu_read_lock();
> + preempt_disable();
> time_start = ktime_get_ns();
> for (i = 0; i < repeat; i++) {
> - preempt_disable();
> - rcu_read_lock();
> retval = __skb_flow_bpf_dissect(prog, skb,
> &flow_keys_dissector,
> &flow_keys);
> - rcu_read_unlock();
> - preempt_enable();
> +
> + if (signal_pending(current)) {
> + preempt_enable();
> + rcu_read_unlock();
> +
> + ret = -EINTR;
> + goto out;
> + }
>
> if (need_resched()) {
> - if (signal_pending(current))
> - break;
> time_spent += ktime_get_ns() - time_start;
> + preempt_enable();
> + rcu_read_unlock();
> +
> cond_resched();
> +
> + rcu_read_lock();
> + preempt_disable();
> time_start = ktime_get_ns();
> }
> }
> time_spent += ktime_get_ns() - time_start;
> + preempt_enable();
> + rcu_read_unlock();
> +
> do_div(time_spent, repeat);
> duration = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
>
> ret = bpf_test_finish(kattr, uattr, &flow_keys, sizeof(flow_keys),
> retval, duration);
>
> +out:
> kfree_skb(skb);
> kfree(sk);
> return ret;
>
^ permalink raw reply
* Re: [PATCH iproute2 1/1] man: Document COLORFGBG environment variable
From: Petr Vorel @ 2019-02-25 21:38 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20190221100931.0bfcf868@shemminger-XPS-13-9360>
Hi Stephen,
> > Default colors are not contrast enough on dark backround
> > and this functionality, which uses more suitable colors
> > is hidden in the code.
> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> COLORFGBG is a semi-standard thing used by many programs.
> Maybe a reference to a more complete description of it on the web
> might be better.
If there is any, I'd have added the link instead :).
So I used the description from the code I got inspiration when adding COLORFGBG
support into iproute2. This description is actually complete for our usage of
the variable. If you don't like it, even mentioning COLORFGBG in man would be
better than nothing (one would look into the source).
People have asked for it before [1].
Kind regards,
Petr
[1] https://patchwork.ozlabs.org/patch/814394/
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: make sure signal interrupts BPF_PROG_TEST_RUN
From: Daniel Borkmann @ 2019-02-25 21:37 UTC (permalink / raw)
To: Stanislav Fomichev, netdev; +Cc: davem, ast
In-Reply-To: <20190221191100.84147-1-sdf@google.com>
On 02/21/2019 08:11 PM, Stanislav Fomichev wrote:
> Simple test that I used to reproduce the issue in the previous commit:
> Do BPF_PROG_TEST_RUN with max iterations, each program is 4096 simple
> move instructions. File alarm in 0.1 second and check that
> bpf_prog_test_run is interrupted (i.e. test doesn't hang).
>
> Note: reposting this for bpf-next to avoid linux-next conflict. In this
> version I test both BPF_PROG_TYPE_SOCKET_FILTER (which uses generic
> bpf_test_run implementation) and BPF_PROG_TYPE_FLOW_DISSECTOR (which has
> it own loop with preempt handling in bpf_prog_test_run_flow_dissector).
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
Applied, thanks!
^ permalink raw reply
* [PATCH 4.20 040/183] soc/fsl/qe: fix err handling of ucc_of_parse_tdm
From: Greg Kroah-Hartman @ 2019-02-25 21:10 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Li Yang, Christophe LEROY, Wen Yang,
Peng Hao, Julia Lawall, Zhao Qiang, David S. Miller, netdev,
linuxppc-dev, Sasha Levin
In-Reply-To: <20190225195054.748060397@linuxfoundation.org>
4.20-stable review patch. If anyone has any objections, please let me know.
------------------
[ Upstream commit 8d68100ab4ad92560a16a68b72e068613ac4d573 ]
Currently there are some issues with the ucc_of_parse_tdm function:
1, a possible null pointer dereference in ucc_of_parse_tdm,
detected by the semantic patch deref_null.cocci,
with the following warning:
drivers/soc/fsl/qe/qe_tdm.c:177:21-24: ERROR: pdev is NULL but dereferenced.
2, dev gets modified, so in any case that devm_iounmap() will fail
even when the new pdev is valid, because the iomap was done with a
different pdev.
3, there is no driver bind with the "fsl,t1040-qe-si" or
"fsl,t1040-qe-siram" device. So allocating resources using devm_*()
with these devices won't provide a cleanup path for these resources
when the caller fails.
This patch fixes them.
Suggested-by: Li Yang <leoyang.li@nxp.com>
Suggested-by: Christophe LEROY <christophe.leroy@c-s.fr>
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Reviewed-by: Peng Hao <peng.hao2@zte.com.cn>
CC: Julia Lawall <julia.lawall@lip6.fr>
CC: Zhao Qiang <qiang.zhao@nxp.com>
CC: David S. Miller <davem@davemloft.net>
CC: netdev@vger.kernel.org
CC: linuxppc-dev@lists.ozlabs.org
CC: linux-kernel@vger.kernel.org
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/net/wan/fsl_ucc_hdlc.c | 62 +++++++++++++++++++++++++++++++++-
drivers/soc/fsl/qe/qe_tdm.c | 55 ------------------------------
2 files changed, 61 insertions(+), 56 deletions(-)
diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 4d6409605207c..af13d8cf94ad4 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -1049,6 +1049,54 @@ static const struct net_device_ops uhdlc_ops = {
.ndo_tx_timeout = uhdlc_tx_timeout,
};
+static int hdlc_map_iomem(char *name, int init_flag, void __iomem **ptr)
+{
+ struct device_node *np;
+ struct platform_device *pdev;
+ struct resource *res;
+ static int siram_init_flag;
+ int ret = 0;
+
+ np = of_find_compatible_node(NULL, NULL, name);
+ if (!np)
+ return -EINVAL;
+
+ pdev = of_find_device_by_node(np);
+ if (!pdev) {
+ pr_err("%pOFn: failed to lookup pdev\n", np);
+ of_node_put(np);
+ return -EINVAL;
+ }
+
+ of_node_put(np);
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ ret = -EINVAL;
+ goto error_put_device;
+ }
+ *ptr = ioremap(res->start, resource_size(res));
+ if (!*ptr) {
+ ret = -ENOMEM;
+ goto error_put_device;
+ }
+
+ /* We've remapped the addresses, and we don't need the device any
+ * more, so we should release it.
+ */
+ put_device(&pdev->dev);
+
+ if (init_flag && siram_init_flag == 0) {
+ memset_io(*ptr, 0, resource_size(res));
+ siram_init_flag = 1;
+ }
+ return 0;
+
+error_put_device:
+ put_device(&pdev->dev);
+
+ return ret;
+}
+
static int ucc_hdlc_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
@@ -1143,6 +1191,15 @@ static int ucc_hdlc_probe(struct platform_device *pdev)
ret = ucc_of_parse_tdm(np, utdm, ut_info);
if (ret)
goto free_utdm;
+
+ ret = hdlc_map_iomem("fsl,t1040-qe-si", 0,
+ (void __iomem **)&utdm->si_regs);
+ if (ret)
+ goto free_utdm;
+ ret = hdlc_map_iomem("fsl,t1040-qe-siram", 1,
+ (void __iomem **)&utdm->siram);
+ if (ret)
+ goto unmap_si_regs;
}
if (of_property_read_u16(np, "fsl,hmask", &uhdlc_priv->hmask))
@@ -1151,7 +1208,7 @@ static int ucc_hdlc_probe(struct platform_device *pdev)
ret = uhdlc_init(uhdlc_priv);
if (ret) {
dev_err(&pdev->dev, "Failed to init uhdlc\n");
- goto free_utdm;
+ goto undo_uhdlc_init;
}
dev = alloc_hdlcdev(uhdlc_priv);
@@ -1181,6 +1238,9 @@ static int ucc_hdlc_probe(struct platform_device *pdev)
free_dev:
free_netdev(dev);
undo_uhdlc_init:
+ iounmap(utdm->siram);
+unmap_si_regs:
+ iounmap(utdm->si_regs);
free_utdm:
if (uhdlc_priv->tsa)
kfree(utdm);
diff --git a/drivers/soc/fsl/qe/qe_tdm.c b/drivers/soc/fsl/qe/qe_tdm.c
index f78c34647ca2d..76480df195a87 100644
--- a/drivers/soc/fsl/qe/qe_tdm.c
+++ b/drivers/soc/fsl/qe/qe_tdm.c
@@ -44,10 +44,6 @@ int ucc_of_parse_tdm(struct device_node *np, struct ucc_tdm *utdm,
const char *sprop;
int ret = 0;
u32 val;
- struct resource *res;
- struct device_node *np2;
- static int siram_init_flag;
- struct platform_device *pdev;
sprop = of_get_property(np, "fsl,rx-sync-clock", NULL);
if (sprop) {
@@ -124,57 +120,6 @@ int ucc_of_parse_tdm(struct device_node *np, struct ucc_tdm *utdm,
utdm->siram_entry_id = val;
set_si_param(utdm, ut_info);
-
- np2 = of_find_compatible_node(NULL, NULL, "fsl,t1040-qe-si");
- if (!np2)
- return -EINVAL;
-
- pdev = of_find_device_by_node(np2);
- if (!pdev) {
- pr_err("%pOFn: failed to lookup pdev\n", np2);
- of_node_put(np2);
- return -EINVAL;
- }
-
- of_node_put(np2);
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- utdm->si_regs = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(utdm->si_regs)) {
- ret = PTR_ERR(utdm->si_regs);
- goto err_miss_siram_property;
- }
-
- np2 = of_find_compatible_node(NULL, NULL, "fsl,t1040-qe-siram");
- if (!np2) {
- ret = -EINVAL;
- goto err_miss_siram_property;
- }
-
- pdev = of_find_device_by_node(np2);
- if (!pdev) {
- ret = -EINVAL;
- pr_err("%pOFn: failed to lookup pdev\n", np2);
- of_node_put(np2);
- goto err_miss_siram_property;
- }
-
- of_node_put(np2);
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- utdm->siram = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(utdm->siram)) {
- ret = PTR_ERR(utdm->siram);
- goto err_miss_siram_property;
- }
-
- if (siram_init_flag == 0) {
- memset_io(utdm->siram, 0, resource_size(res));
- siram_init_flag = 1;
- }
-
- return ret;
-
-err_miss_siram_property:
- devm_iounmap(&pdev->dev, utdm->si_regs);
return ret;
}
EXPORT_SYMBOL(ucc_of_parse_tdm);
--
2.19.1
^ permalink raw reply related
* Re: [PATCH v2 6/6] sched: Annotate perf_domain pointer with __rcu
From: Paul E. McKenney @ 2019-02-25 21:12 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ingo Molnar,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, kernel-hardening,
kernel-team, Kirill Tkhai, Lai Jiangshan, Martin KaFai Lau,
Mathieu Desnoyers, netdev, Peter Zijlstra, Quentin Perret, rcu,
Song Liu, Steven Rostedt, Vincent Guittot, xdp-newbies,
Yonghong Song
In-Reply-To: <20190223063434.6793-7-joel@joelfernandes.org>
On Sat, Feb 23, 2019 at 01:34:34AM -0500, Joel Fernandes (Google) wrote:
> This fixes the following sparse errors in sched/fair.c:
>
> fair.c:6506:14: error: incompatible types in comparison expression
> fair.c:8642:21: error: incompatible types in comparison expression
>
> Using __rcu will also help sparse catch any future bugs.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
From an RCU perspective:
Reviewed-by: Paul E. McKenney <paulmck@linux.ibm.com>
> ---
> kernel/sched/sched.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index ca6a79f57e7a..c8e6514433a9 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -780,7 +780,7 @@ struct root_domain {
> * NULL-terminated list of performance domains intersecting with the
> * CPUs of the rd. Protected by RCU.
> */
> - struct perf_domain *pd;
> + struct perf_domain __rcu *pd;
> };
>
> extern struct root_domain def_root_domain;
> --
> 2.21.0.rc0.258.g878e2cd30e-goog
>
^ permalink raw reply
* Re: [PATCH v2 5/6] rcuwait: Annotate task_struct with __rcu
From: Paul E. McKenney @ 2019-02-25 21:11 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ingo Molnar,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, kernel-hardening,
kernel-team, Kirill Tkhai, Lai Jiangshan, Martin KaFai Lau,
Mathieu Desnoyers, netdev, Peter Zijlstra, Quentin Perret, rcu,
Song Liu, Steven Rostedt, Vincent Guittot, xdp-newbies,
Yonghong Song
In-Reply-To: <20190223063434.6793-6-joel@joelfernandes.org>
On Sat, Feb 23, 2019 at 01:34:33AM -0500, Joel Fernandes (Google) wrote:
> This suppresses sparse error generated due to the recently added
> rcu_assign_pointer sparse check.
>
> percpu-rwsem.c:162:9: sparse: error: incompatible types in comparison expression
> exit.c:316:16: sparse: error: incompatible types in comparison expression
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
From an RCU perspective:
Reviewed-by: Paul E. McKenney <paulmck@linux.ibm.com>
> ---
> include/linux/rcuwait.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/rcuwait.h b/include/linux/rcuwait.h
> index 90bfa3279a01..563290fc194f 100644
> --- a/include/linux/rcuwait.h
> +++ b/include/linux/rcuwait.h
> @@ -18,7 +18,7 @@
> * awoken.
> */
> struct rcuwait {
> - struct task_struct *task;
> + struct task_struct __rcu *task;
> };
>
> #define __RCUWAIT_INITIALIZER(name) \
> --
> 2.21.0.rc0.258.g878e2cd30e-goog
>
^ permalink raw reply
* Re: [PATCH v2 4/6] sched_domain: Annotate RCU pointers properly
From: Paul E. McKenney @ 2019-02-25 21:11 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ingo Molnar,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, kernel-hardening,
kernel-team, Kirill Tkhai, Lai Jiangshan, Martin KaFai Lau,
Mathieu Desnoyers, netdev, Peter Zijlstra, Quentin Perret, rcu,
Song Liu, Steven Rostedt, Vincent Guittot, xdp-newbies,
Yonghong Song
In-Reply-To: <20190223063434.6793-5-joel@joelfernandes.org>
On Sat, Feb 23, 2019 at 01:34:32AM -0500, Joel Fernandes (Google) wrote:
> The scheduler uses RCU API in various places to access sched_domain
> pointers. These cause sparse errors as below.
>
> Many new errors show up because of an annotation check I added to
> rcu_assign_pointer(). Let us annotate the pointers correctly which also
> will help sparse catch any potential future bugs.
>
> This fixes the following sparse errors:
>
> rt.c:1681:9: error: incompatible types in comparison expression
> deadline.c:1904:9: error: incompatible types in comparison expression
> core.c:519:9: error: incompatible types in comparison expression
> core.c:1634:17: error: incompatible types in comparison expression
> fair.c:6193:14: error: incompatible types in comparison expression
> fair.c:9883:22: error: incompatible types in comparison expression
> fair.c:9897:9: error: incompatible types in comparison expression
> sched.h:1287:9: error: incompatible types in comparison expression
> topology.c:612:9: error: incompatible types in comparison expression
> topology.c:615:9: error: incompatible types in comparison expression
> sched.h:1300:9: error: incompatible types in comparison expression
> topology.c:618:9: error: incompatible types in comparison expression
> sched.h:1287:9: error: incompatible types in comparison expression
> topology.c:621:9: error: incompatible types in comparison expression
> sched.h:1300:9: error: incompatible types in comparison expression
> topology.c:624:9: error: incompatible types in comparison expression
> topology.c:671:9: error: incompatible types in comparison expression
> stats.c:45:17: error: incompatible types in comparison expression
> fair.c:5998:15: error: incompatible types in comparison expression
> fair.c:5989:15: error: incompatible types in comparison expression
> fair.c:5998:15: error: incompatible types in comparison expression
> fair.c:5989:15: error: incompatible types in comparison expression
> fair.c:6120:19: error: incompatible types in comparison expression
> fair.c:6506:14: error: incompatible types in comparison expression
> fair.c:6515:14: error: incompatible types in comparison expression
> fair.c:6623:9: error: incompatible types in comparison expression
> fair.c:5970:17: error: incompatible types in comparison expression
> fair.c:8642:21: error: incompatible types in comparison expression
> fair.c:9253:9: error: incompatible types in comparison expression
> fair.c:9331:9: error: incompatible types in comparison expression
> fair.c:9519:15: error: incompatible types in comparison expression
> fair.c:9533:14: error: incompatible types in comparison expression
> fair.c:9542:14: error: incompatible types in comparison expression
> fair.c:9567:14: error: incompatible types in comparison expression
> fair.c:9597:14: error: incompatible types in comparison expression
> fair.c:9421:16: error: incompatible types in comparison expression
> fair.c:9421:16: error: incompatible types in comparison expression
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
From an RCU perspective:
Reviewed-by: Paul E. McKenney <paulmck@linux.ibm.com>
> ---
> include/linux/sched/topology.h | 4 ++--
> kernel/sched/sched.h | 14 +++++++-------
> kernel/sched/topology.c | 10 +++++-----
> 3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
> index c31d3a47a47c..4819c9e01e42 100644
> --- a/include/linux/sched/topology.h
> +++ b/include/linux/sched/topology.h
> @@ -76,8 +76,8 @@ struct sched_domain_shared {
>
> struct sched_domain {
> /* These fields must be setup */
> - struct sched_domain *parent; /* top domain must be null terminated */
> - struct sched_domain *child; /* bottom domain must be null terminated */
> + struct sched_domain __rcu *parent; /* top domain must be null terminated */
> + struct sched_domain __rcu *child; /* bottom domain must be null terminated */
> struct sched_group *groups; /* the balancing groups of the domain */
> unsigned long min_interval; /* Minimum balance interval ms */
> unsigned long max_interval; /* Maximum balance interval ms */
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 2ab545d40381..ca6a79f57e7a 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -866,8 +866,8 @@ struct rq {
> atomic_t nr_iowait;
>
> #ifdef CONFIG_SMP
> - struct root_domain *rd;
> - struct sched_domain *sd;
> + struct root_domain *rd;
> + struct sched_domain __rcu *sd;
>
> unsigned long cpu_capacity;
> unsigned long cpu_capacity_orig;
> @@ -1305,13 +1305,13 @@ static inline struct sched_domain *lowest_flag_domain(int cpu, int flag)
> return sd;
> }
>
> -DECLARE_PER_CPU(struct sched_domain *, sd_llc);
> +DECLARE_PER_CPU(struct sched_domain __rcu *, sd_llc);
> DECLARE_PER_CPU(int, sd_llc_size);
> DECLARE_PER_CPU(int, sd_llc_id);
> -DECLARE_PER_CPU(struct sched_domain_shared *, sd_llc_shared);
> -DECLARE_PER_CPU(struct sched_domain *, sd_numa);
> -DECLARE_PER_CPU(struct sched_domain *, sd_asym_packing);
> -DECLARE_PER_CPU(struct sched_domain *, sd_asym_cpucapacity);
> +DECLARE_PER_CPU(struct sched_domain_shared __rcu *, sd_llc_shared);
> +DECLARE_PER_CPU(struct sched_domain __rcu *, sd_numa);
> +DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
> +DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
> extern struct static_key_false sched_asym_cpucapacity;
>
> struct sched_group_capacity {
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 3f35ba1d8fde..0844ee757dad 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -586,13 +586,13 @@ static void destroy_sched_domains(struct sched_domain *sd)
> * the cpumask of the domain), this allows us to quickly tell if
> * two CPUs are in the same cache domain, see cpus_share_cache().
> */
> -DEFINE_PER_CPU(struct sched_domain *, sd_llc);
> +DEFINE_PER_CPU(struct sched_domain __rcu *, sd_llc);
> DEFINE_PER_CPU(int, sd_llc_size);
> DEFINE_PER_CPU(int, sd_llc_id);
> -DEFINE_PER_CPU(struct sched_domain_shared *, sd_llc_shared);
> -DEFINE_PER_CPU(struct sched_domain *, sd_numa);
> -DEFINE_PER_CPU(struct sched_domain *, sd_asym_packing);
> -DEFINE_PER_CPU(struct sched_domain *, sd_asym_cpucapacity);
> +DEFINE_PER_CPU(struct sched_domain_shared __rcu *, sd_llc_shared);
> +DEFINE_PER_CPU(struct sched_domain __rcu *, sd_numa);
> +DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
> +DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
> DEFINE_STATIC_KEY_FALSE(sched_asym_cpucapacity);
>
> static void update_top_cache_domain(int cpu)
> --
> 2.21.0.rc0.258.g878e2cd30e-goog
>
^ permalink raw reply
* Re: [PATCH v2 3/6] sched/cpufreq: Annotate cpufreq_update_util_data pointer with __rcu
From: Paul E. McKenney @ 2019-02-25 21:10 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ingo Molnar,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, kernel-hardening,
kernel-team, Kirill Tkhai, Lai Jiangshan, Martin KaFai Lau,
Mathieu Desnoyers, netdev, Peter Zijlstra, Quentin Perret, rcu,
Song Liu, Steven Rostedt, Vincent Guittot, xdp-newbies,
Yonghong Song
In-Reply-To: <20190223063434.6793-4-joel@joelfernandes.org>
On Sat, Feb 23, 2019 at 01:34:31AM -0500, Joel Fernandes (Google) wrote:
> Recently I added an RCU annotation check to rcu_assign_pointer(). All
> pointers assigned to RCU protected data are to be annotated with __rcu
> inorder to be able to use rcu_assign_pointer() similar to checks in
> other RCU APIs.
>
> This resulted in a sparse error: kernel//sched/cpufreq.c:41:9: sparse:
> error: incompatible types in comparison expression (different address
> spaces)
>
> Fix this by annotating cpufreq_update_util_data pointer with __rcu. This
> will also help sparse catch any future RCU misuage bugs.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
From an RCU perspective:
Reviewed-by: Paul E. McKenney <paulmck@linux.ibm.com>
> ---
> kernel/sched/cpufreq.c | 2 +-
> kernel/sched/sched.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/cpufreq.c b/kernel/sched/cpufreq.c
> index 22bd8980f32f..e316ee7bb2e5 100644
> --- a/kernel/sched/cpufreq.c
> +++ b/kernel/sched/cpufreq.c
> @@ -7,7 +7,7 @@
> */
> #include "sched.h"
>
> -DEFINE_PER_CPU(struct update_util_data *, cpufreq_update_util_data);
> +DEFINE_PER_CPU(struct update_util_data __rcu *, cpufreq_update_util_data);
>
> /**
> * cpufreq_add_update_util_hook - Populate the CPU's update_util_data pointer.
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index d04530bf251f..2ab545d40381 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -2166,7 +2166,7 @@ static inline u64 irq_time_read(int cpu)
> #endif /* CONFIG_IRQ_TIME_ACCOUNTING */
>
> #ifdef CONFIG_CPU_FREQ
> -DECLARE_PER_CPU(struct update_util_data *, cpufreq_update_util_data);
> +DECLARE_PER_CPU(struct update_util_data __rcu *, cpufreq_update_util_data);
>
> /**
> * cpufreq_update_util - Take a note about CPU utilization changes.
> --
> 2.21.0.rc0.258.g878e2cd30e-goog
>
^ permalink raw reply
* Re: [PATCH v2 2/6] ixgbe: Fix incorrect RCU API usage
From: Paul E. McKenney @ 2019-02-25 21:09 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ingo Molnar,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, kernel-hardening,
kernel-team, Kirill Tkhai, Lai Jiangshan, Martin KaFai Lau,
Mathieu Desnoyers, netdev, Peter Zijlstra, Quentin Perret, rcu,
Song Liu, Steven Rostedt, Vincent Guittot, xdp-newbies,
Yonghong Song
In-Reply-To: <20190223063434.6793-3-joel@joelfernandes.org>
On Sat, Feb 23, 2019 at 01:34:30AM -0500, Joel Fernandes (Google) wrote:
> Recently, I added an RCU annotation check in rcu_assign_pointer. This
> caused a sparse error to be reported by the ixgbe driver.
>
> Further looking, it seems the adapter->xdp_prog pointer is not annotated
> with __rcu. Annonating it fixed the error, but caused a bunch of other
> warnings.
>
> This patch tries to fix all warnings by using RCU API properly. This
> makes sense to do because not using RCU properly can result in various
> hard to find bugs. This is a best effort fix and is only build tested.
> The sparse errors and warnings go away with the change. I request
> maintainers / developers in this area to review / test it properly.
>
> The sparse error fixed is:
> ixgbe_main.c:10256:25: error: incompatible types in comparison expression
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
From an RCU perspective:
Reviewed-by: Paul E. McKenney <paulmck@linux.ibm.com>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 4 ++--
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 15 ++++++++++-----
> 2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index 08d85e336bd4..3b14daf27516 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -311,7 +311,7 @@ struct ixgbe_ring {
> struct ixgbe_ring *next; /* pointer to next ring in q_vector */
> struct ixgbe_q_vector *q_vector; /* backpointer to host q_vector */
> struct net_device *netdev; /* netdev ring belongs to */
> - struct bpf_prog *xdp_prog;
> + struct bpf_prog __rcu *xdp_prog;
> struct device *dev; /* device for DMA mapping */
> void *desc; /* descriptor ring memory */
> union {
> @@ -560,7 +560,7 @@ struct ixgbe_adapter {
> unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
> /* OS defined structs */
> struct net_device *netdev;
> - struct bpf_prog *xdp_prog;
> + struct bpf_prog __rcu *xdp_prog;
> struct pci_dev *pdev;
> struct mii_bus *mii_bus;
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index daff8183534b..408a312aa6ba 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -2199,7 +2199,7 @@ static struct sk_buff *ixgbe_run_xdp(struct ixgbe_adapter *adapter,
> u32 act;
>
> rcu_read_lock();
> - xdp_prog = READ_ONCE(rx_ring->xdp_prog);
> + xdp_prog = rcu_dereference(rx_ring->xdp_prog);
>
> if (!xdp_prog)
> goto xdp_out;
> @@ -6547,7 +6547,7 @@ int ixgbe_setup_rx_resources(struct ixgbe_adapter *adapter,
> rx_ring->queue_index) < 0)
> goto err;
>
> - rx_ring->xdp_prog = adapter->xdp_prog;
> + rcu_assign_pointer(rx_ring->xdp_prog, adapter->xdp_prog);
>
> return 0;
> err:
> @@ -10246,7 +10246,8 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
> if (nr_cpu_ids > MAX_XDP_QUEUES)
> return -ENOMEM;
>
> - old_prog = xchg(&adapter->xdp_prog, prog);
> + old_prog = rcu_access_pointer(adapter->xdp_prog);
> + rcu_assign_pointer(adapter->xdp_prog, prog);
>
> /* If transitioning XDP modes reconfigure rings */
> if (!!prog != !!old_prog) {
> @@ -10271,13 +10272,17 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
> static int ixgbe_xdp(struct net_device *dev, struct netdev_bpf *xdp)
> {
> struct ixgbe_adapter *adapter = netdev_priv(dev);
> + struct bpf_prog *prog;
>
> switch (xdp->command) {
> case XDP_SETUP_PROG:
> return ixgbe_xdp_setup(dev, xdp->prog);
> case XDP_QUERY_PROG:
> - xdp->prog_id = adapter->xdp_prog ?
> - adapter->xdp_prog->aux->id : 0;
> + rcu_read_lock();
> + prog = rcu_dereference(adapter->xdp_prog);
> + xdp->prog_id = prog ? prog->aux->id : 0;
> + rcu_read_unlock();
> +
> return 0;
> case XDP_QUERY_XSK_UMEM:
> return ixgbe_xsk_umem_query(adapter, &xdp->xsk.umem,
> --
> 2.21.0.rc0.258.g878e2cd30e-goog
>
^ permalink raw reply
* Re: [PATCH v2 1/6] net: rtnetlink: Fix incorrect RCU API usage
From: Paul E. McKenney @ 2019-02-25 21:05 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ingo Molnar,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, kernel-hardening,
kernel-team, Kirill Tkhai, Lai Jiangshan, Martin KaFai Lau,
Mathieu Desnoyers, netdev, Peter Zijlstra, Quentin Perret, rcu,
Song Liu, Steven Rostedt, Vincent Guittot, xdp-newbies,
Yonghong Song
In-Reply-To: <20190223063434.6793-2-joel@joelfernandes.org>
On Sat, Feb 23, 2019 at 01:34:29AM -0500, Joel Fernandes (Google) wrote:
> rtnl_register_internal() and rtnl_unregister_all tries to directly
> dereference an RCU protected pointed outside RCU read side section.
> While this is Ok to do since a lock is held, let us use the correct
> API to avoid programmer bugs in the future.
>
> This also fixes sparse warnings arising from not using RCU API.
>
> net/core/rtnetlink.c:332:13: warning: incorrect type in assignment
> (different address spaces) net/core/rtnetlink.c:332:13: expected
> struct rtnl_link **tab net/core/rtnetlink.c:332:13: got struct
> rtnl_link *[noderef] <asn:4>*<noident>
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
From an RCU perspective:
Reviewed-by: Paul E. McKenney <paulmck@linux.ibm.com>
> ---
> net/core/rtnetlink.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 5ea1bed08ede..98be4b4818a9 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -188,7 +188,7 @@ static int rtnl_register_internal(struct module *owner,
> msgindex = rtm_msgindex(msgtype);
>
> rtnl_lock();
> - tab = rtnl_msg_handlers[protocol];
> + tab = rtnl_dereference(rtnl_msg_handlers[protocol]);
> if (tab == NULL) {
> tab = kcalloc(RTM_NR_MSGTYPES, sizeof(void *), GFP_KERNEL);
> if (!tab)
> @@ -329,7 +329,7 @@ void rtnl_unregister_all(int protocol)
> BUG_ON(protocol < 0 || protocol > RTNL_FAMILY_MAX);
>
> rtnl_lock();
> - tab = rtnl_msg_handlers[protocol];
> + tab = rtnl_dereference(rtnl_msg_handlers[protocol]);
> if (!tab) {
> rtnl_unlock();
> return;
> --
> 2.21.0.rc0.258.g878e2cd30e-goog
>
^ permalink raw reply
* Re: [PATCH 2/2] bpf: initial implementation for BPF_PROG_TYPE_GTRACE
From: kbuild test robot @ 2019-02-25 20:57 UTC (permalink / raw)
To: Kris Van Hees; +Cc: kbuild-all, netdev
In-Reply-To: <201902251554.x1PFsj2G026501@aserv0121.oracle.com>
[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]
Hi Kris,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
[also build test ERROR on next-20190225]
[cannot apply to v5.0-rc8]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Kris-Van-Hees/bpf-context-casting-for-tail-call-and-gtrace-prog-type/20190226-034827
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: riscv-defconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 8.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=riscv
All errors (new ones prefixed by >>):
kernel/bpf/syscall.o: In function `.LANCHOR1':
>> syscall.c:(.rodata+0x3d8): undefined reference to `gtrace_prog_ops'
kernel/bpf/verifier.o: In function `.LANCHOR1':
>> verifier.c:(.rodata+0x4c0): undefined reference to `gtrace_verifier_ops'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 16821 bytes --]
^ permalink raw reply
* Re: [RFC bpf-next 0/7] net: flow_dissector: trigger BPF hook when called from eth_get_headlen
From: Stanislav Fomichev @ 2019-02-25 20:33 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Willem de Bruijn, Stanislav Fomichev, Network Development,
David Miller, Alexei Starovoitov, Daniel Borkmann, simon.horman,
Willem de Bruijn
In-Reply-To: <20190214173542.GA20651@mini-arch>
On 02/14, Stanislav Fomichev wrote:
> On 02/13, Alexei Starovoitov wrote:
> > On Wed, Feb 13, 2019 at 09:57:25PM -0800, Stanislav Fomichev wrote:
> > >
> > > > That 'stuck with __sk_buff' is what bothers me.
> > > I might have use the wrong word here. I don't think there is another
> > > option to be honest. Using __sk_buff makes flow dissector programs work
> > > with fragmented packets;
> >
> > good point. indeed real skb is essential.
> >
> > > > It's an indication that api wasn't thought through if first thing
> > > > it needs is this fake skb hack.
> > > > If bpf_flow.c is a realistic example of such flow dissector prog
> > > > it means that real skb fields are accessed.
> > > > In particular skb->vlan_proto, skb->protocol.
> > > I do manually set skb->protocol to eth->h_proto in my proposal. This is later
> > > correctly handled by bpf_flow.c: parse_eth_proto() is called on skb->protocol
> > > and we correctly handle bpf_htons(ETH_P_8021Q) there. So existing
> > > bpf_flow.c works as expected.
> > ...
> > > The goal of this patch series was to essentially make this skb/no-skb
> > > context transparent to the bpf_flow.c (i.e. no changes from the user
> > > flow programs). Adding another flow dissector for eth_get_headlen case
> > > also seems as a no go.
> >
> > The problem with this thinking is assumption that bpf_flow.c is the only program.
> I agree, it's a bad assumption, but it is sort of a reference implementation,
> I don't expect other users to do something wildly different. Hopefully :-)
>
> > Since ctx of flow_dissector prog type is 'struct __sk_buff'
> > all fields should be valid or the verifier has to reject access
> > to fields that were not set.
> > You cannot "manually set skb->protocol to eth->h_proto" in fake skb
> > and ignore the rest.
> Ugh, I did expect that we only allow a minimal set of __sk_buff fields
> to be allowed from the flow dissector program type, but that's not the
> case. We explicitly prohibit access only to
> family/ips/ports/tc_classid/tstamp/wire_len, everything else is readable :-/
> Any idea why?
> Stuff like ingress_ifindex/ifindex/hash/mark/queue_mapping, does flow dissector
> programs really need to know that?
>
> For the most part, using zero-initialized fake skb looks fine, except:
> * infindex, where we do skb->dev->ifndex (skb->dev is NULL)
> * gso_segs, where we do skb_shinfo(skb)->gso_segs (we are missing
> shinfo)
>
> So there is indeed a couple of problems.
>
> How do you feel about tightening down the access to sk_buff fields from
> the flow dissector program type? That is an API change, but I don't see why
> existing users should use those fields. Let's allow access only to
> len/data/data_end, protocol, vlan_{present,tci,proto}, cb, flow_keys,
> that should be enough to dissect the packet (I also looked at C-based
> implementation, it doesn't use anything besides that).
> We can always rollback if somebody complains about.
To revive the conversation, here is what I was thinking about (whitelist
the skb fields, not blacklist them):
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6591,11 +6591,14 @@ static bool flow_dissector_is_valid_access(int off, int size,
case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
info->reg_type = PTR_TO_FLOW_KEYS;
break;
- case bpf_ctx_range(struct __sk_buff, tc_classid):
- case bpf_ctx_range(struct __sk_buff, data_meta):
- case bpf_ctx_range_till(struct __sk_buff, family, local_port):
- case bpf_ctx_range(struct __sk_buff, tstamp):
- case bpf_ctx_range(struct __sk_buff, wire_len):
+ case bpf_ctx_range(struct __sk_buff, len):
+ case bpf_ctx_range(struct __sk_buff, protocol):
+ case bpf_ctx_range(struct __sk_buff, vlan_present):
+ case bpf_ctx_range(struct __sk_buff, vlan_tci):
+ case bpf_ctx_range(struct __sk_buff, vlan_proto):
+ case bpf_ctx_range_till(struct __sk_buff, cb[0], cb[4]):
+ break;
+ default:
return false;
}
What do you think?
^ permalink raw reply
* Re: [PATCH net] ipvs: get sctphdr by sctphoff in sctp_csum_check
From: Julian Anastasov @ 2019-02-25 20:19 UTC (permalink / raw)
To: Xin Long
Cc: network dev, netfilter-devel, Marcelo Ricardo Leitner,
Neil Horman, pablo, Simon Horman, lvs-devel
In-Reply-To: <cd1c829cfd9ee916e57c51f6879947f0f88c1eb1.1551094063.git.lucien.xin@gmail.com>
Hello,
On Mon, 25 Feb 2019, Xin Long wrote:
> sctp_csum_check() is called by sctp_s/dnat_handler() where it calls
> skb_make_writable() to ensure sctphdr to be linearized.
>
> So there's no need to get sctphdr by calling skb_header_pointer()
> in sctp_csum_check().
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Looks good to me, thanks!
Acked-by: Julian Anastasov <ja@ssi.bg>
I guess, it is for the nf-next/net-next tree because it just
eliminates a duplicate check.
> ---
> net/netfilter/ipvs/ip_vs_proto_sctp.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> index b0cd7d0..0ecf241 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> @@ -183,7 +183,7 @@ static int
> sctp_csum_check(int af, struct sk_buff *skb, struct ip_vs_protocol *pp)
> {
> unsigned int sctphoff;
> - struct sctphdr *sh, _sctph;
> + struct sctphdr *sh;
> __le32 cmp, val;
>
> #ifdef CONFIG_IP_VS_IPV6
> @@ -193,10 +193,7 @@ sctp_csum_check(int af, struct sk_buff *skb, struct ip_vs_protocol *pp)
> #endif
> sctphoff = ip_hdrlen(skb);
>
> - sh = skb_header_pointer(skb, sctphoff, sizeof(_sctph), &_sctph);
> - if (sh == NULL)
> - return 0;
> -
> + sh = (struct sctphdr *)(skb->data + sctphoff);
> cmp = sh->checksum;
> val = sctp_compute_cksum(skb, sctphoff);
>
> --
> 2.1.0
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH] tun: remove unnecessary memory barrier
From: Eric Dumazet @ 2019-02-25 20:15 UTC (permalink / raw)
To: Timur Celik, David Miller; +Cc: netdev, linux-kernel
In-Reply-To: <20190225201313.GA30005@timur-laptop.fritz.box>
On 02/25/2019 12:13 PM, Timur Celik wrote:
> Replace set_current_state with __set_current_state since no memory
> barrier is needed at this point.
>
> Signed-off-by: Timur Celik <mail@timurcelik.de>
> ---
> drivers/net/tun.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
SGTM, thanks.
Reviewed-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* [PATCH] tun: remove unnecessary memory barrier
From: Timur Celik @ 2019-02-25 20:13 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, netdev, linux-kernel
In-Reply-To: <20190225.102548.247362521843809607.davem@davemloft.net>
Replace set_current_state with __set_current_state since no memory
barrier is needed at this point.
Signed-off-by: Timur Celik <mail@timurcelik.de>
---
drivers/net/tun.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index d291762b9e9d..53f4f37b0ffd 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2185,7 +2185,7 @@ static void *tun_ring_recv(struct tun_file *tfile, int noblock, int *err)
schedule();
}
- set_current_state(TASK_RUNNING);
+ __set_current_state(TASK_RUNNING);
remove_wait_queue(&tfile->wq.wait, &wait);
out:
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] tun: fix blocking read
From: Timur Celik @ 2019-02-25 20:12 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, netdev, linux-kernel
In-Reply-To: <20190225.102548.247362521843809607.davem@davemloft.net>
On Mon, Feb 25, 2019 at 10:25:48AM -0800, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 25 Feb 2019 07:53:08 -0800
>
>>
>>
>> On 02/24/2019 10:12 PM, David Miller wrote:
>>> From: Timur Celik <mail@timurcelik.de>
>>> Date: Sat, 23 Feb 2019 12:53:13 +0100
>>>
>>>> This patch moves setting of the current state into the loop. Otherwise
>>>> the task may end up in a busy wait loop if none of the break conditions
>>>> are met.
>>>>
>>>> Signed-off-by: Timur Celik <mail@timurcelik.de>
>>>
>>> Applied and queued up for -stable, thanks.
>>>
>>
>> First part of the patch was matching the changelog, however
>> the second change was not really needed, or should have used
>>
>> __set_current_state(TASK_RUNNING);
>
> Timur, please followup.
Thanks for taking a look at this. Eric is right of course.
^ permalink raw reply
* Re: [PATCH 1/2] bpf: context casting for tail call
From: kbuild test robot @ 2019-02-25 20:10 UTC (permalink / raw)
To: Kris Van Hees; +Cc: kbuild-all, netdev
In-Reply-To: <201902251554.x1PFsWmr017326@userv0122.oracle.com>
[-- Attachment #1: Type: text/plain, Size: 8559 bytes --]
Hi Kris,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
[also build test WARNING on v5.0-rc8 next-20190225]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Kris-Van-Hees/bpf-context-casting-for-tail-call-and-gtrace-prog-type/20190226-034827
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=xtensa
All warnings (new ones prefixed by >>):
kernel//bpf/core.c: In function '___bpf_prog_run':
>> kernel//bpf/core.c:1452:13: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
BPF_R1 = (u64) ctx;
^
vim +1452 kernel//bpf/core.c
1290
1291 select_insn:
1292 goto *jumptable[insn->code];
1293
1294 /* ALU */
1295 #define ALU(OPCODE, OP) \
1296 ALU64_##OPCODE##_X: \
1297 DST = DST OP SRC; \
1298 CONT; \
1299 ALU_##OPCODE##_X: \
1300 DST = (u32) DST OP (u32) SRC; \
1301 CONT; \
1302 ALU64_##OPCODE##_K: \
1303 DST = DST OP IMM; \
1304 CONT; \
1305 ALU_##OPCODE##_K: \
1306 DST = (u32) DST OP (u32) IMM; \
1307 CONT;
1308
1309 ALU(ADD, +)
1310 ALU(SUB, -)
1311 ALU(AND, &)
1312 ALU(OR, |)
1313 ALU(LSH, <<)
1314 ALU(RSH, >>)
1315 ALU(XOR, ^)
1316 ALU(MUL, *)
1317 #undef ALU
1318 ALU_NEG:
1319 DST = (u32) -DST;
1320 CONT;
1321 ALU64_NEG:
1322 DST = -DST;
1323 CONT;
1324 ALU_MOV_X:
1325 DST = (u32) SRC;
1326 CONT;
1327 ALU_MOV_K:
1328 DST = (u32) IMM;
1329 CONT;
1330 ALU64_MOV_X:
1331 DST = SRC;
1332 CONT;
1333 ALU64_MOV_K:
1334 DST = IMM;
1335 CONT;
1336 LD_IMM_DW:
1337 DST = (u64) (u32) insn[0].imm | ((u64) (u32) insn[1].imm) << 32;
1338 insn++;
1339 CONT;
1340 ALU_ARSH_X:
1341 DST = (u64) (u32) ((*(s32 *) &DST) >> SRC);
1342 CONT;
1343 ALU_ARSH_K:
1344 DST = (u64) (u32) ((*(s32 *) &DST) >> IMM);
1345 CONT;
1346 ALU64_ARSH_X:
1347 (*(s64 *) &DST) >>= SRC;
1348 CONT;
1349 ALU64_ARSH_K:
1350 (*(s64 *) &DST) >>= IMM;
1351 CONT;
1352 ALU64_MOD_X:
1353 div64_u64_rem(DST, SRC, &AX);
1354 DST = AX;
1355 CONT;
1356 ALU_MOD_X:
1357 AX = (u32) DST;
1358 DST = do_div(AX, (u32) SRC);
1359 CONT;
1360 ALU64_MOD_K:
1361 div64_u64_rem(DST, IMM, &AX);
1362 DST = AX;
1363 CONT;
1364 ALU_MOD_K:
1365 AX = (u32) DST;
1366 DST = do_div(AX, (u32) IMM);
1367 CONT;
1368 ALU64_DIV_X:
1369 DST = div64_u64(DST, SRC);
1370 CONT;
1371 ALU_DIV_X:
1372 AX = (u32) DST;
1373 do_div(AX, (u32) SRC);
1374 DST = (u32) AX;
1375 CONT;
1376 ALU64_DIV_K:
1377 DST = div64_u64(DST, IMM);
1378 CONT;
1379 ALU_DIV_K:
1380 AX = (u32) DST;
1381 do_div(AX, (u32) IMM);
1382 DST = (u32) AX;
1383 CONT;
1384 ALU_END_TO_BE:
1385 switch (IMM) {
1386 case 16:
1387 DST = (__force u16) cpu_to_be16(DST);
1388 break;
1389 case 32:
1390 DST = (__force u32) cpu_to_be32(DST);
1391 break;
1392 case 64:
1393 DST = (__force u64) cpu_to_be64(DST);
1394 break;
1395 }
1396 CONT;
1397 ALU_END_TO_LE:
1398 switch (IMM) {
1399 case 16:
1400 DST = (__force u16) cpu_to_le16(DST);
1401 break;
1402 case 32:
1403 DST = (__force u32) cpu_to_le32(DST);
1404 break;
1405 case 64:
1406 DST = (__force u64) cpu_to_le64(DST);
1407 break;
1408 }
1409 CONT;
1410
1411 /* CALL */
1412 JMP_CALL:
1413 /* Function call scratches BPF_R1-BPF_R5 registers,
1414 * preserves BPF_R6-BPF_R9, and stores return value
1415 * into BPF_R0.
1416 */
1417 BPF_R0 = (__bpf_call_base + insn->imm)(BPF_R1, BPF_R2, BPF_R3,
1418 BPF_R4, BPF_R5);
1419 CONT;
1420
1421 JMP_CALL_ARGS:
1422 BPF_R0 = (__bpf_call_base_args + insn->imm)(BPF_R1, BPF_R2,
1423 BPF_R3, BPF_R4,
1424 BPF_R5,
1425 insn + insn->off + 1);
1426 CONT;
1427
1428 JMP_TAIL_CALL: {
1429 void *ctx = (void *) (unsigned long) BPF_R1;
1430 struct bpf_map *map = (struct bpf_map *) (unsigned long) BPF_R2;
1431 struct bpf_array *array = container_of(map, struct bpf_array, map);
1432 struct bpf_prog *prog;
1433 u32 index = BPF_R3;
1434 u32 type = BPF_R4;
1435
1436 if (unlikely(index >= array->map.max_entries))
1437 goto out;
1438 if (unlikely(tail_call_cnt > MAX_TAIL_CALL_CNT))
1439 goto out;
1440
1441 tail_call_cnt++;
1442
1443 prog = READ_ONCE(array->ptrs[index]);
1444 if (!prog)
1445 goto out;
1446 if (prog->aux->ops->cast_context) {
1447 ctx = prog->aux->ops->cast_context(type, prog->type,
1448 ctx);
1449 if (!ctx)
1450 goto out;
1451
> 1452 BPF_R1 = (u64) ctx;
1453 }
1454
1455 /* ARG1 at this point is guaranteed to point to CTX from
1456 * the verifier side due to the fact that the tail call is
1457 * handeled like a helper, that is, bpf_tail_call_proto,
1458 * where arg1_type is ARG_PTR_TO_CTX.
1459 */
1460 insn = prog->insnsi;
1461 goto select_insn;
1462 out:
1463 CONT;
1464 }
1465 JMP_JA:
1466 insn += insn->off;
1467 CONT;
1468 JMP_EXIT:
1469 return BPF_R0;
1470 /* JMP */
1471 #define COND_JMP(SIGN, OPCODE, CMP_OP) \
1472 JMP_##OPCODE##_X: \
1473 if ((SIGN##64) DST CMP_OP (SIGN##64) SRC) { \
1474 insn += insn->off; \
1475 CONT_JMP; \
1476 } \
1477 CONT; \
1478 JMP32_##OPCODE##_X: \
1479 if ((SIGN##32) DST CMP_OP (SIGN##32) SRC) { \
1480 insn += insn->off; \
1481 CONT_JMP; \
1482 } \
1483 CONT; \
1484 JMP_##OPCODE##_K: \
1485 if ((SIGN##64) DST CMP_OP (SIGN##64) IMM) { \
1486 insn += insn->off; \
1487 CONT_JMP; \
1488 } \
1489 CONT; \
1490 JMP32_##OPCODE##_K: \
1491 if ((SIGN##32) DST CMP_OP (SIGN##32) IMM) { \
1492 insn += insn->off; \
1493 CONT_JMP; \
1494 } \
1495 CONT;
1496 COND_JMP(u, JEQ, ==)
1497 COND_JMP(u, JNE, !=)
1498 COND_JMP(u, JGT, >)
1499 COND_JMP(u, JLT, <)
1500 COND_JMP(u, JGE, >=)
1501 COND_JMP(u, JLE, <=)
1502 COND_JMP(u, JSET, &)
1503 COND_JMP(s, JSGT, >)
1504 COND_JMP(s, JSLT, <)
1505 COND_JMP(s, JSGE, >=)
1506 COND_JMP(s, JSLE, <=)
1507 #undef COND_JMP
1508 /* STX and ST and LDX*/
1509 #define LDST(SIZEOP, SIZE) \
1510 STX_MEM_##SIZEOP: \
1511 *(SIZE *)(unsigned long) (DST + insn->off) = SRC; \
1512 CONT; \
1513 ST_MEM_##SIZEOP: \
1514 *(SIZE *)(unsigned long) (DST + insn->off) = IMM; \
1515 CONT; \
1516 LDX_MEM_##SIZEOP: \
1517 DST = *(SIZE *)(unsigned long) (SRC + insn->off); \
1518 CONT;
1519
1520 LDST(B, u8)
1521 LDST(H, u16)
1522 LDST(W, u32)
1523 LDST(DW, u64)
1524 #undef LDST
1525 STX_XADD_W: /* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
1526 atomic_add((u32) SRC, (atomic_t *)(unsigned long)
1527 (DST + insn->off));
1528 CONT;
1529 STX_XADD_DW: /* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
1530 atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
1531 (DST + insn->off));
1532 CONT;
1533
1534 default_label:
1535 /* If we ever reach this, we have a bug somewhere. Die hard here
1536 * instead of just returning 0; we could be somewhere in a subprog,
1537 * so execution could continue otherwise which we do /not/ want.
1538 *
1539 * Note, verifier whitelists all opcodes in bpf_opcode_in_insntable().
1540 */
1541 pr_warn("BPF interpreter: unknown opcode %02x\n", insn->code);
1542 BUG_ON(1);
1543 return 0;
1544 }
1545 STACK_FRAME_NON_STANDARD(___bpf_prog_run); /* jump table */
1546
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 56355 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] bpf: context casting for tail call
From: kbuild test robot @ 2019-02-25 20:10 UTC (permalink / raw)
To: Kris Van Hees; +Cc: kbuild-all, netdev
In-Reply-To: <201902251554.x1PFsWmr017326@userv0122.oracle.com>
[-- Attachment #1: Type: text/plain, Size: 1718 bytes --]
Hi Kris,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
[also build test ERROR on v5.0-rc8 next-20190225]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Kris-Van-Hees/bpf-context-casting-for-tail-call-and-gtrace-prog-type/20190226-034827
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-x016-201908 (attached as .config)
compiler: gcc-8 (Debian 8.2.0-20) 8.2.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
kernel/bpf/core.c: In function 'bpf_prog_array_can_cast':
>> kernel/bpf/core.c:1657:8: error: 'bpf_prog_types' undeclared (first use in this function); did you mean 'bpf_prog_type'?
ops = bpf_prog_types[array->owner_prog_type];
^~~~~~~~~~~~~~
bpf_prog_type
kernel/bpf/core.c:1657:8: note: each undeclared identifier is reported only once for each function it appears in
vim +1657 kernel/bpf/core.c
1649
1650 bool bpf_prog_array_can_cast(struct bpf_array *array, const struct bpf_prog *fp)
1651 {
1652 const struct bpf_prog_ops *ops;
1653
1654 if (array->owner_jited != fp->jited)
1655 return false;
1656
> 1657 ops = bpf_prog_types[array->owner_prog_type];
1658 if (ops->can_cast)
1659 return ops->can_cast(fp->type, array->owner_prog_type);
1660
1661 return false;
1662 }
1663
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28326 bytes --]
^ permalink raw reply
* Re: [PATCH v2 bpf-next 2/4] bpf: expose program stats via bpf_prog_info
From: Andrii Nakryiko @ 2019-02-25 19:47 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, Daniel Borkmann, edumazet, netdev, bpf, Kernel Team
In-Reply-To: <20190223174422.663270-3-ast@kernel.org>
On Sat, Feb 23, 2019 at 9:44 AM Alexei Starovoitov <ast@kernel.org> wrote:
>
> Return bpf program runtime and runcnt via bpf_prog_info
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
> include/uapi/linux/bpf.h | 2 ++
> kernel/bpf/syscall.c | 5 +++++
> 2 files changed, 7 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index bcdd2474eee7..d2cb85d85b39 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2813,6 +2813,8 @@ struct bpf_prog_info {
> __u32 jited_line_info_rec_size;
> __u32 nr_prog_tags;
> __aligned_u64 prog_tags;
> + __u64 runtime;
> + __u64 runcnt;
> } __attribute__((aligned(8)));
>
> struct bpf_map_info {
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 9a8c990c5df4..3405e0a4cf77 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2152,6 +2152,7 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
> struct bpf_prog_info __user *uinfo = u64_to_user_ptr(attr->info.info);
> struct bpf_prog_info info = {};
> u32 info_len = attr->info.info_len;
> + struct bpf_prog_stats stats;
> char __user *uinsns;
> u32 ulen;
> int err;
> @@ -2191,6 +2192,10 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
> if (err)
> return err;
>
> + bpf_prog_get_stats(prog, &stats);
> + info.runtime = stats.nsecs;
> + info.runcnt = stats.cnt;
> +
lgtm, but see naming discussion in other thread.
Acked-by: Andrii Nakryiko <andriin@fb.com>
> if (!capable(CAP_SYS_ADMIN)) {
> info.jited_prog_len = 0;
> info.xlated_prog_len = 0;
> --
> 2.20.0
>
^ permalink raw reply
* Re: [PATCH net-next 7/8] net: switchdev: Replace port attr set SDO with a notification
From: Florian Fainelli @ 2019-02-25 19:47 UTC (permalink / raw)
To: Ido Schimmel
Cc: netdev@vger.kernel.org, David S. Miller, open list,
open list:STAGING SUBSYSTEM, moderated list:ETHERNET BRIDGE,
Jiri Pirko, andrew@lunn.ch, vivien.didelot@gmail.com
In-Reply-To: <20190225094940.GA15128@splinter>
On 2/25/19 1:49 AM, Ido Schimmel wrote:
> On Sun, Feb 24, 2019 at 08:47:27AM -0800, Florian Fainelli wrote:
>> Le 2/23/19 à 2:32 AM, Ido Schimmel a écrit :
>>> On Fri, Feb 22, 2019 at 03:59:25PM -0800, Florian Fainelli wrote:
>>>> - if (attr->flags & SWITCHDEV_F_NO_RECURSE)
>>>> + if (attr & SWITCHDEV_F_DEFER)
>>>> + rc = call_switchdev_blocking_notifiers(nt, dev,
>>>> + &attr_info.info, NULL);
>>>> + else
>>>> + rc = call_switchdev_notifiers(nt, dev, &attr_info.info, NULL);
>>>
>>> I don't believe this is needed. You're calling this function from
>>> switchdev_port_attr_set_now() which is always called from process
>>> context. switchdev_port_attr_set() takes care of that. Similar to
>>> switchdev_port_obj_add().
>>
>> Except for net/bridge/br_switchdev.c when we check the bridge port's
>> flags support with PRE_BRIDGE_FLAGS. In that case we are executing from
>> the caller (atomic) context and we can't defer otherwise that trumps the
>> whole idea of being able to do a quick check and return that to the
>> caller that we cannot support specific flags. How would you recommend
>> approaching that?
>
> In this case you can invoke call_switchdev_notifiers() directly from
> br_switchdev_set_port_flag(). Eventually switchdev_port_attr_set() will
> be gone and bridge code will invoke the notifiers directly.
That can be done, but it still requires the target driver (mlxsw,
ocelot, dsa, etc.) to support attribute notification from blocking and
non-blocking context. Are you fine with that?
--
Florian
^ permalink raw reply
* Re: INFO: task hung in rtnetlink_rcv_msg
From: Jesper Dangaard Brouer @ 2019-02-25 19:26 UTC (permalink / raw)
To: Joe Perches
Cc: Daniel Borkmann, Jakub Kicinski, syzbot, ast, christian, davem,
dsahern, hawk, idosch, john.fastabend, kafai, ktkhai,
linux-kernel, netdev, petrm, roopa, songliubraving,
syzkaller-bugs, xdp-newbies, yhs, brouer
In-Reply-To: <57807dca3bf554d0a07356ba1e53fae4d58cfb51.camel@perches.com>
On Mon, 25 Feb 2019 08:55:40 -0800
Joe Perches <joe@perches.com> wrote:
> On Mon, 2019-02-25 at 09:39 +0100, Jesper Dangaard Brouer wrote:
> > On Sat, 23 Feb 2019 01:47:00 +0100
> > Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > > 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
> >
> > I'm unsure how K: works, but you grep definitely selects some wrong files.
> >
> > I tried with "xdp_":
> > git grep --name-only xdp_
> >
> > That does catch all the driver that have XDP support, which is the
> > point of the exercise (to catch drivers).
> >
> > It does contain a couple of false-positives:
> > drivers/net/ethernet/neterion/vxge/vxge-traffic.c
> > drivers/thunderbolt/tb_msgs.h
> > drivers/thunderbolt/xdomain.c
> > sound/soc/codecs/rt5670.c
> >
> > Via the pattern '[^a-z]xdp_' I'm only left with the thunderbolt
> > false-positive, as it have a data struct's called tb_xdp_*.
>
> Then it's probably better to use
>
> K: \Wxdp_
>
> and if XDP_ is also to be matched, maybe
>
> K: (?i:\Wxdp_)
>
The last one seems to be the best at matching the relevant files :-)
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH v2 bpf-next 4/4] tools/bpftool: recognize bpf_prog_info runtime and runcnt
From: Andrii Nakryiko @ 2019-02-25 19:24 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, davem, edumazet, netdev, bpf, Kernel Team
In-Reply-To: <cf8b7ffc-22f9-a095-7614-2d191ba53522@iogearbox.net>
On Mon, Feb 25, 2019 at 3:17 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 02/23/2019 06:44 PM, Alexei Starovoitov wrote:
> > $ bpftool p s
> > 1: kprobe tag a56587d488d216c9 gpl runtime 79786 runcnt 8
> > loaded_at 2019-02-22T12:22:51-0800 uid 0
> > xlated 352B not jited memlock 4096B
> >
> > $ bpftool --json --pretty p s
> > [{
> > "id": 1,
> > "type": "kprobe",
> > "tag": "a56587d488d216c9",
> > "gpl_compatible": true,
> > "run_time_ns": 79786,
> > "run_cnt": 8,
> > "loaded_at": 1550866971,
> > "uid": 0,
> > "bytes_xlated": 352,
> > "jited": false,
> > "bytes_memlock": 4096
> > }
> > ]
> >
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > ---
> > tools/bpf/bpftool/Documentation/bpftool-prog.rst | 4 +++-
> > tools/bpf/bpftool/prog.c | 7 +++++++
> > 2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > index 12bc1e2d4b46..d8b46a8f17e0 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > @@ -171,7 +171,7 @@ EXAMPLES
> >
> > ::
> >
> > - 10: xdp name some_prog tag 005a3d2123620c8b gpl
> > + 10: xdp name some_prog tag 005a3d2123620c8b gpl runtime 81632 runcnt 10
> > loaded_at 2017-09-29T20:11:00+0000 uid 0
> > xlated 528B jited 370B memlock 4096B map_ids 10
> >
> > @@ -184,6 +184,8 @@ EXAMPLES
> > "type": "xdp",
> > "tag": "005a3d2123620c8b",
> > "gpl_compatible": true,
> > + "run_time_ns": 81632,
> > + "run_cnt": 10,
> > "loaded_at": 1506715860,
> > "uid": 0,
> > "bytes_xlated": 528,
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index db978c8d76a8..cdb6bd424340 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -214,6 +214,10 @@ static void print_prog_json(struct bpf_prog_info *info, int fd)
> > info->tag[4], info->tag[5], info->tag[6], info->tag[7]);
> >
> > jsonw_bool_field(json_wtr, "gpl_compatible", info->gpl_compatible);
> > + if (info->runtime) {
> > + jsonw_uint_field(json_wtr, "run_time_ns", info->runtime);
> > + jsonw_uint_field(json_wtr, "run_cnt", info->runcnt);
>
> Didn't Andrii mean to generally name these fields run_time_ns and run_cnt, so also
> uapi aside from just changing bpftool output (why keeping these names not the same)?
Yeah, I think consistency is important. This also applies to patch #1
(bpf: enable program stats), which exposes same stats through procfs.
Ideally all those names would be consistent (for obvious reasons). As
for _ns, I realize it's a bit longer and more verbose, but having time
units as part of a name is really great for users and doesn't require
extra documentation (and extra man lookups).
>
> > + }
> >
> > print_dev_json(info->ifindex, info->netns_dev, info->netns_ino);
> >
> > @@ -277,6 +281,9 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd)
> > fprint_hex(stdout, info->tag, BPF_TAG_SIZE, "");
> > print_dev_plain(info->ifindex, info->netns_dev, info->netns_ino);
> > printf("%s", info->gpl_compatible ? " gpl" : "");
> > + if (info->runtime)
> > + printf(" runtime %lld runcnt %lld",
> > + info->runtime, info->runcnt);
> > printf("\n");
> >
> > if (info->load_time) {
> >
>
^ 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