LinuxLists.cc - [PATCH v2] sound/hda: Add NULL check to component match callback function

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, 31 Mar 2022 11:25:43 +0200,
Heikki Krogerus wrote:
>
> On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > >
> > > > Can NULL dev->driver be really seen? I thought the components are
> > > > added by the drivers, hence they ought to have the driver field set.
> > > > But there can be corner cases I overlooked.
> > > >
> > > >
> > > > thanks,
> > > >
> > > > Takashi
> > >
> > > Hi Takashi,
> > >
> > > When I try using component_add in a different driver (usb4 in my
> > > case), I think dev->driver here is NULL because the i915 drivers do
> > > not have their component master fully bound when this new component is
> > > registered. When I test it, it seems to be causing a crash.
> >
> > Hm, from where component_add*() is called? Basically dev->driver must
> > be already set before the corresponding driver gets bound at
> > __driver_probe_deviec(). So, if the device is added to component from
> > the corresponding driver's probe, dev->driver must be non-NULL.
>
> The code that declares a device as component does not have to be the
> driver of that device.
>
> In our case the components are USB ports, and they are devices that
> are actually never bind to any drivers: drivers/usb/core/port.c

OK, that's what I wanted to know. It'd be helpful if it's more
clearly mentioned in the commit log.

BTW, the same problem must be seen in MEI drivers, too.

Takashi

2022-03-31 14:26:07

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, Mar 31, 2022 at 12:25:43PM +0300, Heikki Krogerus wrote:
> On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > >
> > > > Can NULL dev->driver be really seen? I thought the components are
> > > > added by the drivers, hence they ought to have the driver field set.
> > > > But there can be corner cases I overlooked.
> > > >
> > > >
> > > > thanks,
> > > >
> > > > Takashi
> > >
> > > Hi Takashi,
> > >
> > > When I try using component_add in a different driver (usb4 in my
> > > case), I think dev->driver here is NULL because the i915 drivers do
> > > not have their component master fully bound when this new component is
> > > registered. When I test it, it seems to be causing a crash.
> >
> > Hm, from where component_add*() is called? Basically dev->driver must
> > be already set before the corresponding driver gets bound at
> > __driver_probe_deviec(). So, if the device is added to component from
> > the corresponding driver's probe, dev->driver must be non-NULL.
>
> The code that declares a device as component does not have to be the
> driver of that device.
>
> In our case the components are USB ports, and they are devices that
> are actually never bind to any drivers: drivers/usb/core/port.c

Why is a USB device being passed to this code that assumes it is looking
for a PCI device with a specific driver name? As I mentioned on the
mei patch, triggering off of a name is really a bad idea, as is assuming
the device type without any assurance it is such a device (there's a
reason we didn't provide device type identification in the driver core,
don't abuse that please...)

thanks,

greg k-h

2022-03-31 15:32:33

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, 31 Mar 2022 11:34:38 +0200,
Heikki Krogerus wrote:
>
> On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
> > On Thu, 31 Mar 2022 11:25:43 +0200,
> > Heikki Krogerus wrote:
> > >
> > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > > > >
> > > > > > Can NULL dev->driver be really seen? I thought the components are
> > > > > > added by the drivers, hence they ought to have the driver field set.
> > > > > > But there can be corner cases I overlooked.
> > > > > >
> > > > > >
> > > > > > thanks,
> > > > > >
> > > > > > Takashi
> > > > >
> > > > > Hi Takashi,
> > > > >
> > > > > When I try using component_add in a different driver (usb4 in my
> > > > > case), I think dev->driver here is NULL because the i915 drivers do
> > > > > not have their component master fully bound when this new component is
> > > > > registered. When I test it, it seems to be causing a crash.
> > > >
> > > > Hm, from where component_add*() is called? Basically dev->driver must
> > > > be already set before the corresponding driver gets bound at
> > > > __driver_probe_deviec(). So, if the device is added to component from
> > > > the corresponding driver's probe, dev->driver must be non-NULL.
> > >
> > > The code that declares a device as component does not have to be the
> > > driver of that device.
> > >
> > > In our case the components are USB ports, and they are devices that
> > > are actually never bind to any drivers: drivers/usb/core/port.c
> >
> > OK, that's what I wanted to know. It'd be helpful if it's more
> > clearly mentioned in the commit log.
>
> Agree.
>
> > BTW, the same problem must be seen in MEI drivers, too.
>
> Wasn't there a patch for those too? I lost track...

I don't know, I just checked the latest Linus tree.

And, looking at the HD-audio code, I still wonder how NULL dev->driver
can reach there. Is there any PCI device that is added to component
without binding to a driver? We have dev_is_pci() check at the
beginning, so non-PCI devices should bail out there...

I'm not against adding NULL checks, but just for better understanding
the situation.

Takashi

2022-03-31 15:34:21

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

Hi Greg,

On Thu, Mar 31, 2022 at 01:39:51PM +0200, Greg KH wrote:
> On Thu, Mar 31, 2022 at 12:25:43PM +0300, Heikki Krogerus wrote:
> > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > > >
> > > > > Can NULL dev->driver be really seen? I thought the components are
> > > > > added by the drivers, hence they ought to have the driver field set.
> > > > > But there can be corner cases I overlooked.
> > > > >
> > > > >
> > > > > thanks,
> > > > >
> > > > > Takashi
> > > >
> > > > Hi Takashi,
> > > >
> > > > When I try using component_add in a different driver (usb4 in my
> > > > case), I think dev->driver here is NULL because the i915 drivers do
> > > > not have their component master fully bound when this new component is
> > > > registered. When I test it, it seems to be causing a crash.
> > >
> > > Hm, from where component_add*() is called? Basically dev->driver must
> > > be already set before the corresponding driver gets bound at
> > > __driver_probe_deviec(). So, if the device is added to component from
> > > the corresponding driver's probe, dev->driver must be non-NULL.
> >
> > The code that declares a device as component does not have to be the
> > driver of that device.
> >
> > In our case the components are USB ports, and they are devices that
> > are actually never bind to any drivers: drivers/usb/core/port.c
>
> Why is a USB device being passed to this code that assumes it is looking
> for a PCI device with a specific driver name? As I mentioned on the
> mei patch, triggering off of a name is really a bad idea, as is assuming
> the device type without any assurance it is such a device (there's a
> reason we didn't provide device type identification in the driver core,
> don't abuse that please...)

I totally agree. This driver is making a whole bunch of assumptions
when it should not make any assumptions. And yes, one of those
assumptions is that the driver of the device has a specific name, and
that is totally crazy. So why is it making those assumptions? I have
no idea, but is does, and they are now causing the first problem -
NULL pointer dereference.

This patch (and that other) is only proposing a simple way to solve
that NULL pointer dereference issue by adding some sanity checks. If
that's no OK, and the whole driver should be refactored instead, then
that is perfectly OK by me, but that has to be done by somebody who
understands what exactly is the driver and the device it's controlling
doing (and for).

thanks,

--
heikki

2022-03-31 16:22:51

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, 31 Mar 2022 14:52:14 +0200,
Heikki Krogerus wrote:
>
> Hi Greg,
>
> On Thu, Mar 31, 2022 at 01:39:51PM +0200, Greg KH wrote:
> > On Thu, Mar 31, 2022 at 12:25:43PM +0300, Heikki Krogerus wrote:
> > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > > > >
> > > > > > Can NULL dev->driver be really seen? I thought the components are
> > > > > > added by the drivers, hence they ought to have the driver field set.
> > > > > > But there can be corner cases I overlooked.
> > > > > >
> > > > > >
> > > > > > thanks,
> > > > > >
> > > > > > Takashi
> > > > >
> > > > > Hi Takashi,
> > > > >
> > > > > When I try using component_add in a different driver (usb4 in my
> > > > > case), I think dev->driver here is NULL because the i915 drivers do
> > > > > not have their component master fully bound when this new component is
> > > > > registered. When I test it, it seems to be causing a crash.
> > > >
> > > > Hm, from where component_add*() is called? Basically dev->driver must
> > > > be already set before the corresponding driver gets bound at
> > > > __driver_probe_deviec(). So, if the device is added to component from
> > > > the corresponding driver's probe, dev->driver must be non-NULL.
> > >
> > > The code that declares a device as component does not have to be the
> > > driver of that device.
> > >
> > > In our case the components are USB ports, and they are devices that
> > > are actually never bind to any drivers: drivers/usb/core/port.c
> >
> > Why is a USB device being passed to this code that assumes it is looking
> > for a PCI device with a specific driver name? As I mentioned on the
> > mei patch, triggering off of a name is really a bad idea, as is assuming
> > the device type without any assurance it is such a device (there's a
> > reason we didn't provide device type identification in the driver core,
> > don't abuse that please...)
>
> I totally agree. This driver is making a whole bunch of assumptions
> when it should not make any assumptions. And yes, one of those
> assumptions is that the driver of the device has a specific name, and
> that is totally crazy. So why is it making those assumptions? I have
> no idea, but is does, and they are now causing the first problem -
> NULL pointer dereference.

Well, it's a sort of best-effort approach for the component framework.
Currently the framework passes a device pointer without knowing what
it is and every master component tries to match with it unless it's
already bound. Because of that, the driver has to judge which one is
the right one by itself. The device driver's string is a loose
matching target that practically worked, so far.

Maybe we should define unique subcomponent numbers and rather check
the passed number at matching. (Though, only relying on the number is
dangerous, too.)

Takashi

2022-03-31 17:59:25

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
> On Thu, 31 Mar 2022 11:25:43 +0200,
> Heikki Krogerus wrote:
> >
> > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > > >
> > > > > Can NULL dev->driver be really seen? I thought the components are
> > > > > added by the drivers, hence they ought to have the driver field set.
> > > > > But there can be corner cases I overlooked.
> > > > >
> > > > >
> > > > > thanks,
> > > > >
> > > > > Takashi
> > > >
> > > > Hi Takashi,
> > > >
> > > > When I try using component_add in a different driver (usb4 in my
> > > > case), I think dev->driver here is NULL because the i915 drivers do
> > > > not have their component master fully bound when this new component is
> > > > registered. When I test it, it seems to be causing a crash.
> > >
> > > Hm, from where component_add*() is called? Basically dev->driver must
> > > be already set before the corresponding driver gets bound at
> > > __driver_probe_deviec(). So, if the device is added to component from
> > > the corresponding driver's probe, dev->driver must be non-NULL.
> >
> > The code that declares a device as component does not have to be the
> > driver of that device.
> >
> > In our case the components are USB ports, and they are devices that
> > are actually never bind to any drivers: drivers/usb/core/port.c
>
> OK, that's what I wanted to know. It'd be helpful if it's more
> clearly mentioned in the commit log.

Agree.

> BTW, the same problem must be seen in MEI drivers, too.

Wasn't there a patch for those too? I lost track...

thanks,

--
heikki

2022-03-31 17:59:48

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, Mar 31, 2022 at 09:58:43AM -0700, Won Chung wrote:
> On Thu, Mar 31, 2022 at 9:38 AM Greg KH <[email protected]> wrote:
> >
> > On Thu, Mar 31, 2022 at 08:33:03AM -0700, Benson Leung wrote:
> > > Hi Takashi,
> > >
> > > On Thu, Mar 31, 2022 at 04:19:15PM +0200, Takashi Iwai wrote:
> > > > On Thu, 31 Mar 2022 15:29:10 +0200,
> > > > Takashi Iwai wrote:
> > > > >
> > > > > On Thu, 31 Mar 2022 11:45:47 +0200,
> > > > > Takashi Iwai wrote:
> > > > > >
> > > > > > On Thu, 31 Mar 2022 11:34:38 +0200,
> > > > > > Heikki Krogerus wrote:
> > > > > > >
> > > > > > > On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
> > > > > > > > On Thu, 31 Mar 2022 11:25:43 +0200,
> > > > > > > > Heikki Krogerus wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > > > > > > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > > > > > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > > > > > > > > > >
> > > > > > > > > > > > Can NULL dev->driver be really seen? I thought the components are
> > > > > > > > > > > > added by the drivers, hence they ought to have the driver field set.
> > > > > > > > > > > > But there can be corner cases I overlooked.
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > thanks,
> > > > > > > > > > > >
> > > > > > > > > > > > Takashi
> > > > > > > > > > >
> > > > > > > > > > > Hi Takashi,
> > > > > > > > > > >
> > > > > > > > > > > When I try using component_add in a different driver (usb4 in my
> > > > > > > > > > > case), I think dev->driver here is NULL because the i915 drivers do
> > > > > > > > > > > not have their component master fully bound when this new component is
> > > > > > > > > > > registered. When I test it, it seems to be causing a crash.
> > > > > > > > > >
> > > > > > > > > > Hm, from where component_add*() is called? Basically dev->driver must
> > > > > > > > > > be already set before the corresponding driver gets bound at
> > > > > > > > > > __driver_probe_deviec(). So, if the device is added to component from
> > > > > > > > > > the corresponding driver's probe, dev->driver must be non-NULL.
> > > > > > > > >
> > > > > > > > > The code that declares a device as component does not have to be the
> > > > > > > > > driver of that device.
> > > > > > > > >
> > > > > > > > > In our case the components are USB ports, and they are devices that
> > > > > > > > > are actually never bind to any drivers: drivers/usb/core/port.c
> > > > > > > >
> > > > > > > > OK, that's what I wanted to know. It'd be helpful if it's more
> > > > > > > > clearly mentioned in the commit log.
> > > > > > >
> > > > > > > Agree.
> > > > > > >
> > > > > > > > BTW, the same problem must be seen in MEI drivers, too.
> > > > > > >
> > > > > > > Wasn't there a patch for those too? I lost track...
> > > > > >
> > > > > > I don't know, I just checked the latest Linus tree.
> > > > > >
> > > > > > And, looking at the HD-audio code, I still wonder how NULL dev->driver
> > > > > > can reach there. Is there any PCI device that is added to component
> > > > > > without binding to a driver? We have dev_is_pci() check at the
> > > > > > beginning, so non-PCI devices should bail out there...
> > > > >
> > > > > Further reading on, I'm really confused. How data=NULL can be passed
> > > > > to this function? The data argument is the value passed from the
> > > > > component_match_add_typed() call in HD-audio driver, hence it must be
> > > > > always the snd_hdac_bus object.
> > > > >
> > > > > And, I guess the i915 string check can be omitted completely, at
> > > > > least, for HD-audio driver. It already have a check of the parent of
> > > > > the device and that should be enough.
> > > >
> > > > That said, something like below (supposing data NULL check being
> > > > superfluous), instead.
> > > >
> > > >
> > > > Takashi
> > > >
> > > > --- a/sound/hda/hdac_i915.c
> > > > +++ b/sound/hda/hdac_i915.c
> > > > @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
> > > > struct pci_dev *hdac_pci, *i915_pci;
> > > > struct hdac_bus *bus = data;
> > > >
> > > > - if (!dev_is_pci(dev))
> > > > + if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev))
> > > > return 0;
> > > >
> > >
> > > If I recall this bug correctly, it's not the usb port perse that is falling
> > > through this !dev_is_pci(dev) check, it's actually the usb4-port in a new
> > > proposed patch by Heikki and Mika to extend the usb type-c component to
> > > encompass the usb4 specific pieces too. Is it possible usb4 ports are considered
> > > pci devices, and that's how we got into this situation?
> > >
> > > Also, a little more background information: This crash happens because in
> > > our kernel configs, we config'd the usb4 driver as =y (built in) instead of
> > > =m module, which meant that the usb4 port's driver was adding a component
> > > likely much earlier than hdac_i915.
> >
> > So is this actually triggering on 5.17 right now? Or is it due to some
> > other not-applied changes you are testing at the moment?
> >
> > confused,
> >
> > greg k-h
>
> Hi Greg,
>
> I believe it is not causing an issue in 5.17 at the moment. It is
> triggered when we try to apply new changes and test it locally.
> (registering a component for usb4_port)

Then why would it ever be needed to be backported to a stable kernel?

Please be more careful.

greg k-h

2022-03-31 18:07:27

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, Mar 31, 2022 at 10:33:57AM -0700, Benson Leung wrote:
> Hi Greg,
>
> On Thu, Mar 31, 2022 at 07:18:00PM +0200, Greg KH wrote:
> > On Thu, Mar 31, 2022 at 09:58:43AM -0700, Won Chung wrote:
> > > > So is this actually triggering on 5.17 right now? Or is it due to some
> > > > other not-applied changes you are testing at the moment?
> > > >
> > > > confused,
> > > >
> > > > greg k-h
> > >
> > > Hi Greg,
> > >
> > > I believe it is not causing an issue in 5.17 at the moment. It is
> > > triggered when we try to apply new changes and test it locally.
> > > (registering a component for usb4_port)
> >
> > Then why would it ever be needed to be backported to a stable kernel?
> >
> > Please be more careful.
> >
> > greg k-h
>
> Sorry about that. I gave Won bad advice to cc stable. You're right, it will
> only be relevant when a future patch lands in usb4.

It isn't even relevant now, please only worry about this when you have
your patches ready for submission that causes this breakage.

thanks,

greg k-h

2022-03-31 18:46:15

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, 31 Mar 2022 15:29:10 +0200,
Takashi Iwai wrote:
>
> On Thu, 31 Mar 2022 11:45:47 +0200,
> Takashi Iwai wrote:
> >
> > On Thu, 31 Mar 2022 11:34:38 +0200,
> > Heikki Krogerus wrote:
> > >
> > > On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
> > > > On Thu, 31 Mar 2022 11:25:43 +0200,
> > > > Heikki Krogerus wrote:
> > > > >
> > > > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > > > > > >
> > > > > > > > Can NULL dev->driver be really seen? I thought the components are
> > > > > > > > added by the drivers, hence they ought to have the driver field set.
> > > > > > > > But there can be corner cases I overlooked.
> > > > > > > >
> > > > > > > >
> > > > > > > > thanks,
> > > > > > > >
> > > > > > > > Takashi
> > > > > > >
> > > > > > > Hi Takashi,
> > > > > > >
> > > > > > > When I try using component_add in a different driver (usb4 in my
> > > > > > > case), I think dev->driver here is NULL because the i915 drivers do
> > > > > > > not have their component master fully bound when this new component is
> > > > > > > registered. When I test it, it seems to be causing a crash.
> > > > > >
> > > > > > Hm, from where component_add*() is called? Basically dev->driver must
> > > > > > be already set before the corresponding driver gets bound at
> > > > > > __driver_probe_deviec(). So, if the device is added to component from
> > > > > > the corresponding driver's probe, dev->driver must be non-NULL.
> > > > >
> > > > > The code that declares a device as component does not have to be the
> > > > > driver of that device.
> > > > >
> > > > > In our case the components are USB ports, and they are devices that
> > > > > are actually never bind to any drivers: drivers/usb/core/port.c
> > > >
> > > > OK, that's what I wanted to know. It'd be helpful if it's more
> > > > clearly mentioned in the commit log.
> > >
> > > Agree.
> > >
> > > > BTW, the same problem must be seen in MEI drivers, too.
> > >
> > > Wasn't there a patch for those too? I lost track...
> >
> > I don't know, I just checked the latest Linus tree.
> >
> > And, looking at the HD-audio code, I still wonder how NULL dev->driver
> > can reach there. Is there any PCI device that is added to component
> > without binding to a driver? We have dev_is_pci() check at the
> > beginning, so non-PCI devices should bail out there...
>
> Further reading on, I'm really confused. How data=NULL can be passed
> to this function? The data argument is the value passed from the
> component_match_add_typed() call in HD-audio driver, hence it must be
> always the snd_hdac_bus object.
>
> And, I guess the i915 string check can be omitted completely, at
> least, for HD-audio driver. It already have a check of the parent of
> the device and that should be enough.

That said, something like below (supposing data NULL check being
superfluous), instead.

Takashi

--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
struct pci_dev *hdac_pci, *i915_pci;
struct hdac_bus *bus = data;

- if (!dev_is_pci(dev))
+ if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev))
return 0;

hdac_pci = to_pci_dev(bus->dev);
i915_pci = to_pci_dev(dev);

- if (!strcmp(dev->driver->name, "i915") &&
- subcomponent == I915_COMPONENT_AUDIO &&
- connectivity_check(i915_pci, hdac_pci))
- return 1;
-
- return 0;
+ return connectivity_check(i915_pci, hdac_pci);
}

/* check whether intel graphics is present */

2022-03-31 19:39:19

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, Mar 31, 2022 at 03:52:14PM +0300, Heikki Krogerus wrote:
> Hi Greg,
>
> On Thu, Mar 31, 2022 at 01:39:51PM +0200, Greg KH wrote:
> > On Thu, Mar 31, 2022 at 12:25:43PM +0300, Heikki Krogerus wrote:
> > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > > > >
> > > > > > Can NULL dev->driver be really seen? I thought the components are
> > > > > > added by the drivers, hence they ought to have the driver field set.
> > > > > > But there can be corner cases I overlooked.
> > > > > >
> > > > > >
> > > > > > thanks,
> > > > > >
> > > > > > Takashi
> > > > >
> > > > > Hi Takashi,
> > > > >
> > > > > When I try using component_add in a different driver (usb4 in my
> > > > > case), I think dev->driver here is NULL because the i915 drivers do
> > > > > not have their component master fully bound when this new component is
> > > > > registered. When I test it, it seems to be causing a crash.
> > > >
> > > > Hm, from where component_add*() is called? Basically dev->driver must
> > > > be already set before the corresponding driver gets bound at
> > > > __driver_probe_deviec(). So, if the device is added to component from
> > > > the corresponding driver's probe, dev->driver must be non-NULL.
> > >
> > > The code that declares a device as component does not have to be the
> > > driver of that device.
> > >
> > > In our case the components are USB ports, and they are devices that
> > > are actually never bind to any drivers: drivers/usb/core/port.c
> >
> > Why is a USB device being passed to this code that assumes it is looking
> > for a PCI device with a specific driver name? As I mentioned on the
> > mei patch, triggering off of a name is really a bad idea, as is assuming
> > the device type without any assurance it is such a device (there's a
> > reason we didn't provide device type identification in the driver core,
> > don't abuse that please...)
>
> I totally agree. This driver is making a whole bunch of assumptions
> when it should not make any assumptions. And yes, one of those
> assumptions is that the driver of the device has a specific name, and
> that is totally crazy. So why is it making those assumptions? I have
> no idea, but is does, and they are now causing the first problem -
> NULL pointer dereference.
>
> This patch (and that other) is only proposing a simple way to solve
> that NULL pointer dereference issue by adding some sanity checks. If
> that's no OK, and the whole driver should be refactored instead, then
> that is perfectly OK by me, but that has to be done by somebody who
> understands what exactly is the driver and the device it's controlling
> doing (and for).

This all needs to be refactored to not do this at all.

thanks,

greg k-h

2022-03-31 19:39:40

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, 31 Mar 2022 11:45:47 +0200,
Takashi Iwai wrote:
>
> On Thu, 31 Mar 2022 11:34:38 +0200,
> Heikki Krogerus wrote:
> >
> > On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
> > > On Thu, 31 Mar 2022 11:25:43 +0200,
> > > Heikki Krogerus wrote:
> > > >
> > > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > > > > >
> > > > > > > Can NULL dev->driver be really seen? I thought the components are
> > > > > > > added by the drivers, hence they ought to have the driver field set.
> > > > > > > But there can be corner cases I overlooked.
> > > > > > >
> > > > > > >
> > > > > > > thanks,
> > > > > > >
> > > > > > > Takashi
> > > > > >
> > > > > > Hi Takashi,
> > > > > >
> > > > > > When I try using component_add in a different driver (usb4 in my
> > > > > > case), I think dev->driver here is NULL because the i915 drivers do
> > > > > > not have their component master fully bound when this new component is
> > > > > > registered. When I test it, it seems to be causing a crash.
> > > > >
> > > > > Hm, from where component_add*() is called? Basically dev->driver must
> > > > > be already set before the corresponding driver gets bound at
> > > > > __driver_probe_deviec(). So, if the device is added to component from
> > > > > the corresponding driver's probe, dev->driver must be non-NULL.
> > > >
> > > > The code that declares a device as component does not have to be the
> > > > driver of that device.
> > > >
> > > > In our case the components are USB ports, and they are devices that
> > > > are actually never bind to any drivers: drivers/usb/core/port.c
> > >
> > > OK, that's what I wanted to know. It'd be helpful if it's more
> > > clearly mentioned in the commit log.
> >
> > Agree.
> >
> > > BTW, the same problem must be seen in MEI drivers, too.
> >
> > Wasn't there a patch for those too? I lost track...
>
> I don't know, I just checked the latest Linus tree.
>
> And, looking at the HD-audio code, I still wonder how NULL dev->driver
> can reach there. Is there any PCI device that is added to component
> without binding to a driver? We have dev_is_pci() check at the
> beginning, so non-PCI devices should bail out there...

Further reading on, I'm really confused. How data=NULL can be passed
to this function? The data argument is the value passed from the
component_match_add_typed() call in HD-audio driver, hence it must be
always the snd_hdac_bus object.

And, I guess the i915 string check can be omitted completely, at
least, for HD-audio driver. It already have a check of the parent of
the device and that should be enough.

Takashi

2022-03-31 20:04:01

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, 31 Mar 2022 10:38:34 +0200,
Won Chung wrote:
>
> On Thu, Mar 31, 2022 at 12:27 AM Takashi Iwai <[email protected]> wrote:
> >
> > On Wed, 30 Mar 2022 23:19:13 +0200,
> > Won Chung wrote:
> > >
> > > Component match callback function needs to check if expected data is
> > > passed to it. Without this check, it can cause a NULL pointer
> > > dereference when another driver registers a component before i915
> > > drivers have their component master fully bind.
> > >
> > > Fixes: 7b882fe3e3e8b ("ALSA: hda - handle multiple i915 device instances")
> > > Signed-off-by: Heikki Krogerus <[email protected]>
> > > Signed-off-by: Mika Westerberg <[email protected]>
> > > Signed-off-by: Won Chung <[email protected]>
> > > ---
> > > - Add "Fixes" tag
> > > - Send to [email protected]
> >
> > You rather need to add "Cc: [email protected]" line to the patch
> > itself (around sign-off block), not actually Cc'ing the mail.
> > I edited manually, but please do it so at the next time.
> >
> > Although I applied the patch as-is now, I wonder...
> >
> >
> > > - if (!strcmp(dev->driver->name, "i915") &&
> > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> >
> > Can NULL dev->driver be really seen? I thought the components are
> > added by the drivers, hence they ought to have the driver field set.
> > But there can be corner cases I overlooked.
> >
> >
> > thanks,
> >
> > Takashi
>
> Hi Takashi,
>
> When I try using component_add in a different driver (usb4 in my
> case), I think dev->driver here is NULL because the i915 drivers do
> not have their component master fully bound when this new component is
> registered. When I test it, it seems to be causing a crash.

Hm, from where component_add*() is called? Basically dev->driver must
be already set before the corresponding driver gets bound at
__driver_probe_deviec(). So, if the device is added to component from
the corresponding driver's probe, dev->driver must be non-NULL.

> Would it be okay for me to resend a new patch with the flags
> corrected? I have mistakenly added Heikki and Mika as "Signed-off-by"
> instead of "Suggested-by". I am sorry for that.

Ah that's bad. I'll reset the branch, then. Please resubmit
properly.

thanks,

Takashi

2022-03-31 21:42:47

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, Mar 31, 2022 at 08:33:03AM -0700, Benson Leung wrote:
> Hi Takashi,
>
> On Thu, Mar 31, 2022 at 04:19:15PM +0200, Takashi Iwai wrote:
> > On Thu, 31 Mar 2022 15:29:10 +0200,
> > Takashi Iwai wrote:
> > >
> > > On Thu, 31 Mar 2022 11:45:47 +0200,
> > > Takashi Iwai wrote:
> > > >
> > > > On Thu, 31 Mar 2022 11:34:38 +0200,
> > > > Heikki Krogerus wrote:
> > > > >
> > > > > On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
> > > > > > On Thu, 31 Mar 2022 11:25:43 +0200,
> > > > > > Heikki Krogerus wrote:
> > > > > > >
> > > > > > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > > > > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > > > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > > > > > > > >
> > > > > > > > > > Can NULL dev->driver be really seen? I thought the components are
> > > > > > > > > > added by the drivers, hence they ought to have the driver field set.
> > > > > > > > > > But there can be corner cases I overlooked.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > thanks,
> > > > > > > > > >
> > > > > > > > > > Takashi
> > > > > > > > >
> > > > > > > > > Hi Takashi,
> > > > > > > > >
> > > > > > > > > When I try using component_add in a different driver (usb4 in my
> > > > > > > > > case), I think dev->driver here is NULL because the i915 drivers do
> > > > > > > > > not have their component master fully bound when this new component is
> > > > > > > > > registered. When I test it, it seems to be causing a crash.
> > > > > > > >
> > > > > > > > Hm, from where component_add*() is called? Basically dev->driver must
> > > > > > > > be already set before the corresponding driver gets bound at
> > > > > > > > __driver_probe_deviec(). So, if the device is added to component from
> > > > > > > > the corresponding driver's probe, dev->driver must be non-NULL.
> > > > > > >
> > > > > > > The code that declares a device as component does not have to be the
> > > > > > > driver of that device.
> > > > > > >
> > > > > > > In our case the components are USB ports, and they are devices that
> > > > > > > are actually never bind to any drivers: drivers/usb/core/port.c
> > > > > >
> > > > > > OK, that's what I wanted to know. It'd be helpful if it's more
> > > > > > clearly mentioned in the commit log.
> > > > >
> > > > > Agree.
> > > > >
> > > > > > BTW, the same problem must be seen in MEI drivers, too.
> > > > >
> > > > > Wasn't there a patch for those too? I lost track...
> > > >
> > > > I don't know, I just checked the latest Linus tree.
> > > >
> > > > And, looking at the HD-audio code, I still wonder how NULL dev->driver
> > > > can reach there. Is there any PCI device that is added to component
> > > > without binding to a driver? We have dev_is_pci() check at the
> > > > beginning, so non-PCI devices should bail out there...
> > >
> > > Further reading on, I'm really confused. How data=NULL can be passed
> > > to this function? The data argument is the value passed from the
> > > component_match_add_typed() call in HD-audio driver, hence it must be
> > > always the snd_hdac_bus object.
> > >
> > > And, I guess the i915 string check can be omitted completely, at
> > > least, for HD-audio driver. It already have a check of the parent of
> > > the device and that should be enough.
> >
> > That said, something like below (supposing data NULL check being
> > superfluous), instead.
> >
> >
> > Takashi
> >
> > --- a/sound/hda/hdac_i915.c
> > +++ b/sound/hda/hdac_i915.c
> > @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
> > struct pci_dev *hdac_pci, *i915_pci;
> > struct hdac_bus *bus = data;
> >
> > - if (!dev_is_pci(dev))
> > + if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev))
> > return 0;
> >
>
> If I recall this bug correctly, it's not the usb port perse that is falling
> through this !dev_is_pci(dev) check, it's actually the usb4-port in a new
> proposed patch by Heikki and Mika to extend the usb type-c component to
> encompass the usb4 specific pieces too. Is it possible usb4 ports are considered
> pci devices, and that's how we got into this situation?
>
> Also, a little more background information: This crash happens because in
> our kernel configs, we config'd the usb4 driver as =y (built in) instead of
> =m module, which meant that the usb4 port's driver was adding a component
> likely much earlier than hdac_i915.

So is this actually triggering on 5.17 right now? Or is it due to some
other not-applied changes you are testing at the moment?

confused,

greg k-h

2022-04-01 02:27:36

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, 31 Mar 2022 17:33:03 +0200,
Benson Leung wrote:
>
> Hi Takashi,
>
> On Thu, Mar 31, 2022 at 04:19:15PM +0200, Takashi Iwai wrote:
> > On Thu, 31 Mar 2022 15:29:10 +0200,
> > Takashi Iwai wrote:
> > >
> > > On Thu, 31 Mar 2022 11:45:47 +0200,
> > > Takashi Iwai wrote:
> > > >
> > > > On Thu, 31 Mar 2022 11:34:38 +0200,
> > > > Heikki Krogerus wrote:
> > > > >
> > > > > On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
> > > > > > On Thu, 31 Mar 2022 11:25:43 +0200,
> > > > > > Heikki Krogerus wrote:
> > > > > > >
> > > > > > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > > > > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > > > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > > > > > > > >
> > > > > > > > > > Can NULL dev->driver be really seen? I thought the components are
> > > > > > > > > > added by the drivers, hence they ought to have the driver field set.
> > > > > > > > > > But there can be corner cases I overlooked.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > thanks,
> > > > > > > > > >
> > > > > > > > > > Takashi
> > > > > > > > >
> > > > > > > > > Hi Takashi,
> > > > > > > > >
> > > > > > > > > When I try using component_add in a different driver (usb4 in my
> > > > > > > > > case), I think dev->driver here is NULL because the i915 drivers do
> > > > > > > > > not have their component master fully bound when this new component is
> > > > > > > > > registered. When I test it, it seems to be causing a crash.
> > > > > > > >
> > > > > > > > Hm, from where component_add*() is called? Basically dev->driver must
> > > > > > > > be already set before the corresponding driver gets bound at
> > > > > > > > __driver_probe_deviec(). So, if the device is added to component from
> > > > > > > > the corresponding driver's probe, dev->driver must be non-NULL.
> > > > > > >
> > > > > > > The code that declares a device as component does not have to be the
> > > > > > > driver of that device.
> > > > > > >
> > > > > > > In our case the components are USB ports, and they are devices that
> > > > > > > are actually never bind to any drivers: drivers/usb/core/port.c
> > > > > >
> > > > > > OK, that's what I wanted to know. It'd be helpful if it's more
> > > > > > clearly mentioned in the commit log.
> > > > >
> > > > > Agree.
> > > > >
> > > > > > BTW, the same problem must be seen in MEI drivers, too.
> > > > >
> > > > > Wasn't there a patch for those too? I lost track...
> > > >
> > > > I don't know, I just checked the latest Linus tree.
> > > >
> > > > And, looking at the HD-audio code, I still wonder how NULL dev->driver
> > > > can reach there. Is there any PCI device that is added to component
> > > > without binding to a driver? We have dev_is_pci() check at the
> > > > beginning, so non-PCI devices should bail out there...
> > >
> > > Further reading on, I'm really confused. How data=NULL can be passed
> > > to this function? The data argument is the value passed from the
> > > component_match_add_typed() call in HD-audio driver, hence it must be
> > > always the snd_hdac_bus object.
> > >
> > > And, I guess the i915 string check can be omitted completely, at
> > > least, for HD-audio driver. It already have a check of the parent of
> > > the device and that should be enough.
> >
> > That said, something like below (supposing data NULL check being
> > superfluous), instead.
> >
> >
> > Takashi
> >
> > --- a/sound/hda/hdac_i915.c
> > +++ b/sound/hda/hdac_i915.c
> > @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
> > struct pci_dev *hdac_pci, *i915_pci;
> > struct hdac_bus *bus = data;
> >
> > - if (!dev_is_pci(dev))
> > + if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev))
> > return 0;
> >
>
> If I recall this bug correctly, it's not the usb port perse that is falling
> through this !dev_is_pci(dev) check, it's actually the usb4-port in a new
> proposed patch by Heikki and Mika to extend the usb type-c component to
> encompass the usb4 specific pieces too. Is it possible usb4 ports are considered
> pci devices, and that's how we got into this situation?

Yes, that explains for one of two changes in the original patch.
But why data==NULL check is needed is still unclear.

> Also, a little more background information: This crash happens because in
> our kernel configs, we config'd the usb4 driver as =y (built in) instead of
> =m module, which meant that the usb4 port's driver was adding a component
> likely much earlier than hdac_i915.

Thanks, it's what I supposed, too.

Takashi

2022-04-01 02:38:30

by Mika Westerberg

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

Hi,

On Thu, Mar 31, 2022 at 08:33:03AM -0700, Benson Leung wrote:
> > --- a/sound/hda/hdac_i915.c
> > +++ b/sound/hda/hdac_i915.c
> > @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
> > struct pci_dev *hdac_pci, *i915_pci;
> > struct hdac_bus *bus = data;
> >
> > - if (!dev_is_pci(dev))
> > + if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev))
> > return 0;
> >
>
> If I recall this bug correctly, it's not the usb port perse that is falling
> through this !dev_is_pci(dev) check, it's actually the usb4-port in a new
> proposed patch by Heikki and Mika to extend the usb type-c component to
> encompass the usb4 specific pieces too. Is it possible usb4 ports are considered
> pci devices, and that's how we got into this situation?

For usb4_port the dev_ic_pci(dev) returns false:

#define dev_is_pci(d) ((d)->bus == &pci_bus_type)

We don't attach them to PCI bus (well they are not PCI devices).

2022-04-01 03:40:29

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > >
> > > Can NULL dev->driver be really seen? I thought the components are
> > > added by the drivers, hence they ought to have the driver field set.
> > > But there can be corner cases I overlooked.
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> >
> > Hi Takashi,
> >
> > When I try using component_add in a different driver (usb4 in my
> > case), I think dev->driver here is NULL because the i915 drivers do
> > not have their component master fully bound when this new component is
> > registered. When I test it, it seems to be causing a crash.
>
> Hm, from where component_add*() is called? Basically dev->driver must
> be already set before the corresponding driver gets bound at
> __driver_probe_deviec(). So, if the device is added to component from
> the corresponding driver's probe, dev->driver must be non-NULL.

The code that declares a device as component does not have to be the
driver of that device.

In our case the components are USB ports, and they are devices that
are actually never bind to any drivers: drivers/usb/core/port.c

thanks,

--
heikki

2022-04-01 06:38:01

by Won Chung

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

On Thu, Mar 31, 2022 at 9:38 AM Greg KH <[email protected]> wrote:
>
> On Thu, Mar 31, 2022 at 08:33:03AM -0700, Benson Leung wrote:
> > Hi Takashi,
> >
> > On Thu, Mar 31, 2022 at 04:19:15PM +0200, Takashi Iwai wrote:
> > > On Thu, 31 Mar 2022 15:29:10 +0200,
> > > Takashi Iwai wrote:
> > > >
> > > > On Thu, 31 Mar 2022 11:45:47 +0200,
> > > > Takashi Iwai wrote:
> > > > >
> > > > > On Thu, 31 Mar 2022 11:34:38 +0200,
> > > > > Heikki Krogerus wrote:
> > > > > >
> > > > > > On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
> > > > > > > On Thu, 31 Mar 2022 11:25:43 +0200,
> > > > > > > Heikki Krogerus wrote:
> > > > > > > >
> > > > > > > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > > > > > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > > > > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > > > > > > > > >
> > > > > > > > > > > Can NULL dev->driver be really seen? I thought the components are
> > > > > > > > > > > added by the drivers, hence they ought to have the driver field set.
> > > > > > > > > > > But there can be corner cases I overlooked.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > thanks,
> > > > > > > > > > >
> > > > > > > > > > > Takashi
> > > > > > > > > >
> > > > > > > > > > Hi Takashi,
> > > > > > > > > >
> > > > > > > > > > When I try using component_add in a different driver (usb4 in my
> > > > > > > > > > case), I think dev->driver here is NULL because the i915 drivers do
> > > > > > > > > > not have their component master fully bound when this new component is
> > > > > > > > > > registered. When I test it, it seems to be causing a crash.
> > > > > > > > >
> > > > > > > > > Hm, from where component_add*() is called? Basically dev->driver must
> > > > > > > > > be already set before the corresponding driver gets bound at
> > > > > > > > > __driver_probe_deviec(). So, if the device is added to component from
> > > > > > > > > the corresponding driver's probe, dev->driver must be non-NULL.
> > > > > > > >
> > > > > > > > The code that declares a device as component does not have to be the
> > > > > > > > driver of that device.
> > > > > > > >
> > > > > > > > In our case the components are USB ports, and they are devices that
> > > > > > > > are actually never bind to any drivers: drivers/usb/core/port.c
> > > > > > >
> > > > > > > OK, that's what I wanted to know. It'd be helpful if it's more
> > > > > > > clearly mentioned in the commit log.
> > > > > >
> > > > > > Agree.
> > > > > >
> > > > > > > BTW, the same problem must be seen in MEI drivers, too.
> > > > > >
> > > > > > Wasn't there a patch for those too? I lost track...
> > > > >
> > > > > I don't know, I just checked the latest Linus tree.
> > > > >
> > > > > And, looking at the HD-audio code, I still wonder how NULL dev->driver
> > > > > can reach there. Is there any PCI device that is added to component
> > > > > without binding to a driver? We have dev_is_pci() check at the
> > > > > beginning, so non-PCI devices should bail out there...
> > > >
> > > > Further reading on, I'm really confused. How data=NULL can be passed
> > > > to this function? The data argument is the value passed from the
> > > > component_match_add_typed() call in HD-audio driver, hence it must be
> > > > always the snd_hdac_bus object.
> > > >
> > > > And, I guess the i915 string check can be omitted completely, at
> > > > least, for HD-audio driver. It already have a check of the parent of
> > > > the device and that should be enough.
> > >
> > > That said, something like below (supposing data NULL check being
> > > superfluous), instead.
> > >
> > >
> > > Takashi
> > >
> > > --- a/sound/hda/hdac_i915.c
> > > +++ b/sound/hda/hdac_i915.c
> > > @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
> > > struct pci_dev *hdac_pci, *i915_pci;
> > > struct hdac_bus *bus = data;
> > >
> > > - if (!dev_is_pci(dev))
> > > + if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev))
> > > return 0;
> > >
> >
> > If I recall this bug correctly, it's not the usb port perse that is falling
> > through this !dev_is_pci(dev) check, it's actually the usb4-port in a new
> > proposed patch by Heikki and Mika to extend the usb type-c component to
> > encompass the usb4 specific pieces too. Is it possible usb4 ports are considered
> > pci devices, and that's how we got into this situation?
> >
> > Also, a little more background information: This crash happens because in
> > our kernel configs, we config'd the usb4 driver as =y (built in) instead of
> > =m module, which meant that the usb4 port's driver was adding a component
> > likely much earlier than hdac_i915.
>
> So is this actually triggering on 5.17 right now? Or is it due to some
> other not-applied changes you are testing at the moment?
>
> confused,
>
> greg k-h

Hi Greg,

I believe it is not causing an issue in 5.17 at the moment. It is
triggered when we try to apply new changes and test it locally.
(registering a component for usb4_port)

Thanks,
Won

2022-04-01 15:05:58

by Benson Leung

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

Hi Takashi,

On Thu, Mar 31, 2022 at 04:19:15PM +0200, Takashi Iwai wrote:
> On Thu, 31 Mar 2022 15:29:10 +0200,
> Takashi Iwai wrote:
> >
> > On Thu, 31 Mar 2022 11:45:47 +0200,
> > Takashi Iwai wrote:
> > >
> > > On Thu, 31 Mar 2022 11:34:38 +0200,
> > > Heikki Krogerus wrote:
> > > >
> > > > On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
> > > > > On Thu, 31 Mar 2022 11:25:43 +0200,
> > > > > Heikki Krogerus wrote:
> > > > > >
> > > > > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > > > > > > > > - if (!strcmp(dev->driver->name, "i915") &&
> > > > > > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
> > > > > > > > >
> > > > > > > > > Can NULL dev->driver be really seen? I thought the components are
> > > > > > > > > added by the drivers, hence they ought to have the driver field set.
> > > > > > > > > But there can be corner cases I overlooked.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > thanks,
> > > > > > > > >
> > > > > > > > > Takashi
> > > > > > > >
> > > > > > > > Hi Takashi,
> > > > > > > >
> > > > > > > > When I try using component_add in a different driver (usb4 in my
> > > > > > > > case), I think dev->driver here is NULL because the i915 drivers do
> > > > > > > > not have their component master fully bound when this new component is
> > > > > > > > registered. When I test it, it seems to be causing a crash.
> > > > > > >
> > > > > > > Hm, from where component_add*() is called? Basically dev->driver must
> > > > > > > be already set before the corresponding driver gets bound at
> > > > > > > __driver_probe_deviec(). So, if the device is added to component from
> > > > > > > the corresponding driver's probe, dev->driver must be non-NULL.
> > > > > >
> > > > > > The code that declares a device as component does not have to be the
> > > > > > driver of that device.
> > > > > >
> > > > > > In our case the components are USB ports, and they are devices that
> > > > > > are actually never bind to any drivers: drivers/usb/core/port.c
> > > > >
> > > > > OK, that's what I wanted to know. It'd be helpful if it's more
> > > > > clearly mentioned in the commit log.
> > > >
> > > > Agree.
> > > >
> > > > > BTW, the same problem must be seen in MEI drivers, too.
> > > >
> > > > Wasn't there a patch for those too? I lost track...
> > >
> > > I don't know, I just checked the latest Linus tree.
> > >
> > > And, looking at the HD-audio code, I still wonder how NULL dev->driver
> > > can reach there. Is there any PCI device that is added to component
> > > without binding to a driver? We have dev_is_pci() check at the
> > > beginning, so non-PCI devices should bail out there...
> >
> > Further reading on, I'm really confused. How data=NULL can be passed
> > to this function? The data argument is the value passed from the
> > component_match_add_typed() call in HD-audio driver, hence it must be
> > always the snd_hdac_bus object.
> >
> > And, I guess the i915 string check can be omitted completely, at
> > least, for HD-audio driver. It already have a check of the parent of
> > the device and that should be enough.
>
> That said, something like below (supposing data NULL check being
> superfluous), instead.
>
>
> Takashi
>
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
> struct pci_dev *hdac_pci, *i915_pci;
> struct hdac_bus *bus = data;
>
> - if (!dev_is_pci(dev))
> + if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev))
> return 0;
>

If I recall this bug correctly, it's not the usb port perse that is falling
through this !dev_is_pci(dev) check, it's actually the usb4-port in a new
proposed patch by Heikki and Mika to extend the usb type-c component to
encompass the usb4 specific pieces too. Is it possible usb4 ports are considered
pci devices, and that's how we got into this situation?

Also, a little more background information: This crash happens because in
our kernel configs, we config'd the usb4 driver as =y (built in) instead of
=m module, which meant that the usb4 port's driver was adding a component
likely much earlier than hdac_i915.

Thanks,
Benson

> hdac_pci = to_pci_dev(bus->dev);
> i915_pci = to_pci_dev(dev);
>
> - if (!strcmp(dev->driver->name, "i915") &&
> - subcomponent == I915_COMPONENT_AUDIO &&
> - connectivity_check(i915_pci, hdac_pci))
> - return 1;
> -
> - return 0;
> + return connectivity_check(i915_pci, hdac_pci);
> }
>
> /* check whether intel graphics is present */
>

--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
[email protected]
Chromium OS Project
[email protected]

Attachments:

(No filename) (4.77 kB)
signature.asc (235.00 B)
Download all attachments

2022-04-01 21:45:49

by Benson Leung

[permalink] [raw]

Subject: Re: [PATCH v2] sound/hda: Add NULL check to component match callback function

Hi Greg,

On Thu, Mar 31, 2022 at 07:18:00PM +0200, Greg KH wrote:
> On Thu, Mar 31, 2022 at 09:58:43AM -0700, Won Chung wrote:
> > > So is this actually triggering on 5.17 right now? Or is it due to some
> > > other not-applied changes you are testing at the moment?
> > >
> > > confused,
> > >
> > > greg k-h
> >
> > Hi Greg,
> >
> > I believe it is not causing an issue in 5.17 at the moment. It is
> > triggered when we try to apply new changes and test it locally.
> > (registering a component for usb4_port)
>
> Then why would it ever be needed to be backported to a stable kernel?
>
> Please be more careful.
>
> greg k-h

Sorry about that. I gave Won bad advice to cc stable. You're right, it will
only be relevant when a future patch lands in usb4.

Thanks,
Benson

--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
[email protected]
Chromium OS Project
[email protected]

Attachments:

(No filename) (946.00 B)
signature.asc (235.00 B)
Download all attachments

2022-04-04 15:41:01