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 <[email protected]>
--- 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;
* Rakib Mullick <[email protected]>:
> 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 <[email protected]>
>
> --- 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
On 12/5/09, Alex Chiang <[email protected]> wrote:
> * Rakib Mullick <[email protected]>:
>
> 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
>
* Rakib Mullick <[email protected]>:
> On 12/5/09, Alex Chiang <[email protected]> wrote:
> > * Rakib Mullick <[email protected]>:
> >
> > 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
* Alex Chiang <[email protected]>:
> * Rakib Mullick <[email protected]>:
> > On 12/5/09, Alex Chiang <[email protected]> wrote:
> > > * Rakib Mullick <[email protected]>:
> > >
> > > 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
On 12/5/09, Alex Chiang <[email protected]> wrote:
> * Rakib Mullick <[email protected]>:
> > On 12/5/09, Alex Chiang <[email protected]> wrote:
> > > * Rakib Mullick <[email protected]>:
>
>
> 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
>
>
* Rakib Mullick <[email protected]>:
>
> 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
On 12/5/09, Alex Chiang <[email protected]> wrote:
> * Rakib Mullick <[email protected]>:
> >
>
> > 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
>