* Re: [RFC PATCH net-next v3 15/21] ethtool: provide link settings and link modes in GET_SETTINGS request
From: Florian Fainelli @ 2019-02-21 3:14 UTC (permalink / raw)
To: Michal Kubecek, netdev
Cc: David Miller, Andrew Lunn, Jakub Kicinski, Jiri Pirko,
linux-kernel
In-Reply-To: <00e931c1ecd29bf302a190c7d7a3f2cbd0388542.1550513384.git.mkubecek@suse.cz>
On 2/18/2019 10:22 AM, Michal Kubecek wrote:
> Implement GET_SETTINGS netlink request to get link settings and link mode
> information provided by ETHTOOL_GLINKSETTINGS ioctl command.
>
> The information is divided into two parts: supported, advertised and peer
> advertised link modes when ETH_SETTINGS_IM_LINKMODES flag is set in the
> request and other settings when ETH_SETTINGS_IM_LINKINFO is set.
>
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> ---
[snip]
> +#define ETH_SETTINGS_IM_LINKINFO 0x01
> +#define ETH_SETTINGS_IM_LINKMODES 0x02
> +
> +#define ETH_SETTINGS_IM_ALL 0x03
You could define ETH_SETTINGS_IM_ALL as:
#define ETH_SETTING_IM_ALL \
(ETH_SETTINGS_IM_LINKINFO |
ETH_SETTINGS_IM_LINMODES)
that would scale better IMHO, especially given that you have to keep
bumping that mask with new bits in subsequent patches.
[snip]
> + if (tb[ETHA_SETTINGS_INFOMASK])
> + req_info->req_mask = nla_get_u32(tb[ETHA_SETTINGS_INFOMASK]);
> + if (tb[ETHA_SETTINGS_COMPACT])
> + req_info->compact = true;
> + if (req_info->req_mask == 0)
> + req_info->req_mask = ETH_SETTINGS_IM_ALL;
What if userland is newer than the kernel and specifies a req_mask with
bits set that you don't support? Should not you always do an &
ETH_SETTINGS_IM_ALL here?
[snip]
--
Florian
^ permalink raw reply
* Re: [PATCH bpf-next 1/9] bpf: Add bpf helper bpf_tcp_enter_cwr
From: Lawrence Brakmo @ 2019-02-21 3:18 UTC (permalink / raw)
To: Eric Dumazet, netdev; +Cc: Martin Lau, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <67c8aaa3-81a5-b1c2-5be4-938a139f3013@gmail.com>
On 2/19/19, 10:30 AM, "Eric Dumazet" <eric.dumazet@gmail.com> wrote:
On 02/18/2019 09:38 PM, brakmo wrote:
> This patch adds a new bpf helper BPF_FUNC_tcp_enter_cwr
> "int bpf_tcp_enter_cwr(struct bpf_tcp_sock *tp)".
> It is added to BPF_PROG_TYPE_CGROUP_SKB typed bpf_prog
> which currently can be attached to the ingress and egress
> path.
>
Do we have the guarantee socket is a tcp one, and that the caller
owns the socket lock ?
Yes. The BPF verifier insures that a pointer to bpf_tcp_sock points to a non-NULL full tcp socket.
Please describe the exact context for this helper being used.
From cgroup skb egress bpf program. When the BPF program determines that a flow needs to slow down, it will call bpf_tcp_enter_cwr(tp).
^ permalink raw reply
* Re: [PATCH net-next 3/3] nfp: devlink: allow flashing the device via devlink
From: Jakub Kicinski @ 2019-02-21 3:20 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Jiri Pirko, davem, netdev, oss-drivers, mkubecek, andrew
In-Reply-To: <befeae18-f80e-6f2f-3e97-28629200cf92@gmail.com>
On Wed, 20 Feb 2019 18:59:05 -0800, Florian Fainelli wrote:
> On 2/19/2019 4:49 PM, Jakub Kicinski wrote:
> > On Tue, 19 Feb 2019 10:19:42 +0100, Jiri Pirko wrote:
> >> Fri, Feb 15, 2019 at 04:44:29PM CET, jakub.kicinski@netronome.com wrote:
> >>> On Fri, 15 Feb 2019 11:15:14 +0100, Jiri Pirko wrote:
> >>>>> static const struct ethtool_ops nfp_net_ethtool_ops = {
> >>>>
> >>>> Why don't you use the compat fallback? I think you should.
> >>>
> >>> You and Michal both asked the same so let me answer the first to ask :)
> >>> - if devlink is built as a module the fallback is not reachable.
> >>
> >> So the fallback is not really good as you can't use it for real drivers
> >> anyway. Odd. Maybe we should compile devlink in without possibility to
> >> have it as module.
> >
> > Ack, I'll make devlink a bool.
>
> Meh how about those poor and memory constrained embedded systems?
> Ideally ethtool should/could have been modular as well, but that ship
> has now sailed.
To be clear - I'm not going to add a dependency. You'll still be able
to use ethtool without devlink (granted, you won't be able to flash the
device).
It's not immediately clear to me how devlink being modular saves memory.
The way I see it you either:
(a) not have networking or need any devlink related functionality, and
therefore do DEVLINK=n;
or:
(b) have a networking device driver built and loaded which will depend
on devlink, so devlink will practically speaking always be loaded,
even if its =m.
> > I need a little extra time, I forgot that nfp's flower offload still
> > doesn't register all ports (using your port flavour infrastructure).
>
> We have had similar issues with PHYLIB before where we wanted
> net/core/ethtool.c to be able to call into generic PHYLIB functions to
> obtain PHY statistics, an inline helper that de-references the PHY
> device's driver function pointers solved that (look for
> phy_ethtool_get_{strings,sset,stats}) while letting PHYLIB remain modular.
>
> devlink_compat_flash_update() is a bit big to be inlined, but why not?
>
> If we make sure we always provide a devlink_mutex and devlink_list that
> symbols such that this builds wheter CONFIG_DEVLINK=y|m then everything
> else can be determined at runtime whether devlink.ko is loaded or not.
>
> Does that make sense?
I think so, we'd have to have a little object that would always be
built-in when DEVLINK=m, and therefore accessible?
^ permalink raw reply
* Re: [RFC PATCH net-next v3 00/21] ethtool netlink interface, part 1
From: Florian Fainelli @ 2019-02-21 3:21 UTC (permalink / raw)
To: Michal Kubecek, netdev
Cc: David Miller, Andrew Lunn, Jakub Kicinski, Jiri Pirko,
linux-kernel
In-Reply-To: <cover.1550513384.git.mkubecek@suse.cz>
Hi Michal,
Let me start with a big thank you for tackling this humongous amount of
work!
On 2/18/2019 10:21 AM, Michal Kubecek wrote:
> Note: this is marked as RFC because it's rather late in the cycle; the plan
> is to make a regular submission (with changes based on review) once
> net-next reopens after the 5.1 merge window. The full (work in progress)
> series, together with the (userspace) ethtool counterpart can be found at
> https://github.com/mkubecek/ethnl
>
> This is first part of alternative userspace interface for ethtool. The aim
> is to address some long known issues with the ioctl interface, mainly lack
> of extensibility, raciness, limited error reporting and absence of
> notifications.
>
> The interface uses generic netlink family "ethtool"; it provides multicast
> group "monitor" which is used for notifications. Documentation for the
> interface is in Documentation/networking/ethtool-netlink.txt
>
> Basic concepts:
>
> - the goal is to provide all features of ioctl interface but allow
> easier future extensions; at some point, it should be possible to have
> full ethtool functionality without using the ioctl interface
+1
> - inextensibility of ioctl interface resulted in way too many commands,
> many of them obsoleted by newer ones; reduce the number by ignoring the
> obsolete commands and grouping some together
> - for "set" type commands, allows passing only the attributes to be
> changed; therefore we don't need a get-modify-set cycle (which is
> inherently racy), userspace can simply say what it wants to change
> - provide notifications to multicast group "monitor" like rtnetlink does,
> i.e. in the form of messages close to replies to "get" requests
+1
> - allow dump requests to get some information about all network devices
> providing it
And it seems like you have also added some filtering capability with at
least the settings dump, right? That is also highly welcome.
> - be less dependent on ethtool and kernel being in sync; allow e.g. saying
> "ethtool -s eth0 advertise foo off" without ethtool knowing what "foo"
> means; it's kernel's job to know what mode "xyz" is and if it exists and
> is supported
>
> Main changes between RFC v2 and RFC v3:
>
> - do not allow building as a module (no netdev notifiers needed)
I would very much prefer that we could build this as a module. Some
systems might be memory constrained or opt to disable ethtool entirely
(security?). If this is not too much trouble, can we try to maintain that?
> - drop some obsolete fields
> - add permanent hw address, timestamping and private flags support
> - rework bitset handling to get rid of variable length arrays
> - notify monitor on device renames
> - restructure GET_SETTINGS/SET_SETTINGS messages
> - split too long patches and submit only first part of the series
>
> Main changes between RFC v1 and RFC v2:
>
> - support dumps for all "get" requests
> - provide notifications for changes related to supported request types
> - support getting string sets (both global and per device)
> - support getting/setting device features
> - get rid of family specific header, everything passed as attributes
> - split netlink code into multiple files in net/ethtool/ directory
>
> ToDo / open questions:
>
> - some features provided by ethtool would rather belong to devlink (and
> some are already superseded by devlink); however, only few drivers
> provide devlink interface at the moment and as recent discussion on
> flashing revealed, we cannot rely on devlink's presence
Should we just move everything under devlink given that ethtool over
netlink or devlink are essentially manipulating the same (or close to)
objects? It seems to me that at least now is the right time to make
decisions about what should stay in ethtool and be offered via netlink,
what should be migrated to something else (e.g.: Jiri mentioned
rtnetlink), and what is a devlink candidate.
How do we proceed to easily triage which of these netlink facilities
should things be routed to/from?
>
> - while the netlink interface allows easy future extensions, ethtool_ops
> interface does not; some settings could be implemented using tunables and
> accessed via relevant netlink messages (as well as tunables) from
> userspace but in the long term, something better will be needed
Right, so as a driver writer, one thing I kind of hate is having to fill
a netlink skb myself because that's a lot of work, and it makes it
fairly hard to do mass conversion of internal APIs due to the increased
complexity in auditing what drivers do.
>
> - currently, all communication with drivers via ethtool_ops is done
> under RTNL as this is what ioctl interface does and likely many
> ethtool_ops handlers rely on that; if we are going to rework ethtool_ops
> in the future ("phase two"), it would be nice to get rid of it
>
> - ethtool_ops should pass extack pointer to allow drivers more meaningful
> error reporting; it's not clear, however, how to pass information about
> offending attribute
>
> - notifications are sent whenever a change is done via netlink API or
> ioctl API and for netdev features also whenever they are updated using
> netdev_change_features(); it would be desirable to notify also about
> link state and negotiation result (speed/duplex and partner link
> modes) but it would be more tricky
Historically you listen for a link event and you determine the link
parameters from there, I don't know if there is much value in giving the
full link parameter list, especially given those could theoretically
change depending on your medium (e.g.: wireless), or there could be a
lot to query at that time...
>
> Michal Kubecek (21):
> netlink: introduce nla_put_bitfield32()
> ethtool: move to its own directory
> ethtool: introduce ethtool netlink interface
> ethtool: helper functions for netlink interface
> ethtool: netlink bitset handling
> ethtool: support for netlink notifications
> ethtool: implement EVENT notifications
> ethtool: generic handlers for GET requests
> ethtool: move string arrays into common file
> ethtool: provide string sets with GET_STRSET request
> ethtool: provide driver/device information in GET_INFO request
> ethtool: provide permanent hardware address in GET_INFO request
> ethtool: provide timestamping information in GET_INFO request
> ethtool: provide link mode names as a string set
> ethtool: provide link settings and link modes in GET_SETTINGS request
> ethtool: provide WoL information in GET_SETTINGS request
> ethtool: provide message level in GET_SETTINGS request
> ethtool: provide link state in GET_SETTINGS request
> ethtool: provide device features in GET_SETTINGS request
> ethtool: provide private flags in GET_SETTINGS request
> ethtool: send netlink notifications about setting changes
>
> Documentation/networking/ethtool-netlink.txt | 441 +++++++++++++
> include/linux/ethtool_netlink.h | 17 +
> include/linux/netdevice.h | 14 +
> include/net/netlink.h | 15 +
> include/uapi/linux/ethtool.h | 10 +
> include/uapi/linux/ethtool_netlink.h | 265 ++++++++
> include/uapi/linux/net_tstamp.h | 13 +
> net/Kconfig | 8 +
> net/Makefile | 2 +-
> net/core/Makefile | 2 +-
> net/ethtool/Makefile | 7 +
> net/ethtool/bitset.c | 572 +++++++++++++++++
> net/ethtool/bitset.h | 40 ++
> net/ethtool/common.c | 227 +++++++
> net/ethtool/common.h | 29 +
> net/ethtool/info.c | 332 ++++++++++
> net/{core/ethtool.c => ethtool/ioctl.c} | 244 ++-----
> net/ethtool/netlink.c | 634 +++++++++++++++++++
> net/ethtool/netlink.h | 252 ++++++++
> net/ethtool/settings.c | 559 ++++++++++++++++
> net/ethtool/strset.c | 461 ++++++++++++++
> 21 files changed, 3937 insertions(+), 207 deletions(-)
> create mode 100644 Documentation/networking/ethtool-netlink.txt
> create mode 100644 include/linux/ethtool_netlink.h
> create mode 100644 include/uapi/linux/ethtool_netlink.h
> create mode 100644 net/ethtool/Makefile
> create mode 100644 net/ethtool/bitset.c
> create mode 100644 net/ethtool/bitset.h
> create mode 100644 net/ethtool/common.c
> create mode 100644 net/ethtool/common.h
> create mode 100644 net/ethtool/info.c
> rename net/{core/ethtool.c => ethtool/ioctl.c} (91%)
> create mode 100644 net/ethtool/netlink.c
> create mode 100644 net/ethtool/netlink.h
> create mode 100644 net/ethtool/settings.c
> create mode 100644 net/ethtool/strset.c
>
--
Florian
^ permalink raw reply
* Re: [RFC PATCH net-next v3 14/21] ethtool: provide link mode names as a string set
From: Florian Fainelli @ 2019-02-21 3:21 UTC (permalink / raw)
To: Michal Kubecek, netdev
Cc: David Miller, Andrew Lunn, Jakub Kicinski, Jiri Pirko,
linux-kernel
In-Reply-To: <e969567a4630812230efa4fd4c7f0233564822d6.1550513384.git.mkubecek@suse.cz>
On 2/18/2019 10:22 AM, Michal Kubecek wrote:
> Add table of ethernet link mode names and make it available as a string set
> to userspace GET_STRSET requests.
>
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> ---
[snip]
>
> +const char *const link_mode_names[] = {
Maybe you can define a macro (totally untested) to build the table an
avoid some of the repetition, something like:
#define ETHTOOL_LINK_NAME(speed, duplex) \
[ETHTOOL_LINK_MODE_##speed_##duplex_BIT] = stringify(speed ## "/" ##
duplex)
--
Florian
^ permalink raw reply
* [tipc-discussion][net v2 1/1] tipc: fix race condition causing hung sendto
From: Tung Nguyen @ 2019-02-21 3:31 UTC (permalink / raw)
To: davem, netdev; +Cc: tipc-discussion
In-Reply-To: <20190219070310.23888-2-tung.q.nguyen@dektech.com.au>
When sending multicast messages via blocking socket,
if sending link is congested (tsk->cong_link_cnt is set to 1),
the sending thread will be put into sleeping state. However,
tipc_sk_filter_rcv() is called under socket spin lock but
tipc_wait_for_cond() is not. So, there is no guarantee that
the setting of tsk->cong_link_cnt to 0 in tipc_sk_proto_rcv() in
CPU-1 will be perceived by CPU-0. If that is the case, the sending
thread in CPU-0 after being waken up, will continue to see
tsk->cong_link_cnt as 1 and put the sending thread into sleeping
state again. The sending thread will sleep forever.
CPU-0 | CPU-1
tipc_wait_for_cond() |
{ |
// condition_ = !tsk->cong_link_cnt |
while ((rc_ = !(condition_))) { |
... |
release_sock(sk_); |
wait_woken(); |
| if (!sock_owned_by_user(sk))
| tipc_sk_filter_rcv()
| {
| ...
| tipc_sk_proto_rcv()
| {
| ...
| tsk->cong_link_cnt--;
| ...
| sk->sk_write_space(sk);
| ...
| }
| ...
| }
sched_annotate_sleep(); |
lock_sock(sk_); |
remove_wait_queue(); |
} |
} |
This commit fixes it by adding memory barrier to tipc_sk_proto_rcv()
and tipc_wait_for_cond().
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Tung Nguyen <tung.q.nguyen@dektech.com.au>
---
net/tipc/socket.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 1217c90a363b..7cfa0c8ccc2a 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -379,16 +379,18 @@ static int tipc_sk_sock_err(struct socket *sock, long *timeout)
#define tipc_wait_for_cond(sock_, timeo_, condition_) \
({ \
+ DEFINE_WAIT_FUNC(wait_, woken_wake_function); \
struct sock *sk_; \
int rc_; \
\
while ((rc_ = !(condition_))) { \
- DEFINE_WAIT_FUNC(wait_, woken_wake_function); \
+ /* coupled with smp_wmb() in tipc_sk_proto_rcv() */ \
+ smp_rmb(); \
sk_ = (sock_)->sk; \
rc_ = tipc_sk_sock_err((sock_), timeo_); \
if (rc_) \
break; \
- prepare_to_wait(sk_sleep(sk_), &wait_, TASK_INTERRUPTIBLE); \
+ add_wait_queue(sk_sleep(sk_), &wait_); \
release_sock(sk_); \
*(timeo_) = wait_woken(&wait_, TASK_INTERRUPTIBLE, *(timeo_)); \
sched_annotate_sleep(); \
@@ -1982,6 +1984,8 @@ static void tipc_sk_proto_rcv(struct sock *sk,
return;
case SOCK_WAKEUP:
tipc_dest_del(&tsk->cong_links, msg_orignode(hdr), 0);
+ /* coupled with smp_rmb() in tipc_wait_for_cond() */
+ smp_wmb();
tsk->cong_link_cnt--;
wakeup = true;
break;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf-next 3/9] bpf: add bpf helper bpf_skb_set_ecn
From: Lawrence Brakmo @ 2019-02-21 3:43 UTC (permalink / raw)
To: Eric Dumazet, netdev; +Cc: Martin Lau, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <318b6330-555f-224c-f4f5-5c635a78c99e@gmail.com>
On 2/19/19, 10:30 AM, "Eric Dumazet" <eric.dumazet@gmail.com> wrote:
On 02/18/2019 09:38 PM, brakmo wrote:
> This patch adds a new bpf helper BPF_FUNC_skb_set_ecn
> "int bpf_skb_set_Ecn(struct sk_buff *skb)". It is added to
> BPF_PROG_TYPE_CGROUP_SKB typed bpf_prog which currently can
> be attached to the ingress and egress path. This type of
> bpf_prog cannot modify the skb directly.
>
> This helper is used to set the ECN bits (2) of the IPv6 or IPv4
> header in skb. It can be used by a bpf_prog to manage egress
> network bandwdith limit per cgroupv2 by inducing an ECN
> response in the TCP sender (when the packet is ECN enabled).
> This works best when using DCTCP.
> +
> +BPF_CALL_2(bpf_skb_set_ecn, struct sk_buff *, skb, u32, val)
> +{
> + struct ipv6hdr *ip6h = ipv6_hdr(skb);
> +
> + if ((val & ~0x3) != 0)
> + return -EINVAL;
> +
> + if (ip6h->version == 6) {
> + ip6h->flow_lbl[0] = (ip6h->flow_lbl[0] & ~0x30) | (val << 4);
> + return 0;
> + } else if (ip6h->version == 4) {
> + struct iphdr *ip4h = (struct iphdr *)ip6h;
> +
> + ip4h->tos = (ip4h->tos & ~0x3) | val;
Why is not the IPv4 checksum recomputed here ?
If you leave this task to the caller, this should be documented.
These hard coded constants are not really nice.
Why not simply using INET_ECN_set_ce() which is IPv4/IPv6 ready ?
Do you really need to set anything else than CE ?
Good point, thank you. I will use it.
^ permalink raw reply
* Re: [tipc-discussion][net v2 1/1] tipc: fix race condition causing hung sendto
From: David Miller @ 2019-02-21 3:51 UTC (permalink / raw)
To: tung.q.nguyen; +Cc: netdev, tipc-discussion
In-Reply-To: <20190221033121.4220-1-tung.q.nguyen@dektech.com.au>
So if the previous version didn't even compile, I have to ask.
How did you test this?
^ permalink raw reply
* Re: [PATCH] net: marvell: mvneta: fix DMA debug warning
From: David Miller @ 2019-02-21 3:57 UTC (permalink / raw)
To: rmk+kernel; +Cc: thomas.petazzoni, netdev
In-Reply-To: <E1gudxz-0001T0-0K@rmk-PC.armlinux.org.uk>
From: Russell King <rmk+kernel@armlinux.org.uk>
Date: Fri, 15 Feb 2019 13:55:47 +0000
> Booting 4.20 on SolidRun Clearfog issues this warning with DMA API
> debug enabled:
...
> This appears to be caused by mvneta_rx_hwbm() calling
> dma_sync_single_range_for_cpu() with the wrong struct device pointer,
> as the buffer manager device pointer is used to map and unmap the
> buffer. Fix this.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> Please check that this is the correct fix.
This makes it consistent with the other DMA calls in the hwbm
code paths, so on that basis I'm applying this and queueing it
up for -stable.
Thanks.
^ permalink raw reply
* Re: [PATCH bpf-next v2] bpf, seccomp: fix false positive preemption splat for cbpf->ebpf progs
From: Alexei Starovoitov @ 2019-02-21 4:02 UTC (permalink / raw)
To: Daniel Borkmann, Jann Horn, Andy Lutomirski
Cc: Alexei Starovoitov, Kees Cook, Network Development
In-Reply-To: <20190220235952.uzrsjypoqkha7ya6@ast-mbp.dhcp.thefacebook.com>
On Wed, Feb 20, 2019 at 3:59 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Feb 21, 2019 at 12:01:35AM +0100, Daniel Borkmann wrote:
> > In 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> > a check was added for BPF_PROG_RUN() that for every invocation preemption is
> > disabled to not break eBPF assumptions (e.g. per-cpu map). Of course this does
> > not count for seccomp because only cBPF -> eBPF is loaded here and it does
> > not make use of any functionality that would require this assertion. Fix this
> > false positive by adding and using SECCOMP_RUN() variant that does not have
> > the cant_sleep(); check.
> >
> > Fixes: 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> > Reported-by: syzbot+8bf19ee2aa580de7a2a7@syzkaller.appspotmail.com
> > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> > Acked-by: Kees Cook <keescook@chromium.org>
>
> Applied, Thanks
Actually I think it's a wrong approach to go long term.
I'm thinking to revert it.
I think it's better to disable preemption for duration of
seccomp cbpf prog.
It's short and there is really no reason for it to be preemptible.
When seccomp switches to ebpf we'll have this weird inconsistency.
Let's just disable preemption for seccomp as well.
^ permalink raw reply
* RE: [tipc-discussion][net v2 1/1] tipc: fix race condition causing hung sendto
From: tung quang nguyen @ 2019-02-21 4:04 UTC (permalink / raw)
To: 'David Miller'; +Cc: netdev, tipc-discussion
In-Reply-To: <20190220.195104.1394349830331192977.davem@davemloft.net>
Hi David,
I compiled previous version and tested it. But I forgot to observe kernel
warning.
For the way to reproduce the issue: calling sendto() and printf() right
after that to print out a log to see if sendto() was successful. On some
occasions, the log was never printed and stack trace showed sendto() was not
returned.
By applying the patch, the issue disappeared.
Thanks.
Best regards,
Tung Nguyen
-----Original Message-----
From: David Miller <davem@davemloft.net>
Sent: Thursday, February 21, 2019 10:51 AM
To: tung.q.nguyen@dektech.com.au
Cc: netdev@vger.kernel.org; tipc-discussion@lists.sourceforge.net
Subject: Re: [tipc-discussion][net v2 1/1] tipc: fix race condition causing
hung sendto
So if the previous version didn't even compile, I have to ask.
How did you test this?
^ permalink raw reply
* Re: [PATCH][unix] missing barriers in some of unix_sock ->addr and ->path accesses
From: David Miller @ 2019-02-21 4:07 UTC (permalink / raw)
To: viro; +Cc: netdev
In-Reply-To: <20190215200934.GM2217@ZenIV.linux.org.uk>
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Fri, 15 Feb 2019 20:09:35 +0000
> Several u->addr and u->path users are not holding any locks in
> common with unix_bind(). unix_state_lock() is useless for those
> purposes.
>
> u->addr is assign-once and *(u->addr) is fully set up by the time
> we set u->addr (all under unix_table_lock). u->path is also
> set in the same critical area, also before setting u->addr, and
> any unix_sock with ->path filled will have non-NULL ->addr.
>
> So setting ->addr with smp_store_release() is all we need for those
> "lockless" users - just have them fetch ->addr with smp_load_acquire()
> and don't even bother looking at ->path if they see NULL ->addr.
>
> Users of ->addr and ->path fall into several classes now:
> 1) ones that do smp_load_acquire(u->addr) and access *(u->addr)
> and u->path only if smp_load_acquire() has returned non-NULL.
> 2) places holding unix_table_lock. These are guaranteed that
> *(u->addr) is seen fully initialized. If unix_sock is in one of the
> "bound" chains, so's ->path.
> 3) unix_sock_destructor() using ->addr is safe. All places
> that set u->addr are guaranteed to have seen all stores *(u->addr)
> while holding a reference to u and unix_sock_destructor() is called
> when (atomic) refcount hits zero.
> 4) unix_release_sock() using ->path is safe. unix_bind()
> is serialized wrt unix_release() (normally - by struct file
> refcount), and for the instances that had ->path set by unix_bind()
> unix_release_sock() comes from unix_release(), so they are fine.
> Instances that had it set in unix_stream_connect() either end up
> attached to a socket (in unix_accept()), in which case the call
> chain to unix_release_sock() and serialization are the same as in
> the previous case, or they never get accept'ed and unix_release_sock()
> is called when the listener is shut down and its queue gets purged.
> In that case the listener's queue lock provides the barriers needed -
> unix_stream_connect() shoves our unix_sock into listener's queue
> under that lock right after having set ->path and eventual
> unix_release_sock() caller picks them from that queue under the
> same lock right before calling unix_release_sock().
> 5) unix_find_other() use of ->path is pointless, but safe -
> it happens with successful lookup by (abstract) name, so ->path.dentry
> is guaranteed to be NULL there.
>
> earlier-variant-reviewed-by: "Paul E. McKenney" <paulmck@linux.ibm.com>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Applied and queued up for -stable, thanks Al.
^ permalink raw reply
* Re: [Patch net-next 1/2] net_sched: fix a race condition in tcindex_destroy()
From: David Miller @ 2019-02-21 4:11 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, bugs, ben, jhs, jiri
In-Reply-To: <20190216185827.21535-1-xiyou.wangcong@gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sat, 16 Feb 2019 10:58:26 -0800
> (cherry picked from commit 8015d93ebd27484418d4952284fd02172fa4b0b2)
>
> tcindex_destroy() invokes tcindex_destroy_element() via
> a walker to delete each filter result in its perfect hash
> table, and tcindex_destroy_element() calls tcindex_delete()
> which schedules tcf RCU works to do the final deletion work.
> Unfortunately this races with the RCU callback
> __tcindex_destroy(), which could lead to use-after-free as
> reported by Adrian.
>
> Fix this by migrating this RCU callback to tcf RCU work too,
> as that workqueue is ordered, we will not have use-after-free.
>
> Note, we don't need to hold netns refcnt because we don't call
> tcf_exts_destroy() here.
>
> Fixes: 27ce4f05e2ab ("net_sched: use tcf_queue_work() in tcindex filter")
> Reported-by: Adrian <bugs@abtelecom.ro>
> Cc: Ben Hutchings <ben@decadent.org.uk>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied.
^ permalink raw reply
* Re: [Patch net-next 2/2] net_sched: fix a memory leak in cls_tcindex
From: David Miller @ 2019-02-21 4:11 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, jhs, jiri
In-Reply-To: <20190216185827.21535-2-xiyou.wangcong@gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sat, 16 Feb 2019 10:58:27 -0800
> (cherry picked from commit 033b228e7f26b29ae37f8bfa1bc6b209a5365e9f)
>
> When tcindex_destroy() destroys all the filter results in
> the perfect hash table, it invokes the walker to delete
> each of them. However, results with class==0 are skipped
> in either tcindex_walk() or tcindex_delete(), which causes
> a memory leak reported by kmemleak.
>
> This patch fixes it by skipping the walker and directly
> deleting these filter results so we don't miss any filter
> result.
>
> As a result of this change, we have to initialize exts->net
> properly in tcindex_alloc_perfect_hash(). For net-next, we
> need to consider whether we should initialize ->net in
> tcf_exts_init() instead, before that just directly test
> CONFIG_NET_CLS_ACT=y.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied.
^ permalink raw reply
* Re: [pull request][net-next 00/11] Mellanox, mlx5 updates 2019-02-19
From: David Miller @ 2019-02-21 4:15 UTC (permalink / raw)
To: saeedm; +Cc: netdev, ariela
In-Reply-To: <20190219221957.2899-1-saeedm@mellanox.com>
From: Saeed Mahameed <saeedm@mellanox.com>
Date: Tue, 19 Feb 2019 14:19:46 -0800
> This series provides some updates to mlx5 driver.
> In addition, there is one patch that defines new 50Gbps per lane link
> modes in include/uapi/linux/ethtool.h:
> ("ethtool: Added support for 50Gbps per lane link modes")
>
> For more information please see tag log below.
>
> Please pull and let me know if there is any problem.
Pulled, thanks Saeed.
^ permalink raw reply
* Re: [PATCH v2 net-next 0/2] net: phy: disable aneg in genphy_c45_pma_setup_forced
From: David Miller @ 2019-02-21 4:19 UTC (permalink / raw)
To: hkallweit1; +Cc: andrew, f.fainelli, linux, netdev
In-Reply-To: <c029e3b6-5fbe-08fb-a012-90ceef3be100@gmail.com>
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sat, 16 Feb 2019 20:39:57 +0100
> When genphy_c45_pma_setup_forced() is called the "aneg enabled" bit
> may still be set, therefore clear it. This is also in line with what
> genphy_setup_forced() does for Clause 22.
>
> v2:
> - fix a typo in patch 1
Applied, thanks Heiner.
^ permalink raw reply
* Re: [PATCH bpf-next v2] bpf, seccomp: fix false positive preemption splat for cbpf->ebpf progs
From: Kees Cook @ 2019-02-21 5:31 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Jann Horn, Andy Lutomirski, Alexei Starovoitov,
Network Development
In-Reply-To: <CAADnVQJYjXKe7NKwjiCDt-tsgejZ1S0ApA4aJUw6se5XsWY5KQ@mail.gmail.com>
On Wed, Feb 20, 2019 at 8:03 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Feb 20, 2019 at 3:59 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, Feb 21, 2019 at 12:01:35AM +0100, Daniel Borkmann wrote:
> > > In 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> > > a check was added for BPF_PROG_RUN() that for every invocation preemption is
> > > disabled to not break eBPF assumptions (e.g. per-cpu map). Of course this does
> > > not count for seccomp because only cBPF -> eBPF is loaded here and it does
> > > not make use of any functionality that would require this assertion. Fix this
> > > false positive by adding and using SECCOMP_RUN() variant that does not have
> > > the cant_sleep(); check.
> > >
> > > Fixes: 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> > > Reported-by: syzbot+8bf19ee2aa580de7a2a7@syzkaller.appspotmail.com
> > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> > > Acked-by: Kees Cook <keescook@chromium.org>
> >
> > Applied, Thanks
>
> Actually I think it's a wrong approach to go long term.
> I'm thinking to revert it.
> I think it's better to disable preemption for duration of
> seccomp cbpf prog.
> It's short and there is really no reason for it to be preemptible.
> When seccomp switches to ebpf we'll have this weird inconsistency.
> Let's just disable preemption for seccomp as well.
A lot of changes will be needed for seccomp ebpf -- not the least of
which is convincing me there is a use-case. ;)
But the main issue is that I'm not a huge fan of dropping two
barriers() across syscall entry. That seems pretty heavy-duty for
something that is literally not needed right now.
--
Kees Cook
^ permalink raw reply
* Re: linux-next: Fixes tag needs some work in the net-next tree
From: Vinod Koul @ 2019-02-21 5:34 UTC (permalink / raw)
To: Stefano Brivio
Cc: David Miller, jiri, sfr, netdev, linux-next, linux-kernel, andrew
In-Reply-To: <20190220205910.7bd7fd40@redhat.com>
On 20-02-19, 20:59, Stefano Brivio wrote:
> On Wed, 20 Feb 2019 11:02:01 -0800 (PST)
> David Miller <davem@davemloft.net> wrote:
>
> > From: Jiri Pirko <jiri@resnulli.us>
> > Date: Wed, 20 Feb 2019 09:36:11 +0100
> >
> > > Would be good to have some robot checking "Fixes" sanity...
> >
> > I want to add a script to my trees that locally do it for me but the
> > backlog for patch review for me is so huge that I never get to "fun"
> > tasks like that....
>
> If it helps, this is what I use after being bitten once:
>
> #!/bin/sh
>
> [ ${#} -ne 2 ] && echo "Usage: %0 PATCH_FILE GIT_TREE" && exit 1
> grep "^Fixes: " "${1}" | while read -r f; do
> sha="$(echo "${f}" | cut -d' ' -f2)"
> if [ -z "${sha}" ] || [ "${f}" != "$(git -C "${2}" show -s --abbrev=12 --pretty=format:"Fixes: %h (\"%s\")" "${sha}" 2>/dev/null)" ]; then
> echo "Bad tag: ${f}" && exit 1
> fi
> done
Awesome thanks, I am adding this into my patch commit script and well as
send script
--
~Vinod
^ permalink raw reply
* [Patch net-next] net_sched: initialize net pointer inside tcf_exts_init()
From: Cong Wang @ 2019-02-21 5:37 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Jamal Hadi Salim, Jiri Pirko
For tcindex filter, it is too late to initialize the
net pointer in tcf_exts_validate(), as tcf_exts_get_net()
requires a non-NULL net pointer. We can just move its
initialization into tcf_exts_init(), which just requires
an additional parameter.
This makes the code in tcindex_alloc_perfect_hash()
prettier.
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
include/net/pkt_cls.h | 5 +++--
net/sched/cls_api.c | 1 -
net/sched/cls_basic.c | 2 +-
net/sched/cls_bpf.c | 2 +-
net/sched/cls_cgroup.c | 2 +-
net/sched/cls_flow.c | 2 +-
net/sched/cls_flower.c | 2 +-
net/sched/cls_fw.c | 5 +++--
net/sched/cls_matchall.c | 2 +-
net/sched/cls_route.c | 2 +-
net/sched/cls_rsvp.h | 7 ++++---
net/sched/cls_tcindex.c | 19 +++++++++----------
net/sched/cls_u32.c | 8 ++++----
13 files changed, 30 insertions(+), 29 deletions(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 6a530bef9253..58ea48e1221c 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -289,12 +289,13 @@ struct tcf_exts {
int police;
};
-static inline int tcf_exts_init(struct tcf_exts *exts, int action, int police)
+static inline int tcf_exts_init(struct tcf_exts *exts, struct net *net,
+ int action, int police)
{
#ifdef CONFIG_NET_CLS_ACT
exts->type = 0;
exts->nr_actions = 0;
- exts->net = NULL;
+ exts->net = net;
exts->actions = kcalloc(TCA_ACT_MAX_PRIO, sizeof(struct tc_action *),
GFP_KERNEL);
if (!exts->actions)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 9ad53895e604..2ca2bcd7da1c 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3049,7 +3049,6 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
return err;
exts->nr_actions = err;
}
- exts->net = net;
}
#else
if ((exts->action && tb[exts->action]) ||
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index 2383f449d2bc..687b0af67878 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -199,7 +199,7 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
if (!fnew)
return -ENOBUFS;
- err = tcf_exts_init(&fnew->exts, TCA_BASIC_ACT, TCA_BASIC_POLICE);
+ err = tcf_exts_init(&fnew->exts, net, TCA_BASIC_ACT, TCA_BASIC_POLICE);
if (err < 0)
goto errout;
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 062350c6621c..b4ac58039cb1 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -477,7 +477,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
if (!prog)
return -ENOBUFS;
- ret = tcf_exts_init(&prog->exts, TCA_BPF_ACT, TCA_BPF_POLICE);
+ ret = tcf_exts_init(&prog->exts, net, TCA_BPF_ACT, TCA_BPF_POLICE);
if (ret < 0)
goto errout;
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 02b05066b635..4c1567854f95 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -99,7 +99,7 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
if (!new)
return -ENOBUFS;
- err = tcf_exts_init(&new->exts, TCA_CGROUP_ACT, TCA_CGROUP_POLICE);
+ err = tcf_exts_init(&new->exts, net, TCA_CGROUP_ACT, TCA_CGROUP_POLICE);
if (err < 0)
goto errout;
new->handle = handle;
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 204e2edae8d5..eece1ee26930 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -441,7 +441,7 @@ static int flow_change(struct net *net, struct sk_buff *in_skb,
if (err < 0)
goto err1;
- err = tcf_exts_init(&fnew->exts, TCA_FLOW_ACT, TCA_FLOW_POLICE);
+ err = tcf_exts_init(&fnew->exts, net, TCA_FLOW_ACT, TCA_FLOW_POLICE);
if (err < 0)
goto err2;
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 640f83e7f93f..27300a3e76c7 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1344,7 +1344,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
goto errout_tb;
}
- err = tcf_exts_init(&fnew->exts, TCA_FLOWER_ACT, 0);
+ err = tcf_exts_init(&fnew->exts, net, TCA_FLOWER_ACT, 0);
if (err < 0)
goto errout;
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 4e34966f2ae2..456ee6e62dfa 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -285,7 +285,8 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
#endif /* CONFIG_NET_CLS_IND */
fnew->tp = f->tp;
- err = tcf_exts_init(&fnew->exts, TCA_FW_ACT, TCA_FW_POLICE);
+ err = tcf_exts_init(&fnew->exts, net, TCA_FW_ACT,
+ TCA_FW_POLICE);
if (err < 0) {
kfree(fnew);
return err;
@@ -334,7 +335,7 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
if (f == NULL)
return -ENOBUFS;
- err = tcf_exts_init(&f->exts, TCA_FW_ACT, TCA_FW_POLICE);
+ err = tcf_exts_init(&f->exts, net, TCA_FW_ACT, TCA_FW_POLICE);
if (err < 0)
goto errout;
f->id = handle;
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 1f9d481b0fbb..459921bd3d87 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -191,7 +191,7 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
if (!new)
return -ENOBUFS;
- err = tcf_exts_init(&new->exts, TCA_MATCHALL_ACT, 0);
+ err = tcf_exts_init(&new->exts, net, TCA_MATCHALL_ACT, 0);
if (err)
goto err_exts_init;
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 444d15a75d98..f006af23b64a 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -497,7 +497,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
if (!f)
goto errout;
- err = tcf_exts_init(&f->exts, TCA_ROUTE4_ACT, TCA_ROUTE4_POLICE);
+ err = tcf_exts_init(&f->exts, net, TCA_ROUTE4_ACT, TCA_ROUTE4_POLICE);
if (err < 0)
goto errout;
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index 4d3836178fa5..0719a21d9c41 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -501,7 +501,7 @@ static int rsvp_change(struct net *net, struct sk_buff *in_skb,
if (err < 0)
return err;
- err = tcf_exts_init(&e, TCA_RSVP_ACT, TCA_RSVP_POLICE);
+ err = tcf_exts_init(&e, net, TCA_RSVP_ACT, TCA_RSVP_POLICE);
if (err < 0)
return err;
err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, ovr, true,
@@ -523,7 +523,8 @@ static int rsvp_change(struct net *net, struct sk_buff *in_skb,
goto errout2;
}
- err = tcf_exts_init(&n->exts, TCA_RSVP_ACT, TCA_RSVP_POLICE);
+ err = tcf_exts_init(&n->exts, net, TCA_RSVP_ACT,
+ TCA_RSVP_POLICE);
if (err < 0) {
kfree(n);
goto errout2;
@@ -551,7 +552,7 @@ static int rsvp_change(struct net *net, struct sk_buff *in_skb,
if (f == NULL)
goto errout2;
- err = tcf_exts_init(&f->exts, TCA_RSVP_ACT, TCA_RSVP_POLICE);
+ err = tcf_exts_init(&f->exts, net, TCA_RSVP_ACT, TCA_RSVP_POLICE);
if (err < 0)
goto errout;
h2 = 16;
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index fbf3519a12d8..24e0a62a65cc 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -246,10 +246,12 @@ static const struct nla_policy tcindex_policy[TCA_TCINDEX_MAX + 1] = {
[TCA_TCINDEX_CLASSID] = { .type = NLA_U32 },
};
-static int tcindex_filter_result_init(struct tcindex_filter_result *r)
+static int tcindex_filter_result_init(struct tcindex_filter_result *r,
+ struct net *net)
{
memset(r, 0, sizeof(*r));
- return tcf_exts_init(&r->exts, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
+ return tcf_exts_init(&r->exts, net, TCA_TCINDEX_ACT,
+ TCA_TCINDEX_POLICE);
}
static void tcindex_partial_destroy_work(struct work_struct *work)
@@ -281,13 +283,10 @@ static int tcindex_alloc_perfect_hash(struct net *net, struct tcindex_data *cp)
return -ENOMEM;
for (i = 0; i < cp->hash; i++) {
- err = tcf_exts_init(&cp->perfect[i].exts,
+ err = tcf_exts_init(&cp->perfect[i].exts, net,
TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
if (err < 0)
goto errout;
-#ifdef CONFIG_NET_CLS_ACT
- cp->perfect[i].exts.net = net;
-#endif
}
return 0;
@@ -310,7 +309,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
int err, balloc = 0;
struct tcf_exts e;
- err = tcf_exts_init(&e, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
+ err = tcf_exts_init(&e, net, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
if (err < 0)
return err;
err = tcf_exts_validate(net, tp, tb, est, &e, ovr, true, extack);
@@ -344,7 +343,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
}
cp->h = p->h;
- err = tcindex_filter_result_init(&new_filter_result);
+ err = tcindex_filter_result_init(&new_filter_result, net);
if (err < 0)
goto errout1;
if (old_r)
@@ -431,7 +430,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
goto errout_alloc;
f->key = handle;
f->next = NULL;
- err = tcindex_filter_result_init(&f->result);
+ err = tcindex_filter_result_init(&f->result, net);
if (err < 0) {
kfree(f);
goto errout_alloc;
@@ -444,7 +443,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
}
if (old_r && old_r != r) {
- err = tcindex_filter_result_init(old_r);
+ err = tcindex_filter_result_init(old_r, net);
if (err < 0) {
kfree(f);
goto errout_alloc;
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 27d29c04dcc9..48e76a3acf8a 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -804,7 +804,7 @@ static void u32_replace_knode(struct tcf_proto *tp, struct tc_u_common *tp_c,
rcu_assign_pointer(*ins, n);
}
-static struct tc_u_knode *u32_init_knode(struct tcf_proto *tp,
+static struct tc_u_knode *u32_init_knode(struct net *net, struct tcf_proto *tp,
struct tc_u_knode *n)
{
struct tc_u_hnode *ht = rtnl_dereference(n->ht_down);
@@ -849,7 +849,7 @@ static struct tc_u_knode *u32_init_knode(struct tcf_proto *tp,
#endif
memcpy(&new->sel, s, sizeof(*s) + s->nkeys*sizeof(struct tc_u32_key));
- if (tcf_exts_init(&new->exts, TCA_U32_ACT, TCA_U32_POLICE)) {
+ if (tcf_exts_init(&new->exts, net, TCA_U32_ACT, TCA_U32_POLICE)) {
kfree(new);
return NULL;
}
@@ -911,7 +911,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
return -EINVAL;
}
- new = u32_init_knode(tp, n);
+ new = u32_init_knode(net, tp, n);
if (!new)
return -ENOMEM;
@@ -1061,7 +1061,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
n->fshift = s->hmask ? ffs(ntohl(s->hmask)) - 1 : 0;
n->flags = flags;
- err = tcf_exts_init(&n->exts, TCA_U32_ACT, TCA_U32_POLICE);
+ err = tcf_exts_init(&n->exts, net, TCA_U32_ACT, TCA_U32_POLICE);
if (err < 0)
goto errout;
--
2.20.1
^ permalink raw reply related
* [PATCH RFC 1/5] net: rtnetlink: Fix incorrect RCU API usage
From: Joel Fernandes (Google) @ 2019-02-21 5:49 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ido Schimmel,
Ingo Molnar, moderated list:INTEL ETHERNET DRIVERS,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, Lai Jiangshan,
Martin KaFai Lau, Mathieu Desnoyers, netdev, Paul E. McKenney,
Peter Zijlstra, rcu, Song Liu, Steven Rostedt, xdp-newbies,
Yonghong Song
In-Reply-To: <20190221054942.132388-1-joel@joelfernandes.org>
rtnl_register_internal() and rtnl_unregister_all tries to directly
dereference an RCU protected pointed outside RCU read side section.
While this is Ok to do since a lock is held, let us use the correct
API to avoid programmer bugs in the future.
This also fixes sparse warnings arising from not using RCU API.
net/core/rtnetlink.c:332:13: warning: incorrect type in assignment
(different address spaces) net/core/rtnetlink.c:332:13: expected
struct rtnl_link **tab net/core/rtnetlink.c:332:13: got struct
rtnl_link *[noderef] <asn:4>*<noident>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
net/core/rtnetlink.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 5ea1bed08ede..98be4b4818a9 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -188,7 +188,7 @@ static int rtnl_register_internal(struct module *owner,
msgindex = rtm_msgindex(msgtype);
rtnl_lock();
- tab = rtnl_msg_handlers[protocol];
+ tab = rtnl_dereference(rtnl_msg_handlers[protocol]);
if (tab == NULL) {
tab = kcalloc(RTM_NR_MSGTYPES, sizeof(void *), GFP_KERNEL);
if (!tab)
@@ -329,7 +329,7 @@ void rtnl_unregister_all(int protocol)
BUG_ON(protocol < 0 || protocol > RTNL_FAMILY_MAX);
rtnl_lock();
- tab = rtnl_msg_handlers[protocol];
+ tab = rtnl_dereference(rtnl_msg_handlers[protocol]);
if (!tab) {
rtnl_unlock();
return;
--
2.21.0.rc0.258.g878e2cd30e-goog
^ permalink raw reply related
* [PATCH RFC 5/5] rcuwait: Replace rcu_assign_pointer() with WRITE_ONCE
From: Joel Fernandes (Google) @ 2019-02-21 5:49 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ido Schimmel,
Ingo Molnar, moderated list:INTEL ETHERNET DRIVERS,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, Lai Jiangshan,
Martin KaFai Lau, Mathieu Desnoyers, netdev, Paul E. McKenney,
Peter Zijlstra, rcu, Song Liu, Steven Rostedt, xdp-newbies,
Yonghong Song
In-Reply-To: <20190221054942.132388-1-joel@joelfernandes.org>
This suppresses a sparse error generated due to the recently added
rcu_assign_pointer sparse check below. It seems WRITE_ONCE should be
sufficient here.
>> kernel//locking/percpu-rwsem.c:162:9: sparse: error: incompatible
types in comparison expression (different address spaces)
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
include/linux/rcuwait.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/rcuwait.h b/include/linux/rcuwait.h
index 90bfa3279a01..9e5b4760e6c2 100644
--- a/include/linux/rcuwait.h
+++ b/include/linux/rcuwait.h
@@ -44,7 +44,7 @@ extern void rcuwait_wake_up(struct rcuwait *w);
*/ \
WARN_ON(current->exit_state); \
\
- rcu_assign_pointer((w)->task, current); \
+ WRITE_ONCE((w)->task, current); \
for (;;) { \
/* \
* Implicit barrier (A) pairs with (B) in \
--
2.21.0.rc0.258.g878e2cd30e-goog
^ permalink raw reply related
* [PATCH RFC 4/5] sched/topology: Annonate RCU pointers properly
From: Joel Fernandes (Google) @ 2019-02-21 5:49 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ido Schimmel,
Ingo Molnar, moderated list:INTEL ETHERNET DRIVERS,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, Lai Jiangshan,
Martin KaFai Lau, Mathieu Desnoyers, netdev, Paul E. McKenney,
Peter Zijlstra, rcu, Song Liu, Steven Rostedt, xdp-newbies,
Yonghong Song
In-Reply-To: <20190221054942.132388-1-joel@joelfernandes.org>
The scheduler's topology code uses rcu_assign_pointer() to initialize
various pointers.
Let us annotate the pointers correctly which also help avoid future
bugs. This suppresses the new sparse errors caused by an annotation
check I added to rcu_assign_pointer().
Also replace rcu_assign_pointer call on rq->sd with WRITE_ONCE. This
should be sufficient for the rq->sd initialization.
This fixes sparse errors:
kernel//sched/topology.c:378:9: sparse: error: incompatible types in
comparison expression (different address spaces)
kernel//sched/topology.c:387:9: sparse: error: incompatible types in
comparison expression (different address spaces)
kernel//sched/topology.c:612:9: sparse: error: incompatible types in
comparison expression (different address spaces)
kernel//sched/topology.c:615:9: sparse: error: incompatible types in
comparison expression (different address spaces)
kernel//sched/topology.c:618:9: sparse: error: incompatible types in
comparison expression (different address spaces)
kernel//sched/topology.c:621:9: sparse: error: incompatible types in
comparison expression (different address spaces)
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
kernel/sched/sched.h | 12 ++++++------
kernel/sched/topology.c | 12 ++++++------
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 2ab545d40381..806703afd4b0 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -780,7 +780,7 @@ struct root_domain {
* NULL-terminated list of performance domains intersecting with the
* CPUs of the rd. Protected by RCU.
*/
- struct perf_domain *pd;
+ struct perf_domain __rcu *pd;
};
extern struct root_domain def_root_domain;
@@ -1305,13 +1305,13 @@ static inline struct sched_domain *lowest_flag_domain(int cpu, int flag)
return sd;
}
-DECLARE_PER_CPU(struct sched_domain *, sd_llc);
+DECLARE_PER_CPU(struct sched_domain __rcu *, sd_llc);
DECLARE_PER_CPU(int, sd_llc_size);
DECLARE_PER_CPU(int, sd_llc_id);
-DECLARE_PER_CPU(struct sched_domain_shared *, sd_llc_shared);
-DECLARE_PER_CPU(struct sched_domain *, sd_numa);
-DECLARE_PER_CPU(struct sched_domain *, sd_asym_packing);
-DECLARE_PER_CPU(struct sched_domain *, sd_asym_cpucapacity);
+DECLARE_PER_CPU(struct sched_domain_shared __rcu *, sd_llc_shared);
+DECLARE_PER_CPU(struct sched_domain __rcu *, sd_numa);
+DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
+DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
extern struct static_key_false sched_asym_cpucapacity;
struct sched_group_capacity {
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 3f35ba1d8fde..2eab2e16ded5 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -586,13 +586,13 @@ static void destroy_sched_domains(struct sched_domain *sd)
* the cpumask of the domain), this allows us to quickly tell if
* two CPUs are in the same cache domain, see cpus_share_cache().
*/
-DEFINE_PER_CPU(struct sched_domain *, sd_llc);
+DEFINE_PER_CPU(struct sched_domain __rcu *, sd_llc);
DEFINE_PER_CPU(int, sd_llc_size);
DEFINE_PER_CPU(int, sd_llc_id);
-DEFINE_PER_CPU(struct sched_domain_shared *, sd_llc_shared);
-DEFINE_PER_CPU(struct sched_domain *, sd_numa);
-DEFINE_PER_CPU(struct sched_domain *, sd_asym_packing);
-DEFINE_PER_CPU(struct sched_domain *, sd_asym_cpucapacity);
+DEFINE_PER_CPU(struct sched_domain_shared __rcu *, sd_llc_shared);
+DEFINE_PER_CPU(struct sched_domain __rcu *, sd_numa);
+DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
+DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
DEFINE_STATIC_KEY_FALSE(sched_asym_cpucapacity);
static void update_top_cache_domain(int cpu)
@@ -668,7 +668,7 @@ cpu_attach_domain(struct sched_domain *sd, struct root_domain *rd, int cpu)
rq_attach_root(rq, rd);
tmp = rq->sd;
- rcu_assign_pointer(rq->sd, sd);
+ WRITE_ONCE(rq->sd, sd);
dirty_sched_domain_sysctl(cpu);
destroy_sched_domains(tmp);
--
2.21.0.rc0.258.g878e2cd30e-goog
^ permalink raw reply related
* [PATCH RFC 2/5] ixgbe: Fix incorrect RCU API usage
From: Joel Fernandes (Google) @ 2019-02-21 5:49 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ido Schimmel,
Ingo Molnar, moderated list:INTEL ETHERNET DRIVERS,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, Lai Jiangshan,
Martin KaFai Lau, Mathieu Desnoyers, netdev, Paul E. McKenney,
Peter Zijlstra, rcu, Song Liu, Steven Rostedt, xdp-newbies,
Yonghong Song
In-Reply-To: <20190221054942.132388-1-joel@joelfernandes.org>
Recently, I added an RCU annotation check in rcu_assign_pointer. This
caused a sparse error to be reported by the ixgbe driver.
Further looking, it seems the adapter->xdp_prog pointer is not annotated
with __rcu. Annonating it fixed the error, but caused a bunch of other
warnings.
This patch tries to fix all warnings by using RCU API properly. This
makes sense to do because not using RCU properly can result in various
hard to find bugs. This is a best effort fix and is only build tested.
The sparse errors and warnings go away with the change. I request
maintainers / developers in this area to test it properly.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 4 ++--
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 17 ++++++++++++-----
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 08d85e336bd4..3b14daf27516 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -311,7 +311,7 @@ struct ixgbe_ring {
struct ixgbe_ring *next; /* pointer to next ring in q_vector */
struct ixgbe_q_vector *q_vector; /* backpointer to host q_vector */
struct net_device *netdev; /* netdev ring belongs to */
- struct bpf_prog *xdp_prog;
+ struct bpf_prog __rcu *xdp_prog;
struct device *dev; /* device for DMA mapping */
void *desc; /* descriptor ring memory */
union {
@@ -560,7 +560,7 @@ struct ixgbe_adapter {
unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
/* OS defined structs */
struct net_device *netdev;
- struct bpf_prog *xdp_prog;
+ struct bpf_prog __rcu *xdp_prog;
struct pci_dev *pdev;
struct mii_bus *mii_bus;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index daff8183534b..6aa59bb13a14 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -2199,7 +2199,7 @@ static struct sk_buff *ixgbe_run_xdp(struct ixgbe_adapter *adapter,
u32 act;
rcu_read_lock();
- xdp_prog = READ_ONCE(rx_ring->xdp_prog);
+ xdp_prog = rcu_dereference(rx_ring->xdp_prog);
if (!xdp_prog)
goto xdp_out;
@@ -6547,7 +6547,7 @@ int ixgbe_setup_rx_resources(struct ixgbe_adapter *adapter,
rx_ring->queue_index) < 0)
goto err;
- rx_ring->xdp_prog = adapter->xdp_prog;
+ rcu_assign_pointer(rx_ring->xdp_prog, adapter->xdp_prog);
return 0;
err:
@@ -10246,7 +10246,10 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
if (nr_cpu_ids > MAX_XDP_QUEUES)
return -ENOMEM;
- old_prog = xchg(&adapter->xdp_prog, prog);
+ rcu_read_lock();
+ old_prog = rcu_dereference(adapter->xdp_prog);
+ rcu_assign_pointer(adapter->xdp_prog, prog);
+ rcu_read_unlock();
/* If transitioning XDP modes reconfigure rings */
if (!!prog != !!old_prog) {
@@ -10271,13 +10274,17 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
static int ixgbe_xdp(struct net_device *dev, struct netdev_bpf *xdp)
{
struct ixgbe_adapter *adapter = netdev_priv(dev);
+ struct bpf_prog *prog;
switch (xdp->command) {
case XDP_SETUP_PROG:
return ixgbe_xdp_setup(dev, xdp->prog);
case XDP_QUERY_PROG:
- xdp->prog_id = adapter->xdp_prog ?
- adapter->xdp_prog->aux->id : 0;
+ rcu_read_lock();
+ prog = rcu_dereference(adapter->xdp_prog);
+ xdp->prog_id = prog ? prog->aux->id : 0;
+ rcu_read_unlock();
+
return 0;
case XDP_QUERY_XSK_UMEM:
return ixgbe_xsk_umem_query(adapter, &xdp->xsk.umem,
--
2.21.0.rc0.258.g878e2cd30e-goog
^ permalink raw reply related
* [PATCH RFC 3/5] sched/cpufreq: Fix incorrect RCU API usage
From: Joel Fernandes (Google) @ 2019-02-21 5:49 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ido Schimmel,
Ingo Molnar, moderated list:INTEL ETHERNET DRIVERS,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, Lai Jiangshan,
Martin KaFai Lau, Mathieu Desnoyers, netdev, Paul E. McKenney,
Peter Zijlstra, rcu, Song Liu, Steven Rostedt, xdp-newbies,
Yonghong Song
In-Reply-To: <20190221054942.132388-1-joel@joelfernandes.org>
Recently I added an RCU annotation check to rcu_assign_pointer(). All
pointers assigned to RCU protected data are to be annotated with __rcu
inorder to be able to use rcu_assign_pointer() similar to checks in
other RCU APIs.
This resulted in a sparse error: kernel//sched/cpufreq.c:41:9: sparse:
error: incompatible types in comparison expression (different address
spaces)
Fix this by using the correct APIs for RCU accesses. This will
potentially avoid any future bugs in the code. If it is felt that RCU
protection is not needed here, then the rcu_assign_pointer call can be
dropped and replaced with, say, WRITE_ONCE or smp_store_release. Or, may
be we add a new API to do it. But calls rcu_assign_pointer seems an
abuse of the RCU API unless RCU is being used.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
kernel/sched/cpufreq.c | 8 ++++++--
kernel/sched/sched.h | 2 +-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/cpufreq.c b/kernel/sched/cpufreq.c
index 22bd8980f32f..c9aeb3bf5dc2 100644
--- a/kernel/sched/cpufreq.c
+++ b/kernel/sched/cpufreq.c
@@ -7,7 +7,7 @@
*/
#include "sched.h"
-DEFINE_PER_CPU(struct update_util_data *, cpufreq_update_util_data);
+DEFINE_PER_CPU(struct update_util_data __rcu *, cpufreq_update_util_data);
/**
* cpufreq_add_update_util_hook - Populate the CPU's update_util_data pointer.
@@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data,
if (WARN_ON(!data || !func))
return;
- if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu)))
+ rcu_read_lock();
+ if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) {
+ rcu_read_unlock();
return;
+ }
+ rcu_read_unlock();
data->func = func;
rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index d04530bf251f..2ab545d40381 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2166,7 +2166,7 @@ static inline u64 irq_time_read(int cpu)
#endif /* CONFIG_IRQ_TIME_ACCOUNTING */
#ifdef CONFIG_CPU_FREQ
-DECLARE_PER_CPU(struct update_util_data *, cpufreq_update_util_data);
+DECLARE_PER_CPU(struct update_util_data __rcu *, cpufreq_update_util_data);
/**
* cpufreq_update_util - Take a note about CPU utilization changes.
--
2.21.0.rc0.258.g878e2cd30e-goog
^ permalink raw reply related
* [PATCH RFC 0/5] RCU fixes for rcu_assign_pointer() usage
From: Joel Fernandes (Google) @ 2019-02-21 5:49 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexei Starovoitov, Christian Brauner,
Daniel Borkmann, David Ahern, David S. Miller, Ido Schimmel,
Ingo Molnar, moderated list:INTEL ETHERNET DRIVERS,
Jakub Kicinski, Jeff Kirsher, Jesper Dangaard Brouer,
John Fastabend, Josh Triplett, keescook, Lai Jiangshan,
Martin KaFai Lau, Mathieu Desnoyers, netdev, Paul E. McKenney,
Peter Zijlstra, rcu, Song Liu, Steven Rostedt, xdp-newbies,
Yonghong Song
These patches fix various RCU API usage issues found due to sparse errors as a
result of the recent check to add rcu_check_sparse() to rcu_assign_pointer().
The errors in many cases seem to indicate either an incorrect API usage, or
missing annotations. The annotations added can also help avoid future incorrect
usages and bugs so it is a good idea to do in any case.
These are only build/boot tested and I request for feedback from maintainers
and developers in the various areas the patches touch. Thanks for any feedback!
(There are still errors in rbtree.h but I have kept those for a later time
since fixing them is a bit more involved).
Joel Fernandes (Google) (5):
net: rtnetlink: Fix incorrect RCU API usage
ixgbe: Fix incorrect RCU API usage
sched/cpufreq: Fix incorrect RCU API usage
sched/topology: Annonate RCU pointers properly
rcuwait: Replace rcu_assign_pointer() with WRITE_ONCE
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 4 ++--
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 17 ++++++++++++-----
include/linux/rcuwait.h | 2 +-
kernel/sched/cpufreq.c | 8 ++++++--
kernel/sched/sched.h | 14 +++++++-------
kernel/sched/topology.c | 12 ++++++------
net/core/rtnetlink.c | 4 ++--
7 files changed, 36 insertions(+), 25 deletions(-)
--
2.21.0.rc0.258.g878e2cd30e-goog
^ 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