linux-kernel - 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] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <BL1PR21MB3089EF4639B5CC2AD5E70B96C9852@BL1PR21MB3089.namprd21.prod.outlook.com>
Date: Thu, 24 Apr 2025 02:33:24 +0000
From: Shiraz Saleem <shirazsaleem@...rosoft.com>
To: Leon Romanovsky <leon@...nel.org>, Konstantin Taranov
	<kotaranov@...ux.microsoft.com>
CC: Konstantin Taranov <kotaranov@...rosoft.com>, "pabeni@...hat.com"
	<pabeni@...hat.com>, Haiyang Zhang <haiyangz@...rosoft.com>, KY Srinivasan
	<kys@...rosoft.com>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "davem@...emloft.net"
	<davem@...emloft.net>, Dexuan Cui <decui@...rosoft.com>, "wei.liu@...nel.org"
	<wei.liu@...nel.org>, Long Li <longli@...rosoft.com>, "jgg@...pe.ca"
	<jgg@...pe.ca>, "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH rdma-next 4/4] net: mana: Add support for auxiliary device
 servicing events

> Subject: [EXTERNAL] 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?
> 
Hi Leon,

Thanks for spotting this.

There are two cases.

-Probe / Resume
add_adev() stores gd->adev only after auxiliary_device_add() succeeds.
While gd->adev is still NULL the worker drops any GDMA_SERVICE_TYPE_RDMA_SUSPEND event, so an early suspend that arrives during probe is harmless and cannot race with the later add_adev().

-Remove / Suspend / Shutdown
During teardown the worker may still be inside add_adev()/remove_adev() while the PCI thread starts its own remove_adev(). 

In v2 I ll serialize them with flag + flush pattern.

void mana_rdma_remove(struct gdma_dev *gd)
{
[....]
        WRITE_ONCE(gd->rdma_teardown, true);   /* block new events      */
        flush_workqueue(gc->service_wq);       /* wait running worker   */

        if (gd->adev)
                remove_adev(gd);

[....]
}
i.e. during teardown, we stop the producer and drain the queue

and,

static void mana_handle_rdma_servicing(struct work_struct *work)
{
[....]
	if (READ_ONCE(gd->rdma_teardown))
		goto out;
[.....]
}
The flag blocks any new work, and flush_workqueue() waits for anything already running. This serialises the two paths and removes
the race you pointed out.

Shiraz