s390 common I/O layer locking (was: [PATCH v2 07/13] vfio/ccw: Convert to use vfio_register_group_dev())

From: Cornelia Huck <cohuck@redhat.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Eric Farman <farman@linux.ibm.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Heiko Carstens <hca@linux.ibm.com>,
	kvm@vger.kernel.org, linux-s390@vger.kernel.org,
	Peter Oberparleiter <oberpar@linux.ibm.com>,
	Halil Pasic <pasic@linux.ibm.com>,
	Vineeth Vijayan <vneethv@linux.ibm.com>,
	"Raj, Ashok" <ashok.raj@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Daniel Vetter <daniel@ffwll.ch>, Christoph Hellwig <hch@lst.de>,
	Leon Romanovsky <leonro@nvidia.com>,
	Max Gurtovoy <mgurtovoy@nvidia.com>,
	Tarun Gupta <targupta@nvidia.com>
Subject: s390 common I/O layer locking (was: [PATCH v2 07/13] vfio/ccw: Convert to use vfio_register_group_dev())
Date: Mon, 3 May 2021 12:54:40 +0200	[thread overview]
Message-ID: <20210503125440.0acd7c1f.cohuck@redhat.com> (raw)
In-Reply-To: <20210430171908.GD1370958@nvidia.com>

On Fri, 30 Apr 2021 14:19:08 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Fri, Apr 30, 2021 at 02:31:40PM +0200, Cornelia Huck wrote:
> > On Thu, 29 Apr 2021 15:13:47 -0300
> > Jason Gunthorpe <jgg@nvidia.com> wrote:

> > > All the checks for !private need some kind of locking. The driver core
> > > model is that the 'struct device_driver' callbacks are all called
> > > under the device_lock (this prevents the driver unbinding during the
> > > callback). I didn't check if ccs does this or not..  
> > 
> > probe/remove/shutdown are basically a forward of the callbacks at the
> > bus level.  
> 
> These are all covered by device_lock
> 
> > The css bus should make sure that we serialize
> > irq/sch_event/chp_event with probe/remove.  
> 
> Hum it doesn't look OK, like here:
> 
> css_process_crw()
>   css_evaluate_subchannel()
>    sch = bus_find_device()
>       -- So we have a refcount on the struct device
>    css_evaluate_known_subchannel() {
> 	if (sch->driver) {
> 		if (sch->driver->sch_event)
> 			ret = sch->driver->sch_event(sch, slow);
>    }
> 
> But the above call and touches to sch->driver (which is really just
> sch->dev.driver) are unlocked and racy.
> 
> I would hold the device_lock() over all touches to sch->driver outside
> of a driver core callback.

I think this issue did not come up much before, as most drivers on the
css bus tend to stay put during the lifetime of the device; but yes, it
seems we're missing some locking.

For the css bus, we need locking for the event callbacks; for irq, this
may interact with the subchannel lock and likely needs some care.

I also looked at the other busses in the common I/O layer: scm looks
good at a glance, ccwgroup and ccw have locking for online/offline; the
other callbacks for the ccw drivers probably need to take the device
lock as well.

Common I/O layer maintainers, does that look right?