* Hello Beautiful
From: Jack @ 2018-07-29 11:14 UTC (permalink / raw)
Cześć Drogi, nazywam się Jack i szukam związku, w którym będę czuć się kochany po serii nieudanych związków.
Mam nadzieję, że byłbyś zainteresowany i moglibyśmy się lepiej poznać, jeśli nie masz nic przeciwko. Jestem otwarty na udzielanie odpowiedzi na pytania od ciebie, ponieważ uważam, że moje podejście jest trochę niewłaściwe. Mam nadzieję, że odezwę się od ciebie.
Jacek.
^ permalink raw reply
* [PATCH] 9p: fix Use-After-Free in p9_write_work()
From: Tomas Bortoli @ 2018-07-29 13:02 UTC (permalink / raw)
To: ericvh, rminnich, lucho
Cc: asmadeus, davem, v9fs-developer, netdev, linux-kernel, syzkaller,
Tomas Bortoli
There is a race condition between p9_free_req() and p9_write_work().
A request might still need to be processed while p9_free_req() is called.
To fix it, flush the read/write work before freeing any request.
Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
Reported-by: syzbot+467050c1ce275af2a5b8@syzkaller.appspotmail.com
---
To be able to flush the r/w work from client.c we need the p9_conn and
p9_trans_fd definitions. Therefore this commit moves most of the declarations in
trans_fd.c to trans_fd.h and import such file in client.c
Moreover, a couple of identifiers were altered to avoid name conflicts with the
new import.
include/net/9p/trans_fd.h | 139 ++++++++++++++++++++++++++++++++++++++++++++++
net/9p/client.c | 8 ++-
net/9p/trans_fd.c | 113 +------------------------------------
3 files changed, 148 insertions(+), 112 deletions(-)
create mode 100644 include/net/9p/trans_fd.h
diff --git a/include/net/9p/trans_fd.h b/include/net/9p/trans_fd.h
new file mode 100644
index 000000000000..cfd4457c40fb
--- /dev/null
+++ b/include/net/9p/trans_fd.h
@@ -0,0 +1,139 @@
+/*
+ * include/fs/9p/trans_fd.h
+ *
+ * Fd transport layer definitions.
+ *
+ * Copyright (C) 2006 by Russ Cox <rsc@swtch.com>
+ * Copyright (C) 2004-2005 by Latchesar Ionkov <lucho@ionkov.net>
+ * Copyright (C) 2004-2008 by Eric Van Hensbergen <ericvh@gmail.com>
+ * Copyright (C) 1997-2002 by Ron Minnich <rminnich@sarnoff.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to:
+ * Free Software Foundation
+ * 51 Franklin Street, Fifth Floor
+ * Boston, MA 02111-1301 USA
+ *
+ */
+
+#ifndef P9_TRANS_FD_H
+#define P9_TRANS_FD_H
+
+/**
+ * struct p9_fd_opts - per-transport options
+ * @rfd: file descriptor for reading (trans=fd)
+ * @wfd: file descriptor for writing (trans=fd)
+ * @port: port to connect to (trans=tcp)
+ *
+ */
+
+#define P9_PORT 564
+#define MAX_SOCK_BUF (64*1024)
+#define MAXPOLLWADDR 2
+
+struct p9_fd_opts {
+ int rfd;
+ int wfd;
+ u16 port;
+ bool privport;
+};
+
+/*
+ * Option Parsing (code inspired by NFS code)
+ * - a little lazy - parse all fd-transport options
+ */
+
+enum {
+ /* Options that take integer arguments */
+ Opt_port, Opt_rfdno, Opt_wfdno, Opt_err,
+ /* Options that take no arguments */
+ Opt_privport,
+};
+
+static const match_table_t trans_tokens = {
+ {Opt_port, "port=%u"},
+ {Opt_rfdno, "rfdno=%u"},
+ {Opt_wfdno, "wfdno=%u"},
+ {Opt_privport, "privport"},
+ {Opt_err, NULL},
+};
+
+enum {
+ Rworksched = 1, /* read work scheduled or running */
+ Rpending = 2, /* can read */
+ Wworksched = 4, /* write work scheduled or running */
+ Wpending = 8, /* can write */
+};
+
+struct p9_poll_wait {
+ struct p9_conn *conn;
+ wait_queue_entry_t wait;
+ wait_queue_head_t *wait_addr;
+};
+
+/**
+ * struct p9_conn - fd mux connection state information
+ * @mux_list: list link for mux to manage multiple connections (?)
+ * @client: reference to client instance for this connection
+ * @err: error state
+ * @req_list: accounting for requests which have been sent
+ * @unsent_req_list: accounting for requests that haven't been sent
+ * @req: current request being processed (if any)
+ * @tmp_buf: temporary buffer to read in header
+ * @rc: temporary fcall for reading current frame
+ * @wpos: write position for current frame
+ * @wsize: amount of data to write for current frame
+ * @wbuf: current write buffer
+ * @poll_pending_link: pending links to be polled per conn
+ * @poll_wait: array of wait_q's for various worker threads
+ * @pt: poll state
+ * @rq: current read work
+ * @wq: current write work
+ * @wsched: ????
+ *
+ */
+
+struct p9_conn {
+ struct list_head mux_list;
+ struct p9_client *client;
+ int err;
+ struct list_head req_list;
+ struct list_head unsent_req_list;
+ struct p9_req_t *req;
+ char tmp_buf[7];
+ struct p9_fcall rc;
+ int wpos;
+ int wsize;
+ char *wbuf;
+ struct list_head poll_pending_link;
+ struct p9_poll_wait poll_wait[MAXPOLLWADDR];
+ poll_table pt;
+ struct work_struct rq;
+ struct work_struct wq;
+ unsigned long wsched;
+};
+
+/**
+ * struct p9_trans_fd - transport state
+ * @rd: reference to file to read from
+ * @wr: reference of file to write to
+ * @conn: connection state reference
+ *
+ */
+
+struct p9_trans_fd {
+ struct file *rd;
+ struct file *wr;
+ struct p9_conn conn;
+};
+
+#endif
diff --git a/net/9p/client.c b/net/9p/client.c
index 2ec0edc6104f..ddfb63672a63 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -40,6 +40,7 @@
#include <linux/seq_file.h>
#include <net/9p/client.h>
#include <net/9p/transport.h>
+#include <net/9p/trans_fd.h>
#include "protocol.h"
#define CREATE_TRACE_POINTS
@@ -55,7 +56,7 @@ enum {
Opt_trans,
Opt_legacy,
Opt_version,
- Opt_err,
+ Opt_error,
};
static const match_table_t tokens = {
@@ -63,7 +64,7 @@ static const match_table_t tokens = {
{Opt_legacy, "noextend"},
{Opt_trans, "trans=%s"},
{Opt_version, "version=%s"},
- {Opt_err, NULL},
+ {Opt_error, NULL},
};
inline int p9_is_proto_dotl(struct p9_client *clnt)
@@ -329,12 +330,15 @@ EXPORT_SYMBOL(p9_tag_lookup);
static void p9_free_req(struct p9_client *c, struct p9_req_t *r)
{
unsigned long flags;
+ struct p9_trans_fd *ts = c->trans;
u16 tag = r->tc->tag;
p9_debug(P9_DEBUG_MUX, "clnt %p req %p tag: %d\n", c, r, tag);
spin_lock_irqsave(&c->lock, flags);
idr_remove(&c->reqs, tag);
spin_unlock_irqrestore(&c->lock, flags);
+ flush_work(&ts->conn.wq);
+ flush_work(&ts->conn.rq);
kfree(r->tc);
kfree(r->rc);
kmem_cache_free(p9_req_cache, r);
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index e2ef3c782c53..63b3c7ef0d90 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -45,122 +45,15 @@
#include <net/9p/9p.h>
#include <net/9p/client.h>
#include <net/9p/transport.h>
+#include <net/9p/trans_fd.h>
#include <linux/syscalls.h> /* killme */
-#define P9_PORT 564
-#define MAX_SOCK_BUF (64*1024)
-#define MAXPOLLWADDR 2
+static void p9_poll_workfn(struct work_struct *work);
static struct p9_trans_module p9_tcp_trans;
static struct p9_trans_module p9_fd_trans;
-/**
- * struct p9_fd_opts - per-transport options
- * @rfd: file descriptor for reading (trans=fd)
- * @wfd: file descriptor for writing (trans=fd)
- * @port: port to connect to (trans=tcp)
- *
- */
-
-struct p9_fd_opts {
- int rfd;
- int wfd;
- u16 port;
- bool privport;
-};
-
-/*
- * Option Parsing (code inspired by NFS code)
- * - a little lazy - parse all fd-transport options
- */
-
-enum {
- /* Options that take integer arguments */
- Opt_port, Opt_rfdno, Opt_wfdno, Opt_err,
- /* Options that take no arguments */
- Opt_privport,
-};
-
-static const match_table_t tokens = {
- {Opt_port, "port=%u"},
- {Opt_rfdno, "rfdno=%u"},
- {Opt_wfdno, "wfdno=%u"},
- {Opt_privport, "privport"},
- {Opt_err, NULL},
-};
-
-enum {
- Rworksched = 1, /* read work scheduled or running */
- Rpending = 2, /* can read */
- Wworksched = 4, /* write work scheduled or running */
- Wpending = 8, /* can write */
-};
-
-struct p9_poll_wait {
- struct p9_conn *conn;
- wait_queue_entry_t wait;
- wait_queue_head_t *wait_addr;
-};
-
-/**
- * struct p9_conn - fd mux connection state information
- * @mux_list: list link for mux to manage multiple connections (?)
- * @client: reference to client instance for this connection
- * @err: error state
- * @req_list: accounting for requests which have been sent
- * @unsent_req_list: accounting for requests that haven't been sent
- * @req: current request being processed (if any)
- * @tmp_buf: temporary buffer to read in header
- * @rc: temporary fcall for reading current frame
- * @wpos: write position for current frame
- * @wsize: amount of data to write for current frame
- * @wbuf: current write buffer
- * @poll_pending_link: pending links to be polled per conn
- * @poll_wait: array of wait_q's for various worker threads
- * @pt: poll state
- * @rq: current read work
- * @wq: current write work
- * @wsched: ????
- *
- */
-
-struct p9_conn {
- struct list_head mux_list;
- struct p9_client *client;
- int err;
- struct list_head req_list;
- struct list_head unsent_req_list;
- struct p9_req_t *req;
- char tmp_buf[7];
- struct p9_fcall rc;
- int wpos;
- int wsize;
- char *wbuf;
- struct list_head poll_pending_link;
- struct p9_poll_wait poll_wait[MAXPOLLWADDR];
- poll_table pt;
- struct work_struct rq;
- struct work_struct wq;
- unsigned long wsched;
-};
-
-/**
- * struct p9_trans_fd - transport state
- * @rd: reference to file to read from
- * @wr: reference of file to write to
- * @conn: connection state reference
- *
- */
-
-struct p9_trans_fd {
- struct file *rd;
- struct file *wr;
- struct p9_conn conn;
-};
-
-static void p9_poll_workfn(struct work_struct *work);
-
static DEFINE_SPINLOCK(p9_poll_lock);
static LIST_HEAD(p9_poll_pending_list);
static DECLARE_WORK(p9_poll_work, p9_poll_workfn);
@@ -765,7 +658,7 @@ static int parse_opts(char *params, struct p9_fd_opts *opts)
int r;
if (!*p)
continue;
- token = match_token(p, tokens, args);
+ token = match_token(p, trans_tokens, args);
if ((token != Opt_err) && (token != Opt_privport)) {
r = match_int(&args[0], &option);
if (r < 0) {
--
2.11.0
^ permalink raw reply related
* (unknown)
From: Sumitomo Rubber @ 2018-07-29 9:58 UTC (permalink / raw)
--
Did you receive our representative email ?
^ permalink raw reply
* Re: [PATCH v2] net: dsa: Remove VLA usage
From: Pavel Machek @ 2018-07-29 11:40 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, netdev, linux-kernel, Salvatore Mesoraca,
Florian Fainelli, Andrew Lunn, David Laight
In-Reply-To: <20180729113806.GC7333@amd>
[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]
Hi!
> > We avoid 2 VLAs by using a pre-allocated field in dsa_switch. We also
> > try to avoid dynamic allocation whenever possible (when using fewer than
> > bits-per-long ports, which is the common case).
> >
> > Link: http://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
> > Link: http://lkml.kernel.org/r/20180505185145.GB32630@lunn.ch
> > Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
> > [kees: tweak commit subject and message slightly]
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > include/net/dsa.h | 3 +++
> > net/dsa/dsa2.c | 14 ++++++++++++++
> > net/dsa/switch.c | 22 ++++++++++------------
> > 3 files changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index fdbd6082945d..461e8a7661b7 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -259,6 +259,9 @@ struct dsa_switch {
> > /* Number of switch port queues */
> > unsigned int num_tx_queues;
> >
> > + unsigned long *bitmap;
> > + unsigned long _bitmap;
>
> This looks rather confusing. _bitmap -> bitmap_data? bitmap_bits?
Actually, AFAICT that long is not used as a long, but as array of
bits. So maybe char bitmap_data[8] would be more appropriate?
(resend, now with right cc list).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v2] net: dsa: Remove VLA usage
From: Pavel Machek @ 2018-07-29 11:38 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, netdev, linux-kernel, Salvatore Mesoraca,
Florian Fainelli, Andrew Lunn, David Laight
In-Reply-To: <20180717041034.GA37382@beast>
[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]
Hi!
> We avoid 2 VLAs by using a pre-allocated field in dsa_switch. We also
> try to avoid dynamic allocation whenever possible (when using fewer than
> bits-per-long ports, which is the common case).
>
> Link: http://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
> Link: http://lkml.kernel.org/r/20180505185145.GB32630@lunn.ch
> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
> [kees: tweak commit subject and message slightly]
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> include/net/dsa.h | 3 +++
> net/dsa/dsa2.c | 14 ++++++++++++++
> net/dsa/switch.c | 22 ++++++++++------------
> 3 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index fdbd6082945d..461e8a7661b7 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -259,6 +259,9 @@ struct dsa_switch {
> /* Number of switch port queues */
> unsigned int num_tx_queues;
>
> + unsigned long *bitmap;
> + unsigned long _bitmap;
This looks rather confusing. _bitmap -> bitmap_data? bitmap_bits?
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: kernel BUG at net/ipv4/ip_output.c:LINE!
From: syzbot @ 2018-07-29 11:12 UTC (permalink / raw)
To: davem, kuznet, linux-kernel, netdev, syzkaller-bugs, yoshfuji
In-Reply-To: <000000000000f68d660570dcddd8@google.com>
syzbot has found a reproducer for the following crash on:
HEAD commit: 71eb5255f55b bpf: use GFP_ATOMIC instead of GFP_KERNEL in ..
git tree: bpf
console output: https://syzkaller.appspot.com/x/log.txt?x=14d07f94400000
kernel config: https://syzkaller.appspot.com/x/.config?x=75c10f3ddcafec09
dashboard link: https://syzkaller.appspot.com/bug?extid=90d5ec0c05e708f3b66d
compiler: gcc (GCC) 8.0.1 20180413 (experimental)
syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=113514b4400000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+90d5ec0c05e708f3b66d@syzkaller.appspotmail.com
hrtimer: interrupt took 252517 ns
------------[ cut here ]------------
kernel BUG at net/ipv4/ip_output.c:775!
invalid opcode: 0000 [#1] SMP KASAN
CPU: 1 PID: 17086 Comm: syz-executor0 Not tainted 4.18.0-rc5+ #22
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
RIP: 0010:ip_do_fragment+0x2436/0x2aa0 net/ipv4/ip_output.c:775
Code: 8b 8d 70 fe ff ff e9 99 e8 ff ff 4c 89 ef e8 a1 8b 40 fc e9 7b e9 ff
ff 4c 89 f7 e8 94 8b 40 fc e9 f3 e5 ff ff e8 ca a9 02 fc <0f> 0b 4c 89 e7
e8 80 8b 40 fc e9 b7 e8 ff ff 4c 89 f7 89 8d 70 fe
RSP: 0018:ffff8801b17f6880 EFLAGS: 00010293
RAX: ffff8801d4688100 RBX: ffff8801a8ead7c0 RCX: ffffffff85794b66
RDX: 0000000000000000 RSI: ffffffff857961b6 RDI: 0000000000000005
RBP: ffff8801b17f6a58 R08: ffff8801d4688100 R09: ffffed0039188032
R10: ffffed0039188034 R11: ffff8801c8c401a3 R12: ffff8801a8ead884
R13: 00000000fffffff2 R14: ffff8801a8ead890 R15: dffffc0000000000
FS: 00007f00d6501700(0000) GS:ffff8801db100000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000072c081 CR3: 00000001cb927000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
ip_fragment.constprop.49+0x179/0x240 net/ipv4/ip_output.c:548
ip_finish_output+0x6e4/0xfa0 net/ipv4/ip_output.c:315
NF_HOOK_COND include/linux/netfilter.h:276 [inline]
ip_output+0x223/0x880 net/ipv4/ip_output.c:405
dst_output include/net/dst.h:444 [inline]
ip_local_out+0xc5/0x1b0 net/ipv4/ip_output.c:124
iptunnel_xmit+0x53b/0x800 net/ipv4/ip_tunnel_core.c:91
ip_tunnel_xmit+0x1598/0x3af1 net/ipv4/ip_tunnel.c:778
__gre_xmit+0x5b7/0x950 net/ipv4/ip_gre.c:449
ipgre_xmit+0x3e8/0xb50 net/ipv4/ip_gre.c:701
__netdev_start_xmit include/linux/netdevice.h:4148 [inline]
netdev_start_xmit include/linux/netdevice.h:4157 [inline]
xmit_one net/core/dev.c:3034 [inline]
dev_hard_start_xmit+0x26c/0xc30 net/core/dev.c:3050
__dev_queue_xmit+0x29c2/0x38e0 net/core/dev.c:3569
dev_queue_xmit+0x17/0x20 net/core/dev.c:3602
__bpf_tx_skb net/core/filter.c:1995 [inline]
__bpf_redirect_common net/core/filter.c:2033 [inline]
__bpf_redirect+0x563/0xa80 net/core/filter.c:2040
____bpf_clone_redirect net/core/filter.c:2073 [inline]
bpf_clone_redirect+0x2f6/0x490 net/core/filter.c:2045
bpf_prog_bebbfe2050753572+0x359/0x1000
Modules linked in:
Dumping ftrace buffer:
(ftrace buffer empty)
---[ end trace ee1a333a6a4f77b3 ]---
RIP: 0010:ip_do_fragment+0x2436/0x2aa0 net/ipv4/ip_output.c:775
Code: 8b 8d 70 fe ff ff e9 99 e8 ff ff 4c 89 ef e8 a1 8b 40 fc e9 7b e9 ff
ff 4c 89 f7 e8 94 8b 40 fc e9 f3 e5 ff ff e8 ca a9 02 fc <0f> 0b 4c 89 e7
e8 80 8b 40 fc e9 b7 e8 ff ff 4c 89 f7 89 8d 70 fe
RSP: 0018:ffff8801b17f6880 EFLAGS: 00010293
RAX: ffff8801d4688100 RBX: ffff8801a8ead7c0 RCX: ffffffff85794b66
RDX: 0000000000000000 RSI: ffffffff857961b6 RDI: 0000000000000005
RBP: ffff8801b17f6a58 R08: ffff8801d4688100 R09: ffffed0039188032
R10: ffffed0039188034 R11: ffff8801c8c401a3 R12: ffff8801a8ead884
R13: 00000000fffffff2 R14: ffff8801a8ead890 R15: dffffc0000000000
FS: 00007f00d6501700(0000) GS:ffff8801db100000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000072c081 CR3: 00000001cb927000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
^ permalink raw reply
* Re: [net-next 10/16] net/mlx5: Support PCIe buffer congestion handling via Devlink
From: Moshe Shemesh @ 2018-07-29 9:23 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Alexander Duyck, Jiri Pirko, Jakub Kicinski, Eran Ben Elisha,
Saeed Mahameed, David S. Miller, netdev@vger.kernel.org,
linux-pci
In-Reply-To: <20180728160608.GI173328@bhelgaas-glaptop.roam.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 5844 bytes --]
On Sat, Jul 28, 2018 at 7:06 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Jul 26, 2018 at 07:00:20AM -0700, Alexander Duyck wrote:
> > On Thu, Jul 26, 2018 at 12:14 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> > > Thu, Jul 26, 2018 at 02:43:59AM CEST, jakub.kicinski@netronome.com
> wrote:
> > >>On Wed, 25 Jul 2018 08:23:26 -0700, Alexander Duyck wrote:
> > >>> On Wed, Jul 25, 2018 at 5:31 AM, Eran Ben Elisha wrote:
> > >>> > On 7/24/2018 10:51 PM, Jakub Kicinski wrote:
> > >>> >>>> The devlink params haven't been upstream even for a full cycle
> and
> > >>> >>>> already you guys are starting to use them to configure standard
> > >>> >>>> features like queuing.
> > >>> >>>
> > >>> >>> We developed the devlink params in order to support non-standard
> > >>> >>> configuration only. And for non-standard, there are generic and
> vendor
> > >>> >>> specific options.
> > >>> >>
> > >>> >> I thought it was developed for performing non-standard and
> possibly
> > >>> >> vendor specific configuration. Look at DEVLINK_PARAM_GENERIC_*
> for
> > >>> >> examples of well justified generic options for which we have no
> > >>> >> other API. The vendor mlx4 options look fairly vendor specific
> if you
> > >>> >> ask me, too.
> > >>> >>
> > >>> >> Configuring queuing has an API. The question is it acceptable to
> enter
> > >>> >> into the risky territory of controlling offloads via devlink
> parameters
> > >>> >> or would we rather make vendors take the time and effort to model
> > >>> >> things to (a subset) of existing APIs. The HW never fits the APIs
> > >>> >> perfectly.
> > >>> >
> > >>> > I understand what you meant here, I would like to highlight that
> this
> > >>> > mechanism was not meant to handle SRIOV, Representors, etc.
> > >>> > The vendor specific configuration suggested here is to handle a
> congestion
> > >>> > state in Multi Host environment (which includes PF and multiple
> VFs per
> > >>> > host), where one host is not aware to the other hosts, and each is
> running
> > >>> > on its own pci/driver. It is a device working mode configuration.
> > >>> >
> > >>> > This couldn't fit into any existing API, thus creating this
> vendor specific
> > >>> > unique API is needed.
> > >>>
> > >>> If we are just going to start creating devlink interfaces in for
> every
> > >>> one-off option a device wants to add why did we even bother with
> > >>> trying to prevent drivers from using sysfs? This just feels like we
> > >>> are back to the same arguments we had back in the day with it.
> > >>>
> > >>> I feel like the bigger question here is if devlink is how we are
> going
> > >>> to deal with all PCIe related features going forward, or should we
> > >>> start looking at creating a new interface/tool for PCI/PCIe related
> > >>> features? My concern is that we have already had features such as DMA
> > >>> Coalescing that didn't really fit into anything and now we are
> > >>> starting to see other things related to DMA and PCIe bus credits. I'm
> > >>> wondering if we shouldn't start looking at a tool/interface to
> > >>> configure all the PCIe related features such as interrupts, error
> > >>> reporting, DMA configuration, power management, etc. Maybe we could
> > >>> even look at sharing it across subsystems and include things like
> > >>> storage, graphics, and other subsystems in the conversation.
> > >>
> > >>Agreed, for actual PCIe configuration (i.e. not ECN marking) we do need
> > >>to build up an API. Sharing it across subsystems would be very cool!
>
> I read the thread (starting at [1], for anybody else coming in late)
> and I see this has something to do with "configuring outbound PCIe
> buffers", but I haven't seen the connection to PCIe protocol or
> features, i.e., I can't connect this to anything in the PCIe spec.
>
> Can somebody help me understand how the PCI core is relevant? If
> there's some connection with a feature defined by PCIe, or if it
> affects the PCIe transaction protocol somehow, I'm definitely
> interested in this. But if this only affects the data transferred
> over PCIe, i.e., the data payloads of PCIe TLP packets, then I'm not
> sure why the PCI core should care.
>
>
As you wrote, this is not a PCIe feature or affects the PCIe transaction
protocol.
Actually, due to hardware limitation in current device, we have enabled a
workaround in hardware.
This mode is proprietary and not relevant to other PCIe devices, thus is
set using driver-specific parameter in devlink
> > I wonder howcome there isn't such API in place already. Or is it?
> > > If it is not, do you have any idea how should it look like? Should it
> be
> > > an extension of the existing PCI uapi or something completely new?
> > > It would be probably good to loop some PCI people in...
> >
> > The closest thing I can think of in terms of answering your questions
> > as to why we haven't seen anything like that would be setpci.
> > Basically with that tool you can go through the PCI configuration
> > space and update any piece you want. The problem is it can have
> > effects on the driver and I don't recall there ever being any sort of
> > notification mechanism added to make a driver aware of configuration
> > updates.
>
> setpci is a development and debugging tool, not something we should
> use as the standard way of configuring things. Use of setpci should
> probably taint the kernel because the PCI core configures features
> like MPS, ASPM, AER, etc., based on the assumption that nobody else is
> changing things in PCI config space.
>
> > As far as the interface I don't know if we would want to use something
> > like netlink or look at something completely new.
> >
> > I've gone ahead and added the linux-pci mailing list to the thread.
> >
> > - Alex
>
> [1] https://lkml.kernel.org/r/20180719010107.22363-11-saeedm@mellanox.com
>
[-- Attachment #2: Type: text/html, Size: 8068 bytes --]
^ permalink raw reply
* [PATCH rdma-next 05/10] IB/ipoib: Move init code to ndo_init
From: Leon Romanovsky @ 2018-07-29 8:34 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Denis Drozdov, Erez Shitrit,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180729083500.5352-1-leon@kernel.org>
From: Jason Gunthorpe <jgg@mellanox.com>
Now that we have a proper ndo_uninit, move code that naturally pairs
with the ndo_uninit into ndo_init. This allows the netdev core to natually
handle ordering.
This fixes the situation where register_netdev can fail before calling
ndo_init, in which case it wouldn't call ndo_uninit either.
Also move a bunch of duplicated init code that is shared between child
and parent for clarity. Now the child and parent register functions look
very similar.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/ulp/ipoib/ipoib.h | 3 -
drivers/infiniband/ulp/ipoib/ipoib_main.c | 193 +++++++++++++++------------
drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 6 -
drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 31 +----
4 files changed, 114 insertions(+), 119 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index 04fc5ad1b69f..02ad1a60dc80 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -508,8 +508,6 @@ void ipoib_ib_dev_down(struct net_device *dev);
int ipoib_ib_dev_stop_default(struct net_device *dev);
void ipoib_pkey_dev_check_presence(struct net_device *dev);
-int ipoib_dev_init(struct net_device *dev, struct ib_device *ca, int port);
-
void ipoib_mcast_join_task(struct work_struct *work);
void ipoib_mcast_carrier_on_task(struct work_struct *work);
void ipoib_mcast_send(struct net_device *dev, u8 *daddr, struct sk_buff *skb);
@@ -597,7 +595,6 @@ void ipoib_pkey_open(struct ipoib_dev_priv *priv);
void ipoib_drain_cq(struct net_device *dev);
void ipoib_set_ethtool_ops(struct net_device *dev);
-void ipoib_set_dev_features(struct ipoib_dev_priv *priv, struct ib_device *hca);
#define IPOIB_FLAGS_RC 0x80
#define IPOIB_FLAGS_UC 0x40
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index d4e9951dc539..67ab52eec3e9 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1741,13 +1741,11 @@ static int ipoib_ioctl(struct net_device *dev, struct ifreq *ifr,
return priv->rn_ops->ndo_do_ioctl(dev, ifr, cmd);
}
-int ipoib_dev_init(struct net_device *dev, struct ib_device *ca, int port)
+static int ipoib_dev_init(struct net_device *dev)
{
struct ipoib_dev_priv *priv = ipoib_priv(dev);
int ret = -ENOMEM;
- priv->ca = ca;
- priv->port = port;
priv->qp = NULL;
/*
@@ -1763,7 +1761,7 @@ int ipoib_dev_init(struct net_device *dev, struct ib_device *ca, int port)
/* create pd, which used both for control and datapath*/
priv->pd = ib_alloc_pd(priv->ca, 0);
if (IS_ERR(priv->pd)) {
- pr_warn("%s: failed to allocate PD\n", ca->name);
+ pr_warn("%s: failed to allocate PD\n", priv->ca->name);
goto clean_wq;
}
@@ -1837,6 +1835,108 @@ static void ipoib_parent_unregister_pre(struct net_device *ndev)
flush_workqueue(ipoib_workqueue);
}
+static void ipoib_set_dev_features(struct ipoib_dev_priv *priv)
+{
+ priv->hca_caps = priv->ca->attrs.device_cap_flags;
+
+ if (priv->hca_caps & IB_DEVICE_UD_IP_CSUM) {
+ priv->dev->hw_features |= NETIF_F_IP_CSUM | NETIF_F_RXCSUM;
+
+ if (priv->hca_caps & IB_DEVICE_UD_TSO)
+ priv->dev->hw_features |= NETIF_F_TSO;
+
+ priv->dev->features |= priv->dev->hw_features;
+ }
+}
+
+static int ipoib_parent_init(struct net_device *ndev)
+{
+ struct ipoib_dev_priv *priv = ipoib_priv(ndev);
+ struct ib_port_attr attr;
+ int result;
+
+ result = ib_query_port(priv->ca, priv->port, &attr);
+ if (result) {
+ pr_warn("%s: ib_query_port %d failed\n", priv->ca->name,
+ priv->port);
+ return result;
+ }
+ priv->max_ib_mtu = ib_mtu_enum_to_int(attr.max_mtu);
+
+ result = ib_query_pkey(priv->ca, priv->port, 0, &priv->pkey);
+ if (result) {
+ pr_warn("%s: ib_query_pkey port %d failed (ret = %d)\n",
+ priv->ca->name, priv->port, result);
+ return result;
+ }
+
+ result = rdma_query_gid(priv->ca, priv->port, 0, &priv->local_gid);
+ if (result) {
+ pr_warn("%s: rdma_query_gid port %d failed (ret = %d)\n",
+ priv->ca->name, priv->port, result);
+ return result;
+ }
+ memcpy(priv->dev->dev_addr + 4, priv->local_gid.raw,
+ sizeof(union ib_gid));
+
+ SET_NETDEV_DEV(priv->dev, priv->ca->dev.parent);
+ priv->dev->dev_id = priv->port - 1;
+
+ return 0;
+}
+
+static void ipoib_child_init(struct net_device *ndev)
+{
+ struct ipoib_dev_priv *priv = ipoib_priv(ndev);
+ struct ipoib_dev_priv *ppriv = ipoib_priv(priv->parent);
+
+ priv->max_ib_mtu = ppriv->max_ib_mtu;
+ set_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags);
+ memcpy(priv->dev->dev_addr, ppriv->dev->dev_addr, INFINIBAND_ALEN);
+ memcpy(&priv->local_gid, &ppriv->local_gid, sizeof(priv->local_gid));
+}
+
+static int ipoib_ndo_init(struct net_device *ndev)
+{
+ struct ipoib_dev_priv *priv = ipoib_priv(ndev);
+ int rc;
+
+ if (priv->parent) {
+ ipoib_child_init(ndev);
+ } else {
+ rc = ipoib_parent_init(ndev);
+ if (rc)
+ return rc;
+ }
+
+ /* MTU will be reset when mcast join happens */
+ ndev->mtu = IPOIB_UD_MTU(priv->max_ib_mtu);
+ priv->mcast_mtu = priv->admin_mtu = ndev->mtu;
+ ndev->max_mtu = IPOIB_CM_MTU;
+
+ ndev->neigh_priv_len = sizeof(struct ipoib_neigh);
+
+ /*
+ * Set the full membership bit, so that we join the right
+ * broadcast group, etc.
+ */
+ priv->pkey |= 0x8000;
+
+ ndev->broadcast[8] = priv->pkey >> 8;
+ ndev->broadcast[9] = priv->pkey & 0xff;
+ set_bit(IPOIB_FLAG_DEV_ADDR_SET, &priv->flags);
+
+ ipoib_set_dev_features(priv);
+
+ rc = ipoib_dev_init(ndev);
+ if (rc) {
+ pr_warn("%s: failed to initialize device: %s port %d (ret = %d)\n",
+ priv->ca->name, priv->dev->name, priv->port, rc);
+ }
+
+ return 0;
+}
+
static void ipoib_ndo_uninit(struct net_device *dev)
{
struct ipoib_dev_priv *priv = ipoib_priv(dev), *cpriv, *tcpriv;
@@ -1909,6 +2009,7 @@ static const struct header_ops ipoib_header_ops = {
};
static const struct net_device_ops ipoib_netdev_ops_pf = {
+ .ndo_init = ipoib_ndo_init,
.ndo_uninit = ipoib_ndo_uninit,
.ndo_open = ipoib_open,
.ndo_stop = ipoib_stop,
@@ -1928,6 +2029,7 @@ static const struct net_device_ops ipoib_netdev_ops_pf = {
};
static const struct net_device_ops ipoib_netdev_ops_vf = {
+ .ndo_init = ipoib_ndo_init,
.ndo_uninit = ipoib_ndo_uninit,
.ndo_open = ipoib_open,
.ndo_stop = ipoib_stop,
@@ -2054,6 +2156,9 @@ struct ipoib_dev_priv *ipoib_intf_alloc(struct ib_device *hca, u8 port,
if (!priv)
return NULL;
+ priv->ca = hca;
+ priv->port = port;
+
dev = ipoib_get_netdev(hca, port, name);
if (!dev)
goto free_priv;
@@ -2201,12 +2306,6 @@ static ssize_t create_child(struct device *dev,
if (pkey <= 0 || pkey > 0xffff || pkey == 0x8000)
return -EINVAL;
- /*
- * Set the full membership bit, so that we join the right
- * broadcast group, etc.
- */
- pkey |= 0x8000;
-
ret = ipoib_vlan_add(to_net_dev(dev), pkey);
return ret ? ret : count;
@@ -2238,86 +2337,17 @@ int ipoib_add_pkey_attr(struct net_device *dev)
return device_create_file(&dev->dev, &dev_attr_pkey);
}
-void ipoib_set_dev_features(struct ipoib_dev_priv *priv, struct ib_device *hca)
-{
- priv->hca_caps = hca->attrs.device_cap_flags;
-
- if (priv->hca_caps & IB_DEVICE_UD_IP_CSUM) {
- priv->dev->hw_features |= NETIF_F_IP_CSUM | NETIF_F_RXCSUM;
-
- if (priv->hca_caps & IB_DEVICE_UD_TSO)
- priv->dev->hw_features |= NETIF_F_TSO;
-
- priv->dev->features |= priv->dev->hw_features;
- }
-}
-
static struct net_device *ipoib_add_port(const char *format,
struct ib_device *hca, u8 port)
{
struct ipoib_dev_priv *priv;
- struct ib_port_attr attr;
struct rdma_netdev *rn;
- int result = -ENOMEM;
+ int result;
priv = ipoib_intf_alloc(hca, port, format);
if (!priv) {
pr_warn("%s, %d: ipoib_intf_alloc failed\n", hca->name, port);
- goto alloc_mem_failed;
- }
-
- SET_NETDEV_DEV(priv->dev, hca->dev.parent);
- priv->dev->dev_id = port - 1;
-
- result = ib_query_port(hca, port, &attr);
- if (result) {
- pr_warn("%s: ib_query_port %d failed\n", hca->name, port);
- goto device_init_failed;
- }
-
- priv->max_ib_mtu = ib_mtu_enum_to_int(attr.max_mtu);
-
- /* MTU will be reset when mcast join happens */
- priv->dev->mtu = IPOIB_UD_MTU(priv->max_ib_mtu);
- priv->mcast_mtu = priv->admin_mtu = priv->dev->mtu;
- priv->dev->max_mtu = IPOIB_CM_MTU;
-
- priv->dev->neigh_priv_len = sizeof(struct ipoib_neigh);
-
- result = ib_query_pkey(hca, port, 0, &priv->pkey);
- if (result) {
- pr_warn("%s: ib_query_pkey port %d failed (ret = %d)\n",
- hca->name, port, result);
- goto device_init_failed;
- }
-
- ipoib_set_dev_features(priv, hca);
-
- /*
- * Set the full membership bit, so that we join the right
- * broadcast group, etc.
- */
- priv->pkey |= 0x8000;
-
- priv->dev->broadcast[8] = priv->pkey >> 8;
- priv->dev->broadcast[9] = priv->pkey & 0xff;
-
- result = rdma_query_gid(hca, port, 0, &priv->local_gid);
- if (result) {
- pr_warn("%s: rdma_query_gid port %d failed (ret = %d)\n",
- hca->name, port, result);
- goto device_init_failed;
- }
-
- memcpy(priv->dev->dev_addr + 4, priv->local_gid.raw,
- sizeof(union ib_gid));
- set_bit(IPOIB_FLAG_DEV_ADDR_SET, &priv->flags);
-
- result = ipoib_dev_init(priv->dev, hca, port);
- if (result) {
- pr_warn("%s: failed to initialize port %d (ret = %d)\n",
- hca->name, port, result);
- goto device_init_failed;
+ return ERR_PTR(-ENOMEM);
}
INIT_IB_EVENT_HANDLER(&priv->event_handler,
@@ -2358,7 +2388,6 @@ static struct net_device *ipoib_add_port(const char *format,
rn->free_rdma_netdev(priv->dev);
kfree(priv);
-alloc_mem_failed:
return ERR_PTR(result);
}
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_netlink.c b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c
index 3e44087935ae..a86928a80c08 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_netlink.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c
@@ -125,12 +125,6 @@ static int ipoib_new_child_link(struct net *src_net, struct net_device *dev,
if (child_pkey == 0 || child_pkey == 0x8000)
return -EINVAL;
- /*
- * Set the full membership bit, so that we join the right
- * broadcast group, etc.
- */
- child_pkey |= 0x8000;
-
err = __ipoib_vlan_add(ppriv, ipoib_priv(dev),
child_pkey, IPOIB_RTNL_CHILD);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index b62ab85c8ead..3103729a73fd 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -55,35 +55,14 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
{
int result;
- priv->max_ib_mtu = ppriv->max_ib_mtu;
- /* MTU will be reset when mcast join happens */
- priv->dev->mtu = IPOIB_UD_MTU(priv->max_ib_mtu);
- priv->mcast_mtu = priv->admin_mtu = priv->dev->mtu;
priv->parent = ppriv->dev;
- set_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags);
-
- ipoib_set_dev_features(priv, ppriv->ca);
-
priv->pkey = pkey;
-
- memcpy(priv->dev->dev_addr, ppriv->dev->dev_addr, INFINIBAND_ALEN);
- memcpy(&priv->local_gid, &ppriv->local_gid, sizeof(priv->local_gid));
- set_bit(IPOIB_FLAG_DEV_ADDR_SET, &priv->flags);
- priv->dev->broadcast[8] = pkey >> 8;
- priv->dev->broadcast[9] = pkey & 0xff;
-
- result = ipoib_dev_init(priv->dev, ppriv->ca, ppriv->port);
- if (result < 0) {
- ipoib_warn(ppriv, "failed to initialize subinterface: "
- "device %s, port %d",
- ppriv->ca->name, ppriv->port);
- goto err;
- }
+ priv->child_type = type;
result = register_netdevice(priv->dev);
if (result) {
ipoib_warn(priv, "failed to initialize; error %i", result);
- goto err;
+ return result;
}
/* RTNL childs don't need proprietary sysfs entries */
@@ -99,17 +78,13 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
goto sysfs_failed;
}
- priv->child_type = type;
list_add_tail(&priv->list, &ppriv->child_intfs);
return 0;
sysfs_failed:
- result = -ENOMEM;
unregister_netdevice(priv->dev);
-
-err:
- return result;
+ return -ENOMEM;
}
int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
--
2.14.4
^ permalink raw reply related
* [PATCH rdma-next 10/10] IB/ipoib: Consolidate checking of the proposed child interface
From: Leon Romanovsky @ 2018-07-29 8:35 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Denis Drozdov, Erez Shitrit,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180729083500.5352-1-leon@kernel.org>
From: Jason Gunthorpe <jgg@mellanox.com>
Move all the checking for pkey and other validity to the __ipoib_vlan_add
function. This removes the last difference from the control flow
of the __ipoib_vlan_add to make the overall design simpler to
understand.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Erez Shitrit <erezsh@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 3 --
drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 77 +++++++++++++++++++---------
2 files changed, 52 insertions(+), 28 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_netlink.c b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c
index 7e093b7aad8f..d4d553a51fa9 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_netlink.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c
@@ -122,9 +122,6 @@ static int ipoib_new_child_link(struct net *src_net, struct net_device *dev,
} else
child_pkey = nla_get_u16(data[IFLA_IPOIB_PKEY]);
- if (child_pkey == 0 || child_pkey == 0x8000)
- return -EINVAL;
-
err = __ipoib_vlan_add(ppriv, ipoib_priv(dev),
child_pkey, IPOIB_RTNL_CHILD);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index ca3a7f6c0998..341753fbda54 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -50,6 +50,39 @@ static ssize_t show_parent(struct device *d, struct device_attribute *attr,
}
static DEVICE_ATTR(parent, S_IRUGO, show_parent, NULL);
+static bool is_child_unique(struct ipoib_dev_priv *ppriv,
+ struct ipoib_dev_priv *priv)
+{
+ struct ipoib_dev_priv *tpriv;
+
+ ASSERT_RTNL();
+
+ /*
+ * Since the legacy sysfs interface uses pkey for deletion it cannot
+ * support more than one interface with the same pkey, it creates
+ * ambiguity. The RTNL interface deletes using the netdev so it does
+ * not have a problem to support duplicated pkeys.
+ */
+ if (priv->child_type != IPOIB_LEGACY_CHILD)
+ return true;
+
+ /*
+ * First ensure this isn't a duplicate. We check the parent device and
+ * then all of the legacy child interfaces to make sure the Pkey
+ * doesn't match.
+ */
+ if (ppriv->pkey == priv->pkey)
+ return false;
+
+ list_for_each_entry(tpriv, &ppriv->child_intfs, list) {
+ if (tpriv->pkey == priv->pkey &&
+ tpriv->child_type == IPOIB_LEGACY_CHILD)
+ return false;
+ }
+
+ return true;
+}
+
/*
* NOTE: If this function fails then the priv->dev will remain valid, however
* priv can have been freed and must not be touched by caller in the error
@@ -73,10 +106,20 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
*/
WARN_ON(ppriv->dev->reg_state != NETREG_REGISTERED);
+ if (pkey == 0 || pkey == 0x8000) {
+ result = -EINVAL;
+ goto out_early;
+ }
+
priv->parent = ppriv->dev;
priv->pkey = pkey;
priv->child_type = type;
+ if (!is_child_unique(ppriv, priv)) {
+ result = -ENOTUNIQ;
+ goto out_early;
+ }
+
/* We do not need to touch priv if register_netdevice fails */
ndev->priv_destructor = ipoib_intf_free;
@@ -88,9 +131,7 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
* register_netdevice sometimes calls priv_destructor,
* sometimes not. Make sure it was done.
*/
- if (ndev->priv_destructor)
- ndev->priv_destructor(ndev);
- return result;
+ goto out_early;
}
/* RTNL childs don't need proprietary sysfs entries */
@@ -111,6 +152,11 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
sysfs_failed:
unregister_netdevice(priv->dev);
return -ENOMEM;
+
+out_early:
+ if (ndev->priv_destructor)
+ ndev->priv_destructor(ndev);
+ return result;
}
int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
@@ -118,17 +164,11 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
struct ipoib_dev_priv *ppriv, *priv;
char intf_name[IFNAMSIZ];
struct net_device *ndev;
- struct ipoib_dev_priv *tpriv;
int result;
if (!capable(CAP_NET_ADMIN))
return -EPERM;
- ppriv = ipoib_priv(pdev);
-
- snprintf(intf_name, sizeof(intf_name), "%s.%04x",
- ppriv->dev->name, pkey);
-
if (!rtnl_trylock())
return restart_syscall();
@@ -137,23 +177,10 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
return -EPERM;
}
- /*
- * First ensure this isn't a duplicate. We check the parent device and
- * then all of the legacy child interfaces to make sure the Pkey
- * doesn't match.
- */
- if (ppriv->pkey == pkey) {
- result = -ENOTUNIQ;
- goto out;
- }
+ ppriv = ipoib_priv(pdev);
- list_for_each_entry(tpriv, &ppriv->child_intfs, list) {
- if (tpriv->pkey == pkey &&
- tpriv->child_type == IPOIB_LEGACY_CHILD) {
- result = -ENOTUNIQ;
- goto out;
- }
- }
+ snprintf(intf_name, sizeof(intf_name), "%s.%04x",
+ ppriv->dev->name, pkey);
priv = ipoib_intf_alloc(ppriv->ca, ppriv->port, intf_name);
if (!priv) {
--
2.14.4
^ permalink raw reply related
* [PATCH rdma-next 09/10] IB/ipoib: Maintain the child_intfs list from ndo_init/uninit
From: Leon Romanovsky @ 2018-07-29 8:34 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Denis Drozdov, Erez Shitrit,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180729083500.5352-1-leon@kernel.org>
From: Jason Gunthorpe <jgg@mellanox.com>
This fixes a bug in the netlink path where the vlan_rwsem was not
held around __ipoib_vlan_add causing the child_intfs to be manipulated
unsafely.
In the process this greatly simplifies the vlan_rwsem write side locking
to only cover a single non-sleeping statement.
This also further increases the safety of the removal ordering by holding
the netdev of the parent while the child is active to ensure most bugs
become either an oops on a NULL priv or a deadlock on the netdev refcount.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 16 ++++++++++++++++
drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 14 --------------
drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 12 ------------
3 files changed, 16 insertions(+), 26 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index b2fe23d60103..e3d28f9ad9c0 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1890,6 +1890,12 @@ static void ipoib_child_init(struct net_device *ndev)
struct ipoib_dev_priv *priv = ipoib_priv(ndev);
struct ipoib_dev_priv *ppriv = ipoib_priv(priv->parent);
+ dev_hold(priv->parent);
+
+ down_write(&ppriv->vlan_rwsem);
+ list_add_tail(&priv->list, &ppriv->child_intfs);
+ up_write(&ppriv->vlan_rwsem);
+
priv->max_ib_mtu = ppriv->max_ib_mtu;
set_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags);
memcpy(priv->dev->dev_addr, ppriv->dev->dev_addr, INFINIBAND_ALEN);
@@ -1959,6 +1965,16 @@ static void ipoib_ndo_uninit(struct net_device *dev)
destroy_workqueue(priv->wq);
priv->wq = NULL;
}
+
+ if (priv->parent) {
+ struct ipoib_dev_priv *ppriv = ipoib_priv(priv->parent);
+
+ down_write(&ppriv->vlan_rwsem);
+ list_del(&priv->list);
+ up_write(&ppriv->vlan_rwsem);
+
+ dev_put(priv->parent);
+ }
}
static int ipoib_set_vf_link_state(struct net_device *dev, int vf, int link_state)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_netlink.c b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c
index a86928a80c08..7e093b7aad8f 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_netlink.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_netlink.c
@@ -133,19 +133,6 @@ static int ipoib_new_child_link(struct net *src_net, struct net_device *dev,
return err;
}
-static void ipoib_unregister_child_dev(struct net_device *dev, struct list_head *head)
-{
- struct ipoib_dev_priv *priv, *ppriv;
-
- priv = ipoib_priv(dev);
- ppriv = ipoib_priv(priv->parent);
-
- down_write(&ppriv->vlan_rwsem);
- unregister_netdevice_queue(dev, head);
- list_del(&priv->list);
- up_write(&ppriv->vlan_rwsem);
-}
-
static size_t ipoib_get_size(const struct net_device *dev)
{
return nla_total_size(2) + /* IFLA_IPOIB_PKEY */
@@ -161,7 +148,6 @@ static struct rtnl_link_ops ipoib_link_ops __read_mostly = {
.setup = ipoib_setup_common,
.newlink = ipoib_new_child_link,
.changelink = ipoib_changelink,
- .dellink = ipoib_unregister_child_dev,
.get_size = ipoib_get_size,
.fill_info = ipoib_fill_info,
};
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index fa4dfcee2644..ca3a7f6c0998 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -106,8 +106,6 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
goto sysfs_failed;
}
- list_add_tail(&priv->list, &ppriv->child_intfs);
-
return 0;
sysfs_failed:
@@ -139,11 +137,6 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
return -EPERM;
}
- if (!down_write_trylock(&ppriv->vlan_rwsem)) {
- rtnl_unlock();
- return restart_syscall();
- }
-
/*
* First ensure this isn't a duplicate. We check the parent device and
* then all of the legacy child interfaces to make sure the Pkey
@@ -175,7 +168,6 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
free_netdev(ndev);
out:
- up_write(&ppriv->vlan_rwsem);
rtnl_unlock();
return result;
@@ -209,10 +201,6 @@ static void ipoib_vlan_delete_task(struct work_struct *work)
struct ipoib_dev_priv *priv = ipoib_priv(dev);
struct ipoib_dev_priv *ppriv = ipoib_priv(priv->parent);
- down_write(&ppriv->vlan_rwsem);
- list_del(&priv->list);
- up_write(&ppriv->vlan_rwsem);
-
ipoib_dbg(ppriv, "delete child vlan %s\n", dev->name);
unregister_netdevice(dev);
}
--
2.14.4
^ permalink raw reply related
* [PATCH rdma-next 08/10] IB/ipoib: Do not remove child devices from within the ndo_uninit
From: Leon Romanovsky @ 2018-07-29 8:34 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Denis Drozdov, Erez Shitrit,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180729083500.5352-1-leon@kernel.org>
From: Jason Gunthorpe <jgg@mellanox.com>
Switching to priv_destructor and needs_free_netdev created a subtle
ordering problem in ipoib_remove_one.
Now that unregister_netdev frees the netdev and priv we must ensure that
the children are unregistered before trying to unregister the parent,
or child unregister will use after free.
The solution is to unregister the children, then parent, in the same batch
all while holding the rtnl_lock. This closes all the races where a new
child could have been added and ensures proper ordering.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/ulp/ipoib/ipoib.h | 7 +++++++
drivers/infiniband/ulp/ipoib/ipoib_main.c | 28 +++++++++++++++++-----------
drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 6 ++++++
3 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index 804cb4bee57d..1abe3c62f106 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -330,6 +330,13 @@ struct ipoib_dev_priv {
unsigned long flags;
+ /*
+ * This protects access to the child_intfs list.
+ * To READ from child_intfs the RTNL or vlan_rwsem read side must be
+ * held. To WRITE RTNL and the vlan_rwsem write side must be held (in
+ * that order) This lock exists because we have a few contexts where
+ * we need the child_intfs, but do not want to grab the RTNL.
+ */
struct rw_semaphore vlan_rwsem;
struct mutex mcast_mutex;
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index e9f4f261fe20..b2fe23d60103 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1939,18 +1939,15 @@ static int ipoib_ndo_init(struct net_device *ndev)
static void ipoib_ndo_uninit(struct net_device *dev)
{
- struct ipoib_dev_priv *priv = ipoib_priv(dev), *cpriv, *tcpriv;
- LIST_HEAD(head);
+ struct ipoib_dev_priv *priv = ipoib_priv(dev);
ASSERT_RTNL();
- /* Delete any child interfaces first */
- list_for_each_entry_safe(cpriv, tcpriv, &priv->child_intfs, list) {
- /* Stop GC on child */
- cancel_delayed_work_sync(&cpriv->neigh_reap_task);
- unregister_netdevice_queue(cpriv->dev, &head);
- }
- unregister_netdevice_many(&head);
+ /*
+ * ipoib_remove_one guarantees the children are removed before the
+ * parent, and that is the only place where a parent can be removed.
+ */
+ WARN_ON(!list_empty(&priv->child_intfs));
ipoib_neigh_hash_uninit(dev);
@@ -2466,16 +2463,25 @@ static void ipoib_add_one(struct ib_device *device)
static void ipoib_remove_one(struct ib_device *device, void *client_data)
{
- struct ipoib_dev_priv *priv, *tmp;
+ struct ipoib_dev_priv *priv, *tmp, *cpriv, *tcpriv;
struct list_head *dev_list = client_data;
if (!dev_list)
return;
list_for_each_entry_safe(priv, tmp, dev_list, list) {
+ LIST_HEAD(head);
ipoib_parent_unregister_pre(priv->dev);
- unregister_netdev(priv->dev);
+ rtnl_lock();
+
+ list_for_each_entry_safe(cpriv, tcpriv, &priv->child_intfs,
+ list)
+ unregister_netdevice_queue(cpriv->dev, &head);
+ unregister_netdevice_queue(priv->dev, &head);
+ unregister_netdevice_many(&head);
+
+ rtnl_unlock();
}
kfree(dev_list);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index 891c5b40018a..fa4dfcee2644 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -67,6 +67,12 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
ASSERT_RTNL();
+ /*
+ * Racing with unregister of the parent must be prevented by the
+ * caller.
+ */
+ WARN_ON(ppriv->dev->reg_state != NETREG_REGISTERED);
+
priv->parent = ppriv->dev;
priv->pkey = pkey;
priv->child_type = type;
--
2.14.4
^ permalink raw reply related
* [PATCH rdma-next 07/10] IB/ipoib: Get rid of the sysfs_mutex
From: Leon Romanovsky @ 2018-07-29 8:34 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Denis Drozdov, Erez Shitrit,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180729083500.5352-1-leon@kernel.org>
From: Jason Gunthorpe <jgg@mellanox.com>
This mutex was introduced to deal with the deadlock formed by calling
unregister_netdev from within the sysfs callback of a netdev.
Now that we have priv_destructor and needs_free_netdev we can switch
to the more targeted solution of running the unregister from a
work queue. This avoids the deadlock and gets rid of the mutex.
The next patch in the series needs this mutex eliminated to create
atomicity of unregisteration.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/ulp/ipoib/ipoib.h | 1 -
drivers/infiniband/ulp/ipoib/ipoib_cm.c | 7 ---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 7 +--
drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 98 ++++++++++++++++++++-----------
4 files changed, 65 insertions(+), 48 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index d2cb0a8500e3..804cb4bee57d 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -332,7 +332,6 @@ struct ipoib_dev_priv {
struct rw_semaphore vlan_rwsem;
struct mutex mcast_mutex;
- struct mutex sysfs_mutex;
struct rb_root path_tree;
struct list_head path_list;
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index 83fa402e5d03..04785d3f8195 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -1514,19 +1514,13 @@ static ssize_t set_mode(struct device *d, struct device_attribute *attr,
{
struct net_device *dev = to_net_dev(d);
int ret;
- struct ipoib_dev_priv *priv = ipoib_priv(dev);
-
- if (!mutex_trylock(&priv->sysfs_mutex))
- return restart_syscall();
if (!rtnl_trylock()) {
- mutex_unlock(&priv->sysfs_mutex);
return restart_syscall();
}
if (dev->reg_state != NETREG_REGISTERED) {
rtnl_unlock();
- mutex_unlock(&priv->sysfs_mutex);
return -EPERM;
}
@@ -1538,7 +1532,6 @@ static ssize_t set_mode(struct device *d, struct device_attribute *attr,
*/
if (ret != -EBUSY)
rtnl_unlock();
- mutex_unlock(&priv->sysfs_mutex);
return (!ret || ret == -EBUSY) ? count : ret;
}
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 73d917d57f93..e9f4f261fe20 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2079,7 +2079,6 @@ static void ipoib_build_priv(struct net_device *dev)
spin_lock_init(&priv->lock);
init_rwsem(&priv->vlan_rwsem);
mutex_init(&priv->mcast_mutex);
- mutex_init(&priv->sysfs_mutex);
INIT_LIST_HEAD(&priv->path_list);
INIT_LIST_HEAD(&priv->child_intfs);
@@ -2476,10 +2475,7 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
list_for_each_entry_safe(priv, tmp, dev_list, list) {
ipoib_parent_unregister_pre(priv->dev);
- /* Wrap rtnl_lock/unlock with mutex to protect sysfs calls */
- mutex_lock(&priv->sysfs_mutex);
unregister_netdev(priv->dev);
- mutex_unlock(&priv->sysfs_mutex);
}
kfree(dev_list);
@@ -2527,8 +2523,7 @@ static int __init ipoib_init_module(void)
* its private workqueue, and we only queue up flush events
* on our global flush workqueue. This avoids the deadlocks.
*/
- ipoib_workqueue = alloc_ordered_workqueue("ipoib_flush",
- WQ_MEM_RECLAIM);
+ ipoib_workqueue = alloc_ordered_workqueue("ipoib_flush", 0);
if (!ipoib_workqueue) {
ret = -ENOMEM;
goto err_fs;
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index 7776334cf8c5..891c5b40018a 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -125,23 +125,16 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
snprintf(intf_name, sizeof(intf_name), "%s.%04x",
ppriv->dev->name, pkey);
- if (!mutex_trylock(&ppriv->sysfs_mutex))
+ if (!rtnl_trylock())
return restart_syscall();
- if (!rtnl_trylock()) {
- mutex_unlock(&ppriv->sysfs_mutex);
- return restart_syscall();
- }
-
if (pdev->reg_state != NETREG_REGISTERED) {
rtnl_unlock();
- mutex_unlock(&ppriv->sysfs_mutex);
return -EPERM;
}
if (!down_write_trylock(&ppriv->vlan_rwsem)) {
rtnl_unlock();
- mutex_unlock(&ppriv->sysfs_mutex);
return restart_syscall();
}
@@ -178,58 +171,95 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
out:
up_write(&ppriv->vlan_rwsem);
rtnl_unlock();
- mutex_unlock(&ppriv->sysfs_mutex);
return result;
}
+struct ipoib_vlan_delete_work {
+ struct work_struct work;
+ struct net_device *dev;
+};
+
+/*
+ * sysfs callbacks of a netdevice cannot obtain the rtnl lock as
+ * unregister_netdev ultimately deletes the sysfs files while holding the rtnl
+ * lock. This deadlocks the system.
+ *
+ * A callback can use rtnl_trylock to avoid the deadlock but it cannot call
+ * unregister_netdev as that internally takes and releases the rtnl_lock. So
+ * instead we find the netdev to unregister and then do the actual unregister
+ * from the global work queue where we can obtain the rtnl_lock safely.
+ */
+static void ipoib_vlan_delete_task(struct work_struct *work)
+{
+ struct ipoib_vlan_delete_work *pwork =
+ container_of(work, struct ipoib_vlan_delete_work, work);
+ struct net_device *dev = pwork->dev;
+
+ rtnl_lock();
+
+ /* Unregistering tasks can race with another task or parent removal */
+ if (dev->reg_state == NETREG_REGISTERED) {
+ struct ipoib_dev_priv *priv = ipoib_priv(dev);
+ struct ipoib_dev_priv *ppriv = ipoib_priv(priv->parent);
+
+ down_write(&ppriv->vlan_rwsem);
+ list_del(&priv->list);
+ up_write(&ppriv->vlan_rwsem);
+
+ ipoib_dbg(ppriv, "delete child vlan %s\n", dev->name);
+ unregister_netdevice(dev);
+ }
+
+ rtnl_unlock();
+
+ kfree(pwork);
+}
+
int ipoib_vlan_delete(struct net_device *pdev, unsigned short pkey)
{
struct ipoib_dev_priv *ppriv, *priv, *tpriv;
- struct net_device *dev = NULL;
+ int rc;
if (!capable(CAP_NET_ADMIN))
return -EPERM;
- ppriv = ipoib_priv(pdev);
-
- if (!mutex_trylock(&ppriv->sysfs_mutex))
+ if (!rtnl_trylock())
return restart_syscall();
- if (!rtnl_trylock()) {
- mutex_unlock(&ppriv->sysfs_mutex);
- return restart_syscall();
- }
-
if (pdev->reg_state != NETREG_REGISTERED) {
rtnl_unlock();
- mutex_unlock(&ppriv->sysfs_mutex);
return -EPERM;
}
- if (!down_write_trylock(&ppriv->vlan_rwsem)) {
- rtnl_unlock();
- mutex_unlock(&ppriv->sysfs_mutex);
- return restart_syscall();
- }
+ ppriv = ipoib_priv(pdev);
+ rc = -ENODEV;
list_for_each_entry_safe(priv, tpriv, &ppriv->child_intfs, list) {
if (priv->pkey == pkey &&
priv->child_type == IPOIB_LEGACY_CHILD) {
- list_del(&priv->list);
- dev = priv->dev;
+ struct ipoib_vlan_delete_work *work;
+
+ work = kmalloc(sizeof(*work), GFP_KERNEL);
+ if (!work) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ down_write(&ppriv->vlan_rwsem);
+ list_del_init(&priv->list);
+ up_write(&ppriv->vlan_rwsem);
+ work->dev = priv->dev;
+ INIT_WORK(&work->work, ipoib_vlan_delete_task);
+ queue_work(ipoib_workqueue, &work->work);
+
+ rc = 0;
break;
}
}
- up_write(&ppriv->vlan_rwsem);
-
- if (dev) {
- ipoib_dbg(ppriv, "delete child vlan %s\n", dev->name);
- unregister_netdevice(dev);
- }
+out:
rtnl_unlock();
- mutex_unlock(&ppriv->sysfs_mutex);
- return (dev) ? 0 : -ENODEV;
+ return rc;
}
--
2.14.4
^ permalink raw reply related
* [PATCH rdma-next 06/10] RDMA/netdev: Use priv_destructor for netdev cleanup
From: Leon Romanovsky @ 2018-07-29 8:34 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Denis Drozdov, Erez Shitrit,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180729083500.5352-1-leon@kernel.org>
From: Jason Gunthorpe <jgg@mellanox.com>
Now that the unregister_netdev flow for IPoIB no longer relies on external
code we can now introduce the use of priv_destructor and
needs_free_netdev.
The rdma_netdev flow is switched to use the netdev common priv_destructor
instead of the special free_rdma_netdev and the IPOIB ULP adjusted:
- priv_destructor needs to switch to point to the ULP's destructor
which will then call the rdma_ndev's in the right order
- We need to be careful around the error unwind of register_netdev
as it sometimes calls priv_destructor on failure
- ULPs need to use ndo_init/uninit to ensure proper ordering
of failures around register_netdev
Switching to priv_destructor is a necessary pre-requisite to using
the rtnl new_link mechanism.
The VNIC user for rdma_netdev should also be revised, but that is left for
another patch.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Denis Drozdov <denisd@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/hw/mlx5/main.c | 10 --
drivers/infiniband/ulp/ipoib/ipoib.h | 2 +
drivers/infiniband/ulp/ipoib/ipoib_main.c | 101 +++++++++++++--------
drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 68 ++++++++------
.../net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c | 37 ++++----
include/linux/mlx5/driver.h | 3 -
include/rdma/ib_verbs.h | 6 +-
7 files changed, 129 insertions(+), 98 deletions(-)
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index a93ef6367d38..9011187ca081 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -5157,11 +5157,6 @@ static int mlx5_ib_get_hw_stats(struct ib_device *ibdev,
return num_counters;
}
-static void mlx5_ib_free_rdma_netdev(struct net_device *netdev)
-{
- return mlx5_rdma_netdev_free(netdev);
-}
-
static struct net_device*
mlx5_ib_alloc_rdma_netdev(struct ib_device *hca,
u8 port_num,
@@ -5171,17 +5166,12 @@ mlx5_ib_alloc_rdma_netdev(struct ib_device *hca,
void (*setup)(struct net_device *))
{
struct net_device *netdev;
- struct rdma_netdev *rn;
if (type != RDMA_NETDEV_IPOIB)
return ERR_PTR(-EOPNOTSUPP);
netdev = mlx5_rdma_netdev_alloc(to_mdev(hca)->mdev, hca,
name, setup);
- if (likely(!IS_ERR_OR_NULL(netdev))) {
- rn = netdev_priv(netdev);
- rn->free_rdma_netdev = mlx5_ib_free_rdma_netdev;
- }
return netdev;
}
diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index 02ad1a60dc80..d2cb0a8500e3 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -323,6 +323,7 @@ struct ipoib_dev_priv {
spinlock_t lock;
struct net_device *dev;
+ void (*next_priv_destructor)(struct net_device *dev);
struct napi_struct send_napi;
struct napi_struct recv_napi;
@@ -481,6 +482,7 @@ static inline void ipoib_put_ah(struct ipoib_ah *ah)
kref_put(&ah->ref, ipoib_free_ah);
}
int ipoib_open(struct net_device *dev);
+void ipoib_intf_free(struct net_device *dev);
int ipoib_add_pkey_attr(struct net_device *dev);
int ipoib_add_umcast_attr(struct net_device *dev);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 67ab52eec3e9..73d917d57f93 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2062,6 +2062,13 @@ void ipoib_setup_common(struct net_device *dev)
netif_keep_dst(dev);
memcpy(dev->broadcast, ipv4_bcast_addr, INFINIBAND_ALEN);
+
+ /*
+ * unregister_netdev always frees the netdev, we use this mode
+ * consistently to unify all the various unregister paths, including
+ * those connected to rtnl_link_ops which require it.
+ */
+ dev->needs_free_netdev = true;
}
static void ipoib_build_priv(struct net_device *dev)
@@ -2116,9 +2123,7 @@ static struct net_device
rn->send = ipoib_send;
rn->attach_mcast = ipoib_mcast_attach;
rn->detach_mcast = ipoib_mcast_detach;
- rn->free_rdma_netdev = free_netdev;
rn->hca = hca;
-
dev->netdev_ops = &ipoib_netdev_default_pf;
return dev;
@@ -2173,6 +2178,15 @@ struct ipoib_dev_priv *ipoib_intf_alloc(struct ib_device *hca, u8 port,
rn = netdev_priv(dev);
rn->clnt_priv = priv;
+
+ /*
+ * Only the child register_netdev flows can handle priv_destructor
+ * being set, so we force it to NULL here and handle manually until it
+ * is safe to turn on.
+ */
+ priv->next_priv_destructor = dev->priv_destructor;
+ dev->priv_destructor = NULL;
+
ipoib_build_priv(dev);
return priv;
@@ -2181,6 +2195,27 @@ struct ipoib_dev_priv *ipoib_intf_alloc(struct ib_device *hca, u8 port,
return NULL;
}
+void ipoib_intf_free(struct net_device *dev)
+{
+ struct ipoib_dev_priv *priv = ipoib_priv(dev);
+ struct rdma_netdev *rn = netdev_priv(dev);
+
+ dev->priv_destructor = priv->next_priv_destructor;
+ if (dev->priv_destructor)
+ dev->priv_destructor(dev);
+
+ /*
+ * There are some error flows around register_netdev failing that may
+ * attempt to call priv_destructor twice, prevent that from happening.
+ */
+ dev->priv_destructor = NULL;
+
+ /* unregister/destroy is very complicated. Make bugs more obvious. */
+ rn->clnt_priv = NULL;
+
+ kfree(priv);
+}
+
static ssize_t show_pkey(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -2341,7 +2376,7 @@ static struct net_device *ipoib_add_port(const char *format,
struct ib_device *hca, u8 port)
{
struct ipoib_dev_priv *priv;
- struct rdma_netdev *rn;
+ struct net_device *ndev;
int result;
priv = ipoib_intf_alloc(hca, port, format);
@@ -2349,6 +2384,7 @@ static struct net_device *ipoib_add_port(const char *format,
pr_warn("%s, %d: ipoib_intf_alloc failed\n", hca->name, port);
return ERR_PTR(-ENOMEM);
}
+ ndev = priv->dev;
INIT_IB_EVENT_HANDLER(&priv->event_handler,
priv->ca, ipoib_event);
@@ -2357,38 +2393,43 @@ static struct net_device *ipoib_add_port(const char *format,
/* call event handler to ensure pkey in sync */
queue_work(ipoib_workqueue, &priv->flush_heavy);
- result = register_netdev(priv->dev);
+ result = register_netdev(ndev);
if (result) {
pr_warn("%s: couldn't register ipoib port %d; error %d\n",
hca->name, port, result);
- ipoib_parent_unregister_pre(priv->dev);
- goto device_init_failed;
+
+ ipoib_parent_unregister_pre(ndev);
+ ipoib_intf_free(ndev);
+ free_netdev(ndev);
+
+ return ERR_PTR(result);
}
- result = -ENOMEM;
- if (ipoib_cm_add_mode_attr(priv->dev))
+ /*
+ * We cannot set priv_destructor before register_netdev because we
+ * need priv to be always valid during the error flow to execute
+ * ipoib_parent_unregister_pre(). Instead handle it manually and only
+ * enter priv_destructor mode once we are completely registered.
+ */
+ ndev->priv_destructor = ipoib_intf_free;
+
+ if (ipoib_cm_add_mode_attr(ndev))
goto sysfs_failed;
- if (ipoib_add_pkey_attr(priv->dev))
+ if (ipoib_add_pkey_attr(ndev))
goto sysfs_failed;
- if (ipoib_add_umcast_attr(priv->dev))
+ if (ipoib_add_umcast_attr(ndev))
goto sysfs_failed;
- if (device_create_file(&priv->dev->dev, &dev_attr_create_child))
+ if (device_create_file(&ndev->dev, &dev_attr_create_child))
goto sysfs_failed;
- if (device_create_file(&priv->dev->dev, &dev_attr_delete_child))
+ if (device_create_file(&ndev->dev, &dev_attr_delete_child))
goto sysfs_failed;
- return priv->dev;
+ return ndev;
sysfs_failed:
- ipoib_parent_unregister_pre(priv->dev);
- unregister_netdev(priv->dev);
-
-device_init_failed:
- rn = netdev_priv(priv->dev);
- rn->free_rdma_netdev(priv->dev);
- kfree(priv);
-
- return ERR_PTR(result);
+ ipoib_parent_unregister_pre(ndev);
+ unregister_netdev(ndev);
+ return ERR_PTR(-ENOMEM);
}
static void ipoib_add_one(struct ib_device *device)
@@ -2426,33 +2467,19 @@ static void ipoib_add_one(struct ib_device *device)
static void ipoib_remove_one(struct ib_device *device, void *client_data)
{
- struct ipoib_dev_priv *priv, *tmp, *cpriv, *tcpriv;
+ struct ipoib_dev_priv *priv, *tmp;
struct list_head *dev_list = client_data;
if (!dev_list)
return;
list_for_each_entry_safe(priv, tmp, dev_list, list) {
- struct rdma_netdev *parent_rn = netdev_priv(priv->dev);
-
ipoib_parent_unregister_pre(priv->dev);
/* Wrap rtnl_lock/unlock with mutex to protect sysfs calls */
mutex_lock(&priv->sysfs_mutex);
unregister_netdev(priv->dev);
mutex_unlock(&priv->sysfs_mutex);
-
- parent_rn->free_rdma_netdev(priv->dev);
-
- list_for_each_entry_safe(cpriv, tcpriv, &priv->child_intfs, list) {
- struct rdma_netdev *child_rn;
-
- child_rn = netdev_priv(cpriv->dev);
- child_rn->free_rdma_netdev(cpriv->dev);
- kfree(cpriv);
- }
-
- kfree(priv);
}
kfree(dev_list);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index 3103729a73fd..7776334cf8c5 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -50,31 +50,53 @@ static ssize_t show_parent(struct device *d, struct device_attribute *attr,
}
static DEVICE_ATTR(parent, S_IRUGO, show_parent, NULL);
+/*
+ * NOTE: If this function fails then the priv->dev will remain valid, however
+ * priv can have been freed and must not be touched by caller in the error
+ * case.
+ *
+ * If (ndev->reg_state == NETREG_UNINITIALIZED) then it is up to the caller to
+ * free the net_device (just as rtnl_newlink does) otherwise the net_device
+ * will be freed when the rtnl is unlocked.
+ */
int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
u16 pkey, int type)
{
+ struct net_device *ndev = priv->dev;
int result;
+ ASSERT_RTNL();
+
priv->parent = ppriv->dev;
priv->pkey = pkey;
priv->child_type = type;
- result = register_netdevice(priv->dev);
+ /* We do not need to touch priv if register_netdevice fails */
+ ndev->priv_destructor = ipoib_intf_free;
+
+ result = register_netdevice(ndev);
if (result) {
ipoib_warn(priv, "failed to initialize; error %i", result);
+
+ /*
+ * register_netdevice sometimes calls priv_destructor,
+ * sometimes not. Make sure it was done.
+ */
+ if (ndev->priv_destructor)
+ ndev->priv_destructor(ndev);
return result;
}
/* RTNL childs don't need proprietary sysfs entries */
if (type == IPOIB_LEGACY_CHILD) {
- if (ipoib_cm_add_mode_attr(priv->dev))
+ if (ipoib_cm_add_mode_attr(ndev))
goto sysfs_failed;
- if (ipoib_add_pkey_attr(priv->dev))
+ if (ipoib_add_pkey_attr(ndev))
goto sysfs_failed;
- if (ipoib_add_umcast_attr(priv->dev))
+ if (ipoib_add_umcast_attr(ndev))
goto sysfs_failed;
- if (device_create_file(&priv->dev->dev, &dev_attr_parent))
+ if (device_create_file(&ndev->dev, &dev_attr_parent))
goto sysfs_failed;
}
@@ -91,6 +113,7 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
{
struct ipoib_dev_priv *ppriv, *priv;
char intf_name[IFNAMSIZ];
+ struct net_device *ndev;
struct ipoib_dev_priv *tpriv;
int result;
@@ -122,12 +145,6 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
return restart_syscall();
}
- priv = ipoib_intf_alloc(ppriv->ca, ppriv->port, intf_name);
- if (!priv) {
- result = -ENOMEM;
- goto out;
- }
-
/*
* First ensure this isn't a duplicate. We check the parent device and
* then all of the legacy child interfaces to make sure the Pkey
@@ -146,21 +163,23 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
}
}
+ priv = ipoib_intf_alloc(ppriv->ca, ppriv->port, intf_name);
+ if (!priv) {
+ result = -ENOMEM;
+ goto out;
+ }
+ ndev = priv->dev;
+
result = __ipoib_vlan_add(ppriv, priv, pkey, IPOIB_LEGACY_CHILD);
+ if (result && ndev->reg_state == NETREG_UNINITIALIZED)
+ free_netdev(ndev);
+
out:
up_write(&ppriv->vlan_rwsem);
rtnl_unlock();
mutex_unlock(&ppriv->sysfs_mutex);
- if (result && priv) {
- struct rdma_netdev *rn;
-
- rn = netdev_priv(priv->dev);
- rn->free_rdma_netdev(priv->dev);
- kfree(priv);
- }
-
return result;
}
@@ -212,14 +231,5 @@ int ipoib_vlan_delete(struct net_device *pdev, unsigned short pkey)
rtnl_unlock();
mutex_unlock(&ppriv->sysfs_mutex);
- if (dev) {
- struct rdma_netdev *rn;
-
- rn = netdev_priv(dev);
- rn->free_rdma_netdev(priv->dev);
- kfree(priv);
- return 0;
- }
-
- return -ENODEV;
+ return (dev) ? 0 : -ENODEV;
}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c
index af3bb2f7a504..b8d150d2fd72 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c
@@ -580,6 +580,22 @@ static int mlx5i_check_required_hca_cap(struct mlx5_core_dev *mdev)
return 0;
}
+static void mlx5_rdma_netdev_free(struct net_device *netdev)
+{
+ struct mlx5e_priv *priv = mlx5i_epriv(netdev);
+ struct mlx5i_priv *ipriv = priv->ppriv;
+ const struct mlx5e_profile *profile = priv->profile;
+
+ mlx5e_detach_netdev(priv);
+ profile->cleanup(priv);
+ destroy_workqueue(priv->wq);
+
+ if (!ipriv->sub_interface) {
+ mlx5i_pkey_qpn_ht_cleanup(netdev);
+ mlx5e_destroy_mdev_resources(priv->mdev);
+ }
+}
+
struct net_device *mlx5_rdma_netdev_alloc(struct mlx5_core_dev *mdev,
struct ib_device *ibdev,
const char *name,
@@ -653,6 +669,9 @@ struct net_device *mlx5_rdma_netdev_alloc(struct mlx5_core_dev *mdev,
rn->detach_mcast = mlx5i_detach_mcast;
rn->set_id = mlx5i_set_pkey_index;
+ netdev->priv_destructor = mlx5_rdma_netdev_free;
+ netdev->needs_free_netdev = 1;
+
return netdev;
destroy_ht:
@@ -665,21 +684,3 @@ struct net_device *mlx5_rdma_netdev_alloc(struct mlx5_core_dev *mdev,
return NULL;
}
EXPORT_SYMBOL(mlx5_rdma_netdev_alloc);
-
-void mlx5_rdma_netdev_free(struct net_device *netdev)
-{
- struct mlx5e_priv *priv = mlx5i_epriv(netdev);
- struct mlx5i_priv *ipriv = priv->ppriv;
- const struct mlx5e_profile *profile = priv->profile;
-
- mlx5e_detach_netdev(priv);
- profile->cleanup(priv);
- destroy_workqueue(priv->wq);
-
- if (!ipriv->sub_interface) {
- mlx5i_pkey_qpn_ht_cleanup(netdev);
- mlx5e_destroy_mdev_resources(priv->mdev);
- }
- free_netdev(netdev);
-}
-EXPORT_SYMBOL(mlx5_rdma_netdev_free);
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 0c1beb14837b..789489e3aa76 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -1218,14 +1218,11 @@ struct net_device *mlx5_rdma_netdev_alloc(struct mlx5_core_dev *mdev,
{
return ERR_PTR(-EOPNOTSUPP);
}
-
-static inline void mlx5_rdma_netdev_free(struct net_device *netdev) {}
#else
struct net_device *mlx5_rdma_netdev_alloc(struct mlx5_core_dev *mdev,
struct ib_device *ibdev,
const char *name,
void (*setup)(struct net_device *));
-void mlx5_rdma_netdev_free(struct net_device *netdev);
#endif /* CONFIG_MLX5_CORE_IPOIB */
struct mlx5_profile {
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 42cbf8eabe9d..73c780c99482 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2202,7 +2202,11 @@ struct rdma_netdev {
struct ib_device *hca;
u8 port_num;
- /* cleanup function must be specified */
+ /*
+ * cleanup function must be specified.
+ * FIXME: This is only used for OPA_VNIC and that usage should be
+ * removed too.
+ */
void (*free_rdma_netdev)(struct net_device *netdev);
/* control functions */
^ permalink raw reply related
* [PATCH rdma-next 04/10] IB/ipoib: Make ipoib_neigh_hash_uninit fully fence its work
From: Leon Romanovsky @ 2018-07-29 8:34 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Denis Drozdov, Erez Shitrit,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180729083500.5352-1-leon@kernel.org>
From: Erez Shitrit <erezsh@mellanox.com>
The neigh_reap_task is self restarting, but so long as we call
cancel_delayed_work_sync() it will be guaranteed to not be running and
never start again. Thus there is no longer any purpose to
IPOIB_STOP_NEIGH_GC (which was racy/buggy anyhow), so remove it too.
This fixes a situation where the GC work could have been running
Signed-off-by: Erez Shitrit <erezsh@mellanox.com>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/ulp/ipoib/ipoib.h | 1 -
drivers/infiniband/ulp/ipoib/ipoib_main.c | 25 +++++++------------------
2 files changed, 7 insertions(+), 19 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index 48e9ea50ca87..04fc5ad1b69f 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -91,7 +91,6 @@ enum {
IPOIB_STOP_REAPER = 7,
IPOIB_FLAG_ADMIN_CM = 9,
IPOIB_FLAG_UMCAST = 10,
- IPOIB_STOP_NEIGH_GC = 11,
IPOIB_NEIGH_TBL_FLUSH = 12,
IPOIB_FLAG_DEV_ADDR_SET = 13,
IPOIB_FLAG_DEV_ADDR_CTRL = 14,
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 4eec2781e83f..d4e9951dc539 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1305,9 +1305,6 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
int i;
LIST_HEAD(remove_list);
- if (test_bit(IPOIB_STOP_NEIGH_GC, &priv->flags))
- return;
-
spin_lock_irqsave(&priv->lock, flags);
htbl = rcu_dereference_protected(ntbl->htbl,
@@ -1319,9 +1316,6 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
/* neigh is obsolete if it was idle for two GC periods */
dt = 2 * arp_tbl.gc_interval;
neigh_obsolete = jiffies - dt;
- /* handle possible race condition */
- if (test_bit(IPOIB_STOP_NEIGH_GC, &priv->flags))
- goto out_unlock;
for (i = 0; i < htbl->size; i++) {
struct ipoib_neigh *neigh;
@@ -1359,9 +1353,8 @@ static void ipoib_reap_neigh(struct work_struct *work)
__ipoib_reap_neigh(priv);
- if (!test_bit(IPOIB_STOP_NEIGH_GC, &priv->flags))
- queue_delayed_work(priv->wq, &priv->neigh_reap_task,
- arp_tbl.gc_interval);
+ queue_delayed_work(priv->wq, &priv->neigh_reap_task,
+ arp_tbl.gc_interval);
}
@@ -1523,7 +1516,6 @@ static int ipoib_neigh_hash_init(struct ipoib_dev_priv *priv)
htbl = kzalloc(sizeof(*htbl), GFP_KERNEL);
if (!htbl)
return -ENOMEM;
- set_bit(IPOIB_STOP_NEIGH_GC, &priv->flags);
size = roundup_pow_of_two(arp_tbl.gc_thresh3);
buckets = kvcalloc(size, sizeof(*buckets), GFP_KERNEL);
if (!buckets) {
@@ -1538,7 +1530,6 @@ static int ipoib_neigh_hash_init(struct ipoib_dev_priv *priv)
atomic_set(&ntbl->entries, 0);
/* start garbage collection */
- clear_bit(IPOIB_STOP_NEIGH_GC, &priv->flags);
queue_delayed_work(priv->wq, &priv->neigh_reap_task,
arp_tbl.gc_interval);
@@ -1648,15 +1639,11 @@ static void ipoib_flush_neighs(struct ipoib_dev_priv *priv)
static void ipoib_neigh_hash_uninit(struct net_device *dev)
{
struct ipoib_dev_priv *priv = ipoib_priv(dev);
- int stopped;
ipoib_dbg(priv, "ipoib_neigh_hash_uninit\n");
init_completion(&priv->ntbl.deleted);
- /* Stop GC if called at init fail need to cancel work */
- stopped = test_and_set_bit(IPOIB_STOP_NEIGH_GC, &priv->flags);
- if (!stopped)
- cancel_delayed_work_sync(&priv->neigh_reap_task);
+ cancel_delayed_work_sync(&priv->neigh_reap_task);
ipoib_flush_neighs(priv);
@@ -1796,12 +1783,15 @@ int ipoib_dev_init(struct net_device *dev, struct ib_device *ca, int port)
if (ipoib_ib_dev_open(dev)) {
pr_warn("%s failed to open device\n", dev->name);
ret = -ENODEV;
- goto out_dev_uninit;
+ goto out_hash_uninit;
}
}
return 0;
+out_hash_uninit:
+ ipoib_neigh_hash_uninit(dev);
+
out_dev_uninit:
ipoib_ib_dev_cleanup(dev);
@@ -1857,7 +1847,6 @@ static void ipoib_ndo_uninit(struct net_device *dev)
/* Delete any child interfaces first */
list_for_each_entry_safe(cpriv, tcpriv, &priv->child_intfs, list) {
/* Stop GC on child */
- set_bit(IPOIB_STOP_NEIGH_GC, &cpriv->flags);
cancel_delayed_work_sync(&cpriv->neigh_reap_task);
unregister_netdevice_queue(cpriv->dev, &head);
}
--
2.14.4
^ permalink raw reply related
* [PATCH rdma-next 03/10] IB/ipoib: Move all uninit code into ndo_uninit
From: Leon Romanovsky @ 2018-07-29 8:34 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Denis Drozdov, Erez Shitrit,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180729083500.5352-1-leon@kernel.org>
From: Jason Gunthorpe <jgg@mellanox.com>
Currently uninit is sometimes done twice in error flows, and is sprinkled
a bit all over the place.
Improve the clarity of the design by moving all uninit only into
ndo_uinit.
Some duplication is removed:
- Sometimes IPOIB_STOP_NEIGH_GC was done before unregister, but
this duplicates the process in ipoib_neigh_hash_init
- Flushing priv->wq was sometimes done before unregister,
but that duplicates what has been done in ndo_uninit
Uniniting the IB event queue must remain before unregister_netdev as it
requires the RTNL lock to be dropped, this is moved to a helper to make
that flow really clear and remove some duplication in error flows.
If register_netdev fails (and ndo_init is NULL) then it almost always
calls ndo_uninit, which lets us remove all the extra code from the error
unwinds. The next patch in the series will close the 'almost always' hole
by pairing a proper ndo_init with ndo_uninit.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/ulp/ipoib/ipoib.h | 1 -
drivers/infiniband/ulp/ipoib/ipoib_main.c | 62 ++++++++++++++++---------------
drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 5 +--
3 files changed, 34 insertions(+), 34 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index 9eebb705d994..48e9ea50ca87 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -510,7 +510,6 @@ int ipoib_ib_dev_stop_default(struct net_device *dev);
void ipoib_pkey_dev_check_presence(struct net_device *dev);
int ipoib_dev_init(struct net_device *dev, struct ib_device *ca, int port);
-void ipoib_dev_cleanup(struct net_device *dev);
void ipoib_mcast_join_task(struct work_struct *work);
void ipoib_mcast_carrier_on_task(struct work_struct *work);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 7cd42619deb2..4eec2781e83f 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -215,11 +215,6 @@ static int ipoib_stop(struct net_device *dev)
return 0;
}
-static void ipoib_uninit(struct net_device *dev)
-{
- ipoib_dev_cleanup(dev);
-}
-
static netdev_features_t ipoib_fix_features(struct net_device *dev, netdev_features_t features)
{
struct ipoib_dev_priv *priv = ipoib_priv(dev);
@@ -1826,7 +1821,33 @@ int ipoib_dev_init(struct net_device *dev, struct ib_device *ca, int port)
return ret;
}
-void ipoib_dev_cleanup(struct net_device *dev)
+/*
+ * This must be called before doing an unregister_netdev on a parent device to
+ * shutdown the IB event handler.
+ */
+static void ipoib_parent_unregister_pre(struct net_device *ndev)
+{
+ struct ipoib_dev_priv *priv = ipoib_priv(ndev);
+
+ /*
+ * ipoib_set_mac checks netif_running before pushing work, clearing
+ * running ensures the it will not add more work.
+ */
+ rtnl_lock();
+ dev_change_flags(priv->dev, priv->dev->flags & ~IFF_UP);
+ rtnl_unlock();
+
+ /* ipoib_event() cannot be running once this returns */
+ ib_unregister_event_handler(&priv->event_handler);
+
+ /*
+ * Work on the queue grabs the rtnl lock, so this cannot be done while
+ * also holding it.
+ */
+ flush_workqueue(ipoib_workqueue);
+}
+
+static void ipoib_ndo_uninit(struct net_device *dev)
{
struct ipoib_dev_priv *priv = ipoib_priv(dev), *cpriv, *tcpriv;
LIST_HEAD(head);
@@ -1899,7 +1920,7 @@ static const struct header_ops ipoib_header_ops = {
};
static const struct net_device_ops ipoib_netdev_ops_pf = {
- .ndo_uninit = ipoib_uninit,
+ .ndo_uninit = ipoib_ndo_uninit,
.ndo_open = ipoib_open,
.ndo_stop = ipoib_stop,
.ndo_change_mtu = ipoib_change_mtu,
@@ -1918,7 +1939,7 @@ static const struct net_device_ops ipoib_netdev_ops_pf = {
};
static const struct net_device_ops ipoib_netdev_ops_vf = {
- .ndo_uninit = ipoib_uninit,
+ .ndo_uninit = ipoib_ndo_uninit,
.ndo_open = ipoib_open,
.ndo_stop = ipoib_stop,
.ndo_change_mtu = ipoib_change_mtu,
@@ -2321,7 +2342,8 @@ static struct net_device *ipoib_add_port(const char *format,
if (result) {
pr_warn("%s: couldn't register ipoib port %d; error %d\n",
hca->name, port, result);
- goto register_failed;
+ ipoib_parent_unregister_pre(priv->dev);
+ goto device_init_failed;
}
result = -ENOMEM;
@@ -2339,17 +2361,9 @@ static struct net_device *ipoib_add_port(const char *format,
return priv->dev;
sysfs_failed:
+ ipoib_parent_unregister_pre(priv->dev);
unregister_netdev(priv->dev);
-register_failed:
- ib_unregister_event_handler(&priv->event_handler);
- flush_workqueue(ipoib_workqueue);
- /* Stop GC if started before flush */
- set_bit(IPOIB_STOP_NEIGH_GC, &priv->flags);
- cancel_delayed_work_sync(&priv->neigh_reap_task);
- flush_workqueue(priv->wq);
- ipoib_dev_cleanup(priv->dev);
-
device_init_failed:
rn = netdev_priv(priv->dev);
rn->free_rdma_netdev(priv->dev);
@@ -2403,17 +2417,7 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
list_for_each_entry_safe(priv, tmp, dev_list, list) {
struct rdma_netdev *parent_rn = netdev_priv(priv->dev);
- ib_unregister_event_handler(&priv->event_handler);
- flush_workqueue(ipoib_workqueue);
-
- rtnl_lock();
- dev_change_flags(priv->dev, priv->dev->flags & ~IFF_UP);
- rtnl_unlock();
-
- /* Stop GC */
- set_bit(IPOIB_STOP_NEIGH_GC, &priv->flags);
- cancel_delayed_work_sync(&priv->neigh_reap_task);
- flush_workqueue(priv->wq);
+ ipoib_parent_unregister_pre(priv->dev);
/* Wrap rtnl_lock/unlock with mutex to protect sysfs calls */
mutex_lock(&priv->sysfs_mutex);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index 1b7bfd500893..b62ab85c8ead 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -83,7 +83,7 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
result = register_netdevice(priv->dev);
if (result) {
ipoib_warn(priv, "failed to initialize; error %i", result);
- goto register_failed;
+ goto err;
}
/* RTNL childs don't need proprietary sysfs entries */
@@ -108,9 +108,6 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
result = -ENOMEM;
unregister_netdevice(priv->dev);
-register_failed:
- ipoib_dev_cleanup(priv->dev);
-
err:
return result;
}
--
2.14.4
^ permalink raw reply related
* [PATCH rdma-next 02/10] IB/ipoib: Use cancel_delayed_work_sync for neigh-clean task
From: Leon Romanovsky @ 2018-07-29 8:34 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Denis Drozdov, Erez Shitrit,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180729083500.5352-1-leon@kernel.org>
From: Erez Shitrit <erezsh@mellanox.com>
The neigh_reap_task is self restarting, but so long as we call
cancel_delayed_work_sync() it will be guaranteed to not be running and
never start again. Thus we don't need to have the racy
IPOIB_STOP_NEIGH_GC bit, or the confusing mismatch of places sometimes
calling flush_workqueue after the cancel.
This fixes a situation where the GC work could have been left running
in some rare situations.
Signed-off-by: Erez Shitrit <erezsh@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 7ca9013bf05c..7cd42619deb2 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1661,7 +1661,7 @@ static void ipoib_neigh_hash_uninit(struct net_device *dev)
/* Stop GC if called at init fail need to cancel work */
stopped = test_and_set_bit(IPOIB_STOP_NEIGH_GC, &priv->flags);
if (!stopped)
- cancel_delayed_work(&priv->neigh_reap_task);
+ cancel_delayed_work_sync(&priv->neigh_reap_task);
ipoib_flush_neighs(priv);
@@ -1837,7 +1837,7 @@ void ipoib_dev_cleanup(struct net_device *dev)
list_for_each_entry_safe(cpriv, tcpriv, &priv->child_intfs, list) {
/* Stop GC on child */
set_bit(IPOIB_STOP_NEIGH_GC, &cpriv->flags);
- cancel_delayed_work(&cpriv->neigh_reap_task);
+ cancel_delayed_work_sync(&cpriv->neigh_reap_task);
unregister_netdevice_queue(cpriv->dev, &head);
}
unregister_netdevice_many(&head);
@@ -2346,7 +2346,7 @@ static struct net_device *ipoib_add_port(const char *format,
flush_workqueue(ipoib_workqueue);
/* Stop GC if started before flush */
set_bit(IPOIB_STOP_NEIGH_GC, &priv->flags);
- cancel_delayed_work(&priv->neigh_reap_task);
+ cancel_delayed_work_sync(&priv->neigh_reap_task);
flush_workqueue(priv->wq);
ipoib_dev_cleanup(priv->dev);
@@ -2412,7 +2412,7 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
/* Stop GC */
set_bit(IPOIB_STOP_NEIGH_GC, &priv->flags);
- cancel_delayed_work(&priv->neigh_reap_task);
+ cancel_delayed_work_sync(&priv->neigh_reap_task);
flush_workqueue(priv->wq);
/* Wrap rtnl_lock/unlock with mutex to protect sysfs calls */
--
2.14.4
^ permalink raw reply related
* [PATCH rdma-next 01/10] IB/ipoib: Get rid of IPOIB_FLAG_GOING_DOWN
From: Leon Romanovsky @ 2018-07-29 8:34 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Denis Drozdov, Erez Shitrit,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180729083500.5352-1-leon@kernel.org>
From: Jason Gunthorpe <jgg@mellanox.com>
This essentially duplicates the netdev's reg_state, so just use that
directly. The reg_state is updated under the rntl_lock, and all places
using GOING_DOWN already acquire the rtnl_lock so checking is safe.
Since the only place we use GOING_DOWN is for the parent device this
does not fix any bugs, but it is a step to tidy up the unregister flow
so that after later patches the flow is uniform and sane.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/ulp/ipoib/ipoib.h | 1 -
drivers/infiniband/ulp/ipoib/ipoib_cm.c | 9 ++++++---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 3 ---
drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 18 ++++++++++++------
4 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index e255a7e5a4c3..9eebb705d994 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -95,7 +95,6 @@ enum {
IPOIB_NEIGH_TBL_FLUSH = 12,
IPOIB_FLAG_DEV_ADDR_SET = 13,
IPOIB_FLAG_DEV_ADDR_CTRL = 14,
- IPOIB_FLAG_GOING_DOWN = 15,
IPOIB_MAX_BACKOFF_SECONDS = 16,
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index 8b44f33c7ae0..83fa402e5d03 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -1516,9 +1516,6 @@ static ssize_t set_mode(struct device *d, struct device_attribute *attr,
int ret;
struct ipoib_dev_priv *priv = ipoib_priv(dev);
- if (test_bit(IPOIB_FLAG_GOING_DOWN, &priv->flags))
- return -EPERM;
-
if (!mutex_trylock(&priv->sysfs_mutex))
return restart_syscall();
@@ -1527,6 +1524,12 @@ static ssize_t set_mode(struct device *d, struct device_attribute *attr,
return restart_syscall();
}
+ if (dev->reg_state != NETREG_REGISTERED) {
+ rtnl_unlock();
+ mutex_unlock(&priv->sysfs_mutex);
+ return -EPERM;
+ }
+
ret = ipoib_set_mode(dev, buf);
/* The assumption is that the function ipoib_set_mode returned
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 82f0e3869b04..7ca9013bf05c 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2406,9 +2406,6 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
ib_unregister_event_handler(&priv->event_handler);
flush_workqueue(ipoib_workqueue);
- /* mark interface in the middle of destruction */
- set_bit(IPOIB_FLAG_GOING_DOWN, &priv->flags);
-
rtnl_lock();
dev_change_flags(priv->dev, priv->dev->flags & ~IFF_UP);
rtnl_unlock();
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index b067ad5e4c7e..1b7bfd500893 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -127,9 +127,6 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
ppriv = ipoib_priv(pdev);
- if (test_bit(IPOIB_FLAG_GOING_DOWN, &ppriv->flags))
- return -EPERM;
-
snprintf(intf_name, sizeof(intf_name), "%s.%04x",
ppriv->dev->name, pkey);
@@ -141,6 +138,12 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
return restart_syscall();
}
+ if (pdev->reg_state != NETREG_REGISTERED) {
+ rtnl_unlock();
+ mutex_unlock(&ppriv->sysfs_mutex);
+ return -EPERM;
+ }
+
if (!down_write_trylock(&ppriv->vlan_rwsem)) {
rtnl_unlock();
mutex_unlock(&ppriv->sysfs_mutex);
@@ -199,9 +202,6 @@ int ipoib_vlan_delete(struct net_device *pdev, unsigned short pkey)
ppriv = ipoib_priv(pdev);
- if (test_bit(IPOIB_FLAG_GOING_DOWN, &ppriv->flags))
- return -EPERM;
-
if (!mutex_trylock(&ppriv->sysfs_mutex))
return restart_syscall();
@@ -210,6 +210,12 @@ int ipoib_vlan_delete(struct net_device *pdev, unsigned short pkey)
return restart_syscall();
}
+ if (pdev->reg_state != NETREG_REGISTERED) {
+ rtnl_unlock();
+ mutex_unlock(&ppriv->sysfs_mutex);
+ return -EPERM;
+ }
+
if (!down_write_trylock(&ppriv->vlan_rwsem)) {
rtnl_unlock();
mutex_unlock(&ppriv->sysfs_mutex);
--
2.14.4
^ permalink raw reply related
* [PATCH rdma-next 00/10] IPoIB uninit
From: Leon Romanovsky @ 2018-07-29 8:34 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Denis Drozdov, Erez Shitrit,
Saeed Mahameed, linux-netdev
From: Leon Romanovsky <leonro@mellanox.com>
IP link was broken due to the changes in IPoIB for the rdma_netdev
support after commit cd565b4b51e5 ("IB/IPoIB: Support acceleration options callbacks").
This patchset restores IPoIB pkey creation and removal using rtnetlink.
It is completely rewritten variant of
https://marc.info/?l=linux-rdma&m=151553425830918&w=2 patch series.
Thanks
Erez Shitrit (2):
IB/ipoib: Use cancel_delayed_work_sync for neigh-clean task
IB/ipoib: Make ipoib_neigh_hash_uninit fully fence its work
Jason Gunthorpe (8):
IB/ipoib: Get rid of IPOIB_FLAG_GOING_DOWN
IB/ipoib: Move all uninit code into ndo_uninit
IB/ipoib: Move init code to ndo_init
RDMA/netdev: Use priv_destructor for netdev cleanup
IB/ipoib: Get rid of the sysfs_mutex
IB/ipoib: Do not remove child devices from within the ndo_uninit
IB/ipoib: Maintain the child_intfs list from ndo_init/uninit
IB/ipoib: Consolidate checking of the proposed child interface
drivers/infiniband/hw/mlx5/main.c | 10 -
drivers/infiniband/ulp/ipoib/ipoib.h | 16 +-
drivers/infiniband/ulp/ipoib/ipoib_cm.c | 14 +-
drivers/infiniband/ulp/ipoib/ipoib_main.c | 419 ++++++++++++---------
drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 23 --
drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 259 +++++++------
.../net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c | 37 +-
include/linux/mlx5/driver.h | 3 -
include/rdma/ib_verbs.h | 6 +-
9 files changed, 428 insertions(+), 359 deletions(-)
^ permalink raw reply
* Re: [patch net-next] net: sched: don't dump chains only held by actions
From: Jiri Pirko @ 2018-07-29 7:51 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
Jakub Kicinski, mlxsw
In-Reply-To: <CAM_iQpV2LXPD-8PD=gK_xsuSv7nvQKZbg-miQdUDaQCcx3XjNw@mail.gmail.com>
Sat, Jul 28, 2018 at 07:39:36PM CEST, xiyou.wangcong@gmail.com wrote:
>On Sat, Jul 28, 2018 at 10:20 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>
>> On Fri, Jul 27, 2018 at 12:47 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >
>> > From: Jiri Pirko <jiri@mellanox.com>
>> >
>> > In case a chain is empty and not explicitly created by a user,
>> > such chain should not exist. The only exception is if there is
>> > an action "goto chain" pointing to it. In that case, don't show the
>> > chain in the dump. Track the chain references held by actions and
>> > use them to find out if a chain should or should not be shown
>> > in chain dump.
>> >
>> > Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>
>> Looks reasonable to me.
>>
>> Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
>
>Hold on...
>
>If you increase the refcnt for a zombie chain on NEWCHAIN path,
>then it would become a non-zombie, this makes sense. However,
>if the action_refcnt gets increased again when another action uses it,
>it become a zombie again because refcnt==action_refcnt??
No. action always increases both refcnt and action_refcnt
^ permalink raw reply
* Re: [PATCH v5 bpf-next 6/9] bpf: Make redirect_info accessible from modules
From: kbuild test robot @ 2018-07-29 7:06 UTC (permalink / raw)
To: Toshiaki Makita
Cc: kbuild-all, netdev, Alexei Starovoitov, Daniel Borkmann,
Toshiaki Makita, Jesper Dangaard Brouer, Jakub Kicinski
In-Reply-To: <20180726144032.2116-7-toshiaki.makita1@gmail.com>
Hi Toshiaki,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
url: https://github.com/0day-ci/linux/commits/Toshiaki-Makita/net-Export-skb_headers_offset_update/20180729-094722
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
net/core/filter.c:116:48: sparse: expression using sizeof(void)
net/core/filter.c:116:48: sparse: expression using sizeof(void)
net/core/filter.c:210:32: sparse: cast to restricted __be16
net/core/filter.c:210:32: sparse: cast to restricted __be16
net/core/filter.c:210:32: sparse: cast to restricted __be16
net/core/filter.c:210:32: sparse: cast to restricted __be16
net/core/filter.c:210:32: sparse: cast to restricted __be16
net/core/filter.c:210:32: sparse: cast to restricted __be16
net/core/filter.c:210:32: sparse: cast to restricted __be16
net/core/filter.c:210:32: sparse: cast to restricted __be16
net/core/filter.c:237:32: sparse: cast to restricted __be32
net/core/filter.c:237:32: sparse: cast to restricted __be32
net/core/filter.c:237:32: sparse: cast to restricted __be32
net/core/filter.c:237:32: sparse: cast to restricted __be32
net/core/filter.c:237:32: sparse: cast to restricted __be32
net/core/filter.c:237:32: sparse: cast to restricted __be32
net/core/filter.c:237:32: sparse: cast to restricted __be32
net/core/filter.c:237:32: sparse: cast to restricted __be32
net/core/filter.c:237:32: sparse: cast to restricted __be32
net/core/filter.c:237:32: sparse: cast to restricted __be32
net/core/filter.c:237:32: sparse: cast to restricted __be32
net/core/filter.c:237:32: sparse: cast to restricted __be32
net/core/filter.c:410:33: sparse: subtraction of functions? Share your drugs
net/core/filter.c:413:33: sparse: subtraction of functions? Share your drugs
net/core/filter.c:416:33: sparse: subtraction of functions? Share your drugs
net/core/filter.c:419:33: sparse: subtraction of functions? Share your drugs
net/core/filter.c:422:33: sparse: subtraction of functions? Share your drugs
net/core/filter.c:495:27: sparse: subtraction of functions? Share your drugs
net/core/filter.c:498:27: sparse: subtraction of functions? Share your drugs
net/core/filter.c:501:27: sparse: subtraction of functions? Share your drugs
include/linux/slab.h:631:13: sparse: undefined identifier '__builtin_mul_overflow'
include/linux/slab.h:631:13: sparse: not a function <noident>
include/linux/filter.h:640:16: sparse: expression using sizeof(void)
include/linux/filter.h:640:16: sparse: expression using sizeof(void)
include/linux/filter.h:640:16: sparse: expression using sizeof(void)
include/linux/filter.h:640:16: sparse: expression using sizeof(void)
net/core/filter.c:1389:39: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct sock_filter const *filter @@ got struct sockstruct sock_filter const *filter @@
net/core/filter.c:1389:39: expected struct sock_filter const *filter
net/core/filter.c:1389:39: got struct sock_filter [noderef] <asn:1>*filter
include/linux/filter.h:640:16: sparse: expression using sizeof(void)
include/linux/filter.h:640:16: sparse: expression using sizeof(void)
net/core/filter.c:1491:39: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct sock_filter const *filter @@ got struct sockstruct sock_filter const *filter @@
net/core/filter.c:1491:39: expected struct sock_filter const *filter
net/core/filter.c:1491:39: got struct sock_filter [noderef] <asn:1>*filter
include/linux/filter.h:640:16: sparse: expression using sizeof(void)
net/core/filter.c:1824:43: sparse: incorrect type in argument 2 (different base types) @@ expected restricted __wsum [usertype] diff @@ got unsigned lonrestricted __wsum [usertype] diff @@
net/core/filter.c:1824:43: expected restricted __wsum [usertype] diff
net/core/filter.c:1824:43: got unsigned long long [unsigned] [usertype] to
net/core/filter.c:1827:36: sparse: incorrect type in argument 2 (different base types) @@ expected restricted __be16 [usertype] old @@ got unsigned lonrestricted __be16 [usertype] old @@
net/core/filter.c:1827:36: expected restricted __be16 [usertype] old
net/core/filter.c:1827:36: got unsigned long long [unsigned] [usertype] from
net/core/filter.c:1827:42: sparse: incorrect type in argument 3 (different base types) @@ expected restricted __be16 [usertype] new @@ got unsigned lonrestricted __be16 [usertype] new @@
net/core/filter.c:1827:42: expected restricted __be16 [usertype] new
net/core/filter.c:1827:42: got unsigned long long [unsigned] [usertype] to
net/core/filter.c:1830:36: sparse: incorrect type in argument 2 (different base types) @@ expected restricted __be32 [usertype] from @@ got unsigned lonrestricted __be32 [usertype] from @@
net/core/filter.c:1830:36: expected restricted __be32 [usertype] from
net/core/filter.c:1830:36: got unsigned long long [unsigned] [usertype] from
net/core/filter.c:1830:42: sparse: incorrect type in argument 3 (different base types) @@ expected restricted __be32 [usertype] to @@ got unsigned lonrestricted __be32 [usertype] to @@
net/core/filter.c:1830:42: expected restricted __be32 [usertype] to
net/core/filter.c:1830:42: got unsigned long long [unsigned] [usertype] to
net/core/filter.c:1875:59: sparse: incorrect type in argument 3 (different base types) @@ expected restricted __wsum [usertype] diff @@ got unsigned lonrestricted __wsum [usertype] diff @@
net/core/filter.c:1875:59: expected restricted __wsum [usertype] diff
net/core/filter.c:1875:59: got unsigned long long [unsigned] [usertype] to
net/core/filter.c:1878:52: sparse: incorrect type in argument 3 (different base types) @@ expected restricted __be16 [usertype] from @@ got unsigned lonrestricted __be16 [usertype] from @@
net/core/filter.c:1878:52: expected restricted __be16 [usertype] from
net/core/filter.c:1878:52: got unsigned long long [unsigned] [usertype] from
net/core/filter.c:1878:58: sparse: incorrect type in argument 4 (different base types) @@ expected restricted __be16 [usertype] to @@ got unsigned lonrestricted __be16 [usertype] to @@
net/core/filter.c:1878:58: expected restricted __be16 [usertype] to
net/core/filter.c:1878:58: got unsigned long long [unsigned] [usertype] to
net/core/filter.c:1881:52: sparse: incorrect type in argument 3 (different base types) @@ expected restricted __be32 [usertype] from @@ got unsigned lonrestricted __be32 [usertype] from @@
net/core/filter.c:1881:52: expected restricted __be32 [usertype] from
net/core/filter.c:1881:52: got unsigned long long [unsigned] [usertype] from
net/core/filter.c:1881:58: sparse: incorrect type in argument 4 (different base types) @@ expected restricted __be32 [usertype] to @@ got unsigned lonrestricted __be32 [usertype] to @@
net/core/filter.c:1881:58: expected restricted __be32 [usertype] to
net/core/filter.c:1881:58: got unsigned long long [unsigned] [usertype] to
net/core/filter.c:1927:28: sparse: incorrect type in return expression (different base types) @@ expected unsigned long long @@ got nsigned long long @@
net/core/filter.c:1927:28: expected unsigned long long
net/core/filter.c:1927:28: got restricted __wsum
net/core/filter.c:1949:35: sparse: incorrect type in return expression (different base types) @@ expected unsigned long long @@ got restricted unsigned long long @@
net/core/filter.c:1949:35: expected unsigned long long
net/core/filter.c:1949:35: got restricted __wsum [usertype] csum
>> net/core/filter.c:2088:40: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] <asn:3>*__vpp_verify @@ got d const [noderef] <asn:3>*__vpp_verify @@
net/core/filter.c:2088:40: expected void const [noderef] <asn:3>*__vpp_verify
net/core/filter.c:2088:40: got struct bpf_redirect_info *<noident>
net/core/filter.c:2101:40: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] <asn:3>*__vpp_verify @@ got d const [noderef] <asn:3>*__vpp_verify @@
net/core/filter.c:2101:40: expected void const [noderef] <asn:3>*__vpp_verify
net/core/filter.c:2101:40: got struct bpf_redirect_info *<noident>
net/core/filter.c:3194:40: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] <asn:3>*__vpp_verify @@ got d const [noderef] <asn:3>*__vpp_verify @@
net/core/filter.c:3194:40: expected void const [noderef] <asn:3>*__vpp_verify
net/core/filter.c:3194:40: got struct bpf_redirect_info *<noident>
net/core/filter.c:3239:40: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] <asn:3>*__vpp_verify @@ got d const [noderef] <asn:3>*__vpp_verify @@
net/core/filter.c:3239:40: expected void const [noderef] <asn:3>*__vpp_verify
net/core/filter.c:3239:40: got struct bpf_redirect_info *<noident>
net/core/filter.c:3279:40: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] <asn:3>*__vpp_verify @@ got d const [noderef] <asn:3>*__vpp_verify @@
net/core/filter.c:3279:40: expected void const [noderef] <asn:3>*__vpp_verify
net/core/filter.c:3279:40: got struct bpf_redirect_info *<noident>
net/core/filter.c:3311:40: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] <asn:3>*__vpp_verify @@ got d const [noderef] <asn:3>*__vpp_verify @@
net/core/filter.c:3311:40: expected void const [noderef] <asn:3>*__vpp_verify
net/core/filter.c:3311:40: got struct bpf_redirect_info *<noident>
net/core/filter.c:3362:40: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] <asn:3>*__vpp_verify @@ got d const [noderef] <asn:3>*__vpp_verify @@
net/core/filter.c:3362:40: expected void const [noderef] <asn:3>*__vpp_verify
net/core/filter.c:3362:40: got struct bpf_redirect_info *<noident>
net/core/filter.c:3393:40: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] <asn:3>*__vpp_verify @@ got d const [noderef] <asn:3>*__vpp_verify @@
net/core/filter.c:3393:40: expected void const [noderef] <asn:3>*__vpp_verify
net/core/filter.c:3393:40: got struct bpf_redirect_info *<noident>
net/core/filter.c:3417:40: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] <asn:3>*__vpp_verify @@ got d const [noderef] <asn:3>*__vpp_verify @@
net/core/filter.c:3417:40: expected void const [noderef] <asn:3>*__vpp_verify
net/core/filter.c:3417:40: got struct bpf_redirect_info *<noident>
net/core/filter.c:3845:41: sparse: expression using sizeof(void)
net/core/filter.c:3849:41: sparse: expression using sizeof(void)
net/core/filter.c:3853:46: sparse: expression using sizeof(void)
net/core/filter.c:3853:46: sparse: expression using sizeof(void)
net/core/filter.c:3921:47: sparse: expression using sizeof(void)
net/core/filter.c:4127:17: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [unsigned] [usertype] spi @@ got unsigned int [unsigned] [usertype] spi @@
net/core/filter.c:4127:17: expected unsigned int [unsigned] [usertype] spi
net/core/filter.c:4127:17: got restricted __be32 const [usertype] spi
net/core/filter.c:4135:33: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [unsigned] [usertype] remote_ipv4 @@ got unsigned int [unsigned] [usertype] remote_ipv4 @@
net/core/filter.c:4135:33: expected unsigned int [unsigned] [usertype] remote_ipv4
net/core/filter.c:4135:33: got restricted __be32 const [usertype] a4
net/core/filter.c:5304:27: sparse: subtraction of functions? Share your drugs
net/core/filter.c:5307:27: sparse: subtraction of functions? Share your drugs
net/core/filter.c:5310:27: sparse: subtraction of functions? Share your drugs
include/linux/slab.h:631:13: sparse: call with no type!
vim +2088 net/core/filter.c
2085
2086 BPF_CALL_2(bpf_redirect, u32, ifindex, u64, flags)
2087 {
> 2088 struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
2089
2090 if (unlikely(flags & ~(BPF_F_INGRESS)))
2091 return TC_ACT_SHOT;
2092
2093 ri->ifindex = ifindex;
2094 ri->flags = flags;
2095
2096 return TC_ACT_REDIRECT;
2097 }
2098
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* RE: [net-next v6 1/2] net/tls: Use socket data_ready callback on record availability
From: Vakul Garg @ 2018-07-29 6:20 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, borisp@mellanox.com, aviadye@mellanox.com,
davejwatson@fb.com
In-Reply-To: <20180728.231735.773910012836290231.davem@davemloft.net>
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Sunday, July 29, 2018 11:48 AM
> To: Vakul Garg <vakul.garg@nxp.com>
> Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com; davejwatson@fb.com
> Subject: Re: [net-next v6 1/2] net/tls: Use socket data_ready callback on
> record availability
>
> From: Vakul Garg <vakul.garg@nxp.com>
> Date: Sun, 29 Jul 2018 06:01:29 +0000
>
> > Could you please correct me if my counter-reasoning behind changing
> > the socket callback is wrong?
>
> Ok, after stufying the code a bit I agree with your analysis.
Thanks.
Kindly advise, if I need to resubmit/rebase the patch.
^ permalink raw reply
* Re: [net-next v6 1/2] net/tls: Use socket data_ready callback on record availability
From: David Miller @ 2018-07-29 6:17 UTC (permalink / raw)
To: vakul.garg; +Cc: netdev, borisp, aviadye, davejwatson
In-Reply-To: <DB7PR04MB42528E1528A442E42FA7CBF38B280@DB7PR04MB4252.eurprd04.prod.outlook.com>
From: Vakul Garg <vakul.garg@nxp.com>
Date: Sun, 29 Jul 2018 06:01:29 +0000
> Could you please correct me if my counter-reasoning behind changing
> the socket callback is wrong?
Ok, after stufying the code a bit I agree with your analysis.
^ permalink raw reply
* Re: [PATCH][net-next] openvswitch: eliminate cpu_used_mask from sw_flow
From: David Miller @ 2018-07-29 6:12 UTC (permalink / raw)
To: lirongqing; +Cc: netdev, pshelar
In-Reply-To: <1532678637-15079-1-git-send-email-lirongqing@baidu.com>
From: Li RongQing <lirongqing@baidu.com>
Date: Fri, 27 Jul 2018 16:03:57 +0800
> The size of struct cpumask varies with CONFIG_NR_CPUS, some config
> CONFIG_NR_CPUS is very larger, like 5120, struct cpumask will take
> 640 bytes, if there is thousands of flows, it will take lots of
> memory
>
> cpu_used_mask has two purposes
> 1: Assume first cpu as cpu0 which maybe not true; now use
> cpumask_first(cpu_possible_mask)
> 2: when get/clear statistic, reduce the iteratation; but it
> is not hot path, so use for_each_possible_cpu
>
> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
This seems to completely undo the optimization done by:
commit c4b2bf6b4a35348fe6d1eb06928eb68d7b9d99a9
Author: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Date: Mon Jul 17 23:28:06 2017 -0700
openvswitch: Optimize operations for OvS flow_stats.
And in that commit message it states clearly that flow_free()
performance matters, and that the iteration over cpu_possible_mask in
the for() loop is the problem.
At a minimum, we can't apply this unless you explain why the
above performance issue won't be reintroudced by your change.
Thank you.
^ permalink raw reply
* RE: [net-next v6 1/2] net/tls: Use socket data_ready callback on record availability
From: Vakul Garg @ 2018-07-29 6:01 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, borisp@mellanox.com, aviadye@mellanox.com,
davejwatson@fb.com
In-Reply-To: <DB7PR04MB4252186A40EA1A3E0773B34D8B540@DB7PR04MB4252.eurprd04.prod.outlook.com>
Hi David
Could you please correct me if my counter-reasoning behind changing the socket callback is wrong?
Thanks & Regards
Vakul
> -----Original Message-----
> From: Vakul Garg
> Sent: Wednesday, July 25, 2018 11:22 AM
> To: David Miller <davem@davemloft.net>
> Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com; davejwatson@fb.com
> Subject: RE: [net-next v6 1/2] net/tls: Use socket data_ready callback on
> record availability
>
>
>
> > -----Original Message-----
> > From: David Miller [mailto:davem@davemloft.net]
> > Sent: Wednesday, July 25, 2018 1:43 AM
> > To: Vakul Garg <vakul.garg@nxp.com>
> > Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com;
> > davejwatson@fb.com
> > Subject: Re: [net-next v6 1/2] net/tls: Use socket data_ready callback
> > on record availability
> >
> > From: Vakul Garg <vakul.garg@nxp.com>
> > Date: Tue, 24 Jul 2018 15:44:02 +0530
> >
> > > On receipt of a complete tls record, use socket's saved data_ready
> > > callback instead of state_change callback.
> > >
> > > Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
> >
> > I don't think this is correct.
> >
> > Here, the stream parser has given us a complete TLS record.
> >
> > But we haven't decrypted this packet yet. It sits on the stream
> > parser's queue to be processed by tls_sw_recvmsg(), not the saved
> > socket's receive queue.
>
> I understand that at this point in code, the TLS record is still queued in
> encrypted state. But the decryption happens inline when tls_sw_recvmsg()
> gets invokved.
> So it should be ok to notify the waiting context about the availability of data
> as soon as we could collect a full TLS record.
>
> For new data availability notification, sk_data_ready callback should be more
> more appropriate. It points to sock_def_readable() which wakes up
> specifically for EPOLLIN event.
>
> This is in contrast to the socket callback sk_state_change which points to
> sock_def_wakeup() which issues a wakeup unconditionally (without event
> mask).
^ permalink raw reply
* Re: [PATCH net-next v2 0/2] tls: Fix improper revert in zerocopy_from_iter
From: David Miller @ 2018-07-29 5:53 UTC (permalink / raw)
To: doronrk; +Cc: vakul.garg, davejwatson, mmullins, netdev
In-Reply-To: <20180726145936.3547913-1-doronrk@fb.com>
From: Doron Roberts-Kedes <doronrk@fb.com>
Date: Thu, 26 Jul 2018 07:59:34 -0700
> This series fixes the improper iov_iter_revert introcded in
> "tls: Fix zerocopy_from_iter iov handling".
>
> Changes from v1:
> - call iov_iter_revert inside zerocopy_from_iter
Series applied, thank you.
^ 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