Re: [PATCH AUTOSEL 5.4 01/10] devlink: remove reload failed checks in params get/set callbacks

[Jacob Keller]

On 9/8/2023 11:20 AM, Sasha Levin wrote:
> From: Jiri Pirko <jiri@xxxxxxxxxx>
> 
> [ Upstream commit 633d76ad01ad0321a1ace3e5cc4fed06753d7ac4 ]
> 
> The checks in question were introduced by:
> commit 6b4db2e528f6 ("devlink: Fix use-after-free after a failed reload").
> That fixed an issue of reload with mlxsw driver.
> 
> Back then, that was a valid fix, because there was a limitation
> in place that prevented drivers from registering/unregistering params
> when devlink instance was registered.
> 
> It was possible to do the fix differently by changing drivers to
> register/unregister params in appropriate places making sure the ops
> operate only on memory which is allocated and initialized. But that,
> as a dependency, would require to remove the limitation mentioned above.
> 
> Eventually, this limitation was lifted by:
> commit 1d18bb1a4ddd ("devlink: allow registering parameters after the instance")
> 
> Also, the alternative fix (which also fixed another issue) was done by:
> commit 74cbc3c03c82 ("mlxsw: spectrum_acl_tcam: Move devlink param to TCAM code").
> 
> Therefore, the checks are no longer relevant. Each driver should make
> sure to have the params registered only when the memory the ops
> are working with is allocated and initialized.
> 
> So remove the checks.
> 


Hmmmm. Based on the description above this feels a bit odd to backport.
Are we sure that its safe to remove this limitation on older kernels?

Both mentioned commits are in v6.3 so they're not in any of the stable
trees by default.

Indeed grep over stable/linux-5.4.y shows nothing for either commit.

Thus, I am not convinced this is safe to backport. I didn't double check
every single stable branch but given that the mentioned dependencies are
in 6.3 and don't appear to have been fixes, it seems problematic for all
including 5.4, 5.10, 5.15, and 6.1. No driver in those trees is going to
be registering parameters early so I don't see the benefit of the patch.

Thus, it is my view this shouldn't be backported, at least not without
porting the relevant dependencies as well.

Thanks,
Jake

> Signed-off-by: Jiri Pirko <jiri@xxxxxxxxxx>
> Reviewed-by: Ido Schimmel <idosch@xxxxxxxxxx>
> Reviewed-by: Jakub Kicinski <kuba@xxxxxxxxxx>
> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> ---
>  net/core/devlink.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index b4dabe5d89f72..5bd6330ab4275 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -2953,7 +2953,7 @@ static int devlink_param_get(struct devlink *devlink,
>  			     const struct devlink_param *param,
>  			     struct devlink_param_gset_ctx *ctx)
>  {
> -	if (!param->get || devlink->reload_failed)
> +	if (!param->get)
>  		return -EOPNOTSUPP;
>  	return param->get(devlink, param->id, ctx);
>  }
> @@ -2962,7 +2962,7 @@ static int devlink_param_set(struct devlink *devlink,
>  			     const struct devlink_param *param,
>  			     struct devlink_param_gset_ctx *ctx)
>  {
> -	if (!param->set || devlink->reload_failed)
> +	if (!param->set)
>  		return -EOPNOTSUPP;
>  	return param->set(devlink, param->id, ctx);
>  }