Re: arm: Minor bug report & Fix in gic_route_irq_to_guest

[Julien Grall <julien.grall@citrix.com>]

On 04/24/2013 03:24 AM, Sengul Thomas wrote:

> Hello,
> 
> I found that when calling gic_route_irq_to_guest in construct_dom0 function,
> it uses local variable "name" for passing devname argument.
> And, gic_route_irq_to_guest just copies the pointer of this devname
> and afterward,
> reading this devname gives data abort.
> 
> Here goes a simple fix: just copying the data, not the pointer

Thanks for this report. I prefer to remove all uses of local variable
"name", because I intend to remove this code soon.

I have pushed the commit in the arndale branch with another minor change.

Cheers,

Julien

> Signed-off-by: Thomas Sengul <thomas.sengul@gmail.com>
> ---
>  xen/arch/arm/gic.c |   13 ++++++++++++-
>  xen/arch/arm/irq.c |   14 +++++++++++++-
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 63caeb8..012aae9 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -468,7 +468,10 @@ void __init release_irq(unsigned int irq)
>      do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
> 
>      if (action && action->free_on_release)
> +    {
> +        xfree((void *)action->name);
>          xfree(action);
> +    }
>  }
> 
>  static int __setup_irq(struct irq_desc *desc, unsigned int irq,
> @@ -617,13 +620,20 @@ int gic_route_irq_to_guest(struct domain *d,
> unsigned int irq,
>      struct irq_desc *desc = irq_to_desc(irq);
>      unsigned long flags;
>      int retval;
> +    char *name;
> 
>      action = xmalloc(struct irqaction);
>      if (!action)
>          return -ENOMEM;
> 
>      action->dev_id = d;
> -    action->name = devname;
> +
> +#define MIN_ACTION_NAME_LEN 16
> +    name = xmalloc_array(char, MIN_ACTION_NAME_LEN);
> +    if (!name)
> +        return -ENOMEM;
> +    strlcpy(name, devname, strnlen(devname, MIN_ACTION_NAME_LEN));
> +    action->name = name;
> 
>      spin_lock_irqsave(&desc->lock, flags);
>      spin_lock(&gic.lock);
> @@ -635,6 +645,7 @@ int gic_route_irq_to_guest(struct domain *d,
> unsigned int irq,
> 
>      retval = __setup_irq(desc, irq, action);
>      if (retval) {
> +        xfree((void *)action->name);
>          xfree(action);
>          goto out;
>      }
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 8c96a0a..e6c24f9 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -99,6 +99,7 @@ int __init request_irq(unsigned int irq,
>  {
>      struct irqaction *action;
>      int retval;
> +    char *name;
> 
>      /*
>       * Sanity-check: shared interrupts must pass in a real dev-ID,
> @@ -116,13 +117,24 @@ int __init request_irq(unsigned int irq,
>          return -ENOMEM;
> 
>      action->handler = handler;
> -    action->name = devname;
> +
> +#define MIN_ACTION_NAME_LEN 16
> +    name = xmalloc_array(char, MIN_ACTION_NAME_LEN);
> +    if (!name)
> +        return -ENOMEM;
> +    strlcpy(name, devname, strnlen(devname, MIN_ACTION_NAME_LEN));
> +    action->name = name;
> +
>      action->dev_id = dev_id;
>      action->free_on_release = 1;
> 
>      retval = setup_irq(irq, action);
>      if (retval)
> +    {
> +        xfree((void *)action->name);
>          xfree(action);
> +    }
> +
> 
>      return retval;
>  }
> 
> 
> Sincerely,
> Thomas
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel