[PATCH] Xen bug-fixes for 3.2 (v1)

[PATCH] Xen bug-fixes for 3.2 (v1)

[Konrad Rzeszutek Wilk]

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(-)

[PATCH 1/9] xen/pciback: Do not dereference psdev during printk when it is NULL.

[Konrad Rzeszutek Wilk]

. 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

[PATCH 2/9] xen/pciback: Return proper error code from sscanf.

[Konrad Rzeszutek Wilk]

. 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

[PATCH 3/9] xen/pciback: Check if the device is found instead of blindly assuming so.

[Konrad Rzeszutek Wilk]

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

[PATCH 3/9] xen/pciback: Check if the device is found instead of blindly assuming so.

[Konrad Rzeszutek Wilk]

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

[PATCH 4/9] xen/events: BUG() when we can't allocate our event->irq array.

[Konrad Rzeszutek Wilk]

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

[PATCH 5/9] xen/events: Don't check the info for NULL as it is already done.

[Konrad Rzeszutek Wilk]

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

[PATCH 6/9] xen/irq: If we fail during msi_capability_init return proper error code.

[Konrad Rzeszutek Wilk]

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

[PATCH 7/9] xen/xenbus: Check before dereferencing it.

[Konrad Rzeszutek Wilk]

. 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

[PATCH 8/9] xen/enlighten: Fix compile warnings.

[Konrad Rzeszutek Wilk]

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

[PATCH 9/9] xen/p2m/debugfs: Fix potential pointer exception.

[Konrad Rzeszutek Wilk]

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

Re: [Xen-devel] [PATCH 1/9] xen/pciback: Do not dereference psdev during printk when it is NULL.

[Jan Beulich]

>>> 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';

Re: [Xen-devel] [PATCH 1/9] xen/pciback: Do not dereference psdev during printk when it is NULL.

[Jan Beulich]

>>> 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';

Re: [Xen-devel] [PATCH 2/9] xen/pciback: Return proper error code from sscanf.

[Jan Beulich]

>>> 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

Re: [Xen-devel] [PATCH 2/9] xen/pciback: Return proper error code from sscanf.

[Jan Beulich]

>>> 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

Re: [Xen-devel] [PATCH 3/9] xen/pciback: Check if the device is found instead of blindly assuming so.

[Jan Beulich]

>>> 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

Re: [PATCH 3/9] xen/pciback: Check if the device is found instead of blindly assuming so.

[Jan Beulich]

>>> 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

Re: [Xen-devel] [PATCH 4/9] xen/events: BUG() when we can't allocate our event->irq array.

[Ian Campbell]

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>

Re: [Xen-devel] [PATCH 7/9] xen/xenbus: Check before dereferencing it.

[Ian Campbell]

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;
>  

Re: [Xen-devel] [PATCH 8/9] xen/enlighten: Fix compile warnings.

[Ian Campbell]

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
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 =

Re: [PATCH 8/9] xen/enlighten: Fix compile warnings.

[Ian Campbell]

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
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 =

Re: [Xen-devel] [PATCH 9/9] xen/p2m/debugfs: Fix potential pointer exception.

[Ian Campbell]

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

Re: [Xen-devel] [PATCH 1/9] xen/pciback: Do not dereference psdev during printk when it is NULL.

[Konrad Rzeszutek Wilk]

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!

Re: [Xen-devel] [PATCH 2/9] xen/pciback: Return proper error code from sscanf.

[Konrad Rzeszutek Wilk]

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..

Re: [Xen-devel] [PATCH 4/9] xen/events: BUG() when we can't allocate our event->irq array.

[Konrad Rzeszutek Wilk]

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>
> 

Re: [Xen-devel] [PATCH 7/9] xen/xenbus: Check before dereferencing it.

[Konrad Rzeszutek Wilk]

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.

Re: [Xen-devel] [PATCH 8/9] xen/enlighten: Fix compile warnings.

[Konrad Rzeszutek Wilk]

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!

Re: [Xen-devel] [PATCH 9/9] xen/p2m/debugfs: Fix potential pointer exception.

[Konrad Rzeszutek Wilk]

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.

Re: [PATCH 9/9] xen/p2m/debugfs: Fix potential pointer exception.

[Konrad Rzeszutek Wilk]

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.