* [PATCH net-next 1/1] devlink: Add APIs to publish/unpublish the port parameters.
From: Sudarsana Reddy Kalluru @ 2019-07-02 15:20 UTC (permalink / raw)
To: davem; +Cc: netdev, mkalderon, aelior, jiri
The patch adds devlink interfaces for drivers to publish/unpublish the
devlink port parameters.
Signed-off-by: Sudarsana Reddy Kalluru <skalluru@marvell.com>
Signed-off-by: Ariel Elior <aelior@marvell.com>
---
include/net/devlink.h | 2 ++
net/core/devlink.c | 42 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 6c51e86..2e2d7fc 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -651,6 +651,8 @@ int devlink_port_params_register(struct devlink_port *devlink_port,
void devlink_port_params_unregister(struct devlink_port *devlink_port,
const struct devlink_param *params,
size_t params_count);
+void devlink_port_params_publish(struct devlink_port *devlink_port);
+void devlink_port_params_unpublish(struct devlink_port *ddevlink_port);
int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
union devlink_param_value *init_val);
int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 4baf716..c06c23f 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6378,6 +6378,48 @@ void devlink_port_params_unregister(struct devlink_port *devlink_port,
}
EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
+/**
+ * devlink_port_params_publish - publish port configuration parameters
+ *
+ * @devlink_port: devlink port
+ *
+ * Publish previously registered port configuration parameters.
+ */
+void devlink_port_params_publish(struct devlink_port *devlink_port)
+{
+ struct devlink_param_item *param_item;
+
+ list_for_each_entry(param_item, &devlink_port->param_list, list) {
+ if (param_item->published)
+ continue;
+ param_item->published = true;
+ devlink_param_notify(devlink_port->devlink, devlink_port->index,
+ param_item, DEVLINK_CMD_PORT_PARAM_NEW);
+ }
+}
+EXPORT_SYMBOL_GPL(devlink_port_params_publish);
+
+/**
+ * devlink_port_params_unpublish - unpublish port configuration parameters
+ *
+ * @devlink_port: devlink port
+ *
+ * Unpublish previously registered port configuration parameters.
+ */
+void devlink_port_params_unpublish(struct devlink_port *devlink_port)
+{
+ struct devlink_param_item *param_item;
+
+ list_for_each_entry(param_item, &devlink_port->param_list, list) {
+ if (!param_item->published)
+ continue;
+ param_item->published = false;
+ devlink_param_notify(devlink_port->devlink, devlink_port->index,
+ param_item, DEVLINK_CMD_PORT_PARAM_DEL);
+ }
+}
+EXPORT_SYMBOL_GPL(devlink_port_params_unpublish);
+
static int
__devlink_param_driverinit_value_get(struct list_head *param_list, u32 param_id,
union devlink_param_value *init_val)
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 1/6] gtp: fix suspicious RCU usage
From: Taehee Yoo @ 2019-07-02 15:20 UTC (permalink / raw)
To: davem, pablo, laforge, osmocom-net-gprs, netdev; +Cc: ap420073
gtp_encap_enable_socket() and gtp_encap_destroy() are not protected
by rcu_read_lock(). and it's not safe to write sk->sk_user_data.
This patch make these functions to use lock_sock() instead of
rcu_dereference_sk_user_data().
Test commands:
gtp-link add gtp1
Splat looks like:
[ 83.238315] =============================
[ 83.239127] WARNING: suspicious RCU usage
[ 83.239702] 5.2.0-rc6+ #49 Not tainted
[ 83.240268] -----------------------------
[ 83.241205] drivers/net/gtp.c:799 suspicious rcu_dereference_check() usage!
[ 83.243828]
[ 83.243828] other info that might help us debug this:
[ 83.243828]
[ 83.246325]
[ 83.246325] rcu_scheduler_active = 2, debug_locks = 1
[ 83.247314] 1 lock held by gtp-link/1008:
[ 83.248523] #0: 0000000017772c7f (rtnl_mutex){+.+.}, at: __rtnl_newlink+0x5f5/0x11b0
[ 83.251503]
[ 83.251503] stack backtrace:
[ 83.252173] CPU: 0 PID: 1008 Comm: gtp-link Not tainted 5.2.0-rc6+ #49
[ 83.253271] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[ 83.254562] Call Trace:
[ 83.254995] dump_stack+0x7c/0xbb
[ 83.255567] gtp_encap_enable_socket+0x2df/0x360 [gtp]
[ 83.256415] ? gtp_find_dev+0x1a0/0x1a0 [gtp]
[ 83.257161] ? memset+0x1f/0x40
[ 83.257843] gtp_newlink+0x90/0xa21 [gtp]
[ 83.258497] ? __netlink_ns_capable+0xc3/0xf0
[ 83.259260] __rtnl_newlink+0xb9f/0x11b0
[ 83.260022] ? rtnl_link_unregister+0x230/0x230
[ ... ]
Fixes: 1e3a3abd8b28 ("gtp: make GTP sockets in gtp_newlink optional")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
drivers/net/gtp.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index fc45b749db46..939da5442f65 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -289,12 +289,14 @@ static void gtp_encap_destroy(struct sock *sk)
{
struct gtp_dev *gtp;
- gtp = rcu_dereference_sk_user_data(sk);
+ lock_sock(sk);
+ gtp = sk->sk_user_data;
if (gtp) {
udp_sk(sk)->encap_type = 0;
rcu_assign_sk_user_data(sk, NULL);
sock_put(sk);
}
+ release_sock(sk);
}
static void gtp_encap_disable_sock(struct sock *sk)
@@ -796,7 +798,8 @@ static struct sock *gtp_encap_enable_socket(int fd, int type,
goto out_sock;
}
- if (rcu_dereference_sk_user_data(sock->sk)) {
+ lock_sock(sock->sk);
+ if (sock->sk->sk_user_data) {
sk = ERR_PTR(-EBUSY);
goto out_sock;
}
@@ -812,6 +815,7 @@ static struct sock *gtp_encap_enable_socket(int fd, int type,
setup_udp_tunnel_sock(sock_net(sock->sk), sock, &tuncfg);
out_sock:
+ release_sock(sock->sk);
sockfd_put(sock);
return sk;
}
--
2.17.1
^ permalink raw reply related
* [PATCH net 0/6] gtp: fix several bugs
From: Taehee Yoo @ 2019-07-02 15:20 UTC (permalink / raw)
To: davem, pablo, laforge, osmocom-net-gprs, netdev; +Cc: ap420073
This patch series fixes several bugs in the gtp module.
First patch fixes suspicious RCU usage.
The problem is to use rcu_dereference_sk_user_data() outside of
RCU read critical section.
Second patch fixes use-after-free.
gtp_encap_destroy() is called twice.
gtp_encap_destroy() use both gtp->sk0 and gtp->sk1u.
these pointers can be freed in gtp_encap_destroy().
So, gtp_encap_destroy() should avoid using freed sk pointer.
Third patch removes duplicate code in gtp_dellink().
gtp_dellink() calls gtp_encap_disable() twice.
So, remove one of them.
Fourth patch fixes usage of GFP_KERNEL.
GFP_KERNEL can not be used in RCU read critical section.
This patch make ipv4_pdp_add() to use GFP_ATOMIC instead of GFP_KERNEL.
Fifth patch fixes use-after-free in gtp_newlink().
gtp_newlink() uses gtp_net which would be destroyed by the __exit_net
routine.
So, gtp_newlink should not be called after the __exit_net routine.
Sixth patch adds missing error handling routine in gtp_encap_enable().
gtp_encap_enable() will fail, if invalid role value is sent from
user-space. if so, gtp_encap_enable() should execute error handling
routine.
Taehee Yoo (6):
gtp: fix suspicious RCU usage
gtp: fix use-after-free in gtp_encap_destroy()
gtp: remove duplicate code in gtp_dellink()
gtp: fix Illegal context switch in RCU read-side critical section.
gtp: fix use-after-free in gtp_newlink()
gtp: add missing gtp_encap_disable_sock() in gtp_encap_enable()
drivers/net/gtp.c | 37 +++++++++++++++++++++++++++++--------
1 file changed, 29 insertions(+), 8 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH v2 bpf-next] libbpf: fix GCC8 warning for strncpy
From: Andrii Nakryiko @ 2019-07-02 15:16 UTC (permalink / raw)
To: andrii.nakryiko, bpf, netdev, ast, daniel, andriin,
magnus.karlsson
GCC8 started emitting warning about using strncpy with number of bytes
exactly equal destination size, which is generally unsafe, as can lead
to non-zero terminated string being copied. Use IFNAMSIZ - 1 as number
of bytes to ensure name is always zero-terminated.
Cc: Magnus Karlsson <magnus.karlsson@intel.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/lib/bpf/xsk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index bf15a80a37c2..b33740221b7e 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -327,7 +327,8 @@ static int xsk_get_max_queues(struct xsk_socket *xsk)
channels.cmd = ETHTOOL_GCHANNELS;
ifr.ifr_data = (void *)&channels;
- strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ);
+ strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ - 1);
+ ifr.ifr_name[IFNAMSIZ - 1] = '\0';
err = ioctl(fd, SIOCETHTOOL, &ifr);
if (err && errno != EOPNOTSUPP) {
ret = -errno;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf-next] libbpf: fix GCC8 warning for strncpy
From: Andrii Nakryiko @ 2019-07-02 15:14 UTC (permalink / raw)
To: Y Song
Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, Daniel Borkmann,
Magnus Karlsson
In-Reply-To: <CAH3MdRUv9eJuecKq7weG614+6oEtfLeUHnTxoU19qr39p9-mrQ@mail.gmail.com>
On Mon, Jul 1, 2019 at 11:10 PM Y Song <ys114321@gmail.com> wrote:
>
> On Mon, Jul 1, 2019 at 10:47 PM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > GCC8 started emitting warning about using strncpy with number of bytes
> > exactly equal destination size, which is generally unsafe, as can lead
> > to non-zero terminated string being copied. Use IFNAMSIZ - 1 as number
> > of bytes to ensure name is always zero-terminated.
> >
> > Cc: Magnus Karlsson <magnus.karlsson@intel.com>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > tools/lib/bpf/xsk.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > index bf15a80a37c2..9588e7f87d0b 100644
> > --- a/tools/lib/bpf/xsk.c
> > +++ b/tools/lib/bpf/xsk.c
> > @@ -327,7 +327,7 @@ static int xsk_get_max_queues(struct xsk_socket *xsk)
> >
> > channels.cmd = ETHTOOL_GCHANNELS;
> > ifr.ifr_data = (void *)&channels;
> > - strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ);
> > + strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ - 1);
>
> To accommodate the xsk->ifname string length FNAMSIZ - 1, we need to have
> ifr.ifr_name[FNAMSIZ - 1] = '\0';
> right?
Yes. I somehow misread description of strncpy and assumed it does that
automatically (which would make sense), but it actually doesn't. Only
strlcpy does. v2 with fix is coming.
>
> > err = ioctl(fd, SIOCETHTOOL, &ifr);
> > if (err && errno != EOPNOTSUPP) {
> > ret = -errno;
> > --
> > 2.17.1
> >
^ permalink raw reply
* Re: [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Jesper Dangaard Brouer @ 2019-07-02 15:10 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: netdev, Ilias Apalodimas, grygorii.strashko, jakub.kicinski,
daniel, john.fastabend, ast, linux-kernel, linux-omap, brouer
In-Reply-To: <20190702145612.GF4510@khorivan>
On Tue, 2 Jul 2019 17:56:13 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> On Tue, Jul 02, 2019 at 04:52:30PM +0200, Jesper Dangaard Brouer wrote:
> >On Tue, 2 Jul 2019 17:44:27 +0300
> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> >
> >> On Tue, Jul 02, 2019 at 04:31:39PM +0200, Jesper Dangaard Brouer wrote:
> >> >From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> >
> >> >Jesper recently removed page_pool_destroy() (from driver invocation) and
> >> >moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
> >> >handle in-flight packets/pages. This created an asymmetry in drivers
> >> >create/destroy pairs.
> >> >
> >> >This patch add page_pool user refcnt and reintroduce page_pool_destroy.
> >> >This serves two purposes, (1) simplify drivers error handling as driver now
> >> >drivers always calls page_pool_destroy() and don't need to track if
> >> >xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
> >> >where a single RX-queue (with a single page_pool) provides packets for two
> >> >net_device'es, and thus needs to register the same page_pool twice with two
> >> >xdp_rxq_info structures.
> >>
> >> As I tend to use xdp level patch there is no more reason to mention (2) case
> >> here. XDP patch serves it better and can prevent not only obj deletion but also
> >> pool flush, so, this one patch I could better leave only for (1) case.
> >
> >I don't understand what you are saying.
> >
> >Do you approve this patch, or do you reject this patch?
> >
> It's not reject, it's proposition to use both, XDP and page pool patches,
> each having its goal.
Just to be clear, if you want this patch to get accepted you have to
reply with your Signed-off-by (as I wrote).
Maybe we should discuss it in another thread, about why you want two
solutions to the same problem.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH net-next 1/1] qed: Add support for Timestamping the unicast PTP packets.
From: Sudarsana Reddy Kalluru @ 2019-07-02 15:04 UTC (permalink / raw)
To: davem; +Cc: netdev, mkalderon, aelior
This patch masks the lower 4 bits of PARAM-MASK registers as required
by the Hardware to detect/timestamp the unicast PTP packets. The register
definition in the header file captures more details on the individual bits.
Signed-off-by: Sudarsana Reddy Kalluru <skalluru@marvell.com>
Signed-off-by: Ariel Elior <aelior@marvell.com>
---
drivers/net/ethernet/qlogic/qed/qed_ptp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_ptp.c b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
index f3ebdc5..4a7acfc 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_ptp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
@@ -243,7 +243,7 @@ static int qed_ptp_hw_cfg_filters(struct qed_dev *cdev,
return -EINVAL;
}
- qed_wr(p_hwfn, p_ptt, NIG_REG_LLH_PTP_PARAM_MASK, 0);
+ qed_wr(p_hwfn, p_ptt, NIG_REG_LLH_PTP_PARAM_MASK, 0xF);
qed_wr(p_hwfn, p_ptt, NIG_REG_LLH_PTP_RULE_MASK, rule_mask);
qed_wr(p_hwfn, p_ptt, NIG_REG_RX_PTP_EN, enable_cfg);
@@ -253,7 +253,7 @@ static int qed_ptp_hw_cfg_filters(struct qed_dev *cdev,
qed_wr(p_hwfn, p_ptt, NIG_REG_TX_LLH_PTP_RULE_MASK, 0x3FFF);
} else {
qed_wr(p_hwfn, p_ptt, NIG_REG_TX_PTP_EN, enable_cfg);
- qed_wr(p_hwfn, p_ptt, NIG_REG_TX_LLH_PTP_PARAM_MASK, 0);
+ qed_wr(p_hwfn, p_ptt, NIG_REG_TX_LLH_PTP_PARAM_MASK, 0xF);
qed_wr(p_hwfn, p_ptt, NIG_REG_TX_LLH_PTP_RULE_MASK, rule_mask);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net] rxrpc: Fix oops in tracepoint
From: David Howells @ 2019-07-02 15:04 UTC (permalink / raw)
To: netdev; +Cc: Marc Dionne, dhowells, linux-afs, linux-kernel
If the rxrpc_eproto tracepoint is enabled, an oops will be cause by the
trace line that rxrpc_extract_header() tries to emit when a protocol error
occurs (typically because the packet is short) because the call argument is
NULL.
Fix this by using ?: to assume 0 as the debug_id if call is NULL.
This can then be induced by:
echo -e '\0\0\0\0\0\0\0\0' | ncat -4u --send-only <addr> 20001
where addr has the following program running on it:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <linux/rxrpc.h>
int main(void)
{
struct sockaddr_rxrpc srx;
int fd;
memset(&srx, 0, sizeof(srx));
srx.srx_family = AF_RXRPC;
srx.srx_service = 0;
srx.transport_type = AF_INET;
srx.transport_len = sizeof(srx.transport.sin);
srx.transport.sin.sin_family = AF_INET;
srx.transport.sin.sin_port = htons(0x4e21);
fd = socket(AF_RXRPC, SOCK_DGRAM, AF_INET6);
bind(fd, (struct sockaddr *)&srx, sizeof(srx));
sleep(20);
return 0;
}
It results in the following oops.
BUG: kernel NULL pointer dereference, address: 0000000000000340
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
...
RIP: 0010:trace_event_raw_event_rxrpc_rx_eproto+0x47/0xac
...
Call Trace:
<IRQ>
rxrpc_extract_header+0x86/0x171
? rcu_read_lock_sched_held+0x5d/0x63
? rxrpc_new_skb+0xd4/0x109
rxrpc_input_packet+0xef/0x14fc
? rxrpc_input_data+0x986/0x986
udp_queue_rcv_one_skb+0xbf/0x3d0
udp_unicast_rcv_skb.isra.8+0x64/0x71
ip_protocol_deliver_rcu+0xe4/0x1b4
ip_local_deliver+0xf0/0x154
__netif_receive_skb_one_core+0x50/0x6c
netif_receive_skb_internal+0x26b/0x2e9
napi_gro_receive+0xf8/0x1da
rtl8169_poll+0x303/0x4c4
net_rx_action+0x10e/0x333
__do_softirq+0x1a5/0x38f
irq_exit+0x54/0xc4
do_IRQ+0xda/0xf8
common_interrupt+0xf/0xf
</IRQ>
...
? cpuidle_enter_state+0x23c/0x34d
cpuidle_enter+0x2a/0x36
do_idle+0x163/0x1ea
cpu_startup_entry+0x1d/0x1f
start_secondary+0x157/0x172
secondary_startup_64+0xa4/0xb0
Fixes: a25e21f0bcd2 ("rxrpc, afs: Use debug_ids rather than pointers in traces")
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Marc Dionne <marc.dionne@auristor.com>
---
include/trace/events/rxrpc.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/trace/events/rxrpc.h b/include/trace/events/rxrpc.h
index d85816878a52..cc1d060cbf13 100644
--- a/include/trace/events/rxrpc.h
+++ b/include/trace/events/rxrpc.h
@@ -1379,7 +1379,7 @@ TRACE_EVENT(rxrpc_rx_eproto,
),
TP_fast_assign(
- __entry->call = call->debug_id;
+ __entry->call = call ? call->debug_id : 0;
__entry->serial = serial;
__entry->why = why;
),
^ permalink raw reply related
* re: mlxsw: spectrum: PTP: Support timestamping on Spectrum-1 - potential null ptr dereference
From: Colin Ian King @ 2019-07-02 15:01 UTC (permalink / raw)
To: Petr Machata, Jiri Pirko, Ido Schimmel, David S. Miller, netdev,
linux-kernel@vger.kernel.org
Hi,
Static analysis with Coverity on today's linux-next has found a
potential null pointer dereference bug with the following commit:
commit d92e4e6e33c8b19635be70fb8935b627d2e4f8fe
Author: Petr Machata <petrm@mellanox.com>
Date: Sun Jun 30 09:04:56 2019 +0300
mlxsw: spectrum: PTP: Support timestamping on Spectrum-1
In function: mlxsw_sp1_ptp_packet_finish the offending code is as follows:
/* Between capturing the packet and finishing it, there is a
window of
* opportunity for the originating port to go away (e.g. due to a
* split). Also make sure the SKB device reference is still valid.
*/
mlxsw_sp_port = mlxsw_sp->ports[local_port];
if (!mlxsw_sp_port && (!skb->dev || skb->dev ==
mlxsw_sp_port->dev)) {
dev_kfree_skb_any(skb);
return;
}
If mlxsw_sp_port is null and skb->dev is not-null then the comparison
"skb->dev == mlxsw_sp_port->dev" ends up with a null pointer dereference.
I think the if statement should be:
if (mlxsw_sp_port && (!skb->dev || skb->dev == mlxsw_sp_port->dev))
..but I'm not 100% sure as I may be missing something a bit more subtle
here.
Colin
^ permalink raw reply
* Re: [PATCH bpf] xdp: fix race on generic receive path
From: Magnus Karlsson @ 2019-07-02 15:01 UTC (permalink / raw)
To: Ilya Maximets
Cc: Network Development, linux-kernel, bpf, xdp-newbies,
David S. Miller, Björn Töpel, Magnus Karlsson,
Jonathan Lemon, Jakub Kicinski, Alexei Starovoitov,
Daniel Borkmann
In-Reply-To: <20190702143634.19688-1-i.maximets@samsung.com>
On Tue, Jul 2, 2019 at 4:36 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> Unlike driver mode, generic xdp receive could be triggered
> by different threads on different CPU cores at the same time
> leading to the fill and rx queue breakage. For example, this
> could happen while sending packets from two processes to the
> first interface of veth pair while the second part of it is
> open with AF_XDP socket.
>
> Need to take a lock for each generic receive to avoid race.
Thanks for this catch Ilya. Do you have any performance numbers you
could share of the impact of adding this spin lock? The reason I ask
is that if the impact is negligible, then let us just add it. But if
it is too large, we might want to brain storm about some other
possible solutions.
Thanks: Magnus
> Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support")
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> include/net/xdp_sock.h | 2 ++
> net/xdp/xsk.c | 32 +++++++++++++++++++++++---------
> 2 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> index d074b6d60f8a..ac3c047d058c 100644
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
> @@ -67,6 +67,8 @@ struct xdp_sock {
> * in the SKB destructor callback.
> */
> spinlock_t tx_completion_lock;
> + /* Protects generic receive. */
> + spinlock_t rx_lock;
> u64 rx_dropped;
> };
>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index a14e8864e4fa..19f41d2b670c 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -119,17 +119,22 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
> {
> u32 metalen = xdp->data - xdp->data_meta;
> u32 len = xdp->data_end - xdp->data;
> + unsigned long flags;
> void *buffer;
> u64 addr;
> int err;
>
> - if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index)
> - return -EINVAL;
> + spin_lock_irqsave(&xs->rx_lock, flags);
> +
> + if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index) {
> + err = -EINVAL;
> + goto out_unlock;
> + }
>
> if (!xskq_peek_addr(xs->umem->fq, &addr) ||
> len > xs->umem->chunk_size_nohr - XDP_PACKET_HEADROOM) {
> - xs->rx_dropped++;
> - return -ENOSPC;
> + err = -ENOSPC;
> + goto out_drop;
> }
>
> addr += xs->umem->headroom;
> @@ -138,13 +143,21 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
> memcpy(buffer, xdp->data_meta, len + metalen);
> addr += metalen;
> err = xskq_produce_batch_desc(xs->rx, addr, len);
> - if (!err) {
> - xskq_discard_addr(xs->umem->fq);
> - xsk_flush(xs);
> - return 0;
> - }
> + if (err)
> + goto out_drop;
> +
> + xskq_discard_addr(xs->umem->fq);
> + xskq_produce_flush_desc(xs->rx);
>
> + spin_unlock_irqrestore(&xs->rx_lock, flags);
> +
> + xs->sk.sk_data_ready(&xs->sk);
> + return 0;
> +
> +out_drop:
> xs->rx_dropped++;
> +out_unlock:
> + spin_unlock_irqrestore(&xs->rx_lock, flags);
> return err;
> }
>
> @@ -765,6 +778,7 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
>
> xs = xdp_sk(sk);
> mutex_init(&xs->mutex);
> + spin_lock_init(&xs->rx_lock);
> spin_lock_init(&xs->tx_completion_lock);
>
> mutex_lock(&net->xdp.lock);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 1/1] mlx5: Fix build when CONFIG_MLX5_EN_RXNFC is disabled
From: Jes Sorensen @ 2019-07-02 15:00 UTC (permalink / raw)
To: Saeed Mahameed
Cc: kernel-team@fb.com, jsorensen@fb.com, netdev@vger.kernel.org
In-Reply-To: <ff76fcde486792ad01be1476a66a726d6e1ab933.camel@mellanox.com>
On 6/25/19 2:00 PM, Saeed Mahameed wrote:
> On Tue, 2019-06-25 at 11:27 -0400, Jes Sorensen wrote:
>> From: Jes Sorensen <jsorensen@fb.com>
>>
>> The previous patch broke the build with a static declaration for
>> a public function.
>>
>> Fixes: 8f0916c6dc5c (net/mlx5e: Fix ethtool rxfh commands when
>> CONFIG_MLX5_EN_RXNFC is disabled)
>> Signed-off-by: Jes Sorensen <jsorensen@fb.com>
>> ---
>> drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> index dd764e0471f2..776040d91bd4 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> @@ -1905,7 +1905,8 @@ static int mlx5e_flash_device(struct net_device
>> *dev,
>> /* When CONFIG_MLX5_EN_RXNFC=n we only support ETHTOOL_GRXRINGS
>> * otherwise this function will be defined from en_fs_ethtool.c
>> */
>
> As the above comment states, when CONFIG_MLX5_EN_RXNFC is disabled,
> mlx5e_get_rxnfc is only defined, declared and used in this file, so it
> must be static. Otherwise it will be defined in another file which
> provides much much more functionality for ethtool flow steering.
>
> can you please provide more information of what went wrong on your
> build machine ?
Sorry was swamped here!
Looks like you're right, it only triggers in our build due to some
patches we don't have from upstream. I did the patch against upstream
and applied it to our tree, so should have checked further there.
Cheers,
Jes
^ permalink raw reply
* [PATCH net] rxrpc: Fix send on a connected, but unbound socket
From: David Howells @ 2019-07-02 14:59 UTC (permalink / raw)
To: netdev
Cc: syzbot+7966f2a0b2c7da8939b4, Marc Dionne, dhowells, linux-afs,
linux-kernel
If sendmsg() or sendmmsg() is called on a connected socket that hasn't had
bind() called on it, then an oops will occur when the kernel tries to
connect the call because no local endpoint has been allocated.
Fix this by implicitly binding the socket if it is in the
RXRPC_CLIENT_UNBOUND state, just like it does for the RXRPC_UNBOUND state.
Further, the state should be transitioned to RXRPC_CLIENT_BOUND after this
to prevent further attempts to bind it.
This can be tested with:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <linux/rxrpc.h>
static const unsigned char inet6_addr[16] = {
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, -1, -1, 0xac, 0x14, 0x14, 0xaa
};
int main(void)
{
struct sockaddr_rxrpc srx;
struct cmsghdr *cm;
struct msghdr msg;
unsigned char control[16];
int fd;
memset(&srx, 0, sizeof(srx));
srx.srx_family = 0x21;
srx.srx_service = 0;
srx.transport_type = AF_INET;
srx.transport_len = 0x1c;
srx.transport.sin6.sin6_family = AF_INET6;
srx.transport.sin6.sin6_port = htons(0x4e22);
srx.transport.sin6.sin6_flowinfo = htons(0x4e22);
srx.transport.sin6.sin6_scope_id = htons(0xaa3b);
memcpy(&srx.transport.sin6.sin6_addr, inet6_addr, 16);
cm = (struct cmsghdr *)control;
cm->cmsg_len = CMSG_LEN(sizeof(unsigned long));
cm->cmsg_level = SOL_RXRPC;
cm->cmsg_type = RXRPC_USER_CALL_ID;
*(unsigned long *)CMSG_DATA(cm) = 0;
msg.msg_name = NULL;
msg.msg_namelen = 0;
msg.msg_iov = NULL;
msg.msg_iovlen = 0;
msg.msg_control = control;
msg.msg_controllen = cm->cmsg_len;
msg.msg_flags = 0;
fd = socket(AF_RXRPC, SOCK_DGRAM, AF_INET);
connect(fd, (struct sockaddr *)&srx, sizeof(srx));
sendmsg(fd, &msg, 0);
return 0;
}
Leading to the following oops:
BUG: kernel NULL pointer dereference, address: 0000000000000018
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
...
RIP: 0010:rxrpc_connect_call+0x42/0xa01
...
Call Trace:
? mark_held_locks+0x47/0x59
? __local_bh_enable_ip+0xb6/0xba
rxrpc_new_client_call+0x3b1/0x762
? rxrpc_do_sendmsg+0x3c0/0x92e
rxrpc_do_sendmsg+0x3c0/0x92e
rxrpc_sendmsg+0x16b/0x1b5
sock_sendmsg+0x2d/0x39
___sys_sendmsg+0x1a4/0x22a
? release_sock+0x19/0x9e
? reacquire_held_locks+0x136/0x160
? release_sock+0x19/0x9e
? find_held_lock+0x2b/0x6e
? __lock_acquire+0x268/0xf73
? rxrpc_connect+0xdd/0xe4
? __local_bh_enable_ip+0xb6/0xba
__sys_sendmsg+0x5e/0x94
do_syscall_64+0x7d/0x1bf
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Fixes: 2341e0775747 ("rxrpc: Simplify connect() implementation and simplify sendmsg() op")
Reported-by: syzbot+7966f2a0b2c7da8939b4@syzkaller.appspotmail.com
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Marc Dionne <marc.dionne@auristor.com>
---
net/rxrpc/af_rxrpc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c
index f9f4721cdfa7..d09eaf153544 100644
--- a/net/rxrpc/af_rxrpc.c
+++ b/net/rxrpc/af_rxrpc.c
@@ -545,6 +545,7 @@ static int rxrpc_sendmsg(struct socket *sock, struct msghdr *m, size_t len)
switch (rx->sk.sk_state) {
case RXRPC_UNBOUND:
+ case RXRPC_CLIENT_UNBOUND:
rx->srx.srx_family = AF_RXRPC;
rx->srx.srx_service = 0;
rx->srx.transport_type = SOCK_DGRAM;
@@ -569,10 +570,9 @@ static int rxrpc_sendmsg(struct socket *sock, struct msghdr *m, size_t len)
}
rx->local = local;
- rx->sk.sk_state = RXRPC_CLIENT_UNBOUND;
+ rx->sk.sk_state = RXRPC_CLIENT_BOUND;
/* Fall through */
- case RXRPC_CLIENT_UNBOUND:
case RXRPC_CLIENT_BOUND:
if (!m->msg_name &&
test_bit(RXRPC_SOCK_CONNECTED, &rx->flags)) {
^ permalink raw reply related
* Re: [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Ivan Khoronzhuk @ 2019-07-02 14:56 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Ilias Apalodimas, grygorii.strashko, jakub.kicinski,
daniel, john.fastabend, ast, linux-kernel, linux-omap
In-Reply-To: <20190702165230.6caa36e3@carbon>
On Tue, Jul 02, 2019 at 04:52:30PM +0200, Jesper Dangaard Brouer wrote:
>On Tue, 2 Jul 2019 17:44:27 +0300
>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
>> On Tue, Jul 02, 2019 at 04:31:39PM +0200, Jesper Dangaard Brouer wrote:
>> >From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> >
>> >Jesper recently removed page_pool_destroy() (from driver invocation) and
>> >moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
>> >handle in-flight packets/pages. This created an asymmetry in drivers
>> >create/destroy pairs.
>> >
>> >This patch add page_pool user refcnt and reintroduce page_pool_destroy.
>> >This serves two purposes, (1) simplify drivers error handling as driver now
>> >drivers always calls page_pool_destroy() and don't need to track if
>> >xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
>> >where a single RX-queue (with a single page_pool) provides packets for two
>> >net_device'es, and thus needs to register the same page_pool twice with two
>> >xdp_rxq_info structures.
>>
>> As I tend to use xdp level patch there is no more reason to mention (2) case
>> here. XDP patch serves it better and can prevent not only obj deletion but also
>> pool flush, so, this one patch I could better leave only for (1) case.
>
>I don't understand what you are saying.
>
>Do you approve this patch, or do you reject this patch?
>
It's not reject, it's proposition to use both, XDP and page pool patches,
each having its goal.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* [PATCH net-next] rxrpc: Fix uninitialized error code in rxrpc_send_data_packet()
From: David Howells @ 2019-07-02 14:55 UTC (permalink / raw)
To: netdev; +Cc: Geert Uytterhoeven, dhowells, linux-afs, linux-kernel
With gcc 4.1:
net/rxrpc/output.c: In function ‘rxrpc_send_data_packet’:
net/rxrpc/output.c:338: warning: ‘ret’ may be used uninitialized in this function
Indeed, if the first jump to the send_fragmentable label is made, and
the address family is not handled in the switch() statement, ret will be
used uninitialized.
Fix this by BUG()'ing as is done in other places in rxrpc where internal
support for future address families will need adding. It should not be
possible to reach this normally as the address families are checked
up-front.
Fixes: 5a924b8951f835b5 ("rxrpc: Don't store the rxrpc header in the Tx queue sk_buffs")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/output.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/rxrpc/output.c b/net/rxrpc/output.c
index a0b6abfbd277..948e3fe249ec 100644
--- a/net/rxrpc/output.c
+++ b/net/rxrpc/output.c
@@ -519,6 +519,9 @@ int rxrpc_send_data_packet(struct rxrpc_call *call, struct sk_buff *skb,
}
break;
#endif
+
+ default:
+ BUG();
}
if (ret < 0)
^ permalink raw reply related
* Re: [PATCH net-next v6 01/15] rtnetlink: provide permanent hardware address in RTM_NEWLINK
From: Stephen Hemminger @ 2019-07-02 14:55 UTC (permalink / raw)
To: Michal Kubecek
Cc: David Miller, netdev, Jakub Kicinski, Jiri Pirko, Andrew Lunn,
Florian Fainelli, John Linville, Johannes Berg, linux-kernel
In-Reply-To: <b6e0aefbcb58297b3ec0a12ee4be8e5194eee61a.1562067622.git.mkubecek@suse.cz>
On Tue, 2 Jul 2019 13:49:44 +0200 (CEST)
Michal Kubecek <mkubecek@suse.cz> wrote:
> Permanent hardware address of a network device was traditionally provided
> via ethtool ioctl interface but as Jiri Pirko pointed out in a review of
> ethtool netlink interface, rtnetlink is much more suitable for it so let's
> add it to the RTM_NEWLINK message.
>
> Add IFLA_PERM_ADDRESS attribute to RTM_NEWLINK messages unless the
> permanent address is all zeros (i.e. device driver did not fill it). As
> permanent address is not modifiable, reject userspace requests containing
> IFLA_PERM_ADDRESS attribute.
>
> Note: we already provide permanent hardware address for bond slaves;
> unfortunately we cannot drop that attribute for backward compatibility
> reasons.
>
> v5 -> v6: only add the attribute if permanent address is not zero
>
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
Do you want to make an iproute patch to display this?
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
^ permalink raw reply
* Re: [PATCH v5 net-next 1/6] xdp: allow same allocator usage
From: Ivan Khoronzhuk @ 2019-07-02 14:53 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: grygorii.strashko, hawk, davem, ast, linux-kernel, linux-omap,
xdp-newbies, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend
In-Reply-To: <20190702164648.56ff0761@carbon>
On Tue, Jul 02, 2019 at 04:46:48PM +0200, Jesper Dangaard Brouer wrote:
>On Tue, 2 Jul 2019 13:27:07 +0300
>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
>> On Mon, Jul 01, 2019 at 01:40:59PM +0200, Jesper Dangaard Brouer wrote:
>> >
>> >I'm very skeptical about this approach.
>> >
>> >On Sun, 30 Jun 2019 20:23:43 +0300
>> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>> >
>> >> XDP rxqs can be same for ndevs running under same rx napi softirq.
>> >> But there is no ability to register same allocator for both rxqs,
>> >> by fact it can same rxq but has different ndev as a reference.
>> >
>> >This description is not very clear. It can easily be misunderstood.
>> >
>> >It is an absolute requirement that each RX-queue have their own
>> >page_pool object/allocator. (This where the performance comes from) as
>> >the page_pool have NAPI protected array for alloc and XDP_DROP recycle.
>> >
>> >Your driver/hardware seems to have special case, where a single
>> >RX-queue can receive packets for two different net_device'es.
>> >
>> >Do you violate this XDP devmap redirect assumption[1]?
>> >[1] https://github.com/torvalds/linux/blob/v5.2-rc7/kernel/bpf/devmap.c#L324-L329
>> Seems that yes, but that's used only for trace for now.
>> As it runs under napi and flush clear dev_rx i must do it right in the
>> rx_handler. So next patchset version will have one patch less.
>>
>> Thanks!
>>
>> >
>> >
>> >> Due to last changes allocator destroy can be defered till the moment
>> >> all packets are recycled by destination interface, afterwards it's
>> >> freed. In order to schedule allocator destroy only after all users are
>> >> unregistered, add refcnt to allocator object and schedule to destroy
>> >> only it reaches 0.
>> >
>> >The guiding principles when designing an API, is to make it easy to
>> >use, but also make it hard to misuse.
>> >
>> >Your API change makes it easy to misuse the API. As it make it easy to
>> >(re)use the allocator pointer (likely page_pool) for multiple
>> >xdp_rxq_info structs. It is only valid for your use-case, because you
>> >have hardware where a single RX-queue delivers to two different
>> >net_devices. For other normal use-cases, this will be a violation.
>> >
>> >If I was a user of this API, and saw your xdp_allocator_get(), then I
>> >would assume that this was the normal case. As minimum, we need to add
>> >a comment in the code, about this specific/intended use-case. I
>> >through about detecting the misuse, by adding a queue_index to
>> >xdp_mem_allocator, that can be checked against, when calling
>> >xdp_rxq_info_reg_mem_model() with another xdp_rxq_info struct (to catch
>> >the obvious mistake where queue_index mismatch).
>>
>> I can add, but not sure if it has or can have some conflicts with another
>> memory allocators, now or in future. Main here to not became a cornerstone
>> in some not obvious use-cases.
>>
>> So, for now, let it be in this way:
>>
>> --- a/include/net/xdp_priv.h
>> +++ b/include/net/xdp_priv.h
>> @@ -19,6 +19,7 @@ struct xdp_mem_allocator {
>> struct delayed_work defer_wq;
>> unsigned long defer_warn;
>> unsigned long refcnt;
>> + u32 queue_index;
>> };
>>
>> #endif /* __LINUX_NET_XDP_PRIV_H__ */
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index a44621190fdc..c4bf29810f4d 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -324,7 +324,7 @@ static bool __is_supported_mem_type(enum xdp_mem_type type)
>> return true;
>> }
>>
>> -static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
>> +static struct xdp_mem_allocator *xdp_allocator_find(void *allocator)
>> {
>> struct xdp_mem_allocator *xae, *xa = NULL;
>> struct rhashtable_iter iter;
>> @@ -336,7 +336,6 @@ static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
>>
>> while ((xae = rhashtable_walk_next(&iter)) && !IS_ERR(xae)) {
>> if (xae->allocator == allocator) {
>> - xae->refcnt++;
>> xa = xae;
>> break;
>> }
>> @@ -386,9 +385,13 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
>> }
>> }
>>
>> - xdp_alloc = xdp_allocator_get(allocator);
>> + xdp_alloc = xdp_allocator_find(allocator);
>> if (xdp_alloc) {
>> + if (xdp_alloc->queue_index != xdp_rxq->queue_index)
>> + return -EINVAL;
>> +
>> xdp_rxq->mem.id = xdp_alloc->mem.id;
>> + xdp_alloc->refcnt++;
>
>This is now adjusted outside lock, not good.
In final it serves:
From f43a0b85838f75814cc93e5a724c4c7e5615f936 Mon Sep 17 00:00:00 2001
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date: Fri, 28 Jun 2019 03:17:24 +0300
Subject: [PATCH] xdp: allow same allocator usage
XDP rxqs can be same for ndevs running under same rx napi softirq.
But there is no ability to register same allocator for both rxqs,
by fact it can same rxq but has different ndev as a reference.
Due to last changes allocator destroy can be defered till the moment
all packets are recycled by destination interface, afterwards it's
freed. In order to schedule allocator destroy only after all users are
unregistered, add refcnt to allocator object and schedule to destroy
only it reaches 0.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
include/net/xdp_priv.h | 2 ++
net/core/xdp.c | 52 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+)
diff --git a/include/net/xdp_priv.h b/include/net/xdp_priv.h
index 6a8cba6ea79a..9858a4057842 100644
--- a/include/net/xdp_priv.h
+++ b/include/net/xdp_priv.h
@@ -18,6 +18,8 @@ struct xdp_mem_allocator {
struct rcu_head rcu;
struct delayed_work defer_wq;
unsigned long defer_warn;
+ unsigned long refcnt;
+ u32 queue_index;
};
#endif /* __LINUX_NET_XDP_PRIV_H__ */
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 829377cc83db..090f26e4f793 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -98,6 +98,18 @@ static bool __mem_id_disconnect(int id, bool force)
WARN(1, "Request remove non-existing id(%d), driver bug?", id);
return true;
}
+
+ /* to avoid calling hash lookup twice, decrement refcnt here till it
+ * reaches zero, then it can be called from workqueue afterwards.
+ */
+ if (xa->refcnt)
+ xa->refcnt--;
+
+ if (xa->refcnt) {
+ mutex_unlock(&mem_id_lock);
+ return true;
+ }
+
xa->disconnect_cnt++;
/* Detects in-flight packet-pages for page_pool */
@@ -312,6 +324,30 @@ static bool __is_supported_mem_type(enum xdp_mem_type type)
return true;
}
+static struct xdp_mem_allocator *xdp_allocator_find(void *allocator)
+{
+ struct xdp_mem_allocator *xae, *xa = NULL;
+ struct rhashtable_iter iter;
+
+ rhashtable_walk_enter(mem_id_ht, &iter);
+ do {
+ rhashtable_walk_start(&iter);
+
+ while ((xae = rhashtable_walk_next(&iter)) && !IS_ERR(xae)) {
+ if (xae->allocator == allocator) {
+ xa = xae;
+ break;
+ }
+ }
+
+ rhashtable_walk_stop(&iter);
+
+ } while (xae == ERR_PTR(-EAGAIN));
+ rhashtable_walk_exit(&iter);
+
+ return xa;
+}
+
int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
enum xdp_mem_type type, void *allocator)
{
@@ -347,6 +383,20 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
}
}
+ mutex_lock(&mem_id_lock);
+ xdp_alloc = xdp_allocator_find(allocator);
+ if (xdp_alloc) {
+ /* One allocator per queue is supposed only */
+ if (xdp_alloc->queue_index != xdp_rxq->queue_index)
+ return -EINVAL;
+
+ xdp_rxq->mem.id = xdp_alloc->mem.id;
+ xdp_alloc->refcnt++;
+ mutex_unlock(&mem_id_lock);
+ return 0;
+ }
+ mutex_unlock(&mem_id_lock);
+
xdp_alloc = kzalloc(sizeof(*xdp_alloc), gfp);
if (!xdp_alloc)
return -ENOMEM;
@@ -360,6 +410,8 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
xdp_rxq->mem.id = id;
xdp_alloc->mem = xdp_rxq->mem;
xdp_alloc->allocator = allocator;
+ xdp_alloc->refcnt = 1;
+ xdp_alloc->queue_index = xdp_rxq->queue_index;
/* Insert allocator into ID lookup table */
ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node);
>
>> return 0;
>> }
>>
>> @@ -406,6 +409,7 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
>> xdp_alloc->mem = xdp_rxq->mem;
>> xdp_alloc->allocator = allocator;
>> xdp_alloc->refcnt = 1;
>> + xdp_alloc->queue_index = xdp_rxq->queue_index;
>>
>> /* Insert allocator into ID lookup table */
>> ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node);
>>
>> Jesper, are you Ok with this version?
>
>Please see my other patch, this is based on our first refcnt attempt.
>I think that other patch is a better way forward.
XDP patch serves it better and can prevent not only obj deletion but also
pool flush. So I propose use 2 patches.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply related
* Re: [PATCH net-next v6 04/15] ethtool: introduce ethtool netlink interface
From: Michal Kubecek @ 2019-07-02 14:52 UTC (permalink / raw)
To: netdev
Cc: Jiri Pirko, David Miller, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <20190702122521.GN2250@nanopsycho>
On Tue, Jul 02, 2019 at 02:25:21PM +0200, Jiri Pirko wrote:
> Tue, Jul 02, 2019 at 01:49:59PM CEST, mkubecek@suse.cz wrote:
> >+Request header
> >+--------------
> >+
> >+Each request or reply message contains a nested attribute with common header.
> >+Structure of this header is
>
> Missing ":"
OK
> >+
> >+ ETHTOOL_A_HEADER_DEV_INDEX (u32) device ifindex
> >+ ETHTOOL_A_HEADER_DEV_NAME (string) device name
> >+ ETHTOOL_A_HEADER_INFOMASK (u32) info mask
> >+ ETHTOOL_A_HEADER_GFLAGS (u32) flags common for all requests
> >+ ETHTOOL_A_HEADER_RFLAGS (u32) request specific flags
> >+
> >+ETHTOOL_A_HEADER_DEV_INDEX and ETHTOOL_A_HEADER_DEV_NAME identify the device
> >+message relates to. One of them is sufficient in requests, if both are used,
> >+they must identify the same device. Some requests, e.g. global string sets, do
> >+not require device identification. Most GET requests also allow dump requests
> >+without device identification to query the same information for all devices
> >+providing it (each device in a separate message).
> >+
> >+Optional info mask allows to ask only for a part of data provided by GET
>
> How this "infomask" works? What are the bits related to? Is that request
> specific?
The interpretation is request specific, the information returned for
a GET request is divided into multiple parts and client can choose to
request one of them (usually one). In the code so far, infomask bits
correspond to top level (nest) attributes but I would rather not make it
a strict rule.
I'll make the paragraph more verbose.
> >+request types. If omitted or zero, all data is returned. The two flag bitmaps
> >+allow enabling requestoptions; ETHTOOL_A_HEADER_GFLAGS are global flags common
>
> s/requestoptions;/request options./ ?
Yes.
> >+for all request types, flags recognized in ETHTOOL_A_HEADER_RFLAGS and their
> >+interpretation are specific for each request type. Global flags are
> >+
> >+ ETHTOOL_RF_COMPACT use compact format bitsets in reply
>
> Why "RF"? Isn't this "GF"? I would like "ETHTOOL_GFLAG_COMPACT" better.
RF as Request Flags. At the moment, global flags use ETHTOOL_RF_name
pattern and request specific flags ETHTOOL_RF_msgtype_name. GFLAG and
RFLAG would probably show the relation better, so how about
ETHTOOL_GFLAG_name for global
ETHTOOL_RFLAG_msgtype_name for request specific
> >+ ETHTOOL_RF_REPLY send optional reply (SET and ACT requests)
> >+
> >+Request specific flags are described with each request type. For both flag
> >+attributes, new flags should follow the general idea that if the flag is not
> >+set, the behaviour is the same as (or closer to) the behaviour before it was
>
> "closer to" ? That would be unfortunate I believe...
There may be situations where it cannot be exactly the same, e.g.
because the flag affects interpretation of an attribute which was
introduced together with the flag. How about "...the behaviour is
backward compatible"?
> >+List of message types
> >+---------------------
> >+
> >+All constants identifying message types use ETHTOOL_CMD_ prefix and suffix
> >+according to message purpose:
> >+
> >+ _GET userspace request to retrieve data
> >+ _SET userspace request to set data
> >+ _ACT userspace request to perform an action
> >+ _GET_REPLY kernel reply to a GET request
> >+ _SET_REPLY kernel reply to a SET request
> >+ _ACT_REPLY kernel reply to an ACT request
> >+ _NTF kernel notification
> >+
> >+"GET" requests are sent by userspace applications to retrieve device
> >+information. They usually do not contain any message specific attributes.
> >+Kernel replies with corresponding "GET_REPLY" message. For most types, "GET"
> >+request with NLM_F_DUMP and no device identification can be used to query the
> >+information for all devices supporting the request.
> >+
> >+If the data can be also modified, corresponding "SET" message with the same
> >+layout as "GET" reply is used to request changes. Only attributes where
>
> s/"GET" reply"/"GET_REPLY" ?
> Maybe better to emphasize that the "GET_REPLY" is the one corresponding
> with "SET". But perhaps I got this sentence all wrong :/
OK
> >+a change is requested are included in such request (also, not all attributes
> >+may be changed). Replies to most "SET" request consist only of error code and
> >+extack; if kernel provides additional data, it is sent in the form of
> >+corresponding "SET_REPLY" message (if ETHTOOL_RF_REPLY flag was set in request
> >+header).
> >+
> >+Data modification also triggers sending a "NTF" message with a notification.
> >+These usually bear only a subset of attributes which was affected by the
> >+change. The same notification is issued if the data is modified using other
> >+means (mostly ioctl ethtool interface). Unlike notifications from ethtool
> >+netlink code which are only sent if something actually changed, notifications
> >+triggered by ioctl interface may be sent even if the request did not actually
> >+change any data.
>
> Interesting. What's the reason for that?
Most setting commands in ioctl interface do not even query the original
state, they just pass the structure from ioctl() to ethtool_ops handler.
We could add retrieving the original state first but I suppose we would
still have to call the handler anyway even if requested values are the
same (as that's what kernel does now) and it's not clear if omitting the
notification in such case is the right thing to do.
> >diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
> >index 3ebfab2bca66..f30e0da88be5 100644
> >--- a/net/ethtool/Makefile
> >+++ b/net/ethtool/Makefile
> >@@ -1,3 +1,7 @@
> > # SPDX-License-Identifier: GPL-2.0
> >
> >-obj-y += ioctl.o
> >+obj-y += ioctl.o
> >+
> >+obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o
>
> Hmm, I wonder, why not to make this always on? We want users to use
> it, memory savings in case it is off would be minimal. RTNetlink is also
> always on. Ethtool ioctl is also always on.
We have already discussed this in the previous version. Someone claimed
earlier that building a kernel without ethtool interface would make
sense for some minimalistic systems. My plan is to make the ioctl
interface also optional once it's possible for (sufficiently new)
ethtool to work without it.
Michal
^ permalink raw reply
* Re: [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Jesper Dangaard Brouer @ 2019-07-02 14:52 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: netdev, Ilias Apalodimas, grygorii.strashko, jakub.kicinski,
daniel, john.fastabend, ast, linux-kernel, linux-omap, brouer
In-Reply-To: <20190702144426.GD4510@khorivan>
On Tue, 2 Jul 2019 17:44:27 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> On Tue, Jul 02, 2019 at 04:31:39PM +0200, Jesper Dangaard Brouer wrote:
> >From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >
> >Jesper recently removed page_pool_destroy() (from driver invocation) and
> >moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
> >handle in-flight packets/pages. This created an asymmetry in drivers
> >create/destroy pairs.
> >
> >This patch add page_pool user refcnt and reintroduce page_pool_destroy.
> >This serves two purposes, (1) simplify drivers error handling as driver now
> >drivers always calls page_pool_destroy() and don't need to track if
> >xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
> >where a single RX-queue (with a single page_pool) provides packets for two
> >net_device'es, and thus needs to register the same page_pool twice with two
> >xdp_rxq_info structures.
>
> As I tend to use xdp level patch there is no more reason to mention (2) case
> here. XDP patch serves it better and can prevent not only obj deletion but also
> pool flush, so, this one patch I could better leave only for (1) case.
I don't understand what you are saying.
Do you approve this patch, or do you reject this patch?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH v5 net-next 1/6] xdp: allow same allocator usage
From: Jesper Dangaard Brouer @ 2019-07-02 14:46 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: grygorii.strashko, hawk, davem, ast, linux-kernel, linux-omap,
xdp-newbies, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend, brouer
In-Reply-To: <20190702102700.GA4510@khorivan>
On Tue, 2 Jul 2019 13:27:07 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> On Mon, Jul 01, 2019 at 01:40:59PM +0200, Jesper Dangaard Brouer wrote:
> >
> >I'm very skeptical about this approach.
> >
> >On Sun, 30 Jun 2019 20:23:43 +0300
> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> >
> >> XDP rxqs can be same for ndevs running under same rx napi softirq.
> >> But there is no ability to register same allocator for both rxqs,
> >> by fact it can same rxq but has different ndev as a reference.
> >
> >This description is not very clear. It can easily be misunderstood.
> >
> >It is an absolute requirement that each RX-queue have their own
> >page_pool object/allocator. (This where the performance comes from) as
> >the page_pool have NAPI protected array for alloc and XDP_DROP recycle.
> >
> >Your driver/hardware seems to have special case, where a single
> >RX-queue can receive packets for two different net_device'es.
> >
> >Do you violate this XDP devmap redirect assumption[1]?
> >[1] https://github.com/torvalds/linux/blob/v5.2-rc7/kernel/bpf/devmap.c#L324-L329
> Seems that yes, but that's used only for trace for now.
> As it runs under napi and flush clear dev_rx i must do it right in the
> rx_handler. So next patchset version will have one patch less.
>
> Thanks!
>
> >
> >
> >> Due to last changes allocator destroy can be defered till the moment
> >> all packets are recycled by destination interface, afterwards it's
> >> freed. In order to schedule allocator destroy only after all users are
> >> unregistered, add refcnt to allocator object and schedule to destroy
> >> only it reaches 0.
> >
> >The guiding principles when designing an API, is to make it easy to
> >use, but also make it hard to misuse.
> >
> >Your API change makes it easy to misuse the API. As it make it easy to
> >(re)use the allocator pointer (likely page_pool) for multiple
> >xdp_rxq_info structs. It is only valid for your use-case, because you
> >have hardware where a single RX-queue delivers to two different
> >net_devices. For other normal use-cases, this will be a violation.
> >
> >If I was a user of this API, and saw your xdp_allocator_get(), then I
> >would assume that this was the normal case. As minimum, we need to add
> >a comment in the code, about this specific/intended use-case. I
> >through about detecting the misuse, by adding a queue_index to
> >xdp_mem_allocator, that can be checked against, when calling
> >xdp_rxq_info_reg_mem_model() with another xdp_rxq_info struct (to catch
> >the obvious mistake where queue_index mismatch).
>
> I can add, but not sure if it has or can have some conflicts with another
> memory allocators, now or in future. Main here to not became a cornerstone
> in some not obvious use-cases.
>
> So, for now, let it be in this way:
>
> --- a/include/net/xdp_priv.h
> +++ b/include/net/xdp_priv.h
> @@ -19,6 +19,7 @@ struct xdp_mem_allocator {
> struct delayed_work defer_wq;
> unsigned long defer_warn;
> unsigned long refcnt;
> + u32 queue_index;
> };
>
> #endif /* __LINUX_NET_XDP_PRIV_H__ */
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index a44621190fdc..c4bf29810f4d 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -324,7 +324,7 @@ static bool __is_supported_mem_type(enum xdp_mem_type type)
> return true;
> }
>
> -static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
> +static struct xdp_mem_allocator *xdp_allocator_find(void *allocator)
> {
> struct xdp_mem_allocator *xae, *xa = NULL;
> struct rhashtable_iter iter;
> @@ -336,7 +336,6 @@ static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
>
> while ((xae = rhashtable_walk_next(&iter)) && !IS_ERR(xae)) {
> if (xae->allocator == allocator) {
> - xae->refcnt++;
> xa = xae;
> break;
> }
> @@ -386,9 +385,13 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
> }
> }
>
> - xdp_alloc = xdp_allocator_get(allocator);
> + xdp_alloc = xdp_allocator_find(allocator);
> if (xdp_alloc) {
> + if (xdp_alloc->queue_index != xdp_rxq->queue_index)
> + return -EINVAL;
> +
> xdp_rxq->mem.id = xdp_alloc->mem.id;
> + xdp_alloc->refcnt++;
This is now adjusted outside lock, not good.
> return 0;
> }
>
> @@ -406,6 +409,7 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
> xdp_alloc->mem = xdp_rxq->mem;
> xdp_alloc->allocator = allocator;
> xdp_alloc->refcnt = 1;
> + xdp_alloc->queue_index = xdp_rxq->queue_index;
>
> /* Insert allocator into ID lookup table */
> ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node);
>
> Jesper, are you Ok with this version?
Please see my other patch, this is based on our first refcnt attempt.
I think that other patch is a better way forward.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Ivan Khoronzhuk @ 2019-07-02 14:44 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Ilias Apalodimas, grygorii.strashko, jakub.kicinski,
daniel, john.fastabend, ast, linux-kernel, linux-omap
In-Reply-To: <156207778364.29180.5111562317930943530.stgit@firesoul>
On Tue, Jul 02, 2019 at 04:31:39PM +0200, Jesper Dangaard Brouer wrote:
>From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>
>Jesper recently removed page_pool_destroy() (from driver invocation) and
>moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
>handle in-flight packets/pages. This created an asymmetry in drivers
>create/destroy pairs.
>
>This patch add page_pool user refcnt and reintroduce page_pool_destroy.
>This serves two purposes, (1) simplify drivers error handling as driver now
>drivers always calls page_pool_destroy() and don't need to track if
>xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
>where a single RX-queue (with a single page_pool) provides packets for two
>net_device'es, and thus needs to register the same page_pool twice with two
>xdp_rxq_info structures.
As I tend to use xdp level patch there is no more reason to mention (2) case
here. XDP patch serves it better and can prevent not only obj deletion but also
pool flush, so, this one patch I could better leave only for (1) case.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* [PATCH][next] wil6210: fix wil_cid_valid with negative cid values
From: Colin King @ 2019-07-02 14:40 UTC (permalink / raw)
To: Maya Erez, Kalle Valo, David S . Miller, linux-wireless, wil6210,
netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
There are several occasions where a negative cid value is passed
into wil_cid_valid and this is converted into a u8 causing the
range check of cid >= 0 to always succeed. Fix this by making
the cid argument an int to handle any -ve error value of cid.
An example of this behaviour is in wil_cfg80211_dump_station,
where cid is assigned -ENOENT if the call to wil_find_cid_by_idx
fails, and this -ve value is passed to wil_cid_valid. I believe
that the conversion of -ENOENT to the u8 value 254 which is
greater than wil->max_assoc_sta causes wil_find_cid_by_idx to
currently work fine, but I think is by luck and not the
intended behaviour.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wireless/ath/wil6210/wil6210.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h
index 6f456b311a39..25a1adcb38eb 100644
--- a/drivers/net/wireless/ath/wil6210/wil6210.h
+++ b/drivers/net/wireless/ath/wil6210/wil6210.h
@@ -1144,7 +1144,7 @@ static inline void wil_c(struct wil6210_priv *wil, u32 reg, u32 val)
/**
* wil_cid_valid - check cid is valid
*/
-static inline bool wil_cid_valid(struct wil6210_priv *wil, u8 cid)
+static inline bool wil_cid_valid(struct wil6210_priv *wil, int cid)
{
return (cid >= 0 && cid < wil->max_assoc_sta);
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] User mode linux bump maximum MTU tuntap interface [RESAND]
From: Richard Weinberger @ 2019-07-02 14:40 UTC (permalink / raw)
To: Алексей
Cc: netdev, linux-um, Anton Ivanov
In-Reply-To: <fc526c78-2d3f-90ca-8317-a89eb653cbf9@yandex.ru>
CC'ing um folks.
On Tue, Jul 2, 2019 at 3:01 PM Алексей <ne-vlezay80@yandex.ru> wrote:
>
> Hello, the parameter ETH_MAX_PACKET limited to 1500 bytes is the not
> support jumbo frames.
>
> This patch change ETH_MAX_PACKET the 65535 bytes to jumbo frame support
> with user mode linux tuntap driver.
>
>
> PATCH:
>
> -------------------
>
>
> diff -ruNP ../linux_orig/linux-5.1/arch/um/include/shared/net_user.h
> ./arch/um/include/shared/net_user.h
> --- a/arch/um/include/shared/net_user.h 2019-05-06 00:42:58.000000000
> +0000
> +++ b/arch/um/include/shared/net_user.h 2019-07-02 07:14:13.593333356
> +0000
> @@ -9,7 +9,7 @@
> #define ETH_ADDR_LEN (6)
> #define ETH_HEADER_ETHERTAP (16)
> #define ETH_HEADER_OTHER (26) /* 14 for ethernet + VLAN + MPLS for
> crazy people */
> -#define ETH_MAX_PACKET (1500)
> +#define ETH_MAX_PACKET (65535)
>
> #define UML_NET_VERSION (4)
>
> -------------------
>
>
--
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH net-next 5/5] ipv4: use indirect call wrappers for {tcp,udp}_{recv,send}msg()
From: Willem de Bruijn @ 2019-07-02 14:38 UTC (permalink / raw)
To: Paolo Abeni; +Cc: Network Development, David S. Miller
In-Reply-To: <a2806dab7e472b5316da87318f1b8e48ff68cd4b.camel@redhat.com>
On Tue, Jul 2, 2019 at 9:03 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Mon, 2019-07-01 at 15:07 -0400, Willem de Bruijn wrote:
> > On Mon, Jul 1, 2019 at 1:10 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > > This avoids an indirect call per syscall for common ipv4 transports
> > >
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > ---
> > > net/ipv4/af_inet.c | 12 +++++++++---
> > > 1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> > > index 8421e2f5bbb3..9a2f17d0c5f5 100644
> > > --- a/net/ipv4/af_inet.c
> > > +++ b/net/ipv4/af_inet.c
> > > @@ -797,6 +797,8 @@ int inet_send_prepare(struct sock *sk)
> > > }
> > > EXPORT_SYMBOL_GPL(inet_send_prepare);
> > >
> > > +INDIRECT_CALLABLE_DECLARE(int udp_sendmsg(struct sock *, struct msghdr *,
> > > + size_t));
> >
> > Small nit: this is already defined in include/net/udp.h, which is
> > included. So like tcp_sendmsg, probably no need to declare.
>
> Thank you for the review!
>
> You are right, that declaration can be dropped.
> >
> > If defining inet6_sendmsg and inet6_recvmsg in include/net/ipv6.h,
> > perhaps do the same for the other missing functions, instead of these
> > indirect declarations at the callsite?
>
> Uhm... since inet6_{send,recv}msg exists only for retpoline sake and
> are not exported, I think is probably better move their declaration to
> socket.c via INDIRECT_CALLABLE_DECLARE(), to that ICWs are all self-
> contained.
>
> Unless there are objections about spamming, I can repost the series
> with the above changes.
If just for the spurious declaration, can be merged as is, too.
Either way. Not spammy at all to resend after a day.
^ permalink raw reply
* [PATCH bpf] xdp: fix race on generic receive path
From: Ilya Maximets @ 2019-07-02 14:36 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, bpf, xdp-newbies, David S. Miller,
Björn Töpel, Magnus Karlsson, Jonathan Lemon,
Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
Ilya Maximets
In-Reply-To: <CGME20190702143639eucas1p2b168c68c35b70aac75cad6c72ccc81ad@eucas1p2.samsung.com>
Unlike driver mode, generic xdp receive could be triggered
by different threads on different CPU cores at the same time
leading to the fill and rx queue breakage. For example, this
could happen while sending packets from two processes to the
first interface of veth pair while the second part of it is
open with AF_XDP socket.
Need to take a lock for each generic receive to avoid race.
Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support")
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
include/net/xdp_sock.h | 2 ++
net/xdp/xsk.c | 32 +++++++++++++++++++++++---------
2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index d074b6d60f8a..ac3c047d058c 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -67,6 +67,8 @@ struct xdp_sock {
* in the SKB destructor callback.
*/
spinlock_t tx_completion_lock;
+ /* Protects generic receive. */
+ spinlock_t rx_lock;
u64 rx_dropped;
};
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index a14e8864e4fa..19f41d2b670c 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -119,17 +119,22 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
{
u32 metalen = xdp->data - xdp->data_meta;
u32 len = xdp->data_end - xdp->data;
+ unsigned long flags;
void *buffer;
u64 addr;
int err;
- if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index)
- return -EINVAL;
+ spin_lock_irqsave(&xs->rx_lock, flags);
+
+ if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index) {
+ err = -EINVAL;
+ goto out_unlock;
+ }
if (!xskq_peek_addr(xs->umem->fq, &addr) ||
len > xs->umem->chunk_size_nohr - XDP_PACKET_HEADROOM) {
- xs->rx_dropped++;
- return -ENOSPC;
+ err = -ENOSPC;
+ goto out_drop;
}
addr += xs->umem->headroom;
@@ -138,13 +143,21 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
memcpy(buffer, xdp->data_meta, len + metalen);
addr += metalen;
err = xskq_produce_batch_desc(xs->rx, addr, len);
- if (!err) {
- xskq_discard_addr(xs->umem->fq);
- xsk_flush(xs);
- return 0;
- }
+ if (err)
+ goto out_drop;
+
+ xskq_discard_addr(xs->umem->fq);
+ xskq_produce_flush_desc(xs->rx);
+ spin_unlock_irqrestore(&xs->rx_lock, flags);
+
+ xs->sk.sk_data_ready(&xs->sk);
+ return 0;
+
+out_drop:
xs->rx_dropped++;
+out_unlock:
+ spin_unlock_irqrestore(&xs->rx_lock, flags);
return err;
}
@@ -765,6 +778,7 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
xs = xdp_sk(sk);
mutex_init(&xs->mutex);
+ spin_lock_init(&xs->rx_lock);
spin_lock_init(&xs->tx_completion_lock);
mutex_lock(&net->xdp.lock);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next 3/3] macsec: add brackets and indentation after calling macsec_decrypt
From: Willem de Bruijn @ 2019-07-02 14:35 UTC (permalink / raw)
To: Andreas Steinmetz; +Cc: Sabrina Dubroca, Network Development
In-Reply-To: <b393fc9f9a9e4e49b9cbe6edebb3cd38301ffd92.camel@domdv.de>
On Tue, Jul 2, 2019 at 12:38 AM Andreas Steinmetz <ast@domdv.de> wrote:
>
> Ouch, I missed that when Andreas sent me that patch before. No, it is
> > actually intended. If we skip macsec_decrypt(), we should still
> > account for that packet in the InPktsUnchecked/InPktsDelayed
> > counters. That's in Figure 10-5 in the standard.
> >
> > Thanks for catching this, Willem. That patch should only move the
> > IS_ERR(skb) case under the block where macsec_decrypt() is called, but
> > not move the call to macsec_post_decrypt().
>
> Updated patch below.
>
> Signed-off-by: Andreas Steinmetz <ast@domdv.de>
When making a change in a patch set, please resubmit the entire
patchset (with a v2, and record the changelog).
^ 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