WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH 1/2] xen/event: Add reference counting to event c

To: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH 1/2] xen/event: Add reference counting to event channel
From: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
Date: Mon, 24 Oct 2011 18:20:29 -0400
Cc: Ian.Jackson@xxxxxxxxxxxxx, jeremy@xxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxx, keir@xxxxxxx
Delivery-date: Mon, 24 Oct 2011 15:21:15 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20111024202242.GD2441@xxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: National Security Agency
References: <1316207684-19860-1-git-send-email-dgdegra@xxxxxxxxxxxxx> <1318971851-12809-1-git-send-email-dgdegra@xxxxxxxxxxxxx> <1318971851-12809-2-git-send-email-dgdegra@xxxxxxxxxxxxx> <20111024202242.GD2441@xxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20110927 Thunderbird/7.0
On 10/24/2011 04:22 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Oct 18, 2011 at 05:04:10PM -0400, Daniel De Graaf wrote:
>> Event channels exposed to userspace by the evtchn module may be used by
>> other modules in an asynchronous manner, which requires that reference
>> counting be used to prevent the event channel from being closed before
>> the signals are delivered.
> 
> You should probably also remove the comment in "xen_bind_pirq_gsi_to_irq"
> which talks about refcount (as the comment would now apply to this code and
> might confuse people reading the code).

OK.

> There are two scenarios I am concerned about:
> 
>  1). Xen pciback allocates/setups an physical IRQ on behalf of a guest. Lets
>      concentrate on MSI as that is more interesting. The PV guests sends
>      XEN_PCI_OP_enable_msi, dom0 calls pci_enable_msi(), MSI libs end up 
> calling
>      xen_initdom_setup_msi_irqs, which calls xen_bind_pirq_msi_to_irq and 
>      irq->refcnt==2.
> 
>      Guest dies without calling XEN_PCI_OP_disable_msi, so we end up in
>      xen_pcibk_reset_device which calls pci_disable_msi().. which calls 
> xen_free_irq().
>      And all of that sets refcnt==1.. OK, and if we do call 
> xen_pcibk_reset_device()
>      again it is smart enough _not_ to call pci_disable_msi() twice.
> 
>      So I guess that case is actually OK, but if there was a driver that 
> decided to
>      call pci_disable_msi (or pci_disable_irq) we could hit the BUG_ON(). 
> Perhaps
>      that should be altered to WARN_ON.
 
Agreed, WARN_ON is better as nothing is likely to explode if the refcnt is off.

>  2). Grantdev holding the refcnt forever. That is probably the easiest as it 
> would
>      be a bug in the code.

Right; same as any other reference leak in external (kernel) code.
 
> Hmm, I  think I've talked myself out of actually finding any cases where this 
> would
> be problematic from a design perspective. The only issue I can see is 
> exposing bugs
> in the users of event channel API - which there might be. So definitly needs 
> some
> heavy duty testing. 
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>> ---
>>  drivers/xen/events.c |   34 ++++++++++++++++++++++++++++++++++
>>  include/xen/events.h |    6 ++++++
>>  2 files changed, 40 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
>> index 7523719..36d3390 100644
>> --- a/drivers/xen/events.c
>> +++ b/drivers/xen/events.c
>> @@ -88,6 +88,7 @@ enum xen_irq_type {
>>  struct irq_info
>>  {
>>      struct list_head list;
>> +    atomic_t refcount;
> 
> refcnt
> 

Ah yes, vowels are much too valuable to use more than one here...

>>      enum xen_irq_type type; /* type */
>>      unsigned irq;
>>      unsigned short evtchn;  /* event channel */
>> @@ -407,6 +408,7 @@ static void xen_irq_init(unsigned irq)
>>              panic("Unable to allocate metadata for IRQ%d\n", irq);
>>  
>>      info->type = IRQT_UNBOUND;
>> +    atomic_set(&info->refcount, 1);
>>  
>>      irq_set_handler_data(irq, info);
>>  
>> @@ -469,6 +471,8 @@ static void xen_free_irq(unsigned irq)
>>  
>>      irq_set_handler_data(irq, NULL);
>>  
>> +    BUG_ON(atomic_read(&info->refcount) > 1);
>> +
>>      kfree(info);
>>  
>>      /* Legacy IRQ descriptors are managed by the arch. */
>> @@ -912,6 +916,10 @@ static void unbind_from_irq(unsigned int irq)
>>  {
>>      struct evtchn_close close;
>>      int evtchn = evtchn_from_irq(irq);
>> +    struct irq_info *info = irq_get_handler_data(irq);
>> +
>> +    if (!atomic_dec_and_test(&info->refcount))
>> +            return;
>>  
>>      mutex_lock(&irq_mapping_update_lock);
>>  
>> @@ -1038,6 +1046,32 @@ void unbind_from_irqhandler(unsigned int irq, void 
>> *dev_id)
>>  }
>>  EXPORT_SYMBOL_GPL(unbind_from_irqhandler);
>>  
>> +int evtchn_get(unsigned int evtchn)
>> +{
>> +    int irq = evtchn_to_irq[evtchn];
>> +    struct irq_info *info;
>> +
>> +    if (irq == -1)
>> +            return -ENOENT;
>> +
>> +    info = irq_get_handler_data(irq);
>> +
>> +    if (!info)
>> +            return -ENOENT;
>> +
>> +    atomic_inc(&info->refcount);
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(evtchn_get);
>> +
>> +void evtchn_put(unsigned int evtchn)
> 
> The decleration for 'evtchn' is 'unsigned short' so that can be
> used instead of 'unsigned int'.
> 
>> +{
>> +    int irq = evtchn_to_irq[evtchn];
> 
> Not checking if the irq is valid? Or if the evtchn is valid?

All callers currently check this (by the _get function), but it's probably a
good idea to double-check with a WARN_ON just in case it's not valid.

>> +    unbind_from_irq(irq);
>> +}
>> +EXPORT_SYMBOL_GPL(evtchn_put);
>> +
>>  void xen_send_IPI_one(unsigned int cpu, enum ipi_vector vector)
>>  {
>>      int irq = per_cpu(ipi_to_irq, cpu)[vector];
>> diff --git a/include/xen/events.h b/include/xen/events.h
>> index d287997..a459cca 100644
>> --- a/include/xen/events.h
>> +++ b/include/xen/events.h
>> @@ -37,6 +37,12 @@ int bind_interdomain_evtchn_to_irqhandler(unsigned int 
>> remote_domain,
>>   */
>>  void unbind_from_irqhandler(unsigned int irq, void *dev_id);
>>  
>> +/*
>> + * Allow extra references to event channels exposed to userspace by evtchn
>> + */
>> +int evtchn_get(unsigned int evtchn);
>> +void evtchn_put(unsigned int evtchn);
>> +
>>  void xen_send_IPI_one(unsigned int cpu, enum ipi_vector vector);
>>  int resend_irq_on_evtchn(unsigned int irq);
>>  void rebind_evtchn_irq(int evtchn, int irq);
>> -- 
>> 1.7.6.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>