netdev - Re: [PATCH rdma-next 4/4] net: mana: Add support for auxiliary device servicing events

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250420105309.GC10635@unreal>
Date: Sun, 20 Apr 2025 13:53:09 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Konstantin Taranov <kotaranov@...ux.microsoft.com>
Cc: kotaranov@...rosoft.com, pabeni@...hat.com, haiyangz@...rosoft.com,
	kys@...rosoft.com, edumazet@...gle.com, kuba@...nel.org,
	davem@...emloft.net, decui@...rosoft.com, wei.liu@...nel.org,
	longli@...rosoft.com, jgg@...pe.ca, linux-rdma@...r.kernel.org,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH rdma-next 4/4] net: mana: Add support for auxiliary
 device servicing events

On Mon, Apr 14, 2025 at 11:28:49AM -0700, Konstantin Taranov wrote:
> From: Shiraz Saleem <shirazsaleem@...rosoft.com>
> 
> Handle soc servcing events which require the rdma auxiliary device resources to
> be cleaned up during a suspend, and re-initialized during a resume.
> 
> Signed-off-by: Shiraz Saleem <shirazsaleem@...rosoft.com>
> Signed-off-by: Konstantin Taranov <kotaranov@...rosoft.com>
> ---
>  .../net/ethernet/microsoft/mana/gdma_main.c   | 11 +++-
>  .../net/ethernet/microsoft/mana/hw_channel.c  | 19 ++++++
>  drivers/net/ethernet/microsoft/mana/mana_en.c | 60 +++++++++++++++++++
>  include/net/mana/gdma.h                       | 18 ++++++
>  include/net/mana/hw_channel.h                 |  9 +++
>  5 files changed, 116 insertions(+), 1 deletion(-)

<...>

> @@ -1474,6 +1481,8 @@ static void mana_gd_cleanup(struct pci_dev *pdev)
>  	mana_hwc_destroy_channel(gc);
>  
>  	mana_gd_remove_irqs(pdev);
> +
> +	destroy_workqueue(gc->service_wq);
>  }

<...>

> +static void mana_handle_rdma_servicing(struct work_struct *work)
> +{
> +	struct mana_service_work *serv_work =
> +		container_of(work, struct mana_service_work, work);
> +	struct gdma_dev *gd = serv_work->gdma_dev;
> +	struct device *dev = gd->gdma_context->dev;
> +	int ret;
> +
> +	switch (serv_work->event) {
> +	case GDMA_SERVICE_TYPE_RDMA_SUSPEND:
> +		if (!gd->adev || gd->is_suspended)
> +			break;
> +
> +		remove_adev(gd);
> +		gd->is_suspended = true;
> +		break;
> +
> +	case GDMA_SERVICE_TYPE_RDMA_RESUME:
> +		if (!gd->is_suspended)
> +			break;
> +
> +		ret = add_adev(gd, "rdma");
> +		if (ret)
> +			dev_err(dev, "Failed to add adev on resume: %d\n", ret);
> +		else
> +			gd->is_suspended = false;
> +		break;
> +
> +	default:
> +		dev_warn(dev, "unknown adev service event %u\n",
> +			 serv_work->event);
> +		break;
> +	}
> +
> +	kfree(serv_work);

The series looks ok to me, except one question. Are you sure that it is
safe to have not-connected and not-locked general work while add_adev/remove_adev
can be called in parallel from different thread? For example getting event
GDMA_SERVICE_TYPE_RDMA_SUSPEND while mana_gd_probe() fails or some other
intervention with PCI (GDMA_SERVICE_TYPE_RDMA_SUSPEND and PCI shutdown).

What type of protection do you have here?

Thanks