| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 5 Apr 2018 23:17:59 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: David Ahern <dsahern@...il.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, idosch@...lanox.com,
jakub.kicinski@...ronome.com, mlxsw@...lanox.com
Subject: Re: [patch net] devlink: convert occ_get op to separate registration
Thu, Apr 05, 2018 at 10:55:58PM CEST, dsahern@...il.com wrote:
>On 4/5/18 2:13 PM, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@...lanox.com>
>>
>> This resolves race during initialization where the resources with
>> ops are registered before driver and the structures used by occ_get
>> op is initialized. So keep occ_get callbacks registered only when
>> all structs are initialized.
>>
>> The example flows, as it is in mlxsw:
>> 1) driver load/asic probe:
>> mlxsw_core
>> -> mlxsw_sp_resources_register
>> -> mlxsw_sp_kvdl_resources_register
>> -> devlink_resource_register IDX
>> mlxsw_spectrum
>> -> mlxsw_sp_kvdl_init
>> -> mlxsw_sp_kvdl_parts_init
>> -> mlxsw_sp_kvdl_part_init
>> -> devlink_resource_size_get IDX (to get the current setup
>> size from devlink)
>> -> devlink_resource_occ_get_register IDX (register current
>> occupancy getter)
>> 2) reload triggered by devlink command:
>> -> mlxsw_devlink_core_bus_device_reload
>> -> mlxsw_sp_fini
>> -> mlxsw_sp_kvdl_fini
>> -> devlink_resource_occ_get_unregister IDX
>> (struct mlxsw_sp *mlxsw_sp is freed at this point, call to occ get
>> which is using mlxsw_sp would cause use-after free)
>> -> mlxsw_sp_init
>> -> mlxsw_sp_kvdl_init
>> -> mlxsw_sp_kvdl_parts_init
>> -> mlxsw_sp_kvdl_part_init
>> -> devlink_resource_size_get IDX (to get the current setup
>> size from devlink)
>> -> devlink_resource_occ_get_register IDX (register current
>> occupancy getter)
>>
>> Fixes: d9f9b9a4d05f ("devlink: Add support for resource abstraction")
>> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
>> ---
>
>
>Why won't something like the attached work as opposed to playing
>register / unregister games?
Because it is quite driver-specific. For example in netdevsim, with
current implementation you don't have to unreg as the priv is there the
whole time.
Also, I think it is more correct to register getter with the priv
directly. Not to depend on getting with via devlink struct somehow.
I'm not saying that my solution is super nice. But what you suggest
seems much more uglier to me.
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
>index 93ea56620a24..dcded613faa6 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
>@@ -113,6 +113,7 @@ struct mlxsw_core {
> struct mlxsw_thermal *thermal;
> struct mlxsw_core_port *ports;
> unsigned int max_ports;
>+ bool reload_in_progress;
> bool reload_fail;
> unsigned long driver_priv[0];
> /* driver_priv has to be always the last item */
>@@ -154,6 +155,12 @@ void *mlxsw_core_driver_priv(struct mlxsw_core *mlxsw_core)
> }
> EXPORT_SYMBOL(mlxsw_core_driver_priv);
>
>+bool mlxsw_core_reload_in_progress(struct mlxsw_core *mlxsw_core)
>+{
>+ return mlxsw_core->mlxsw_core_driver_priv;
>+}
>+EXPORT_SYMBOL(mlxsw_core_reload_in_progress);
>+
> struct mlxsw_rx_listener_item {
> struct list_head list;
> struct mlxsw_rx_listener rxl;
>@@ -972,12 +979,16 @@ static int mlxsw_devlink_core_bus_device_reload(struct devlink *devlink)
> if (!mlxsw_bus->reset)
> return -EOPNOTSUPP;
>
>+ mlxsw_core->reload_in_progress = true;
>+
> mlxsw_core_bus_device_unregister(mlxsw_core, true);
> mlxsw_bus->reset(mlxsw_core->bus_priv);
> err = mlxsw_core_bus_device_register(mlxsw_core->bus_info,
> mlxsw_core->bus,
> mlxsw_core->bus_priv, true,
> devlink);
>+ mlxsw_core->reload_in_progress = false;
>+
> if (err)
> mlxsw_core->reload_fail = true;
> return err;
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
>index 092d39399f3c..ffa1db154757 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/core.h
>+++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
>@@ -60,6 +60,7 @@ struct mlxsw_bus_info;
> unsigned int mlxsw_core_max_ports(const struct mlxsw_core *mlxsw_core);
>
> void *mlxsw_core_driver_priv(struct mlxsw_core *mlxsw_core);
>+bool mlxsw_core_reload_in_progress(struct mlxsw_core *mlxsw_core);
>
> int mlxsw_core_driver_register(struct mlxsw_driver *mlxsw_driver);
> void mlxsw_core_driver_unregister(struct mlxsw_driver *mlxsw_driver);
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>index 53fffd09d133..09b89af37d8a 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>@@ -3808,8 +3808,12 @@ static const struct mlxsw_config_profile mlxsw_sp_config_profile = {
> static u64 mlxsw_sp_resource_kvd_linear_occ_get(struct devlink *devlink)
> {
> struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>- struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
>+ struct mlxsw_sp *mlxsw_sp;
>+
>+ if (mlxsw_core_reload_in_progress(mlxsw_core))
>+ return 0;
>
>+ mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> return mlxsw_sp_kvdl_occ_get(mlxsw_sp);
> }
>
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
>index 8796db44dcc3..dd66285bafb5 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
>@@ -329,9 +329,13 @@ u64 mlxsw_sp_kvdl_occ_get(const struct mlxsw_sp *mlxsw_sp)
> static u64 mlxsw_sp_kvdl_single_occ_get(struct devlink *devlink)
> {
> struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>- struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> struct mlxsw_sp_kvdl_part *part;
>+ struct mlxsw_sp *mlxsw_sp;
>
>+ if (mlxsw_core_reload_in_progress(mlxsw_core))
>+ return 0;
>+
>+ mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_SINGLE];
> return mlxsw_sp_kvdl_part_occ(part);
> }
>@@ -339,8 +343,13 @@ static u64 mlxsw_sp_kvdl_single_occ_get(struct devlink *devlink)
> static u64 mlxsw_sp_kvdl_chunks_occ_get(struct devlink *devlink)
> {
> struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>- struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> struct mlxsw_sp_kvdl_part *part;
>+ struct mlxsw_sp *mlxsw_sp;
>+
>+ if (mlxsw_core_reload_in_progress(mlxsw_core))
>+ return 0;
>+
>+ mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
>
> part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_CHUNKS];
> return mlxsw_sp_kvdl_part_occ(part);
>@@ -349,9 +358,13 @@ static u64 mlxsw_sp_kvdl_chunks_occ_get(struct devlink *devlink)
> static u64 mlxsw_sp_kvdl_large_chunks_occ_get(struct devlink *devlink)
> {
> struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>- struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> struct mlxsw_sp_kvdl_part *part;
>+ struct mlxsw_sp *mlxsw_sp;
>+
>+ if (mlxsw_core_reload_in_progress(mlxsw_core))
>+ return 0;
>
>+ mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_LARGE_CHUNKS];
> return mlxsw_sp_kvdl_part_occ(part);
> }
Powered by blists - more mailing lists