* [PATCH net-next 7/8] mlxsw: spectrum: Set up PTP shaper when port status has changed
From: Ido Schimmel @ 2019-07-04 7:07 UTC (permalink / raw)
To: netdev; +Cc: davem, richardcochran, jiri, petrm, shalomt, mlxsw, Ido Schimmel
In-Reply-To: <20190704070740.302-1-idosch@idosch.org>
From: Shalom Toledo <shalomt@mellanox.com>
When getting port up down event (PUDE), change the PTP shaper
configuration based on hardware time stamping on/off and the port's
speed.
Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 8 ++++++++
drivers/net/ethernet/mellanox/mlxsw/spectrum.h | 1 +
.../net/ethernet/mellanox/mlxsw/spectrum_ptp.c | 17 +++++++++++++++++
.../net/ethernet/mellanox/mlxsw/spectrum_ptp.h | 10 ++++++++++
4 files changed, 36 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 7cfca2be09fc..ce285fbeebd3 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -171,6 +171,7 @@ struct mlxsw_sp_ptp_ops {
struct hwtstamp_config *config);
int (*hwtstamp_set)(struct mlxsw_sp_port *mlxsw_sp_port,
struct hwtstamp_config *config);
+ void (*shaper_work)(struct work_struct *work);
int (*get_ts_info)(struct mlxsw_sp *mlxsw_sp,
struct ethtool_ts_info *info);
};
@@ -3716,6 +3717,9 @@ static int mlxsw_sp_port_create(struct mlxsw_sp *mlxsw_sp, u8 local_port,
}
mlxsw_sp_port->default_vlan = mlxsw_sp_port_vlan;
+ INIT_DELAYED_WORK(&mlxsw_sp_port->ptp.shaper_dw,
+ mlxsw_sp->ptp_ops->shaper_work);
+
mlxsw_sp->ports[local_port] = mlxsw_sp_port;
err = register_netdev(dev);
if (err) {
@@ -3770,6 +3774,7 @@ static void mlxsw_sp_port_remove(struct mlxsw_sp *mlxsw_sp, u8 local_port)
struct mlxsw_sp_port *mlxsw_sp_port = mlxsw_sp->ports[local_port];
cancel_delayed_work_sync(&mlxsw_sp_port->periodic_hw_stats.update_dw);
+ cancel_delayed_work_sync(&mlxsw_sp_port->ptp.shaper_dw);
mlxsw_sp_port_ptp_clear(mlxsw_sp_port);
mlxsw_core_port_clear(mlxsw_sp->core, local_port, mlxsw_sp);
unregister_netdev(mlxsw_sp_port->dev); /* This calls ndo_stop */
@@ -4055,6 +4060,7 @@ static void mlxsw_sp_pude_event_func(const struct mlxsw_reg_info *reg,
if (status == MLXSW_PORT_OPER_STATUS_UP) {
netdev_info(mlxsw_sp_port->dev, "link up\n");
netif_carrier_on(mlxsw_sp_port->dev);
+ mlxsw_core_schedule_dw(&mlxsw_sp_port->ptp.shaper_dw, 0);
} else {
netdev_info(mlxsw_sp_port->dev, "link down\n");
netif_carrier_off(mlxsw_sp_port->dev);
@@ -4572,6 +4578,7 @@ static const struct mlxsw_sp_ptp_ops mlxsw_sp1_ptp_ops = {
.transmitted = mlxsw_sp1_ptp_transmitted,
.hwtstamp_get = mlxsw_sp1_ptp_hwtstamp_get,
.hwtstamp_set = mlxsw_sp1_ptp_hwtstamp_set,
+ .shaper_work = mlxsw_sp1_ptp_shaper_work,
.get_ts_info = mlxsw_sp1_ptp_get_ts_info,
};
@@ -4584,6 +4591,7 @@ static const struct mlxsw_sp_ptp_ops mlxsw_sp2_ptp_ops = {
.transmitted = mlxsw_sp2_ptp_transmitted,
.hwtstamp_get = mlxsw_sp2_ptp_hwtstamp_get,
.hwtstamp_set = mlxsw_sp2_ptp_hwtstamp_set,
+ .shaper_work = mlxsw_sp2_ptp_shaper_work,
.get_ts_info = mlxsw_sp2_ptp_get_ts_info,
};
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index c21cd1a425c7..abbb563db440 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -267,6 +267,7 @@ struct mlxsw_sp_port {
struct mlxsw_sp_acl_block *ing_acl_block;
struct mlxsw_sp_acl_block *eg_acl_block;
struct {
+ struct delayed_work shaper_dw;
struct hwtstamp_config hwtstamp_config;
u16 ing_types;
u16 egr_types;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
index 49aba0ce896b..0f7c4bd22a45 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
@@ -1000,6 +1000,23 @@ static int mlxsw_sp1_ptp_port_shaper_check(struct mlxsw_sp_port *mlxsw_sp_port)
return mlxsw_sp1_ptp_port_shaper_set(mlxsw_sp_port, ptps);
}
+void mlxsw_sp1_ptp_shaper_work(struct work_struct *work)
+{
+ struct delayed_work *dwork = to_delayed_work(work);
+ struct mlxsw_sp_port *mlxsw_sp_port;
+ int err;
+
+ mlxsw_sp_port = container_of(dwork, struct mlxsw_sp_port,
+ ptp.shaper_dw);
+
+ if (!mlxsw_sp1_ptp_hwtstamp_enabled(mlxsw_sp_port))
+ return;
+
+ err = mlxsw_sp1_ptp_port_shaper_check(mlxsw_sp_port);
+ if (err)
+ netdev_err(mlxsw_sp_port->dev, "Failed to set up PTP shaper\n");
+}
+
int mlxsw_sp1_ptp_hwtstamp_set(struct mlxsw_sp_port *mlxsw_sp_port,
struct hwtstamp_config *config)
{
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h
index b23abfc0bd76..72e55f6926b9 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h
@@ -54,6 +54,8 @@ int mlxsw_sp1_ptp_hwtstamp_get(struct mlxsw_sp_port *mlxsw_sp_port,
int mlxsw_sp1_ptp_hwtstamp_set(struct mlxsw_sp_port *mlxsw_sp_port,
struct hwtstamp_config *config);
+void mlxsw_sp1_ptp_shaper_work(struct work_struct *work);
+
int mlxsw_sp1_ptp_get_ts_info(struct mlxsw_sp *mlxsw_sp,
struct ethtool_ts_info *info);
@@ -113,6 +115,10 @@ mlxsw_sp1_ptp_hwtstamp_set(struct mlxsw_sp_port *mlxsw_sp_port,
return -EOPNOTSUPP;
}
+static inline void mlxsw_sp1_ptp_shaper_work(struct work_struct *work)
+{
+}
+
static inline int mlxsw_sp1_ptp_get_ts_info(struct mlxsw_sp *mlxsw_sp,
struct ethtool_ts_info *info)
{
@@ -167,6 +173,10 @@ mlxsw_sp2_ptp_hwtstamp_set(struct mlxsw_sp_port *mlxsw_sp_port,
return -EOPNOTSUPP;
}
+static inline void mlxsw_sp2_ptp_shaper_work(struct work_struct *work)
+{
+}
+
static inline int mlxsw_sp2_ptp_get_ts_info(struct mlxsw_sp *mlxsw_sp,
struct ethtool_ts_info *info)
{
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 8/8] mlxsw: spectrum_ptp: Apply the PTP shaper enable/disable logic
From: Ido Schimmel @ 2019-07-04 7:07 UTC (permalink / raw)
To: netdev; +Cc: davem, richardcochran, jiri, petrm, shalomt, mlxsw, Ido Schimmel
In-Reply-To: <20190704070740.302-1-idosch@idosch.org>
From: Shalom Toledo <shalomt@mellanox.com>
Apply by filling the PTP shaper parameters array.
Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
.../ethernet/mellanox/mlxsw/spectrum_ptp.c | 44 +++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
index 0f7c4bd22a45..bd9c2bc2d5d6 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
@@ -767,6 +767,50 @@ struct mlxsw_sp1_ptp_shaper_params {
static const struct mlxsw_sp1_ptp_shaper_params
mlxsw_sp1_ptp_shaper_params[] = {
+ {
+ .ethtool_speed = SPEED_100,
+ .port_speed = MLXSW_REG_QPSC_PORT_SPEED_100M,
+ .shaper_time_exp = 4,
+ .shaper_time_mantissa = 12,
+ .shaper_inc = 9,
+ .shaper_bs = 1,
+ .port_to_shaper_credits = 1,
+ .ing_timestamp_inc = -313,
+ .egr_timestamp_inc = 313,
+ },
+ {
+ .ethtool_speed = SPEED_1000,
+ .port_speed = MLXSW_REG_QPSC_PORT_SPEED_1G,
+ .shaper_time_exp = 0,
+ .shaper_time_mantissa = 12,
+ .shaper_inc = 6,
+ .shaper_bs = 0,
+ .port_to_shaper_credits = 1,
+ .ing_timestamp_inc = -35,
+ .egr_timestamp_inc = 35,
+ },
+ {
+ .ethtool_speed = SPEED_10000,
+ .port_speed = MLXSW_REG_QPSC_PORT_SPEED_10G,
+ .shaper_time_exp = 0,
+ .shaper_time_mantissa = 2,
+ .shaper_inc = 14,
+ .shaper_bs = 1,
+ .port_to_shaper_credits = 1,
+ .ing_timestamp_inc = -11,
+ .egr_timestamp_inc = 11,
+ },
+ {
+ .ethtool_speed = SPEED_25000,
+ .port_speed = MLXSW_REG_QPSC_PORT_SPEED_25G,
+ .shaper_time_exp = 0,
+ .shaper_time_mantissa = 0,
+ .shaper_inc = 11,
+ .shaper_bs = 1,
+ .port_to_shaper_credits = 1,
+ .ing_timestamp_inc = -14,
+ .egr_timestamp_inc = 14,
+ },
};
#define MLXSW_SP1_PTP_SHAPER_PARAMS_LEN ARRAY_SIZE(mlxsw_sp1_ptp_shaper_params)
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 3/4] net: dsa: vsc73xx: add support for parallel mode
From: Linus Walleij @ 2019-07-04 7:16 UTC (permalink / raw)
To: Pawel Dembicki
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Rob Herring, Mark Rutland, netdev,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel@vger.kernel.org
In-Reply-To: <20190703171924.31801-4-paweldembicki@gmail.com>
Hi Pawel,
this looks overall good and the following is just
documentation nit-pick so feel free to add my
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
On Wed, Jul 3, 2019 at 7:21 PM Pawel Dembicki <paweldembicki@gmail.com> wrote:
> This patch add platform part of vsc73xx driver.
> It allows to use chip connected by parallel interface.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
I would elaborate on the fact that we're dealing with memory-mapped
I/O but perhaps even more important to do that in the source code file and
Kconfig so it is close to the users rather than in the changelog.
Write something about the address bus wireing making all
accesses big endian.
> +config NET_DSA_VITESSE_VSC73XX_PLATFORM
> + tristate "Vitesse VSC7385/7388/7395/7398 Platform mode support"
> + depends on HAS_IOMEM
> + select NET_DSA_VITESSE_VSC73XX
> + ---help---
> + This enables support for the Vitesse VSC7385, VSC7388, VSC7395
> + and VSC7398 SparX integrated ethernet switches in Platform managed mode.
I would insert something about memory-mapped I/O mode
over a CPU-attached address bus.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2 4/4] net: dsa: vsc73xx: Assert reset if iCPU is enabled
From: Linus Walleij @ 2019-07-04 7:22 UTC (permalink / raw)
To: Pawel Dembicki
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Rob Herring, Mark Rutland, netdev,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel@vger.kernel.org
In-Reply-To: <20190703171924.31801-5-paweldembicki@gmail.com>
On Wed, Jul 3, 2019 at 7:21 PM Pawel Dembicki <paweldembicki@gmail.com> wrote:
> Driver allow to use devices with disabled iCPU only.
>
> Some devices have pre-initialised iCPU by bootloader.
> That state make switch unmanaged. This patch force reset
> if device is in unmanaged state. In the result chip lost
> internal firmware from RAM and it can be managed.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
My devices do not have direct access to the reset line so I
can't assert reset no matter how I try, if it works for you, the
code is certainly better like this.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2] let proc net directory inodes reflect to active net namespace
From: Alexey Dobriyan @ 2019-07-04 7:32 UTC (permalink / raw)
To: Hallsmark, Per
Cc: David S. Miller, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org
In-Reply-To: <B7B4BB465792624BAF51F33077E99065DC5D8E5D@ALA-MBD.corp.ad.wrs.com>
On Mon, Jul 01, 2019 at 11:06:34AM +0000, Hallsmark, Per wrote:
> +struct proc_dir_entry *proc_net_mkdir(struct net *net, const char *name,
> + struct proc_dir_entry *parent)
> +{
> + struct proc_dir_entry *pde;
> +
> + pde = proc_mkdir_data(name, 0, parent, net);
> + if (!pde)
> + return NULL;
> + pde->proc_dops = &proc_net_dentry_ops;
OK, this is buggy in a different way:
once proc_mkdir_data() returns, proc entry is live and should be fully
ready, so dentry operations should be glued before that.
I'll send proper patch.
^ permalink raw reply
* Re: [rdma 14/16] RDMA/irdma: Add ABI definitions
From: Leon Romanovsky @ 2019-07-04 7:40 UTC (permalink / raw)
To: Jeff Kirsher
Cc: dledford, jgg, davem, Mustafa Ismail, linux-rdma, netdev, nhorman,
sassmann, poswald, david.m.ertman, Shiraz Saleem
In-Reply-To: <20190704021259.15489-16-jeffrey.t.kirsher@intel.com>
On Wed, Jul 03, 2019 at 07:12:57PM -0700, Jeff Kirsher wrote:
> From: Mustafa Ismail <mustafa.ismail@intel.com>
>
> Add ABI definitions for irdma.
>
> Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com>
> Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
> ---
> include/uapi/rdma/irdma-abi.h | 130 ++++++++++++++++++++++++++++++++++
> 1 file changed, 130 insertions(+)
> create mode 100644 include/uapi/rdma/irdma-abi.h
>
> diff --git a/include/uapi/rdma/irdma-abi.h b/include/uapi/rdma/irdma-abi.h
> new file mode 100644
> index 000000000000..bdfbda4c829e
> --- /dev/null
> +++ b/include/uapi/rdma/irdma-abi.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
> +/* Copyright (c) 2006 - 2019 Intel Corporation. All rights reserved.
> + * Copyright (c) 2005 Topspin Communications. All rights reserved.
> + * Copyright (c) 2005 Cisco Systems. All rights reserved.
> + * Copyright (c) 2005 Open Grid Computing, Inc. All rights reserved.
> + */
> +
> +#ifndef IRDMA_ABI_H
> +#define IRDMA_ABI_H
> +
> +#include <linux/types.h>
> +
> +/* irdma must support legacy GEN_1 i40iw kernel
> + * and user-space whose last ABI ver is 5
> + */
> +#define IRDMA_ABI_VER 6
Can you please elaborate about it more?
There is no irdma code in RDMA yet, so it makes me wonder why new define
shouldn't start from 1.
Thanks
^ permalink raw reply
* Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and port attribute
From: Jiri Pirko @ 2019-07-04 7:44 UTC (permalink / raw)
To: Parav Pandit
Cc: Andrew Lunn, Jakub Kicinski, Jiri Pirko, netdev@vger.kernel.org,
Saeed Mahameed, vivien.didelot@gmail.com, f.fainelli@gmail.com
In-Reply-To: <AM0PR05MB48665F6CA614A3770D6ABCF4D1FB0@AM0PR05MB4866.eurprd05.prod.outlook.com>
Wed, Jul 03, 2019 at 06:13:17PM CEST, parav@mellanox.com wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@resnulli.us>
>> Sent: Wednesday, July 3, 2019 8:05 PM
>> To: Andrew Lunn <andrew@lunn.ch>
>> Cc: Parav Pandit <parav@mellanox.com>; Jakub Kicinski
>> <jakub.kicinski@netronome.com>; Jiri Pirko <jiri@mellanox.com>;
>> netdev@vger.kernel.org; Saeed Mahameed <saeedm@mellanox.com>;
>> vivien.didelot@gmail.com; f.fainelli@gmail.com
>> Subject: Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and
>> port attribute
>>
>> Wed, Jul 03, 2019 at 04:09:58PM CEST, andrew@lunn.ch wrote:
>> >> However, we expose it for DEVLINK_PORT_FLAVOUR_CPU and
>> >> DEVLINK_PORT_FLAVOUR_DSA. Not sure if it makes sense there either.
>> >> Ccing Florian, Andrew and Vivien.
>> >> What do you guys think?
>> >
>> >Hi Jiri
>> >
>> >DSA and CPU ports are physical ports of the switch. And there can be
>> >multiple DSA ports, and maybe sometime real soon now, multiple CPU
>> >ports. So having a number associated with them is useful.
>>
>> Okay. Makes sense.
>>
>Ok. I should probably update the comment section for port_number as its scope is expanded.
>Should I revise the series with updated comment?
Please do. Also put a check to not fillup port_number attribute in case
of pf/vf flavour. Thanks!
>
>> >
>> > Andrew
^ permalink raw reply
* Re: [PATCH v2] rt2x00: no need to check return value of debugfs_create functions
From: Stanislaw Gruszka @ 2019-07-04 7:50 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Helmut Schaa, Kalle Valo, David S. Miller, linux-wireless, netdev
In-Reply-To: <20190703113956.GA26652@kroah.com>
On Wed, Jul 03, 2019 at 01:39:56PM +0200, Greg Kroah-Hartman wrote:
>
> When calling debugfs functions, there is no need to ever check the
> return value. The function can work or not, but the code logic should
> never do something different based on this.
>
> Because we don't need to save the individual debugfs files and
> directories, remove the local storage of them and just remove the entire
> debugfs directory in a single call, making things a lot simpler.
>
> Cc: Stanislaw Gruszka <sgruszka@redhat.com>
> Cc: Helmut Schaa <helmut.schaa@googlemail.com>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-wireless@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>
^ permalink raw reply
* Re: [for-next V2 10/10] RDMA/core: Provide RDMA DIM support for ULPs
From: Leon Romanovsky @ 2019-07-04 7:51 UTC (permalink / raw)
To: Sagi Grimberg
Cc: Idan Burstein, Saeed Mahameed, David S. Miller, Doug Ledford,
Jason Gunthorpe, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Yamin Friedman, Max Gurtovoy
In-Reply-To: <8d525d64-6da1-48c3-952d-8c6b0d541859@grimberg.me>
On Wed, Jul 03, 2019 at 11:56:04AM -0700, Sagi Grimberg wrote:
>
> > Hi Sagi,
> >
> > I'm not sharing your worries about bad out-of-the-box experience for a
> > number of reasons.
> >
> > First of all, this code is part of upstream kernel and will take time
> > till users actually start to use it as is and not as part of some distro
> > backports or MOFED packages.
>
> True, but I am still saying that this feature is damaging sync IO which
> represents the majority of the users. It might not be an extreme impact
> but it is still a degradation (from a very limited testing I did this
> morning I'm seeing a consistent 5%-10% latency increase for low QD
> workloads which is consistent with what Yamin reported AFAIR).
>
> But having said that, the call is for you guys to make as this is a
> Mellanox device. I absolutely think that this is useful (as I said
> before), I just don't think its necessarily a good idea to opt it by
> default given that only a limited set of users would take full advantage
> of it while the rest would see a negative impact (even if its 10%).
>
> I don't have a hard objection here, just wanted to give you my
> opinion on this because mlx5 is an important driver for rdma
> users.
Your opinion is very valuable for us and we started internal thread to
challenge this "enable by default", it just takes time and I prefer to
enable this code to get test coverage as wide as possible.
>
> > Second, Yamin did extensive testing and worked very close with Or G.
> > and I have very high confident in the results of their team work.
>
> Has anyone tested other RDMA ulps? NFS/RDMA or SRP/iSER?
>
> Would be interesting to understand how other subsystems with different
> characteristics behave with this.
Me too, and I'll revert this default if needed.
Thanks
^ permalink raw reply
* Re: [PATCH][next] wil6210: fix wil_cid_valid with negative cid values
From: merez @ 2019-07-04 7:53 UTC (permalink / raw)
To: Colin King
Cc: Kalle Valo, David S . Miller, linux-wireless, wil6210, netdev,
kernel-janitors, linux-kernel
In-Reply-To: <20190702144026.13013-1-colin.king@canonical.com>
On 2019-07-02 17:40, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There are several occasions where a negative cid value is passed
> into wil_cid_valid and this is converted into a u8 causing the
> range check of cid >= 0 to always succeed. Fix this by making
> the cid argument an int to handle any -ve error value of cid.
>
> An example of this behaviour is in wil_cfg80211_dump_station,
> where cid is assigned -ENOENT if the call to wil_find_cid_by_idx
> fails, and this -ve value is passed to wil_cid_valid. I believe
> that the conversion of -ENOENT to the u8 value 254 which is
> greater than wil->max_assoc_sta causes wil_find_cid_by_idx to
> currently work fine, but I think is by luck and not the
> intended behaviour.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/net/wireless/ath/wil6210/wil6210.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h
> b/drivers/net/wireless/ath/wil6210/wil6210.h
> index 6f456b311a39..25a1adcb38eb 100644
> --- a/drivers/net/wireless/ath/wil6210/wil6210.h
> +++ b/drivers/net/wireless/ath/wil6210/wil6210.h
> @@ -1144,7 +1144,7 @@ static inline void wil_c(struct wil6210_priv
> *wil, u32 reg, u32 val)
> /**
> * wil_cid_valid - check cid is valid
> */
> -static inline bool wil_cid_valid(struct wil6210_priv *wil, u8 cid)
> +static inline bool wil_cid_valid(struct wil6210_priv *wil, int cid)
> {
> return (cid >= 0 && cid < wil->max_assoc_sta);
> }
Reviewed-by: Maya Erez <merez@codeaurora.org>
--
Maya Erez
Qualcomm Israel, Inc. on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH net-next v6 06/15] ethtool: netlink bitset handling
From: Jiri Pirko @ 2019-07-04 8:04 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, David Miller, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <20190703181851.GP20101@unicorn.suse.cz>
Wed, Jul 03, 2019 at 08:18:51PM CEST, mkubecek@suse.cz wrote:
>On Wed, Jul 03, 2019 at 01:49:33PM +0200, Jiri Pirko wrote:
>> Tue, Jul 02, 2019 at 01:50:09PM CEST, mkubecek@suse.cz wrote:
>> >diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
>> >index 97c369aa290b..4636682c551f 100644
>> >--- a/Documentation/networking/ethtool-netlink.txt
>> >+++ b/Documentation/networking/ethtool-netlink.txt
>> >@@ -73,6 +73,67 @@ set, the behaviour is the same as (or closer to) the behaviour before it was
>> > introduced.
>> >
>> >
>> >+Bit sets
>> >+--------
>> >+
>> >+For short bitmaps of (reasonably) fixed length, standard NLA_BITFIELD32 type
>> >+is used. For arbitrary length bitmaps, ethtool netlink uses a nested attribute
>> >+with contents of one of two forms: compact (two binary bitmaps representing
>> >+bit values and mask of affected bits) and bit-by-bit (list of bits identified
>> >+by either index or name).
>> >+
>> >+Compact form: nested (bitset) atrribute contents:
>> >+
>> >+ ETHTOOL_A_BITSET_LIST (flag) no mask, only a list
>> >+ ETHTOOL_A_BITSET_SIZE (u32) number of significant bits
>> >+ ETHTOOL_A_BITSET_VALUE (binary) bitmap of bit values
>> >+ ETHTOOL_A_BITSET_MASK (binary) bitmap of valid bits
>> >+
>> >+Value and mask must have length at least ETHTOOL_A_BITSET_SIZE bits rounded up
>> >+to a multiple of 32 bits. They consist of 32-bit words in host byte order,
>>
>> Looks like the blocks are similar to NLA_BITFIELD32. Why don't you user
>> nested array of NLA_BITFIELD32 instead?
>
>That would mean a layout like
>
> 4 bytes of attr header
> 4 bytes of value
> 4 bytes of mask
> 4 bytes of attr header
> 4 bytes of value
> 4 bytes of mask
> ...
>
>i.e. interleaved headers, words of value and words of mask. Having value
>and mask contiguous looks cleaner to me. Also, I can quickly check the
>sizes without iterating through a (potentially long) array.
Yeah, if you are not happy with this, I suggest to introduce
NLA_BITFIELD with arbitrary size. That would be probably cleanest.
>
>> >+words ordered from least significant to most significant (i.e. the same way as
>> >+bitmaps are passed with ioctl interface).
>> >+
>> >+For compact form, ETHTOOL_A_BITSET_SIZE and ETHTOOL_A_BITSET_VALUE are
>> >+mandatory. Similar to BITFIELD32, a compact form bit set requests to set bits
>>
>> Double space^^
>
>Hm, I have to learn how to tell vim not to do that with "gq".
>
>> >+in the mask to 1 (if the bit is set in value) or 0 (if not) and preserve the
>> >+rest. If ETHTOOL_A_BITSET_LIST is present, there is no mask and bitset
>> >+represents a simple list of bits.
>>
>> Okay, that is a bit confusing. Why not to rename to something like:
>> ETHTOOL_A_BITSET_NO_MASK (flag)
>> ?
>
>From the logical point of view, it's used for lists - list of link
>modes, list of netdev features, list of timestamping modes etc.
>
>The point is that in userspace requests, we sometimes want to change
>some values (enable A, disable B), sometimes to define the list of
>values to be set (I want (only) A, C and E to be enabled). In kernel
>replies, sometimes there is a natural value/mask pairing (e.g.
>advertised and supported link modes, enabled and supported WoL modes)
>but often there is just one bitmap.
>
>> >+Kernel bit set length may differ from userspace length if older application is
>> >+used on newer kernel or vice versa. If userspace bitmap is longer, an error is
>> >+issued only if the request actually tries to set values of some bits not
>> >+recognized by kernel.
>> >+
>> >+Bit-by-bit form: nested (bitset) attribute contents:
>> >+
>> >+ ETHTOOL_A_BITSET_LIST (flag) no mask, only a list
>> >+ ETHTOOL_A_BITSET_SIZE (u32) number of significant bits
>> >+ ETHTOOL_A_BITSET_BIT (nested) array of bits
>> >+ ETHTOOL_A_BITSET_BIT+ (nested) one bit
>> >+ ETHTOOL_A_BIT_INDEX (u32) bit index (0 for LSB)
>> >+ ETHTOOL_A_BIT_NAME (string) bit name
>> >+ ETHTOOL_A_BIT_VALUE (flag) present if bit is set
>> >+
>> >+Bit size is optional for bit-by-bit form. ETHTOOL_A_BITSET_BITS nest can only
>> >+contain ETHTOOL_A_BITS_BIT attributes but there can be an arbitrary number of
>> >+them. A bit may be identified by its index or by its name. When used in
>> >+requests, listed bits are set to 0 or 1 according to ETHTOOL_A_BIT_VALUE, the
>> >+rest is preserved. A request fails if index exceeds kernel bit length or if
>> >+name is not recognized.
>> >+
>> >+When ETHTOOL_A_BITSET_LIST flag is present, bitset is interpreted as a simple
>> >+bit list. ETHTOOL_A_BIT_VALUE attributes are not used in such case. Bit list
>> >+represents a bitmap with listed bits set and the rest zero.
>> >+
>> >+In requests, application can use either form. Form used by kernel in reply is
>> >+determined by a flag in flags field of request header. Semantics of value and
>> >+mask depends on the attribute. General idea is that flags control request
>> >+processing, info_mask control which parts of the information are returned in
>> >+"get" request and index identifies a particular subcommand or an object to
>> >+which the request applies.
>>
>> This is quite complex and confusing. Having the same API for 2 APIs is
>> odd. The API should be crystal clear, easy to use.
>>
>> Why can't you have 2 commands, one working with bit arrays only, one
>> working with strings? Something like:
>> X_GET
>> ETHTOOL_A_BITS (nested)
>> ETHTOOL_A_BIT_ARRAY (BITFIELD32)
>> X_NAMES_GET
>> ETHTOOL_A_BIT_NAMES (nested)
>> ETHTOOL_A_BIT_INDEX
>> ETHTOOL_A_BIT_NAME
>>
>> For set, you can also have multiple cmds:
>> X_SET - to set many at once, by bit index
>> ETHTOOL_A_BITS (nested)
>> ETHTOOL_A_BIT_ARRAY (BITFIELD32)
>> X_ONE_SET - to set one, by bit index
>> ETHTOOL_A_BIT_INDEX
>> ETHTOOL_A_BIT_VALUE
>> X_ONE_SET - to set one, by name
>> ETHTOOL_A_BIT_NAME
>> ETHTOOL_A_BIT_VALUE
>
>This looks as if you assume there is nothing except the bitset in the
>message but that is not true. Even with your proposed breaking of
>current groups, you would still have e.g. 4 bitsets in reply to netdev
>features query, 3 in timestamping info GET request and often bitsets
>combined with other data (e.g. WoL modes and optional WoL password).
>If you wanted to further refine the message granularity to the level of
>single parameters, we might be out of message type ids already.
You can still have multiple bitsets(bitfields) in single message and
have separate cmd/cmds to get string-bit mapping. No need to mangle it.
>
>Unless you want to forget about structured data completely and turn
>everything into tunables - but that's rather scary idea.
>
>Michal
^ permalink raw reply
* Re: [PATCH net-next v6 07/15] ethtool: support for netlink notifications
From: Jiri Pirko @ 2019-07-04 8:06 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, David Miller, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <20190703141614.GL20101@unicorn.suse.cz>
Wed, Jul 03, 2019 at 04:16:14PM CEST, mkubecek@suse.cz wrote:
>On Wed, Jul 03, 2019 at 03:33:52PM +0200, Jiri Pirko wrote:
>> >+/* notifications */
>> >+
>> >+typedef void (*ethnl_notify_handler_t)(struct net_device *dev,
>> >+ struct netlink_ext_ack *extack,
>> >+ unsigned int cmd, u32 req_mask,
>> >+ const void *data);
>> >+
>> >+static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
>> >+};
>> >+
>> >+void ethtool_notify(struct net_device *dev, struct netlink_ext_ack *extack,
>> >+ unsigned int cmd, u32 req_mask, const void *data)
>>
>> What's "req_mask" ?
>
>It's infomask to interpret the same way as if it came from request
>header (the notification triggered by a SET request or its ioctl
>equivalent uses the same format as corresponding GET_REPLY message and
>is created by the same code). But it could be called infomask, I have no
>strong opinion about that.
The name should be same all along the code so the reader can track it.
>
>> >+{
>> >+ if (unlikely(!ethnl_ok))
>> >+ return;
>> >+ ASSERT_RTNL();
>> >+
>> >+ if (likely(cmd < ARRAY_SIZE(ethnl_notify_handlers) &&
>> >+ ethnl_notify_handlers[cmd]))
>>
>> How it could be null?
>
>Notification message types share the enum with other kernel messages:
>
>/* message types - kernel to userspace */
>enum {
> ETHTOOL_MSG_KERNEL_NONE,
> ETHTOOL_MSG_STRSET_GET_REPLY,
> ETHTOOL_MSG_SETTINGS_GET_REPLY,
> ETHTOOL_MSG_SETTINGS_NTF,
> ETHTOOL_MSG_SETTINGS_SET_REPLY,
> ETHTOOL_MSG_INFO_GET_REPLY,
> ETHTOOL_MSG_PARAMS_GET_REPLY,
> ETHTOOL_MSG_PARAMS_NTF,
> ETHTOOL_MSG_NWAYRST_NTF,
> ETHTOOL_MSG_PHYSID_NTF,
> ETHTOOL_MSG_RESET_NTF,
> ETHTOOL_MSG_RESET_ACT_REPLY,
> ETHTOOL_MSG_RXFLOW_GET_REPLY,
> ETHTOOL_MSG_RXFLOW_NTF,
> ETHTOOL_MSG_RXFLOW_SET_REPLY,
>
> /* add new constants above here */
> __ETHTOOL_MSG_KERNEL_CNT,
> ETHTOOL_MSG_KERNEL_MAX = (__ETHTOOL_MSG_KERNEL_CNT - 1)
>};
>
>Only entries for *_NTF types are non-null in ethnl_notify_handlers[]:
>
>static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
> [ETHTOOL_MSG_SETTINGS_NTF] = ethnl_std_notify,
> [ETHTOOL_MSG_PARAMS_NTF] = ethnl_std_notify,
> [ETHTOOL_MSG_NWAYRST_NTF] = ethnl_nwayrst_notify,
> [ETHTOOL_MSG_PHYSID_NTF] = ethnl_physid_notify,
> [ETHTOOL_MSG_RESET_NTF] = ethnl_reset_notify,
> [ETHTOOL_MSG_RXFLOW_NTF] = ethnl_rxflow_notify,
>};
>
>If the check above fails, it means that kernel code tried to send
>a notification with type which does not exist or is not a notification,
>i.e. a bug in kernel; that's why the WARN_ONCE.
Got it, thanks!
>
>Michal
>
>> >+ ethnl_notify_handlers[cmd](dev, extack, cmd, req_mask, data);
>> >+ else
>> >+ WARN_ONCE(1, "notification %u not implemented (dev=%s, req_mask=0x%x)\n",
>> >+ cmd, netdev_name(dev), req_mask);
>> >+}
>> >+EXPORT_SYMBOL(ethtool_notify);
^ permalink raw reply
* Re: [PATCH net-next v6 08/15] ethtool: move string arrays into common file
From: Jiri Pirko @ 2019-07-04 8:09 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, David Miller, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <20190703143722.GN20101@unicorn.suse.cz>
Wed, Jul 03, 2019 at 04:37:22PM CEST, mkubecek@suse.cz wrote:
>On Wed, Jul 03, 2019 at 03:44:52PM +0200, Jiri Pirko wrote:
>> Tue, Jul 02, 2019 at 01:50:19PM CEST, mkubecek@suse.cz wrote:
>> >Introduce file net/ethtool/common.c for code shared by ioctl and netlink
>> >ethtool interface. Move name tables of features, RSS hash functions,
>> >tunables and PHY tunables into this file.
>> >
>> >Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
>> >---
>> > net/ethtool/Makefile | 2 +-
>> > net/ethtool/common.c | 84 ++++++++++++++++++++++++++++++++++++++++++++
>> > net/ethtool/common.h | 17 +++++++++
>> > net/ethtool/ioctl.c | 83 ++-----------------------------------------
>> > 4 files changed, 104 insertions(+), 82 deletions(-)
>> > create mode 100644 net/ethtool/common.c
>> > create mode 100644 net/ethtool/common.h
>> >
>> >diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
>> >index 482fdb9380fa..11782306593b 100644
>> >--- a/net/ethtool/Makefile
>> >+++ b/net/ethtool/Makefile
>> >@@ -1,6 +1,6 @@
>> > # SPDX-License-Identifier: GPL-2.0
>> >
>> >-obj-y += ioctl.o
>> >+obj-y += ioctl.o common.o
>> >
>> > obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o
>> >
>> >diff --git a/net/ethtool/common.c b/net/ethtool/common.c
>> >new file mode 100644
>> >index 000000000000..b0ce420e994e
>> >--- /dev/null
>> >+++ b/net/ethtool/common.c
>> >@@ -0,0 +1,84 @@
>> >+// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
>> >+
>> >+#include "common.h"
>> >+
>> >+const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = {
>>
>> const char *netdev_features_strings[NETDEV_FEATURE_COUNT] = {
>> ?
>>
>> Same with the other arrays.
>
>These are not new tables, this patch only moves existing tables from
>ioctl.c (originally net/core/ethtool.c) into common.c so that they can
>be used by both ioctl and netlink code.
>
>This fixed size string array format is used by ETHTOOL_GSTRINGS ioctl
>command. So if we switch these into simple const char *table[], we can
>get rid of some complexity in strset.c and bitset.c (the "simple" vs.
>"legacy" string set mess) but we would have to convert them into the
>fixed size string array in ioctl ETHTOOL_GSTRINGS handler. And then we
>would also have to convert (or rather "index") string sets retrieved
>from NIC driver (e.g. private flags, stats, tests) - which also means an
>extra kmalloc() (or rather kmalloc_array()).
>
>It an option I'm certainly open to if we agree on it but it's not for
>free.
Got it. I don't think we need to do this now. But it would be certainly
nice to fix this later on.
>
>Michal
^ permalink raw reply
* Re: [PATCH net-next v6 10/15] ethtool: provide string sets with STRSET_GET request
From: Jiri Pirko @ 2019-07-04 8:17 UTC (permalink / raw)
To: Michal Kubecek
Cc: David Miller, netdev, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <3c30527bef64c030078a1e305613080bb372cbe6.1562067622.git.mkubecek@suse.cz>
Tue, Jul 02, 2019 at 01:50:29PM CEST, mkubecek@suse.cz wrote:
[...]
>@@ -87,6 +89,64 @@ enum {
> ETHTOOL_A_BITSET_MAX = (__ETHTOOL_A_BITSET_CNT - 1)
You don't need "()". Same for the others below.
> };
>
>+/* string sets */
>+
>+enum {
>+ ETHTOOL_A_STRING_UNSPEC,
>+ ETHTOOL_A_STRING_INDEX, /* u32 */
>+ ETHTOOL_A_STRING_VALUE, /* string */
>+
>+ /* add new constants above here */
>+ __ETHTOOL_A_STRING_CNT,
>+ ETHTOOL_A_STRING_MAX = (__ETHTOOL_A_STRING_CNT - 1)
>+};
>+
>+enum {
>+ ETHTOOL_A_STRINGS_UNSPEC,
>+ ETHTOOL_A_STRINGS_STRING, /* nest - _A_STRINGS_* */
>+
>+ /* add new constants above here */
>+ __ETHTOOL_A_STRINGS_CNT,
>+ ETHTOOL_A_STRINGS_MAX = (__ETHTOOL_A_STRINGS_CNT - 1)
>+};
>+
>+enum {
>+ ETHTOOL_A_STRINGSET_UNSPEC,
>+ ETHTOOL_A_STRINGSET_ID, /* u32 */
>+ ETHTOOL_A_STRINGSET_COUNT, /* u32 */
>+ ETHTOOL_A_STRINGSET_STRINGS, /* nest - _A_STRINGS_* */
>+
>+ /* add new constants above here */
>+ __ETHTOOL_A_STRINGSET_CNT,
>+ ETHTOOL_A_STRINGSET_MAX = (__ETHTOOL_A_STRINGSET_CNT - 1)
>+};
>+
>+/* STRSET */
>+
>+enum {
>+ ETHTOOL_A_STRSET_UNSPEC,
>+ ETHTOOL_A_STRSET_HEADER, /* nest - _A_HEADER_* */
>+ ETHTOOL_A_STRSET_STRINGSETS, /* nest - _A_STRINGSETS_* */
>+
>+ /* add new constants above here */
>+ __ETHTOOL_A_STRSET_CNT,
>+ ETHTOOL_A_STRSET_MAX = (__ETHTOOL_A_STRSET_CNT - 1)
>+};
>+
>+enum {
>+ ETHTOOL_A_STRINGSETS_UNSPEC,
>+ ETHTOOL_A_STRINGSETS_STRINGSET, /* nest - _A_STRINGSET_* */
>+
>+ /* add new constants above here */
>+ __ETHTOOL_A_STRINGSETS_CNT,
>+ ETHTOOL_A_STRINGSETS_MAX = (__ETHTOOL_A_STRINGSETS_CNT - 1)
>+};
>+
[...]
>+ nla_for_each_nested(attr, nest, rem) {
>+ u32 id;
>+
>+ if (WARN_ONCE(nla_type(attr) != ETHTOOL_A_STRINGSETS_STRINGSET,
>+ "unexpected attrtype %u in ETHTOOL_A_STRSET_STRINGSETS\n",
>+ nla_type(attr)))
>+ return -EINVAL;
>+
>+ ret = strset_get_id(attr, &id, extack);
>+ if (ret < 0)
>+ return ret;
>+ if (ret >= ETH_SS_COUNT) {
>+ NL_SET_ERR_MSG_ATTR(extack, attr,
>+ "unknown string set id");
>+ return -EOPNOTSUPP;
>+ }
>+
>+ data->req_ids |= (1U << id);
You don't need "()" here either.
[...]
^ permalink raw reply
* Re: [PATCH net-next v5 5/5] selftests: tc-tests: actions: add MPLS tests
From: Davide Caratti @ 2019-07-04 8:40 UTC (permalink / raw)
To: John Hurley, netdev
Cc: davem, jiri, xiyou.wangcong, dsahern, willemdebruijn.kernel,
simon.horman, jakub.kicinski, oss-drivers
In-Reply-To: <1562113531-29296-6-git-send-email-john.hurley@netronome.com>
On Wed, 2019-07-03 at 01:25 +0100, John Hurley wrote:
> Add a new series of selftests to verify the functionality of act_mpls in
> TC.
>
> Signed-off-by: John Hurley <john.hurley@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> .../tc-testing/tc-tests/actions/mpls.json | 812 +++++++++++++++++++++
> 1 file changed, 812 insertions(+)
> create mode 100644 tools/testing/selftests/tc-testing/tc-tests/actions/mpls.json
>
hello John,
(sorry for noticing this late). some scripts use
tools/testing/selftests/tc-testing/config
to rebuild vmlinux before running TDC. I think you should add a line
there that sets CONFIG_NET_ACT_MPLS=y.
WDYT?
thanks!
--
davide
^ permalink raw reply
* Re: [PATCH net-next 3/3] selftests: forwarding: Test multipath hashing on inner IP pkts for GRE tunnel
From: Ido Schimmel @ 2019-07-04 8:42 UTC (permalink / raw)
To: Stephen Suryaputra; +Cc: netdev, nikolay, dsahern
In-Reply-To: <20190703151934.9567-4-ssuryaextr@gmail.com>
On Wed, Jul 03, 2019 at 11:19:34AM -0400, Stephen Suryaputra wrote:
> Add selftest scripts for multipath hashing on inner IP pkts when there
> is a single GRE tunnel but there are multiple underlay routes to reach
> the other end of the tunnel.
>
> Four cases are covered in these scripts:
> - IPv4 over GRE over IPv4
> - IPv6 over GRE over IPv4
> - IPv4 over GRE over IPv6
> - IPv6 over GRE over IPv6
>
> Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
Thanks a lot for all the tests! Ran gre_inner_v4_multipath.sh and it
looks good to me.
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
^ permalink raw reply
* Re: [PATCH net-next v6 09/15] ethtool: generic handlers for GET requests
From: Jiri Pirko @ 2019-07-04 8:45 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, David Miller, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <20190703175339.GO20101@unicorn.suse.cz>
Wed, Jul 03, 2019 at 07:53:39PM CEST, mkubecek@suse.cz wrote:
>On Wed, Jul 03, 2019 at 04:25:10PM +0200, Jiri Pirko wrote:
>> Tue, Jul 02, 2019 at 01:50:24PM CEST, mkubecek@suse.cz wrote:
>>
>> [...]
>>
>> >+/* generic ->doit() handler for GET type requests */
>> >+static int ethnl_get_doit(struct sk_buff *skb, struct genl_info *info)
>>
>> It is very unfortunate for review to introduce function in a patch and
>> don't use it. In general, this approach is frowned upon. You should use
>> whatever you introduce in the same patch. I understand it is sometimes
>> hard.
>
>It's not as if I introduced something and didn't show how to use it.
>First use is in the very next patch so if you insist on reading each
>patch separately without context, just combine 09/15 and 10/15 together;
>the overlap is minimal (10/15 adds an entry into get_requests[]
>introduced in 09/15).
>
>I could have done that myself but the resulting patch would add over
>1000 lines (also something frown upon in general) and if someone asked
>if it could be split, the only honest answer I could give would be:
>"Of course it should be split, it consists of two completely logically
>separated parts (which are also 99% separated in code)."
>
>> IIUC, you have one ethnl_get_doit for all possible commands, and you
>
>Not all of them, only GET requests (and related notifications) and out
>of them, only those which fit the common pattern. There will be e.g. Rx
>rules and stats (maybe others) where dump request won't be iterating
>through devices so that they will need at least their own dumpit
>handler.
>
>> have this ops to do cmd-specific tasks. That is quite unusual. Plus if
>> you consider the complicated datastructures connected with this,
>> I'm lost from the beginning :( Any particular reason form this indirection?
>> I don't think any other generic netlink code does that (correct me if
>> I'm wrong). The nice thing about generic netlink is the fact that
>> you have separate handlers per cmd.
>>
>> I don't think you need these ops and indirections. For the common parts,
>> just have a set of common helpers, as the other generic netlink users
>> are doing. The code would be much easier to read and follow then.
>
>As I said last time, what you suggest is going back to what I already
>had in the early versions; so I have pretty good idea what the result
>would look like.
>
>I could go that way, having a separate main handler for each request
>type and call common helpers from it. But as there would always be
>a doit() handler, a dumpit() handler and mostly also a notification
>handler, I would have to factor out the functions which are now
>callbacks in struct get_request_ops anyway. To avoid too many
>parameters, I would end up with structures very similar to what I have
>now. (Not really "I would", the structures were already there, the only
>difference was that the "request" and "data" parts were two structures
>rather than one.)
>
>So at the moment, I would have 5 functions looking almost the same as
>ethnl_get_doit(), 5 functions looking almost as ethnl_get_dumpit() and
>2 functions looking like ethnl_std_notify(), with the prospect of more
>to be added. Any change in the logic would need to be repeated for all
>of them. Moreover, you also proposed (or rather requested) to drop the
>infomask concept and split the message types into multiple separate
>ones. With that change, the number of almost copies would be 21 doit(),
>21 dumpit() and 13 notification handlers (for now, that is).
I understand. It's a tradeoff. The code as you introduce is hard for
me to follow, so I thought that the other way would help readability.
Also it seems to be that you replicate a lot of generic netlink API
(per-cmd-doit/dumpit ops and privileged/GENL_ADMIN_PERM) in your code.
Seems more natural to use the API as others are doing.
>
>I'm also not happy about the way typical GET and SET request processing
>looks now. But I would much rather go in the opposite direction: define
>relationship between message attributes and data structure members so
>that most of the size estimate, data prepare, message fill and data
>update functions which are all repeating the same pattern could be
>replaced by universal functions doing these actions according to the
>description. The direction you suggest is the direction I came from.
>
>Seriously, I don't know what to think. Anywhere I look, return code is
>checked with "if (ret < 0)" (sure, some use "if (ret)" but it's
>certainly not prevalent or universally preferred, more like 1:1), now
>you tell me it's wrong. Networking stack is full of simple helpers and
>wrappers, yet you keep telling me simple wrappers are wrong. Networking
>stack is full of abstractions and ops, you tell me it's wrong. It's
>really confusing...
It is all just a matter of readability I believe.
For example when I see "if (ret < 0) goto err" I assume that there
might be positive non-error value returned. There are many places where
the code is not in optimal shape. But for new code, I believe we have to
be careful.
Simple helpers are fine as far as they don't cover simple things going
under the hood. Typical example is "myown_lock() myown_unlock()" which
just call mutex_lock/unlock. Another nice example is macro putting
netlink attributes having goto nla_failure inside - this was removed
couple years ago. The code still have many things like this. Again, for
new code, I believe we have to be careful.
^ permalink raw reply
* Re: [PATCH net-next v6 09/15] ethtool: generic handlers for GET requests
From: Jiri Pirko @ 2019-07-04 8:49 UTC (permalink / raw)
To: Michal Kubecek
Cc: David Miller, netdev, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <4faa0ce52dfe02c9cde5a46012b16c9af6764c5e.1562067622.git.mkubecek@suse.cz>
Tue, Jul 02, 2019 at 01:50:24PM CEST, mkubecek@suse.cz wrote:
[...]
>+/* The structure holding data for unified processing GET requests consists of
>+ * two parts: request info and reply data. Request info is related to client
>+ * request and for dump request it stays constant through all processing;
>+ * reply data contains data for composing a reply message. When processing
>+ * a dump request, request info is filled only once but reply data is filled
>+ * from scratch for each reply message.
>+ *
>+ * +-----------------+-----------------+------------------+-----------------+
>+ * | common_req_info | specific info | ethnl_reply_data | specific data |
>+ * +-----------------+-----------------+------------------+-----------------+
>+ * |<---------- request info --------->|<----------- reply data ----------->|
>+ *
>+ * Request info always starts at offset 0 with struct ethnl_req_info which
>+ * holds information from parsing the common header. It may be followed by
>+ * other members for request attributes specific for current message type.
>+ * Reply data starts with struct ethnl_reply_data which may be followed by
>+ * other members holding data needed to compose a message.
>+ */
>+
[...]
>+/**
>+ * struct get_request_ops - unified handling of GET requests
>+ * @request_cmd: command id for request (GET)
>+ * @reply_cmd: command id for reply (GET_REPLY)
>+ * @hdr_attr: attribute type for request header
>+ * @max_attr: maximum (top level) attribute type
>+ * @data_size: total length of data structure
>+ * @repdata_offset: offset of "reply data" part (struct ethnl_reply_data)
For example, this looks quite scarry for me. You have one big chunk of
data (according to the scheme above) specific for cmd with reply starting
at arbitrary offset.
>+ * @request_policy: netlink policy for message contents
>+ * @header_policy: (optional) netlink policy for request header
>+ * @default_infomask: default infomask (to use if none specified)
>+ * @all_reqflags: allowed request specific flags
>+ * @allow_nodev_do: allow non-dump request with no device identification
>+ * @parse_request:
>+ * Parse request except common header (struct ethnl_req_info). Common
>+ * header is already filled on entry, the rest up to @repdata_offset
>+ * is zero initialized. This callback should only modify type specific
>+ * request info by parsed attributes from request message.
>+ * @prepare_data:
>+ * Retrieve and prepare data needed to compose a reply message. Calls to
>+ * ethtool_ops handlers should be limited to this callback. Common reply
>+ * data (struct ethnl_reply_data) is filled on entry, type specific part
>+ * after it is zero initialized. This callback should only modify the
>+ * type specific part of reply data. Device identification from struct
>+ * ethnl_reply_data is to be used as for dump requests, it iterates
>+ * through network devices which common_req_info::dev points to the
>+ * device from client request.
>+ * @reply_size:
>+ * Estimate reply message size. Returned value must be sufficient for
>+ * message payload without common reply header. The callback may returned
>+ * estimate higher than actual message size if exact calculation would
>+ * not be worth the saved memory space.
>+ * @fill_reply:
>+ * Fill reply message payload (except for common header) from reply data.
>+ * The callback must not generate more payload than previously called
>+ * ->reply_size() estimated.
>+ * @cleanup:
>+ * Optional cleanup called when reply data is no longer needed. Can be
>+ * used e.g. to free any additional data structures outside the main
>+ * structure which were allocated by ->prepare_data(). When processing
>+ * dump requests, ->cleanup() is called for each message.
>+ *
>+ * Description of variable parts of GET request handling when using the unified
>+ * infrastructure. When used, a pointer to an instance of this structure is to
>+ * be added to &get_requests array and generic handlers ethnl_get_doit(),
>+ * ethnl_get_dumpit(), ethnl_get_start() and ethnl_get_done() used in
>+ * @ethnl_genl_ops
>+ */
>+struct get_request_ops {
>+ u8 request_cmd;
>+ u8 reply_cmd;
>+ u16 hdr_attr;
>+ unsigned int max_attr;
>+ unsigned int data_size;
>+ unsigned int repdata_offset;
>+ const struct nla_policy *request_policy;
>+ const struct nla_policy *header_policy;
>+ u32 default_infomask;
>+ u32 all_reqflags;
>+ bool allow_nodev_do;
>+
>+ int (*parse_request)(struct ethnl_req_info *req_info,
>+ struct nlattr **tb,
>+ struct netlink_ext_ack *extack);
>+ int (*prepare_data)(struct ethnl_req_info *req_info,
>+ struct genl_info *info);
>+ int (*reply_size)(const struct ethnl_req_info *req_info);
>+ int (*fill_reply)(struct sk_buff *skb,
>+ const struct ethnl_req_info *req_info);
>+ void (*cleanup)(struct ethnl_req_info *req_info);
>+};
>+
> #endif /* _NET_ETHTOOL_NETLINK_H */
>--
>2.22.0
>
^ permalink raw reply
* [PATCH v2 bpf-next] selftests/bpf: fix "alu with different scalars 1" on s390
From: Ilya Leoshkevich @ 2019-07-04 8:52 UTC (permalink / raw)
To: bpf, netdev, ys114321; +Cc: Ilya Leoshkevich
BPF_LDX_MEM is used to load the least significant byte of the retrieved
test_val.index, however, on big-endian machines it ends up retrieving
the most significant byte.
Use the correct least significant byte offset on big-endian machines.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
v1->v2:
- use __BYTE_ORDER instead of __BYTE_ORDER__.
tools/testing/selftests/bpf/verifier/value_ptr_arith.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tools/testing/selftests/bpf/verifier/value_ptr_arith.c b/tools/testing/selftests/bpf/verifier/value_ptr_arith.c
index c3de1a2c9dc5..e5940c4e8b8f 100644
--- a/tools/testing/selftests/bpf/verifier/value_ptr_arith.c
+++ b/tools/testing/selftests/bpf/verifier/value_ptr_arith.c
@@ -183,7 +183,11 @@
BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
BPF_EXIT_INSN(),
+#if __BYTE_ORDER == __LITTLE_ENDIAN
BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
+#else
+ BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, sizeof(int) - 1),
+#endif
BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 3),
BPF_MOV64_IMM(BPF_REG_2, 0),
BPF_MOV64_IMM(BPF_REG_3, 0x100000),
--
2.21.0
^ permalink raw reply related
* Re: i.mx6ul with DSA in multi chip addressing mode - no MDIO access
From: Benjamin Beckmeyer @ 2019-07-04 8:54 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev
In-Reply-To: <20190703155518.GE18473@lunn.ch>
On 03.07.19 17:55, Andrew Lunn wrote:
> On Wed, Jul 03, 2019 at 03:10:34PM +0200, Benjamin Beckmeyer wrote:
>> Hey folks,
>>
>> I'm having a problem with a custom i.mx6ul board. When DSA is loaded I can't
>> get access to the switch via MDIO, but the DSA is working properly. I set up
>> a bridge for testing and the switch is in forwarding mode and i can ping the
>> board. But the MDIO access isn't working at address 2 for the switch. When I
>> delete the DSA from the devicetree and start the board up, I can access the
>> switch via MDIO.
>>
>> With DSA up and running:
>>
>> mii -i 2 0 0x9800
>> mii -i 2 1
>> phyid:2, reg:0x01 -> 0x4000
>> mii -i 2 0 0x9803
>> mii -i 2 1
>> phyid:2, reg:0x01 -> 0x4000
>> mii -i 2 1 0x1883
>> mii -i 2 1
>> phyid:2, reg:0x01 -> 0x4000
> Hi Benjamin
>
> I'm guessing that the driver is also using register 0 and 1 at the
> same time you are, e.g. to poll the PHYs for link status etc.
>
> There are trace points for MDIO, so you can get the kernel to log all
> registers access. That should confirm if i'm right.
>
> Andrew
Hi Andrew,
you were absolutly right. The bus is really busy the whole time, I've
checked that with the tracepoints in mdio_access.
But I'm still wondering why isn't that with a single chip addressing
mode configured switch? I mean, okay, the switch has more ports, but
I've checked the accesses for both. The 6321(single chip addressing
mode) has around 4-5 accesses to the MDIO bus and the 6390(multi chip
addressing mode) has around 600 accesses per second.
Thanks,
Benjamin
^ permalink raw reply
* [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs
From: Quentin Monnet @ 2019-07-04 8:56 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: bpf, netdev, oss-drivers, Quentin Monnet
Add a new "bpftool prog run" subcommand to run a loaded program on input
data (and possibly with input context) passed by the user.
Print output data (and output context if relevant) into a file or into
the console. Print return value and duration for the test run into the
console.
A "repeat" argument can be passed to run the program several times in a
row.
The command does not perform any kind of verification based on program
type (Is this program type allowed to use an input context?) or on data
consistency (Can I work with empty input data?), this is left to the
kernel.
Example invocation:
# perl -e 'print "\x0" x 14' | ./bpftool prog run \
pinned /sys/fs/bpf/sample_ret0 \
data_in - data_out - repeat 5
0000000 0000 0000 0000 0000 0000 0000 0000 | ........ ......
Return value: 0, duration (average): 260ns
When one of data_in or ctx_in is "-", bpftool reads from standard input,
in binary format. Other formats (JSON, hexdump) might be supported (via
an optional command line keyword like "data_fmt_in") in the future if
relevant, but this would require doing more parsing in bpftool.
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
.../bpftool/Documentation/bpftool-prog.rst | 34 ++
tools/bpf/bpftool/bash-completion/bpftool | 28 +-
tools/bpf/bpftool/main.c | 29 ++
tools/bpf/bpftool/main.h | 1 +
tools/bpf/bpftool/prog.c | 348 +++++++++++++++++-
tools/include/linux/sizes.h | 48 +++
6 files changed, 485 insertions(+), 3 deletions(-)
create mode 100644 tools/include/linux/sizes.h
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 1df637f85f94..7a374b3c851d 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -29,6 +29,7 @@ PROG COMMANDS
| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
| **bpftool** **prog tracelog**
+| **bpftool** **prog run** *PROG* **data_in** *FILE* [**data_out** *FILE* [**data_size_out** *L*]] [**ctx_in** *FILE* [**ctx_out** *FILE* [**ctx_size_out** *M*]]] [**repeat** *N*]
| **bpftool** **prog help**
|
| *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
@@ -146,6 +147,39 @@ DESCRIPTION
streaming data from BPF programs to user space, one can use
perf events (see also **bpftool-map**\ (8)).
+ **bpftool prog run** *PROG* **data_in** *FILE* [**data_out** *FILE* [**data_size_out** *L*]] [**ctx_in** *FILE* [**ctx_out** *FILE* [**ctx_size_out** *M*]]] [**repeat** *N*]
+ Run BPF program *PROG* in the kernel testing infrastructure
+ for BPF, meaning that the program works on the data and
+ context provided by the user, and not on actual packets or
+ monitored functions etc. Return value and duration for the
+ test run are printed out to the console.
+
+ Input data is read from the *FILE* passed with **data_in**.
+ If this *FILE* is "**-**", input data is read from standard
+ input. Input context, if any, is read from *FILE* passed with
+ **ctx_in**. Again, "**-**" can be used to read from standard
+ input, but only if standard input is not already in use for
+ input data. If a *FILE* is passed with **data_out**, output
+ data is written to that file. Similarly, output context is
+ written to the *FILE* passed with **ctx_out**. For both
+ output flows, "**-**" can be used to print to the standard
+ output (as plain text, or JSON if relevant option was
+ passed). If output keywords are omitted, output data and
+ context are discarded. Keywords **data_size_out** and
+ **ctx_size_out** are used to pass the size (in bytes) for the
+ output buffers to the kernel, although the default of 32 kB
+ should be more than enough for most cases.
+
+ Keyword **repeat** is used to indicate the number of
+ consecutive runs to perform. Note that output data and
+ context printed to files correspond to the last of those
+ runs. The duration printed out at the end of the runs is an
+ average over all runs performed by the command.
+
+ Not all program types support test run. Among those which do,
+ not all of them can take the **ctx_in**/**ctx_out**
+ arguments. bpftool does not perform checks on program types.
+
**bpftool prog help**
Print short help message.
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index ba37095e1f62..965a8658cca3 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -408,10 +408,34 @@ _bpftool()
tracelog)
return 0
;;
+ run)
+ if [[ ${#words[@]} -lt 5 ]]; then
+ _filedir
+ return 0
+ fi
+ case $prev in
+ id)
+ _bpftool_get_prog_ids
+ return 0
+ ;;
+ data_in|data_out|ctx_in|ctx_out)
+ _filedir
+ return 0
+ ;;
+ repeat|data_size_out|ctx_size_out)
+ return 0
+ ;;
+ *)
+ _bpftool_once_attr 'data_in data_out data_size_out \
+ ctx_in ctx_out ctx_size_out repeat'
+ return 0
+ ;;
+ esac
+ ;;
*)
[[ $prev == $object ]] && \
- COMPREPLY=( $( compgen -W 'dump help pin attach detach load \
- show list tracelog' -- "$cur" ) )
+ COMPREPLY=( $( compgen -W 'dump help pin attach detach \
+ load show list tracelog run' -- "$cur" ) )
;;
esac
;;
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 4879f6395c7e..e916ff25697f 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -117,6 +117,35 @@ bool is_prefix(const char *pfx, const char *str)
return !memcmp(str, pfx, strlen(pfx));
}
+/* Last argument MUST be NULL pointer */
+int detect_common_prefix(const char *arg, ...)
+{
+ unsigned int count = 0;
+ const char *ref;
+ char msg[256];
+ va_list ap;
+
+ snprintf(msg, sizeof(msg), "ambiguous prefix: '%s' could be '", arg);
+ va_start(ap, arg);
+ while ((ref = va_arg(ap, const char *))) {
+ if (!is_prefix(arg, ref))
+ continue;
+ count++;
+ if (count > 1)
+ strncat(msg, "' or '", sizeof(msg) - strlen(msg) - 1);
+ strncat(msg, ref, sizeof(msg) - strlen(msg) - 1);
+ }
+ va_end(ap);
+ strncat(msg, "'", sizeof(msg) - strlen(msg) - 1);
+
+ if (count >= 2) {
+ p_err(msg);
+ return -1;
+ }
+
+ return 0;
+}
+
void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
{
unsigned char *data = arg;
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 9c5d9c80f71e..3ef0d9051e10 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -101,6 +101,7 @@ void p_err(const char *fmt, ...);
void p_info(const char *fmt, ...);
bool is_prefix(const char *pfx, const char *str);
+int detect_common_prefix(const char *arg, ...);
void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
void usage(void) __noreturn;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 9b0db5d14e31..8dcbaa0a8ab1 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -15,6 +15,7 @@
#include <sys/stat.h>
#include <linux/err.h>
+#include <linux/sizes.h>
#include <bpf.h>
#include <btf.h>
@@ -748,6 +749,344 @@ static int do_detach(int argc, char **argv)
return 0;
}
+static int check_single_stdin(char *file_in, char *other_file_in)
+{
+ if (file_in && other_file_in &&
+ !strcmp(file_in, "-") && !strcmp(other_file_in, "-")) {
+ p_err("cannot use standard input for both data_in and ctx_in");
+ return -1;
+ }
+
+ return 0;
+}
+
+static int get_run_data(const char *fname, void **data_ptr, unsigned int *size)
+{
+ size_t block_size = 256;
+ size_t buf_size = block_size;
+ size_t nb_read = 0;
+ void *tmp;
+ FILE *f;
+
+ if (!fname) {
+ *data_ptr = NULL;
+ *size = 0;
+ return 0;
+ }
+
+ if (!strcmp(fname, "-"))
+ f = stdin;
+ else
+ f = fopen(fname, "r");
+ if (!f) {
+ p_err("failed to open %s: %s", fname, strerror(errno));
+ return -1;
+ }
+
+ *data_ptr = malloc(block_size);
+ if (!*data_ptr) {
+ p_err("failed to allocate memory for data_in/ctx_in: %s",
+ strerror(errno));
+ goto err_fclose;
+ }
+
+ while ((nb_read += fread(*data_ptr + nb_read, 1, block_size, f))) {
+ if (feof(f))
+ break;
+ if (ferror(f)) {
+ p_err("failed to read data_in/ctx_in from %s: %s",
+ fname, strerror(errno));
+ goto err_free;
+ }
+ if (nb_read > buf_size - block_size) {
+ if (buf_size == UINT32_MAX) {
+ p_err("data_in/ctx_in is too long (max: %d)",
+ UINT32_MAX);
+ goto err_free;
+ }
+ /* No space for fread()-ing next chunk; realloc() */
+ buf_size *= 2;
+ tmp = realloc(*data_ptr, buf_size);
+ if (!tmp) {
+ p_err("failed to reallocate data_in/ctx_in: %s",
+ strerror(errno));
+ goto err_free;
+ }
+ *data_ptr = tmp;
+ }
+ }
+ if (f != stdin)
+ fclose(f);
+
+ *size = nb_read;
+ return 0;
+
+err_free:
+ free(*data_ptr);
+ *data_ptr = NULL;
+err_fclose:
+ if (f != stdin)
+ fclose(f);
+ return -1;
+}
+
+static void hex_print(void *data, unsigned int size, FILE *f)
+{
+ size_t i, j;
+ char c;
+
+ for (i = 0; i < size; i += 16) {
+ /* Row offset */
+ fprintf(f, "%07zx\t", i);
+
+ /* Hexadecimal values */
+ for (j = i; j < i + 16 && j < size; j++)
+ fprintf(f, "%02x%s", *(uint8_t *)(data + j),
+ j % 2 ? " " : "");
+ for (; j < i + 16; j++)
+ fprintf(f, " %s", j % 2 ? " " : "");
+
+ /* ASCII values (if relevant), '.' otherwise */
+ fprintf(f, "| ");
+ for (j = i; j < i + 16 && j < size; j++) {
+ c = *(char *)(data + j);
+ if (c < ' ' || c > '~')
+ c = '.';
+ fprintf(f, "%c%s", c, j == i + 7 ? " " : "");
+ }
+
+ fprintf(f, "\n");
+ }
+}
+
+static int
+print_run_output(void *data, unsigned int size, const char *fname,
+ const char *json_key)
+{
+ size_t nb_written;
+ FILE *f;
+
+ if (!fname)
+ return 0;
+
+ if (!strcmp(fname, "-")) {
+ f = stdout;
+ if (json_output) {
+ jsonw_name(json_wtr, json_key);
+ print_data_json(data, size);
+ } else {
+ hex_print(data, size, f);
+ }
+ return 0;
+ }
+
+ f = fopen(fname, "w");
+ if (!f) {
+ p_err("failed to open %s: %s", fname, strerror(errno));
+ return -1;
+ }
+
+ nb_written = fwrite(data, 1, size, f);
+ fclose(f);
+ if (nb_written != size) {
+ p_err("failed to write output data/ctx: %s", strerror(errno));
+ return -1;
+ }
+
+ return 0;
+}
+
+static int alloc_run_data(void **data_ptr, unsigned int size_out)
+{
+ *data_ptr = calloc(size_out, 1);
+ if (!*data_ptr) {
+ p_err("failed to allocate memory for output data/ctx: %s",
+ strerror(errno));
+ return -1;
+ }
+
+ return 0;
+}
+
+static int do_run(int argc, char **argv)
+{
+ char *data_fname_in = NULL, *data_fname_out = NULL;
+ char *ctx_fname_in = NULL, *ctx_fname_out = NULL;
+ struct bpf_prog_test_run_attr test_attr = {0};
+ const unsigned int default_size = SZ_32K;
+ void *data_in = NULL, *data_out = NULL;
+ void *ctx_in = NULL, *ctx_out = NULL;
+ unsigned int repeat = 1;
+ int fd, err;
+
+ if (!REQ_ARGS(4))
+ return -1;
+
+ fd = prog_parse_fd(&argc, &argv);
+ if (fd < 0)
+ return -1;
+
+ while (argc) {
+ if (detect_common_prefix(*argv, "data_in", "data_out",
+ "data_size_out", NULL))
+ return -1;
+ if (detect_common_prefix(*argv, "ctx_in", "ctx_out",
+ "ctx_size_out", NULL))
+ return -1;
+
+ if (is_prefix(*argv, "data_in")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ data_fname_in = GET_ARG();
+ if (check_single_stdin(data_fname_in, ctx_fname_in))
+ return -1;
+ } else if (is_prefix(*argv, "data_out")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ data_fname_out = GET_ARG();
+ } else if (is_prefix(*argv, "data_size_out")) {
+ char *endptr;
+
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ test_attr.data_size_out = strtoul(*argv, &endptr, 0);
+ if (*endptr) {
+ p_err("can't parse %s as output data size",
+ *argv);
+ return -1;
+ }
+ NEXT_ARG();
+ } else if (is_prefix(*argv, "ctx_in")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ ctx_fname_in = GET_ARG();
+ if (check_single_stdin(ctx_fname_in, data_fname_in))
+ return -1;
+ } else if (is_prefix(*argv, "ctx_out")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ ctx_fname_out = GET_ARG();
+ } else if (is_prefix(*argv, "ctx_size_out")) {
+ char *endptr;
+
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ test_attr.ctx_size_out = strtoul(*argv, &endptr, 0);
+ if (*endptr) {
+ p_err("can't parse %s as output context size",
+ *argv);
+ return -1;
+ }
+ NEXT_ARG();
+ } else if (is_prefix(*argv, "repeat")) {
+ char *endptr;
+
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ repeat = strtoul(*argv, &endptr, 0);
+ if (*endptr) {
+ p_err("can't parse %s as repeat number",
+ *argv);
+ return -1;
+ }
+ NEXT_ARG();
+ } else {
+ p_err("expected no more arguments, 'data_in', 'data_out', 'data_size_out', 'ctx_in', 'ctx_out', 'ctx_size_out' or 'repeat', got: '%s'?",
+ *argv);
+ return -1;
+ }
+ }
+
+ err = get_run_data(data_fname_in, &data_in, &test_attr.data_size_in);
+ if (err)
+ return -1;
+
+ if (data_in) {
+ if (!test_attr.data_size_out)
+ test_attr.data_size_out = default_size;
+ err = alloc_run_data(&data_out, test_attr.data_size_out);
+ if (err)
+ goto free_data_in;
+ }
+
+ err = get_run_data(ctx_fname_in, &ctx_in, &test_attr.ctx_size_in);
+ if (err)
+ goto free_data_out;
+
+ if (ctx_in) {
+ if (!test_attr.ctx_size_out)
+ test_attr.ctx_size_out = default_size;
+ err = alloc_run_data(&ctx_out, test_attr.ctx_size_out);
+ if (err)
+ goto free_ctx_in;
+ }
+
+ test_attr.prog_fd = fd;
+ test_attr.repeat = repeat;
+ test_attr.data_in = data_in;
+ test_attr.data_out = data_out;
+ test_attr.ctx_in = ctx_in;
+ test_attr.ctx_out = ctx_out;
+
+ err = bpf_prog_test_run_xattr(&test_attr);
+ if (err) {
+ p_err("failed to run program: %s", strerror(errno));
+ goto free_ctx_out;
+ }
+
+ err = 0;
+
+ if (json_output)
+ jsonw_start_object(json_wtr); /* root */
+
+ /* Do not exit on errors occurring when printing output data/context,
+ * we still want to print return value and duration for program run.
+ */
+ if (test_attr.data_size_out)
+ err += print_run_output(test_attr.data_out,
+ test_attr.data_size_out,
+ data_fname_out, "data_out");
+ if (test_attr.ctx_size_out)
+ err += print_run_output(test_attr.ctx_out,
+ test_attr.ctx_size_out,
+ ctx_fname_out, "ctx_out");
+
+ if (json_output) {
+ jsonw_uint_field(json_wtr, "retval", test_attr.retval);
+ jsonw_uint_field(json_wtr, "duration", test_attr.duration);
+ jsonw_end_object(json_wtr); /* root */
+ } else {
+ fprintf(stdout, "Return value: %u, duration%s: %uns\n",
+ test_attr.retval,
+ repeat > 1 ? " (average)" : "", test_attr.duration);
+ }
+
+free_ctx_out:
+ free(ctx_out);
+free_ctx_in:
+ free(ctx_in);
+free_data_out:
+ free(data_out);
+free_data_in:
+ free(data_in);
+
+ return err;
+}
+
static int load_with_options(int argc, char **argv, bool first_prog_only)
{
struct bpf_object_load_attr load_attr = { 0 };
@@ -1058,6 +1397,11 @@ static int do_help(int argc, char **argv)
" [pinmaps MAP_DIR]\n"
" %s %s attach PROG ATTACH_TYPE [MAP]\n"
" %s %s detach PROG ATTACH_TYPE [MAP]\n"
+ " %s %s run PROG \\\n"
+ " data_in FILE \\\n"
+ " [data_out FILE [data_size_out L]] \\\n"
+ " [ctx_in FILE [ctx_out FILE [ctx_size_out M]]] \\\n"
+ " [repeat N]\n"
" %s %s tracelog\n"
" %s %s help\n"
"\n"
@@ -1079,7 +1423,8 @@ static int do_help(int argc, char **argv)
"",
bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
- bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2]);
+ bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
+ bin_name, argv[-2]);
return 0;
}
@@ -1095,6 +1440,7 @@ static const struct cmd cmds[] = {
{ "attach", do_attach },
{ "detach", do_detach },
{ "tracelog", do_tracelog },
+ { "run", do_run },
{ 0 }
};
diff --git a/tools/include/linux/sizes.h b/tools/include/linux/sizes.h
new file mode 100644
index 000000000000..1cbb4c4d016e
--- /dev/null
+++ b/tools/include/linux/sizes.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * include/linux/sizes.h
+ */
+#ifndef __LINUX_SIZES_H__
+#define __LINUX_SIZES_H__
+
+#include <linux/const.h>
+
+#define SZ_1 0x00000001
+#define SZ_2 0x00000002
+#define SZ_4 0x00000004
+#define SZ_8 0x00000008
+#define SZ_16 0x00000010
+#define SZ_32 0x00000020
+#define SZ_64 0x00000040
+#define SZ_128 0x00000080
+#define SZ_256 0x00000100
+#define SZ_512 0x00000200
+
+#define SZ_1K 0x00000400
+#define SZ_2K 0x00000800
+#define SZ_4K 0x00001000
+#define SZ_8K 0x00002000
+#define SZ_16K 0x00004000
+#define SZ_32K 0x00008000
+#define SZ_64K 0x00010000
+#define SZ_128K 0x00020000
+#define SZ_256K 0x00040000
+#define SZ_512K 0x00080000
+
+#define SZ_1M 0x00100000
+#define SZ_2M 0x00200000
+#define SZ_4M 0x00400000
+#define SZ_8M 0x00800000
+#define SZ_16M 0x01000000
+#define SZ_32M 0x02000000
+#define SZ_64M 0x04000000
+#define SZ_128M 0x08000000
+#define SZ_256M 0x10000000
+#define SZ_512M 0x20000000
+
+#define SZ_1G 0x40000000
+#define SZ_2G 0x80000000
+
+#define SZ_4G _AC(0x100000000, ULL)
+
+#endif /* __LINUX_SIZES_H__ */
--
2.17.1
^ permalink raw reply related
* [PATCH] tools bpftool: Fix json dump crash on powerpc
From: Jiri Olsa @ 2019-07-04 8:58 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: Michael Petlan, netdev, bpf, Martin KaFai Lau
Michael reported crash with by bpf program in json mode on powerpc:
# bpftool prog -p dump jited id 14
[{
"name": "0xd00000000a9aa760",
"insns": [{
"pc": "0x0",
"operation": "nop",
"operands": [null
]
},{
"pc": "0x4",
"operation": "nop",
"operands": [null
]
},{
"pc": "0x8",
"operation": "mflr",
Segmentation fault (core dumped)
The code is assuming char pointers in format, which is not always
true at least for powerpc. Fixing this by dumping the whole string
into buffer based on its format.
Please note that libopcodes code does not check return values from
fprintf callback, so there's no point to return error in case of
allocation failure.
Reported-by: Michael Petlan <mpetlan@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/bpf/bpftool/jit_disasm.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/tools/bpf/bpftool/jit_disasm.c b/tools/bpf/bpftool/jit_disasm.c
index 3ef3093560ba..05fa6dc970f8 100644
--- a/tools/bpf/bpftool/jit_disasm.c
+++ b/tools/bpf/bpftool/jit_disasm.c
@@ -11,6 +11,8 @@
* Licensed under the GNU General Public License, version 2.0 (GPLv2)
*/
+#define _GNU_SOURCE
+#include <stdio.h>
#include <stdarg.h>
#include <stdint.h>
#include <stdio.h>
@@ -44,11 +46,13 @@ static int fprintf_json(void *out, const char *fmt, ...)
char *s;
va_start(ap, fmt);
+ if (vasprintf(&s, fmt, ap) < 0)
+ return 0;
+ va_end(ap);
+
if (!oper_count) {
int i;
- s = va_arg(ap, char *);
-
/* Strip trailing spaces */
i = strlen(s) - 1;
while (s[i] == ' ')
@@ -61,11 +65,10 @@ static int fprintf_json(void *out, const char *fmt, ...)
} else if (!strcmp(fmt, ",")) {
/* Skip */
} else {
- s = va_arg(ap, char *);
jsonw_string(json_wtr, s);
oper_count++;
}
- va_end(ap);
+ free(s);
return 0;
}
--
2.21.0
^ permalink raw reply related
* [PATCH][net-next] net: remove unused parameter from skb_checksum_try_convert
From: Li RongQing @ 2019-07-04 9:03 UTC (permalink / raw)
To: netdev
the check parameter is never used
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
include/linux/skbuff.h | 8 +++-----
net/ipv4/gre_demux.c | 2 +-
net/ipv4/udp.c | 3 +--
net/ipv6/udp.c | 3 +--
4 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index c922ac8a8bd6..f0b5adeb644d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3914,18 +3914,16 @@ static inline bool __skb_checksum_convert_check(struct sk_buff *skb)
return (skb->ip_summed == CHECKSUM_NONE && skb->csum_valid);
}
-static inline void __skb_checksum_convert(struct sk_buff *skb,
- __sum16 check, __wsum pseudo)
+static inline void __skb_checksum_convert(struct sk_buff *skb, __wsum pseudo)
{
skb->csum = ~pseudo;
skb->ip_summed = CHECKSUM_COMPLETE;
}
-#define skb_checksum_try_convert(skb, proto, check, compute_pseudo) \
+#define skb_checksum_try_convert(skb, proto, compute_pseudo) \
do { \
if (__skb_checksum_convert_check(skb)) \
- __skb_checksum_convert(skb, check, \
- compute_pseudo(skb, proto)); \
+ __skb_checksum_convert(skb, compute_pseudo(skb, proto)); \
} while (0)
static inline void skb_remcsum_adjust_partial(struct sk_buff *skb, void *ptr,
diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c
index 293acfb36376..44bfeecac33e 100644
--- a/net/ipv4/gre_demux.c
+++ b/net/ipv4/gre_demux.c
@@ -83,7 +83,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
options = (__be32 *)(greh + 1);
if (greh->flags & GRE_CSUM) {
if (!skb_checksum_simple_validate(skb)) {
- skb_checksum_try_convert(skb, IPPROTO_GRE, 0,
+ skb_checksum_try_convert(skb, IPPROTO_GRE,
null_compute_pseudo);
} else if (csum_err) {
*csum_err = true;
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 1b971bd95786..c21862ba9c02 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2224,8 +2224,7 @@ static int udp_unicast_rcv_skb(struct sock *sk, struct sk_buff *skb,
int ret;
if (inet_get_convert_csum(sk) && uh->check && !IS_UDPLITE(sk))
- skb_checksum_try_convert(skb, IPPROTO_UDP, uh->check,
- inet_compute_pseudo);
+ skb_checksum_try_convert(skb, IPPROTO_UDP, inet_compute_pseudo);
ret = udp_queue_rcv_skb(sk, skb);
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 66ca5a4b17c4..4406e059da68 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -826,8 +826,7 @@ static int udp6_unicast_rcv_skb(struct sock *sk, struct sk_buff *skb,
int ret;
if (inet_get_convert_csum(sk) && uh->check && !IS_UDPLITE(sk))
- skb_checksum_try_convert(skb, IPPROTO_UDP, uh->check,
- ip6_compute_pseudo);
+ skb_checksum_try_convert(skb, IPPROTO_UDP, ip6_compute_pseudo);
ret = udpv6_queue_rcv_skb(sk, skb);
--
2.16.2
^ permalink raw reply related
* Re: [PATCH v6 net-next 5/5] net: ethernet: ti: cpsw: add XDP support
From: Jesper Dangaard Brouer @ 2019-07-04 9:19 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: grygorii.strashko, hawk, davem, ast, linux-kernel, linux-omap,
xdp-newbies, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend, brouer
In-Reply-To: <20190703101903.8411-6-ivan.khoronzhuk@linaro.org>
On Wed, 3 Jul 2019 13:19:03 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> Add XDP support based on rx page_pool allocator, one frame per page.
> Page pool allocator is used with assumption that only one rx_handler
> is running simultaneously. DMA map/unmap is reused from page pool
> despite there is no need to map whole page.
>
> Due to specific of cpsw, the same TX/RX handler can be used by 2
> network devices, so special fields in buffer are added to identify
> an interface the frame is destined to. Thus XDP works for both
> interfaces, that allows to test xdp redirect between two interfaces
> easily. Aslo, each rx queue have own page pools, but common for both
> netdevs.
>
> XDP prog is common for all channels till appropriate changes are added
> in XDP infrastructure. Also, once page_pool recycling becomes part of
> skb netstack some simplifications can be added, like removing
> page_pool_release_page() before skb receive.
>
> In order to keep rx_dev while redirect, that can be somehow used in
> future, do flush in rx_handler, that allows to keep rx dev the same
> while reidrect. It allows to conform with tracing rx_dev pointed
> by Jesper.
So, you simply call xdp_do_flush_map() after each xdp_do_redirect().
It will kill RX-bulk and performance, but I guess it will work.
I guess, we can optimized it later, by e.g. in function calling
cpsw_run_xdp() have a variable that detect if net_device changed
(priv->ndev) and then call xdp_do_flush_map() when needed.
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> drivers/net/ethernet/ti/Kconfig | 1 +
> drivers/net/ethernet/ti/cpsw.c | 485 ++++++++++++++++++++++---
> drivers/net/ethernet/ti/cpsw_ethtool.c | 66 +++-
> drivers/net/ethernet/ti/cpsw_priv.h | 7 +
> 4 files changed, 502 insertions(+), 57 deletions(-)
>
[...]
> +static int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,
> + struct page *page)
> +{
> + struct cpsw_common *cpsw = priv->cpsw;
> + struct net_device *ndev = priv->ndev;
> + int ret = CPSW_XDP_CONSUMED;
> + struct xdp_frame *xdpf;
> + struct bpf_prog *prog;
> + u32 act;
> +
> + rcu_read_lock();
> +
> + prog = READ_ONCE(priv->xdp_prog);
> + if (!prog) {
> + ret = CPSW_XDP_PASS;
> + goto out;
> + }
> +
> + act = bpf_prog_run_xdp(prog, xdp);
> + switch (act) {
> + case XDP_PASS:
> + ret = CPSW_XDP_PASS;
> + break;
> + case XDP_TX:
> + xdpf = convert_to_xdp_frame(xdp);
> + if (unlikely(!xdpf))
> + goto drop;
> +
> + cpsw_xdp_tx_frame(priv, xdpf, page);
> + break;
> + case XDP_REDIRECT:
> + if (xdp_do_redirect(ndev, xdp, prog))
> + goto drop;
> +
> + /* as flush requires rx_dev to be per NAPI handle and there
> + * is can be two devices putting packets on bulk queue,
> + * do flush here avoid this just for sure.
> + */
> + xdp_do_flush_map();
> + break;
> + default:
> + bpf_warn_invalid_xdp_action(act);
> + /* fall through */
> + case XDP_ABORTED:
> + trace_xdp_exception(ndev, prog, act);
> + /* fall through -- handle aborts by dropping packet */
> + case XDP_DROP:
> + goto drop;
> + }
> +out:
> + rcu_read_unlock();
> + return ret;
> +drop:
> + rcu_read_unlock();
> + page_pool_recycle_direct(cpsw->page_pool[ch], page);
> + return ret;
> +}
--
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 v2 1/3] vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock
From: Stefano Garzarella @ 2019-07-04 9:20 UTC (permalink / raw)
To: Jason Wang
Cc: Stefan Hajnoczi, netdev, kvm, virtualization, Michael S. Tsirkin,
David S. Miller, linux-kernel
In-Reply-To: <07e5bc00-ebde-4dac-d38c-f008fa230b5f@redhat.com>
On Thu, Jul 04, 2019 at 11:58:00AM +0800, Jason Wang wrote:
>
> On 2019/7/3 下午6:41, Stefano Garzarella wrote:
> > On Wed, Jul 03, 2019 at 05:53:58PM +0800, Jason Wang wrote:
> > > On 2019/6/28 下午8:36, Stefano Garzarella wrote:
> > > > Some callbacks used by the upper layers can run while we are in the
> > > > .remove(). A potential use-after-free can happen, because we free
> > > > the_virtio_vsock without knowing if the callbacks are over or not.
> > > >
> > > > To solve this issue we move the assignment of the_virtio_vsock at the
> > > > end of .probe(), when we finished all the initialization, and at the
> > > > beginning of .remove(), before to release resources.
> > > > For the same reason, we do the same also for the vdev->priv.
> > > >
> > > > We use RCU to be sure that all callbacks that use the_virtio_vsock
> > > > ended before freeing it. This is not required for callbacks that
> > > > use vdev->priv, because after the vdev->config->del_vqs() we are sure
> > > > that they are ended and will no longer be invoked.
> > > >
> > > > We also take the mutex during the .remove() to avoid that .probe() can
> > > > run while we are resetting the device.
> > > >
> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > ---
> > > > net/vmw_vsock/virtio_transport.c | 67 +++++++++++++++++++++-----------
> > > > 1 file changed, 44 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> > > > index 9c287e3e393c..7ad510ec12e0 100644
> > > > --- a/net/vmw_vsock/virtio_transport.c
> > > > +++ b/net/vmw_vsock/virtio_transport.c
> > > > @@ -65,19 +65,22 @@ struct virtio_vsock {
> > > > u32 guest_cid;
> > > > };
> > > > -static struct virtio_vsock *virtio_vsock_get(void)
> > > > -{
> > > > - return the_virtio_vsock;
> > > > -}
> > > > -
> > > > static u32 virtio_transport_get_local_cid(void)
> > > > {
> > > > - struct virtio_vsock *vsock = virtio_vsock_get();
> > > > + struct virtio_vsock *vsock;
> > > > + u32 ret;
> > > > - if (!vsock)
> > > > - return VMADDR_CID_ANY;
> > > > + rcu_read_lock();
> > > > + vsock = rcu_dereference(the_virtio_vsock);
> > > > + if (!vsock) {
> > > > + ret = VMADDR_CID_ANY;
> > > > + goto out_rcu;
> > > > + }
> > > > - return vsock->guest_cid;
> > > > + ret = vsock->guest_cid;
> > > > +out_rcu:
> > > > + rcu_read_unlock();
> > > > + return ret;
> > > > }
> > > > static void virtio_transport_loopback_work(struct work_struct *work)
> > > > @@ -197,14 +200,18 @@ virtio_transport_send_pkt(struct virtio_vsock_pkt *pkt)
> > > > struct virtio_vsock *vsock;
> > > > int len = pkt->len;
> > > > - vsock = virtio_vsock_get();
> > > > + rcu_read_lock();
> > > > + vsock = rcu_dereference(the_virtio_vsock);
> > > > if (!vsock) {
> > > > virtio_transport_free_pkt(pkt);
> > > > - return -ENODEV;
> > > > + len = -ENODEV;
> > > > + goto out_rcu;
> > > > }
> > > > - if (le64_to_cpu(pkt->hdr.dst_cid) == vsock->guest_cid)
> > > > - return virtio_transport_send_pkt_loopback(vsock, pkt);
> > > > + if (le64_to_cpu(pkt->hdr.dst_cid) == vsock->guest_cid) {
> > > > + len = virtio_transport_send_pkt_loopback(vsock, pkt);
> > > > + goto out_rcu;
> > > > + }
> > > > if (pkt->reply)
> > > > atomic_inc(&vsock->queued_replies);
> > > > @@ -214,6 +221,9 @@ virtio_transport_send_pkt(struct virtio_vsock_pkt *pkt)
> > > > spin_unlock_bh(&vsock->send_pkt_list_lock);
> > > > queue_work(virtio_vsock_workqueue, &vsock->send_pkt_work);
> > > > +
> > > > +out_rcu:
> > > > + rcu_read_unlock();
> > > > return len;
> > > > }
> > > > @@ -222,12 +232,14 @@ virtio_transport_cancel_pkt(struct vsock_sock *vsk)
> > > > {
> > > > struct virtio_vsock *vsock;
> > > > struct virtio_vsock_pkt *pkt, *n;
> > > > - int cnt = 0;
> > > > + int cnt = 0, ret;
> > > > LIST_HEAD(freeme);
> > > > - vsock = virtio_vsock_get();
> > > > + rcu_read_lock();
> > > > + vsock = rcu_dereference(the_virtio_vsock);
> > > > if (!vsock) {
> > > > - return -ENODEV;
> > > > + ret = -ENODEV;
> > > > + goto out_rcu;
> > > > }
> > > > spin_lock_bh(&vsock->send_pkt_list_lock);
> > > > @@ -255,7 +267,11 @@ virtio_transport_cancel_pkt(struct vsock_sock *vsk)
> > > > queue_work(virtio_vsock_workqueue, &vsock->rx_work);
> > > > }
> > > > - return 0;
> > > > + ret = 0;
> > > > +
> > > > +out_rcu:
> > > > + rcu_read_unlock();
> > > > + return ret;
> > > > }
> > > > static void virtio_vsock_rx_fill(struct virtio_vsock *vsock)
> > > > @@ -590,8 +606,6 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
> > > > vsock->rx_buf_max_nr = 0;
> > > > atomic_set(&vsock->queued_replies, 0);
> > > > - vdev->priv = vsock;
> > > > - the_virtio_vsock = vsock;
> > > > mutex_init(&vsock->tx_lock);
> > > > mutex_init(&vsock->rx_lock);
> > > > mutex_init(&vsock->event_lock);
> > > > @@ -613,6 +627,9 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
> > > > virtio_vsock_event_fill(vsock);
> > > > mutex_unlock(&vsock->event_lock);
> > > > + vdev->priv = vsock;
> > > > + rcu_assign_pointer(the_virtio_vsock, vsock);
> > >
> > > You probably need to use rcu_dereference_protected() to access
> > > the_virtio_vsock in the function in order to survive from sparse.
> > >
> > Ooo, thanks!
> >
> > Do you mean when we check if the_virtio_vsock is not null at the beginning of
> > virtio_vsock_probe()?
>
>
> I mean instead of:
>
> /* Only one virtio-vsock device per guest is supported */
> if (the_virtio_vsock) {
> ret = -EBUSY;
> goto out;
> }
>
> you should use:
>
> if (rcu_dereference_protected(the_virtio_vosck,
> lock_dep_is_held(&the_virtio_vsock_mutex))
>
> ...
Okay, thanks for confirming! I'll send a v3 to fix this!
>
>
> >
> > > > +
> > > > mutex_unlock(&the_virtio_vsock_mutex);
> > > > return 0;
> > > > @@ -627,6 +644,12 @@ static void virtio_vsock_remove(struct virtio_device *vdev)
> > > > struct virtio_vsock *vsock = vdev->priv;
> > > > struct virtio_vsock_pkt *pkt;
> > > > + mutex_lock(&the_virtio_vsock_mutex);
> > > > +
> > > > + vdev->priv = NULL;
> > > > + rcu_assign_pointer(the_virtio_vsock, NULL);
> > >
> > > This is still suspicious, can we access the_virtio_vsock through vdev->priv?
> > > If yes, we may still get use-after-free since it was not protected by RCU.
> > We will free the object only after calling the del_vqs(), so we are sure
> > that the vq_callbacks ended and will no longer be invoked.
> > So, IIUC it shouldn't happen.
>
>
> Yes, but any dereference that is not done in vq_callbacks will be very
> dangerous in the future.
Right.
Do you think make sense to continue with this series in order to fix the
hot-unplug issue, then I'll work to refactor the driver code to use the refcnt
(as you suggested in patch 2) and singleton for the_virtio_vsock?
Thanks,
Stefano
^ 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