* 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 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 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 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] 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-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 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 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] 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 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 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
* 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] 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
* [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
* [PATCH] bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id()
From: zerons @ 2019-02-26 14:15 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-kernel
[ Upstream commit c91951f15978f1a0c6b65f063d30f7ea7bc6fb42 ]
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 related
* Re: [PATCH net 2/4] tls: Fix write space handling
From: Boris Pismenny @ 2019-02-26 14:13 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: <DB7PR04MB425215CB92707A2FD5CD6A138B7B0@DB7PR04MB4252.eurprd04.prod.outlook.com>
On 2/26/2019 2:49 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 2/4] tls: Fix write space handling
>>
>> TLS device cannot use the sw context. This patch returns the original
>> tls device write space handler and moves the sw/device specific portions
>> to the relevant files.
>>
>> Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption of records
>> for performance")
>> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
>> Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com>
>> ---
>> include/net/tls.h | 3 +++
>> net/tls/tls_device.c | 16 ++++++++++++++++
>> net/tls/tls_main.c | 17 +++++++++--------
>> net/tls/tls_sw.c | 15 +++++++++++++++
>> 4 files changed, 43 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/net/tls.h b/include/net/tls.h
>> index a528a082da73..9d7c53737b13 100644
>> --- a/include/net/tls.h
>> +++ b/include/net/tls.h
>> @@ -519,6 +519,9 @@ static inline bool tls_sw_has_ctx_tx(const struct sock
>> *sk)
>> return !!tls_sw_ctx_tx(ctx);
>> }
>>
>> +int tls_sw_write_space(struct sock *sk, struct tls_context *ctx);
>> +int tls_device_write_space(struct sock *sk, struct tls_context *ctx);
>> +
>> static inline struct tls_offload_context_rx *
>> tls_offload_ctx_rx(const struct tls_context *tls_ctx)
>> {
>> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
>> index 3e5e8e021a87..e8988b3f3236 100644
>> --- a/net/tls/tls_device.c
>> +++ b/net/tls/tls_device.c
>> @@ -546,6 +546,22 @@ static int tls_device_push_pending_record(struct
>> sock *sk, int flags)
>> return tls_push_data(sk, &msg_iter, 0, flags,
>> TLS_RECORD_TYPE_DATA);
>> }
>>
>> +int tls_device_write_space(struct sock *sk, struct tls_context *ctx)
>> +{
>> + int rc = 0;
>> +
>> + if (!sk->sk_write_pending && tls_is_partially_sent_record(ctx)) {
>> + gfp_t sk_allocation = sk->sk_allocation;
>> +
>> + sk->sk_allocation = GFP_ATOMIC;
>> + rc = tls_push_partial_record(sk, ctx,
>> + MSG_DONTWAIT |
>> MSG_NOSIGNAL);
>> + sk->sk_allocation = sk_allocation;
>> + }
>> +
>> + return rc;
>> +}
>> +
>> void handle_device_resync(struct sock *sk, u32 seq, u64 rcd_sn)
>> {
>> struct tls_context *tls_ctx = tls_get_ctx(sk);
>> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
>> index 7e05af75536d..11c1980a75cb 100644
>> --- a/net/tls/tls_main.c
>> +++ b/net/tls/tls_main.c
>> @@ -212,7 +212,7 @@ int tls_push_partial_record(struct sock *sk, struct
>> tls_context *ctx,
>> static void tls_write_space(struct sock *sk)
>> {
>> struct tls_context *ctx = tls_get_ctx(sk);
>> - struct tls_sw_context_tx *tx_ctx = tls_sw_ctx_tx(ctx);
>> + int rc;
>>
>> /* If in_tcp_sendpages call lower protocol write space handler
>> * to ensure we wake up any waiting operations there. For example
>> @@ -223,14 +223,15 @@ static void tls_write_space(struct sock *sk)
>> return;
>> }
>>
>> - /* Schedule the transmission if tx list is ready */
>> - if (is_tx_ready(tx_ctx) && !sk->sk_write_pending) {
>> - /* Schedule the transmission */
>> - if (!test_and_set_bit(BIT_TX_SCHEDULED, &tx_ctx-
>>> tx_bitmask))
>> - schedule_delayed_work(&tx_ctx->tx_work.work, 0);
>> - }
>> +#ifdef CONFIG_TLS_DEVICE
>> + if (ctx->tx_conf == TLS_HW)
>> + rc = tls_device_write_space(sk, ctx);
>> + else
>> +#endif
>> + rc = tls_sw_write_space(sk, ctx);
>>
>> - ctx->sk_write_space(sk);
>> + if (!rc)
>
> Why do we need to check 'rc'?
>
> If it is required, then ' ctx->sk_write_space(sk)' can move to tls_device_write_space()
> since tls_sw_write_space() always returns '0'.
>
It is not necessary in the software code path due to the delayed work
that is there. But, we need in the device flow. I'll move it there.
>> + ctx->sk_write_space(sk);
>> }
>>
>> static void tls_ctx_free(struct tls_context *ctx)
>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
>> index 1cc830582fa8..4afa67b00aaf 100644
>> --- a/net/tls/tls_sw.c
>> +++ b/net/tls/tls_sw.c
>> @@ -2126,6 +2126,21 @@ static void tx_work_handler(struct work_struct
>> *work)
>> release_sock(sk);
>> }
>>
>> +int tls_sw_write_space(struct sock *sk, struct tls_context *ctx)
>> +{
>> + struct tls_sw_context_tx *tx_ctx = tls_sw_ctx_tx(ctx);
>> +
>> + /* Schedule the transmission if tx list is ready */
>> + if (is_tx_ready(tx_ctx) && !sk->sk_write_pending) {
>> + /* Schedule the transmission */
>> + if (!test_and_set_bit(BIT_TX_SCHEDULED,
>> + &tx_ctx->tx_bitmask))
>> + schedule_delayed_work(&tx_ctx->tx_work.work, 0);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
>> {
>> struct tls_context *tls_ctx = tls_get_ctx(sk);
>> --
>> 2.12.2
>
^ permalink raw reply
* Re: [PATCH net-next 1/2] vxlan: add extack support for create and changelink
From: Roopa Prabhu @ 2019-02-26 14:06 UTC (permalink / raw)
To: Petr Machata
Cc: davem@davemloft.net, netdev@vger.kernel.org,
dsa@cumulusnetworks.com, sd@queasysnail.net,
johannes@sipsolutions.net
In-Reply-To: <87imx6fyk1.fsf@mellanox.com>
On Tue, Feb 26, 2019 at 5:51 AM Petr Machata <petrm@mellanox.com> wrote:
>
>
> Roopa Prabhu <roopa@cumulusnetworks.com> writes:
>
> > From: Roopa Prabhu <roopa@cumulusnetworks.com>
> >
> > This patch adds extack coverage in vxlan link
> > create and changelink paths. Introduces a new helper
> > vxlan_nl2flags to consolidate flag attribute validation.
> >
> > thanks to Johannes Berg for some tips to construct the
> > generic vxlan flag extack strings.
> >
> > Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> > ---
> > drivers/net/vxlan.c | 208 +++++++++++++++++++++++++++++++++++-----------------
> > include/net/vxlan.h | 31 ++++++++
> > 2 files changed, 172 insertions(+), 67 deletions(-)
> >
> > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> > index 577201c..a3c46d7 100644
> > --- a/drivers/net/vxlan.c
> > +++ b/drivers/net/vxlan.c
> > @@ -3583,11 +3583,40 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
> > return err;
> > }
> >
> > +/* Set/clear flags based on attribute */
> > +static int vxlan_nl2flag(struct vxlan_config *conf, struct nlattr *tb[],
> > + int attrtype, unsigned long mask, bool changelink,
> > + bool changelink_supported,
> > + struct netlink_ext_ack *extack)
> > +{
> > + unsigned long flags;
> > +
> > + if (!tb[attrtype])
> > + return 0;
> > +
> > + if (changelink && !changelink_supported) {
> > + vxlan_flag_attr_error(attrtype, extack);
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if (vxlan_policy[attrtype].type == NLA_FLAG)
> > + flags = conf->flags | mask;
> > + else if (nla_get_u8(tb[attrtype]))
> > + flags = conf->flags | mask;
> > + else
> > + flags = conf->flags & ~mask;
>
> Many of the flags for which you call this don't actually have the else
> branch. However I suspect there are no good reasons not to allow
> resetting a flag.
>
> Reviewed-by: Petr Machata <petrm@mellanox.com>
yes, correct, that was intentional.
also, I am not sure what is the best way to support reseting of a NLA_FLAG.
Absence of the flag attribute in the request cannot be the reason for
resetting or clearing the flag.
None of the NLA_FLAG attributes support changing the flag today. This
patch does not change that.
^ permalink raw reply
* Re: [PATCH] net: phy: Micrel KSZ8061: link failure after cable connect
From: Andrew Lunn @ 2019-02-26 14:06 UTC (permalink / raw)
To: Rajasingh Thavamani
Cc: Florian Fainelli, Heiner Kallweit, David S. Miller, netdev,
linux-kernel
In-Reply-To: <20190226081506.25887-1-T.Rajasingh@landisgyr.com>
On Tue, Feb 26, 2019 at 01:45:06PM +0530, Rajasingh Thavamani wrote:
> With Micrel KSZ8061 PHY, the link may occasionally not come up after
> Ethernet cable connect. The vendor's (Microchip, former Micrel) errata
> sheet 80000688A.pdf descripes the problem and possible workarounds in
> detail, see below.
> The batch implements workaround 1, which permanently fixes the issue.
...
> PLAN
> This errata will not be corrected in the future revision.
>
> Signed-off-by: Rajasingh Thavamani <T.Rajasingh@landisgyr.com>
We have been here before:
https://lkml.org/lkml/2018/11/23/466
At that time, the patch was from Alexander Onnasch, and has his
signed-off-by. Now it only has your SOB. It probably should have
both. My reviewed-by has gone missing. And this is version 3? And in
the comments i also included a Fixed: tag, which should be added.
Please submit a v4 with all these issues fixed.
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 1/5] net/mlx5e: Return -EOPNOTSUPP when modify header action zero
From: Roi Dayan @ 2019-02-26 13:54 UTC (permalink / raw)
To: xiangxia.m.yue@gmail.com, Saeed Mahameed, gerlitz.or@gmail.com
Cc: netdev@vger.kernel.org
In-Reply-To: <1551091207-10366-2-git-send-email-xiangxia.m.yue@gmail.com>
On 25/02/2019 12:40, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> When max modify header action is zero, we return -EOPNOTSUPP
> directly. In this way, we can ignore wrong message info (e.g.
> "mlx5: parsed 0 pedit actions, can't do more").
>
> This happens when offloading pedit actions on mlx VFs.
>
> For example:
> $ tc filter add dev mlx5_vf parent ffff: protocol ip prio 1 \
> flower skip_sw dst_mac 00:10:56:fb:64:e8 \
> dst_ip 1.1.1.100 src_ip 1.1.1.200 \
> action pedit ex munge eth src set 00:10:56:b4:5d:20
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index b38986e..708f819 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -2002,7 +2002,8 @@ static int offload_pedit_fields(struct pedit_headers_action *hdrs,
> static int alloc_mod_hdr_actions(struct mlx5e_priv *priv,
> struct pedit_headers_action *hdrs,
> int namespace,
> - struct mlx5e_tc_flow_parse_attr *parse_attr)
> + struct mlx5e_tc_flow_parse_attr *parse_attr,
> + struct netlink_ext_ack *extack)
> {
> int nkeys, action_size, max_actions;
>
> @@ -2015,6 +2016,12 @@ static int alloc_mod_hdr_actions(struct mlx5e_priv *priv,
> else /* namespace is MLX5_FLOW_NAMESPACE_KERNEL - NIC offloading */
> max_actions = MLX5_CAP_FLOWTABLE_NIC_RX(priv->mdev, max_modify_header_actions);
>
> + if (!max_actions) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "don't support pedit actions, can't offload");
can we rephrase that to match the msg style you did in patch 5 ?
i.e. The pedit offload action is not supported
> + return -EOPNOTSUPP;
> + }
> +
> /* can get up to crazingly 16 HW actions in 32 bits pedit SW key */
> max_actions = min(max_actions, nkeys * 16);
>
> @@ -2072,7 +2079,8 @@ static int alloc_tc_pedit_action(struct mlx5e_priv *priv, int namespace,
> u8 cmd;
>
> if (!parse_attr->mod_hdr_actions) {
> - err = alloc_mod_hdr_actions(priv, hdrs, namespace, parse_attr);
> + err = alloc_mod_hdr_actions(priv, hdrs,
> + namespace, parse_attr, extack);
> if (err)
> goto out_err;
> }
>
^ permalink raw reply
* general protection fault in __xfrm_policy_check
From: syzbot @ 2019-02-26 13:54 UTC (permalink / raw)
To: davem, herbert, linux-kernel, netdev, steffen.klassert,
syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: ff7b11aa481f net: socket: set sock->sk to NULL after calli..
git tree: net
console output: https://syzkaller.appspot.com/x/log.txt?x=12222e5cc00000
kernel config: https://syzkaller.appspot.com/x/.config?x=7132344728e7ec3f
dashboard link: https://syzkaller.appspot.com/bug?extid=4ea28a8b817ee28bf324
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+4ea28a8b817ee28bf324@syzkaller.appspotmail.com
kasan: CONFIG_KASAN_INLINE enabled
__sys_recvmmsg+0xe5/0x270 net/socket.c:2471
kasan: GPF could be caused by NULL-ptr deref or user memory access
__do_sys_recvmmsg net/socket.c:2492 [inline]
__se_sys_recvmmsg net/socket.c:2485 [inline]
__x64_sys_recvmmsg+0xe6/0x140 net/socket.c:2485
do_syscall_64+0x103/0x610 arch/x86/entry/common.c:290
general protection fault: 0000 [#1] PREEMPT SMP KASAN
entry_SYSCALL_64_after_hwframe+0x49/0xbe
CPU: 1 PID: 16691 Comm: syz-executor.0 Not tainted 5.0.0-rc7+ #96
RIP: 0033:0x457e29
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Code: ad b8 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 7b b8 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RIP: 0010:xs_net include/net/xfrm.h:253 [inline]
RIP: 0010:xfrmi_decode_session net/xfrm/xfrm_interface.c:82 [inline]
RIP: 0010:xfrmi_decode_session+0x15c/0x6c0 net/xfrm/xfrm_interface.c:73
RSP: 002b:00007f715c86bc78 EFLAGS: 00000246 ORIG_RAX: 000000000000012b
Code: 7c fc 08 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 2e 05 00 00 48 b8 00
00 00 00 00 fc ff df 4f 8b 64 fc 08 4c 89 e2 48 c1 ea 03 <80> 3c 02 00 0f
85 01 05 00 00 4d 8b 3c 24 e8 71 34 5b fb e8 9c 70
RAX: ffffffffffffffda RBX: 00007f715c86bc90 RCX: 0000000000457e29
RSP: 0018:ffff88804594f120 EFLAGS: 00010246
RDX: 04000000000001de RSI: 00000000200037c0 RDI: 0000000000000004
RAX: dffffc0000000000 RBX: ffff88809fbd1d00 RCX: ffffc90005df3000
RBP: 000000000073bf00 R08: 0000000020003700 R09: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff86027000 RDI: ffff8880a0766b48
R10: 0000000000000006 R11: 0000000000000246 R12: 00007f715c86c6d4
RBP: ffff88804594f148 R08: ffff88805caa44c0 R09: ffffed1015d25bd0
R13: 00000000004c4b3e R14: 00000000004d8648 R15: 0000000000000005
R10: ffffed1015d25bcf R11: ffff8880ae92de7b R12: 0000000000000000
kobject: 'loop3' (0000000085c2aeb9): kobject_uevent_env
R13: 0000000000000037 R14: ffff88809fbd1d10 R15: ffffffffffffffff
kobject: 'loop3' (0000000085c2aeb9): fill_kobj_path: path
= '/devices/virtual/block/loop3'
FS: 00007f88e71e4700(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000004db9c0 CR3: 00000000908bb000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
kobject: 'loop3' (0000000085c2aeb9): kobject_uevent_env
__xfrm_policy_check+0x1f8/0x2730 net/xfrm/xfrm_policy.c:3316
kobject: 'loop3' (0000000085c2aeb9): fill_kobj_path: path
= '/devices/virtual/block/loop3'
kobject: 'loop2' (000000007df39881): kobject_uevent_env
__xfrm_policy_check2 include/net/xfrm.h:1176 [inline]
xfrm_policy_check include/net/xfrm.h:1181 [inline]
xfrm4_policy_check include/net/xfrm.h:1186 [inline]
vti_input+0x4e3/0x7b0 net/ipv4/ip_vti.c:63
vti_rcv+0x10b/0x140 net/ipv4/ip_vti.c:109
xfrm4_esp_rcv+0xd8/0x230 net/ipv4/xfrm4_protocol.c:100
ip_protocol_deliver_rcu+0x60/0x8e0 net/ipv4/ip_input.c:208
kobject: 'loop2' (000000007df39881): fill_kobj_path: path
= '/devices/virtual/block/loop2'
ip_local_deliver_finish+0x23b/0x390 net/ipv4/ip_input.c:234
NF_HOOK include/linux/netfilter.h:289 [inline]
NF_HOOK include/linux/netfilter.h:283 [inline]
ip_local_deliver+0x1e9/0x520 net/ipv4/ip_input.c:255
kobject: 'loop4' (00000000be00e036): kobject_uevent_env
dst_input include/net/dst.h:450 [inline]
ip_rcv_finish+0x1db/0x2f0 net/ipv4/ip_input.c:414
NF_HOOK include/linux/netfilter.h:289 [inline]
NF_HOOK include/linux/netfilter.h:283 [inline]
ip_rcv+0xe8/0x3f0 net/ipv4/ip_input.c:524
kobject: 'loop4' (00000000be00e036): fill_kobj_path: path
= '/devices/virtual/block/loop4'
__netif_receive_skb_one_core+0x115/0x1a0 net/core/dev.c:4973
__netif_receive_skb+0x2c/0x1c0 net/core/dev.c:5083
netif_receive_skb_internal+0x117/0x660 net/core/dev.c:5186
kobject: 'loop5' (0000000040ba3921): kobject_uevent_env
kobject: 'loop5' (0000000040ba3921): fill_kobj_path: path
= '/devices/virtual/block/loop5'
napi_frags_finish net/core/dev.c:5753 [inline]
napi_gro_frags+0xade/0xd10 net/core/dev.c:5827
tun_get_user+0x2c0e/0x3dd0 drivers/net/tun.c:1974
tun_chr_write_iter+0xbd/0x160 drivers/net/tun.c:2019
call_write_iter include/linux/fs.h:1863 [inline]
do_iter_readv_writev+0x5e0/0x8e0 fs/read_write.c:680
do_iter_write fs/read_write.c:956 [inline]
do_iter_write+0x184/0x610 fs/read_write.c:937
vfs_writev+0x1b3/0x2f0 fs/read_write.c:1001
do_writev+0xf6/0x290 fs/read_write.c:1036
__do_sys_writev fs/read_write.c:1109 [inline]
__se_sys_writev fs/read_write.c:1106 [inline]
__x64_sys_writev+0x75/0xb0 fs/read_write.c:1106
do_syscall_64+0x103/0x610 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x457ce1
Code: 75 14 b8 14 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 e4 b9 fb ff c3 48
83 ec 08 e8 1a 2d 00 00 48 89 04 24 b8 14 00 00 00 0f 05 <48> 8b 3c 24 48
89 c2 e8 63 2d 00 00 48 89 d0 48 83 c4 08 48 3d 01
kobject: 'loop1' (000000005642785e): kobject_uevent_env
RSP: 002b:00007f88e71e3ba0 EFLAGS: 00000293 ORIG_RAX: 0000000000000014
RAX: ffffffffffffffda RBX: 000000000000003e RCX: 0000000000457ce1
RDX: 0000000000000001 RSI: 00007f88e71e3bf0 RDI: 00000000000000f0
RBP: 0000000020000040 R08: 00000000000000f0 R09: 0000000000000000
R10: 00007f88e71e49d0 R11: 0000000000000293 R12: 00007f88e71e46d4
R13: 00000000004c64e1 R14: 00000000004db9c0 R15: 00000000ffffffff
Modules linked in:
---[ end trace 2fbdf2db5ff7df79 ]---
kobject: 'loop1' (000000005642785e): fill_kobj_path: path
= '/devices/virtual/block/loop1'
RIP: 0010:xs_net include/net/xfrm.h:253 [inline]
RIP: 0010:xfrmi_decode_session net/xfrm/xfrm_interface.c:82 [inline]
RIP: 0010:xfrmi_decode_session+0x15c/0x6c0 net/xfrm/xfrm_interface.c:73
Code: 7c fc 08 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 2e 05 00 00 48 b8 00
00 00 00 00 fc ff df 4f 8b 64 fc 08 4c 89 e2 48 c1 ea 03 <80> 3c 02 00 0f
85 01 05 00 00 4d 8b 3c 24 e8 71 34 5b fb e8 9c 70
kobject: 'loop3' (0000000085c2aeb9): kobject_uevent_env
RSP: 0018:ffff88804594f120 EFLAGS: 00010246
RAX: dffffc0000000000 RBX: ffff88809fbd1d00 RCX: ffffc90005df3000
RDX: 0000000000000000 RSI: ffffffff86027000 RDI: ffff8880a0766b48
kobject: 'loop3' (0000000085c2aeb9): fill_kobj_path: path
= '/devices/virtual/block/loop3'
RBP: ffff88804594f148 R08: ffff88805caa44c0 R09: ffffed1015d25bd0
R10: ffffed1015d25bcf R11: ffff8880ae92de7b R12: 0000000000000000
R13: 0000000000000037 R14: ffff88809fbd1d10 R15: ffffffffffffffff
FAULT_INJECTION: forcing a failure.
name failslab, interval 1, probability 0, space 0, times 0
FS: 00007f88e71e4700(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CPU: 0 PID: 16722 Comm: syz-executor.2 Tainted: G D
5.0.0-rc7+ #96
CR2: 00000000004db9c0 CR3: 00000000908bb000 CR4: 00000000001406e0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x172/0x1f0 lib/dump_stack.c:113
fail_dump lib/fault-inject.c:51 [inline]
should_fail.cold+0xa/0x1b lib/fault-inject.c:149
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
__should_failslab+0x121/0x190 mm/failslab.c:32
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
should_failslab+0x9/0x14 mm/slab_common.c:1604
slab_pre_alloc_hook mm/slab.h:423 [inline]
slab_alloc_node mm/slab.c:3295 [inline]
kmem_cache_alloc_node+0x56/0x710 mm/slab.c:3637
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
syzbot.
^ permalink raw reply
* Re: [PATCH net-next 1/2] vxlan: add extack support for create and changelink
From: Petr Machata @ 2019-02-26 13:51 UTC (permalink / raw)
To: Roopa Prabhu
Cc: davem@davemloft.net, netdev@vger.kernel.org,
dsa@cumulusnetworks.com, sd@queasysnail.net,
johannes@sipsolutions.net
In-Reply-To: <1551160982-32685-2-git-send-email-roopa@cumulusnetworks.com>
Roopa Prabhu <roopa@cumulusnetworks.com> writes:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> This patch adds extack coverage in vxlan link
> create and changelink paths. Introduces a new helper
> vxlan_nl2flags to consolidate flag attribute validation.
>
> thanks to Johannes Berg for some tips to construct the
> generic vxlan flag extack strings.
>
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
> drivers/net/vxlan.c | 208 +++++++++++++++++++++++++++++++++++-----------------
> include/net/vxlan.h | 31 ++++++++
> 2 files changed, 172 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 577201c..a3c46d7 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -3583,11 +3583,40 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
> return err;
> }
>
> +/* Set/clear flags based on attribute */
> +static int vxlan_nl2flag(struct vxlan_config *conf, struct nlattr *tb[],
> + int attrtype, unsigned long mask, bool changelink,
> + bool changelink_supported,
> + struct netlink_ext_ack *extack)
> +{
> + unsigned long flags;
> +
> + if (!tb[attrtype])
> + return 0;
> +
> + if (changelink && !changelink_supported) {
> + vxlan_flag_attr_error(attrtype, extack);
> + return -EOPNOTSUPP;
> + }
> +
> + if (vxlan_policy[attrtype].type == NLA_FLAG)
> + flags = conf->flags | mask;
> + else if (nla_get_u8(tb[attrtype]))
> + flags = conf->flags | mask;
> + else
> + flags = conf->flags & ~mask;
Many of the flags for which you call this don't actually have the else
branch. However I suspect there are no good reasons not to allow
resetting a flag.
Reviewed-by: Petr Machata <petrm@mellanox.com>
> +
> + conf->flags = flags;
> +
> + return 0;
> +}
> +
> static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> struct net_device *dev, struct vxlan_config *conf,
> - bool changelink)
> + bool changelink, struct netlink_ext_ack *extack)
> {
> struct vxlan_dev *vxlan = netdev_priv(dev);
> + int err = 0;
>
> memset(conf, 0, sizeof(*conf));
>
> @@ -3598,40 +3627,54 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> if (data[IFLA_VXLAN_ID]) {
> __be32 vni = cpu_to_be32(nla_get_u32(data[IFLA_VXLAN_ID]));
>
> - if (changelink && (vni != conf->vni))
> + if (changelink && (vni != conf->vni)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_ID], "Cannot change VNI");
> return -EOPNOTSUPP;
> + }
> conf->vni = cpu_to_be32(nla_get_u32(data[IFLA_VXLAN_ID]));
> }
>
> if (data[IFLA_VXLAN_GROUP]) {
> - if (changelink && (conf->remote_ip.sa.sa_family != AF_INET))
> + if (changelink && (conf->remote_ip.sa.sa_family != AF_INET)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP], "New group address family does not match old group");
> return -EOPNOTSUPP;
> + }
>
> conf->remote_ip.sin.sin_addr.s_addr = nla_get_in_addr(data[IFLA_VXLAN_GROUP]);
> conf->remote_ip.sa.sa_family = AF_INET;
> } else if (data[IFLA_VXLAN_GROUP6]) {
> - if (!IS_ENABLED(CONFIG_IPV6))
> + if (!IS_ENABLED(CONFIG_IPV6)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP6], "IPv6 support not enabled in the kernel");
> return -EPFNOSUPPORT;
> + }
>
> - if (changelink && (conf->remote_ip.sa.sa_family != AF_INET6))
> + if (changelink && (conf->remote_ip.sa.sa_family != AF_INET6)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP6], "New group address family does not match old group");
> return -EOPNOTSUPP;
> + }
>
> conf->remote_ip.sin6.sin6_addr = nla_get_in6_addr(data[IFLA_VXLAN_GROUP6]);
> conf->remote_ip.sa.sa_family = AF_INET6;
> }
>
> if (data[IFLA_VXLAN_LOCAL]) {
> - if (changelink && (conf->saddr.sa.sa_family != AF_INET))
> + if (changelink && (conf->saddr.sa.sa_family != AF_INET)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL], "New local address family does not match old");
> return -EOPNOTSUPP;
> + }
>
> conf->saddr.sin.sin_addr.s_addr = nla_get_in_addr(data[IFLA_VXLAN_LOCAL]);
> conf->saddr.sa.sa_family = AF_INET;
> } else if (data[IFLA_VXLAN_LOCAL6]) {
> - if (!IS_ENABLED(CONFIG_IPV6))
> + if (!IS_ENABLED(CONFIG_IPV6)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL6], "IPv6 support not enabled in the kernel");
> return -EPFNOSUPPORT;
> + }
>
> - if (changelink && (conf->saddr.sa.sa_family != AF_INET6))
> + if (changelink && (conf->saddr.sa.sa_family != AF_INET6)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL6], "New local address family does not match old");
> return -EOPNOTSUPP;
> + }
>
> /* TODO: respect scope id */
> conf->saddr.sin6.sin6_addr = nla_get_in6_addr(data[IFLA_VXLAN_LOCAL6]);
> @@ -3648,9 +3691,12 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> conf->ttl = nla_get_u8(data[IFLA_VXLAN_TTL]);
>
> if (data[IFLA_VXLAN_TTL_INHERIT]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - conf->flags |= VXLAN_F_TTL_INHERIT;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_TTL_INHERIT,
> + VXLAN_F_TTL_INHERIT, changelink, false,
> + extack);
> + if (err)
> + return err;
> +
> }
>
> if (data[IFLA_VXLAN_LABEL])
> @@ -3658,10 +3704,11 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> IPV6_FLOWLABEL_MASK;
>
> if (data[IFLA_VXLAN_LEARNING]) {
> - if (nla_get_u8(data[IFLA_VXLAN_LEARNING]))
> - conf->flags |= VXLAN_F_LEARN;
> - else
> - conf->flags &= ~VXLAN_F_LEARN;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_LEARNING,
> + VXLAN_F_LEARN, changelink, true,
> + extack);
> + if (err)
> + return err;
> } else if (!changelink) {
> /* default to learn on a new device */
> conf->flags |= VXLAN_F_LEARN;
> @@ -3671,44 +3718,52 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> conf->age_interval = nla_get_u32(data[IFLA_VXLAN_AGEING]);
>
> if (data[IFLA_VXLAN_PROXY]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_PROXY]))
> - conf->flags |= VXLAN_F_PROXY;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_PROXY,
> + VXLAN_F_PROXY, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_RSC]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_RSC]))
> - conf->flags |= VXLAN_F_RSC;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_RSC,
> + VXLAN_F_RSC, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_L2MISS]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_L2MISS]))
> - conf->flags |= VXLAN_F_L2MISS;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_L2MISS,
> + VXLAN_F_L2MISS, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_L3MISS]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_L3MISS]))
> - conf->flags |= VXLAN_F_L3MISS;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_L3MISS,
> + VXLAN_F_L3MISS, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_LIMIT]) {
> - if (changelink)
> + if (changelink) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LIMIT],
> + "Cannot change limit");
> return -EOPNOTSUPP;
> + }
> conf->addrmax = nla_get_u32(data[IFLA_VXLAN_LIMIT]);
> }
>
> if (data[IFLA_VXLAN_COLLECT_METADATA]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_COLLECT_METADATA]))
> - conf->flags |= VXLAN_F_COLLECT_METADATA;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_COLLECT_METADATA,
> + VXLAN_F_COLLECT_METADATA, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_PORT_RANGE]) {
> @@ -3718,72 +3773,92 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> conf->port_min = ntohs(p->low);
> conf->port_max = ntohs(p->high);
> } else {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_PORT_RANGE],
> + "Cannot change port range");
> return -EOPNOTSUPP;
> }
> }
>
> if (data[IFLA_VXLAN_PORT]) {
> - if (changelink)
> + if (changelink) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_PORT],
> + "Cannot change port");
> return -EOPNOTSUPP;
> + }
> conf->dst_port = nla_get_be16(data[IFLA_VXLAN_PORT]);
> }
>
> if (data[IFLA_VXLAN_UDP_CSUM]) {
> - if (changelink)
> + if (changelink) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_UDP_CSUM],
> + "Cannot change UDP_CSUM flag");
> return -EOPNOTSUPP;
> + }
> if (!nla_get_u8(data[IFLA_VXLAN_UDP_CSUM]))
> conf->flags |= VXLAN_F_UDP_ZERO_CSUM_TX;
> }
>
> if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]))
> - conf->flags |= VXLAN_F_UDP_ZERO_CSUM6_TX;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_UDP_ZERO_CSUM6_TX,
> + VXLAN_F_UDP_ZERO_CSUM6_TX, changelink,
> + false, extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]))
> - conf->flags |= VXLAN_F_UDP_ZERO_CSUM6_RX;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_UDP_ZERO_CSUM6_RX,
> + VXLAN_F_UDP_ZERO_CSUM6_RX, changelink,
> + false, extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_REMCSUM_TX]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_REMCSUM_TX]))
> - conf->flags |= VXLAN_F_REMCSUM_TX;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_TX,
> + VXLAN_F_REMCSUM_TX, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_REMCSUM_RX]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_REMCSUM_RX]))
> - conf->flags |= VXLAN_F_REMCSUM_RX;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_RX,
> + VXLAN_F_REMCSUM_RX, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_GBP]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - conf->flags |= VXLAN_F_GBP;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_GBP,
> + VXLAN_F_GBP, changelink, false, extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_GPE]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - conf->flags |= VXLAN_F_GPE;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_GPE,
> + VXLAN_F_GPE, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_NOPARTIAL,
> + VXLAN_F_REMCSUM_NOPARTIAL, changelink,
> + false, extack);
> + if (err)
> + return err;
> }
>
> if (tb[IFLA_MTU]) {
> - if (changelink)
> + if (changelink) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_MTU],
> + "Cannot change mtu");
> return -EOPNOTSUPP;
> + }
> conf->mtu = nla_get_u32(tb[IFLA_MTU]);
> }
>
> @@ -3800,7 +3875,7 @@ static int vxlan_newlink(struct net *src_net, struct net_device *dev,
> struct vxlan_config conf;
> int err;
>
> - err = vxlan_nl2conf(tb, data, dev, &conf, false);
> + err = vxlan_nl2conf(tb, data, dev, &conf, false, extack);
> if (err)
> return err;
>
> @@ -3817,8 +3892,7 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> struct vxlan_config conf;
> int err;
>
> - err = vxlan_nl2conf(tb, data,
> - dev, &conf, true);
> + err = vxlan_nl2conf(tb, data, dev, &conf, true, extack);
> if (err)
> return err;
>
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index 0976781..00254a5 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -453,4 +453,35 @@ vxlan_fdb_clear_offload(const struct net_device *dev, __be32 vni)
> }
> #endif
>
> +static inline void vxlan_flag_attr_error(int attrtype,
> + struct netlink_ext_ack *extack)
> +{
> +#define VXLAN_FLAG(flg) \
> + case IFLA_VXLAN_##flg: \
> + NL_SET_ERR_MSG_MOD(extack, \
> + "cannot change " #flg " flag"); \
> + break
> + switch (attrtype) {
> + VXLAN_FLAG(TTL_INHERIT);
> + VXLAN_FLAG(LEARNING);
> + VXLAN_FLAG(PROXY);
> + VXLAN_FLAG(RSC);
> + VXLAN_FLAG(L2MISS);
> + VXLAN_FLAG(L3MISS);
> + VXLAN_FLAG(COLLECT_METADATA);
> + VXLAN_FLAG(UDP_ZERO_CSUM6_TX);
> + VXLAN_FLAG(UDP_ZERO_CSUM6_RX);
> + VXLAN_FLAG(REMCSUM_TX);
> + VXLAN_FLAG(REMCSUM_RX);
> + VXLAN_FLAG(GBP);
> + VXLAN_FLAG(GPE);
> + VXLAN_FLAG(REMCSUM_NOPARTIAL);
> + default:
> + NL_SET_ERR_MSG_MOD(extack, \
> + "cannot change flag");
> + break;
> + }
> +#undef VXLAN_FLAG
> +}
> +
> #endif
^ permalink raw reply
* Re: [PATCH net-next v2 2/5] net/mlx5e: Make the log friendly when decapsulation offload not supported
From: Roi Dayan @ 2019-02-26 13:50 UTC (permalink / raw)
To: xiangxia.m.yue@gmail.com, Saeed Mahameed, gerlitz.or@gmail.com
Cc: netdev@vger.kernel.org
In-Reply-To: <1551091207-10366-3-git-send-email-xiangxia.m.yue@gmail.com>
On 25/02/2019 12:40, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> If we try to offload decapsulation actions to VFs hw, we get the log [1].
> It's not friendly, because the kind of net device is null, and we don't
> know what '0' means.
>
> [1] "mlx5_core 0000:05:01.2 vf_0: decapsulation offload is not supported for net device (0)"
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
> index bdcc5e7..6cbfbfa 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
> @@ -84,7 +84,7 @@ static const char *mlx5e_netdev_kind(struct net_device *dev)
> if (dev->rtnl_link_ops)
> return dev->rtnl_link_ops->kind;
> else
> - return "";
> + return "unknown";
> }
>
> static int mlx5e_route_lookup_ipv6(struct mlx5e_priv *priv,
> @@ -620,8 +620,10 @@ int mlx5e_tc_tun_parse(struct net_device *filter_dev,
> headers_c, headers_v);
> } else {
> netdev_warn(priv->netdev,
> - "decapsulation offload is not supported for %s net device (%d)\n",
> - mlx5e_netdev_kind(filter_dev), tunnel_type);
> + "decapsulation offload is not supported for %s (kind: \"%s\")\n",
> + netdev_name(filter_dev),
> + mlx5e_netdev_kind(filter_dev));
> +
> return -EOPNOTSUPP;
> }
> return err;
>
Reviewed-by: Roi Dayan <roid@mellanox.com>
^ permalink raw reply
* Re: [PATCH net-next v2 5/5] net/mlx5e: Return -EOPNOTSUPP when attempting to offload an unsupported action
From: Roi Dayan @ 2019-02-26 13:49 UTC (permalink / raw)
To: xiangxia.m.yue@gmail.com, Saeed Mahameed, gerlitz.or@gmail.com
Cc: netdev@vger.kernel.org
In-Reply-To: <1551091207-10366-6-git-send-email-xiangxia.m.yue@gmail.com>
On 25/02/2019 12:40, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> The encapsulation is not supported for mlx5 VFs. When we try to
> offload that action, the -EINVAL is returned, but not -EOPNOTSUPP.
> This patch changes the returned value and ignore to confuse user.
>
> For example: (p2p1_0 is VF net device)
> tc filter add dev p2p1_0 protocol ip parent ffff: prio 1 flower skip_sw \
> src_mac e4:11:22:33:44:01 \
> action tunnel_key set \
> src_ip 1.1.1.100 \
> dst_ip 1.1.1.200 \
> dst_port 4789 id 100 \
> action mirred egress redirect dev vxlan0
>
> "RTNETLINK answers: Invalid argument"
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index d9fcb14..f5029ea 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -2302,7 +2302,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> }
> break;
> default:
> - return -EINVAL;
> + NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> + return -EOPNOTSUPP;
> }
> }
>
> @@ -2624,7 +2625,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> break;
> }
> default:
> - return -EINVAL;
> + NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> + return -EOPNOTSUPP;
> }
> }
>
>
Reviewed-by: Roi Dayan <roid@mellanox.com>
^ permalink raw reply
* Re: [PATCH net-next v2 4/5] net/mlx5e: Deletes unnecessary setting of esw_attr->parse_attr
From: Roi Dayan @ 2019-02-26 13:43 UTC (permalink / raw)
To: xiangxia.m.yue@gmail.com, Saeed Mahameed, gerlitz.or@gmail.com
Cc: netdev@vger.kernel.org
In-Reply-To: <1551091207-10366-5-git-send-email-xiangxia.m.yue@gmail.com>
On 25/02/2019 12:40, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> This patch deletes unnecessary setting of the esw_attr->parse_attr
> to parse_attr in parse_tc_fdb_actions() because it is already done
> by the mlx5e_flow_esw_attr_init() function.
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index e6583b9..d9fcb14 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -2566,7 +2566,6 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> out_dev->ifindex;
> parse_attr->tun_info[attr->out_count] = *info;
> encap = false;
> - attr->parse_attr = parse_attr;
> attr->dests[attr->out_count].flags |=
> MLX5_ESW_DEST_ENCAP;
> attr->out_count++;
>
Reviewed-by: Roi Dayan <roid@mellanox.com>
^ permalink raw reply
* Re: [PATCH net 3/4] tls: Fix mixing between async capable and async
From: Boris Pismenny @ 2019-02-26 13:43 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: <DB7PR04MB4252FD5C491CA99AF45BD10C8B7B0@DB7PR04MB4252.eurprd04.prod.outlook.com>
On 2/26/2019 2:38 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 3/4] tls: Fix mixing between async capable and async
>>
>> From: Eran Ben Elisha <eranbe@mellanox.com>
>>
>> Today, tls_sw_recvmsg is capable of using asynchronous mode to handle
>> application data TLS records. Moreover, it assumes that if the cipher can be
>> handled asynchronously, then all packets will be processed asynchronously.
>>
>> However, this assumption is not always true.
>
> Could you please elaborate, what happens?
>
When decryption doesn't occur asynchronously e.g. return code is not
EINPROGRESS, then async should be turned off.
>> Specifically, for AES-GCM in
>> TLS1.2, it causes data corruption, and breaks user applications.
>>
>> This patch fixes this problem by separating the async capability from the
>> decryption operation result.
>>
>> Fixes: c0ab4732d4c6 ("net/tls: Do not use async crypto for non-data
>> records")
>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>> Reviewed-by: Boris Pismenny <borisp@mellanox.com>
>> ---
>> net/tls/tls_sw.c | 15 +++++++++------
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index
>> 4afa67b00aaf..f515cd7e984e 100644
>> --- a/net/tls/tls_sw.c
>> +++ b/net/tls/tls_sw.c
>> @@ -1693,7 +1693,8 @@ int tls_sw_recvmsg(struct sock *sk,
>> bool zc = false;
>> int to_decrypt;
>> int chunk = 0;
>> - bool async;
>> + bool async_capable;
>> + bool async = false;
>>
>> skb = tls_wait_data(sk, psock, flags, timeo, &err);
>> if (!skb) {
>> @@ -1727,21 +1728,23 @@ int tls_sw_recvmsg(struct sock *sk,
>>
>> /* Do not use async mode if record is non-data */
>> if (ctx->control == TLS_RECORD_TYPE_DATA)
>> - async = ctx->async_capable;
>> + async_capable = ctx->async_capable;
>> else
>> - async = false;
>> + async_capable = false;
>>
>> err = decrypt_skb_update(sk, skb, &msg->msg_iter,
>> - &chunk, &zc, async);
>> + &chunk, &zc, async_capable);
>> if (err < 0 && err != -EINPROGRESS) {
>> tls_err_abort(sk, EBADMSG);
>> goto recv_end;
>> }
>>
>> - if (err == -EINPROGRESS)
>> + if (err == -EINPROGRESS && async_capable) {
> Why do we need to check 'async_capable'?
> Do we get err == -EINPROGRESS even when async_capable is false?
>
I've missed this. I'll remove this and send V2.
>> + async = true;
>> num_async++;
>> - else if (prot->version == TLS_1_3_VERSION)
>> + } else if (prot->version == TLS_1_3_VERSION) {
>> tlm->control = ctx->control;
>> + }
>>
>> /* If the type of records being processed is not known yet,
>> * set it to record type just dequeued. If it is already known,
>> --
>> 2.12.2
>
>
>
>
^ 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