Re: [PATCH net 1/5] net/mlx5e: Fix netif state handling

[Simon Horman <horms@kernel.org>]

On Thu, May 09, 2024 at 02:29:47PM +0300, Tariq Toukan wrote:
> From: Shay Drory <shayd@nvidia.com>
> 
> mlx5e_suspend cleans resources only if netif_device_present() returns
> true. However, mlx5e_resume changes the state of netif, via
> mlx5e_nic_enable, only if reg_state == NETREG_REGISTERED.
> In the below case, the above leads to NULL-ptr Oops[1] and memory
> leaks:
> 
> mlx5e_probe
>  _mlx5e_resume
>   mlx5e_attach_netdev
>    mlx5e_nic_enable  <-- netdev not reg, not calling netif_device_attach()
>   register_netdev <-- failed for some reason.
> ERROR_FLOW:
>  _mlx5e_suspend <-- netif_device_present return false, resources aren't freed :(
> 
> Hence, clean resources in this case as well.
> 
> [1]
> BUG: kernel NULL pointer dereference, address: 0000000000000000

...

> Fixes: 2c3b5beec46a ("net/mlx5e: More generic netdev management API")
> Signed-off-by: Shay Drory <shayd@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>

Hi,

I think that this bug is caused by asymmetry in resource allocation/freeing
such that there are cases where _mlx5e_suspend() doesn't unwind
_mlx5e_resume().

It seems to me that asymmetry was introduced by the check for
reg_state != NETREG_REGISTERED in mlx5e_nic_enable() by:

610e89e05c3f ("net/mlx5e: Don't sync netdev state when not registered")

So perhaps that is a more appropriate commit for the Fixes tag.
I do note that commit was a fix for:

26e59d8077a3 ("net/mlx5e: Implement mlx5e interface attach/detach callbacks")

So perhaps a second fixes tag for that commit is also appropriate.

Perhaps it's not important enough to revise things, I don't feel strongly
about it, so feel free to add the following regardless.

Reviewed-by: Simon Horman <horms@kernel.org>

All that said, I do wonder if it would be better in the long run to
implement things in such a way that there is symmetry in resource
allocation / deallocation. Passing flags to control how much cleanup is
performed does seem a bit awkward.