Re: [PATCH 1/2] xen/event: Add reference counting to event channel

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

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@tycho.nsa.gov>
>> ---
>>  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
>