* Re: [PATCH 2/3] IB/ipoib: Stop using dev_id to expose port numbers
From: Arseny Maslennikov @ 2018-08-13 13:57 UTC (permalink / raw)
To: Yuval Shaia; +Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, netdev
In-Reply-To: <20180813124018.GA6122@lap1>
[-- Attachment #1: Type: text/plain, Size: 2342 bytes --]
On Mon, Aug 13, 2018 at 03:40:20PM +0300, Yuval Shaia wrote:
> On Mon, Aug 13, 2018 at 02:42:23PM +0300, Arseny Maslennikov wrote:
> > Some InfiniBand network devices have multiple ports on the same PCI
> > function. Prior to this the kernel erroneously used the `dev_id' sysfs
> > field of those network interfaces to convey the port number to userspace.
> >
> > `dev_id' is currently reserved for distinguishing stacked ifaces
> > (e.g: VLANs) with the same hardware address as their parent device.
> >
> > Similar fixes to net/mlx4_en and many other drivers, which started
> > exporting this information through `dev_id' before 3.15, were accepted
> > into the kernel 4 years ago.
> > See 76a066f2a2a0268b565459c417b59724b5a3197b, commit message:
> > `net/mlx4_en: Expose port number through sysfs'.
> >
> > I would be OK with this commit not being backported to stable, since
> > it might break admin-supplied udev rules and the likes.
> >
> > Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> > ---
> > drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > index 6eb0594fffec..f64535038147 100644
> > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > @@ -2252,7 +2252,6 @@ static struct net_device *ipoib_add_port(const char *format,
> > }
> >
> > SET_NETDEV_DEV(priv->dev, hca->dev.parent);
> > - priv->dev->dev_id = port - 1;
>
> Correct me if i'm wrong here but besides some changes in commit message
> looks like patch 1/3 is the same as 2/3, isn't it?
>
> Yuval
>
1/3 has an extra line, 2/3 removes a different line.
(a) If you apply both 1/3 and 2/3, the port number can be seen at
/sys/class/net/*/dev_port and not at .../dev_id.
(b) If you apply only 1/3, the port number can be seen at _both_
.../dev_port and .../dev_id (to preserve backward compatibility with
e.g. existing udev rules that rely on "ATTR{dev_id}")
By splitting those up we have both options (a) and (b) available,
so the maintainers are free to decide which one is wiser.
> > priv->dev->dev_port = port - 1;
> >
> > result = ib_query_port(hca, port, &attr);
> > --
> > 2.18.0
> >
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] net: Change the layout of structure trace_event_raw_fib_table_lookup
From: David Miller @ 2018-08-13 16:37 UTC (permalink / raw)
To: zong; +Cc: rostedt, mingo, dsahern, netdev, linux-kernel, zongbox, greentime
In-Reply-To: <1534127212-13186-1-git-send-email-zong@andestech.com>
From: Zong Li <zong@andestech.com>
Date: Mon, 13 Aug 2018 10:26:52 +0800
> There is an unalignment access about the structure
> 'trace_event_raw_fib_table_lookup'.
>
> In include/trace/events/fib.h, there is a memory operation which casting
> the 'src' data member to a pointer, and then store a value to this
> pointer point to.
>
> p32 = (__be32 *) __entry->src;
> *p32 = flp->saddr;
>
> The offset of 'src' in structure trace_event_raw_fib_table_lookup is not
> four bytes alignment. On some architectures, they don't permit the
> unalignment access, it need to pay the price to handle this situation in
> exception handler.
>
> Adjust the layout of structure to avoid this case.
>
> Signed-off-by: Zong Li <zong@andestech.com>
Applied, with Fixes tag from David added.
^ permalink raw reply
* Re: [Query]: DSA Understanding
From: Andrew Lunn @ 2018-08-13 13:38 UTC (permalink / raw)
To: Lad, Prabhakar; +Cc: Florian Fainelli, netdev
In-Reply-To: <CA+V-a8tXStPqWGtJdvC0KzsgsPyGoXY7H8gGfXC+7s0dE9koaQ@mail.gmail.com>
> > I agree, this should be padding packets correctly, can you still
> > instrument cpsw to make sure that what comes to its ndo_start_xmit() is
> > ETH_ZLEN + tag_len or more?
> >
> Yes I can confirm the skb->len is always >= 62 (ETH_ZLEN + 2)
Which switch are you using?
Marvell switches use either 4 or 8 bytes of tag. Broadcom has 4, KSZ
has 1 for packets going to the switch, lan9303 has 4, mtd uses 4, qca
has 2.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 5/9] net: hns3: Fix for vf vlan delete failed problem
From: David Miller @ 2018-08-13 15:56 UTC (permalink / raw)
To: salil.mehta
Cc: yisen.zhuang, lipeng321, mehta.salil.lnk, netdev, linux-kernel,
linuxarm, linyunsheng
In-Reply-To: <20180812094738.14852-6-salil.mehta@huawei.com>
From: Salil Mehta <salil.mehta@huawei.com>
Date: Sun, 12 Aug 2018 10:47:34 +0100
> Fixes: 9dba194574e3 ("{topost} net: hns3: fix for vlan table problem")
This commit ID doesn't exist.
Also, I really don't think the string "{topost}" would be in the commit
header line.
^ permalink raw reply
* Re: [PATCH net-next 1/9] net: hns3: Add support for serdes loopback selftest
From: David Miller @ 2018-08-13 15:54 UTC (permalink / raw)
To: salil.mehta
Cc: yisen.zhuang, lipeng321, mehta.salil.lnk, netdev, linux-kernel,
linuxarm, linyunsheng
In-Reply-To: <20180812094738.14852-2-salil.mehta@huawei.com>
From: Salil Mehta <salil.mehta@huawei.com>
Date: Sun, 12 Aug 2018 10:47:30 +0100
> -#define HNS3_SELF_TEST_TPYE_NUM 1
> +#define HNS3_SELF_TEST_TPYE_NUM 2
Is this supposed to be the number of self test "types"? If so, this CPP
macro should be named "HNS3_SELF_TEST_TYPE_NUM".
> +
> + count ++;
^
Please remove that unnecessary space.
> + } while (++i < HCLGE_SERDES_RETRY_NUM &&
^^
Only need one space there, not two.
^ permalink raw reply
* Re: [PATCH v1 0/3] WireGuard: Secure Network Tunnel
From: Willy Tarreau @ 2018-08-13 15:53 UTC (permalink / raw)
To: James Bottomley
Cc: Jason A. Donenfeld, linux-kernel, netdev, davem, linux-crypto
In-Reply-To: <1534174811.7872.3.camel@HansenPartnership.com>
On Mon, Aug 13, 2018 at 08:40:11AM -0700, James Bottomley wrote:
> Could we please build planning for this crypto failure day into
> wireguard now rather than have to do it later? It doesn't need to be
> full cipher agility, it just needs to be the ability to handle multiple
> protocol versions ... two should do it because that gives a template to
> follow (and test version to try to find bugs in the implementation).
It's also what provides a *real* upgrade path to future versions :
before deploying you need something which works, and the only way to
get something working at a large scale is to have early adopters. Those
willing to deploy a beta version will not do it if it requires to lose
all their users and possibly to make rollbacks impossible. At least for
this it's important to support an optionnal new version on top of the
existing one (i.e. prod + beta together).
Cheers,
Willy
^ permalink raw reply
* Re: [PATCH v2] xen-netfront: fix warn message as irq device name has '/'
From: David Miller @ 2018-08-13 15:52 UTC (permalink / raw)
To: xiliang; +Cc: netdev, xen-devel, jgross, boris.ostrovsky, linux-kernel
In-Reply-To: <20180811152137.3627-1-xiliang@redhat.com>
From: Xiao Liang <xiliang@redhat.com>
Date: Sat, 11 Aug 2018 23:21:37 +0800
> There is a call trace generated after commit 2d408c0d4574b01b9ed45e02516888bf925e11a9(
> xen-netfront: fix queue name setting). There is no 'device/vif/xx-q0-tx' file found
> under /proc/irq/xx/.
>
> This patch only picks up device type and id as its name.
This adds a compile warning:
drivers/net/xen-netfront.c: In function ‘xennet_init_queue’:
drivers/net/xen-netfront.c:1614:2: warning: ignoring return value of ‘kstrtoint’, declared with attribute warn_unused_result [-Wunused-result]
kstrtoint(strrchr(queue->info->xbdev->nodename, '/') + 1, 10, &devid);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
^ permalink raw reply
* Re: [PATCH 0/2] net: nixge: Minor cleanups
From: David Miller @ 2018-08-13 15:49 UTC (permalink / raw)
To: mdf; +Cc: keescook, netdev, linux-kernel, alex.williams
In-Reply-To: <20180811011941.6187-1-mdf@kernel.org>
From: Moritz Fischer <mdf@kernel.org>
Date: Fri, 10 Aug 2018 18:19:39 -0700
> in preparation of my 64-bit support series, here's some
> minor cleanup in preparation that gets rid of unneccesary
> accesses to the descriptor application fields.
>
> I've confirmed that the hardware does not access the fields
> in all our configurations.
Series applied, thank you.
^ permalink raw reply
* Re: [V9fs-developer] [PATCH 2/2] 9p: Add refcount to p9_req_t
From: Dmitry Vyukov @ 2018-08-13 13:04 UTC (permalink / raw)
To: Dominique Martinet
Cc: piaojun, Tomas Bortoli, Eric Van Hensbergen, Ron Minnich,
Latchesar Ionkov, Dominique Martinet, netdev, LKML, syzkaller,
v9fs-developer, David Miller
In-Reply-To: <20180813014815.GB6777@nautica>
On Mon, Aug 13, 2018 at 3:48 AM, Dominique Martinet
<asmadeus@codewreck.org> wrote:
> piaojun wrote on Mon, Aug 13, 2018:
>> Could you help paste the reason of the crash bug to help others
>> understand more clearly? And I have another question below.
>
> The problem for tcp (but other transports have a similar problem) is
> that with a malicious server like syzkaller they can try to submit
> replies before the request came in.
>
> This leads in the writer thread trying to write a buffer that has
> already been freed, and if memory has been reused could potentially leak
> some information.
>
> Now, with the previous patches this is based on this would be a slab and
> the likeliness of it being sensitive information is rather low (it would
> likely be some other packet being sent twice, or a mix and match of two
> packets that would have been sent anyway), but it would nevertheless be
> a use after free.
>
>
> There is a second advantage to this reference counting, that is now we
> have this system we will be able to implement flush asynchronously.
> This will remove the need for the 'goto again' in p9_client_rpc which
> was making 9p threads unkillable in practice if the server would not
> reply to the flush requests.
Fixing unkillalble task would be nice. Don't know how much they are of
a problem in real life, but fixing them would allow fuzzer to find
other, potentially more critical bugs in 9p. These "task hung" crashes
are quite unpleasant for the fuzzer.
Thanks for all recent 9p work, Tomas!
> Even if the server replies I've always found myself needing to hit ^C
> multiple times to exit a process doing I/Os and I think fixing that
> behaviour will make 9p more comfortable to use.
>
>
>> > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
>> > index 20f46f13fe83..686e24e355d0 100644
>> > --- a/net/9p/trans_fd.c
>> > +++ b/net/9p/trans_fd.c
>> > @@ -132,6 +132,7 @@ struct p9_conn {
>> > struct list_head req_list;
>> > struct list_head unsent_req_list;
>> > struct p9_req_t *req;
>> > + struct p9_req_t *wreq;
>>
>> Why adding a wreq for write work? And I wonder we should rename req to
>> rreq?
>
> We need to store a pointer to the request for the write thread because
> we need to put the reference to it when we're done writing its content.
>
> Previously, the worker would only store the write buffer there but
> that's not enough to figure what request to dereference.
>
>
> I personally don't think renaming req to rreq would bring much but it
> could be done in another patch if you think that'd be helpful; I think
> it shouldn't be done here at least to make the patch more readable.
>
> --
> Dominique
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH v2] Add icmp_echo_ignore_all support for ICMPv6
From: David Miller @ 2018-08-13 15:42 UTC (permalink / raw)
To: virgile; +Cc: kuznet, yoshfuji, netdev, linux-kernel
In-Reply-To: <20180810154815.GA18171@_>
From: Virgile Jarry <virgile@acceis.fr>
Date: Fri, 10 Aug 2018 17:48:15 +0200
> Preventing the kernel from responding to ICMP Echo Requests messages
> can be useful in several ways. The sysctl parameter
> 'icmp_echo_ignore_all' can be used to prevent the kernel from
> responding to IPv4 ICMP echo requests. For IPv6 pings, such
> a sysctl kernel parameter did not exist.
>
> Add the ability to prevent the kernel from responding to IPv6
> ICMP echo requests through the use of the following sysctl
> parameter : /proc/sys/net/ipv6/icmp/echo_ignore_all.
> Update the documentation to reflect this change.
>
> Signed-off-by: Virgile Jarry <virgile@acceis.fr>
> ---
> Changes in v2:
> - Added support for sysctl interface (and not just /proc filesystem)
Applied.
^ permalink raw reply
* Re: [PATCH v1 0/3] WireGuard: Secure Network Tunnel
From: James Bottomley @ 2018-08-13 15:40 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: linux-kernel, netdev, davem, linux-crypto
In-Reply-To: <20180731191102.2434-1-Jason@zx2c4.com>
> Ample information, including documentation, installation
> instructions,
> and project details, is available at:
>
> * https://www.wireguard.com/
> * https://www.wireguard.com/papers/wireguard.pdf
In your paper you say this:
> Finally, WireGuard is cryptographically opinionated. It intentionally
> lacks cipher and protocol agility. If
> holes are found in the underlying primitives, all endpoints will be
> required to update.
The only thing that's certain (beyond death and taxes) is that your
crypto choice will one day need updating; either in response to an
urgent CVE because an algorithm is compromised or in response to a less
urgent one because it is deprecated. Assuming wireguard is reasonably
successful we'll have a large ecosystem dependent on it. On this day,
we're going to have the choice of either breaking the entire ecosystem
by rolling out a change that can't connect to lower protocol versions
or trying to wedge version agility into wireguard in a hurry. The
former is too awful to contemplate because of the almost universal
ecosystem breakage it would cause and the latter is going to lead to
additional bugs because people in a hurry aren't as careful as they
should be.
Could we please build planning for this crypto failure day into
wireguard now rather than have to do it later? It doesn't need to be
full cipher agility, it just needs to be the ability to handle multiple
protocol versions ... two should do it because that gives a template to
follow (and test version to try to find bugs in the implementation).
It looks like the protocol could simply be updated to put the version
into one (or more) of the three reserved bytes in the handshake
headers, so perhaps doing this before they get used for something else
would be a good first step?
James
^ permalink raw reply
* Re: possible deadlock in flush_work (3)
From: Xin Long @ 2018-08-13 15:39 UTC (permalink / raw)
To: syzbot
Cc: Christian Brauner, davem, David Ahern, Florian Westphal,
Jiri Benc, Kirill Tkhai, LKML, network dev, Nicolas Dichtel,
syzkaller-bugs, Moni Shoua
In-Reply-To: <0000000000009637a6057350529b@google.com>
On Mon, Aug 13, 2018 at 8:35 PM, syzbot
<syzbot+a8371264572a6872b8a3@syzkaller.appspotmail.com> wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: d6dd6431591b Merge branch 'fixes' of git://git.kernel.org/..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=14e800aa400000
> kernel config: https://syzkaller.appspot.com/x/.config?x=152cb8ccd35b1f70
> dashboard link: https://syzkaller.appspot.com/bug?extid=a8371264572a6872b8a3
> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+a8371264572a6872b8a3@syzkaller.appspotmail.com
>
> 8021q: adding VLAN 0 to HW filter on device bond0
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 4.18.0-rc8+ #185 Not tainted
> ------------------------------------------------------
> syz-executor2/6421 is trying to acquire lock:
> 00000000209b4e4b ((wq_completion)bond_dev->name){+.+.}, at: start_flush_work
> kernel/workqueue.c:2888 [inline]
> 00000000209b4e4b ((wq_completion)bond_dev->name){+.+.}, at:
> flush_work+0x4b8/0x900 kernel/workqueue.c:2917
>
> but task is already holding lock:
> 00000000f0f3d47a (rtnl_mutex){+.+.}, at: rtnl_lock+0x17/0x20
> net/core/rtnetlink.c:77
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #2 (rtnl_mutex){+.+.}:
> __mutex_lock_common kernel/locking/mutex.c:757 [inline]
> __mutex_lock+0x176/0x1820 kernel/locking/mutex.c:894
> mutex_lock_nested+0x16/0x20 kernel/locking/mutex.c:909
> rtnl_lock+0x17/0x20 net/core/rtnetlink.c:77
> bond_netdev_notify drivers/net/bonding/bond_main.c:1310 [inline]
> bond_netdev_notify_work+0x44/0xd0
> drivers/net/bonding/bond_main.c:1320
> process_one_work+0xc73/0x1ba0 kernel/workqueue.c:2153
> worker_thread+0x189/0x13c0 kernel/workqueue.c:2296
> kthread+0x345/0x410 kernel/kthread.c:246
> ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
>
> -> #1 ((work_completion)(&(&nnw->work)->work)){+.+.}:
> process_one_work+0xc0b/0x1ba0 kernel/workqueue.c:2129
> worker_thread+0x189/0x13c0 kernel/workqueue.c:2296
> kthread+0x345/0x410 kernel/kthread.c:246
> ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
>
> -> #0 ((wq_completion)bond_dev->name){+.+.}:
> lock_acquire+0x1e4/0x540 kernel/locking/lockdep.c:3924
> start_flush_work kernel/workqueue.c:2889 [inline]
> flush_work+0x4dd/0x900 kernel/workqueue.c:2917
> __cancel_work_timer+0x4bd/0x830 kernel/workqueue.c:2989
> cancel_delayed_work_sync+0x1a/0x20 kernel/workqueue.c:3121
> bond_work_cancel_all drivers/net/bonding/bond_main.c:3318 [inline]
> bond_close+0x1b/0x130 drivers/net/bonding/bond_main.c:3381
> __dev_close_many+0x21e/0x380 net/core/dev.c:1476
> __dev_close net/core/dev.c:1488 [inline]
> __dev_change_flags+0x38d/0x9c0 net/core/dev.c:6989
> dev_change_flags+0x89/0x150 net/core/dev.c:7060
> dev_ifsioc+0x84f/0xb30 net/core/dev_ioctl.c:237
> dev_ioctl+0x1b5/0xcc0 net/core/dev_ioctl.c:493
> sock_do_ioctl+0x1d3/0x3e0 net/socket.c:993
> sock_ioctl+0x30d/0x680 net/socket.c:1094
> vfs_ioctl fs/ioctl.c:46 [inline]
> file_ioctl fs/ioctl.c:500 [inline]
> do_vfs_ioctl+0x1de/0x1720 fs/ioctl.c:684
> ksys_ioctl+0xa9/0xd0 fs/ioctl.c:701
> __do_sys_ioctl fs/ioctl.c:708 [inline]
> __se_sys_ioctl fs/ioctl.c:706 [inline]
> __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:706
> do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> other info that might help us debug this:
>
> Chain exists of:
> (wq_completion)bond_dev->name --> (work_completion)(&(&nnw->work)->work)
> --> rtnl_mutex
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(rtnl_mutex);
> lock((work_completion)(&(&nnw->work)->work));
> lock(rtnl_mutex);
> lock((wq_completion)bond_dev->name);
nnw->work is queuing up into bond->wq, so it seems bond_netdev_notify()
should have used rtnl_trylock() instead of rtnl_lock(), as do other
delayed_work handlers in bond->wq.
>
> *** DEADLOCK ***
>
> 1 lock held by syz-executor2/6421:
> #0: 00000000f0f3d47a (rtnl_mutex){+.+.}, at: rtnl_lock+0x17/0x20
> net/core/rtnetlink.c:77
>
> stack backtrace:
> CPU: 0 PID: 6421 Comm: syz-executor2 Not tainted 4.18.0-rc8+ #185
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113
> print_circular_bug.isra.36.cold.57+0x1bd/0x27d
> kernel/locking/lockdep.c:1227
> check_prev_add kernel/locking/lockdep.c:1867 [inline]
> check_prevs_add kernel/locking/lockdep.c:1980 [inline]
> validate_chain kernel/locking/lockdep.c:2421 [inline]
> __lock_acquire+0x3449/0x5020 kernel/locking/lockdep.c:3435
> lock_acquire+0x1e4/0x540 kernel/locking/lockdep.c:3924
> start_flush_work kernel/workqueue.c:2889 [inline]
> flush_work+0x4dd/0x900 kernel/workqueue.c:2917
> __cancel_work_timer+0x4bd/0x830 kernel/workqueue.c:2989
> cancel_delayed_work_sync+0x1a/0x20 kernel/workqueue.c:3121
> bond_work_cancel_all drivers/net/bonding/bond_main.c:3318 [inline]
> bond_close+0x1b/0x130 drivers/net/bonding/bond_main.c:3381
> __dev_close_many+0x21e/0x380 net/core/dev.c:1476
> __dev_close net/core/dev.c:1488 [inline]
> __dev_change_flags+0x38d/0x9c0 net/core/dev.c:6989
> dev_change_flags+0x89/0x150 net/core/dev.c:7060
> dev_ifsioc+0x84f/0xb30 net/core/dev_ioctl.c:237
> dev_ioctl+0x1b5/0xcc0 net/core/dev_ioctl.c:493
> sock_do_ioctl+0x1d3/0x3e0 net/socket.c:993
> kernel msg: ebtables bug: please report to author: Wrong nr of counters
> sock_ioctl+0x30d/0x680 net/socket.c:1094
> vfs_ioctl fs/ioctl.c:46 [inline]
> file_ioctl fs/ioctl.c:500 [inline]
> do_vfs_ioctl+0x1de/0x1720 fs/ioctl.c:684
> ksys_ioctl+0xa9/0xd0 fs/ioctl.c:701
> __do_sys_ioctl fs/ioctl.c:708 [inline]
> __se_sys_ioctl fs/ioctl.c:706 [inline]
> __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:706
> do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x457089
> Code: fd b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff
> 0f 83 cb b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007fee7b2ccc78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 00007fee7b2cd6d4 RCX: 0000000000457089
> RDX: 0000000020000140 RSI: 0000000000008914 RDI: 0000000000000004
> RBP: 00000000009300a0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
> R13: 00000000004d19e0 R14: 00000000004c7454 R15: 0000000000000000
> 8021q: adding VLAN 0 to HW filter on device bond0
> (unnamed net_device) (uninitialized): option miimon: invalid value
> (18446744073709551615)
> (unnamed net_device) (uninitialized): option miimon: allowed values 0 -
> 2147483647
> (unnamed net_device) (uninitialized): option miimon: invalid value
> (18446744073709551615)
> (unnamed net_device) (uninitialized): option miimon: allowed values 0 -
> 2147483647
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> device bond0 left promiscuous mode
> IPv6: ADDRCONF(NETDEV_UP): bond0: link is not ready
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> IPv6: ADDRCONF(NETDEV_UP): bond0: link is not ready
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> IPv6: ADDRCONF(NETDEV_UP): bond0: link is not ready
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
> syzbot.
^ permalink raw reply
* Re: [PATCH 2/3] IB/ipoib: Stop using dev_id to expose port numbers
From: Yuval Shaia @ 2018-08-13 12:40 UTC (permalink / raw)
To: Arseny Maslennikov; +Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, netdev
In-Reply-To: <20180813114224.7065-3-ar@cs.msu.ru>
On Mon, Aug 13, 2018 at 02:42:23PM +0300, Arseny Maslennikov wrote:
> Some InfiniBand network devices have multiple ports on the same PCI
> function. Prior to this the kernel erroneously used the `dev_id' sysfs
> field of those network interfaces to convey the port number to userspace.
>
> `dev_id' is currently reserved for distinguishing stacked ifaces
> (e.g: VLANs) with the same hardware address as their parent device.
>
> Similar fixes to net/mlx4_en and many other drivers, which started
> exporting this information through `dev_id' before 3.15, were accepted
> into the kernel 4 years ago.
> See 76a066f2a2a0268b565459c417b59724b5a3197b, commit message:
> `net/mlx4_en: Expose port number through sysfs'.
>
> I would be OK with this commit not being backported to stable, since
> it might break admin-supplied udev rules and the likes.
>
> Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> ---
> drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index 6eb0594fffec..f64535038147 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -2252,7 +2252,6 @@ static struct net_device *ipoib_add_port(const char *format,
> }
>
> SET_NETDEV_DEV(priv->dev, hca->dev.parent);
> - priv->dev->dev_id = port - 1;
Correct me if i'm wrong here but besides some changes in commit message
looks like patch 1/3 is the same as 2/3, isn't it?
Yuval
> priv->dev->dev_port = port - 1;
>
> result = ib_query_port(hca, port, &attr);
> --
> 2.18.0
>
^ permalink raw reply
* Re: [PATCH net 2/2] net/mlx5e: Cleanup of dcbnl related fields
From: Yuval Shaia @ 2018-08-13 12:32 UTC (permalink / raw)
To: Saeed Mahameed; +Cc: David S. Miller, netdev, Huy Nguyen
In-Reply-To: <20180808224808.12600-3-saeedm@mellanox.com>
On Wed, Aug 08, 2018 at 03:48:08PM -0700, Saeed Mahameed wrote:
> From: Huy Nguyen <huyn@mellanox.com>
>
> Remove unused netdev_registered_init/remove in en.h
> Return ENOSUPPORT if the check MLX5_DSCP_SUPPORTED fails.
> Remove extra white space
>
> Fixes: 2a5e7a1344f4 ("net/mlx5e: Add dcbnl dscp to priority support")
> Signed-off-by: Huy Nguyen <huyn@mellanox.com>
> Cc: Yuval Shaia <yuval.shaia@oracle.com>
> Reviewed-by: Parav Pandit <parav@mellanox.com>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
FWIW:
Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
Tested-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en.h | 2 --
> .../ethernet/mellanox/mlx5/core/en_dcbnl.c | 30 +++++++------------
> 2 files changed, 11 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> index eb9eb7aa953a..405236cf0b04 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> @@ -858,8 +858,6 @@ struct mlx5e_profile {
> mlx5e_fp_handle_rx_cqe handle_rx_cqe;
> mlx5e_fp_handle_rx_cqe handle_rx_cqe_mpwqe;
> } rx_handlers;
> - void (*netdev_registered_init)(struct mlx5e_priv *priv);
> - void (*netdev_registered_remove)(struct mlx5e_priv *priv);
> int max_tc;
> };
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> index e33afa8d2417..722998d68564 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> @@ -443,16 +443,12 @@ static int mlx5e_dcbnl_ieee_setapp(struct net_device *dev, struct dcb_app *app)
> bool is_new;
> int err;
>
> - if (app->selector != IEEE_8021QAZ_APP_SEL_DSCP)
> - return -EINVAL;
> -
> - if (!MLX5_CAP_GEN(priv->mdev, vport_group_manager))
> - return -EINVAL;
> -
> - if (!MLX5_DSCP_SUPPORTED(priv->mdev))
> - return -EINVAL;
> + if (!MLX5_CAP_GEN(priv->mdev, vport_group_manager) ||
> + !MLX5_DSCP_SUPPORTED(priv->mdev))
> + return -EOPNOTSUPP;
>
> - if (app->protocol >= MLX5E_MAX_DSCP)
> + if ((app->selector != IEEE_8021QAZ_APP_SEL_DSCP) ||
> + (app->protocol >= MLX5E_MAX_DSCP))
> return -EINVAL;
>
> /* Save the old entry info */
> @@ -500,16 +496,12 @@ static int mlx5e_dcbnl_ieee_delapp(struct net_device *dev, struct dcb_app *app)
> struct mlx5e_priv *priv = netdev_priv(dev);
> int err;
>
> - if (app->selector != IEEE_8021QAZ_APP_SEL_DSCP)
> - return -EINVAL;
> -
> - if (!MLX5_CAP_GEN(priv->mdev, vport_group_manager))
> - return -EINVAL;
> -
> - if (!MLX5_DSCP_SUPPORTED(priv->mdev))
> - return -EINVAL;
> + if (!MLX5_CAP_GEN(priv->mdev, vport_group_manager) ||
> + !MLX5_DSCP_SUPPORTED(priv->mdev))
> + return -EOPNOTSUPP;
>
> - if (app->protocol >= MLX5E_MAX_DSCP)
> + if ((app->selector != IEEE_8021QAZ_APP_SEL_DSCP) ||
> + (app->protocol >= MLX5E_MAX_DSCP))
> return -EINVAL;
>
> /* Skip if no dscp app entry */
> @@ -1146,7 +1138,7 @@ static int mlx5e_set_trust_state(struct mlx5e_priv *priv, u8 trust_state)
> {
> int err;
>
> - err = mlx5_set_trust_state(priv->mdev, trust_state);
> + err = mlx5_set_trust_state(priv->mdev, trust_state);
> if (err)
> return err;
> priv->dcbx_dp.trust_state = trust_state;
> --
> 2.17.0
>
^ permalink raw reply
* Re: [PATCH bpf-next V3] net/xdp: Fix suspicious RCU usage warning
From: Daniel Borkmann @ 2018-08-13 12:22 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Tariq Toukan
Cc: Alexei Starovoitov, netdev, Eran Ben Elisha, Neil Brown,
Paul E. McKenney
In-Reply-To: <20180813140243.4484815e@redhat.com>
On 08/13/2018 02:02 PM, Jesper Dangaard Brouer wrote:
> On Mon, 13 Aug 2018 13:57:04 +0300
> Tariq Toukan <tariqt@mellanox.com> wrote:
>> On 13/08/2018 1:31 PM, Jesper Dangaard Brouer wrote:
>>> On Mon, 13 Aug 2018 12:21:58 +0300
>>> Tariq Toukan <tariqt@mellanox.com> wrote:
[...]
>>> In the example[1], the sequence is wrapped in rcu_read_lock/unlock,
>>> while you have not done so. The rhashtable_lookup_fast and
>>> rhashtable_remove_fast calls have their own rcu_read_lock/unlock,
>>> but the outer rcu_read_lock/unlock, makes sure that a RCU period
>>> cannot slip in between the two calls.
>>>
>>> I still think your fix is correct, due to the mutex_lock. Given the
>>> mutex sync removal and insert in this rhashtable.
>>
>> Right, we rely here on the mutex to avoid the scenario you described.
>> So the outer rcu lock calls are not necessary.
Agree.
>>> I do wonder if it would be better to add the outer
>>> rcu_read_lock/unlock, calls if someone else reads and copy-paste
>>> this code (and don't have an mutex sync scheme) ?
>>
>> Yeah it'll be safer for future unaware developers, but I think
>> reviewers should always comment and make it clear that the best
>> generic reference is [1], not any specific/optimized use case.
>>
>> If you guys still want to me to fix this then please let me know and
>> I'll re-spin.
>
> I'll let Daniel make the choice.
Patch is fine as is. If we would be adding the RCU read lock/unlock pair
even though it's not necessary but for other developers to copy paste
from, I think this might be double-confusing: in the one case for people
reading the current code as they will wonder why the additional RCU read
side is needed here (so it will leave them puzzling), and in the other
case for people trying to copy-paste from it wondering whether they would
need similar scheme with mutex in addition. So I strongly prefer to 'do
the right thing' based on the situation. Given BPF PR is still pending,
I'll get the patch in once it has been pulled.
Thanks,
Daniel
^ permalink raw reply
* [PATCH 0/3] net, IB/ipoib: Use dev_port to disambiguate
From: Arseny Maslennikov @ 2018-08-13 11:42 UTC (permalink / raw)
To: linux-rdma; +Cc: Arseny Maslennikov, Doug Ledford, Jason Gunthorpe, netdev
Pre-3.15 userspace had trouble distinguishing different ports of a NIC
on a single PCI bus/device/function. To solve this, a sysfs field `dev_port'
was introduced quite a while ago (commit v3.14-rc3-739-g3f85944fe207), and
some relevant device drivers were fixed to use it, but not in case of IPoIB.
The convention for some reason never got documented in the kernel, but
was immediately adopted by userspace (notably udev[1][2], biosdevname[3])
3/3 documents the sysfs field — that's why I'm CC-ing netdev.
This series was tested on current LTS and 4.18.
[1] https://lists.freedesktop.org/archives/systemd-devel/2014-June/020788.html
[2] https://lists.freedesktop.org/archives/systemd-devel/2014-July/020804.html
[3] https://github.com/CloudAutomationNTools/biosdevname/blob/c795d51dd93a5309652f0d635f12a3ecfabfaa72/src/eths.c#L38
Arseny Maslennikov (3):
IB/ipoib: Use dev_port to expose network interface port numbers
IB/ipoib: Stop using dev_id to expose port numbers
Documentation/ABI: document /sys/class/net/*/dev_port
Documentation/ABI/testing/sysfs-class-net | 10 ++++++++++
drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 +-
2 files changed, 11 insertions(+), 1 deletion(-)
--
2.18.0
^ permalink raw reply
* [PATCH 1/3] IB/ipoib: Use dev_port to expose network interface port numbers
From: Arseny Maslennikov @ 2018-08-13 11:42 UTC (permalink / raw)
To: linux-rdma; +Cc: Arseny Maslennikov, Doug Ledford, Jason Gunthorpe, netdev
In-Reply-To: <20180813114224.7065-1-ar@cs.msu.ru>
Some InfiniBand network devices have multiple ports on the same PCI
function. This initializes the `dev_port' sysfs field of those
network interfaces with their port number.
The use of `dev_id' was considered correct until Linux 3.15,
when another field, `dev_port', was defined for this particular
purpose and `dev_id' was reserved for distinguishing stacked ifaces
(e.g: VLANs) with the same hardware address as their parent device.
Similar fixes to net/mlx4_en and many other drivers, which started
exporting this information through `dev_id' before 3.15, were accepted
into the kernel 4 years ago.
See 76a066f2a2a0268b565459c417b59724b5a3197b, commit message:
`net/mlx4_en: Expose port number through sysfs'.
Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 26cde95bc0f3..6eb0594fffec 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2253,6 +2253,7 @@ static struct net_device *ipoib_add_port(const char *format,
SET_NETDEV_DEV(priv->dev, hca->dev.parent);
priv->dev->dev_id = port - 1;
+ priv->dev->dev_port = port - 1;
result = ib_query_port(hca, port, &attr);
if (result) {
--
2.18.0
^ permalink raw reply related
* [PATCH 3/3] Documentation/ABI: document /sys/class/net/*/dev_port
From: Arseny Maslennikov @ 2018-08-13 11:42 UTC (permalink / raw)
To: linux-rdma; +Cc: Arseny Maslennikov, Doug Ledford, Jason Gunthorpe, netdev
In-Reply-To: <20180813114224.7065-1-ar@cs.msu.ru>
The sysfs field was introduced 4 years ago along with fixes to various
drivers that erroneously used `dev_id' for that purpose, but it was not
properly documented anywhere.
See commit v3.14-rc3-739-g3f85944fe207.
Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
---
Documentation/ABI/testing/sysfs-class-net | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-net b/Documentation/ABI/testing/sysfs-class-net
index 2f1788111cd9..1593d8997ade 100644
--- a/Documentation/ABI/testing/sysfs-class-net
+++ b/Documentation/ABI/testing/sysfs-class-net
@@ -91,6 +91,16 @@ Description:
stacked (e.g: VLAN interfaces) but still have the same MAC
address as their parent device.
+What: /sys/class/net/<iface>/dev_port
+Date: February 2014
+KernelVersion: 3.15
+Contact: netdev@vger.kernel.org
+Description:
+ Indicates the port number of this network device, formatted
+ as a decimal value. Some NICs have multiple independent ports
+ on the same PCI bus, device and function. This field allows
+ userspace to distinguish the respective interfaces.
+
What: /sys/class/net/<iface>/dormant
Date: March 2006
KernelVersion: 2.6.17
--
2.18.0
^ permalink raw reply related
* [PATCH 2/3] IB/ipoib: Stop using dev_id to expose port numbers
From: Arseny Maslennikov @ 2018-08-13 11:42 UTC (permalink / raw)
To: linux-rdma; +Cc: Arseny Maslennikov, Doug Ledford, Jason Gunthorpe, netdev
In-Reply-To: <20180813114224.7065-1-ar@cs.msu.ru>
Some InfiniBand network devices have multiple ports on the same PCI
function. Prior to this the kernel erroneously used the `dev_id' sysfs
field of those network interfaces to convey the port number to userspace.
`dev_id' is currently reserved for distinguishing stacked ifaces
(e.g: VLANs) with the same hardware address as their parent device.
Similar fixes to net/mlx4_en and many other drivers, which started
exporting this information through `dev_id' before 3.15, were accepted
into the kernel 4 years ago.
See 76a066f2a2a0268b565459c417b59724b5a3197b, commit message:
`net/mlx4_en: Expose port number through sysfs'.
I would be OK with this commit not being backported to stable, since
it might break admin-supplied udev rules and the likes.
Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 6eb0594fffec..f64535038147 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2252,7 +2252,6 @@ static struct net_device *ipoib_add_port(const char *format,
}
SET_NETDEV_DEV(priv->dev, hca->dev.parent);
- priv->dev->dev_id = port - 1;
priv->dev->dev_port = port - 1;
result = ib_query_port(hca, port, &attr);
--
2.18.0
^ permalink raw reply related
* Re: [PATCH bpf-next V3] net/xdp: Fix suspicious RCU usage warning
From: Jesper Dangaard Brouer @ 2018-08-13 12:02 UTC (permalink / raw)
To: Tariq Toukan
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, Eran Ben Elisha,
Neil Brown, Paul E. McKenney, brouer
In-Reply-To: <85462631-9e86-5681-3ee7-558079d4776c@mellanox.com>
On Mon, 13 Aug 2018 13:57:04 +0300
Tariq Toukan <tariqt@mellanox.com> wrote:
> On 13/08/2018 1:31 PM, Jesper Dangaard Brouer wrote:
> > On Mon, 13 Aug 2018 12:21:58 +0300
> > Tariq Toukan <tariqt@mellanox.com> wrote:
> >
> >> Fix the warning below by calling rhashtable_lookup_fast.
> >> Also, make some code movements for better quality and human
> >> readability.
> >>
> >> [ 342.450870] WARNING: suspicious RCU usage
> >> [ 342.455856] 4.18.0-rc2+ #17 Tainted: G O
> >> [ 342.462210] -----------------------------
> >> [ 342.467202] ./include/linux/rhashtable.h:481 suspicious rcu_dereference_check() usage!
> >> [ 342.476568]
> >> [ 342.476568] other info that might help us debug this:
> >> [ 342.476568]
> >> [ 342.486978]
> >> [ 342.486978] rcu_scheduler_active = 2, debug_locks = 1
> >> [ 342.495211] 4 locks held by modprobe/3934:
> >> [ 342.500265] #0: 00000000e23116b2 (mlx5_intf_mutex){+.+.}, at:
> >> mlx5_unregister_interface+0x18/0x90 [mlx5_core]
> >> [ 342.511953] #1: 00000000ca16db96 (rtnl_mutex){+.+.}, at: unregister_netdev+0xe/0x20
> >> [ 342.521109] #2: 00000000a46e2c4b (&priv->state_lock){+.+.}, at: mlx5e_close+0x29/0x60
> >> [mlx5_core]
> >> [ 342.531642] #3: 0000000060c5bde3 (mem_id_lock){+.+.}, at: xdp_rxq_info_unreg+0x93/0x6b0
> >> [ 342.541206]
> >> [ 342.541206] stack backtrace:
> >> [ 342.547075] CPU: 12 PID: 3934 Comm: modprobe Tainted: G O 4.18.0-rc2+ #17
> >> [ 342.556621] Hardware name: Dell Inc. PowerEdge R730/0H21J3, BIOS 1.5.4 10/002/2015
> >> [ 342.565606] Call Trace:
> >> [ 342.568861] dump_stack+0x78/0xb3
> >> [ 342.573086] xdp_rxq_info_unreg+0x3f5/0x6b0
> >> [ 342.578285] ? __call_rcu+0x220/0x300
> >> [ 342.582911] mlx5e_free_rq+0x38/0xc0 [mlx5_core]
> >> [ 342.588602] mlx5e_close_channel+0x20/0x120 [mlx5_core]
> >> [ 342.594976] mlx5e_close_channels+0x26/0x40 [mlx5_core]
> >> [ 342.601345] mlx5e_close_locked+0x44/0x50 [mlx5_core]
> >> [ 342.607519] mlx5e_close+0x42/0x60 [mlx5_core]
> >> [ 342.613005] __dev_close_many+0xb1/0x120
> >> [ 342.617911] dev_close_many+0xa2/0x170
> >> [ 342.622622] rollback_registered_many+0x148/0x460
> >> [ 342.628401] ? __lock_acquire+0x48d/0x11b0
> >> [ 342.633498] ? unregister_netdev+0xe/0x20
> >> [ 342.638495] rollback_registered+0x56/0x90
> >> [ 342.643588] unregister_netdevice_queue+0x7e/0x100
> >> [ 342.649461] unregister_netdev+0x18/0x20
> >> [ 342.654362] mlx5e_remove+0x2a/0x50 [mlx5_core]
> >> [ 342.659944] mlx5_remove_device+0xe5/0x110 [mlx5_core]
> >> [ 342.666208] mlx5_unregister_interface+0x39/0x90 [mlx5_core]
> >> [ 342.673038] cleanup+0x5/0xbfc [mlx5_core]
> >> [ 342.678094] __x64_sys_delete_module+0x16b/0x240
> >> [ 342.683725] ? do_syscall_64+0x1c/0x210
> >> [ 342.688476] do_syscall_64+0x5a/0x210
> >> [ 342.693025] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >>
> >> Fixes: 8d5d88527587 ("xdp: rhashtable with allocator ID to pointer mapping")
> >> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
> >> Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
> >> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> >> ---
> >> net/core/xdp.c | 13 +++----------
> >> 1 file changed, 3 insertions(+), 10 deletions(-)
> >>
> >> V2 -> V3:
> >> * Fix return value test for rhashtable_remove_fast, per Jesper's comment.
> >>
> >> V1 -> V2:
> >> * Use rhashtable_lookup_fast and make some code movements, per Daniel's
> >> and Alexei's comments.
> >>
> >> Please queue to -stable v4.18.
> >>
> >> diff --git a/net/core/xdp.c b/net/core/xdp.c
> >> index 3dd99e1c04f5..8b1c7b699982 100644
> >> --- a/net/core/xdp.c
> >> +++ b/net/core/xdp.c
> >> @@ -105,16 +105,9 @@ static void __xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq)
> >>
> >> mutex_lock(&mem_id_lock);
> >>
> >> - xa = rhashtable_lookup(mem_id_ht, &id, mem_id_rht_params);
> >> - if (!xa) {
> >> - mutex_unlock(&mem_id_lock);
> >> - return;
> >> - }
> >> -
> >> - err = rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params);
> >> - WARN_ON(err);
> >> -
> >> - call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free);
> >> + xa = rhashtable_lookup_fast(mem_id_ht, &id, mem_id_rht_params);
> >> + if (xa && !rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params))
> >> + call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free);
> >>
> >> mutex_unlock(&mem_id_lock);
> >> }
> >
> > I would personally prefer to write it as in the example as "== 0", look
> > at example in [1] section "Object removal", but is it semantically the
> > same to write !rhashtable_remove_fast(). So, I'm fine with this.
> >
>
> I thought the coding convention is to not explicitly compare to zero,
> just like we do not compare to NULL on page allocation, but do:
> if (!page)
If the return value is a pointer, then I use the (!ptr) check, and also
if the return value is a bool. In this case where the success is 0, I
find it slightly confusing to read if(!remove) then success-case.
> But I don't mind changing this one.
I also don't care much... if you do respin, it would be nice to do.
> > In the example[1], the sequence is wrapped in rcu_read_lock/unlock,
> > while you have not done so. The rhashtable_lookup_fast and
> > rhashtable_remove_fast calls have their own rcu_read_lock/unlock,
> > but the outer rcu_read_lock/unlock, makes sure that a RCU period
> > cannot slip in between the two calls.
> >
> > I still think your fix is correct, due to the mutex_lock. Given the
> > mutex sync removal and insert in this rhashtable.
> >
>
> Right, we rely here on the mutex to avoid the scenario you described.
> So the outer rcu lock calls are not necessary.
>
> > I do wonder if it would be better to add the outer
> > rcu_read_lock/unlock, calls if someone else reads and copy-paste
> > this code (and don't have an mutex sync scheme) ?
> >
>
> Yeah it'll be safer for future unaware developers, but I think
> reviewers should always comment and make it clear that the best
> generic reference is [1], not any specific/optimized use case.
>
> If you guys still want to me to fix this then please let me know and
> I'll re-spin.
I'll let Daniel make the choice.
> > If you think this is all fine, and want to proceed as is then you
> > have my ack:
> >
> > Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> >
> >
> > [1] https://lwn.net/Articles/751374/
> >
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH] net: Change the layout of structure trace_event_raw_fib_table_lookup
From: David Ahern @ 2018-08-13 14:40 UTC (permalink / raw)
To: Zong Li, rostedt, mingo, netdev, linux-kernel; +Cc: zongbox, greentime
In-Reply-To: <1534127212-13186-1-git-send-email-zong@andestech.com>
On 8/12/18 8:26 PM, Zong Li wrote:
> There is an unalignment access about the structure
> 'trace_event_raw_fib_table_lookup'.
>
> In include/trace/events/fib.h, there is a memory operation which casting
> the 'src' data member to a pointer, and then store a value to this
> pointer point to.
>
> p32 = (__be32 *) __entry->src;
> *p32 = flp->saddr;
>
> The offset of 'src' in structure trace_event_raw_fib_table_lookup is not
> four bytes alignment. On some architectures, they don't permit the
> unalignment access, it need to pay the price to handle this situation in
> exception handler.
>
> Adjust the layout of structure to avoid this case.
>
> Signed-off-by: Zong Li <zong@andestech.com>
> ---
> include/trace/events/fib.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Fixes: 9f323973c915 ("net/ipv4: Udate fib_table_lookup tracepoint")
Acked-by: David Ahern <dsahern@gmail.com>
Do you also need a similar change to fib6_table_lookup in
include/trace/events/fib6.h?
^ permalink raw reply
* [PATCH v2 iproute2-next] sch_cake: Make gso-splitting configurable
From: Toke Høiland-Jørgensen @ 2018-08-13 11:36 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, Toke Høiland-Jørgensen, Dave Taht
This patch makes sch_cake's gso/gro splitting configurable
from userspace.
To disable breaking apart superpackets in sch_cake:
tc qdisc replace dev whatever root cake no-split-gso
to enable:
tc qdisc replace dev whatever root cake split-gso
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
Signed-off-by: Dave Taht <dave.taht@gmail.com>
---
v2:
- Fix x-mas tree variable order
- Also update man page
man/man8/tc-cake.8 | 23 +++++++++++++++++++++++
tc/q_cake.c | 9 +++++++++
2 files changed, 32 insertions(+)
diff --git a/man/man8/tc-cake.8 b/man/man8/tc-cake.8
index 0e84bc6e..b8e3089b 100644
--- a/man/man8/tc-cake.8
+++ b/man/man8/tc-cake.8
@@ -73,6 +73,12 @@ TIME |
]
.br
[
+.BR split-gso*
+|
+.BR no-split-gso
+]
+.br
+[
.BR ack-filter
|
.BR ack-filter-aggressive
@@ -546,6 +552,23 @@ If you are shaping inbound, and cannot trust the diffserv markings (as is the
case for Comcast Cable, among others), it is best to use a single queue
"besteffort" mode with wash.
+.PP
+.B split-gso
+
+.br
+ This option controls whether CAKE will split General Segmentation
+Offload (GSO) super-packets into their on-the-wire components and
+dequeue them individually.
+
+.br
+Super-packets are created by the networking stack to improve efficiency.
+However, because they are larger they take longer to dequeue, which
+translates to higher latency for competing flows, especially at lower
+bandwidths. CAKE defaults to splitting GSO packets to achieve the lowest
+possible latency. At link speeds higher than 10 Gbps, setting the
+no-split-gso parameter can increase the maximum achievable throughput by
+retaining the full GSO packets.
+
.SH EXAMPLES
# tc qdisc delete root dev eth0
.br
diff --git a/tc/q_cake.c b/tc/q_cake.c
index f1e232a6..50de46a7 100644
--- a/tc/q_cake.c
+++ b/tc/q_cake.c
@@ -79,6 +79,7 @@ static void explain(void)
" dual-srchost | dual-dsthost | triple-isolate* ]\n"
" [ nat | nonat* ]\n"
" [ wash | nowash* ]\n"
+" [ split-gso* | no-split-gso ]\n"
" [ ack-filter | ack-filter-aggressive | no-ack-filter* ]\n"
" [ memlimit LIMIT ]\n"
" [ ptm | atm | noatm* ] [ overhead N | conservative | raw* ]\n"
@@ -99,6 +100,7 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
__u64 bandwidth = 0;
int ack_filter = -1;
struct rtattr *tail;
+ int split_gso = -1;
int unlimited = 0;
int flowmode = -1;
int autorate = -1;
@@ -155,6 +157,10 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
wash = 0;
} else if (strcmp(*argv, "wash") == 0) {
wash = 1;
+ } else if (strcmp(*argv, "split-gso") == 0) {
+ split_gso = 1;
+ } else if (strcmp(*argv, "no-split-gso") == 0) {
+ split_gso = 0;
} else if (strcmp(*argv, "flowblind") == 0) {
flowmode = CAKE_FLOW_NONE;
} else if (strcmp(*argv, "srchost") == 0) {
@@ -374,6 +380,9 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
addattr_l(n, 1024, TCA_CAKE_NAT, &nat, sizeof(nat));
if (wash != -1)
addattr_l(n, 1024, TCA_CAKE_WASH, &wash, sizeof(wash));
+ if (split_gso != -1)
+ addattr_l(n, 1024, TCA_CAKE_SPLIT_GSO, &split_gso,
+ sizeof(split_gso));
if (ingress != -1)
addattr_l(n, 1024, TCA_CAKE_INGRESS, &ingress, sizeof(ingress));
if (ack_filter != -1)
--
2.18.0
^ permalink raw reply related
* [PATCH][bpf-next] bpf: test: fix spelling mistake "REUSEEPORT" -> "REUSEPORT"
From: Colin King @ 2018-08-13 14:00 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Shuah Khan, netdev,
linux-kselftest
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Trivial fix to spelling mistake in error message
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
tools/testing/selftests/bpf/test_maps.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index 4b7c74f5faa7..6f54f84144a0 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -1191,7 +1191,7 @@ static void prepare_reuseport_grp(int type, int map_fd,
err = setsockopt(fd64, SOL_SOCKET, SO_REUSEPORT,
&optval, sizeof(optval));
- CHECK(err == -1, "setsockopt(SO_REUSEEPORT)",
+ CHECK(err == -1, "setsockopt(SO_REUSEPORT)",
"err:%d errno:%d\n", err, errno);
/* reuseport_array does not allow unbound sk */
--
2.17.1
^ permalink raw reply related
* Re: [Query]: DSA Understanding
From: Lad, Prabhakar @ 2018-08-13 11:06 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Andrew Lunn, netdev
In-Reply-To: <d54e8489-6ba0-2ad9-4eb5-c8d3f847f34a@gmail.com>
Hi Florian,
On Fri, Aug 10, 2018 at 6:36 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 08/10/2018 04:26 AM, Lad, Prabhakar wrote:
> > Hi Andrew,
> >
> > On Thu, Aug 9, 2018 at 6:23 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >>
> >>> Its coming from the switch lan4 I have attached the png, where
> >>> C4:F3:12:08:FE:7F is
> >>> the mac of lan4, which is broadcast to ff:ff:ff:ff:ff:ff, which is
> >>> causing rx counter on
> >>> PC to go up.
> >>
> >> So, big packets are making it from the switch to the PC. But the small
> >> ARP packets are not.
> >>
> >> This is what Florian was suggesting.
> >>
> >> ARP packets are smaller than 64 bytes, which is the minimum packet
> >> size for Ethernet. Any packets smaller than 64 bytes are called runt
> >> packets. They have to be padded upto 64 bytes in order to make them
> >> valid. Otherwise the destination, or any switch along the path, might
> >> throw them away.
> >>
> >> What could be happening is that the CSPW driver or hardware is padding
> >> the packet to 64 bytes. But that packet has a DSA header in it. The
> >> switch removes the header, recalculate the checksum and sends the
> >> packet. It is now either 4 or 8 bytes smaller, depending on what DSA
> >> header was used. It then becomes a runt packet.
> >>
> > Thank you for the clarification, this really helped me out.
> >
> >> Florian had to fix this problem recently.
> >>
> >> http://patchwork.ozlabs.org/patch/836534/
> >>
> > But seems like this patch was never accepted, instead
> > brcm_tag_xmit_ll() does it if I am understanding it correctly.
> > similarly to this ksz_xmit() is taking care of padding.
>
> net/dsa/tag_brcm.c ended up doing the padding because that was a more
> generic and central location:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/net/dsa/tag_brcm.c#n73
>
> >
> >> You probably need something similar for the cpsw.
> >>
> > looking at the tag_ksz.c in xmit function this is taken care of
>
> I agree, this should be padding packets correctly, can you still
> instrument cpsw to make sure that what comes to its ndo_start_xmit() is
> ETH_ZLEN + tag_len or more?
>
Yes I can confirm the skb->len is always >= 62 (ETH_ZLEN + 2)
Cheers,
--Prabhakar
^ permalink raw reply
* Re: [PATCH bpf-next V3] net/xdp: Fix suspicious RCU usage warning
From: Tariq Toukan @ 2018-08-13 10:57 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Tariq Toukan
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, Eran Ben Elisha,
Neil Brown, Paul E. McKenney
In-Reply-To: <20180813123159.1f447108@redhat.com>
On 13/08/2018 1:31 PM, Jesper Dangaard Brouer wrote:
> On Mon, 13 Aug 2018 12:21:58 +0300
> Tariq Toukan <tariqt@mellanox.com> wrote:
>
>> Fix the warning below by calling rhashtable_lookup_fast.
>> Also, make some code movements for better quality and human
>> readability.
>>
>> [ 342.450870] WARNING: suspicious RCU usage
>> [ 342.455856] 4.18.0-rc2+ #17 Tainted: G O
>> [ 342.462210] -----------------------------
>> [ 342.467202] ./include/linux/rhashtable.h:481 suspicious rcu_dereference_check() usage!
>> [ 342.476568]
>> [ 342.476568] other info that might help us debug this:
>> [ 342.476568]
>> [ 342.486978]
>> [ 342.486978] rcu_scheduler_active = 2, debug_locks = 1
>> [ 342.495211] 4 locks held by modprobe/3934:
>> [ 342.500265] #0: 00000000e23116b2 (mlx5_intf_mutex){+.+.}, at:
>> mlx5_unregister_interface+0x18/0x90 [mlx5_core]
>> [ 342.511953] #1: 00000000ca16db96 (rtnl_mutex){+.+.}, at: unregister_netdev+0xe/0x20
>> [ 342.521109] #2: 00000000a46e2c4b (&priv->state_lock){+.+.}, at: mlx5e_close+0x29/0x60
>> [mlx5_core]
>> [ 342.531642] #3: 0000000060c5bde3 (mem_id_lock){+.+.}, at: xdp_rxq_info_unreg+0x93/0x6b0
>> [ 342.541206]
>> [ 342.541206] stack backtrace:
>> [ 342.547075] CPU: 12 PID: 3934 Comm: modprobe Tainted: G O 4.18.0-rc2+ #17
>> [ 342.556621] Hardware name: Dell Inc. PowerEdge R730/0H21J3, BIOS 1.5.4 10/002/2015
>> [ 342.565606] Call Trace:
>> [ 342.568861] dump_stack+0x78/0xb3
>> [ 342.573086] xdp_rxq_info_unreg+0x3f5/0x6b0
>> [ 342.578285] ? __call_rcu+0x220/0x300
>> [ 342.582911] mlx5e_free_rq+0x38/0xc0 [mlx5_core]
>> [ 342.588602] mlx5e_close_channel+0x20/0x120 [mlx5_core]
>> [ 342.594976] mlx5e_close_channels+0x26/0x40 [mlx5_core]
>> [ 342.601345] mlx5e_close_locked+0x44/0x50 [mlx5_core]
>> [ 342.607519] mlx5e_close+0x42/0x60 [mlx5_core]
>> [ 342.613005] __dev_close_many+0xb1/0x120
>> [ 342.617911] dev_close_many+0xa2/0x170
>> [ 342.622622] rollback_registered_many+0x148/0x460
>> [ 342.628401] ? __lock_acquire+0x48d/0x11b0
>> [ 342.633498] ? unregister_netdev+0xe/0x20
>> [ 342.638495] rollback_registered+0x56/0x90
>> [ 342.643588] unregister_netdevice_queue+0x7e/0x100
>> [ 342.649461] unregister_netdev+0x18/0x20
>> [ 342.654362] mlx5e_remove+0x2a/0x50 [mlx5_core]
>> [ 342.659944] mlx5_remove_device+0xe5/0x110 [mlx5_core]
>> [ 342.666208] mlx5_unregister_interface+0x39/0x90 [mlx5_core]
>> [ 342.673038] cleanup+0x5/0xbfc [mlx5_core]
>> [ 342.678094] __x64_sys_delete_module+0x16b/0x240
>> [ 342.683725] ? do_syscall_64+0x1c/0x210
>> [ 342.688476] do_syscall_64+0x5a/0x210
>> [ 342.693025] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>> Fixes: 8d5d88527587 ("xdp: rhashtable with allocator ID to pointer mapping")
>> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
>> Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
>> ---
>> net/core/xdp.c | 13 +++----------
>> 1 file changed, 3 insertions(+), 10 deletions(-)
>>
>> V2 -> V3:
>> * Fix return value test for rhashtable_remove_fast, per Jesper's comment.
>>
>> V1 -> V2:
>> * Use rhashtable_lookup_fast and make some code movements, per Daniel's
>> and Alexei's comments.
>>
>> Please queue to -stable v4.18.
>>
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index 3dd99e1c04f5..8b1c7b699982 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -105,16 +105,9 @@ static void __xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq)
>>
>> mutex_lock(&mem_id_lock);
>>
>> - xa = rhashtable_lookup(mem_id_ht, &id, mem_id_rht_params);
>> - if (!xa) {
>> - mutex_unlock(&mem_id_lock);
>> - return;
>> - }
>> -
>> - err = rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params);
>> - WARN_ON(err);
>> -
>> - call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free);
>> + xa = rhashtable_lookup_fast(mem_id_ht, &id, mem_id_rht_params);
>> + if (xa && !rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params))
>> + call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free);
>>
>> mutex_unlock(&mem_id_lock);
>> }
>
> I would personally prefer to write it as in the example as "== 0", look
> at example in [1] section "Object removal", but is it semantically the
> same to write !rhashtable_remove_fast(). So, I'm fine with this.
>
I thought the coding convention is to not explicitly compare to zero,
just like we do not compare to NULL on page allocation, but do:
if (!page)
But I don't mind changing this one.
> In the example[1], the sequence is wrapped in rcu_read_lock/unlock,
> while you have not done so. The rhashtable_lookup_fast and
> rhashtable_remove_fast calls have their own rcu_read_lock/unlock, but
> the outer rcu_read_lock/unlock, makes sure that a RCU period cannot
> slip in between the two calls.
>
> I still think your fix is correct, due to the mutex_lock. Given the
> mutex sync removal and insert in this rhashtable.
>
Right, we rely here on the mutex to avoid the scenario you described.
So the outer rcu lock calls are not necessary.
> I do wonder if it would be better to add the outer rcu_read_lock/unlock,
> calls if someone else reads and copy-paste this code (and don't have an
> mutex sync scheme) ?
>
Yeah it'll be safer for future unaware developers, but I think reviewers
should always comment and make it clear that the best generic reference
is [1], not any specific/optimized use case.
If you guys still want to me to fix this then please let me know and
I'll re-spin.
> If you think this is all fine, and want to proceed as is then you have
> my ack:
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
>
> [1] https://lwn.net/Articles/751374/
>
^ 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