* Re: [PATCH v4 3/7] lib/hexdump.c: Optionally suppress lines of repeated bytes
From: kbuild test robot @ 2019-06-25 19:13 UTC (permalink / raw)
To: Alastair D'Silva
Cc: kbuild-all, alastair, linux-fbdev, Stanislaw Gruszka, Petr Mladek,
David Airlie, Joonas Lahtinen, dri-devel, devel, linux-scsi,
Jassi Brar, ath10k, intel-gfx, Dan Carpenter, Jose Abreu,
Tom Lendacky, James E.J. Bottomley, Jani Nikula, linux-fsdevel,
Steven Rostedt, Rodrigo Vivi, Benson Leung, Kalle Valo,
Karsten Keil, Martin K. Petersen, Greg Kroah-Hartman,
linux-wireless, linux-kernel, Sergey Senozhatsky, David Laight,
Daniel Vetter, netdev, Enric Balletbo i Serra, Andrew Morton,
David S. Miller, Alexander Viro
In-Reply-To: <20190625031726.12173-4-alastair@au1.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]
Hi Alastair,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.2-rc6 next-20190625]
[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/Alastair-D-Silva/Hexdump-Enhancements/20190625-224046
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.4.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=7.4.0 make.cross ARCH=xtensa
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
lib/hexdump.c: In function 'announce_skipped':
>> lib/hexdump.c:243:28: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t {aka unsigned int}' [-Wformat=]
printk("%s%s ** Skipped %lu bytes of value 0x%x **\n",
~~^
%u
vim +243 lib/hexdump.c
236
237 static void announce_skipped(const char *level, const char *prefix_str,
238 u8 val, size_t count)
239 {
240 if (count == 0)
241 return;
242
> 243 printk("%s%s ** Skipped %lu bytes of value 0x%x **\n",
244 level, prefix_str, count, val);
245 }
246
---
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: 58637 bytes --]
^ permalink raw reply
* Re: [PATCH RFC net-next 4/5] dt-bindings: net: dsa: mt7530: Add mediatek,ephy-handle to isolate ext. phy
From: Florian Fainelli @ 2019-06-25 19:16 UTC (permalink / raw)
To: René van Dorst
Cc: sean.wang, linux, davem, matthias.bgg, andrew, vivien.didelot,
frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190625093042.Horde._8BNPFSzW6B9-CI8P6akHTh@www.vdorst.com>
On 6/25/19 2:30 AM, René van Dorst wrote:
> Quoting Florian Fainelli <f.fainelli@gmail.com>:
>
> Hi Florian
>
>> On 6/24/19 7:52 AM, René van Dorst wrote:
>>> On some platforum the external phy can only interface to the port 5
>>> of the
>>> switch because the RGMII TX and RX lines are swapped. But it still
>>> can be
>>> useful to use the internal phy of the switch to act as a WAN port which
>>> connectes to the 2nd GMAC. This gives WAN port dedicated bandwidth to
>>> the SOC. This increases the LAN and WAN routing.
>>>
>>> By adding the optional property mediatek,ephy-handle, the external phy
>>> is put in isolation mode when internal phy is connected to 2nd GMAC via
>>> phy-handle property.
>>
>> Most platforms we have seen so far implement this logic with a mdio-mux,
>> can you see if that is possible here? stmmac has plenty of examples like
>> those.
>
> May I don't understand it correctly, but all the devices are on the same
> MDIO
> bus.
> I tried to make a ASCII diagram to make it a bit more clear.
Based on your diagram and your explanation, then I do not really see a
need for that property you can scan all of the switch port's properties
and determined which configuration is applied and perform the PHY
isolation as necessary. It looks like you are using this property as a
way to simplify your configuration logic, that is not quite what Device
Tree is meant for.
--
Florian
^ permalink raw reply
* [PATCH net] net: make skb_dst_force return false when dst was cleared
From: Florian Westphal @ 2019-06-25 19:22 UTC (permalink / raw)
To: netdev; +Cc: eric.dumazet, Florian Westphal
XFRM and netfilter don't expect that skb_dst_force() can cause skb to lose
its dst entry.
I got a bug report with a skb->dst NULL dereference in netfilter
output path. The backtrace contains nf_reinject(), so the dst
might have been cleared when skb got queued to userspace.
The xfrm part of this change was done after code inspection,
it looks like similar crash could happen here too.
One way to fix this is to add a skb_dst() check right after
skb_dst_force() call, but I think its preferable to make the
'dst might get cleared' part of the function explicit.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
include/net/dst.h | 6 +++++-
net/netfilter/nf_queue.c | 6 +++++-
net/xfrm/xfrm_policy.c | 5 ++++-
3 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/include/net/dst.h b/include/net/dst.h
index 12b31c602cb0..42cd53d51364 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -302,8 +302,9 @@ static inline bool dst_hold_safe(struct dst_entry *dst)
* @skb: buffer
*
* If dst is not yet refcounted and not destroyed, grab a ref on it.
+ * Returns false if skb had a destroyed dst.
*/
-static inline void skb_dst_force(struct sk_buff *skb)
+static inline bool skb_dst_force(struct sk_buff *skb)
{
if (skb_dst_is_noref(skb)) {
struct dst_entry *dst = skb_dst(skb);
@@ -313,7 +314,10 @@ static inline void skb_dst_force(struct sk_buff *skb)
dst = NULL;
skb->_skb_refdst = (unsigned long)dst;
+ return dst != NULL;
}
+
+ return true;
}
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index b5b2be55ca82..dc8628a919a5 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -190,6 +190,11 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
goto err;
}
+ if (!skb_dst_force(skb)) {
+ status = -ENETDOWN;
+ goto err;
+ }
+
*entry = (struct nf_queue_entry) {
.skb = skb,
.state = *state,
@@ -198,7 +203,6 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
};
nf_queue_entry_get_refs(entry);
- skb_dst_force(skb);
switch (entry->state.pf) {
case AF_INET:
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index b1694d5d15d3..5c66c18d5ff5 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2842,7 +2842,10 @@ static int xdst_queue_output(struct net *net, struct sock *sk, struct sk_buff *s
return -EAGAIN;
}
- skb_dst_force(skb);
+ if (!skb_dst_force(skb)) {
+ kfree_skb(skb);
+ return -ENETDOWN;
+ }
spin_lock_bh(&pq->hold_queue.lock);
--
2.21.0
^ permalink raw reply related
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Daniel Santos @ 2019-06-25 19:27 UTC (permalink / raw)
To: Andrew Lunn
Cc: Russell King - ARM Linux admin, René van Dorst, sean.wang,
f.fainelli, davem, matthias.bgg, vivien.didelot, frank-w, netdev,
linux-mediatek, linux-mips
In-Reply-To: <20190625190246.GA27733@lunn.ch>
On 6/25/19 2:02 PM, Andrew Lunn wrote:
>> But will there still be a mechanism to ignore link partner's advertising
>> and force these parameters?
> >From man 1 ethtool:
>
> -a --show-pause
> Queries the specified Ethernet device for pause parameter information.
>
> -A --pause
> Changes the pause parameters of the specified Ethernet device.
>
> autoneg on|off
> Specifies whether pause autonegotiation should be enabled.
>
> rx on|off
> Specifies whether RX pause should be enabled.
>
> tx on|off
> Specifies whether TX pause should be enabled.
>
> You need to check the driver to see if it actually implements this
> ethtool call, but that is how it should be configured.
>
> Andrew
>
Thank you Andrew,
So in this context, my question is the difference between "enabling" and
"forcing". Here's that register for the mt7620 (which has an mt7530 on
its die): https://imgur.com/a/pTk0668 I believe this is also what René
is seeking clarity on?
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH net] net/sched: flower: fix infinite loop in fl_walk()
From: Cong Wang @ 2019-06-25 19:29 UTC (permalink / raw)
To: Davide Caratti
Cc: Vlad Buslov, David S. Miller, Linux Kernel Network Developers,
Lucas Bates
In-Reply-To: <CAM_iQpW_-e+duPqKVXSDn7fp3WOKfs+RgVkFkfeQJQUTP_0x1Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 601 bytes --]
On Tue, Jun 25, 2019 at 11:07 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On one hand, its callers should not need to worry about details
> like overflow. On the other hand, in fact it does exactly what its
> callers tell it to do, the problematic part is actually the
> incremented id. On 64bit, it is fairly easy, we can just simply
> know 'long' is longer than 32bit and leverage this to detect overflow,
> but on 32bit this clearly doesn't work.
>
> Let me think about it.
Davide, do you mind to try the attached patch?
It should handle this overflow case more gracefully, I hope.
Thanks.
[-- Attachment #2: idr_get_next_ul.diff --]
[-- Type: application/octet-stream, Size: 2903 bytes --]
commit 685934f9eed9b50a46d33a9ec9671800397d20cc
Author: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue Jun 25 12:23:18 2019 -0700
idr: fix idr_get_next_ul() usage
Reported-by: Li Shuang <shuali@redhat.com>
Cc: Davide Caratti <dcaratti@redhat.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
index c6c28f56aa29..c73f80bddde4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
@@ -105,10 +105,11 @@ static struct list_head *mlx5_fc_counters_lookup_next(struct mlx5_core_dev *dev,
rcu_read_lock();
/* skip counters that are in idr, but not yet in counters list */
- while ((counter = idr_get_next_ul(&fc_stats->counters_idr,
- &next_id)) != NULL &&
- list_empty(&counter->list))
- next_id++;
+ idr_for_each_entry_continue_ul(&fc_stats->counters_idr,
+ counter, next_id) {
+ if (list_empty(&counter->list))
+ continue;
+ }
rcu_read_unlock();
return counter ? &counter->list : &fc_stats->counters;
diff --git a/include/linux/idr.h b/include/linux/idr.h
index ee7abae143d3..af7a67e65c1c 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -198,7 +198,21 @@ static inline void idr_preload_end(void)
* is convenient for a "not found" value.
*/
#define idr_for_each_entry_ul(idr, entry, id) \
- for (id = 0; ((entry) = idr_get_next_ul(idr, &(id))) != NULL; ++id)
+ for (id = 0; \
+ (u32)id + 1 > id && ((entry) = idr_get_next_ul(idr, &(id))) != NULL; \
+ ++id)
+
+/**
+ * idr_for_each_entry_continue_ul() - Continue iteration over an IDR's elements of a given type
+ * @idr: IDR handle.
+ * @entry: The type * to use as a cursor.
+ * @id: Entry ID.
+ *
+ * Continue to iterate over entries, continuing after the current position.
+ */
+#define idr_for_each_entry_continue_ul(idr, entry, id) \
+ for (; (u32)id + 1 > id && ((entry) = idr_get_next_ul(idr, &(id))) != NULL; \
+ ++id)
/**
* idr_for_each_entry_continue() - Continue iteration over an IDR's elements of a given type
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index eedd5786c084..06338dadd5e4 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -528,17 +528,18 @@ static struct cls_fl_filter *fl_get_next_filter(struct tcf_proto *tp,
unsigned long *handle)
{
struct cls_fl_head *head = fl_head_dereference(tp);
+ unsigned long id = *handle;
struct cls_fl_filter *f;
rcu_read_lock();
- while ((f = idr_get_next_ul(&head->handle_idr, handle))) {
+ idr_for_each_entry_continue_ul(&head->handle_idr, f, id) {
/* don't return filters that are being deleted */
if (refcount_inc_not_zero(&f->refcnt))
break;
- ++(*handle);
}
rcu_read_unlock();
+ *handle = id;
return f;
}
^ permalink raw reply related
* [bpf-next v2 06/10] tools headers: Adopt compiletime_assert from kernel sources
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
This will come in handy to verify that the hardcoded size of the
context data in bpf_test struct is high enough to hold some struct.
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/include/linux/compiler.h | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/tools/include/linux/compiler.h b/tools/include/linux/compiler.h
index 1827c2f973f9..b4e97751000a 100644
--- a/tools/include/linux/compiler.h
+++ b/tools/include/linux/compiler.h
@@ -172,4 +172,32 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
# define __fallthrough
#endif
+
+#ifdef __OPTIMIZE__
+# define __compiletime_assert(condition, msg, prefix, suffix) \
+ do { \
+ extern void prefix ## suffix(void) __compiletime_error(msg); \
+ if (!(condition)) \
+ prefix ## suffix(); \
+ } while (0)
+#else
+# define __compiletime_assert(condition, msg, prefix, suffix) do { } while (0)
+#endif
+
+#define _compiletime_assert(condition, msg, prefix, suffix) \
+ __compiletime_assert(condition, msg, prefix, suffix)
+
+/**
+ * compiletime_assert - break build and emit msg if condition is false
+ * @condition: a compile-time constant condition to check
+ * @msg: a message to emit if condition is false
+ *
+ * In tradition of POSIX assert, this macro will break the build if the
+ * supplied condition is *false*, emitting the supplied error message if the
+ * compiler has support to do so.
+ */
+#define compiletime_assert(condition, msg) \
+ _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
+
+
#endif /* _TOOLS_LINUX_COMPILER_H */
--
2.20.1
^ permalink raw reply related
* [bpf-next v2 10/10] selftests/bpf: Test correctness of narrow 32bit read on 64bit field
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
Test the correctness of the 32bit narrow reads by reading both halves
of the 64 bit field and doing a binary or on them to see if we get the
original value.
It succeeds as it should, but with the commit e2f7fc0ac695 ("bpf: fix
undefined behavior in narrow load handling") reverted, the test fails
with a following message:
> $ sudo ./test_verifier
> ...
> #967/p 32bit loads of a 64bit field (both least and most significant words) FAIL retval -1985229329 != 0
> verification time 17 usec
> stack depth 0
> processed 8 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
> ...
> Summary: 1519 PASSED, 0 SKIPPED, 1 FAILED
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 19 ++++++++++++++++++
.../testing/selftests/bpf/verifier/var_off.c | 20 +++++++++++++++++++
2 files changed, 39 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 6fa962014b64..444c1ea1e326 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -24,6 +24,7 @@
#include <sys/capability.h>
+#include <linux/compiler.h>
#include <linux/unistd.h>
#include <linux/filter.h>
#include <linux/bpf_perf_event.h>
@@ -341,6 +342,24 @@ static void bpf_fill_perf_event_test_run_check(struct bpf_test *self)
self->fill_insns = NULL;
}
+static void bpf_fill_32bit_loads(struct bpf_test *self)
+{
+ compiletime_assert(
+ sizeof(struct bpf_perf_event_data) <= TEST_CTX_LEN,
+ "buffer for ctx is too short to fit struct bpf_perf_event_data");
+ compiletime_assert(
+ sizeof(struct bpf_perf_event_value) <= TEST_DATA_LEN,
+ "buffer for data is too short to fit struct bpf_perf_event_value");
+
+ struct bpf_perf_event_data ctx = {
+ .sample_period = 0x0123456789abcdef,
+ };
+
+ memcpy(self->ctx, &ctx, sizeof(ctx));
+ free(self->fill_insns);
+ self->fill_insns = NULL;
+}
+
/* BPF_SK_LOOKUP contains 13 instructions, if you need to fix up maps */
#define BPF_SK_LOOKUP(func) \
/* struct bpf_sock_tuple tuple = {} */ \
diff --git a/tools/testing/selftests/bpf/verifier/var_off.c b/tools/testing/selftests/bpf/verifier/var_off.c
index 8504ac937809..14d222f37081 100644
--- a/tools/testing/selftests/bpf/verifier/var_off.c
+++ b/tools/testing/selftests/bpf/verifier/var_off.c
@@ -246,3 +246,23 @@
.result = ACCEPT,
.prog_type = BPF_PROG_TYPE_LWT_IN,
},
+{
+ "32bit loads of a 64bit field (both least and most significant words)",
+ .insns = {
+ BPF_LDX_MEM(BPF_W, BPF_REG_4, BPF_REG_1, offsetof(struct bpf_perf_event_data, sample_period)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_5, BPF_REG_1, offsetof(struct bpf_perf_event_data, sample_period) + 4),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, offsetof(struct bpf_perf_event_data, sample_period)),
+ BPF_ALU64_IMM(BPF_LSH, BPF_REG_5, 32),
+ BPF_ALU64_REG(BPF_OR, BPF_REG_4, BPF_REG_5),
+ BPF_ALU64_REG(BPF_XOR, BPF_REG_4, BPF_REG_6),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_4),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_PERF_EVENT,
+ .ctx = { 0, },
+ .ctx_len = sizeof(struct bpf_perf_event_data),
+ .data = { 0, },
+ .data_len = sizeof(struct bpf_perf_event_value),
+ .fill_helper = bpf_fill_32bit_loads,
+},
--
2.20.1
^ permalink raw reply related
* [bpf-next v2 09/10] selftests/bpf: Add tests for bpf_prog_test_run for perf events progs
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
The tests check if ctx and data are correctly prepared from ctx_in and
data_in, so accessing the ctx and using the bpf_perf_prog_read_value
work as expected.
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 48 ++++++++++
.../selftests/bpf/verifier/perf_event_run.c | 93 +++++++++++++++++++
2 files changed, 141 insertions(+)
create mode 100644 tools/testing/selftests/bpf/verifier/perf_event_run.c
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 05bad54f481f..6fa962014b64 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -293,6 +293,54 @@ static void bpf_fill_scale(struct bpf_test *self)
}
}
+static void bpf_fill_perf_event_test_run_check(struct bpf_test *self)
+{
+ compiletime_assert(
+ sizeof(struct bpf_perf_event_data) <= TEST_CTX_LEN,
+ "buffer for ctx is too short to fit struct bpf_perf_event_data");
+ compiletime_assert(
+ sizeof(struct bpf_perf_event_value) <= TEST_DATA_LEN,
+ "buffer for data is too short to fit struct bpf_perf_event_value");
+
+ struct bpf_perf_event_data ctx = {
+ .regs = (bpf_user_pt_regs_t) {
+ .r15 = 1,
+ .r14 = 2,
+ .r13 = 3,
+ .r12 = 4,
+ .rbp = 5,
+ .rbx = 6,
+ .r11 = 7,
+ .r10 = 8,
+ .r9 = 9,
+ .r8 = 10,
+ .rax = 11,
+ .rcx = 12,
+ .rdx = 13,
+ .rsi = 14,
+ .rdi = 15,
+ .orig_rax = 16,
+ .rip = 17,
+ .cs = 18,
+ .eflags = 19,
+ .rsp = 20,
+ .ss = 21,
+ },
+ .sample_period = 1,
+ .addr = 2,
+ };
+ struct bpf_perf_event_value data = {
+ .counter = 1,
+ .enabled = 2,
+ .running = 3,
+ };
+
+ memcpy(self->ctx, &ctx, sizeof(ctx));
+ memcpy(self->data, &data, sizeof(data));
+ free(self->fill_insns);
+ self->fill_insns = NULL;
+}
+
/* BPF_SK_LOOKUP contains 13 instructions, if you need to fix up maps */
#define BPF_SK_LOOKUP(func) \
/* struct bpf_sock_tuple tuple = {} */ \
diff --git a/tools/testing/selftests/bpf/verifier/perf_event_run.c b/tools/testing/selftests/bpf/verifier/perf_event_run.c
new file mode 100644
index 000000000000..d451932a6fc0
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/perf_event_run.c
@@ -0,0 +1,93 @@
+#define PER_LOAD_AND_CHECK_PTREG(PT_REG_FIELD, VALUE) \
+ PER_LOAD_AND_CHECK_CTX(offsetof(bpf_user_pt_regs_t, PT_REG_FIELD), VALUE)
+#define PER_LOAD_AND_CHECK_EVENT(PED_FIELD, VALUE) \
+ PER_LOAD_AND_CHECK_CTX(offsetof(struct bpf_perf_event_data, PED_FIELD), VALUE)
+#define PER_LOAD_AND_CHECK_CTX(OFFSET, VALUE) \
+ PER_LOAD_AND_CHECK_64(BPF_REG_4, BPF_REG_1, OFFSET, VALUE)
+#define PER_LOAD_AND_CHECK_VALUE(PEV_FIELD, VALUE) \
+ PER_LOAD_AND_CHECK_64(BPF_REG_7, BPF_REG_6, offsetof(struct bpf_perf_event_value, PEV_FIELD), VALUE)
+#define PER_LOAD_AND_CHECK_64(DST, SRC, OFFSET, VALUE) \
+ BPF_LDX_MEM(BPF_DW, DST, SRC, OFFSET), \
+ BPF_JMP_IMM(BPF_JEQ, DST, VALUE, 2), \
+ BPF_MOV64_IMM(BPF_REG_0, VALUE), \
+ BPF_EXIT_INSN()
+
+{
+ "check if regs contain expected values",
+ .insns = {
+ PER_LOAD_AND_CHECK_PTREG(r15, 1),
+ PER_LOAD_AND_CHECK_PTREG(r14, 2),
+ PER_LOAD_AND_CHECK_PTREG(r13, 3),
+ PER_LOAD_AND_CHECK_PTREG(r12, 4),
+ PER_LOAD_AND_CHECK_PTREG(rbp, 5),
+ PER_LOAD_AND_CHECK_PTREG(rbx, 6),
+ PER_LOAD_AND_CHECK_PTREG(r11, 7),
+ PER_LOAD_AND_CHECK_PTREG(r10, 8),
+ PER_LOAD_AND_CHECK_PTREG(r9, 9),
+ PER_LOAD_AND_CHECK_PTREG(r8, 10),
+ PER_LOAD_AND_CHECK_PTREG(rax, 11),
+ PER_LOAD_AND_CHECK_PTREG(rcx, 12),
+ PER_LOAD_AND_CHECK_PTREG(rdx, 13),
+ PER_LOAD_AND_CHECK_PTREG(rsi, 14),
+ PER_LOAD_AND_CHECK_PTREG(rdi, 15),
+ PER_LOAD_AND_CHECK_PTREG(orig_rax, 16),
+ PER_LOAD_AND_CHECK_PTREG(rip, 17),
+ PER_LOAD_AND_CHECK_PTREG(cs, 18),
+ PER_LOAD_AND_CHECK_PTREG(eflags, 19),
+ PER_LOAD_AND_CHECK_PTREG(rsp, 20),
+ PER_LOAD_AND_CHECK_PTREG(ss, 21),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_PERF_EVENT,
+ .ctx_len = sizeof(struct bpf_perf_event_data),
+ .data_len = sizeof(struct bpf_perf_event_value),
+ .fill_helper = bpf_fill_perf_event_test_run_check,
+},
+{
+ "check if sample period and addr contain expected values",
+ .insns = {
+ PER_LOAD_AND_CHECK_EVENT(sample_period, 1),
+ PER_LOAD_AND_CHECK_EVENT(addr, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_PERF_EVENT,
+ .ctx_len = sizeof(struct bpf_perf_event_data),
+ .data_len = sizeof(struct bpf_perf_event_value),
+ .fill_helper = bpf_fill_perf_event_test_run_check,
+},
+{
+ "check if bpf_perf_prog_read_value returns expected data",
+ .insns = {
+ // allocate space for a struct bpf_perf_event_value
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, -(int)sizeof(struct bpf_perf_event_value)),
+ // prepare parameters for bpf_perf_prog_read_value(ctx, struct bpf_perf_event_value*, u32)
+ // BPF_REG_1 already contains the context
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_6),
+ BPF_MOV64_IMM(BPF_REG_3, sizeof(struct bpf_perf_event_value)),
+ BPF_EMIT_CALL(BPF_FUNC_perf_prog_read_value),
+ // check the return value
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
+ BPF_EXIT_INSN(),
+ // check if the fields match the expected values
+ PER_LOAD_AND_CHECK_VALUE(counter, 1),
+ PER_LOAD_AND_CHECK_VALUE(enabled, 2),
+ PER_LOAD_AND_CHECK_VALUE(running, 3),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_PERF_EVENT,
+ .ctx_len = sizeof(struct bpf_perf_event_data),
+ .data_len = sizeof(struct bpf_perf_event_value),
+ .fill_helper = bpf_fill_perf_event_test_run_check,
+},
+#undef PER_LOAD_AND_CHECK_64
+#undef PER_LOAD_AND_CHECK_VALUE
+#undef PER_LOAD_AND_CHECK_CTX
+#undef PER_LOAD_AND_CHECK_EVENT
+#undef PER_LOAD_AND_CHECK_PTREG
--
2.20.1
^ permalink raw reply related
* [bpf-next v2 08/10] bpf: Implement bpf_prog_test_run for perf event programs
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
As an input, test run for perf event program takes struct
bpf_perf_event_data as ctx_in and struct bpf_perf_event_value as
data_in. For an output, it basically ignores ctx_out and data_out.
The implementation sets an instance of struct bpf_perf_event_data_kern
in such a way that the BPF program reading data from context will
receive what we passed to the bpf prog test run in ctx_in. Also BPF
program can call bpf_perf_prog_read_value to receive what was passed
in data_in.
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
kernel/trace/bpf_trace.c | 107 ++++++++++++++++++
.../bpf/verifier/perf_event_sample_period.c | 8 ++
2 files changed, 115 insertions(+)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index c102c240bb0b..2fa49ea8a475 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -16,6 +16,8 @@
#include <asm/tlb.h>
+#include <trace/events/bpf_test_run.h>
+
#include "trace_probe.h"
#include "trace.h"
@@ -1160,7 +1162,112 @@ const struct bpf_verifier_ops perf_event_verifier_ops = {
.convert_ctx_access = pe_prog_convert_ctx_access,
};
+static int pe_prog_test_run(struct bpf_prog *prog,
+ const union bpf_attr *kattr,
+ union bpf_attr __user *uattr)
+{
+ void __user *ctx_in = u64_to_user_ptr(kattr->test.ctx_in);
+ void __user *data_in = u64_to_user_ptr(kattr->test.data_in);
+ u32 data_size_in = kattr->test.data_size_in;
+ u32 ctx_size_in = kattr->test.ctx_size_in;
+ u32 repeat = kattr->test.repeat;
+ u32 retval = 0, duration = 0;
+ int err = -EINVAL;
+ u64 time_start, time_spent = 0;
+ int i;
+ struct perf_sample_data sample_data = {0, };
+ struct perf_event event = {0, };
+ struct bpf_perf_event_data_kern real_ctx = {0, };
+ struct bpf_perf_event_data fake_ctx = {0, };
+ struct bpf_perf_event_value value = {0, };
+
+ if (ctx_size_in != sizeof(fake_ctx))
+ goto out;
+ if (data_size_in != sizeof(value))
+ goto out;
+
+ if (copy_from_user(&fake_ctx, ctx_in, ctx_size_in)) {
+ err = -EFAULT;
+ goto out;
+ }
+ if (copy_from_user(&value, data_in, data_size_in)) {
+ err = -EFAULT;
+ goto out;
+ }
+
+ real_ctx.regs = &fake_ctx.regs;
+ real_ctx.data = &sample_data;
+ real_ctx.event = &event;
+ perf_sample_data_init(&sample_data, fake_ctx.addr,
+ fake_ctx.sample_period);
+ event.cpu = smp_processor_id();
+ event.oncpu = -1;
+ event.state = PERF_EVENT_STATE_OFF;
+ local64_set(&event.count, value.counter);
+ event.total_time_enabled = value.enabled;
+ event.total_time_running = value.running;
+ /* make self as a leader - it is used only for checking the
+ * state field
+ */
+ event.group_leader = &event;
+
+ /* slightly changed copy pasta from bpf_test_run() in
+ * net/bpf/test_run.c
+ */
+ if (!repeat)
+ repeat = 1;
+
+ rcu_read_lock();
+ preempt_disable();
+ time_start = ktime_get_ns();
+ for (i = 0; i < repeat; i++) {
+ retval = BPF_PROG_RUN(prog, &real_ctx);
+
+ if (signal_pending(current)) {
+ err = -EINTR;
+ preempt_enable();
+ rcu_read_unlock();
+ goto out;
+ }
+
+ if (need_resched()) {
+ 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;
+ /* end of slightly changed copy pasta from bpf_test_run() in
+ * net/bpf/test_run.c
+ */
+
+ if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval))) {
+ err = -EFAULT;
+ goto out;
+ }
+ if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration))) {
+ err = -EFAULT;
+ goto out;
+ }
+ err = 0;
+out:
+ trace_bpf_test_finish(&err);
+ return err;
+}
+
const struct bpf_prog_ops perf_event_prog_ops = {
+ .test_run = pe_prog_test_run,
};
static DEFINE_MUTEX(bpf_event_mutex);
diff --git a/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c b/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
index 471c1a5950d8..16e9e5824d14 100644
--- a/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
+++ b/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
@@ -13,6 +13,8 @@
},
.result = ACCEPT,
.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+ .ctx_len = sizeof(struct bpf_perf_event_data),
+ .data_len = sizeof(struct bpf_perf_event_value),
},
{
"check bpf_perf_event_data->sample_period half load permitted",
@@ -29,6 +31,8 @@
},
.result = ACCEPT,
.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+ .ctx_len = sizeof(struct bpf_perf_event_data),
+ .data_len = sizeof(struct bpf_perf_event_value),
},
{
"check bpf_perf_event_data->sample_period word load permitted",
@@ -45,6 +49,8 @@
},
.result = ACCEPT,
.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+ .ctx_len = sizeof(struct bpf_perf_event_data),
+ .data_len = sizeof(struct bpf_perf_event_value),
},
{
"check bpf_perf_event_data->sample_period dword load permitted",
@@ -56,4 +62,6 @@
},
.result = ACCEPT,
.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+ .ctx_len = sizeof(struct bpf_perf_event_data),
+ .data_len = sizeof(struct bpf_perf_event_value),
},
--
2.20.1
^ permalink raw reply related
* [bpf-next v2 07/10] tools headers: sync struct bpf_perf_event_data
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
struct bpf_perf_event_data in kernel headers has the addr field, which
is missing in the tools version of the struct. This will be important
for the bpf prog test run implementation for perf events as it will
expect data to be an instance of struct bpf_perf_event_data, so the
size of the data needs to match sizeof(bpf_perf_event_data).
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/include/uapi/linux/bpf_perf_event.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/include/uapi/linux/bpf_perf_event.h b/tools/include/uapi/linux/bpf_perf_event.h
index 8f95303f9d80..eb1b9d21250c 100644
--- a/tools/include/uapi/linux/bpf_perf_event.h
+++ b/tools/include/uapi/linux/bpf_perf_event.h
@@ -13,6 +13,7 @@
struct bpf_perf_event_data {
bpf_user_pt_regs_t regs;
__u64 sample_period;
+ __u64 addr;
};
#endif /* _UAPI__LINUX_BPF_PERF_EVENT_H__ */
--
2.20.1
^ permalink raw reply related
* [bpf-next v2 05/10] selftests/bpf: Allow passing more information to BPF prog test run
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
The test case can specify a custom length of the data member, context
data and its length, which will be passed to
bpf_prog_test_run_xattr. For backward compatilibity, if the data
length is 0 (which is what will happen when the field is left
unspecified in the designated initializer of a struct), then the
length passed to the bpf_prog_test_run_xattr is TEST_DATA_LEN.
Also for backward compatilibity, if context data length is 0, NULL is
passed as a context to bpf_prog_test_run_xattr. This is to avoid
breaking other tests, where context data being NULL and context data
length being 0 is handled differently from the case where context data
is not NULL and context data length is 0.
Custom lengths still can't be greater than hardcoded 64 bytes for data
and 192 for context data.
192 for context data was picked to allow passing struct
bpf_perf_event_data as a context for perf event programs. The struct
is quite large, because it contains struct pt_regs.
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 68 +++++++++++++++++++--
1 file changed, 62 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index db1f0f758f81..05bad54f481f 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -54,6 +54,7 @@
#define MAX_TEST_RUNS 8
#define POINTER_VALUE 0xcafe4all
#define TEST_DATA_LEN 64
+#define TEST_CTX_LEN 192
#define F_NEEDS_EFFICIENT_UNALIGNED_ACCESS (1 << 0)
#define F_LOAD_WITH_STRICT_ALIGNMENT (1 << 1)
@@ -96,6 +97,9 @@ struct bpf_test {
enum bpf_prog_type prog_type;
uint8_t flags;
__u8 data[TEST_DATA_LEN];
+ __u32 data_len;
+ __u8 ctx[TEST_CTX_LEN];
+ __u32 ctx_len;
void (*fill_helper)(struct bpf_test *self);
uint8_t runs;
struct {
@@ -104,6 +108,9 @@ struct bpf_test {
__u8 data[TEST_DATA_LEN];
__u64 data64[TEST_DATA_LEN / 8];
};
+ __u32 data_len;
+ __u8 ctx[TEST_CTX_LEN];
+ __u32 ctx_len;
} retvals[MAX_TEST_RUNS];
};
@@ -818,7 +825,7 @@ static int set_admin(bool admin)
}
static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
- void *data, size_t size_data)
+ void *data, size_t size_data, void *ctx, size_t size_ctx)
{
__u8 tmp[TEST_DATA_LEN << 2];
__u32 size_tmp = sizeof(tmp);
@@ -831,6 +838,8 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
.data_size_in = size_data,
.data_out = tmp,
.data_size_out = size_tmp,
+ .ctx_in = ctx,
+ .ctx_size_in = size_ctx,
};
if (unpriv)
@@ -956,13 +965,39 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
if (!alignment_prevented_execution && fd_prog >= 0) {
uint32_t expected_val;
int i;
+ __u32 size_data;
+ __u32 size_ctx;
+ bool bad_size;
+ void *ctx;
if (!test->runs) {
+ if (test->data_len > 0)
+ size_data = test->data_len;
+ else
+ size_data = sizeof(test->data);
+ size_ctx = test->ctx_len;
+ bad_size = false;
expected_val = unpriv && test->retval_unpriv ?
test->retval_unpriv : test->retval;
- err = do_prog_test_run(fd_prog, unpriv, expected_val,
- test->data, sizeof(test->data));
+ if (size_data > sizeof(test->data)) {
+ printf("FAIL: data size (%u) greater than TEST_DATA_LEN (%lu) ", size_data, sizeof(test->data));
+ bad_size = true;
+ }
+ if (size_ctx > sizeof(test->ctx)) {
+ printf("FAIL: ctx size (%u) greater than TEST_CTX_LEN (%lu) ", size_ctx, sizeof(test->ctx));
+ bad_size = true;
+ }
+ if (size_ctx)
+ ctx = test->ctx;
+ else
+ ctx = NULL;
+ if (bad_size)
+ err = 1;
+ else
+ err = do_prog_test_run(fd_prog, unpriv, expected_val,
+ test->data, size_data,
+ ctx, size_ctx);
if (err)
run_errs++;
else
@@ -970,14 +1005,35 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
}
for (i = 0; i < test->runs; i++) {
+ if (test->retvals[i].data_len > 0)
+ size_data = test->retvals[i].data_len;
+ else
+ size_data = sizeof(test->retvals[i].data);
+ size_ctx = test->retvals[i].ctx_len;
+ bad_size = false;
if (unpriv && test->retvals[i].retval_unpriv)
expected_val = test->retvals[i].retval_unpriv;
else
expected_val = test->retvals[i].retval;
- err = do_prog_test_run(fd_prog, unpriv, expected_val,
- test->retvals[i].data,
- sizeof(test->retvals[i].data));
+ if (size_data > sizeof(test->retvals[i].data)) {
+ printf("FAIL: data size (%u) at run %i greater than TEST_DATA_LEN (%lu) ", size_data, i + 1, sizeof(test->retvals[i].data));
+ bad_size = true;
+ }
+ if (size_ctx > sizeof(test->retvals[i].ctx)) {
+ printf("FAIL: ctx size (%u) at run %i greater than TEST_CTX_LEN (%lu) ", size_ctx, i + 1, sizeof(test->retvals[i].ctx));
+ bad_size = true;
+ }
+ if (size_ctx)
+ ctx = test->retvals[i].ctx;
+ else
+ ctx = NULL;
+ if (bad_size)
+ err = 1;
+ else
+ err = do_prog_test_run(fd_prog, unpriv, expected_val,
+ test->retvals[i].data, size_data,
+ ctx, size_ctx);
if (err) {
printf("(run %d/%d) ", i + 1, test->runs);
run_errs++;
--
2.20.1
^ permalink raw reply related
* [bpf-next v2 04/10] selftests/bpf: Use bpf_prog_test_run_xattr
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
The bpf_prog_test_run_xattr function gives more options to set up a
test run of a BPF program than the bpf_prog_test_run function.
We will need this extra flexibility to pass ctx data later.
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 779e30b96ded..db1f0f758f81 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -822,14 +822,20 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
{
__u8 tmp[TEST_DATA_LEN << 2];
__u32 size_tmp = sizeof(tmp);
- uint32_t retval;
int err;
int saved_errno;
+ struct bpf_prog_test_run_attr attr = {
+ .prog_fd = fd_prog,
+ .repeat = 1,
+ .data_in = data,
+ .data_size_in = size_data,
+ .data_out = tmp,
+ .data_size_out = size_tmp,
+ };
if (unpriv)
set_admin(true);
- err = bpf_prog_test_run(fd_prog, 1, data, size_data,
- tmp, &size_tmp, &retval, NULL);
+ err = bpf_prog_test_run_xattr(&attr);
saved_errno = errno;
if (unpriv)
set_admin(false);
@@ -846,9 +852,9 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
return err;
}
}
- if (retval != expected_val &&
+ if (attr.retval != expected_val &&
expected_val != POINTER_VALUE) {
- printf("FAIL retval %d != %d ", retval, expected_val);
+ printf("FAIL retval %d != %d ", attr.retval, expected_val);
return 1;
}
--
2.20.1
^ permalink raw reply related
* [bpf-next v2 03/10] selftests/bpf: Avoid another case of errno clobbering
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak, Stanislav Fomichev
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
Commit 8184d44c9a57 ("selftests/bpf: skip verifier tests for
unsupported program types") added a check for an unsupported program
type. The function doing it changes errno, so test_verifier should
save it before calling it if test_verifier wants to print a reason why
verifying a BPF program of a supported type failed.
Fixes: 8184d44c9a57 ("selftests/bpf: skip verifier tests for unsupported program types")
Cc: Stanislav Fomichev <sdf@google.com>
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 12589da13487..779e30b96ded 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -867,6 +867,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
int fixup_skips;
__u32 pflags;
int i, err;
+ int saved_errno;
for (i = 0; i < MAX_NR_MAPS; i++)
map_fds[i] = -1;
@@ -894,6 +895,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
pflags |= BPF_F_ANY_ALIGNMENT;
fd_prog = bpf_verify_program(prog_type, prog, prog_len, pflags,
"GPL", 0, bpf_vlog, sizeof(bpf_vlog), 4);
+ saved_errno = errno;
if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
printf("SKIP (unsupported program type %d)\n", prog_type);
skips++;
@@ -910,7 +912,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
if (expected_ret == ACCEPT) {
if (fd_prog < 0) {
printf("FAIL\nFailed to load prog '%s'!\n",
- strerror(errno));
+ strerror(saved_errno));
goto fail_log;
}
#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
--
2.20.1
^ permalink raw reply related
* [bpf-next v2 02/10] selftests/bpf: Avoid a clobbering of errno
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
Save errno right after bpf_prog_test_run returns, so we later check
the error code actually set by bpf_prog_test_run, not by some libcap
function.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Fixes: 832c6f2c29ec ("bpf: test make sure to run unpriv test cases in test_verifier")
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 9e17bda016ef..12589da13487 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -824,15 +824,17 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
__u32 size_tmp = sizeof(tmp);
uint32_t retval;
int err;
+ int saved_errno;
if (unpriv)
set_admin(true);
err = bpf_prog_test_run(fd_prog, 1, data, size_data,
tmp, &size_tmp, &retval, NULL);
+ saved_errno = errno;
if (unpriv)
set_admin(false);
if (err) {
- switch (errno) {
+ switch (saved_errno) {
case 524/*ENOTSUPP*/:
printf("Did not run the program (not supported) ");
return 0;
--
2.20.1
^ permalink raw reply related
* [bpf-next v2 00/10] Test the 32bit narrow reads
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
These patches try to test the fix made in commit e2f7fc0ac695 ("bpf:
fix undefined behavior in narrow load handling"). The problem existed
in the generated BPF bytecode that was doing a 32bit narrow read of a
64bit field, so to test it the code would need to be executed.
Currently the only such field exists in BPF_PROG_TYPE_PERF_EVENT,
which was not supported by bpf_prog_test_run().
I'm sending these patches to bpf-next now as they introduce a new
feature. But maybe some of those patches could go to the bpf branch?
There is a bit of yak shaving to do for the test to be run:
1. Print why the program could not be run (patch 1).
2. Some fixes for errno clobbering (patches 2 and 3).
3. Using bpf_prog_test_run_xattr, so I can pass ctx_in stuff too
(patch 4).
4. Adding ctx stuff to struct bpf_test (patch 5).
5. Some tools headers syncing (patches 6 and 7).
6. Implement bpf_prog_test_run for perf event programs and test it
(patches 8 and 9).
The last point is where I'm least sure how things should be done
properly:
1. There is a bunch of stuff to prepare for the
bpf_perf_prog_read_value to work, and that stuff is very hacky. I
would welcome some hints about how to set up the perf_event and
perf_sample_data structs in a way that is a bit more future-proof
than just setting some fields in a specific way, so some other code
won't use some other fields (like setting event.oncpu to -1 to
avoid event.pmu to be used for reading the value of the event).
2. The tests try to see if the test run for perf event sets up the
context properly, so they verify the struct pt_regs contents. They
way it is currently written Works For Me, but surely it won't work
on other arch. So what would be the way forward? Just put the test
case inside #ifdef __x86_64__?
3. Another thing in tests - I'm trying to make sure that the
bpf_perf_prog_read_value helper works as it seems to be the only
bpf perf helper that takes the ctx as a parameter. Is that enough
or I should test other helpers too?
About the test itself - I'm not sure if it will work on a big endian
machine. I think it should, but I don't have anything handy here to
verify it.
Krzesimir Nowak (10):
selftests/bpf: Print a message when tester could not run a program
selftests/bpf: Avoid a clobbering of errno
selftests/bpf: Avoid another case of errno clobbering
selftests/bpf: Use bpf_prog_test_run_xattr
selftests/bpf: Allow passing more information to BPF prog test run
tools headers: Adopt compiletime_assert from kernel sources
tools headers: sync struct bpf_perf_event_data
bpf: Implement bpf_prog_test_run for perf event programs
selftests/bpf: Add tests for bpf_prog_test_run for perf events progs
selftests/bpf: Test correctness of narrow 32bit read on 64bit field
kernel/trace/bpf_trace.c | 107 +++++++++++
tools/include/linux/compiler.h | 28 +++
tools/include/uapi/linux/bpf_perf_event.h | 1 +
tools/testing/selftests/bpf/test_verifier.c | 172 ++++++++++++++++--
.../selftests/bpf/verifier/perf_event_run.c | 93 ++++++++++
.../bpf/verifier/perf_event_sample_period.c | 8 +
.../testing/selftests/bpf/verifier/var_off.c | 20 ++
7 files changed, 414 insertions(+), 15 deletions(-)
create mode 100644 tools/testing/selftests/bpf/verifier/perf_event_run.c
--
2.20.1
^ permalink raw reply
* [bpf-next v2 01/10] selftests/bpf: Print a message when tester could not run a program
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
This prints a message when the error is about program type being not
supported by the test runner or because of permissions problem. This
is to see if the program we expected to run was actually executed.
The messages are open-coded because strerror(ENOTSUPP) returns
"Unknown error 524".
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index c5514daf8865..9e17bda016ef 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -831,11 +831,20 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
tmp, &size_tmp, &retval, NULL);
if (unpriv)
set_admin(false);
- if (err && errno != 524/*ENOTSUPP*/ && errno != EPERM) {
- printf("Unexpected bpf_prog_test_run error ");
- return err;
+ if (err) {
+ switch (errno) {
+ case 524/*ENOTSUPP*/:
+ printf("Did not run the program (not supported) ");
+ return 0;
+ case EPERM:
+ printf("Did not run the program (no permission) ");
+ return 0;
+ default:
+ printf("Unexpected bpf_prog_test_run error (%s) ", strerror(saved_errno));
+ return err;
+ }
}
- if (!err && retval != expected_val &&
+ if (retval != expected_val &&
expected_val != POINTER_VALUE) {
printf("FAIL retval %d != %d ", retval, expected_val);
return 1;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] xsk: Properly terminate assignment in xskq_produce_flush_desc
From: Jonathan Lemon @ 2019-06-25 19:46 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Björn Töpel, Magnus Karlsson, David S. Miller,
Alexei Starovoitov, Daniel Borkmann, Jakub Kicinski,
Jesper Dangaard Brouer, John Fastabend, netdev, bpf, xdp-newbies,
linux-kernel, clang-built-linux, Nick Desaulniers,
Nathan Huckleberry
In-Reply-To: <20190625182352.13918-1-natechancellor@gmail.com>
On 25 Jun 2019, at 11:23, Nathan Chancellor wrote:
> Clang warns:
>
> In file included from net/xdp/xsk_queue.c:10:
> net/xdp/xsk_queue.h:292:2: warning: expression result unused
> [-Wunused-value]
> WRITE_ONCE(q->ring->producer, q->prod_tail);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/linux/compiler.h:284:6: note: expanded from macro 'WRITE_ONCE'
> __u.__val; \
> ~~~ ^~~~~
> 1 warning generated.
>
> The q->prod_tail assignment has a comma at the end, not a semi-colon.
> Fix that so clang no longer warns and everything works as expected.
>
> Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support")
> Link: https://github.com/ClangBuiltLinux/linux/issues/544
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Nice find.
Acked-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> ---
> net/xdp/xsk_queue.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
> index 88b9ae24658d..cba4a640d5e8 100644
> --- a/net/xdp/xsk_queue.h
> +++ b/net/xdp/xsk_queue.h
> @@ -288,7 +288,7 @@ static inline void xskq_produce_flush_desc(struct
> xsk_queue *q)
> /* Order producer and data */
> smp_wmb(); /* B, matches C */
>
> - q->prod_tail = q->prod_head,
> + q->prod_tail = q->prod_head;
> WRITE_ONCE(q->ring->producer, q->prod_tail);
> }
>
> --
> 2.22.0
^ permalink raw reply
* Re: [PATCH 00/26] Netfilter updates for net-next
From: David Miller @ 2019-06-25 19:46 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, netdev
In-Reply-To: <20190625001233.22057-1-pablo@netfilter.org>
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 25 Jun 2019 02:12:07 +0200
> The following patches contains Netfilter updates for net-next:
...
> You can pull these changes from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git
>
> This batch comes with a conflict resolution between a patch to remove
> the GPL disclaimer by SPDX tags and Jozsef Kladecsik's email update.
Pulled, thanks.
^ permalink raw reply
* Re: [PATCH net] net: make skb_dst_force return false when dst was cleared
From: Eric Dumazet @ 2019-06-25 19:47 UTC (permalink / raw)
To: Florian Westphal, netdev
In-Reply-To: <20190625192209.6250-1-fw@strlen.de>
On 6/25/19 12:22 PM, Florian Westphal wrote:
> XFRM and netfilter don't expect that skb_dst_force() can cause skb to lose
> its dst entry.
>
> I got a bug report with a skb->dst NULL dereference in netfilter
> output path. The backtrace contains nf_reinject(), so the dst
> might have been cleared when skb got queued to userspace.
>
> The xfrm part of this change was done after code inspection,
> it looks like similar crash could happen here too.
>
> One way to fix this is to add a skb_dst() check right after
> skb_dst_force() call, but I think its preferable to make the
> 'dst might get cleared' part of the function explicit.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
> include/net/dst.h | 6 +++++-
> net/netfilter/nf_queue.c | 6 +++++-
> net/xfrm/xfrm_policy.c | 5 ++++-
> 3 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 12b31c602cb0..42cd53d51364 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -302,8 +302,9 @@ static inline bool dst_hold_safe(struct dst_entry *dst)
> * @skb: buffer
> *
> * If dst is not yet refcounted and not destroyed, grab a ref on it.
> + * Returns false if skb had a destroyed dst.
> */
> -static inline void skb_dst_force(struct sk_buff *skb)
> +static inline bool skb_dst_force(struct sk_buff *skb)
> {
> if (skb_dst_is_noref(skb)) {
> struct dst_entry *dst = skb_dst(skb);
> @@ -313,7 +314,10 @@ static inline void skb_dst_force(struct sk_buff *skb)
> dst = NULL;
>
> skb->_skb_refdst = (unsigned long)dst;
> + return dst != NULL;
> }
> +
> + return true;
This will return true, even if skb has a NULL dst.
Say if we have two skb_dst_force() calls for some reason
on the same skb, only the first one will return false.
^ permalink raw reply
* Re: [PATCH net] ipv4: Use return value of inet_iif() for __raw_v4_lookup in the while loop
From: David Miller @ 2019-06-25 19:47 UTC (permalink / raw)
To: ssuryaextr; +Cc: netdev, dsahern
In-Reply-To: <20190625001406.6437-1-ssuryaextr@gmail.com>
From: Stephen Suryaputra <ssuryaextr@gmail.com>
Date: Mon, 24 Jun 2019 20:14:06 -0400
> In commit 19e4e768064a8 ("ipv4: Fix raw socket lookup for local
> traffic"), the dif argument to __raw_v4_lookup() is coming from the
> returned value of inet_iif() but the change was done only for the first
> lookup. Subsequent lookups in the while loop still use skb->dev->ifIndex.
>
> Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
Applied and queued up for -stable.
I added the appropriate Fixes: tag, please do so next time.
^ permalink raw reply
* Re: [PATCH net-next 1/1] tc-testing: Restore original behaviour for namespaces in tdc
From: David Miller @ 2019-06-25 19:57 UTC (permalink / raw)
To: lucasb
Cc: netdev, nicolas.dichtel, jhs, xiyou.wangcong, jiri, mleitner,
vladbu, dcaratti, kernel
In-Reply-To: <1561424427-9949-1-git-send-email-lucasb@mojatatu.com>
From: Lucas Bates <lucasb@mojatatu.com>
Date: Mon, 24 Jun 2019 21:00:27 -0400
> This patch restores the original behaviour for tdc prior to the
> introduction of the plugin system, where the network namespace
> functionality was split from the main script.
>
> It introduces the concept of required plugins for testcases,
> and will automatically load any plugin that isn't already
> enabled when said plugin is required by even one testcase.
>
> Additionally, the -n option for the nsPlugin is deprecated
> so the default action is to make use of the namespaces.
> Instead, we introduce -N to not use them, but still create
> the veth pair.
>
> buildebpfPlugin's -B option is also deprecated.
>
> If a test cases requires the features of a specific plugin
> in order to pass, it should instead include a new key/value
> pair describing plugin interactions:
>
> "plugins": {
> "requires": "buildebpfPlugin"
> },
>
> A test case can have more than one required plugin: a list
> can be inserted as the value for 'requires'.
>
> Signed-off-by: Lucas Bates <lucasb@mojatatu.com>
Applied.
^ permalink raw reply
* Re: [PATCH net] ipv4: Use return value of inet_iif() for __raw_v4_lookup in the while loop
From: Stefano Brivio @ 2019-06-25 19:59 UTC (permalink / raw)
To: David Miller; +Cc: ssuryaextr, netdev, dsahern
In-Reply-To: <20190625.124738.1945131933038317898.davem@davemloft.net>
On Tue, 25 Jun 2019 12:47:38 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> From: Stephen Suryaputra <ssuryaextr@gmail.com>
> Date: Mon, 24 Jun 2019 20:14:06 -0400
>
> > In commit 19e4e768064a8 ("ipv4: Fix raw socket lookup for local
> > traffic"), the dif argument to __raw_v4_lookup() is coming from the
> > returned value of inet_iif() but the change was done only for the first
> > lookup. Subsequent lookups in the while loop still use skb->dev->ifIndex.
> >
> > Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
>
> Applied and queued up for -stable.
>
> I added the appropriate Fixes: tag, please do so next time.
I was about to point that out, but then I noticed that this
doesn't actually fix 19e4e768064a8 ("ipv4: Fix raw socket lookup for
local traffic"), it's just related as it fixes the same issue in
another (very likely) path in the same function.
I think this should have been:
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
I *guess* for -stable purposes the effect is the same.
--
Stefano
^ permalink raw reply
* Re: [PATCH 2/2] net/ipv6: Fix misuse of proc_dointvec "skip_notify_on_dev_down"
From: David Miller @ 2019-06-25 19:59 UTC (permalink / raw)
To: devel; +Cc: gregkh, jslaby, kuznet, yoshfuji, linux-kernel, netdev
In-Reply-To: <20190625030801.24538-2-devel@etsukata.com>
From: Eiichi Tsukata <devel@etsukata.com>
Date: Tue, 25 Jun 2019 12:08:01 +0900
> /proc/sys/net/ipv6/route/skip_notify_on_dev_down assumes given value to be
> 0 or 1. Use proc_dointvec_minmax instead of proc_dointvec.
>
> Fixes: 7c6bb7d2faaf ("net/ipv6: Add knob to skip DELROUTE message ondevice down")
> Signed-off-by: Eiichi Tsukata <devel@etsukata.com>
Applied.
^ permalink raw reply
* Re: [PATCH net] net: make skb_dst_force return false when dst was cleared
From: Florian Westphal @ 2019-06-25 19:59 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Florian Westphal, netdev
In-Reply-To: <8483d4dc-1ef6-20b5-735f-8d78da579a28@gmail.com>
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > -static inline void skb_dst_force(struct sk_buff *skb)
> > +static inline bool skb_dst_force(struct sk_buff *skb)
> > {
> > if (skb_dst_is_noref(skb)) {
> > struct dst_entry *dst = skb_dst(skb);
> > @@ -313,7 +314,10 @@ static inline void skb_dst_force(struct sk_buff *skb)
> > dst = NULL;
> >
> > skb->_skb_refdst = (unsigned long)dst;
> > + return dst != NULL;
> > }
> > +
> > + return true;
>
> This will return true, even if skb has a NULL dst.
Yes, that was intentional -- it should return false to
let caller know that no reference could be obtained and
that the dst was invalidated as a result.
> Say if we have two skb_dst_force() calls for some reason
> on the same skb, only the first one will return false.
What would you suggest instead?
Alternative is something like
if (skb_dst(skb)) {
skb_dst_force(skb);
if (!skb_dst(skb)) {
kfree_skb(skb);
goto err;
}
}
... i find this a bit ugly.
^ permalink raw reply
* Re: hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks
From: David Miller @ 2019-06-25 20:03 UTC (permalink / raw)
To: liuhangbin; +Cc: sukumarg1973, karn, kuznet, yoshfuji, netdev, linux-kernel
In-Reply-To: <20190625061507.GG18865@dhcp-12-139.nay.redhat.com>
From: Hangbin Liu <liuhangbin@gmail.com>
Date: Tue, 25 Jun 2019 14:15:07 +0800
> On Tue, Dec 18, 2018 at 09:55:45PM -0800, David Miller wrote:
>> From: Sukumar Gopalakrishnan <sukumarg1973@gmail.com>
>> Date: Wed, 19 Dec 2018 10:57:02 +0530
>>
>> > Hi David,
>> >
>> > There are two patch for this issue:
>> > 1) Your changes which removes cache_resolve_queue_len
>> > 2) Hangbin's changes which make cache_resolve_queue_len configurable.
>> >
>> > Which one will be chosen for this issue ?
>>
>> I do plan to look into this, sorry for taking so long.
>>
>> Right now I am overwhelmed preparing for the next merge window and
>> synchronizing with other developers for that.
>>
>> Please be patient.
>
> Hi David,
>
> Any progress for this issue?
I have absolutely no context from a discussion that happened back in Dec 2018
If it is important to you, please restart the discussion with a new mailing list
posting restating the problem from the beginning and reiterating all of the
points and arguments that have been made thus far.
^ 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