* [PATCH net] selftests: fixes for UDP GRO
From: Paolo Abeni @ 2019-02-26 14:27 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Willem de Bruijn
The current implementation for UDP GRO tests is racy: the receiver
may flush the RX queue while the sending is still transmitting and
incorrectly report RX errors, with a wrong number of packet received.
Add explicit timeouts to the receiver for both connection activation
(first packet received for UDP) and reception completion, so that
in the above critical scenario the receiver will wait for the
transfer completion.
Fixes: 3327a9c46352 ("selftests: add functionals test for UDP GRO")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
tools/testing/selftests/net/udpgro.sh | 8 ++--
tools/testing/selftests/net/udpgso_bench_rx.c | 42 +++++++++++++------
2 files changed, 33 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/net/udpgro.sh b/tools/testing/selftests/net/udpgro.sh
index aeac53a99aeb..ac2a30be9b32 100755
--- a/tools/testing/selftests/net/udpgro.sh
+++ b/tools/testing/selftests/net/udpgro.sh
@@ -37,7 +37,7 @@ run_one() {
cfg_veth
- ip netns exec "${PEER_NS}" ./udpgso_bench_rx ${rx_args} && \
+ ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${rx_args} && \
echo "ok" || \
echo "failed" &
@@ -81,7 +81,7 @@ run_one_nat() {
# will land on the 'plain' one
ip netns exec "${PEER_NS}" ./udpgso_bench_rx -G ${family} -b ${addr1} -n 0 &
pid=$!
- ip netns exec "${PEER_NS}" ./udpgso_bench_rx ${family} -b ${addr2%/*} ${rx_args} && \
+ ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${family} -b ${addr2%/*} ${rx_args} && \
echo "ok" || \
echo "failed"&
@@ -99,8 +99,8 @@ run_one_2sock() {
cfg_veth
- ip netns exec "${PEER_NS}" ./udpgso_bench_rx ${rx_args} -p 12345 &
- ip netns exec "${PEER_NS}" ./udpgso_bench_rx ${rx_args} && \
+ ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${rx_args} -p 12345 &
+ ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 2000 -R 10 ${rx_args} && \
echo "ok" || \
echo "failed" &
diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c b/tools/testing/selftests/net/udpgso_bench_rx.c
index 0c960f673324..db3d4a8b5a4c 100644
--- a/tools/testing/selftests/net/udpgso_bench_rx.c
+++ b/tools/testing/selftests/net/udpgso_bench_rx.c
@@ -45,6 +45,8 @@ static int cfg_alen = sizeof(struct sockaddr_in6);
static int cfg_expected_pkt_nr;
static int cfg_expected_pkt_len;
static int cfg_expected_gso_size;
+static int cfg_connect_timeout_ms;
+static int cfg_rcv_timeout_ms;
static struct sockaddr_storage cfg_bind_addr;
static bool interrupted;
@@ -87,7 +89,7 @@ static unsigned long gettimeofday_ms(void)
return (tv.tv_sec * 1000) + (tv.tv_usec / 1000);
}
-static void do_poll(int fd)
+static void do_poll(int fd, int timeout_ms)
{
struct pollfd pfd;
int ret;
@@ -102,8 +104,16 @@ static void do_poll(int fd)
break;
if (ret == -1)
error(1, errno, "poll");
- if (ret == 0)
- continue;
+ if (ret == 0) {
+ if (!timeout_ms)
+ continue;
+
+ timeout_ms -= 10;
+ if (timeout_ms <= 0) {
+ interrupted = true;
+ break;
+ }
+ }
if (pfd.revents != POLLIN)
error(1, errno, "poll: 0x%x expected 0x%x\n",
pfd.revents, POLLIN);
@@ -134,7 +144,7 @@ static int do_socket(bool do_tcp)
if (listen(accept_fd, 1))
error(1, errno, "listen");
- do_poll(accept_fd);
+ do_poll(accept_fd, cfg_connect_timeout_ms);
if (interrupted)
exit(0);
@@ -273,7 +283,9 @@ static void do_flush_udp(int fd)
static void usage(const char *filepath)
{
- error(1, 0, "Usage: %s [-Grtv] [-b addr] [-p port] [-l pktlen] [-n packetnr] [-S gsosize]", filepath);
+ error(1, 0, "Usage: %s [-C connect_timeout] [-Grtv] [-b addr] [-p port]"
+ " [-l pktlen] [-n packetnr] [-R rcv_timeout] [-S gsosize]",
+ filepath);
}
static void parse_opts(int argc, char **argv)
@@ -282,7 +294,7 @@ static void parse_opts(int argc, char **argv)
/* bind to any by default */
setup_sockaddr(PF_INET6, "::", &cfg_bind_addr);
- while ((c = getopt(argc, argv, "4b:Gl:n:p:rS:tv")) != -1) {
+ while ((c = getopt(argc, argv, "4b:C:Gl:n:p:rR:S:tv")) != -1) {
switch (c) {
case '4':
cfg_family = PF_INET;
@@ -292,6 +304,9 @@ static void parse_opts(int argc, char **argv)
case 'b':
setup_sockaddr(cfg_family, optarg, &cfg_bind_addr);
break;
+ case 'C':
+ cfg_connect_timeout_ms = strtoul(optarg, NULL, 0);
+ break;
case 'G':
cfg_gro_segment = true;
break;
@@ -307,6 +322,9 @@ static void parse_opts(int argc, char **argv)
case 'r':
cfg_read_all = true;
break;
+ case 'R':
+ cfg_rcv_timeout_ms = strtoul(optarg, NULL, 0);
+ break;
case 'S':
cfg_expected_gso_size = strtol(optarg, NULL, 0);
break;
@@ -329,8 +347,9 @@ static void parse_opts(int argc, char **argv)
static void do_recv(void)
{
+ int timeout_ms = cfg_tcp ? cfg_rcv_timeout_ms : cfg_connect_timeout_ms;
unsigned long tnow, treport;
- int fd, loop = 0;
+ int fd;
fd = do_socket(cfg_tcp);
@@ -342,12 +361,7 @@ static void do_recv(void)
treport = gettimeofday_ms() + 1000;
do {
- /* force termination after the second poll(); this cope both
- * with sender slower than receiver and missing packet errors
- */
- if (cfg_expected_pkt_nr && loop++)
- interrupted = true;
- do_poll(fd);
+ do_poll(fd, timeout_ms);
if (cfg_tcp)
do_flush_tcp(fd);
@@ -365,6 +379,8 @@ static void do_recv(void)
treport = tnow + 1000;
}
+ timeout_ms = cfg_rcv_timeout_ms;
+
} while (!interrupted);
if (cfg_expected_pkt_nr && (packets != cfg_expected_pkt_nr))
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id()
From: Daniel Borkmann @ 2019-02-26 14:44 UTC (permalink / raw)
To: zerons, ast; +Cc: netdev, linux-kernel
In-Reply-To: <1551190537-28694-1-git-send-email-sironhide0null@gmail.com>
On 02/26/2019 03:15 PM, zerons wrote:
> [ Upstream commit c91951f15978f1a0c6b65f063d30f7ea7bc6fb42 ]
Thanks for the fix! What do you mean by "upstream commit" above in this context?
> In bpf/syscall.c, bpf_map_get_fd_by_id() use bpf_map_inc_not_zero() to increase
> the refcount, both map->refcnt and map->usercnt. Then, if bpf_map_new_fd() fails,
> should handle map->usercnt too.
>
> Signed-off-by: zerons <sironhide0null@gmail.com>
> ---
> kernel/bpf/syscall.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index cf5040f..db1ed12 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1966,7 +1966,7 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
>
> fd = bpf_map_new_fd(map, f_flags);
> if (fd < 0)
> - bpf_map_put(map);
> + bpf_map_put_with_uref(map);
>
> return fd;
> }
> --
> 2.7.4
>
^ permalink raw reply
* AF_XDP design flaws
From: Maxim Mikityanskiy @ 2019-02-26 14:49 UTC (permalink / raw)
To: netdev@vger.kernel.org, Björn Töpel, Magnus Karlsson,
David S. Miller
Cc: Tariq Toukan, Saeed Mahameed, Eran Ben Elisha
Hi everyone,
I would like to discuss some design flaws of AF_XDP socket (XSK) implementation
in kernel. At the moment I don't see a way to work around them without changing
the API, so I would like to make sure that I'm not missing anything and to
suggest and discuss some possible improvements that can be made.
The issues I describe below are caused by the fact that the driver depends on
the application doing some things, and if the application is
slow/buggy/malicious, the driver is forced to busy poll because of the lack of a
notification mechanism from the application side. I will refer to the i40e
driver implementation a lot, as it is the first implementation of AF_XDP, but
the issues are general and affect any driver. I already considered trying to fix
it on driver level, but it doesn't seem possible, so it looks like the behavior
and implementation of AF_XDP in the kernel has to be changed.
RX side busy polling
====================
On the RX side, the driver expects the application to put some descriptors in
the Fill Ring. There is no way for the application to notify the driver that
there are more Fill Ring descriptors to take, so the driver is forced to busy
poll the Fill Ring if it gets empty. E.g., the i40e driver does it in NAPI poll:
int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
{
...
failure = failure ||
!i40e_alloc_rx_buffers_fast_zc(rx_ring,
cleaned_count);
...
return failure ? budget : (int)total_rx_packets;
}
Basically, it means that if there are no descriptors in the Fill Ring, NAPI will
never stop, draining CPU.
Possible cases when it happens
------------------------------
1. The application is slow, it received some frames in the RX Ring, and it is
still handling the data, so it has no free frames to put to the Fill Ring.
2. The application is malicious, it opens an XSK and puts no frames to the Fill
Ring. It can be used as a local DoS attack.
3. The application is buggy and stops filling the Fill Ring for whatever reason
(deadlock, waiting for another blocking operation, other bugs).
Although loading an XDP program requires root access, the DoS attack can be
targeted to setups that already use XDP, i.e. an XDP program is already loaded.
Even under root, userspace applications should not be able to disrupt system
stability by just calling normal APIs without an intention to destroy the
system, and here it happens in case 1.
Possible way to solve the issue
-------------------------------
When the driver can't take new Fill Ring frames, it shouldn't busy poll.
Instead, it signals the failure to the application (e.g., with POLLERR), and
after that it's up to the application to restart polling (e.g., by calling
sendto()) after refilling the Fill Ring. The issue with this approach is that it
changes the API, so we either have to deal with it or to introduce some API
version field.
TX side getting stuck
=====================
On the TX side, there is the Completion Ring that the application has to clean.
If it doesn't, the i40e driver stops taking descriptors from the TX Ring. If the
application finally completes something, the driver can go on transmitting.
However, it would require busy polling the Completion Ring (just like with the
Fill Ring on the RX side). i40e doesn't do it, instead, it relies on the
application to kick the TX by calling sendto(). The issue is that poll() doesn't
return POLLOUT in this case, because the TX Ring is full, so the application
will never call sendto(), and the ring is stuck forever (or at least until
something triggers NAPI).
Possible way to solve the issue
-------------------------------
When the driver can't reserve a descriptor in the Completion Ring, it should
signal the failure to the application (e.g., with POLLERR). The application
shouldn't call sendto() every time it sees that the number of not completed
frames is greater than zero (like xdpsock sample does). Instead, the application
should kick the TX only when it wants to flush the ring, and, in addition, after
resolving the cause for POLLERR, i.e. after handling Completion Ring entries.
The API will also have to change with this approach.
Triggering NAPI on a different CPU core
=======================================
.ndo_xsk_async_xmit runs on a random CPU core, so, to preserve CPU affinity,
i40e triggers an interrupt to schedule NAPI, instead of calling napi_schedule
directly. Scheduling NAPI on the correct CPU is what would every driver do, I
guess, but currently it has to be implemented differently in every driver, and
it relies on hardware features (the ability to trigger an IRQ).
I suggest introducing a kernel API that would allow triggering NAPI on a given
CPU. A brief look shows that something like smp_call_function_single_async can
be used. Advantages:
1. It lifts the hardware requirement to be able to raise an interrupt on demand.
2. It would allow to move common code to the kernel (.ndo_xsk_async_xmit).
3. It is also useful in the situation where CPU affinity changes while being in
NAPI poll. Currently, i40e and mlx5e try to stop NAPI polling by returning
a value less than budget if CPU affinity changes. However, there are cases
(e.g., NAPIF_STATE_MISSED) when NAPI will be rescheduled on a wrong CPU. It's a
race between the interrupt, which will move NAPI to the correct CPU, and
__napi_schedule from a wrong CPU. Having an API to schedule NAPI on a given CPU
will benefit both mlx5e and i40e, because when this situation happens, it kills
the performance.
I would be happy to hear your thoughts about these issues.
Thanks,
Max
^ permalink raw reply
* Re: [PATCH net-next 01/12] net: sched: flower: don't check for rtnl on head dereference
From: Vlad Buslov @ 2019-02-26 14:57 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <CAM_iQpW9wv_0Rc96we2rkGGpeVnL3gJBxhQ9npuJ7WJsd8+MVQ@mail.gmail.com>
On Mon 25 Feb 2019 at 22:39, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Feb 25, 2019 at 8:11 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>>
>> On Fri 22 Feb 2019 at 19:32, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> >
>> > So if it is no longer RCU any more, why do you still use
>> > rcu_dereference_protected()? That is, why not just deref it as a raw
>> > pointer?
>
>
> Any answer for this question?
I decided that since there is neither possibility of concurrent pointer
assignment nor deallocation of object that it points to, most performant
solution would be using rcu_dereference_protected() which is the only
RCU dereference helper that doesn't use READ_ONCE. I now understand that
this is confusing (and most likely doesn't provide any noticeable
performance improvement anyway!) and will change this patch to use
rcu_dereference_raw() as you suggest.
>
>
>> >
>> > And, I don't think I can buy your argument here. The RCU infrastructure
>> > should not be changed even after your patches, the fast path is still
>> > protocted by RCU read lock, while the slow path now is protected by
>> > some smaller-scope locks. What makes cls_flower so unique that
>> > it doesn't even need RCU here? tp->root is not reassigned but it is still
>> > freed via RCU infra, that is in fl_destroy_sleepable().
>> >
>> > Thanks.
>>
>> My cls API patch set introduced reference counting for tcf_proto
>> structure. With that change tp->ops->destroy() (which calls fl_destroy()
>> and fl_destroy_sleepable(), in case of flower classifier) is only called
>> after last reference to tp is released. All slow path users of tp->ops
>> must obtain reference to tp, so concurrent call to fl_destroy() is not
>> possible. Before this change tcf_proto structure didn't have reference
>> counting support and required users to obtain rtnl mutex before calling
>> its ops callbacks. This was verified in flower by using rtnl_dereference
>> to obtain tp->root.
>
> Yes, but fast path doesn't hold a refnct of tp, does it? If not, you still
> rely on RCU for sync with readers. If yes, then probably RCU can be
> gone.
>
> Now you are in a middle of the two, that is taking RCU read lock on
> fast path without a refcnt, meanwhile still uses rcu_dereference on
> slow paths without any lock.
>
> For me, you at least don't use the RCU API correctly here.
>
> Thanks.
Yes, fast path still relies on RCU. What I meant is that slow path (cls
API) now only calls tp ops after obtaining reference to tp, so there is
no need to protect it from concurrent tp->ops->destroy() by means of
rtnl or any other lock. I understand that using
rcu_dereference_protected() is confusing in this case and will refactor
this patch appropriately.
^ permalink raw reply
* RE: [PATCH net 1/4] tls: Fix tls_device handling of partial records
From: Vakul Garg @ 2019-02-26 14:57 UTC (permalink / raw)
To: Boris Pismenny, aviadye@mellanox.com, davejwatson@fb.com,
john.fastabend@gmail.com, daniel@iogearbox.net,
netdev@vger.kernel.org
Cc: eranbe@mellanox.com
In-Reply-To: <20190226121235.20784-2-borisp@mellanox.com>
> -----Original Message-----
> From: Boris Pismenny <borisp@mellanox.com>
> Sent: Tuesday, February 26, 2019 5:43 PM
> To: aviadye@mellanox.com; davejwatson@fb.com;
> john.fastabend@gmail.com; daniel@iogearbox.net; Vakul Garg
> <vakul.garg@nxp.com>; netdev@vger.kernel.org
> Cc: eranbe@mellanox.com; borisp@mellanox.com
> Subject: [PATCH net 1/4] tls: Fix tls_device handling of partial records
>
> Cleanup the handling of partial records while fixing a bug where the
> tls_push_pending_closed_record function is using the software tls
> context instead of the hardware context.
Can you provide details of what cleanup has been done?
I see that we got rid of concept of 'TLS_PENDING_CLOSED_RECORD'.
I vaguely remember that at one point in time, it seemed to me redundant.
But I was not sure. Please confirm if it is the case.
Can this patch be split into two? One for the cleanup and one for the bug.
>
> The bug resulted in the following crash:
> [ 88.791229] BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000000
> [ 88.793271] #PF error: [normal kernel read fault]
> [ 88.794449] PGD 800000022a426067 P4D 800000022a426067 PUD
> 22a156067 PMD 0
> [ 88.795958] Oops: 0000 [#1] SMP PTI
> [ 88.796884] CPU: 2 PID: 4973 Comm: openssl Not tainted 5.0.0-rc4+ #3
> [ 88.798314] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS Bochs 01/01/2011
> [ 88.800067] RIP: 0010:tls_tx_records+0xef/0x1d0 [tls]
> [ 88.801256] Code: 00 02 48 89 43 08 e8 a0 0b 96 d9 48 89 df e8 48 dd
> 4d d9 4c 89 f8 4d 8b bf 98 00 00 00 48 05 98 00 00 00 48 89 04 24 49 39
> c7 <49> 8b 1f 4d 89 fd 0f 84 af 00 00 00 41 8b 47 10 85 c0 0f 85 8d 00
> [ 88.805179] RSP: 0018:ffffbd888186fca8 EFLAGS: 00010213
> [ 88.806458] RAX: ffff9af1ed657c98 RBX: ffff9af1e88a1980 RCX:
> 0000000000000000
> [ 88.808050] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
> ffff9af1e88a1980
> [ 88.809724] RBP: ffff9af1e88a1980 R08: 0000000000000017 R09:
> ffff9af1ebeeb700
> [ 88.811294] R10: 0000000000000000 R11: 0000000000000000 R12:
> 0000000000000000
> [ 88.812917] R13: ffff9af1e88a1980 R14: ffff9af1ec13f800 R15:
> 0000000000000000
> [ 88.814506] FS: 00007fcad2240740(0000) GS:ffff9af1f7880000(0000)
> knlGS:0000000000000000
> [ 88.816337] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 88.817717] CR2: 0000000000000000 CR3: 0000000228b3e000 CR4:
> 00000000001406e0
> [ 88.819328] Call Trace:
> [ 88.820123] tls_push_data+0x628/0x6a0 [tls]
> [ 88.821283] ? remove_wait_queue+0x20/0x60
> [ 88.822383] ? n_tty_read+0x683/0x910
> [ 88.823363] tls_device_sendmsg+0x53/0xa0 [tls]
> [ 88.824505] sock_sendmsg+0x36/0x50
> [ 88.825492] sock_write_iter+0x87/0x100
> [ 88.826521] __vfs_write+0x127/0x1b0
> [ 88.827499] vfs_write+0xad/0x1b0
> [ 88.828454] ksys_write+0x52/0xc0
> [ 88.829378] do_syscall_64+0x5b/0x180
> [ 88.830369] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 88.831603] RIP: 0033:0x7fcad1451680
>
> [ 1248.470626] BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000000
> [ 1248.472564] #PF error: [normal kernel read fault]
> [ 1248.473790] PGD 0 P4D 0
> [ 1248.474642] Oops: 0000 [#1] SMP PTI
> [ 1248.475651] CPU: 3 PID: 7197 Comm: openssl Tainted: G OE 5.0.0-
> rc4+ #3
> [ 1248.477426] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS Bochs 01/01/2011
> [ 1248.479310] RIP: 0010:tls_tx_records+0x110/0x1f0 [tls]
> [ 1248.480644] Code: 00 02 48 89 43 08 e8 4f cb 63 d7 48 89 df e8 f7 9c
> 1b d7 4c 89 f8 4d 8b bf 98 00 00 00 48 05 98 00 00 00 48 89 04 24 49 39
> c7 <49> 8b 1f 4d 89 fd 0f 84 af 00 00 00 41 8b 47 10 85 c0 0f 85 8d 00
> [ 1248.484825] RSP: 0018:ffffaa0a41543c08 EFLAGS: 00010213
> [ 1248.486154] RAX: ffff955a2755dc98 RBX: ffff955a36031980 RCX:
> 0000000000000006
> [ 1248.487855] RDX: 0000000000000000 RSI: 000000000000002b RDI:
> 0000000000000286
> [ 1248.489524] RBP: ffff955a36031980 R08: 0000000000000000 R09:
> 00000000000002b1
> [ 1248.491394] R10: 0000000000000003 R11: 00000000ad55ad55 R12:
> 0000000000000000
> [ 1248.493162] R13: 0000000000000000 R14: ffff955a2abe6c00 R15:
> 0000000000000000
> [ 1248.494923] FS: 0000000000000000(0000) GS:ffff955a378c0000(0000)
> knlGS:0000000000000000
> [ 1248.496847] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1248.498357] CR2: 0000000000000000 CR3: 000000020c40e000 CR4:
> 00000000001406e0
> [ 1248.500136] Call Trace:
> [ 1248.500998] ? tcp_check_oom+0xd0/0xd0
> [ 1248.502106] tls_sk_proto_close+0x127/0x1e0 [tls]
> [ 1248.503411] inet_release+0x3c/0x60
> [ 1248.504530] __sock_release+0x3d/0xb0
> [ 1248.505611] sock_close+0x11/0x20
> [ 1248.506612] __fput+0xb4/0x220
> [ 1248.507559] task_work_run+0x88/0xa0
> [ 1248.508617] do_exit+0x2cb/0xbc0
> [ 1248.509597] ? core_sys_select+0x17a/0x280
> [ 1248.510740] do_group_exit+0x39/0xb0
> [ 1248.511789] get_signal+0x1d0/0x630
> [ 1248.512823] do_signal+0x36/0x620
> [ 1248.513822] exit_to_usermode_loop+0x5c/0xc6
> [ 1248.515003] do_syscall_64+0x157/0x180
> [ 1248.516094] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 1248.517456] RIP: 0033:0x7fb398bd3f53
> [ 1248.518537] Code: Bad RIP value.
>
> Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption of records
> for performance")
> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
> ---
> include/net/tls.h | 20 ++++----------------
> net/tls/tls_device.c | 9 +++++----
> net/tls/tls_main.c | 13 -------------
> 3 files changed, 9 insertions(+), 33 deletions(-)
>
> diff --git a/include/net/tls.h b/include/net/tls.h
> index 9f4117ae2297..a528a082da73 100644
> --- a/include/net/tls.h
> +++ b/include/net/tls.h
> @@ -199,10 +199,6 @@ struct tls_offload_context_tx {
> (ALIGN(sizeof(struct tls_offload_context_tx), sizeof(void *)) + \
> TLS_DRIVER_STATE_SIZE)
>
> -enum {
> - TLS_PENDING_CLOSED_RECORD
> -};
> -
> struct cipher_context {
> char *iv;
> char *rec_seq;
> @@ -335,17 +331,14 @@ int tls_push_sg(struct sock *sk, struct tls_context
> *ctx,
> int tls_push_partial_record(struct sock *sk, struct tls_context *ctx,
> int flags);
>
> -int tls_push_pending_closed_record(struct sock *sk, struct tls_context *ctx,
> - int flags, long *timeo);
> -
> static inline struct tls_msg *tls_msg(struct sk_buff *skb)
> {
> return (struct tls_msg *)strp_msg(skb);
> }
>
> -static inline bool tls_is_pending_closed_record(struct tls_context *ctx)
> +static inline bool tls_is_partially_sent_record(struct tls_context *ctx)
> {
> - return test_bit(TLS_PENDING_CLOSED_RECORD, &ctx->flags);
> + return !!ctx->partially_sent_record;
> }
>
> static inline int tls_complete_pending_work(struct sock *sk,
> @@ -357,17 +350,12 @@ static inline int tls_complete_pending_work(struct
> sock *sk,
> if (unlikely(sk->sk_write_pending))
> rc = wait_on_pending_writer(sk, timeo);
>
> - if (!rc && tls_is_pending_closed_record(ctx))
> - rc = tls_push_pending_closed_record(sk, ctx, flags, timeo);
> + if (!rc && tls_is_partially_sent_record(ctx))
> + rc = tls_push_partial_record(sk, ctx, flags);
>
> return rc;
> }
>
> -static inline bool tls_is_partially_sent_record(struct tls_context *ctx)
> -{
> - return !!ctx->partially_sent_record;
> -}
> -
> static inline bool tls_is_pending_open_record(struct tls_context *tls_ctx)
> {
> return tls_ctx->pending_open_record_frags;
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index a5c17c47d08a..3e5e8e021a87 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -271,7 +271,6 @@ static int tls_push_record(struct sock *sk,
> list_add_tail(&record->list, &offload_ctx->records_list);
> spin_unlock_irq(&offload_ctx->lock);
> offload_ctx->open_record = NULL;
> - set_bit(TLS_PENDING_CLOSED_RECORD, &ctx->flags);
> tls_advance_record_sn(sk, &ctx->tx, ctx->crypto_send.info.version);
>
> for (i = 0; i < record->num_frags; i++) {
> @@ -368,9 +367,11 @@ static int tls_push_data(struct sock *sk,
> return -sk->sk_err;
>
> timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT);
> - rc = tls_complete_pending_work(sk, tls_ctx, flags, &timeo);
> - if (rc < 0)
> - return rc;
> + if (tls_is_partially_sent_record(tls_ctx)) {
> + rc = tls_push_partial_record(sk, tls_ctx, flags);
> + if (rc < 0)
> + return rc;
> + }
>
> pfrag = sk_page_frag(sk);
>
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index caff15b2f9b2..7e05af75536d 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -209,19 +209,6 @@ int tls_push_partial_record(struct sock *sk, struct
> tls_context *ctx,
> return tls_push_sg(sk, ctx, sg, offset, flags);
> }
>
> -int tls_push_pending_closed_record(struct sock *sk,
> - struct tls_context *tls_ctx,
> - int flags, long *timeo)
> -{
> - struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> -
> - if (tls_is_partially_sent_record(tls_ctx) ||
> - !list_empty(&ctx->tx_list))
> - return tls_tx_records(sk, flags);
> - else
> - return tls_ctx->push_pending_record(sk, flags);
> -}
> -
> static void tls_write_space(struct sock *sk)
> {
> struct tls_context *ctx = tls_get_ctx(sk);
> --
> 2.12.2
^ permalink raw reply
* Re: [PATCH] bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id()
From: zerons @ 2019-02-26 14:58 UTC (permalink / raw)
To: Daniel Borkmann, ast; +Cc: netdev, linux-kernel
In-Reply-To: <564058e0-1432-676e-2f83-c8694911d8e5@iogearbox.net>
On 2/26/19 22:44, Daniel Borkmann wrote:
> On 02/26/2019 03:15 PM, zerons wrote:
>> [ Upstream commit c91951f15978f1a0c6b65f063d30f7ea7bc6fb42 ]
>
> Thanks for the fix! What do you mean by "upstream commit" above in this context?
>
This patch is based on that commit, I thought I should mention this.
Sorry for the confusion.
^ permalink raw reply
* RE: [PATCH net 4/4] tls: Fix tls_device receive
From: Vakul Garg @ 2019-02-26 15:01 UTC (permalink / raw)
To: Boris Pismenny, aviadye@mellanox.com, davejwatson@fb.com,
john.fastabend@gmail.com, daniel@iogearbox.net,
netdev@vger.kernel.org
Cc: eranbe@mellanox.com
In-Reply-To: <20190226121235.20784-5-borisp@mellanox.com>
> -----Original Message-----
> From: Boris Pismenny <borisp@mellanox.com>
> Sent: Tuesday, February 26, 2019 5:43 PM
> To: aviadye@mellanox.com; davejwatson@fb.com;
> john.fastabend@gmail.com; daniel@iogearbox.net; Vakul Garg
> <vakul.garg@nxp.com>; netdev@vger.kernel.org
> Cc: eranbe@mellanox.com; borisp@mellanox.com
> Subject: [PATCH net 4/4] tls: Fix tls_device receive
>
> Currently, the receive function fails to handle records already decrypted by
> the device due to the commit mentioned below.
>
> This commit advances the TLS record sequence number and prepares the
> context to handle the next record.
>
> Fixes: fedf201e1296 ("net: tls: Refactor control message handling on recv")
> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
> Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com>
> ---
> net/tls/tls_sw.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index
> f515cd7e984e..85da10182d8d 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -1481,18 +1481,17 @@ static int decrypt_skb_update(struct sock *sk,
> struct sk_buff *skb,
>
> return err;
> }
> -
> - rxm->full_len -= padding_length(ctx, tls_ctx, skb);
> -
> - rxm->offset += prot->prepend_size;
> - rxm->full_len -= prot->overhead_size;
> - tls_advance_record_sn(sk, &tls_ctx->rx, version);
> - ctx->decrypted = true;
> - ctx->saved_data_ready(sk);
> } else {
> *zc = false;
> }
>
> + rxm->full_len -= padding_length(ctx, tls_ctx, skb);
> + rxm->offset += prot->prepend_size;
> + rxm->full_len -= prot->overhead_size;
> + tls_advance_record_sn(sk, &tls_ctx->rx, version);
> + ctx->decrypted = true;
> + ctx->saved_data_ready(sk);
> +
> return err;
> }
>
> --
> 2.12.2
Reviewed-by: Vakul Garg <vakul.garg@nxp.com>
^ permalink raw reply
* Re: [PATCH net 1/4] tls: Fix tls_device handling of partial records
From: Boris Pismenny @ 2019-02-26 15:05 UTC (permalink / raw)
To: Vakul Garg, Aviad Yehezkel, davejwatson@fb.com,
john.fastabend@gmail.com, daniel@iogearbox.net,
netdev@vger.kernel.org
Cc: Eran Ben Elisha
In-Reply-To: <DB7PR04MB425213EFED43D2631988C6B98B7B0@DB7PR04MB4252.eurprd04.prod.outlook.com>
On 2/26/2019 4:57 PM, Vakul Garg wrote:
>
>
>> -----Original Message-----
>> From: Boris Pismenny <borisp@mellanox.com>
>> Sent: Tuesday, February 26, 2019 5:43 PM
>> To: aviadye@mellanox.com; davejwatson@fb.com;
>> john.fastabend@gmail.com; daniel@iogearbox.net; Vakul Garg
>> <vakul.garg@nxp.com>; netdev@vger.kernel.org
>> Cc: eranbe@mellanox.com; borisp@mellanox.com
>> Subject: [PATCH net 1/4] tls: Fix tls_device handling of partial records
>>
>> Cleanup the handling of partial records while fixing a bug where the
>> tls_push_pending_closed_record function is using the software tls
>> context instead of the hardware context.
>
> Can you provide details of what cleanup has been done?
> I see that we got rid of concept of 'TLS_PENDING_CLOSED_RECORD'.
> I vaguely remember that at one point in time, it seemed to me redundant.
> But I was not sure. Please confirm if it is the case.
>
The cleanup refers to the PENDING_CLOSED_RECORD. This code was
previously used by both tls_sw and tls_device to handle the closed
records. However, at some point tls_sw moved to using the partially sent
record code, which is equivalent. So this code became unused after we
fixed the tls_device code path, and this is why it is removed here.
> Can this patch be split into two? One for the cleanup and one for the bug.
>
The bug fix will cause the PENDING_CLOSED_RECORD code to be unused. IMO,
it is better to keep this as-is to avoid this.
>>
>> The bug resulted in the following crash:
>> [ 88.791229] BUG: unable to handle kernel NULL pointer dereference at
>> 0000000000000000
>> [ 88.793271] #PF error: [normal kernel read fault]
>> [ 88.794449] PGD 800000022a426067 P4D 800000022a426067 PUD
>> 22a156067 PMD 0
>> [ 88.795958] Oops: 0000 [#1] SMP PTI
>> [ 88.796884] CPU: 2 PID: 4973 Comm: openssl Not tainted 5.0.0-rc4+ #3
>> [ 88.798314] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>> BIOS Bochs 01/01/2011
>> [ 88.800067] RIP: 0010:tls_tx_records+0xef/0x1d0 [tls]
>> [ 88.801256] Code: 00 02 48 89 43 08 e8 a0 0b 96 d9 48 89 df e8 48 dd
>> 4d d9 4c 89 f8 4d 8b bf 98 00 00 00 48 05 98 00 00 00 48 89 04 24 49 39
>> c7 <49> 8b 1f 4d 89 fd 0f 84 af 00 00 00 41 8b 47 10 85 c0 0f 85 8d 00
>> [ 88.805179] RSP: 0018:ffffbd888186fca8 EFLAGS: 00010213
>> [ 88.806458] RAX: ffff9af1ed657c98 RBX: ffff9af1e88a1980 RCX:
>> 0000000000000000
>> [ 88.808050] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
>> ffff9af1e88a1980
>> [ 88.809724] RBP: ffff9af1e88a1980 R08: 0000000000000017 R09:
>> ffff9af1ebeeb700
>> [ 88.811294] R10: 0000000000000000 R11: 0000000000000000 R12:
>> 0000000000000000
>> [ 88.812917] R13: ffff9af1e88a1980 R14: ffff9af1ec13f800 R15:
>> 0000000000000000
>> [ 88.814506] FS: 00007fcad2240740(0000) GS:ffff9af1f7880000(0000)
>> knlGS:0000000000000000
>> [ 88.816337] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 88.817717] CR2: 0000000000000000 CR3: 0000000228b3e000 CR4:
>> 00000000001406e0
>> [ 88.819328] Call Trace:
>> [ 88.820123] tls_push_data+0x628/0x6a0 [tls]
>> [ 88.821283] ? remove_wait_queue+0x20/0x60
>> [ 88.822383] ? n_tty_read+0x683/0x910
>> [ 88.823363] tls_device_sendmsg+0x53/0xa0 [tls]
>> [ 88.824505] sock_sendmsg+0x36/0x50
>> [ 88.825492] sock_write_iter+0x87/0x100
>> [ 88.826521] __vfs_write+0x127/0x1b0
>> [ 88.827499] vfs_write+0xad/0x1b0
>> [ 88.828454] ksys_write+0x52/0xc0
>> [ 88.829378] do_syscall_64+0x5b/0x180
>> [ 88.830369] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [ 88.831603] RIP: 0033:0x7fcad1451680
>>
>> [ 1248.470626] BUG: unable to handle kernel NULL pointer dereference at
>> 0000000000000000
>> [ 1248.472564] #PF error: [normal kernel read fault]
>> [ 1248.473790] PGD 0 P4D 0
>> [ 1248.474642] Oops: 0000 [#1] SMP PTI
>> [ 1248.475651] CPU: 3 PID: 7197 Comm: openssl Tainted: G OE 5.0.0-
>> rc4+ #3
>> [ 1248.477426] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>> BIOS Bochs 01/01/2011
>> [ 1248.479310] RIP: 0010:tls_tx_records+0x110/0x1f0 [tls]
>> [ 1248.480644] Code: 00 02 48 89 43 08 e8 4f cb 63 d7 48 89 df e8 f7 9c
>> 1b d7 4c 89 f8 4d 8b bf 98 00 00 00 48 05 98 00 00 00 48 89 04 24 49 39
>> c7 <49> 8b 1f 4d 89 fd 0f 84 af 00 00 00 41 8b 47 10 85 c0 0f 85 8d 00
>> [ 1248.484825] RSP: 0018:ffffaa0a41543c08 EFLAGS: 00010213
>> [ 1248.486154] RAX: ffff955a2755dc98 RBX: ffff955a36031980 RCX:
>> 0000000000000006
>> [ 1248.487855] RDX: 0000000000000000 RSI: 000000000000002b RDI:
>> 0000000000000286
>> [ 1248.489524] RBP: ffff955a36031980 R08: 0000000000000000 R09:
>> 00000000000002b1
>> [ 1248.491394] R10: 0000000000000003 R11: 00000000ad55ad55 R12:
>> 0000000000000000
>> [ 1248.493162] R13: 0000000000000000 R14: ffff955a2abe6c00 R15:
>> 0000000000000000
>> [ 1248.494923] FS: 0000000000000000(0000) GS:ffff955a378c0000(0000)
>> knlGS:0000000000000000
>> [ 1248.496847] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 1248.498357] CR2: 0000000000000000 CR3: 000000020c40e000 CR4:
>> 00000000001406e0
>> [ 1248.500136] Call Trace:
>> [ 1248.500998] ? tcp_check_oom+0xd0/0xd0
>> [ 1248.502106] tls_sk_proto_close+0x127/0x1e0 [tls]
>> [ 1248.503411] inet_release+0x3c/0x60
>> [ 1248.504530] __sock_release+0x3d/0xb0
>> [ 1248.505611] sock_close+0x11/0x20
>> [ 1248.506612] __fput+0xb4/0x220
>> [ 1248.507559] task_work_run+0x88/0xa0
>> [ 1248.508617] do_exit+0x2cb/0xbc0
>> [ 1248.509597] ? core_sys_select+0x17a/0x280
>> [ 1248.510740] do_group_exit+0x39/0xb0
>> [ 1248.511789] get_signal+0x1d0/0x630
>> [ 1248.512823] do_signal+0x36/0x620
>> [ 1248.513822] exit_to_usermode_loop+0x5c/0xc6
>> [ 1248.515003] do_syscall_64+0x157/0x180
>> [ 1248.516094] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [ 1248.517456] RIP: 0033:0x7fb398bd3f53
>> [ 1248.518537] Code: Bad RIP value.
>>
>> Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption of records
>> for performance")
>> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>> ---
>> include/net/tls.h | 20 ++++----------------
>> net/tls/tls_device.c | 9 +++++----
>> net/tls/tls_main.c | 13 -------------
>> 3 files changed, 9 insertions(+), 33 deletions(-)
>>
>> diff --git a/include/net/tls.h b/include/net/tls.h
>> index 9f4117ae2297..a528a082da73 100644
>> --- a/include/net/tls.h
>> +++ b/include/net/tls.h
>> @@ -199,10 +199,6 @@ struct tls_offload_context_tx {
>> (ALIGN(sizeof(struct tls_offload_context_tx), sizeof(void *)) + \
>> TLS_DRIVER_STATE_SIZE)
>>
>> -enum {
>> - TLS_PENDING_CLOSED_RECORD
>> -};
>> -
>> struct cipher_context {
>> char *iv;
>> char *rec_seq;
>> @@ -335,17 +331,14 @@ int tls_push_sg(struct sock *sk, struct tls_context
>> *ctx,
>> int tls_push_partial_record(struct sock *sk, struct tls_context *ctx,
>> int flags);
>>
>> -int tls_push_pending_closed_record(struct sock *sk, struct tls_context *ctx,
>> - int flags, long *timeo);
>> -
>> static inline struct tls_msg *tls_msg(struct sk_buff *skb)
>> {
>> return (struct tls_msg *)strp_msg(skb);
>> }
>>
>> -static inline bool tls_is_pending_closed_record(struct tls_context *ctx)
>> +static inline bool tls_is_partially_sent_record(struct tls_context *ctx)
>> {
>> - return test_bit(TLS_PENDING_CLOSED_RECORD, &ctx->flags);
>> + return !!ctx->partially_sent_record;
>> }
>>
>> static inline int tls_complete_pending_work(struct sock *sk,
>> @@ -357,17 +350,12 @@ static inline int tls_complete_pending_work(struct
>> sock *sk,
>> if (unlikely(sk->sk_write_pending))
>> rc = wait_on_pending_writer(sk, timeo);
>>
>> - if (!rc && tls_is_pending_closed_record(ctx))
>> - rc = tls_push_pending_closed_record(sk, ctx, flags, timeo);
>> + if (!rc && tls_is_partially_sent_record(ctx))
>> + rc = tls_push_partial_record(sk, ctx, flags);
>>
>> return rc;
>> }
>>
>> -static inline bool tls_is_partially_sent_record(struct tls_context *ctx)
>> -{
>> - return !!ctx->partially_sent_record;
>> -}
>> -
>> static inline bool tls_is_pending_open_record(struct tls_context *tls_ctx)
>> {
>> return tls_ctx->pending_open_record_frags;
>> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
>> index a5c17c47d08a..3e5e8e021a87 100644
>> --- a/net/tls/tls_device.c
>> +++ b/net/tls/tls_device.c
>> @@ -271,7 +271,6 @@ static int tls_push_record(struct sock *sk,
>> list_add_tail(&record->list, &offload_ctx->records_list);
>> spin_unlock_irq(&offload_ctx->lock);
>> offload_ctx->open_record = NULL;
>> - set_bit(TLS_PENDING_CLOSED_RECORD, &ctx->flags);
>> tls_advance_record_sn(sk, &ctx->tx, ctx->crypto_send.info.version);
>>
>> for (i = 0; i < record->num_frags; i++) {
>> @@ -368,9 +367,11 @@ static int tls_push_data(struct sock *sk,
>> return -sk->sk_err;
>>
>> timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT);
>> - rc = tls_complete_pending_work(sk, tls_ctx, flags, &timeo);
>> - if (rc < 0)
>> - return rc;
>> + if (tls_is_partially_sent_record(tls_ctx)) {
>> + rc = tls_push_partial_record(sk, tls_ctx, flags);
>> + if (rc < 0)
>> + return rc;
>> + }
>>
>> pfrag = sk_page_frag(sk);
>>
>> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
>> index caff15b2f9b2..7e05af75536d 100644
>> --- a/net/tls/tls_main.c
>> +++ b/net/tls/tls_main.c
>> @@ -209,19 +209,6 @@ int tls_push_partial_record(struct sock *sk, struct
>> tls_context *ctx,
>> return tls_push_sg(sk, ctx, sg, offset, flags);
>> }
>>
>> -int tls_push_pending_closed_record(struct sock *sk,
>> - struct tls_context *tls_ctx,
>> - int flags, long *timeo)
>> -{
>> - struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
>> -
>> - if (tls_is_partially_sent_record(tls_ctx) ||
>> - !list_empty(&ctx->tx_list))
>> - return tls_tx_records(sk, flags);
>> - else
>> - return tls_ctx->push_pending_record(sk, flags);
>> -}
>> -
>> static void tls_write_space(struct sock *sk)
>> {
>> struct tls_context *ctx = tls_get_ctx(sk);
>> --
>> 2.12.2
>
^ permalink raw reply
* Re: [PATCH net-next] net: sched: set dedicated tcf_walker flag when tp is empty
From: Vlad Buslov @ 2019-02-26 15:08 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <CAM_iQpWmRQP8-xBiULnCDHkDTHW7WaKuBmcsYJ7j-yRxfgwOaA@mail.gmail.com>
On Mon 25 Feb 2019 at 22:52, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
>> at least once is unreliable. Some classifiers set 'stop' flag on error
>> before calling walker callback, other classifiers used to call it with NULL
>> filter pointer when empty. In order to prevent further regressions, extend
>> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
>> tcf_walker->fn() implementation that is used to check if classifier has
>> filters configured.
>
>
> So, after this patch commits like 31a998487641 ("net: sched: fw: don't
> set arg->stop in fw_walk() when empty") can be reverted??
Yes, it is safe now to revert following commits:
3027ff41f67c ("net: sched: route: don't set arg->stop in route4_walk() when empty")
31a998487641 ("net: sched: fw: don't set arg->stop in fw_walk() when empty")
>
>
>>
>> Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
>> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
>> ---
>> include/net/pkt_cls.h | 1 +
>> net/sched/cls_api.c | 13 +++++++++----
>> 2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>> index 232f801f2a21..422dd8800478 100644
>> --- a/include/net/pkt_cls.h
>> +++ b/include/net/pkt_cls.h
>> @@ -17,6 +17,7 @@ struct tcf_walker {
>> int stop;
>> int skip;
>> int count;
>> + bool nonempty;
>> unsigned long cookie;
>> int (*fn)(struct tcf_proto *, void *node, struct tcf_walker *);
>> };
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index e2c888961379..3543be31d400 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -238,18 +238,23 @@ static void tcf_proto_put(struct tcf_proto *tp, bool rtnl_held,
>> tcf_proto_destroy(tp, rtnl_held, extack);
>> }
>>
>> -static int walker_noop(struct tcf_proto *tp, void *d, struct tcf_walker *arg)
>> +static int walker_check_empty(struct tcf_proto *tp, void *d,
>> + struct tcf_walker *arg)
>> {
>> - return -1;
>> + if (tp) {
>> + arg->nonempty = true;
>> + return -1;
>> + }
>> + return 0;
>
> How does this even work? If we can simply check tp!=NULL as
> non-empty, why do we even need a walker??
>
> For me, it must be pushed down to each implementation to
> determine how it is empty.
Sorry, this is a typo. Intention is to check the filter pointer (void
*d). Sending the fix.
Thanks for spotting this!
^ permalink raw reply
* Re: [PATCH] tcp: fix __tcp_transmit_skb's comment text
From: Eric Dumazet @ 2019-02-26 15:11 UTC (permalink / raw)
To: Geliang Tang, Eric Dumazet, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI
Cc: netdev, linux-kernel
In-Reply-To: <5599b6d78fa7f5a05d40123d43f955174c2c5baa.1551169935.git.geliangtang@gmail.com>
On 02/26/2019 12:41 AM, Geliang Tang wrote:
> The function name tcp_do_sendmsg has been renamed. But it still
> appears in __tcp_transmit_skb's comment text. This patch changes
> it to tcp_sendmsg_locked.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> net/ipv4/tcp_output.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index e72aa0ff5785..67a43b966b8a 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1003,7 +1003,7 @@ static void tcp_update_skb_after_send(struct sock *sk, struct sk_buff *skb,
> }
>
> /* This routine actually transmits TCP packets queued in by
> - * tcp_do_sendmsg(). This is used by both the initial
> + * tcp_sendmsg_locked(). This is used by both the initial
> * transmission and possible later retransmissions.
> * All SKB's seen here are completely headerless. It is our
> * job to build the TCP header, and pass the packet down to
>
Not sure your patch helps, packets can also be put in write queue
by many different functions, including tcp_sendmsg_locked().
^ permalink raw reply
* Re: [PATCH net] tcp: repaired skbs must init their tso_segs
From: Eric Dumazet @ 2019-02-26 15:13 UTC (permalink / raw)
To: Andrei Vagin, Eric Dumazet
Cc: David S . Miller, netdev, Eric Dumazet, Soheil Hassas Yeganeh,
Neal Cardwell, Yuchung Cheng, syzbot, Andrey Vagin
In-Reply-To: <20190226092302.GA25925@gmail.com>
On 02/26/2019 01:23 AM, Andrei Vagin wrote:
>
> Thank you Eric. I saw a few test fails when tcp_peek_sndq()
> returned more data than we expected. I have executed the test with this
> fix in a loop and it works without any problem. Without this fix, it
> fails after a few iteration.
>
> https://github.com/checkpoint-restore/criu/issues/622
>
Hi Andrei.
Thanks for testing !
^ permalink raw reply
* Re: [PATCH RFC] net: Validate size of non-TSO packets in validate_xmit_skb().
From: Eric Dumazet @ 2019-02-26 15:16 UTC (permalink / raw)
To: Michael Chan, davem, maheshb, edumazet; +Cc: dja, netdev
In-Reply-To: <1551178601-16564-1-git-send-email-michael.chan@broadcom.com>
On 02/26/2019 02:56 AM, Michael Chan wrote:
> There have been reports of oversize UDP packets being sent to the
> driver to be transmitted, causing error conditions. The issue is
> likely caused by the dst of the SKB switching between 'lo' with
> 64K MTU and the hardware device with a smaller MTU. Patches are
> being proposed by Mahesh Bandewar <maheshb@google.com> to fix the
> issue.
>
> Separately, we should add a length check in validate_xmit_skb()
> to drop these oversize packets before they reach the driver.
Why ?
We keep adding checks in the 'fast path' and make slower and slower after each release.
We need to fix the root cause really.
> This patch only validates non-TSO packets. Complete validation
> of segmented TSO packet size will probably be too slow.
>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
> net/core/dev.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5d03889..50c5174 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3373,6 +3373,13 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
> }
> }
>
> + if (!skb_is_gso(skb) &&
> + skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN)) {
> + net_warn_ratelimited("%s(): Dropping %d bytes oversize skb.\n",
> + __func__, skb->len);
> + goto out_kfree_skb;
> + }
> +
> skb = validate_xmit_xfrm(skb, features, again);
>
> return skb;
>
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: Joel Fernandes @ 2019-02-26 15:24 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Linus Torvalds, Masami Hiramatsu, Steven Rostedt, Andy Lutomirski,
will.deacon, Linux List Kernel Mailing, Ingo Molnar,
Andrew Morton, stable, Changbin Du, Jann Horn, Kees Cook,
Andy Lutomirski, daniel, netdev, bpf
In-Reply-To: <20190222192703.epvgxghwybte7gxs@ast-mbp.dhcp.thefacebook.com>
On Fri, Feb 22, 2019 at 11:27:05AM -0800, Alexei Starovoitov wrote:
> On Fri, Feb 22, 2019 at 09:43:14AM -0800, Linus Torvalds wrote:
> >
> > Then we should still probably fix up "__probe_kernel_read()" to not
> > allow user accesses. The easiest way to do that is actually likely to
> > use the "unsafe_get_user()" functions *without* doing a
> > uaccess_begin(), which will mean that modern CPU's will simply fault
> > on a kernel access to user space.
>
> On bpf side the bpf_probe_read() helper just calls probe_kernel_read()
> and users pass both user and kernel addresses into it and expect
> that the helper will actually try to read from that address.
Slightly related and FWIW, BCC's eBPF-based opensnoop tool [1] installs a
kprobe on do_sys_open to monitor calls to the open syscall globally.
do_sys_open() has prototype:
long do_sys_open(int dfd, const char __user *filename, int flags, umode_t mode);
This causes a "blank" filename to be displayed by opensnoop when I run it on
my Pixel 3 (arm64), possibly because this is a user pointer. However, it
works fine on x86-64.
So it seems to me that on arm64, reading user pointers directly still doesn't
work even if there is a distinction between user/kernel addresses. In that
case reading the user pointer using user accessors (possibly using
bpf_probe_user_read helper) should be needed to fix this issue (as Yonghong
also privately discussed with me).
[1] https://github.com/iovisor/bcc/blob/master/tools/opensnoop.py#L140
thanks!
- Joel
>
> If __probe_kernel_read will suddenly start failing on all user addresses
> it will break the expectations.
> How do we solve it in bpf_probe_read?
> Call probe_kernel_read and if that fails call unsafe_get_user byte-by-byte
> in the loop?
> That's doable, but people already complain that bpf_probe_read() is slow
> and shows up in their perf report.
>
^ permalink raw reply
* Re: [PATCH] can: mark expected switch fall-throughs
From: Gustavo A. R. Silva @ 2019-02-26 15:24 UTC (permalink / raw)
To: Marc Kleine-Budde, Wolfgang Grandegger, David S. Miller,
Nicolas Ferre, Alexandre Belloni, Ludovic Desroches
Cc: linux-can, netdev, linux-arm-kernel, linux-kernel
In-Reply-To: <31d206cd-65f2-66be-ed79-583210a88d57@pengutronix.de>
On 2/26/19 2:02 AM, Marc Kleine-Budde wrote:
> On 1/29/19 7:06 PM, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> where we are expecting to fall through.
>>
>> This patch fixes the following warnings:
>>
>> drivers/net/can/peak_canfd/peak_pciefd_main.c:668:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> drivers/net/can/spi/mcp251x.c:875:7: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> drivers/net/can/usb/peak_usb/pcan_usb.c:422:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> drivers/net/can/at91_can.c:895:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> drivers/net/can/at91_can.c:953:15: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> drivers/net/can/usb/peak_usb/pcan_usb.c: In function ‘pcan_usb_decode_error’:
>> drivers/net/can/usb/peak_usb/pcan_usb.c:422:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> if (n & PCAN_USB_ERROR_BUS_LIGHT) {
>> ^
>> drivers/net/can/usb/peak_usb/pcan_usb.c:428:2: note: here
>> case CAN_STATE_ERROR_WARNING:
>> ^~~~
>>
>> Warning level 3 was used: -Wimplicit-fallthrough=3
>>
>> This patch is part of the ongoing efforts to enabling
>> -Wimplicit-fallthrough.
>>
>> Notice that in some cases spelling mistakes were fixed.
>> In other cases, the /* fall through */ comment is placed
>> at the bottom of the case statement, which is what GCC
>> is expecting to find.
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>
> Added to linux-can-next.
>
Thanks, Marc.
--
Gustavo
^ permalink raw reply
* Re: [PATCH bpf-next 1/4] bpf: enable program stats
From: Daniel Borkmann @ 2019-02-26 15:29 UTC (permalink / raw)
To: Alexei Starovoitov, Alexei Starovoitov
Cc: Roman Gushchin, Alexei Starovoitov, davem@davemloft.net,
netdev@vger.kernel.org, bpf@vger.kernel.org, Kernel Team
In-Reply-To: <24d2c526-3f9c-3101-ef60-d92777ebdcb7@fb.com>
On 02/26/2019 05:27 AM, Alexei Starovoitov wrote:
> On 2/25/19 2:36 AM, Daniel Borkmann wrote:
>>
>> Not through the stack, but was more thinking something like low-overhead
>> kprobes-style extension for a BPF prog where such sequence would be added
>> 'inline' at beginning / exit of BPF prog invocation with normal ctx access
>> and helpers as the native program (e.g. time-stamping plus read/write into
>> mark as one example which kprobes couldn't do). But might go much beyond
>> context of this stats collection.
>
> see below.
>
>> That's actually an interesting thought, given the original prog->bpf_func
>> is a known address at that time, this could be templated where an inner
>> dummy bpf_func call to the normal BPF prog gets later replaced with the
>> actual prog->bpf_func address to have no indirect call. This would also
>> allow to keep the actual prog->bpf_func entry call-sites small and simple.
>
> My first thought was that we indeed can have a template of stats
> collecting wrapper in .text,
> then at 'switch stats on' event vmalloc exec region, copy wrapper code
> into it and manually patch 'call foo' x86 insn with direct jump.
> That is still quite involved from coding side.
> It's similar to what kprobe/ftrace is doing, but probably
> an overkill for stats due to potential security
> concerns with new executable code.
> But it won't work due to tail_calls.
You mean for the scenario to measure the _individual_ progs that
are part of the given tail call processing such that each have
their individual inc in count and time-spent attribution. If so,
yeah, agree. Or for the case you have right now where everything
gets attributed to the "entry" program that triggers the tail
call processing? Latter would be similar as we have now in this
patch, imho.
> I've looked into single wrapper approach with indirect call
> (because it's much easer than patching x86) and realized it
> won't work for the same reason.
> Inline kprobe+kretprobe insertion won't work either.
> All because we have tail_calls.
> we cannot have executable epilogue in progs.
> tail_call will jmp into next prog and second part of stats collection
> won't run. Essentially no epilogue allowed unless we disable tail_call.
> Or we somehow reimplement tail_calls so that prog returns,
> an epilogue is executed and then jumps into the next prog.
> tail_call turned out be the worst corner case feature to deal with.
Agree, it's a pain as in most cases. Was more thinking that
'special' epilogue would inc the count, start first ktime/cycle
snapshot and then do a direct function call to the normally
JITed BPF prog (where it can do tail calls, etc, so nothing
changes there), and upon return from this call take the second
measurement, and update the per CPU buffer, similar as we have
now just with static keys approach. So, was thinking similar as
we JIT BPF-to-BPF calls on an individual prog basis.
> With static_key approach the main prog accounts the time
> of all progs called via tail_call.
> We can argue whether it's ideal semantics, but looks like
> it's the only thing we can do.
Yeah true, it's more about measuring how long the prog 'chain'
in the whole hook is taking to complete, which may be interesting
in itself, just perhaps a bit confusing when looking at individual
progs' share.
> I don't see a way how tail_called progs can count their own time.
Me neither, if so I guess it would be of similar fashion as you
have right now where it's accounted to the first prog.
> Stats would somehow need to be computed right before jumping
> into next prog. Which means heavy changes in all JITs
> plus extra performance cost at runtime, since such if (stats_on)
> check in tail_call handling would have to be done at runtime,
> since JITed code is read-only and regenerating progs due to stats
> is non-starter.
> Or tail-called next prog would need to update stats for previous
> prog, but there is no pointer to 'aux->stats' there.
> So imo that is dead end.
> static_key approach is the simplest by far.
It's simplest, yep. I guess if we get to the point of removing
indirect call for BPF_PROG_RUN itself, we might need to revisit
then and move it into bpf_func at that point. Kind of implementation
detail given we only expose count and time spent data to uapi,
though the direct switch at runtime may be a bit problematic in
future. Anyway, lets see how it goes when we get there.
Thanks,
Daniel
^ permalink raw reply
* [PATCH net-next] net: sched: fix typo in walker_check_empty()
From: Vlad Buslov @ 2019-02-26 15:34 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov
Function walker_check_empty() incorrectly verifies that tp pointer is not
NULL, instead of actual filter pointer. Fix conditional to check the right
pointer. Adjust filter pointer naming accordingly to other cls API
functions.
Fixes: 6676d5e416ee ("net: sched: set dedicated tcf_walker flag when tp is empty")
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Reported-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/sched/cls_api.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index cffb7f710de7..578051f8f448 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -238,10 +238,10 @@ static void tcf_proto_put(struct tcf_proto *tp, bool rtnl_held,
tcf_proto_destroy(tp, rtnl_held, extack);
}
-static int walker_check_empty(struct tcf_proto *tp, void *d,
+static int walker_check_empty(struct tcf_proto *tp, void *fh,
struct tcf_walker *arg)
{
- if (tp) {
+ if (fh) {
arg->nonempty = true;
return -1;
}
--
2.13.6
^ permalink raw reply related
* [PATCH net-next] tc-testing: gitignore, ignore local tdc config file
From: Vlad Buslov @ 2019-02-26 15:37 UTC (permalink / raw)
To: netdev; +Cc: jhs, davem, shuah, lucasb, Vlad Buslov
Comment in tdc_config.py recommends putting customizations in
tdc_config_local.py file that wasn't included in gitignore. Add the local
config file to gitignore.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
tools/testing/selftests/tc-testing/.gitignore | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/tc-testing/.gitignore b/tools/testing/selftests/tc-testing/.gitignore
index c5cc160948b3..c26d72e0166f 100644
--- a/tools/testing/selftests/tc-testing/.gitignore
+++ b/tools/testing/selftests/tc-testing/.gitignore
@@ -3,3 +3,4 @@ __pycache__/
plugins/
*.xml
*.tap
+tdc_config_local.py
--
2.13.6
^ permalink raw reply related
* [PATCH net] net: aquantia: regression on cpus with high cores: set mode with 8 queues
From: Igor Russkikh @ 2019-02-26 15:39 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev@vger.kernel.org, Igor Russkikh, Dmitry Bogdanov
From: Dmitry Bogdanov <dmitry.bogdanov@aquantia.com>
Recently the maximum number of queues was increased up to 8, but
NIC was not fully configured for 8 queues. In setups with more than 4 CPU
cores parts of TX traffic gets lost if the kernel routes it to queues 4th-8th.
This patch sets a tx hw traffic mode with 8 queues.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=202651
Fixes: 71a963cfc50b ("net: aquantia: increase max number of hw queues")
Reported-by: Nicholas Johnson <nicholas.johnson@outlook.com.au>
Signed-off-by: Dmitry Bogdanov <dmitry.bogdanov@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
.../ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 3 +++
.../ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c | 9 +++++++++
.../ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h | 4 ++++
.../aquantia/atlantic/hw_atl/hw_atl_llh_internal.h | 13 +++++++++++++
4 files changed, 29 insertions(+)
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
index b58ca7cb8e9d..fbba300c1d01 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
@@ -275,6 +275,9 @@ static int hw_atl_b0_hw_offload_set(struct aq_hw_s *self,
static int hw_atl_b0_hw_init_tx_path(struct aq_hw_s *self)
{
+ /* Tx TC/Queue number config */
+ hw_atl_rpb_tps_tx_tc_mode_set(self, 1U);
+
hw_atl_thm_lso_tcp_flag_of_first_pkt_set(self, 0x0FF6U);
hw_atl_thm_lso_tcp_flag_of_middle_pkt_set(self, 0x0FF6U);
hw_atl_thm_lso_tcp_flag_of_last_pkt_set(self, 0x0F7FU);
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c
index 939f77e2e117..8ac7a67b15c1 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c
@@ -1274,6 +1274,15 @@ void hw_atl_tpb_tx_buff_en_set(struct aq_hw_s *aq_hw, u32 tx_buff_en)
HW_ATL_TPB_TX_BUF_EN_SHIFT, tx_buff_en);
}
+void hw_atl_rpb_tps_tx_tc_mode_set(struct aq_hw_s *aq_hw,
+ u32 tx_traf_class_mode)
+{
+ aq_hw_write_reg_bit(aq_hw, HW_ATL_TPB_TX_TC_MODE_ADDR,
+ HW_ATL_TPB_TX_TC_MODE_MSK,
+ HW_ATL_TPB_TX_TC_MODE_SHIFT,
+ tx_traf_class_mode);
+}
+
void hw_atl_tpb_tx_buff_hi_threshold_per_tc_set(struct aq_hw_s *aq_hw,
u32 tx_buff_hi_threshold_per_tc,
u32 buffer)
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h
index 03c570d115fe..f529540bfd7e 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h
@@ -605,6 +605,10 @@ void hw_atl_thm_lso_tcp_flag_of_middle_pkt_set(struct aq_hw_s *aq_hw,
/* tpb */
+/* set TX Traffic Class Mode */
+void hw_atl_rpb_tps_tx_tc_mode_set(struct aq_hw_s *aq_hw,
+ u32 tx_traf_class_mode);
+
/* set tx buffer enable */
void hw_atl_tpb_tx_buff_en_set(struct aq_hw_s *aq_hw, u32 tx_buff_en);
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h
index 8470d92db812..e91ffce005f1 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h
@@ -1948,6 +1948,19 @@
/* default value of bitfield tx_buf_en */
#define HW_ATL_TPB_TX_BUF_EN_DEFAULT 0x0
+/* register address for bitfield tx_tc_mode */
+#define HW_ATL_TPB_TX_TC_MODE_ADDR 0x00007900
+/* bitmask for bitfield tx_tc_mode */
+#define HW_ATL_TPB_TX_TC_MODE_MSK 0x00000100
+/* inverted bitmask for bitfield tx_tc_mode */
+#define HW_ATL_TPB_TX_TC_MODE_MSKN 0xFFFFFEFF
+/* lower bit position of bitfield tx_tc_mode */
+#define HW_ATL_TPB_TX_TC_MODE_SHIFT 8
+/* width of bitfield tx_tc_mode */
+#define HW_ATL_TPB_TX_TC_MODE_WIDTH 1
+/* default value of bitfield tx_tc_mode */
+#define HW_ATL_TPB_TX_TC_MODE_DEFAULT 0x0
+
/* tx tx{b}_hi_thresh[c:0] bitfield definitions
* preprocessor definitions for the bitfield "tx{b}_hi_thresh[c:0]".
* parameter: buffer {b} | stride size 0x10 | range [0, 7]
--
2.17.1
^ permalink raw reply related
* Re: [RFC] nasty corner case in unix_dgram_sendmsg()
From: Rainer Weikusat @ 2019-02-26 15:31 UTC (permalink / raw)
To: Al Viro; +Cc: Jason Baron, netdev
In-Reply-To: <20190226063804.GI2217@ZenIV.linux.org.uk>
Al Viro <viro@zeniv.linux.org.uk> writes:
> On Tue, Feb 26, 2019 at 06:28:17AM +0000, Al Viro wrote:
[...]
>> * if after relocking we see that unix_peer(sk) now
>> is equal to other, we arrange for wakeup forwarding from other's
>> peer_wait *and* if that has (likely) succeeded we fail with -EAGAIN.
>> Huh?
This returns 1 if sending isn't possible at the moment, ie, if the
process which tries to send has to wait.
^ permalink raw reply
* Re: [PATCH net-next v3 0/7] net: sched: pie: align PIE implementation with RFC 8033
From: Stephen Hemminger @ 2019-02-26 15:50 UTC (permalink / raw)
To: Leslie Monis
Cc: davem, netdev, Mohit P . Tahiliani, Dave Taht, Jamal Hadi Salim
In-Reply-To: <20190226082046.GA2266@Inspiron-3521>
On Tue, 26 Feb 2019 13:50:46 +0530
Leslie Monis <lesliemonis@gmail.com> wrote:
> On Mon, Feb 25, 2019 at 04:38:11PM -0800, Stephen Hemminger wrote:
> > On Tue, 26 Feb 2019 00:39:54 +0530
> > Leslie Monis <lesliemonis@gmail.com> wrote:
> >
> > > The current implementation of the PIE queuing discipline is according to the
> > > IETF draft [http://tools.ietf.org/html/draft-pan-aqm-pie-00] and the paper
> > > [PIE: A Lightweight Control Scheme to Address the Bufferbloat Problem].
> > > However, a lot of necessary modifications and enhancements have been proposed
> > > in RFC 8033, which have not yet been incorporated in the source code of Linux.
> > > This patch series helps in achieving the same.
> > >
> > > Performance tests carried out using Flent [https://flent.org/]
> > >
> > > Changes from v2 to v3:
> > > - Used div_u64() instead of direct division after explicit type casting as
> > > recommended by David
> > >
> > > Changes from v1 to v2:
> > > - Excluded the patch setting PIE dynamically active/inactive as the test
> > > results were unsatisfactory
> > > - Fixed a scaling issue when adding more auto-tuning cases which caused
> > > local variables to underflow
> > > - Changed the long if/else chain to a loop as suggested by Stephen
> > > - Changed the position of the accu_prob variable in the pie_vars
> > > structure as recommended by Stephen
> > >
> > > Mohit P. Tahiliani (7):
> > > net: sched: pie: change value of QUEUE_THRESHOLD
> > > net: sched: pie: change default value of pie_params->target
> > > net: sched: pie: change default value of pie_params->tupdate
> > > net: sched: pie: change initial value of pie_vars->burst_time
> > > net: sched: pie: add more cases to auto-tune alpha and beta
> > > net: sched: pie: add derandomization mechanism
> > > net: sched: pie: update references
> > >
> > > include/uapi/linux/pkt_sched.h | 2 +-
> > > net/sched/sch_pie.c | 107 ++++++++++++++++++++-------------
> > > 2 files changed, 66 insertions(+), 43 deletions(-)
> >
> > Are you concerned at all that changes to default values might change
> > expected behavior of existing users?
>
> Hi Stephen,
>
> As Dave mentioned, the changes which we have made do not really change the
> behaviour of the aqm drastically. Our performance tests show that these changes
> improve performance without any side-effects. So existing users (if there are
> any) should not be negatively affected in any way.
Thanks for answering. Looks good
^ permalink raw reply
* Re: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
From: Eric Dumazet @ 2019-02-26 15:52 UTC (permalink / raw)
To: Sheng Lan, Stephen Hemminger
Cc: davem, netdev, netem, xuhanbing, zhengshaoyu, jiqin.ji,
liuzhiqiang26, yuehaibing
In-Reply-To: <05fa74b9-6e3b-7bd3-fa1c-c02e37a521f8@huawei.com>
On 02/26/2019 05:02 AM, Sheng Lan wrote:
>
>
>
>> On Mon, 25 Feb 2019 22:49:39 +0800
>> Sheng Lan <lansheng@huawei.com> wrote:
>>
>>> From: Sheng Lan <lansheng@huawei.com>
>>> Subject: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
>>>
>>> It can be reproduced by following steps:
>>> 1. virtio_net NIC is configured with gso/tso on
>>> 2. configure nginx as http server with an index file bigger than 1M bytes
>>> 3. use tc netem to produce duplicate packets and delay:
>>> tc qdisc add dev eth0 root netem delay 100ms 10ms 30% duplicate 90%
>>> 4. continually curl the nginx http server to get index file on client
>>> 5. BUG_ON is seen quickly
>>>
>>> [10258690.371129] kernel BUG at net/core/skbuff.c:4028!
>>> [10258690.371748] invalid opcode: 0000 [#1] SMP PTI
>>> [10258690.372094] CPU: 5 PID: 0 Comm: swapper/5 Tainted: G W 5.0.0-rc6 #2
>>> [10258690.372094] RSP: 0018:ffffa05797b43da0 EFLAGS: 00010202
>>> [10258690.372094] RBP: 00000000000005ea R08: 0000000000000000 R09: 00000000000005ea
>>> [10258690.372094] R10: ffffa0579334d800 R11: 00000000000002c0 R12: 0000000000000002
>>> [10258690.372094] R13: 0000000000000000 R14: ffffa05793122900 R15: ffffa0578f7cb028
>>> [10258690.372094] FS: 0000000000000000(0000) GS:ffffa05797b40000(0000) knlGS:0000000000000000
>>> [10258690.372094] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [10258690.372094] CR2: 00007f1a6dc00868 CR3: 000000001000e000 CR4: 00000000000006e0
>>> [10258690.372094] Call Trace:
>>> [10258690.372094] <IRQ>
>>> [10258690.372094] skb_to_sgvec+0x11/0x40
>>> [10258690.372094] start_xmit+0x38c/0x520 [virtio_net]
>>> [10258690.372094] dev_hard_start_xmit+0x9b/0x200
>>> [10258690.372094] sch_direct_xmit+0xff/0x260
>>> [10258690.372094] __qdisc_run+0x15e/0x4e0
>>> [10258690.372094] net_tx_action+0x137/0x210
>>> [10258690.372094] __do_softirq+0xd6/0x2a9
>>> [10258690.372094] irq_exit+0xde/0xf0
>>> [10258690.372094] smp_apic_timer_interrupt+0x74/0x140
>>> [10258690.372094] apic_timer_interrupt+0xf/0x20
>>> [10258690.372094] </IRQ>
>>>
>>> In __skb_to_sgvec, the skb->len is not equal to the sum of the skb's
>>> linear data size and nonlinear data size, thus BUG_ON triggered. The
>>> bad skb's nonlinear data size is less than skb->data_len, because the
>>> skb is cloned and a part of related cloned skb's nonlinear data is
>>> split off.
>>>
>>> Duplicate packet is cloned by skb_clone in netem_enqueue and may be delayed
>>> some time in qdisc. Due to the delay time, the original skb will be pushed
>>> again later in __tcp_push_pending_frames when tcp receives new packets.
>>> In tcp_write_xmit, when the tcp_mss_split_point returns a smaller limit,
>>> the original skb will be fragmented and the skb's nonlinear data will be
>>> split off. The length of the skb cloned by netem will not be updated.
>>> When we use virtio_net NIC, the duplicated cloned skb will be filled into
>>> a scatter-gather list in __skb_to_sgvec and trigger the BUG_ON.
>>>
>>> Here I replace the skb_clone with skb_copy in netem_enqueue to ensure
>>> the duplicated skb's nonlinear data is independent.
>>>
>>> Signed-off-by: Sheng Lan <lansheng@huawei.com>
>>> Reported-by: Qin Ji <jiqin.ji@huawei.com>
>>>
>>> Fixes: 0afb51e7 ("netem: reinsert for duplication")
>>
>> This sounds like a bug in the other layers (either TCP or Virtio net)
>> not handling a cloned skb properly.
>>
>
> I have traced the route of skb by printk, let me take an example to describe the problem to make it clearly:
> Mss value equals to 1448. Limit value is the split size when tcp do tso_fragment, is depending on the size of the sending congestion window and mss value.
>
> TCP layer transmit the index file to client, the original skb1 size is large:
> ...
> tcp_write_xmit (skb1->data_len == 62264, limit == 2*mss == 2896)
> tso_fragment (it needs to be fragmented by limit value)
> skb_split (after split, skb1->data_len == 2896, skb_shinfo(skb1)->frags[0] == 2896, skb_shinfo(skb1)->nr_frags == 1)
> ...
> netem_enqueue (netem construct a duplicate packet of skb1 by skb_clone)
> skb2 = skb_clone(skb1) (skb1->data_len == skb2->data_len == 2896, skb1 and skb2 share the nonlinear data frags[0] == 2896)
> waiting 30ms (skb1 and skb2 will be delayed in qdisc queue due to the netem delay configuration)
>
>
> TCP layer receives new packets and trys to retransmit the skb1:
> tcp_rcv_established
> __tcp_push_pending_frames
> tcp_write_xmit (skb1->data_len == 2896, cwnd size decreased or packets in flight increased, cause the limit decreased to 1*mss == 1448)
tcp_write_xmit() only deals with packet in the write queue,
they never were sent. They can not be any clone of them by definition, since
skbs in the TCP write queue are private to TCP stack,
Once a packet is sent, the master skb is moved to the rtx rb-tree,
while the clone is going through lower stacks.
When/if a retransmit is due, we always make sure there is no clone on it,
look at the various calls to skb_unclone()
> tso_fragment (limit value is less than skb1->data_len, skb1 will be fragmented again)
> skb_split (the second time split, skb1 is cloned now and share nonlinear data with skb2.
> skb1->data_len == 1448, frags[0] == 1448, a part of shared nonlinear data has been split off)
>
> Now we can see:
> skb1->data_len == 1448
> skb2->data_len == 2896 (2896 is wrong value)
> frags[0] == 1448
>
>
> The route of skb2:
> netem_enqueue (netem construct a duplicate packet, skb2 = skb_clone(skb1), put skb2 into queue)
> waiting 30ms (delayed packet)
> ...
> netem_dequeue (skb2->data_len == 2896, frags[0] == 1448)
> sch_direct_xmit
> dev_hard_start_xmit
> xmit_one
> start_xmit [virtio_net driver]
> skb_to_sgvec (BUG_ON here )
>
>
> The key is that skb be split by skb_split in tso_fragment again after netem clone it and share nonlinear data with another skb.
> Processing of TCP seems OK, which push and fragment delayed packets in write queue. And virtio_net is the trigger of the BUG_ON.
> So I replaced skb_clone with skb_copy in netem_enqueue, and the method worked. Currently, I have no better idea to fix it,
> would you give me some inspiring advice ? If I am wrong, please correct me.
>
> Thanks
>
^ permalink raw reply
* Re: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
From: Stephen Hemminger @ 2019-02-26 15:59 UTC (permalink / raw)
To: Sheng Lan
Cc: davem, netdev, netem, xuhanbing, zhengshaoyu, jiqin.ji,
liuzhiqiang26, yuehaibing
In-Reply-To: <05fa74b9-6e3b-7bd3-fa1c-c02e37a521f8@huawei.com>
On Tue, 26 Feb 2019 21:02:10 +0800
Sheng Lan <lansheng@huawei.com> wrote:
> > On Mon, 25 Feb 2019 22:49:39 +0800
> > Sheng Lan <lansheng@huawei.com> wrote:
> >
> >> From: Sheng Lan <lansheng@huawei.com>
> >> Subject: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
> >>
> >> It can be reproduced by following steps:
> >> 1. virtio_net NIC is configured with gso/tso on
> >> 2. configure nginx as http server with an index file bigger than 1M bytes
> >> 3. use tc netem to produce duplicate packets and delay:
> >> tc qdisc add dev eth0 root netem delay 100ms 10ms 30% duplicate 90%
> >> 4. continually curl the nginx http server to get index file on client
> >> 5. BUG_ON is seen quickly
> >>
> >> [10258690.371129] kernel BUG at net/core/skbuff.c:4028!
> >> [10258690.371748] invalid opcode: 0000 [#1] SMP PTI
> >> [10258690.372094] CPU: 5 PID: 0 Comm: swapper/5 Tainted: G W 5.0.0-rc6 #2
> >> [10258690.372094] RSP: 0018:ffffa05797b43da0 EFLAGS: 00010202
> >> [10258690.372094] RBP: 00000000000005ea R08: 0000000000000000 R09: 00000000000005ea
> >> [10258690.372094] R10: ffffa0579334d800 R11: 00000000000002c0 R12: 0000000000000002
> >> [10258690.372094] R13: 0000000000000000 R14: ffffa05793122900 R15: ffffa0578f7cb028
> >> [10258690.372094] FS: 0000000000000000(0000) GS:ffffa05797b40000(0000) knlGS:0000000000000000
> >> [10258690.372094] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [10258690.372094] CR2: 00007f1a6dc00868 CR3: 000000001000e000 CR4: 00000000000006e0
> >> [10258690.372094] Call Trace:
> >> [10258690.372094] <IRQ>
> >> [10258690.372094] skb_to_sgvec+0x11/0x40
> >> [10258690.372094] start_xmit+0x38c/0x520 [virtio_net]
> >> [10258690.372094] dev_hard_start_xmit+0x9b/0x200
> >> [10258690.372094] sch_direct_xmit+0xff/0x260
> >> [10258690.372094] __qdisc_run+0x15e/0x4e0
> >> [10258690.372094] net_tx_action+0x137/0x210
> >> [10258690.372094] __do_softirq+0xd6/0x2a9
> >> [10258690.372094] irq_exit+0xde/0xf0
> >> [10258690.372094] smp_apic_timer_interrupt+0x74/0x140
> >> [10258690.372094] apic_timer_interrupt+0xf/0x20
> >> [10258690.372094] </IRQ>
> >>
> >> In __skb_to_sgvec, the skb->len is not equal to the sum of the skb's
> >> linear data size and nonlinear data size, thus BUG_ON triggered. The
> >> bad skb's nonlinear data size is less than skb->data_len, because the
> >> skb is cloned and a part of related cloned skb's nonlinear data is
> >> split off.
> >>
> >> Duplicate packet is cloned by skb_clone in netem_enqueue and may be delayed
> >> some time in qdisc. Due to the delay time, the original skb will be pushed
> >> again later in __tcp_push_pending_frames when tcp receives new packets.
> >> In tcp_write_xmit, when the tcp_mss_split_point returns a smaller limit,
> >> the original skb will be fragmented and the skb's nonlinear data will be
> >> split off. The length of the skb cloned by netem will not be updated.
> >> When we use virtio_net NIC, the duplicated cloned skb will be filled into
> >> a scatter-gather list in __skb_to_sgvec and trigger the BUG_ON.
> >>
> >> Here I replace the skb_clone with skb_copy in netem_enqueue to ensure
> >> the duplicated skb's nonlinear data is independent.
> >>
> >> Signed-off-by: Sheng Lan <lansheng@huawei.com>
> >> Reported-by: Qin Ji <jiqin.ji@huawei.com>
> >>
> >> Fixes: 0afb51e7 ("netem: reinsert for duplication")
> >
> > This sounds like a bug in the other layers (either TCP or Virtio net)
> > not handling a cloned skb properly.
> >
>
> I have traced the route of skb by printk, let me take an example to describe the problem to make it clearly:
> Mss value equals to 1448. Limit value is the split size when tcp do tso_fragment, is depending on the size of the sending congestion window and mss value.
>
> TCP layer transmit the index file to client, the original skb1 size is large:
> ...
> tcp_write_xmit (skb1->data_len == 62264, limit == 2*mss == 2896)
> tso_fragment (it needs to be fragmented by limit value)
> skb_split (after split, skb1->data_len == 2896, skb_shinfo(skb1)->frags[0] == 2896, skb_shinfo(skb1)->nr_frags == 1)
> ...
> netem_enqueue (netem construct a duplicate packet of skb1 by skb_clone)
> skb2 = skb_clone(skb1) (skb1->data_len == skb2->data_len == 2896, skb1 and skb2 share the nonlinear data frags[0] == 2896)
> waiting 30ms (skb1 and skb2 will be delayed in qdisc queue due to the netem delay configuration)
>
>
> TCP layer receives new packets and trys to retransmit the skb1:
> tcp_rcv_established
> __tcp_push_pending_frames
> tcp_write_xmit (skb1->data_len == 2896, cwnd size decreased or packets in flight increased, cause the limit decreased to 1*mss == 1448)
> tso_fragment (limit value is less than skb1->data_len, skb1 will be fragmented again)
Maybe the fix is to stop TSO fragment from overwriting by doing something like:
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 730bc44dbad9..5fe91d0224f6 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1856,7 +1856,7 @@ static int tso_fragment(struct sock *sk, enum tcp_queue tcp_queue,
u8 flags;
/* All of a TSO frame must be composed of paged data. */
- if (skb->len != skb->data_len)
+ if (skb->len != skb->data_len || skb_cloned(skb))
return tcp_fragment(sk, tcp_queue, skb, len, mss_now, gfp);
buff = sk_stream_alloc_skb(sk, 0, gfp, true);
^ permalink raw reply related
* Re: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
From: Eric Dumazet @ 2019-02-26 16:08 UTC (permalink / raw)
To: Stephen Hemminger, Sheng Lan
Cc: davem, netdev, netem, xuhanbing, zhengshaoyu, jiqin.ji,
liuzhiqiang26, yuehaibing
In-Reply-To: <20190226075906.103fa072@shemminger-XPS-13-9360>
On 02/26/2019 07:59 AM, Stephen Hemminger wrote:
>
>
> Maybe the fix is to stop TSO fragment from overwriting by doing something like:
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 730bc44dbad9..5fe91d0224f6 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1856,7 +1856,7 @@ static int tso_fragment(struct sock *sk, enum tcp_queue tcp_queue,
> u8 flags;
>
> /* All of a TSO frame must be composed of paged data. */
> - if (skb->len != skb->data_len)
> + if (skb->len != skb->data_len || skb_cloned(skb))
> return tcp_fragment(sk, tcp_queue, skb, len, mss_now, gfp);
>
> buff = sk_stream_alloc_skb(sk, 0, gfp, true);
>
tso_fragment() is only called with packets that were not yet transmit.
^ permalink raw reply
* Re: [PATCH net-next] net: sched: don't release block->lock when dumping chains
From: Vlad Buslov @ 2019-02-26 16:09 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <CAM_iQpXQXMxWDTcFvEEXra_1uKujhbeb630_kPfBqO1R8uwYtA@mail.gmail.com>
On Tue 26 Feb 2019 at 00:15, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Feb 25, 2019 at 7:45 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> Function tc_dump_chain() obtains and releases block->lock on each iteration
>> of its inner loop that dumps all chains on block. Outputting chain template
>> info is fast operation so locking/unlocking mutex multiple times is an
>> overhead when lock is highly contested. Modify tc_dump_chain() to only
>> obtain block->lock once and dump all chains without releasing it.
>>
>> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
>
> Thanks for the followup!
>
> Isn't it similar for __tcf_get_next_proto() in tcf_chain_dump()?
> And for tc_dump_tfilter()?
Not really. These two dump all tp filters and not just a template, which
is O(n) on number of filters and can be slow because it calls hw offload
API for each of them. Our typical use-case involves periodic filter dump
(to update stats) while multiple concurrent user-space threads are
updating filters, so it is important for them to be able to execute in
parallel.
^ permalink raw reply
* Re: linux-next: Tree for Feb 26 (net/sched/sch_pie.c)
From: Randy Dunlap @ 2019-02-26 16:21 UTC (permalink / raw)
To: Stephen Rothwell, Linux Next Mailing List
Cc: Linux Kernel Mailing List, netdev@vger.kernel.org,
Vijay Subramanian, Mythili Prabhu
In-Reply-To: <20190226190859.624e21f0@canb.auug.org.au>
On 2/26/19 12:08 AM, Stephen Rothwell wrote:
> Hi all,
>
> Changes since 20190225:
>
on i386:
ld: net/sched/sch_pie.o: in function `pie_timer':
sch_pie.c:(.text+0x604): undefined reference to `__udivdi3'
--
~Randy
^ 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