Konrad Rzeszutek Wilk
2011-Sep-29 19:52 UTC
[Xen-devel] [PATCH] Xen bug-fixes for 3.2 (v1)
Most of them were discovered by running Dan''s static analyzer (git://repo.or.cz/smatch.git). Some of them look/sound familiar which probably means at some point somebody emailed me it and I forgot it - if that is the case, please point me to it so I can use that instead of these. Please review. Konrad Rzeszutek Wilk (9): xen/pciback: Do not dereference psdev during printk when it is NULL. xen/pciback: Return proper error code from sscanf. xen/pciback: Check if the device is found instead of blindly assuming so. xen/events: BUG() when we can''t allocate our event->irq array. xen/events: Don''t check the info for NULL as it is already done. xen/irq: If we fail during msi_capability_init return proper error code. xen/xenbus: Check before dereferencing it. xen/enlighten: Fix compile warnings. xen/p2m/debugfs: Fix potential pointer exception. arch/x86/pci/xen.c | 10 +++++++--- arch/x86/xen/enlighten.c | 2 +- arch/x86/xen/p2m.c | 4 ++-- drivers/xen/events.c | 11 +++++++---- drivers/xen/xen-pciback/pci_stub.c | 12 ++++++++---- drivers/xen/xenbus/xenbus_probe_backend.c | 3 ++- 6 files changed, 27 insertions(+), 15 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Sep-29 19:52 UTC
[Xen-devel] [PATCH 1/9] xen/pciback: Do not dereference psdev during printk when it is NULL.
. instead use printk(.. facility. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- drivers/xen/xen-pciback/pci_stub.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c index aec214a..32d6891 100644 --- a/drivers/xen/xen-pciback/pci_stub.c +++ b/drivers/xen/xen-pciback/pci_stub.c @@ -514,9 +514,11 @@ static void kill_domain_by_device(struct pcistub_device *psdev) int err; char nodename[PCI_NODENAME_MAX]; - if (!psdev) - dev_err(&psdev->dev->dev, - "device is NULL when do AER recovery/kill_domain\n"); + if (!psdev) { + printk(KERN_ERR DRV_NAME + ":device is NULL when do AER recovery/kill_domain\n"); + return; + } snprintf(nodename, PCI_NODENAME_MAX, "/local/domain/0/backend/pci/%d/0", psdev->pdev->xdev->otherend_id); nodename[strlen(nodename)] = ''\0''; -- 1.7.4.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Sep-29 19:52 UTC
[Xen-devel] [PATCH 2/9] xen/pciback: Return proper error code from sscanf.
. instead of just hardcoding it to be -EINVAL. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- drivers/xen/xen-pciback/pci_stub.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c index 32d6891..d985b65 100644 --- a/drivers/xen/xen-pciback/pci_stub.c +++ b/drivers/xen/xen-pciback/pci_stub.c @@ -868,7 +868,7 @@ static inline int str_to_slot(const char *buf, int *domain, int *bus, if (err == 4) return 0; else if (err < 0) - return -EINVAL; + return err; /* try again without domain */ *domain = 0; -- 1.7.4.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Sep-29 19:52 UTC
[Xen-devel] [PATCH 3/9] xen/pciback: Check if the device is found instead of blindly assuming so.
Just in case it is not found, don''t try to dereference it. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- drivers/xen/xen-pciback/pci_stub.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c index d985b65..55086e9 100644 --- a/drivers/xen/xen-pciback/pci_stub.c +++ b/drivers/xen/xen-pciback/pci_stub.c @@ -222,6 +222,8 @@ void pcistub_put_pci_dev(struct pci_dev *dev) } spin_unlock_irqrestore(&pcistub_devices_lock, flags); + if (!found_psdev) + return; /*hold this lock for avoiding breaking link between * pcistub and xen_pcibk when AER is in processing -- 1.7.4.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Sep-29 19:52 UTC
[Xen-devel] [PATCH 4/9] xen/events: BUG() when we can''t allocate our event->irq array.
In case we can''t allocate we are doomed. We should BUG_ON instead of trying to dereference it later on. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- drivers/xen/events.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/xen/events.c b/drivers/xen/events.c index 7523719..5db04bf 100644 --- a/drivers/xen/events.c +++ b/drivers/xen/events.c @@ -1670,6 +1670,8 @@ void __init xen_init_IRQ(void) evtchn_to_irq = kcalloc(NR_EVENT_CHANNELS, sizeof(*evtchn_to_irq), GFP_KERNEL); + if (!evtchn_to_irq) + BUG(); for (i = 0; i < NR_EVENT_CHANNELS; i++) evtchn_to_irq[i] = -1; -- 1.7.4.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Sep-29 19:52 UTC
[Xen-devel] [PATCH 5/9] xen/events: Don''t check the info for NULL as it is already done.
The list operation checks whether the ''info'' structure that is retrieved from the list is NULL (otherwise it would not been able to retrieve it). This check is not neccessary. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- drivers/xen/events.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/xen/events.c b/drivers/xen/events.c index 5db04bf..71d2363 100644 --- a/drivers/xen/events.c +++ b/drivers/xen/events.c @@ -779,7 +779,7 @@ int xen_irq_from_pirq(unsigned pirq) mutex_lock(&irq_mapping_update_lock); list_for_each_entry(info, &xen_irq_list_head, list) { - if (info == NULL || info->type != IRQT_PIRQ) + if (info->type != IRQT_PIRQ) continue; irq = info->irq; if (info->u.pirq.pirq == pirq) -- 1.7.4.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Sep-29 19:52 UTC
[Xen-devel] [PATCH 6/9] xen/irq: If we fail during msi_capability_init return proper error code.
There are three different modes: PV, HVM, and initial domain 0. In all the cases we would return -1 for failure instead of a proper error code. Fix this by propagating the error code from the generic IRQ code. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- arch/x86/pci/xen.c | 10 +++++++--- drivers/xen/events.c | 7 ++++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c index 1017c7b..11a9301 100644 --- a/arch/x86/pci/xen.c +++ b/arch/x86/pci/xen.c @@ -175,8 +175,10 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) "pcifront-msi-x" : "pcifront-msi", DOMID_SELF); - if (irq < 0) + if (irq < 0) { + ret = irq; goto free; + } i++; } kfree(v); @@ -221,8 +223,10 @@ static int xen_hvm_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) if (msg.data != XEN_PIRQ_MSI_DATA || xen_irq_from_pirq(pirq) < 0) { pirq = xen_allocate_pirq_msi(dev, msidesc); - if (pirq < 0) + if (pirq < 0) { + irq = -ENODEV; goto error; + } xen_msi_compose_msg(dev, pirq, &msg); __write_msi_msg(msidesc, &msg); dev_dbg(&dev->dev, "xen: msi bound to pirq=%d\n", pirq); @@ -244,7 +248,7 @@ static int xen_hvm_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) error: dev_err(&dev->dev, "Xen PCI frontend has not registered MSI/MSI-X support!\n"); - return -ENODEV; + return irq; } #ifdef CONFIG_XEN_DOM0 diff --git a/drivers/xen/events.c b/drivers/xen/events.c index 71d2363..93c69ee 100644 --- a/drivers/xen/events.c +++ b/drivers/xen/events.c @@ -432,7 +432,8 @@ static int __must_check xen_allocate_irq_dynamic(void) irq = irq_alloc_desc_from(first, -1); - xen_irq_init(irq); + if (irq >= 0) + xen_irq_init(irq); return irq; } @@ -713,7 +714,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc, mutex_lock(&irq_mapping_update_lock); irq = xen_allocate_irq_dynamic(); - if (irq == -1) + if (irq < 0) goto out; irq_set_chip_and_handler_name(irq, &xen_pirq_chip, handle_edge_irq, @@ -729,7 +730,7 @@ out: error_irq: mutex_unlock(&irq_mapping_update_lock); xen_free_irq(irq); - return -1; + return ret; } #endif -- 1.7.4.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Sep-29 19:52 UTC
[Xen-devel] [PATCH 7/9] xen/xenbus: Check before dereferencing it.
. we do the check whether ''xdev'' is NULL - but after we have dereferenced it. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- drivers/xen/xenbus/xenbus_probe_backend.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/xen/xenbus/xenbus_probe_backend.c b/drivers/xen/xenbus/xenbus_probe_backend.c index 60adf91..331589a 100644 --- a/drivers/xen/xenbus/xenbus_probe_backend.c +++ b/drivers/xen/xenbus/xenbus_probe_backend.c @@ -103,10 +103,11 @@ static int xenbus_uevent_backend(struct device *dev, return -ENODEV; xdev = to_xenbus_device(dev); - bus = container_of(xdev->dev.bus, struct xen_bus_type, bus); if (xdev == NULL) return -ENODEV; + bus = container_of(xdev->dev.bus, struct xen_bus_type, bus); + if (add_uevent_var(env, "MODALIAS=xen-backend:%s", xdev->devicetype)) return -ENOMEM; -- 1.7.4.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Sep-29 19:52 UTC
[Xen-devel] [PATCH 8/9] xen/enlighten: Fix compile warnings.
linux/arch/x86/xen/enlighten.c: In function ‘xen_start_kernel’: linux/arch/x86/xen/enlighten.c:226: warning: ‘cx’ may be used uninitialized in this function linux/arch/x86/xen/enlighten.c:240: note: ‘cx’ was declared here Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- arch/x86/xen/enlighten.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c index 2d69617..9473861 100644 --- a/arch/x86/xen/enlighten.c +++ b/arch/x86/xen/enlighten.c @@ -237,7 +237,7 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx, static void __init xen_init_cpuid_mask(void) { - unsigned int ax, bx, cx, dx; + unsigned int ax, bx, uninitialized_var(cx), dx; unsigned int xsave_mask; cpuid_leaf1_edx_mask -- 1.7.4.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Sep-29 19:52 UTC
[Xen-devel] [PATCH 9/9] xen/p2m/debugfs: Fix potential pointer exception.
We could be referencing the last + 1 element of level_name[] array which would cause a pointer exception. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- arch/x86/xen/p2m.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c index 58efeb9..bc4cf0a 100644 --- a/arch/x86/xen/p2m.c +++ b/arch/x86/xen/p2m.c @@ -786,9 +786,9 @@ EXPORT_SYMBOL_GPL(m2p_find_override_pfn); int p2m_dump_show(struct seq_file *m, void *v) { static const char * const level_name[] = { "top", "middle", - "entry", "abnormal" }; + "entry", "abnormal", NULL}; static const char * const type_name[] = { "identity", "missing", - "pfn", "abnormal"}; + "pfn", "abnormal", NULL}; #define TYPE_IDENTITY 0 #define TYPE_MISSING 1 #define TYPE_PFN 2 -- 1.7.4.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Sep-30 07:31 UTC
Re: [Xen-devel] [PATCH 1/9] xen/pciback: Do not dereference psdev during printk when it is NULL.
>>> On 29.09.11 at 21:52, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > . instead use printk(.. facility. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > drivers/xen/xen-pciback/pci_stub.c | 8 +++++--- > 1 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/xen/xen-pciback/pci_stub.c > b/drivers/xen/xen-pciback/pci_stub.c > index aec214a..32d6891 100644 > --- a/drivers/xen/xen-pciback/pci_stub.c > +++ b/drivers/xen/xen-pciback/pci_stub.c > @@ -514,9 +514,11 @@ static void kill_domain_by_device(struct pcistub_device > *psdev) > int err; > char nodename[PCI_NODENAME_MAX]; > > - if (!psdev) > - dev_err(&psdev->dev->dev, > - "device is NULL when do AER recovery/kill_domain\n"); > + if (!psdev) { > + printk(KERN_ERR DRV_NAME > + ":device is NULL when do AER recovery/kill_domain\n"); > + return; > + }This is bogus - all callers of this function already make sure psdev is non-NULL, so imo the check should be removed or replaced with a BUG_ON(). Jan> snprintf(nodename, PCI_NODENAME_MAX, "/local/domain/0/backend/pci/%d/0", > psdev->pdev->xdev->otherend_id); > nodename[strlen(nodename)] = ''\0'';_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Sep-30 07:45 UTC
Re: [Xen-devel] [PATCH 2/9] xen/pciback: Return proper error code from sscanf.
>>> On 29.09.11 at 21:52, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > . instead of just hardcoding it to be -EINVAL. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > drivers/xen/xen-pciback/pci_stub.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/xen/xen-pciback/pci_stub.c > b/drivers/xen/xen-pciback/pci_stub.c > index 32d6891..d985b65 100644 > --- a/drivers/xen/xen-pciback/pci_stub.c > +++ b/drivers/xen/xen-pciback/pci_stub.c > @@ -868,7 +868,7 @@ static inline int str_to_slot(const char *buf, int > *domain, int *bus, > if (err == 4) > return 0; > else if (err < 0) > - return -EINVAL; > + return err; > > /* try again without domain */ > *domain = 0;This should then also be done for the final return from the function: return err < 0 ? err : -EINVAL; But: Where did you read that {v,}sscanf() would return -E... values in hypothetical error cases? The C standard says it would return EOF when reaching the end of the input string before doing the first conversion; lib/vsprintf.c doesn''t do so, and also doesn''t say it might return -E... codes. Bottom line is that I think the code is more correct the way it is without this change. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Sep-30 07:50 UTC
Re: [Xen-devel] [PATCH 3/9] xen/pciback: Check if the device is found instead of blindly assuming so.
>>> On 29.09.11 at 21:52, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > Just in case it is not found, don''t try to dereference it. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > drivers/xen/xen-pciback/pci_stub.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/xen/xen-pciback/pci_stub.c > b/drivers/xen/xen-pciback/pci_stub.c > index d985b65..55086e9 100644 > --- a/drivers/xen/xen-pciback/pci_stub.c > +++ b/drivers/xen/xen-pciback/pci_stub.c > @@ -222,6 +222,8 @@ void pcistub_put_pci_dev(struct pci_dev *dev) > } > > spin_unlock_irqrestore(&pcistub_devices_lock, flags); > + if (!found_psdev) > + return;If this happens, it''s a bug (backend controller found the device, but the generic backend code didn''t), so I would say minimally a WARN_ON() should be issued here. Jan> > /*hold this lock for avoiding breaking link between > * pcistub and xen_pcibk when AER is in processing_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Sep-30 07:55 UTC
Re: [Xen-devel] [PATCH 4/9] xen/events: BUG() when we can''t allocate our event->irq array.
On Thu, 2011-09-29 at 20:52 +0100, Konrad Rzeszutek Wilk wrote:> In case we can''t allocate we are doomed. We should BUG_ON > instead of trying to dereference it later on. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > drivers/xen/events.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/xen/events.c b/drivers/xen/events.c > index 7523719..5db04bf 100644 > --- a/drivers/xen/events.c > +++ b/drivers/xen/events.c > @@ -1670,6 +1670,8 @@ void __init xen_init_IRQ(void) > > evtchn_to_irq = kcalloc(NR_EVENT_CHANNELS, sizeof(*evtchn_to_irq), > GFP_KERNEL); > + if (!evtchn_to_irq) > + BUG();AKA BUG_ON(!evtchn_to_irq); but Acked-by: Ian Campbell <ian.campbell@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Sep-30 08:01 UTC
Re: [Xen-devel] [PATCH 7/9] xen/xenbus: Check before dereferencing it.
On Thu, 2011-09-29 at 20:52 +0100, Konrad Rzeszutek Wilk wrote:> . we do the check whether ''xdev'' is NULL - but after we have > dereferenced it. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > drivers/xen/xenbus/xenbus_probe_backend.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/drivers/xen/xenbus/xenbus_probe_backend.c b/drivers/xen/xenbus/xenbus_probe_backend.c > index 60adf91..331589a 100644 > --- a/drivers/xen/xenbus/xenbus_probe_backend.c > +++ b/drivers/xen/xenbus/xenbus_probe_backend.c > @@ -103,10 +103,11 @@ static int xenbus_uevent_backend(struct device *dev, > return -ENODEV; > > xdev = to_xenbus_device(dev);to_xenbus_device is just container_of. The only way it would return NULL is if dev == offsetof(struct xenbus_device, dev), which isn''t terribly likely. A more likely error would be dev == NULL and we just checked above.> - bus = container_of(xdev->dev.bus, struct xen_bus_type, bus); > if (xdev == NULL) > return -ENODEV; > > + bus = container_of(xdev->dev.bus, struct xen_bus_type, bus); > + > if (add_uevent_var(env, "MODALIAS=xen-backend:%s", xdev->devicetype)) > return -ENOMEM; >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Sep-30 08:10 UTC
Re: [Xen-devel] [PATCH 8/9] xen/enlighten: Fix compile warnings.
On Thu, 2011-09-29 at 20:52 +0100, Konrad Rzeszutek Wilk wrote:> linux/arch/x86/xen/enlighten.c: In function ‘xen_start_kernel’: > linux/arch/x86/xen/enlighten.c:226: warning: ‘cx’ may be used uninitialized in this function > linux/arch/x86/xen/enlighten.c:240: note: ‘cx’ was declared hereBefore 61f4237d5b005767a76f4f3694e68e6f78f392d9 we used to initialise cx to zero before calling xen_cpuid. 947ccf9c3c30307b774af3666ee74fcd9f47f646 didn''t put it back for some reason. Regardless I''m not sure how cx can be unused while {a,b,d}x apparently are not. All four are passed to xen_cpuid(&ax, &bx, &cx, &dx) and even if gcc were being clever and looking into xen_cpuid all four are in the output constraints of the real cpuid asm call. Oh, I see, ax and cx are also in the input side of the asm and ax is initialised but cx is not and that is the use not the one later in xen_init_cpuid_mask. I think that even if cpuid leaf ax=1 happens not to use the subleaf index in cx we''d be better to initialise cx=0 than use uninitialized_var here. Ian.> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > arch/x86/xen/enlighten.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c > index 2d69617..9473861 100644 > --- a/arch/x86/xen/enlighten.c > +++ b/arch/x86/xen/enlighten.c > @@ -237,7 +237,7 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx, > > static void __init xen_init_cpuid_mask(void) > { > - unsigned int ax, bx, cx, dx; > + unsigned int ax, bx, uninitialized_var(cx), dx; > unsigned int xsave_mask; > > cpuid_leaf1_edx_mask_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Sep-30 08:18 UTC
Re: [Xen-devel] [PATCH 9/9] xen/p2m/debugfs: Fix potential pointer exception.
On Thu, 2011-09-29 at 20:52 +0100, Konrad Rzeszutek Wilk wrote:> We could be referencing the last + 1 element of level_name[] > array which would cause a pointer exception.If we end up accessing it does that not mean something, i.e. should it not be a real string here and not NULL? Otherwise isn''t it a bug in the lookup code that we end up looking there? I think this lookup correspond to the initialisation of lvl=4 and falling through the subsequent list of checks without matching one. In which case I think level_name[4] should be "unknown" or even "error". I don''t think you can hit type_name[4] in the same way, type and prev_type are always one of the TYPE_* defines, which have values 0..3 inclusive. You could make this more obvious and defend against future changes breaking this with: ... type_name[] = { [TYPE_IDENTITY] = "identity", [TYPE_MISSING] = "missing" ... }; Ian.> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > arch/x86/xen/p2m.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c > index 58efeb9..bc4cf0a 100644 > --- a/arch/x86/xen/p2m.c > +++ b/arch/x86/xen/p2m.c > @@ -786,9 +786,9 @@ EXPORT_SYMBOL_GPL(m2p_find_override_pfn); > int p2m_dump_show(struct seq_file *m, void *v) > { > static const char * const level_name[] = { "top", "middle", > - "entry", "abnormal" }; > + "entry", "abnormal", NULL}; > static const char * const type_name[] = { "identity", "missing", > - "pfn", "abnormal"}; > + "pfn", "abnormal", NULL}; > #define TYPE_IDENTITY 0 > #define TYPE_MISSING 1 > #define TYPE_PFN 2_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Oct-03 16:14 UTC
Re: [Xen-devel] [PATCH 1/9] xen/pciback: Do not dereference psdev during printk when it is NULL.
On Fri, Sep 30, 2011 at 08:31:49AM +0100, Jan Beulich wrote:> >>> On 29.09.11 at 21:52, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > . instead use printk(.. facility. > > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > > drivers/xen/xen-pciback/pci_stub.c | 8 +++++--- > > 1 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/xen/xen-pciback/pci_stub.c > > b/drivers/xen/xen-pciback/pci_stub.c > > index aec214a..32d6891 100644 > > --- a/drivers/xen/xen-pciback/pci_stub.c > > +++ b/drivers/xen/xen-pciback/pci_stub.c > > @@ -514,9 +514,11 @@ static void kill_domain_by_device(struct pcistub_device > > *psdev) > > int err; > > char nodename[PCI_NODENAME_MAX]; > > > > - if (!psdev) > > - dev_err(&psdev->dev->dev, > > - "device is NULL when do AER recovery/kill_domain\n"); > > + if (!psdev) { > > + printk(KERN_ERR DRV_NAME > > + ":device is NULL when do AER recovery/kill_domain\n"); > > + return; > > + } > > This is bogus - all callers of this function already make sure psdev is > non-NULL, so imo the check should be removed or replaced with a > BUG_ON().Done! _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Oct-03 16:18 UTC
Re: [Xen-devel] [PATCH 2/9] xen/pciback: Return proper error code from sscanf.
On Fri, Sep 30, 2011 at 08:45:26AM +0100, Jan Beulich wrote:> >>> On 29.09.11 at 21:52, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > . instead of just hardcoding it to be -EINVAL. > > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > > drivers/xen/xen-pciback/pci_stub.c | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/xen/xen-pciback/pci_stub.c > > b/drivers/xen/xen-pciback/pci_stub.c > > index 32d6891..d985b65 100644 > > --- a/drivers/xen/xen-pciback/pci_stub.c > > +++ b/drivers/xen/xen-pciback/pci_stub.c > > @@ -868,7 +868,7 @@ static inline int str_to_slot(const char *buf, int > > *domain, int *bus, > > if (err == 4) > > return 0; > > else if (err < 0) > > - return -EINVAL; > > + return err; > > > > /* try again without domain */ > > *domain = 0; > > This should then also be done for the final return from the function: > > return err < 0 ? err : -EINVAL; > > But: Where did you read that {v,}sscanf() would return -E... values in > hypothetical error cases? The C standard says it would return EOF > when reaching the end of the input string before doing the first > conversion; lib/vsprintf.c doesn''t do so, and also doesn''t say it might > return -E... codes. Bottom line is that I think the code is more correct > the way it is without this change.will drop the patch.. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Oct-03 16:21 UTC
Re: [Xen-devel] [PATCH 4/9] xen/events: BUG() when we can''t allocate our event->irq array.
On Fri, Sep 30, 2011 at 08:55:53AM +0100, Ian Campbell wrote:> On Thu, 2011-09-29 at 20:52 +0100, Konrad Rzeszutek Wilk wrote: > > In case we can''t allocate we are doomed. We should BUG_ON > > instead of trying to dereference it later on. > > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > > drivers/xen/events.c | 2 ++ > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/xen/events.c b/drivers/xen/events.c > > index 7523719..5db04bf 100644 > > --- a/drivers/xen/events.c > > +++ b/drivers/xen/events.c > > @@ -1670,6 +1670,8 @@ void __init xen_init_IRQ(void) > > > > evtchn_to_irq = kcalloc(NR_EVENT_CHANNELS, sizeof(*evtchn_to_irq), > > GFP_KERNEL); > > + if (!evtchn_to_irq) > > + BUG(); > > AKA > BUG_ON(!evtchn_to_irq);Duh, I even say it in the description. Changed it to be BUG_ON.> > but > > Acked-by: Ian Campbell <ian.campbell@citrix.com> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Oct-03 16:26 UTC
Re: [Xen-devel] [PATCH 7/9] xen/xenbus: Check before dereferencing it.
On Fri, Sep 30, 2011 at 09:01:21AM +0100, Ian Campbell wrote:> On Thu, 2011-09-29 at 20:52 +0100, Konrad Rzeszutek Wilk wrote: > > . we do the check whether ''xdev'' is NULL - but after we have > > dereferenced it. > > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > > drivers/xen/xenbus/xenbus_probe_backend.c | 3 ++- > > 1 files changed, 2 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/xen/xenbus/xenbus_probe_backend.c b/drivers/xen/xenbus/xenbus_probe_backend.c > > index 60adf91..331589a 100644 > > --- a/drivers/xen/xenbus/xenbus_probe_backend.c > > +++ b/drivers/xen/xenbus/xenbus_probe_backend.c > > @@ -103,10 +103,11 @@ static int xenbus_uevent_backend(struct device *dev, > > return -ENODEV; > > > > xdev = to_xenbus_device(dev); > > to_xenbus_device is just container_of. The only way it would return NULL > is if dev == offsetof(struct xenbus_device, dev), which isn''t terribly > likely. A more likely error would be dev == NULL and we just checked > above. > > > - bus = container_of(xdev->dev.bus, struct xen_bus_type, bus); > > if (xdev == NULL) > > return -ENODEV;Hm, in which case this ''xdev == NULL'' is pointless. Will remove it. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Oct-03 16:30 UTC
Re: [Xen-devel] [PATCH 8/9] xen/enlighten: Fix compile warnings.
On Fri, Sep 30, 2011 at 09:10:51AM +0100, Ian Campbell wrote:> On Thu, 2011-09-29 at 20:52 +0100, Konrad Rzeszutek Wilk wrote: > > linux/arch/x86/xen/enlighten.c: In function ‘xen_start_kernel’: > > linux/arch/x86/xen/enlighten.c:226: warning: ‘cx’ may be used uninitialized in this function > > linux/arch/x86/xen/enlighten.c:240: note: ‘cx’ was declared here > > Before 61f4237d5b005767a76f4f3694e68e6f78f392d9 we used to initialise cx > to zero before calling xen_cpuid. > > 947ccf9c3c30307b774af3666ee74fcd9f47f646 didn''t put it back for some > reason. > > Regardless I''m not sure how cx can be unused while {a,b,d}x apparently > are not. All four are passed to xen_cpuid(&ax, &bx, &cx, &dx) and even > if gcc were being clever and looking into xen_cpuid all four are in the > output constraints of the real cpuid asm call. > > Oh, I see, ax and cx are also in the input side of the asm and ax is > initialised but cx is not and that is the use not the one later in > xen_init_cpuid_mask. > > I think that even if cpuid leaf ax=1 happens not to use the subleaf > index in cx we''d be better to initialise cx=0 than use uninitialized_var<nods> I somehow read the code as ''cx'' being set in xen_cpuid, but your analysis correct. Done! _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Oct-03 16:37 UTC
Re: [Xen-devel] [PATCH 9/9] xen/p2m/debugfs: Fix potential pointer exception.
On Fri, Sep 30, 2011 at 09:18:17AM +0100, Ian Campbell wrote:> On Thu, 2011-09-29 at 20:52 +0100, Konrad Rzeszutek Wilk wrote: > > We could be referencing the last + 1 element of level_name[] > > array which would cause a pointer exception. > > If we end up accessing it does that not mean something, i.e. should it > not be a real string here and not NULL? Otherwise isn''t it a bug in the > lookup code that we end up looking there?Yup. I altereted it per your recommendation to be "error".> > I think this lookup correspond to the initialisation of lvl=4 and > falling through the subsequent list of checks without matching one. In > which case I think level_name[4] should be "unknown" or even "error".Picked "error"> > I don''t think you can hit type_name[4] in the same way, type and > prev_type are always one of the TYPE_* defines, which have values 0..3 > inclusive. You could make this more obvious and defend against future > changes breaking this with: > ... type_name[] = { > [TYPE_IDENTITY] = "identity", > [TYPE_MISSING] = "missing" > ... > };Oooh, pretty. OK, queued another patch with that. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel