[PATCH] pci: Make pci_dev struct point to NULL.

[PATCH] pci: Make pci_dev struct point to NULL.

[Rakib Mullick]

 pci: Make pci_dev struct point to NULL.

In function enable_device of acpiphp_glue.c, structure pci_dev
doesn't point anything. Due to the check in line 975 we might
end up being uninitialized. So make it point to NULL.

---
Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>

--- linus/drivers/pci/hotplug/acpiphp_glue.c	2009-12-03 21:30:57.000000000 +0600
+++ rakib/drivers/pci/hotplug/acpiphp_glue.c	2009-12-03 23:53:44.000000000 +0600
@@ -964,7 +964,7 @@ static int acpiphp_bus_trim(acpi_handle
  */
 static int __ref enable_device(struct acpiphp_slot *slot)
 {
-	struct pci_dev *dev;
+	struct pci_dev *dev = NULL;
 	struct pci_bus *bus = slot->bridge->pci_bus;
 	struct list_head *l;
 	struct acpiphp_func *func;

Re: [PATCH] pci: Make pci_dev struct point to NULL.

[Alex Chiang]

* Rakib Mullick <rakib.mullick@gmail.com>:
>  pci: Make pci_dev struct point to NULL.
> 
> In function enable_device of acpiphp_glue.c, structure pci_dev
> doesn't point anything. Due to the check in line 975 we might
> end up being uninitialized. So make it point to NULL.
> 
> ---
> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
> 
> --- linus/drivers/pci/hotplug/acpiphp_glue.c	2009-12-03 21:30:57.000000000 +0600
> +++ rakib/drivers/pci/hotplug/acpiphp_glue.c	2009-12-03 23:53:44.000000000 +0600
> @@ -964,7 +964,7 @@ static int acpiphp_bus_trim(acpi_handle
>   */
>  static int __ref enable_device(struct acpiphp_slot *slot)
>  {
> -	struct pci_dev *dev;
> +	struct pci_dev *dev = NULL;
>  	struct pci_bus *bus = slot->bridge->pci_bus;
>  	struct list_head *l;
>  	struct acpiphp_func *func;

This is from Linus's latest tree:

 965 static int __ref enable_device(struct acpiphp_slot *slot)
 966 {
 967         struct pci_dev *dev;
 968         struct pci_bus *bus = slot->bridge->pci_bus;
 969         struct list_head *l;
 970         struct acpiphp_func *func;
 971         int retval = 0;
 972         int num, max, pass;
 973         acpi_status status;
 974 
 975         if (slot->flags & SLOT_ENABLED)
 976                 goto err_exit;
 977 
 978         /* sanity check: dev should be NULL when hot-plugged in */
 979         dev = pci_get_slot(bus, PCI_DEVFN(slot->device, 0));
 980         if (dev) {

I assume your line 975 is my line 980.

pci_get_slot() returns NULL if it doesn't find the devfn, so as
far as I can tell, there's no need to initialize dev to NULL.

Were you fixing a real bug with this patch? Did you actually get
the "pci_dev structure already exists.\n" error message?

Thanks,
/ac

Re: [PATCH] pci: Make pci_dev struct point to NULL.

[Rakib Mullick]

On 12/5/09, Alex Chiang <achiang@hp.com> wrote:
> * Rakib Mullick <rakib.mullick@gmail.com>:
>
> This is from Linus's latest tree:
>
>   974
>   975         if (slot->flags & SLOT_ENABLED)
>   976                 goto err_exit;

I'm talking about this line. From here we can hit 'goto err_exit' without
using pci_get_slot.

>   977
>   978         /* sanity check: dev should be NULL when hot-plugged in */
>   979         dev = pci_get_slot(bus, PCI_DEVFN(slot->device, 0));
>   980         if (dev) {
>
>  I assume your line 975 is my line 980.

Nope, my line 975 is also yours.
>
>  pci_get_slot() returns NULL if it doesn't find the devfn, so as
>  far as I can tell, there's no need to initialize dev to NULL.
>
>  Were you fixing a real bug with this patch? Did you actually get
>  the "pci_dev structure already exists.\n" error message?
>
No - i'm trying to make sure that we're not referencing into a trash.

>  Thanks,
>
> /ac
>

Re: [PATCH] pci: Make pci_dev struct point to NULL.

[Alex Chiang]

* Rakib Mullick <rakib.mullick@gmail.com>:
> On 12/5/09, Alex Chiang <achiang@hp.com> wrote:
> > * Rakib Mullick <rakib.mullick@gmail.com>:
> >
> > This is from Linus's latest tree:
> >
> >   974
> >   975         if (slot->flags & SLOT_ENABLED)
> >   976                 goto err_exit;
> 
> I'm talking about this line. From here we can hit 'goto err_exit' without
> using pci_get_slot.

Right, so what's the problem? If the slot is not enabled, we goto
err_exit and return, never touching dev.

> >   977
> >   978         /* sanity check: dev should be NULL when hot-plugged in */
> >   979         dev = pci_get_slot(bus, PCI_DEVFN(slot->device, 0));
> >   980         if (dev) {
> >
> >  I assume your line 975 is my line 980.
> 
> Nope, my line 975 is also yours.
> >
> >  pci_get_slot() returns NULL if it doesn't find the devfn, so as
> >  far as I can tell, there's no need to initialize dev to NULL.
> >
> >  Were you fixing a real bug with this patch? Did you actually get
> >  the "pci_dev structure already exists.\n" error message?
> >
> No - i'm trying to make sure that we're not referencing into a trash.

I must be slow, because I don't understand how we might reference
trash.

Care to explain it to me?

Thanks,
/ac

Re: [PATCH] pci: Make pci_dev struct point to NULL.

[Alex Chiang]

* Alex Chiang <achiang@hp.com>:
> * Rakib Mullick <rakib.mullick@gmail.com>:
> > On 12/5/09, Alex Chiang <achiang@hp.com> wrote:
> > > * Rakib Mullick <rakib.mullick@gmail.com>:
> > >
> > > This is from Linus's latest tree:
> > >
> > >   974
> > >   975         if (slot->flags & SLOT_ENABLED)
> > >   976                 goto err_exit;
> > 
> > I'm talking about this line. From here we can hit 'goto err_exit' without
> > using pci_get_slot.
> 
> Right, so what's the problem? If the slot is not enabled, we goto
> err_exit and return, never touching dev.
 
Whoops, of course I meant if the slot is already enabled, then we
return early.

/ac

Re: [PATCH] pci: Make pci_dev struct point to NULL.

[Rakib Mullick]

On 12/5/09, Alex Chiang <achiang@hp.com> wrote:
> * Rakib Mullick <rakib.mullick@gmail.com>:
>  > On 12/5/09, Alex Chiang <achiang@hp.com> wrote:
>  > > * Rakib Mullick <rakib.mullick@gmail.com>:
>
>
> Right, so what's the problem? If the slot is not enabled, we goto
>  err_exit and return, never touching dev.
>
>
> I must be slow, because I don't understand how we might reference
>  trash.
>
Since *pdev might be uninitialized. But if we are sure that it is not
uninitialized
then it is okay.

And yes - althrough we weren't warned by the compiler.

Thanks,

>
>  Thanks,
>
> /ac
>
>

Re: [PATCH] pci: Make pci_dev struct point to NULL.

[Alex Chiang]

* Rakib Mullick <rakib.mullick@gmail.com>:
> 
> Since *pdev might be uninitialized. But if we are sure that it
> is not uninitialized then it is okay.
> 
> And yes - althrough we weren't warned by the compiler.

Let's start over.

This is the function:

 965 static int __ref enable_device(struct acpiphp_slot *slot)
 966 {
 967         struct pci_dev *dev;

Your patch makes this change:
             struct pci_dev *dev = NULL;

 968         struct pci_bus *bus = slot->bridge->pci_bus;
 969         struct list_head *l;
 970         struct acpiphp_func *func;
 971         int retval = 0;
 972         int num, max, pass;
 973         acpi_status status;
 974 
 975         if (slot->flags & SLOT_ENABLED)
 976                 goto err_exit;

Here, if the slot is already enabled, we goto the err_exit label
(below). We haven't touched 'dev' yet.

Otherwise, we call pci_get_slot().

 977 
 978         /* sanity check: dev should be NULL when hot-plugged in */
 979         dev = pci_get_slot(bus, PCI_DEVFN(slot->device, 0));
 980         if (dev) {
 981                 /* This case shouldn't happen */
 982                 err("pci_dev structure already exists.\n");
 983                 pci_dev_put(dev);
 984                 retval = -1;
 985                 goto err_exit;
 986         }

If pci_get_slot() finds the devfn, it returns the pointer to the
pdev, puts it into 'dev' and we return early.

If it cannot find the devfn, then we put NULL into dev and
continue with the rest of the function.

1044  err_exit:
1045         return retval;
1046 }


At no point that I can tell do we ever access an uninitialized
'dev'.

Please explain to me one more time what you think you are fixing.

Thanks,
/ac

Re: [PATCH] pci: Make pci_dev struct point to NULL.

[Rakib Mullick]

On 12/5/09, Alex Chiang <achiang@hp.com> wrote:
> * Rakib Mullick <rakib.mullick@gmail.com>:
>  >
>
> > Since *pdev might be uninitialized. But if we are sure that it
>  > is not uninitialized then it is okay.
>  >
>  > And yes - althrough we weren't warned by the compiler.
>
>  At no point that I can tell do we ever access an uninitialized
>  'dev'.
>
>  Please explain to me one more time what you think you are fixing.
>
 Ahh......... I miss the point that we haven't __access__ the uninitialized
pointer. I was messing up with it __remains__ uninitialized ( it was a
clear stupidity from me :-(       ).

Thanks,  Alex for your help.

Rakib,

>  Thanks,
>
> /ac
>