* [PATCH 16/23] tools lib bpf: Add set/is helpers for all prog types
From: Arnaldo Carvalho de Melo @ 2017-01-25 13:53 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, Joe Stringer, Alexei Starovoitov, Daniel Borkmann,
netdev, Arnaldo Carvalho de Melo
In-Reply-To: <20170125135350.22983-1-acme@kernel.org>
From: Joe Stringer <joe@ovn.org>
These bpf_prog_types were exposed in the uapi but there were no
corresponding functions to set these types for programs in libbpf.
Signed-off-by: Joe Stringer <joe@ovn.org>
Acked-by: Wang Nan <wangnan0@huawei.com>
Cc: Alexei Starovoitov <ast@fb.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: netdev@vger.kernel.org
Link: http://lkml.kernel.org/r/20170123011128.26534-4-joe@ovn.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/lib/bpf/libbpf.c | 5 +++++
tools/lib/bpf/libbpf.h | 10 ++++++++++
2 files changed, 15 insertions(+)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 371cb40a2304..406838fa9c4f 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1448,8 +1448,13 @@ bool bpf_program__is_##NAME(struct bpf_program *prog) \
return bpf_program__is_type(prog, TYPE); \
} \
+BPF_PROG_TYPE_FNS(socket_filter, BPF_PROG_TYPE_SOCKET_FILTER);
BPF_PROG_TYPE_FNS(kprobe, BPF_PROG_TYPE_KPROBE);
+BPF_PROG_TYPE_FNS(sched_cls, BPF_PROG_TYPE_SCHED_CLS);
+BPF_PROG_TYPE_FNS(sched_act, BPF_PROG_TYPE_SCHED_ACT);
BPF_PROG_TYPE_FNS(tracepoint, BPF_PROG_TYPE_TRACEPOINT);
+BPF_PROG_TYPE_FNS(xdp, BPF_PROG_TYPE_XDP);
+BPF_PROG_TYPE_FNS(perf_event, BPF_PROG_TYPE_PERF_EVENT);
int bpf_map__fd(struct bpf_map *map)
{
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index a5a8b86a06fe..2188ccdc0e2d 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -174,11 +174,21 @@ int bpf_program__nth_fd(struct bpf_program *prog, int n);
/*
* Adjust type of bpf program. Default is kprobe.
*/
+int bpf_program__set_socket_filter(struct bpf_program *prog);
int bpf_program__set_tracepoint(struct bpf_program *prog);
int bpf_program__set_kprobe(struct bpf_program *prog);
+int bpf_program__set_sched_cls(struct bpf_program *prog);
+int bpf_program__set_sched_act(struct bpf_program *prog);
+int bpf_program__set_xdp(struct bpf_program *prog);
+int bpf_program__set_perf_event(struct bpf_program *prog);
+bool bpf_program__is_socket_filter(struct bpf_program *prog);
bool bpf_program__is_tracepoint(struct bpf_program *prog);
bool bpf_program__is_kprobe(struct bpf_program *prog);
+bool bpf_program__is_sched_cls(struct bpf_program *prog);
+bool bpf_program__is_sched_act(struct bpf_program *prog);
+bool bpf_program__is_xdp(struct bpf_program *prog);
+bool bpf_program__is_perf_event(struct bpf_program *prog);
/*
* We don't need __attribute__((packed)) now since it is
--
2.9.3
^ permalink raw reply related
* [PATCH 17/23] tools lib bpf: Add libbpf_get_error()
From: Arnaldo Carvalho de Melo @ 2017-01-25 13:53 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, Joe Stringer, Alexei Starovoitov, Daniel Borkmann,
netdev, Arnaldo Carvalho de Melo
In-Reply-To: <20170125135350.22983-1-acme@kernel.org>
From: Joe Stringer <joe@ovn.org>
This function will turn a libbpf pointer into a standard error code (or
0 if the pointer is valid).
This also allows removal of the dependency on linux/err.h in the public
header file, which causes problems in userspace programs built against
libbpf.
Signed-off-by: Joe Stringer <joe@ovn.org>
Acked-by: Wang Nan <wangnan0@huawei.com>
Cc: Alexei Starovoitov <ast@fb.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: netdev@vger.kernel.org
Link: http://lkml.kernel.org/r/20170123011128.26534-5-joe@ovn.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/lib/bpf/libbpf.c | 8 ++++++++
tools/lib/bpf/libbpf.h | 4 +++-
tools/perf/tests/llvm.c | 2 +-
3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 406838fa9c4f..e6cd62b1264b 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -28,6 +28,7 @@
#include <fcntl.h>
#include <errno.h>
#include <asm/unistd.h>
+#include <linux/err.h>
#include <linux/kernel.h>
#include <linux/bpf.h>
#include <linux/list.h>
@@ -1542,3 +1543,10 @@ bpf_object__find_map_by_offset(struct bpf_object *obj, size_t offset)
}
return ERR_PTR(-ENOENT);
}
+
+long libbpf_get_error(const void *ptr)
+{
+ if (IS_ERR(ptr))
+ return PTR_ERR(ptr);
+ return 0;
+}
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 2188ccdc0e2d..4014d1ba5e3d 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -22,8 +22,8 @@
#define __BPF_LIBBPF_H
#include <stdio.h>
+#include <stdint.h>
#include <stdbool.h>
-#include <linux/err.h>
#include <sys/types.h> // for size_t
enum libbpf_errno {
@@ -234,4 +234,6 @@ int bpf_map__set_priv(struct bpf_map *map, void *priv,
bpf_map_clear_priv_t clear_priv);
void *bpf_map__priv(struct bpf_map *map);
+long libbpf_get_error(const void *ptr);
+
#endif
diff --git a/tools/perf/tests/llvm.c b/tools/perf/tests/llvm.c
index 02a33ebcd992..d357dab72e68 100644
--- a/tools/perf/tests/llvm.c
+++ b/tools/perf/tests/llvm.c
@@ -13,7 +13,7 @@ static int test__bpf_parsing(void *obj_buf, size_t obj_buf_sz)
struct bpf_object *obj;
obj = bpf_object__open_buffer(obj_buf, obj_buf_sz, NULL);
- if (IS_ERR(obj))
+ if (libbpf_get_error(obj))
return TEST_FAIL;
bpf_object__close(obj);
return TEST_OK;
--
2.9.3
^ permalink raw reply related
* Re: [PATCH net v2 3/4] r8152: re-schedule napi for tx
From: Eric Dumazet @ 2017-01-25 13:57 UTC (permalink / raw)
To: Hayes Wang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <1394712342-15778-245-Taiwan-albertk@realtek.com>
On Wed, 2017-01-25 at 16:13 +0800, Hayes Wang wrote:
> Re-schedule napi after napi_complete() for tx, if it is necessay.
>
> In r8152_poll(), if the tx is completed after tx_bottom() and before
> napi_complete(), the scheduling of napi would be lost. Then, no
> one handles the next tx until the next napi_schedule() is called.
>
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
> drivers/net/usb/r8152.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index ec882be..45d168e 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -1936,6 +1936,9 @@ static int r8152_poll(struct napi_struct *napi, int budget)
> napi_complete(napi);
> if (!list_empty(&tp->rx_done))
> napi_schedule(napi);
> + else if (!skb_queue_empty(&tp->tx_queue) &&
> + !list_empty(&tp->tx_free))
> + napi_schedule(&tp->napi);
Why using &tp->napi instead of napi here, as done 3 lines above ?
^ permalink raw reply
* [PATCH net] net: dsa: Mop up remaining NET_DSA_HWMON references
From: Andrew Lunn @ 2017-01-25 14:04 UTC (permalink / raw)
To: David Miller; +Cc: netdev, valentinrothberg, Andrew Lunn
Previous patches have moved the temperature sensor code into the
Marvell PHYs. A few now dead references to NET_DSA_HWMON were left
behind. Go reap them.
Reported-by: Valentin Rothberg <valentinrothberg@gmail.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
Documentation/networking/dsa/dsa.txt | 24 ------------------------
include/net/dsa.h | 8 --------
2 files changed, 32 deletions(-)
diff --git a/Documentation/networking/dsa/dsa.txt b/Documentation/networking/dsa/dsa.txt
index 63912ef34606..b8b40753133e 100644
--- a/Documentation/networking/dsa/dsa.txt
+++ b/Documentation/networking/dsa/dsa.txt
@@ -295,7 +295,6 @@ DSA currently leverages the following subsystems:
- MDIO/PHY library: drivers/net/phy/phy.c, mdio_bus.c
- Switchdev: net/switchdev/*
- Device Tree for various of_* functions
-- HWMON: drivers/hwmon/*
MDIO/PHY library
----------------
@@ -349,12 +348,6 @@ Documentation/devicetree/bindings/net/dsa/dsa.txt. PHY/MDIO library helper
functions such as of_get_phy_mode(), of_phy_connect() are also used to query
per-port PHY specific details: interface connection, MDIO bus location etc..
-HWMON
------
-
-Some switch drivers feature internal temperature sensors which are exposed as
-regular HWMON devices in /sys/class/hwmon/.
-
Driver development
==================
@@ -495,23 +488,6 @@ Power management
BR_STATE_DISABLED and propagating changes to the hardware if this port is
disabled while being a bridge member
-Hardware monitoring
--------------------
-
-These callbacks are only available if CONFIG_NET_DSA_HWMON is enabled:
-
-- get_temp: this function queries the given switch for its temperature
-
-- get_temp_limit: this function returns the switch current maximum temperature
- limit
-
-- set_temp_limit: this function configures the maximum temperature limit allowed
-
-- get_temp_alarm: this function returns the critical temperature threshold
- returning an alarm notification
-
-See Documentation/hwmon/sysfs-interface for details.
-
Bridge layer
------------
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 9d6cd923c48c..08b340403927 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -178,14 +178,6 @@ struct dsa_switch {
*/
s8 rtable[DSA_MAX_SWITCHES];
-#ifdef CONFIG_NET_DSA_HWMON
- /*
- * Hardware monitoring information
- */
- char hwmon_name[IFNAMSIZ + 8];
- struct device *hwmon_dev;
-#endif
-
/*
* The lower device this switch uses to talk to the host
*/
--
2.11.0
^ permalink raw reply related
* Re: NAPI on USB network drivers
From: Eric Dumazet @ 2017-01-25 14:13 UTC (permalink / raw)
To: Hayes Wang
Cc: Oliver Neukum, netdev@vger.kernel.org, linux-usb@vger.kernel.org
In-Reply-To: <0835B3720019904CB8F7AA43166CEEB201A134A7@RTITMBSV06.realtek.com.tw>
On Wed, 2017-01-25 at 09:39 +0000, Hayes Wang wrote:
> Oliver Neukum [mailto:oneukum@suse.com]
> > Sent: Wednesday, January 25, 2017 5:35 PM
> [...]
> > looking at r8152 I noticed that it uses NAPI. I never considered
> > this for the generic USB networking code as you cannot disable
> > interrupts for USB. Is it still worth it? What are the benefits?
>
> You could use napi_gro_receive() and it influences the performance.
You also could use napi_complete_done() instead of napi_complete(), as
it allows users to tune the performance vs latency for GRO.
Looking at this driver, I do not see any limitation on the number of
skbs that can be pushed into tp->rx_queue.
I wonder if this queue can end up consuming all memory of a host under
stress.
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index e1466b4d2b6c727148a884672bbd9593bf04b3ac..221df4a931b5c1073f1922d0fa0bbff158c73b7d 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1840,7 +1840,10 @@ static int rx_bottom(struct r8152 *tp, int budget)
stats->rx_packets++;
stats->rx_bytes += pkt_len;
} else {
- __skb_queue_tail(&tp->rx_queue, skb);
+ if (unlikely(skb_queue_len(&tp->rx_queue) >= 1000))
+ kfree_skb(skb);
+ else
+ __skb_queue_tail(&tp->rx_queue, skb);
}
find_next_rx:
^ permalink raw reply related
* Re: TCP stops sending packets over loopback on 4.10-rc3?
From: Josef Bacik @ 2017-01-25 14:14 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Linux Netdev List, Lawrence Brakmo, Kernel Team
In-Reply-To: <1485266868.16328.292.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, Jan 24, 2017 at 9:07 AM, Eric Dumazet <eric.dumazet@gmail.com>
wrote:
> On Tue, 2017-01-24 at 06:20 -0500, Josef Bacik wrote:
>> Hello,
>>
>> I've been trying to test some NBD changes I had made recently and I
>> started having packet timeouts. I traced this down to tcp just
>> stopping sending packets after a lot of writing. All NBD does is
>> call
>> kernel_sendmsg() with a request struct and some pages when it does
>> writes. I did a bunch of tracing and I've narrowed it down to
>> running
>> out of sk_wmem_queued space. In tcp_sendmsg() here
>>
>> new_segment:
>> /* Allocate new segment. If the interface
>> is SG,
>> * allocate skb fitting to single page.
>> */
>> if (!sk_stream_memory_free(sk))
>> goto wait_for_sndbuf;
>>
>> we hit this pretty regularly, and eventually just get stuck in
>> sk_stream_wait_memory until the timeout ends and we error out
>> everything. Now sk_stream_memory_free checks the sk_wmem_queued and
>> calls into the sk_prot->stream_memory_free(), so I broke this out
>> like
>> the following
>>
>>
>> if (sk->sk_wmem_queued >= sk->sk_sndbuf) {
>> trace_printk("sk_wmem_queued %d, sk_sndbuf %d\n",
>> sk->sk_wmem_queued, sk->sk_sndbuf);
>> goto wait_for_sndbuf;
>> }
>> if (sk->sk_prot->stream_memory_free &&
>> !sk->sk_prot->stream_memory_free(sk)) {
>> trace_printk("sk_stream_memory_free\n");
>> goto wait_for_sndbuf;
>> }
>>
>> And I got this in my tracing
>>
>> kworker/u16:5-112 [001] .... 1375.637564: tcp_sendmsg:
>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>> kworker/u16:5-112 [001] .... 1375.639657: tcp_sendmsg:
>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>> kworker/u16:5-112 [003] .... 1375.641128: tcp_sendmsg:
>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>> kworker/u16:5-112 [003] .... 1375.643441: tcp_sendmsg:
>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>> kworker/u16:5-112 [001] .... 1375.807614: tcp_sendmsg:
>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>> kworker/u16:5-112 [001] .... 1377.538744: tcp_sendmsg:
>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>> kworker/u16:5-112 [001] .... 1377.543418: tcp_sendmsg:
>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>> kworker/2:4H-1535 [002] .... 1377.544685: tcp_sendmsg:
>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>> kworker/u16:5-112 [000] .... 1379.378352: tcp_sendmsg:
>> sk_wmem_queued 4205796, sk_sndbuf 4194304
>> kworker/u16:5-112 [003] .... 1380.985721: tcp_sendmsg:
>> sk_wmem_queued 4212416, sk_sndbuf 4194304
>>
>> This is as far as I've gotten and I'll keep digging into it, but I
>> was
>> wondering if this looks familiar to anybody? Also one thing I've
>> noticed is sk_stream_wait_memory() will wait on sk_sleep(sk), but
>> basically nothing wakes this up. For example it seems the main way
>> we
>> reduce sk_wmem_queued is through sk_wmem_free_skb(), which doesn't
>> appear to wake anything up in any of its callers, so anybody who
>> does
>> end up sleeping will basically never wake up. That seems like it
>> should be more broken than it is, so I'm curious to know how things
>> are
>> actually woken up in this case. Thanks,
>
>
> git grep -n SOCK_QUEUE_SHRUNK
>
> -> tcp_check_space()
But tcp_check_space() doesn't actually reduce sk_wmem_queued from what
I can see. The only places that appear to reduce it are tcp_trim_head,
which is only called in the retransmit path, and sk_wmem_free_skb,
which seems to be right, but I added a trace_printk() in it to see if
it was firing during my test and it never fires. So we _appear_ to
only ever be incrementing this counter, but never decrementing it. I'm
doing a bunch of tracing trying to figure out what is going on here but
so far nothing is popping which is starting to make me think ftrace is
broken. Thanks,
Josef
^ permalink raw reply
* Re: [PATCH v2] virtio_net: fix PAGE_SIZE > 64k
From: Michael S. Tsirkin @ 2017-01-25 14:23 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: netdev@vger.kernel.org, John Fastabend,
linux-kernel@vger.kernel.org, virtualization, David Miller
In-Reply-To: <CAADnVQ+ioJ7kgsFeFoSV9Nqnq9CkRGMmqS5QM66gsJG01-Rojw@mail.gmail.com>
On Tue, Jan 24, 2017 at 08:07:40PM -0800, Alexei Starovoitov wrote:
> On Tue, Jan 24, 2017 at 7:48 PM, John Fastabend
> <john.fastabend@gmail.com> wrote:
> >
> > It is a concern on my side. I want XDP and Linux stack to work
> > reasonably well together.
>
> btw the micro benchmarks showed that page per packet approach
> that xdp took in mlx4 should be 10% slower vs normal operation
> for tcp/ip stack.
Interesting. TCP only or UDP too? What's the packet size? Are you tuning
your rmem limits at all? The slowdown would be more noticeable with
UDP with default values and small packet sizes.
> We thought that for our LB use case
> it will be an acceptable slowdown, but turned out that overall we
> got a performance boost, since xdp model simplified user space
> and got data path faster, so we magically got extra free cpu
> that is used for other apps on the same host and overall
> perf win despite extra overhead in tcp/ip.
> Not all use cases are the same and not everyone will be as lucky,
> so I'd like to see performance of xdp_pass improving too, though
> it turned out to be not as high priority as I initially estimated.
^ permalink raw reply
* Re: TCP stops sending packets over loopback on 4.10-rc3?
From: Eric Dumazet @ 2017-01-25 14:25 UTC (permalink / raw)
To: Josef Bacik; +Cc: Linux Netdev List, Lawrence Brakmo, Kernel Team
In-Reply-To: <1485353664.5902.2@smtp.office365.com>
On Wed, 2017-01-25 at 09:14 -0500, Josef Bacik wrote:
> On Tue, Jan 24, 2017 at 9:07 AM, Eric Dumazet <eric.dumazet@gmail.com>
> >
> > git grep -n SOCK_QUEUE_SHRUNK
> >
> > -> tcp_check_space()
>
> But tcp_check_space() doesn't actually reduce sk_wmem_queued from what
> I can see. The only places that appear to reduce it are tcp_trim_head,
> which is only called in the retransmit path, and sk_wmem_free_skb,
> which seems to be right,
This is exactly how it works.
We free a bunch of skbs (an ACK can acknowledge dozens of them), and set
the SOCK_QUEUE_SHRUNK.
Then later, tcp_check_space() is called once and check if the bit was
set by a prior call to tcp_trim_head() or full skb freeing.
> but I added a trace_printk() in it to see if
> it was firing during my test and it never fires. So we _appear_ to
> only ever be incrementing this counter, but never decrementing it. I'm
> doing a bunch of tracing trying to figure out what is going on here but
> so far nothing is popping which is starting to make me think ftrace is
> broken. Thanks,
>
Just to make sure, are you telling use native/standard TCP is broken
over loopback, or is that only when using an additional kernel module ?
^ permalink raw reply
* Re: TCP stops sending packets over loopback on 4.10-rc3?
From: Josef Bacik @ 2017-01-25 14:26 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Linux Netdev List, Lawrence Brakmo, Kernel Team
In-Reply-To: <1485353664.5902.2@smtp.office365.com>
On Wed, Jan 25, 2017 at 9:14 AM, Josef Bacik <jbacik@fb.com> wrote:
> On Tue, Jan 24, 2017 at 9:07 AM, Eric Dumazet
> <eric.dumazet@gmail.com> wrote:
>> On Tue, 2017-01-24 at 06:20 -0500, Josef Bacik wrote:
>>> Hello,
>>>
>>> I've been trying to test some NBD changes I had made recently and I
>>> started having packet timeouts. I traced this down to tcp just
>>> stopping sending packets after a lot of writing. All NBD does is
>>> call
>>> kernel_sendmsg() with a request struct and some pages when it does
>>> writes. I did a bunch of tracing and I've narrowed it down to
>>> running
>>> out of sk_wmem_queued space. In tcp_sendmsg() here
>>>
>>> new_segment:
>>> /* Allocate new segment. If the interface
>>> is SG,
>>> * allocate skb fitting to single page.
>>> */
>>> if (!sk_stream_memory_free(sk))
>>> goto wait_for_sndbuf;
>>>
>>> we hit this pretty regularly, and eventually just get stuck in
>>> sk_stream_wait_memory until the timeout ends and we error out
>>> everything. Now sk_stream_memory_free checks the sk_wmem_queued
>>> and
>>> calls into the sk_prot->stream_memory_free(), so I broke this out
>>> like
>>> the following
>>>
>>>
>>> if (sk->sk_wmem_queued >= sk->sk_sndbuf) {
>>> trace_printk("sk_wmem_queued %d, sk_sndbuf %d\n",
>>> sk->sk_wmem_queued, sk->sk_sndbuf);
>>> goto wait_for_sndbuf;
>>> }
>>> if (sk->sk_prot->stream_memory_free &&
>>> !sk->sk_prot->stream_memory_free(sk)) {
>>> trace_printk("sk_stream_memory_free\n");
>>> goto wait_for_sndbuf;
>>> }
>>>
>>> And I got this in my tracing
>>>
>>> kworker/u16:5-112 [001] .... 1375.637564: tcp_sendmsg:
>>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>>> kworker/u16:5-112 [001] .... 1375.639657: tcp_sendmsg:
>>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>>> kworker/u16:5-112 [003] .... 1375.641128: tcp_sendmsg:
>>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>>> kworker/u16:5-112 [003] .... 1375.643441: tcp_sendmsg:
>>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>>> kworker/u16:5-112 [001] .... 1375.807614: tcp_sendmsg:
>>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>>> kworker/u16:5-112 [001] .... 1377.538744: tcp_sendmsg:
>>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>>> kworker/u16:5-112 [001] .... 1377.543418: tcp_sendmsg:
>>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>>> kworker/2:4H-1535 [002] .... 1377.544685: tcp_sendmsg:
>>> sk_wmem_queued 4204872, sk_sndbuf 4194304
>>> kworker/u16:5-112 [000] .... 1379.378352: tcp_sendmsg:
>>> sk_wmem_queued 4205796, sk_sndbuf 4194304
>>> kworker/u16:5-112 [003] .... 1380.985721: tcp_sendmsg:
>>> sk_wmem_queued 4212416, sk_sndbuf 4194304
>>>
>>> This is as far as I've gotten and I'll keep digging into it, but I
>>> was
>>> wondering if this looks familiar to anybody? Also one thing I've
>>> noticed is sk_stream_wait_memory() will wait on sk_sleep(sk), but
>>> basically nothing wakes this up. For example it seems the main
>>> way we
>>> reduce sk_wmem_queued is through sk_wmem_free_skb(), which doesn't
>>> appear to wake anything up in any of its callers, so anybody who
>>> does
>>> end up sleeping will basically never wake up. That seems like it
>>> should be more broken than it is, so I'm curious to know how
>>> things are
>>> actually woken up in this case. Thanks,
>>
>>
>> git grep -n SOCK_QUEUE_SHRUNK
>>
>> -> tcp_check_space()
>
> But tcp_check_space() doesn't actually reduce sk_wmem_queued from
> what I can see. The only places that appear to reduce it are
> tcp_trim_head, which is only called in the retransmit path, and
> sk_wmem_free_skb, which seems to be right, but I added a
> trace_printk() in it to see if it was firing during my test and it
> never fires. So we _appear_ to only ever be incrementing this
> counter, but never decrementing it. I'm doing a bunch of tracing
> trying to figure out what is going on here but so far nothing is
> popping which is starting to make me think ftrace is broken. Thanks,
Nope ftrace isn't broken, I'm just dumb, the space is being reclaimed
by sk_wmem_free_skb(). So I guess I need to figure out why I stop
getting ACK's from the other side of the loopback. Thanks,
Josef
^ permalink raw reply
* Re: TCP stops sending packets over loopback on 4.10-rc3?
From: Eric Dumazet @ 2017-01-25 14:39 UTC (permalink / raw)
To: Josef Bacik; +Cc: Linux Netdev List, Lawrence Brakmo, Kernel Team
In-Reply-To: <1485354408.5902.3@smtp.office365.com>
On Wed, 2017-01-25 at 09:26 -0500, Josef Bacik wrote:
> Nope ftrace isn't broken, I'm just dumb, the space is being reclaimed
> by sk_wmem_free_skb(). So I guess I need to figure out why I stop
> getting ACK's from the other side of the loopback. Thanks,
ss -temoi dst 127.0.0.1
Might give you some hints, like packets being dropped.
ACK can be delayed if the reader is slow to consume bytes.
^ permalink raw reply
* Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive
From: Michael S. Tsirkin @ 2017-01-25 14:45 UTC (permalink / raw)
To: John Fastabend
Cc: Jason Wang, David Miller, john.r.fastabend, netdev,
alexei.starovoitov, daniel
In-Reply-To: <58882355.70307@gmail.com>
On Tue, Jan 24, 2017 at 08:02:29PM -0800, John Fastabend wrote:
> On 17-01-24 07:23 PM, Michael S. Tsirkin wrote:
> > On Wed, Jan 25, 2017 at 10:57:12AM +0800, Jason Wang wrote:
> >>
> >>
> >> On 2017年01月25日 04:08, Michael S. Tsirkin wrote:
> >>> On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
> >>>> From: "Michael S. Tsirkin" <mst@redhat.com>
> >>>> Date: Mon, 23 Jan 2017 23:08:35 +0200
> >>>>
> >>>>> On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
> >>>>>> In the small buffer case during driver unload we currently use
> >>>>>> put_page instead of dev_kfree_skb. Resolve this by adding a check
> >>>>>> for virtnet mode when checking XDP queue type. Also name the
> >>>>>> function so that the code reads correctly to match the additional
> >>>>>> check.
> >>>>>>
> >>>>>> Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
> >>>>>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> >>>>>> Acked-by: Jason Wang <jasowang@redhat.com>
> >>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >>>>>
> >>>>> I think we definitely want this one in -net as it's
> >>>>> a bugfix.
> >>>> This whole series is a bug fix, we must have adjust_header XDP
> >>>> support in the virtio_net driver before v4.10 goes out, it is
> >>>> a requires base feature for XDP.
> >>> I have to say device resets outside probe have a huge potential
> >>> to uncover hypervisor bugs.
> >>
> >> Maybe not if it reuses most of current codes? Since we've already used them
> >> in sleep or hibernation?
> >>
> >> Thanks
> >
> > Except almost no one uses sleep or hybernate with VMs. I'm not saying
> > it's a bad idea, just that it needs a lot of testing before release and
> > we won't get enough if we merge at this point.
> >
>
> Then it would seem like a good thing to have another user of these paths and
> find the bugs versus letting them sit there for some poor folks who do use
> sleep/hybernate.
Absolutely. But -rc6 is not the time to test waters IMO.
> >>> I am rather uncomfortable
> >>> doing that after -rc1.
> >>>
> >>> How about a module option to disable it by default?
> >>> We can then ship a partial implementation in 4.10
> >>> and work on completing it in 4.11.
> >>>
>
> Ugh I would prefer to avoid module options. This will only happen if users
> push XDP program into driver anyways.
Again I agree, it's an idea for a stopgap measure so we can have
something in 4.10 - and also assuming that 256b headroom is a must.
> >
> > To clarify, I'm thinking an option similar to enable_xdp,
> > and have all packets have a 256 byte headroom for 4.10.
>
> An option where? In QEMU side, in driver? Is the reset really that bad, coming
> from a hardware driver side lots of configuration changes can cause resets. I
> agree its not overly elegant but could follow on patches be used to make it
> prettier if possible.
Again I agree and it's not that bad it's just not something we should
do past rc5.
> I know folks prefer to avoid tuning knobs but I think exposing the headroom
> configuration to users might not be a bad idea. After all these same users are
> already programming maps and ebpf codes. A simple tuning knob should not be a
> big deal and reasonable defaults would of course be used. That is a net-next
> debate though.
No arguments from my side here.
> >
> > Consider our options for 4.11.
> >
>
> Finally just to point out here are the drivers with XDP support on latest
> net tree,
>
> mlx/mlx5
> mlx/mlx4
> qlogic/qede
> netronome/nfp
> virtio_net
>
> And here is the list of adjust header support,
>
> mlx/mlx4
Above seems to imply an interface for userspace to detect the amount
of head space would be benefitial.
>
> So we currently have the same feature gap on all the other drivers except one.
> Although I do not think that is a very good excuse. Lets figure out what we
> should do about virtio.
>
> Thanks,
> John
If we can simply defer adjust_head patches to 4.11 then that's fine.
--
MST
^ permalink raw reply
* Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive
From: Michael S. Tsirkin @ 2017-01-25 14:47 UTC (permalink / raw)
To: Jason Wang
Cc: John Fastabend, David Miller, john.r.fastabend, netdev,
alexei.starovoitov, daniel
In-Reply-To: <46915782-1c3f-04a9-ae0a-84e5f805af1f@redhat.com>
On Wed, Jan 25, 2017 at 01:46:46PM +0800, Jason Wang wrote:
> > Then it would seem like a good thing to have another user of these paths and
> > find the bugs versus letting them sit there for some poor folks who do use
> > sleep/hybernate.
> >
>
> Yes, and uncovering hypervisor bugs now is better than uncovering it in the
> future.
>
> Thanks
Not really, all the uncovering should happen in -next or early rc.
Right now we need to fix what has been uncovered so far.
--
MST
^ permalink raw reply
* Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive
From: Michael S. Tsirkin @ 2017-01-25 14:52 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: John Fastabend, Jason Wang, David Miller, John Fastabend,
netdev@vger.kernel.org, Daniel Borkmann
In-Reply-To: <CAADnVQ+O1tNuktnPgkL7JHZFBaQbWfxq0rjxPn86rsbRcteRyg@mail.gmail.com>
On Tue, Jan 24, 2017 at 11:33:56PM -0800, Alexei Starovoitov wrote:
> On Tue, Jan 24, 2017 at 8:02 PM, John Fastabend
> <john.fastabend@gmail.com> wrote:
> >
> > Finally just to point out here are the drivers with XDP support on latest
> > net tree,
> >
> > mlx/mlx5
> > mlx/mlx4
> > qlogic/qede
> > netronome/nfp
> > virtio_net
> >
> > And here is the list of adjust header support,
> >
> > mlx/mlx4
> >
>
> in net-next it's actually:
> yes: mlx4, mlx5
> no: qede, nfp, virtio
> while nfp and virtio are working on it.
>
> xdp_adjust_head() is must have for load balancer,
What amount of head space does it need? 70 bytes
to do vxlan kind of thing?
> so the sooner it lands for virtio the easier it will be
> to develop xdp programs. Initially I expected
> e1k+xdp to be the base line for debugging and
> development of xdp programs, but since not everyone
> agreed on e1k the virtio+xdp filled in the gap.
> So without adjust_head in virtio I see very little use for it
> in our environment.
> It is a must have feature regardless of timing.
> I will backport whatever is necessary, but distros
> will stick with official releases and imo it's not great
> from xdp adoption point of view to have
> virtio driver lacking key features.
If everyone can agree it's net-next material then I'm happy.
--
MST
^ permalink raw reply
* RE: [Xen-devel] xennet_start_xmit assumptions
From: Paul Durrant @ 2017-01-25 15:06 UTC (permalink / raw)
To: Sowmini Varadhan
Cc: Konrad Rzeszutek Wilk, Wei Liu, netdev@vger.kernel.org,
xen-devel@lists.xenproject.org
In-Reply-To: <20170119111426.GA22018@oracle.com>
> -----Original Message-----
> From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
> Sent: 19 January 2017 11:14
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Wei Liu
> <wei.liu2@citrix.com>; netdev@vger.kernel.org; xen-
> devel@lists.xenproject.org
> Subject: Re: [Xen-devel] xennet_start_xmit assumptions
>
> On (01/19/17 09:36), Paul Durrant wrote:
> >
> > Hi Sowmini,
> >
> > Sounds like a straightforward bug to me... netfront should be able
> > to handle an empty skb and clearly, if it's relying on skb_headlen()
> > being non-zero, that's not the case.
> >
> > Paul
>
> I see. Seems like there are 2 things broken here: recovering
> from skb->len = 0, and recovering from the more complex
> case of (skb->len > 0 && skb_headlen(skb) == 0)
>
> Do you folks want to take a shot at fixing this,
> since you know the code better? If you are interested,
> I can share my test program to help you reproduce the
> simpler skb->len == 0 case, but it's the fully non-linear
> skbs that may be more interesting to reproduce/fix.
>
> I'll probably work on fixing packet_snd to return -EINVAL
> or similar when the len is zero this week.
>
Sowmini,
I knocked together the following patch, which seems to work for me:
---8<---
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 40f26b6..a957c89 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -567,6 +567,10 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
u16 queue_index;
struct sk_buff *nskb;
+ /* Drop packets that are not at least ETH_HLEN in length */
+ if (skb->len < ETH_HLEN)
+ goto drop;
+
/* Drop the packet if no queues are set up */
if (num_queues < 1)
goto drop;
@@ -609,6 +613,8 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
}
len = skb_headlen(skb);
+ if ((len < ETH_HLEN) && !__pskb_pull_tail(skb, ETH_HLEN))
+ goto drop;
spin_lock_irqsave(&queue->tx_lock, flags);
---8<---
Making netfront cope with a fully non-linear skb looks like it would be quite intrusive and probably not worth it so I opted for just doing the ETH_HLEN pull-tail if necessary. Can you check it works for you?
Paul
^ permalink raw reply related
* Re: [PATCH net] sctp: sctp_addr_id2transport should verify the addr before looking up assoc
From: Neil Horman @ 2017-01-25 15:10 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner
In-Reply-To: <2ba9d3e0b3aa05baa3791811c92248126d233c67.1485237713.git.lucien.xin@gmail.com>
On Tue, Jan 24, 2017 at 02:01:53PM +0800, Xin Long wrote:
> sctp_addr_id2transport is a function for sockopt to look up assoc by
> address. As the address is from userspace, it can be a v4-mapped v6
> address. But in sctp protocol stack, it always handles a v4-mapped
> v6 address as a v4 address. So it's necessary to convert it to a v4
> address before looking up assoc by address.
>
> This patch is to fix it by calling sctp_verify_addr in which it can do
> this conversion before calling sctp_endpoint_lookup_assoc, just like
> what sctp_sendmsg and __sctp_connect do for the address from users.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/sctp/socket.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 318c678..37eeab7 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -235,8 +235,12 @@ static struct sctp_transport *sctp_addr_id2transport(struct sock *sk,
> sctp_assoc_t id)
> {
> struct sctp_association *addr_asoc = NULL, *id_asoc = NULL;
> - struct sctp_transport *transport;
> + struct sctp_af *af = sctp_get_af_specific(addr->ss_family);
> union sctp_addr *laddr = (union sctp_addr *)addr;
> + struct sctp_transport *transport;
> +
> + if (sctp_verify_addr(sk, laddr, af->sockaddr_len))
> + return NULL;
>
> addr_asoc = sctp_endpoint_lookup_assoc(sctp_sk(sk)->ep,
> laddr,
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH net] sctp: sctp_addr_id2transport should verify the addr before looking up assoc
From: Xin Long @ 2017-01-25 15:34 UTC (permalink / raw)
To: Vladislav Yasevich
Cc: network dev, linux-sctp@vger.kernel.org, David Miller,
Marcelo Ricardo Leitner, Neil Horman
In-Reply-To: <CAGCdqXEAkbeMPhVA5N_vYV_k_54faX_0JWui5kSrZBbsmms7jg@mail.gmail.com>
On Wed, Jan 25, 2017 at 11:27 PM, Vladislav Yasevich
<vyasevich@gmail.com> wrote:
> On Tue, Jan 24, 2017 at 1:01 AM, Xin Long <lucien.xin@gmail.com> wrote:
>>
>> sctp_addr_id2transport is a function for sockopt to look up assoc by
>> address. As the address is from userspace, it can be a v4-mapped v6
>> address. But in sctp protocol stack, it always handles a v4-mapped
>> v6 address as a v4 address. So it's necessary to convert it to a v4
>> address before looking up assoc by address.
>>
>> This patch is to fix it by calling sctp_verify_addr in which it can do
>> this conversion before calling sctp_endpoint_lookup_assoc, just like
>> what sctp_sendmsg and __sctp_connect do for the address from users.
>>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>> net/sctp/socket.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 318c678..37eeab7 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -235,8 +235,12 @@ static struct sctp_transport
>> *sctp_addr_id2transport(struct sock *sk,
>> sctp_assoc_t id)
>> {
>> struct sctp_association *addr_asoc = NULL, *id_asoc = NULL;
>> - struct sctp_transport *transport;
>> + struct sctp_af *af = sctp_get_af_specific(addr->ss_family);
>> union sctp_addr *laddr = (union sctp_addr *)addr;
>> + struct sctp_transport *transport;
>> +
>> + if (sctp_verify_addr(sk, laddr, af->sockaddr_len))
>> + return NULL;
>>
>
> This causes a side-effect such that GET options will end up with ipv4
> address instead
> of a v4mapped address that was passed in.
not really
(more below)
>
> -vlad
>
>>
>> addr_asoc = sctp_endpoint_lookup_assoc(sctp_sk(sk)->ep,
>> laddr,
sctp_get_pf_specific(sk->sk_family)->addr_to_user(sctp_sk(sk),
(union sctp_addr *)addr);
here it will convert it back to v4mapped v6 address.
>> --
>> 2.1.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply
* Re: [Xen-devel] xennet_start_xmit assumptions
From: Sowmini Varadhan @ 2017-01-25 15:45 UTC (permalink / raw)
To: Paul Durrant
Cc: Konrad Rzeszutek Wilk, Wei Liu, netdev@vger.kernel.org,
xen-devel@lists.xenproject.org
In-Reply-To: <fa531e2ed014488baa10556c724e176c@AMSPEX02CL03.citrite.net>
On (01/25/17 15:06), Paul Durrant wrote:
>
> Making netfront cope with a fully non-linear skb looks like it would
> be quite intrusive and probably not worth it so I opted for just doing
> the ETH_HLEN pull-tail if necessary. Can you check it works for you?
I tested it, and it works fine, but note that DaveM's comments in
this thread: the DKI is that we *must* provide at least the hard_header_len
in the non-paged part of the skb. So might not even be necessary to handle
the fully non-linear skb (though it's probably prudent to check
and bail for this, as your patch does)
I just posted an RFC patch for fixing the pf_packet layer,
just in case other drivers like xen_netfront dont explicitly
check for this
http://patchwork.ozlabs.org/patch/719236/
^ permalink raw reply
* Re: ip link SR-IOV VF MAC address disparity
From: Greg @ 2017-01-25 15:49 UTC (permalink / raw)
To: Leon Goldberg; +Cc: netdev, Dan Kenigsberg, Edward Haas
In-Reply-To: <CA+mNUtCquJF9TxTRs9GF7XdBhxDUS_LwWA8M5QZbeXoRGNrZ2A@mail.gmail.com>
On Wed, 2017-01-25 at 15:34 +0200, Leon Goldberg wrote:
> Hey,
>
> Using ip link to retrieve the MAC addresses of some SR-IOV virtual
> functions, I'm receiving mixed results:
>
> [root@nari05 sys]# ip link
> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT
> link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
> 2: enp2s0f0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq
> master ovirtmgmt state UP mode DEFAULT qlen 1000
> link/ether 78:e7:d1:e4:9b:64 brd ff:ff:ff:ff:ff:ff
> 3: enp2s0f1: <BROADCAST,MULTICAST> mtu 1500 qdisc mq master test1
> state DOWN mode DEFAULT qlen 1000
> link/ether 78:e7:d1:e4:9b:65 brd ff:ff:ff:ff:ff:ff
> vf 0 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
> vf 1 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
> vf 2 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
> vf 3 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
> vf 4 MAC 00:00:00:00:00:00, spoof checking on, link-state auto
> 4: ovirtmgmt: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue
> state UP mode DEFAULT
> link/ether 78:e7:d1:e4:9b:64 brd ff:ff:ff:ff:ff:ff
> 5: test1: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue
> state DOWN mode DEFAULT
> link/ether 78:e7:d1:e4:9b:65 brd ff:ff:ff:ff:ff:ff
> 11: ;vdsmdummy;: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
> mode DEFAULT
> link/ether 5e:b4:ac:5c:b9:a1 brd ff:ff:ff:ff:ff:ff
> 37: enp2s16f1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
> mode DEFAULT qlen 1000
> link/ether 00:00:00:00:00:02 brd ff:ff:ff:ff:ff:ff
> 38: enp2s16f3: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
> mode DEFAULT qlen 1000
> link/ether d6:ee:45:57:c0:39 brd ff:ff:ff:ff:ff:ff
> 39: enp2s16f5: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
> mode DEFAULT qlen 1000
> link/ether 4a:2c:25:42:97:4a brd ff:ff:ff:ff:ff:ff
> 40: enp2s16f7: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
> mode DEFAULT qlen 1000
> link/ether c2:fe:2f:5e:f5:e8 brd ff:ff:ff:ff:ff:ff
> 41: enp2s17f1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
> mode DEFAULT qlen 1000
> link/ether e6:31:a9:59:5f:ad brd ff:ff:ff:ff:ff:ff
>
> enp2s0f1 is the physical function; enp2s1f* are the interfaces to the
> virtual functions.
>
> Essentially, I have 2 questions:
> 1) What is the difference between the entries under the physical
> function and the interfaces?
> 2) How should I retrieve the correct MAC addresses? I'm aware of
> /sys/.../<vf>/net/address, but I am now not sure it is the correct
> source.
If you haven't used ip link commands to set the VF device MAC addresses
then the devices will create their own as they register their net device
entries. In that case you'll see the VF MAC addresses as all 00's
because you haven't set them.
Best known methods call for using the ip link command to set the MAC
addresses for the VF devices rather than letting them set their own
temporary LAA type MAC addresses.
- Greg
>
> Thanks,
> Leon
^ permalink raw reply
* Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive
From: John Fastabend @ 2017-01-25 15:52 UTC (permalink / raw)
To: Michael S. Tsirkin, Alexei Starovoitov
Cc: Jason Wang, David Miller, John Fastabend, netdev@vger.kernel.org,
Daniel Borkmann
In-Reply-To: <20170125164844-mutt-send-email-mst@kernel.org>
On 17-01-25 06:52 AM, Michael S. Tsirkin wrote:
> On Tue, Jan 24, 2017 at 11:33:56PM -0800, Alexei Starovoitov wrote:
>> On Tue, Jan 24, 2017 at 8:02 PM, John Fastabend
>> <john.fastabend@gmail.com> wrote:
>>>
>>> Finally just to point out here are the drivers with XDP support on latest
>>> net tree,
>>>
>>> mlx/mlx5
>>> mlx/mlx4
>>> qlogic/qede
>>> netronome/nfp
>>> virtio_net
>>>
>>> And here is the list of adjust header support,
>>>
>>> mlx/mlx4
>>>
>>
>> in net-next it's actually:
>> yes: mlx4, mlx5
>> no: qede, nfp, virtio
>> while nfp and virtio are working on it.
>>
>> xdp_adjust_head() is must have for load balancer,
>
> What amount of head space does it need? 70 bytes
> to do vxlan kind of thing?
>
>> so the sooner it lands for virtio the easier it will be
>> to develop xdp programs. Initially I expected
>> e1k+xdp to be the base line for debugging and
>> development of xdp programs, but since not everyone
>> agreed on e1k the virtio+xdp filled in the gap.
>> So without adjust_head in virtio I see very little use for it
>> in our environment.
>> It is a must have feature regardless of timing.
>> I will backport whatever is necessary, but distros
>> will stick with official releases and imo it's not great
>> from xdp adoption point of view to have
>> virtio driver lacking key features.
>
> If everyone can agree it's net-next material then I'm happy.
>
Considering that the only support for adjust_head in net branch
is in mlx4 and most drivers are aborting when programs get loaded
with adjust_head support I am OK with applying the patch below to
net and this series to net-next.
https://patchwork.ozlabs.org/patch/707118/
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 08327e005ccc..db761f37783e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1677,6 +1677,11 @@ static int virtnet_xdp_set(struct net_device *dev,
struct bpf_prog *prog)
u16 xdp_qp = 0, curr_qp;
int i, err;
+ if (prog && prog->xdp_adjust_head) {
+ netdev_warn(dev, "Does not support bpf_xdp_adjust_head()\n");
+ return -EOPNOTSUPP;
+ }
+
if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6)) {
netdev_warn(dev, "can't set XDP while host is implementing LRO, disable LRO
first\n");
Thanks,
John
^ permalink raw reply
* Re: [PATCH net] net: dsa: Mop up remaining NET_DSA_HWMON references
From: kbuild test robot @ 2017-01-25 15:53 UTC (permalink / raw)
To: Andrew Lunn
Cc: kbuild-all, David Miller, netdev, valentinrothberg, Andrew Lunn
In-Reply-To: <1485353057-4612-1-git-send-email-andrew@lunn.ch>
[-- Attachment #1: Type: text/plain, Size: 3081 bytes --]
Hi Andrew,
[auto build test ERROR on net/master]
url: https://github.com/0day-ci/linux/commits/Andrew-Lunn/net-dsa-Mop-up-remaining-NET_DSA_HWMON-references/20170125-221411
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
net/dsa/dsa.c: In function 'dsa_switch_setup_one':
>> net/dsa/dsa.c:447:15: error: 'struct dsa_switch' has no member named 'hwmon_name'
scnprintf(ds->hwmon_name, sizeof(ds->hwmon_name), "%s_dsa%d",
^~
net/dsa/dsa.c:447:38: error: 'struct dsa_switch' has no member named 'hwmon_name'
scnprintf(ds->hwmon_name, sizeof(ds->hwmon_name), "%s_dsa%d",
^~
>> net/dsa/dsa.c:449:5: error: 'struct dsa_switch' has no member named 'hwmon_dev'
ds->hwmon_dev = hwmon_device_register_with_groups(NULL,
^~
net/dsa/dsa.c:450:8: error: 'struct dsa_switch' has no member named 'hwmon_name'
ds->hwmon_name, ds, dsa_hwmon_groups);
^~
net/dsa/dsa.c:451:16: error: 'struct dsa_switch' has no member named 'hwmon_dev'
if (IS_ERR(ds->hwmon_dev))
^~
net/dsa/dsa.c:452:6: error: 'struct dsa_switch' has no member named 'hwmon_dev'
ds->hwmon_dev = NULL;
^~
net/dsa/dsa.c: In function 'dsa_switch_destroy':
net/dsa/dsa.c:518:8: error: 'struct dsa_switch' has no member named 'hwmon_dev'
if (ds->hwmon_dev)
^~
net/dsa/dsa.c:519:29: error: 'struct dsa_switch' has no member named 'hwmon_dev'
hwmon_device_unregister(ds->hwmon_dev);
^~
vim +447 net/dsa/dsa.c
51579c3f Guenter Roeck 2014-10-29 441 /* Create valid hwmon 'name' attribute */
51579c3f Guenter Roeck 2014-10-29 442 for (i = j = 0; i < IFNAMSIZ && netname[i]; i++) {
51579c3f Guenter Roeck 2014-10-29 443 if (isalnum(netname[i]))
51579c3f Guenter Roeck 2014-10-29 444 hname[j++] = netname[i];
51579c3f Guenter Roeck 2014-10-29 445 }
51579c3f Guenter Roeck 2014-10-29 446 hname[j] = '\0';
51579c3f Guenter Roeck 2014-10-29 @447 scnprintf(ds->hwmon_name, sizeof(ds->hwmon_name), "%s_dsa%d",
51579c3f Guenter Roeck 2014-10-29 448 hname, index);
51579c3f Guenter Roeck 2014-10-29 @449 ds->hwmon_dev = hwmon_device_register_with_groups(NULL,
51579c3f Guenter Roeck 2014-10-29 450 ds->hwmon_name, ds, dsa_hwmon_groups);
51579c3f Guenter Roeck 2014-10-29 451 if (IS_ERR(ds->hwmon_dev))
51579c3f Guenter Roeck 2014-10-29 452 ds->hwmon_dev = NULL;
:::::: The code at line 447 was first introduced by commit
:::::: 51579c3f1a9192b75365576227d40c7619493285 net: dsa: Add support for reporting switch chip temperatures
:::::: TO: Guenter Roeck <linux@roeck-us.net>
:::::: CC: David S. Miller <davem@davemloft.net>
---
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: 57926 bytes --]
^ permalink raw reply
* [PATCH 2/3] net: phy: broadcom: drop duplicated define for RXD to RXC delay
From: Rafał Miłecki @ 2017-01-25 15:54 UTC (permalink / raw)
To: David S . Miller, Florian Fainelli
Cc: Xo Wang, Joel Stanley, Jon Mason, Jaedon Shin, netdev,
Rafał Miłecki
In-Reply-To: <20170125155411.14922-1-zajec5@gmail.com>
From: Rafał Miłecki <rafal@milecki.pl>
We had two defines for the same bit (both were used with the
MII_BCM54XX_AUXCTL_SHDWSEL_MISC register).
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
drivers/net/phy/broadcom.c | 2 +-
include/linux/brcmphy.h | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 25c6e6cea2dc..9b7c2d57ca92 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -42,7 +42,7 @@ static int bcm54810_config(struct phy_device *phydev)
return rc;
val = bcm54xx_auxctl_read(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
- val &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN;
+ val &= ~MII_BCM54XX_AUXCTL_MISC_RXD_RXC_SKEW;
val |= MII_BCM54XX_AUXCTL_MISC_WREN;
rc = bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
val);
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 34e61004b9dc..bff53da82b58 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -113,7 +113,6 @@
#define MII_BCM54XX_AUXCTL_MISC_FORCE_AMDIX 0x0200
#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC 0x0007
#define MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT 12
-#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN (1 << 8)
#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN (1 << 4)
#define MII_BCM54XX_AUXCTL_SHDWSEL_MASK 0x0007
--
2.11.0
^ permalink raw reply related
* [PATCH 3/3] net: phy: bcm-phy-lib: clean up remaining AUXCTL register defines
From: Rafał Miłecki @ 2017-01-25 15:54 UTC (permalink / raw)
To: David S . Miller, Florian Fainelli
Cc: Xo Wang, Joel Stanley, Jon Mason, Jaedon Shin, netdev,
Rafał Miłecki
In-Reply-To: <20170125155411.14922-1-zajec5@gmail.com>
From: Rafał Miłecki <rafal@milecki.pl>
1) Use 0x%02x format for register number. This follows some other
defines and makes it easier to distinct register from values.
2) Put register define above values and sort the values. It makes
reading header code easier.
3) Drop SHDWSEL_ name part from the only value define using it. For all
other values we just start with MISC_.
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
drivers/net/phy/bcm-phy-lib.c | 6 +++---
include/linux/brcmphy.h | 8 ++++----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index ab9ad689617c..8d7e21a9fa77 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -241,7 +241,7 @@ int bcm_phy_downshift_get(struct phy_device *phydev, u8 *count)
return val;
/* Check if wirespeed is enabled or not */
- if (!(val & MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN)) {
+ if (!(val & MII_BCM54XX_AUXCTL_MISC_WIRESPEED_EN)) {
*count = DOWNSHIFT_DEV_DISABLE;
return 0;
}
@@ -283,12 +283,12 @@ int bcm_phy_downshift_set(struct phy_device *phydev, u8 count)
val |= MII_BCM54XX_AUXCTL_MISC_WREN;
if (count == DOWNSHIFT_DEV_DISABLE) {
- val &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN;
+ val &= ~MII_BCM54XX_AUXCTL_MISC_WIRESPEED_EN;
return bcm54xx_auxctl_write(phydev,
MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
val);
} else {
- val |= MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN;
+ val |= MII_BCM54XX_AUXCTL_MISC_WIRESPEED_EN;
ret = bcm54xx_auxctl_write(phydev,
MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
val);
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index bff53da82b58..a79d57caf7f1 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -104,16 +104,16 @@
/*
* AUXILIARY CONTROL SHADOW ACCESS REGISTERS. (PHY REG 0x18)
*/
-#define MII_BCM54XX_AUXCTL_SHDWSEL_AUXCTL 0x0000
+#define MII_BCM54XX_AUXCTL_SHDWSEL_AUXCTL 0x00
#define MII_BCM54XX_AUXCTL_ACTL_TX_6DB 0x0400
#define MII_BCM54XX_AUXCTL_ACTL_SMDSP_ENA 0x0800
-#define MII_BCM54XX_AUXCTL_MISC_WREN 0x8000
+#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC 0x07
+#define MII_BCM54XX_AUXCTL_MISC_WIRESPEED_EN 0x0010
#define MII_BCM54XX_AUXCTL_MISC_RXD_RXC_SKEW 0x0100
#define MII_BCM54XX_AUXCTL_MISC_FORCE_AMDIX 0x0200
-#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC 0x0007
+#define MII_BCM54XX_AUXCTL_MISC_WREN 0x8000
#define MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT 12
-#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN (1 << 4)
#define MII_BCM54XX_AUXCTL_SHDWSEL_MASK 0x0007
--
2.11.0
^ permalink raw reply related
* [PATCH 1/3] net: phy: broadcom: use auxctl reading helper in BCM54612E code
From: Rafał Miłecki @ 2017-01-25 15:54 UTC (permalink / raw)
To: David S . Miller, Florian Fainelli
Cc: Xo Wang, Joel Stanley, Jon Mason, Jaedon Shin, netdev,
Rafał Miłecki
From: Rafał Miłecki <rafal@milecki.pl>
Starting with commit 5b4e29005123 ("net: phy: broadcom: add
bcm54xx_auxctl_read") we have a reading helper so use it and avoid code
duplication.
It also means we don't need MII_BCM54XX_AUXCTL_SHDWSEL_MISC define as
it's the same as MII_BCM54XX_AUXCTL_SHDWSEL_MISC just for reading needs
(same value shifted by 12 bits).
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
drivers/net/phy/broadcom.c | 6 ++----
include/linux/brcmphy.h | 1 -
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 4223e35490b0..25c6e6cea2dc 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -395,10 +395,8 @@ static int bcm54612e_config_aneg(struct phy_device *phydev)
(phydev->interface != PHY_INTERFACE_MODE_RGMII_RXID)) {
u16 reg;
- /* Errata: reads require filling in the write selector field */
- bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
- MII_BCM54XX_AUXCTL_MISC_RDSEL_MISC);
- reg = phy_read(phydev, MII_BCM54XX_AUX_CTL);
+ reg = bcm54xx_auxctl_read(phydev,
+ MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
/* Disable RXD to RXC delay (default set) */
reg &= ~MII_BCM54XX_AUXCTL_MISC_RXD_RXC_SKEW;
/* Clear shadow selector field */
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 295fb3e73de5..34e61004b9dc 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -111,7 +111,6 @@
#define MII_BCM54XX_AUXCTL_MISC_WREN 0x8000
#define MII_BCM54XX_AUXCTL_MISC_RXD_RXC_SKEW 0x0100
#define MII_BCM54XX_AUXCTL_MISC_FORCE_AMDIX 0x0200
-#define MII_BCM54XX_AUXCTL_MISC_RDSEL_MISC 0x7000
#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC 0x0007
#define MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT 12
#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN (1 << 8)
--
2.11.0
^ permalink raw reply related
* Re: [PATCH net] sctp: sctp_addr_id2transport should verify the addr before looking up assoc
From: Vladislav Yasevich @ 2017-01-25 15:57 UTC (permalink / raw)
To: Xin Long
Cc: network dev, linux-sctp@vger.kernel.org, David Miller,
Marcelo Ricardo Leitner, Neil Horman
In-Reply-To: <CADvbK_dw2XhYF7bc8PdyuBG2WgzNwZxJekwWsc36UN=2QE8MDg@mail.gmail.com>
On Wed, Jan 25, 2017 at 10:34 AM, Xin Long <lucien.xin@gmail.com> wrote:
>
> On Wed, Jan 25, 2017 at 11:27 PM, Vladislav Yasevich
> <vyasevich@gmail.com> wrote:
> > On Tue, Jan 24, 2017 at 1:01 AM, Xin Long <lucien.xin@gmail.com> wrote:
> >>
> >> sctp_addr_id2transport is a function for sockopt to look up assoc by
> >> address. As the address is from userspace, it can be a v4-mapped v6
> >> address. But in sctp protocol stack, it always handles a v4-mapped
> >> v6 address as a v4 address. So it's necessary to convert it to a v4
> >> address before looking up assoc by address.
> >>
> >> This patch is to fix it by calling sctp_verify_addr in which it can do
> >> this conversion before calling sctp_endpoint_lookup_assoc, just like
> >> what sctp_sendmsg and __sctp_connect do for the address from users.
> >>
> >> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> >> ---
> >> net/sctp/socket.c | 6 +++++-
> >> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> >> index 318c678..37eeab7 100644
> >> --- a/net/sctp/socket.c
> >> +++ b/net/sctp/socket.c
> >> @@ -235,8 +235,12 @@ static struct sctp_transport
> >> *sctp_addr_id2transport(struct sock *sk,
> >> sctp_assoc_t id)
> >> {
> >> struct sctp_association *addr_asoc = NULL, *id_asoc = NULL;
> >> - struct sctp_transport *transport;
> >> + struct sctp_af *af = sctp_get_af_specific(addr->ss_family);
> >> union sctp_addr *laddr = (union sctp_addr *)addr;
> >> + struct sctp_transport *transport;
> >> +
> >> + if (sctp_verify_addr(sk, laddr, af->sockaddr_len))
> >> + return NULL;
> >>
> >
> > This causes a side-effect such that GET options will end up with ipv4
> > address instead
> > of a v4mapped address that was passed in.
> not really
>
> (more below)
> >
> > -vlad
> >
> >>
> >> addr_asoc = sctp_endpoint_lookup_assoc(sctp_sk(sk)->ep,
> >> laddr,
> sctp_get_pf_specific(sk->sk_family)->addr_to_user(sctp_sk(sk),
> (union sctp_addr *)addr);
>
> here it will convert it back to v4mapped v6 address.
>
Yep, you are right. Missed the fact that it was already there.
ACK
-vlad
> >> --
> >> 2.1.0
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> >
^ permalink raw reply
* Re: [PATCH net-next 5/5] bpf: enable verifier to better track const alu ops
From: William Tu @ 2017-01-25 15:56 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David Miller, Linux Kernel Network Developers, ast,
Gianluca Borello
In-Reply-To: <41914154d137b47e3c9400da58e7e2b15ef5d43a.1485214851.git.daniel@iogearbox.net>
Looks good to me, I tested with several complex program without any
problem. Thanks for the patch.
--William
On Mon, Jan 23, 2017 at 4:06 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> William reported couple of issues in relation to direct packet
> access. Typical scheme is to check for data + [off] <= data_end,
> where [off] can be either immediate or coming from a tracked
> register that contains an immediate, depending on the branch, we
> can then access the data. However, in case of calculating [off]
> for either the mentioned test itself or for access after the test
> in a more "complex" way, then the verifier will stop tracking the
> CONST_IMM marked register and will mark it as UNKNOWN_VALUE one.
>
> Adding that UNKNOWN_VALUE typed register to a pkt() marked
> register, the verifier then bails out in check_packet_ptr_add()
> as it finds the registers imm value below 48. In the first below
> example, that is due to evaluate_reg_imm_alu() not handling right
> shifts and thus marking the register as UNKNOWN_VALUE via helper
> __mark_reg_unknown_value() that resets imm to 0.
>
> In the second case the same happens at the time when r4 is set
> to r4 &= r5, where it transitions to UNKNOWN_VALUE from
> evaluate_reg_imm_alu(). Later on r4 we shift right by 3 inside
> evaluate_reg_alu(), where the register's imm turns into 3. That
> is, for registers with type UNKNOWN_VALUE, imm of 0 means that
> we don't know what value the register has, and for imm > 0 it
> means that the value has [imm] upper zero bits. F.e. when shifting
> an UNKNOWN_VALUE register by 3 to the right, no matter what value
> it had, we know that the 3 upper most bits must be zero now.
> This is to make sure that ALU operations with unknown registers
> don't overflow. Meaning, once we know that we have more than 48
> upper zero bits, or, in other words cannot go beyond 0xffff offset
> with ALU ops, such an addition will track the target register
> as a new pkt() register with a new id, but 0 offset and 0 range,
> so for that a new data/data_end test will be required. Is the source
> register a CONST_IMM one that is to be added to the pkt() register,
> or the source instruction is an add instruction with immediate
> value, then it will get added if it stays within max 0xffff bounds.
> From there, pkt() type, can be accessed should reg->off + imm be
> within the access range of pkt().
>
> [...]
> from 28 to 30: R0=imm1,min_value=1,max_value=1
> R1=pkt(id=0,off=0,r=22) R2=pkt_end
> R3=imm144,min_value=144,max_value=144
> R4=imm0,min_value=0,max_value=0
> R5=inv48,min_value=2054,max_value=2054 R10=fp
> 30: (bf) r5 = r3
> 31: (07) r5 += 23
> 32: (77) r5 >>= 3
> 33: (bf) r6 = r1
> 34: (0f) r6 += r5
> cannot add integer value with 0 upper zero bits to ptr_to_packet
>
> [...]
> from 52 to 80: R0=imm1,min_value=1,max_value=1
> R1=pkt(id=0,off=0,r=34) R2=pkt_end R3=inv
> R4=imm272 R5=inv56,min_value=17,max_value=17
> R6=pkt(id=0,off=26,r=34) R10=fp
> 80: (07) r4 += 71
> 81: (18) r5 = 0xfffffff8
> 83: (5f) r4 &= r5
> 84: (77) r4 >>= 3
> 85: (0f) r1 += r4
> cannot add integer value with 3 upper zero bits to ptr_to_packet
>
> Thus to get above use-cases working, evaluate_reg_imm_alu() has
> been extended for further ALU ops. This is fine, because we only
> operate strictly within realm of CONST_IMM types, so here we don't
> care about overflows as they will happen in the simulated but also
> real execution and interaction with pkt() in check_packet_ptr_add()
> will check actual imm value once added to pkt(), but it's irrelevant
> before.
>
> With regards to 06c1c049721a ("bpf: allow helpers access to variable
> memory") that works on UNKNOWN_VALUE registers, the verifier becomes
> now a bit smarter as it can better resolve ALU ops, so we need to
> adapt two test cases there, as min/max bound tracking only becomes
> necessary when registers were spilled to stack. So while mask was
> set before to track upper bound for UNKNOWN_VALUE case, it's now
> resolved directly as CONST_IMM, and such contructs are only necessary
> when f.e. registers are spilled.
>
> For commit 6b17387307ba ("bpf: recognize 64bit immediate loads as
> consts") that initially enabled dw load tracking only for nfp jit/
> analyzer, I did couple of tests on large, complex programs and we
> don't increase complexity badly (my tests were in ~3% range on avg).
> I've added a couple of tests similar to affected code above, and
> it works fine with verifier now.
>
> Reported-by: William Tu <u9012063@gmail.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Gianluca Borello <g.borello@gmail.com>
> Cc: William Tu <u9012063@gmail.com>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
> ---
> kernel/bpf/verifier.c | 64 +++++++++++++++-------
> tools/testing/selftests/bpf/test_verifier.c | 82 +++++++++++++++++++++++++++++
> 2 files changed, 127 insertions(+), 19 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8f69df7..fb3513b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1566,22 +1566,54 @@ static int evaluate_reg_imm_alu(struct bpf_verifier_env *env,
> struct bpf_reg_state *dst_reg = ®s[insn->dst_reg];
> struct bpf_reg_state *src_reg = ®s[insn->src_reg];
> u8 opcode = BPF_OP(insn->code);
> + u64 dst_imm = dst_reg->imm;
>
> - /* dst_reg->type == CONST_IMM here, simulate execution of 'add'/'or'
> - * insn. Don't care about overflow or negative values, just add them
> + /* dst_reg->type == CONST_IMM here. Simulate execution of insns
> + * containing ALU ops. Don't care about overflow or negative
> + * values, just add/sub/... them; registers are in u64.
> */
> - if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_K)
> - dst_reg->imm += insn->imm;
> - else if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_X &&
> - src_reg->type == CONST_IMM)
> - dst_reg->imm += src_reg->imm;
> - else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_K)
> - dst_reg->imm |= insn->imm;
> - else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_X &&
> - src_reg->type == CONST_IMM)
> - dst_reg->imm |= src_reg->imm;
> - else
> + if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_K) {
> + dst_imm += insn->imm;
> + } else if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_X &&
> + src_reg->type == CONST_IMM) {
> + dst_imm += src_reg->imm;
> + } else if (opcode == BPF_SUB && BPF_SRC(insn->code) == BPF_K) {
> + dst_imm -= insn->imm;
> + } else if (opcode == BPF_SUB && BPF_SRC(insn->code) == BPF_X &&
> + src_reg->type == CONST_IMM) {
> + dst_imm -= src_reg->imm;
> + } else if (opcode == BPF_MUL && BPF_SRC(insn->code) == BPF_K) {
> + dst_imm *= insn->imm;
> + } else if (opcode == BPF_MUL && BPF_SRC(insn->code) == BPF_X &&
> + src_reg->type == CONST_IMM) {
> + dst_imm *= src_reg->imm;
> + } else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_K) {
> + dst_imm |= insn->imm;
> + } else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_X &&
> + src_reg->type == CONST_IMM) {
> + dst_imm |= src_reg->imm;
> + } else if (opcode == BPF_AND && BPF_SRC(insn->code) == BPF_K) {
> + dst_imm &= insn->imm;
> + } else if (opcode == BPF_AND && BPF_SRC(insn->code) == BPF_X &&
> + src_reg->type == CONST_IMM) {
> + dst_imm &= src_reg->imm;
> + } else if (opcode == BPF_RSH && BPF_SRC(insn->code) == BPF_K) {
> + dst_imm >>= insn->imm;
> + } else if (opcode == BPF_RSH && BPF_SRC(insn->code) == BPF_X &&
> + src_reg->type == CONST_IMM) {
> + dst_imm >>= src_reg->imm;
> + } else if (opcode == BPF_LSH && BPF_SRC(insn->code) == BPF_K) {
> + dst_imm <<= insn->imm;
> + } else if (opcode == BPF_LSH && BPF_SRC(insn->code) == BPF_X &&
> + src_reg->type == CONST_IMM) {
> + dst_imm <<= src_reg->imm;
> + } else {
> mark_reg_unknown_value(regs, insn->dst_reg);
> + goto out;
> + }
> +
> + dst_reg->imm = dst_imm;
> +out:
> return 0;
> }
>
> @@ -2225,14 +2257,8 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn)
> return err;
>
> if (insn->src_reg == 0) {
> - /* generic move 64-bit immediate into a register,
> - * only analyzer needs to collect the ld_imm value.
> - */
> u64 imm = ((u64)(insn + 1)->imm << 32) | (u32)insn->imm;
>
> - if (!env->analyzer_ops)
> - return 0;
> -
> regs[insn->dst_reg].type = CONST_IMM;
> regs[insn->dst_reg].imm = imm;
> return 0;
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 1aa7324..0d0912c 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -2326,6 +2326,84 @@ struct test_val {
> .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> },
> {
> + "direct packet access: test11 (shift, good access)",
> + .insns = {
> + BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
> + offsetof(struct __sk_buff, data)),
> + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
> + offsetof(struct __sk_buff, data_end)),
> + BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 22),
> + BPF_JMP_REG(BPF_JGT, BPF_REG_0, BPF_REG_3, 8),
> + BPF_MOV64_IMM(BPF_REG_3, 144),
> + BPF_MOV64_REG(BPF_REG_5, BPF_REG_3),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 23),
> + BPF_ALU64_IMM(BPF_RSH, BPF_REG_5, 3),
> + BPF_MOV64_REG(BPF_REG_6, BPF_REG_2),
> + BPF_ALU64_REG(BPF_ADD, BPF_REG_6, BPF_REG_5),
> + BPF_MOV64_IMM(BPF_REG_0, 1),
> + BPF_EXIT_INSN(),
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + },
> + .result = ACCEPT,
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + },
> + {
> + "direct packet access: test12 (and, good access)",
> + .insns = {
> + BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
> + offsetof(struct __sk_buff, data)),
> + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
> + offsetof(struct __sk_buff, data_end)),
> + BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 22),
> + BPF_JMP_REG(BPF_JGT, BPF_REG_0, BPF_REG_3, 8),
> + BPF_MOV64_IMM(BPF_REG_3, 144),
> + BPF_MOV64_REG(BPF_REG_5, BPF_REG_3),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 23),
> + BPF_ALU64_IMM(BPF_AND, BPF_REG_5, 15),
> + BPF_MOV64_REG(BPF_REG_6, BPF_REG_2),
> + BPF_ALU64_REG(BPF_ADD, BPF_REG_6, BPF_REG_5),
> + BPF_MOV64_IMM(BPF_REG_0, 1),
> + BPF_EXIT_INSN(),
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + },
> + .result = ACCEPT,
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + },
> + {
> + "direct packet access: test13 (branches, good access)",
> + .insns = {
> + BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
> + offsetof(struct __sk_buff, data)),
> + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
> + offsetof(struct __sk_buff, data_end)),
> + BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 22),
> + BPF_JMP_REG(BPF_JGT, BPF_REG_0, BPF_REG_3, 13),
> + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
> + offsetof(struct __sk_buff, mark)),
> + BPF_MOV64_IMM(BPF_REG_4, 1),
> + BPF_JMP_REG(BPF_JGT, BPF_REG_3, BPF_REG_4, 2),
> + BPF_MOV64_IMM(BPF_REG_3, 14),
> + BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> + BPF_MOV64_IMM(BPF_REG_3, 24),
> + BPF_MOV64_REG(BPF_REG_5, BPF_REG_3),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 23),
> + BPF_ALU64_IMM(BPF_AND, BPF_REG_5, 15),
> + BPF_MOV64_REG(BPF_REG_6, BPF_REG_2),
> + BPF_ALU64_REG(BPF_ADD, BPF_REG_6, BPF_REG_5),
> + BPF_MOV64_IMM(BPF_REG_0, 1),
> + BPF_EXIT_INSN(),
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + },
> + .result = ACCEPT,
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + },
> + {
> "helper access to packet: test1, valid packet_ptr range",
> .insns = {
> BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
> @@ -4208,6 +4286,8 @@ struct test_val {
> .insns = {
> BPF_MOV64_IMM(BPF_REG_1, 0),
> BPF_MOV64_IMM(BPF_REG_2, 0),
> + BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -128),
> + BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_10, -128),
> BPF_ALU64_IMM(BPF_AND, BPF_REG_2, 64),
> BPF_MOV64_IMM(BPF_REG_3, 0),
> BPF_MOV64_IMM(BPF_REG_4, 0),
> @@ -4251,6 +4331,8 @@ struct test_val {
> BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_0, -16),
> BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_0, -8),
> BPF_MOV64_IMM(BPF_REG_2, 0),
> + BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -128),
> + BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_10, -128),
> BPF_ALU64_IMM(BPF_AND, BPF_REG_2, 63),
> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, 1),
> BPF_MOV64_IMM(BPF_REG_3, 0),
> --
> 1.9.3
>
^ 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