| Message ID | 1366611236-1811-1-git-send-email-qiudayu@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
2013/4/22 14:13, Mike Qiu wrote: Resend this patch because it has been send to kernel mailling list before :) > In pnv_pci_read_config() or pnv_pci_write_config(), we never check if > the PCI controller is valid before converting that into platform > dependent one, this is very dangerous. > > To avoid this potential risks, the patch check PCI controller first > before use it. > > Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com> > --- > arch/powerpc/platforms/powernv/pci.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c > index b8b8e0b..e7b7f1a 100644 > --- a/arch/powerpc/platforms/powernv/pci.c > +++ b/arch/powerpc/platforms/powernv/pci.c > @@ -286,11 +286,11 @@ static int pnv_pci_read_config(struct pci_bus *bus, > int where, int size, u32 *val) > { > struct pci_controller *hose = pci_bus_to_host(bus); > - struct pnv_phb *phb = hose->private_data; > + struct pnv_phb *phb = hose ? hose->private_data : NULL; > u32 bdfn = (((uint64_t)bus->number) << 8) | devfn; > s64 rc; > > - if (hose == NULL) > + if (!phb) > return PCIBIOS_DEVICE_NOT_FOUND; > > switch (size) { > @@ -330,10 +330,10 @@ static int pnv_pci_write_config(struct pci_bus *bus, > int where, int size, u32 val) > { > struct pci_controller *hose = pci_bus_to_host(bus); > - struct pnv_phb *phb = hose->private_data; > + struct pnv_phb *phb = hose ? hose->private_data : NULL; > u32 bdfn = (((uint64_t)bus->number) << 8) | devfn; > > - if (hose == NULL) > + if (!phb) > return PCIBIOS_DEVICE_NOT_FOUND; > > cfg_dbg("pnv_pci_write_config bus: %x devfn: %x +%x/%x -> %08x\n", -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-04-22 at 02:13 -0400, Mike Qiu wrote: > In pnv_pci_read_config() or pnv_pci_write_config(), we never check if > the PCI controller is valid before converting that into platform > dependent one, this is very dangerous. > > To avoid this potential risks, the patch check PCI controller first > before use it. I don't think there's any remote possibility of that happening, is there ? If it does, maybe it warrants a WARN_ON... Ben. > Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com> > --- > arch/powerpc/platforms/powernv/pci.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c > index b8b8e0b..e7b7f1a 100644 > --- a/arch/powerpc/platforms/powernv/pci.c > +++ b/arch/powerpc/platforms/powernv/pci.c > @@ -286,11 +286,11 @@ static int pnv_pci_read_config(struct pci_bus *bus, > int where, int size, u32 *val) > { > struct pci_controller *hose = pci_bus_to_host(bus); > - struct pnv_phb *phb = hose->private_data; > + struct pnv_phb *phb = hose ? hose->private_data : NULL; > u32 bdfn = (((uint64_t)bus->number) << 8) | devfn; > s64 rc; > > - if (hose == NULL) > + if (!phb) > return PCIBIOS_DEVICE_NOT_FOUND; > > switch (size) { > @@ -330,10 +330,10 @@ static int pnv_pci_write_config(struct pci_bus *bus, > int where, int size, u32 val) > { > struct pci_controller *hose = pci_bus_to_host(bus); > - struct pnv_phb *phb = hose->private_data; > + struct pnv_phb *phb = hose ? hose->private_data : NULL; > u32 bdfn = (((uint64_t)bus->number) << 8) | devfn; > > - if (hose == NULL) > + if (!phb) > return PCIBIOS_DEVICE_NOT_FOUND; > > cfg_dbg("pnv_pci_write_config bus: %x devfn: %x +%x/%x -> %08x\n", -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
? 2013/4/22 14:36, Benjamin Herrenschmidt ??: > On Mon, 2013-04-22 at 02:13 -0400, Mike Qiu wrote: >> In pnv_pci_read_config() or pnv_pci_write_config(), we never check if >> the PCI controller is valid before converting that into platform >> dependent one, this is very dangerous. >> >> To avoid this potential risks, the patch check PCI controller first >> before use it. > I don't think there's any remote possibility of that happening, is > there ? Yes, I agree, I don't exactly mean that it maybe happen, but the code try to check the pci_controller pointer and the way it try is useless, because if this happens, the system will crash before check: try to access the "NULL" pointer. My patch just makes the code more stable and robust. Anyway, I think it's better to remove the check code as it is useless, as it will shows that this "NULL" pci_controller pointer may happen... Thanks Mike > If it does, maybe it warrants a WARN_ON... > > Ben. > >> Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com> >> --- >> arch/powerpc/platforms/powernv/pci.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c >> index b8b8e0b..e7b7f1a 100644 >> --- a/arch/powerpc/platforms/powernv/pci.c >> +++ b/arch/powerpc/platforms/powernv/pci.c >> @@ -286,11 +286,11 @@ static int pnv_pci_read_config(struct pci_bus *bus, >> int where, int size, u32 *val) >> { >> struct pci_controller *hose = pci_bus_to_host(bus); >> - struct pnv_phb *phb = hose->private_data; >> + struct pnv_phb *phb = hose ? hose->private_data : NULL; >> u32 bdfn = (((uint64_t)bus->number) << 8) | devfn; >> s64 rc; >> >> - if (hose == NULL) >> + if (!phb) >> return PCIBIOS_DEVICE_NOT_FOUND; >> >> switch (size) { >> @@ -330,10 +330,10 @@ static int pnv_pci_write_config(struct pci_bus *bus, >> int where, int size, u32 val) >> { >> struct pci_controller *hose = pci_bus_to_host(bus); >> - struct pnv_phb *phb = hose->private_data; >> + struct pnv_phb *phb = hose ? hose->private_data : NULL; >> u32 bdfn = (((uint64_t)bus->number) << 8) | devfn; >> >> - if (hose == NULL) >> + if (!phb) >> return PCIBIOS_DEVICE_NOT_FOUND; >> >> cfg_dbg("pnv_pci_write_config bus: %x devfn: %x +%x/%x -> %08x\n", > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-04-22 at 15:41 +0800, Mike Qiu wrote: > Anyway, I think it's better to remove the check code as it is useless, > as it > will shows that this "NULL" pci_controller pointer may happen... Ok. It *might* still be worth adding a BUG_ON then in pci_bus_to_host() itself ... no big deal either way. Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
? 2013/4/22 16:04, Benjamin Herrenschmidt ??: > On Mon, 2013-04-22 at 15:41 +0800, Mike Qiu wrote: >> Anyway, I think it's better to remove the check code as it is useless, >> as it >> will shows that this "NULL" pci_controller pointer may happen... > Ok. It *might* still be worth adding a BUG_ON then in pci_bus_to_host() > itself ... no big deal either way. OK, you mean the code will remain the same and my patch is worthless? Anyway, it will be OK for me. But I hope it can be accept for more stable reason. Thanks Mike > Cheers, > Ben. > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 22, 2013 at 3:44 AM, Mike Qiu <qiudayu@linux.vnet.ibm.com> wrote: > ? 2013/4/22 16:04, Benjamin Herrenschmidt ??: > >> On Mon, 2013-04-22 at 15:41 +0800, Mike Qiu wrote: >>> >>> Anyway, I think it's better to remove the check code as it is useless, >>> as it >>> will shows that this "NULL" pci_controller pointer may happen... >> >> Ok. It *might* still be worth adding a BUG_ON then in pci_bus_to_host() >> itself ... no big deal either way. > > OK, you mean the code will remain the same and my patch is worthless? > > Anyway, it will be OK for me. But I hope it can be accept for more stable > reason. This is powerpc code, so I'm kibitzing here, but it was cc'd to linux-pci :) There should be no way to get a struct pci_bus * where bus->sysdata (== "hose") is NULL. The sysdata pointer is always supplied to pci_create_root_bus() (or a similar interface that calls pci_create_root_bus()), so every root bus has a valid sysdata pointer. And every child bus inherits the sysdata pointer of its parent (in pci_alloc_child_bus()). Therefore, every pci_bus should have a valid sysdata pointer. So I think you should just remove even the existing "if (hose == NULL)" check. That way, if bus->sysdata actually *does* turn out to be NULL, we'll oops on the null pointer dereference, get a nice backtrace, and have a chance to fix the problem. Testing and returning an error means whatever bug or memory corruption caused the null pointer will most likely be ignored. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-04-22 at 11:35 -0600, Bjorn Helgaas wrote: > On Mon, Apr 22, 2013 at 3:44 AM, Mike Qiu <qiudayu@linux.vnet.ibm.com> wrote: > > ? 2013/4/22 16:04, Benjamin Herrenschmidt ??: > > > >> On Mon, 2013-04-22 at 15:41 +0800, Mike Qiu wrote: > >>> > >>> Anyway, I think it's better to remove the check code as it is useless, > >>> as it > >>> will shows that this "NULL" pci_controller pointer may happen... > >> > >> Ok. It *might* still be worth adding a BUG_ON then in pci_bus_to_host() > >> itself ... no big deal either way. > > > > OK, you mean the code will remain the same and my patch is worthless? > > > > Anyway, it will be OK for me. But I hope it can be accept for more stable > > reason. > > This is powerpc code, so I'm kibitzing here, but it was cc'd to linux-pci :) > > There should be no way to get a struct pci_bus * where bus->sysdata > (== "hose") is NULL. The sysdata pointer is always supplied to > pci_create_root_bus() (or a similar interface that calls > pci_create_root_bus()), so every root bus has a valid sysdata pointer. > And every child bus inherits the sysdata pointer of its parent (in > pci_alloc_child_bus()). Therefore, every pci_bus should have a valid > sysdata pointer. And the kernel has checked whether if sysdata is NULL after call pci_create_root_bus(). > > So I think you should just remove even the existing "if (hose == > NULL)" check. That way, if bus->sysdata actually *does* turn out to > be NULL, we'll oops on the null pointer dereference, get a nice > backtrace, and have a chance to fix the problem. Testing and > returning an error means whatever bug or memory corruption caused the > null pointer will most likely be ignored. > Yes, I agree with you, because it is useless, and meaningless. Also it never achieve its goals even if the "hose==NULL". So I suggest either use my patch or remove that check code. If the second case, I will write v2 patch to remove that check code. I don't know what's to go for next step, it depends on the maintainer :) Thanks Mike > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-04-22 at 21:46 -0400, Mike Qiu wrote: > > So I suggest either use my patch or remove that check code. > If the second case, I will write v2 patch to remove that check code. > I don't know what's to go for next step, it depends on the > maintainer :) Please do. Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c index b8b8e0b..e7b7f1a 100644 --- a/arch/powerpc/platforms/powernv/pci.c +++ b/arch/powerpc/platforms/powernv/pci.c @@ -286,11 +286,11 @@ static int pnv_pci_read_config(struct pci_bus *bus, int where, int size, u32 *val) { struct pci_controller *hose = pci_bus_to_host(bus); - struct pnv_phb *phb = hose->private_data; + struct pnv_phb *phb = hose ? hose->private_data : NULL; u32 bdfn = (((uint64_t)bus->number) << 8) | devfn; s64 rc; - if (hose == NULL) + if (!phb) return PCIBIOS_DEVICE_NOT_FOUND; switch (size) { @@ -330,10 +330,10 @@ static int pnv_pci_write_config(struct pci_bus *bus, int where, int size, u32 val) { struct pci_controller *hose = pci_bus_to_host(bus); - struct pnv_phb *phb = hose->private_data; + struct pnv_phb *phb = hose ? hose->private_data : NULL; u32 bdfn = (((uint64_t)bus->number) << 8) | devfn; - if (hose == NULL) + if (!phb) return PCIBIOS_DEVICE_NOT_FOUND; cfg_dbg("pnv_pci_write_config bus: %x devfn: %x +%x/%x -> %08x\n",
In pnv_pci_read_config() or pnv_pci_write_config(), we never check if the PCI controller is valid before converting that into platform dependent one, this is very dangerous. To avoid this potential risks, the patch check PCI controller first before use it. Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com> --- arch/powerpc/platforms/powernv/pci.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)