linux-kernel - RE: [PATCH v2 1/4] cdx: Introduce lock to protect controller ops and controller list

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]

Date:   Fri, 4 Aug 2023 11:11:39 +0000
From:   "Gangurde, Abhijit" <abhijit.gangurde@....com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "masahiroy@...nel.org" <masahiroy@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Simek, Michal" <michal.simek@....com>,
        "git (AMD-Xilinx)" <git@....com>,
        "Agarwal, Nikhil" <nikhil.agarwal@....com>,
        "Gupta, Nipun" <Nipun.Gupta@....com>
Subject: RE: [PATCH v2 1/4] cdx: Introduce lock to protect controller ops and
 controller list

> On Wed, Aug 02, 2023 at 11:20:17AM +0000, Gangurde, Abhijit wrote:
> > > > > From: Greg KH <gregkh@...uxfoundation.org>
> > > > > Sent: Monday, July 31, 2023 5:55 PM
> > > > > To: Gangurde, Abhijit <abhijit.gangurde@....com>
> > > > > Cc: masahiroy@...nel.org; linux-kernel@...r.kernel.org; Simek, Michal
> > > > > <michal.simek@....com>; git (AMD-Xilinx) <git@....com>; Agarwal,
> > > Nikhil
> > > > > <nikhil.agarwal@....com>; Gupta, Nipun <Nipun.Gupta@....com>
> > > > > Subject: Re: [PATCH v2 1/4] cdx: Introduce lock to protect controller
> ops
> > > and
> > > > > controller list
> > > > >
> > > > > On Mon, Jul 31, 2023 at 05:38:10PM +0530, Abhijit Gangurde wrote:
> > > > > > Add a mutex lock to prevent race between controller ops initiated by
> > > > > > the bus subsystem and the controller registration/unregistration.
> > > > > >
> > > > > > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@....com>
> > > > > > ---
> > > > > >  drivers/cdx/cdx.c | 14 ++++++++++++++
> > > > > >  1 file changed, 14 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> > > > > > index d2cad4c670a0..66797c8fe400 100644
> > > > > > --- a/drivers/cdx/cdx.c
> > > > > > +++ b/drivers/cdx/cdx.c
> > > > > > @@ -72,6 +72,8 @@
> > > > > >
> > > > > >  /* CDX controllers registered with the CDX bus */
> > > > > >  static DEFINE_XARRAY_ALLOC(cdx_controllers);
> > > > > > +/* Lock to protect controller ops and controller list */
> > > > > > +static DEFINE_MUTEX(cdx_controller_lock);
> > > > >
> > > > > Wait, why do you have a local list and not just rely on the list the
> > > > > driver core has for you already?  Isn't this a duplicate list where you
> > > > > have objects on two different lists with a lifespan controlled only by
> > > > > one of them?
> > > >
> > > > cdx_controllers list is holding just the controllers registered on the cdx bus
> > > system.
> > >
> > > Which are devices on the bus, so why do you need a separate list?
> > >
> > > > CDX devices are still maintained by driver core list. Controller list is used
> by
> > > rescan
> > > > which triggers rescan on all the controllers.
> > >
> > > Again, why a separate list?  The driver core already tracks these,
> > > right?
> >
> > As of now, cdx controllers are platform devices and maintained on
> cdx_controllers list.
> 
> Oh, that's not ok.  Please do NOT abuse platform devices for things that
> are not actually platform devices.  Make them real devices on a real
> bus.
> 
> > CDX controller devices are not added on cdx bus. IIUC, you mean to use
> driver core
> > list to find out different cdx controllers, in that case cdx bus would need to
> scan
> > platform bus and access the private data of platform device to get a
> cdx_controller ops.
> > IMHO, that would not be a right approach.
> 
> If these are actually real patform devices, then yes, that's the correct
> thing to do.
> 
> Or you can create a cdx controller device and add that to the device
> tree, that's usually the way "controller" devices work (look at USB host
> controllers as one example.)

Thank you for the suggestion. CDX controller is already in the device tree as
a platform device. We are weighing both the options right now and would
send the changes accordingly in next spin.

Thanks,
Abhijit

> 
> > Or as an alternative cdx controller could be added on cdx bus as well. And we
> can then
> > get these controllers from driver core list.
> 
> Yes, that can work too, but don't keep them outside of the driver model,
> that will not work well over time, as you can see here already.
> 
> thanks,
> 
> greg k-h