[PATCH 0/2] Add reference counting to grant notify ioctls

[PATCH 0/2] Add reference counting to grant notify ioctls

[Daniel De Graaf]

The current notify ioctls assume that an event channel will not be
closed prior to the page being unmapped. If the mappings are associated
with an open file descriptor and the application crashes, the
notification behavior depends on the close ordering of the file
descriptors. To avoid this, event channels now have a reference count
that is used by the grant notify ioctls to postpone the close operation
until the notification is fired.

[PATCH 1/2] xen/event: Add reference counting to event channel
[PATCH 2/2] xen/gnt{dev,alloc}: reserve event channels for notify

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

[Daniel De Graaf]

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.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/events.c |   38 ++++++++++++++++++++++++++++++++++++++
 include/xen/events.h |    6 ++++++
 2 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index da70f5c..c9343b9 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -89,6 +89,7 @@ struct irq_info
 {
 	struct list_head list;
 	enum xen_irq_type type;	/* type */
+	unsigned short refcount;
 	unsigned irq;
 	unsigned short evtchn;	/* event channel */
 	unsigned short cpu;	/* cpu bound */
@@ -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;
+	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(info->refcount > 1);
+
 	kfree(info);
 
 	/* Legacy IRQ descriptors are managed by the arch. */
@@ -912,9 +916,14 @@ 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);
 
 	spin_lock(&irq_mapping_update_lock);
 
+	info->refcount--;
+	if (info->refcount > 0)
+		goto out_unlock;
+
 	if (VALID_EVTCHN(evtchn)) {
 		close.port = evtchn;
 		if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
@@ -943,6 +952,7 @@ static void unbind_from_irq(unsigned int irq)
 
 	xen_free_irq(irq);
 
+ out_unlock:
 	spin_unlock(&irq_mapping_update_lock);
 }
 
@@ -1038,6 +1048,34 @@ void unbind_from_irqhandler(unsigned int irq, void *dev_id)
 }
 EXPORT_SYMBOL_GPL(unbind_from_irqhandler);
 
+int get_evtchn_reservation(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;
+
+	spin_lock(&irq_mapping_update_lock);
+	info->refcount++;
+	spin_unlock(&irq_mapping_update_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(get_evtchn_reservation);
+
+void put_evtchn_reservation(unsigned int evtchn)
+{
+	int irq = evtchn_to_irq[evtchn];
+	unbind_from_irq(irq);
+}
+EXPORT_SYMBOL_GPL(put_evtchn_reservation);
+
 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..23bd5fd 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 get_evtchn_reservation(unsigned int evtchn);
+void put_evtchn_reservation(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

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

[Jeremy Fitzhardinge]

On 09/16/2011 02:14 PM, 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.

Could you use the refcounting at the irq level?  I was quite pleased to
have removed the event channel refcounting (and the use of naked event
channels).

Oh, is it that userspace allocates an event channel with /dev/evtchn,
then passes that event channel to the gntalloc/gntdev drivers so they
can use it to pass events between the two.  That's a bit unfortunate; it
might have been better to expose those event channels as file
descriptors so you could use fd refcounting to manage the lifetimes.

What's the downside of sending the event after the event channel has closed?

That said:

>
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  drivers/xen/events.c |   38 ++++++++++++++++++++++++++++++++++++++
>  include/xen/events.h |    6 ++++++
>  2 files changed, 44 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index da70f5c..c9343b9 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -89,6 +89,7 @@ struct irq_info
>  {
>  	struct list_head list;
>  	enum xen_irq_type type;	/* type */
> +	unsigned short refcount;

Is short large enough?  Is this something that untrusted userspace could
end up wrapping?  If short is sufficient, you should pack it next to the
other short fields to avoid a gap.

>  	unsigned irq;
>  	unsigned short evtchn;	/* event channel */
>  	unsigned short cpu;	/* cpu bound */
> @@ -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;
> +	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(info->refcount > 1);
> +
>  	kfree(info);
>  
>  	/* Legacy IRQ descriptors are managed by the arch. */
> @@ -912,9 +916,14 @@ 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);
>  
>  	spin_lock(&irq_mapping_update_lock);
>  
> +	info->refcount--;
> +	if (info->refcount > 0)
> +		goto out_unlock;
> +
>  	if (VALID_EVTCHN(evtchn)) {
>  		close.port = evtchn;
>  		if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
> @@ -943,6 +952,7 @@ static void unbind_from_irq(unsigned int irq)
>  
>  	xen_free_irq(irq);
>  
> + out_unlock:
>  	spin_unlock(&irq_mapping_update_lock);
>  }
>  
> @@ -1038,6 +1048,34 @@ void unbind_from_irqhandler(unsigned int irq, void *dev_id)
>  }
>  EXPORT_SYMBOL_GPL(unbind_from_irqhandler);
>  
> +int get_evtchn_reservation(unsigned int evtchn)
"reservation"?  I think just evtchn_get/put would be more consistent
with kernel naming conventions.

> +{
> +	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;
> +
> +	spin_lock(&irq_mapping_update_lock);
> +	info->refcount++;
> +	spin_unlock(&irq_mapping_update_lock);

What is this spinlock protecting against?  The non-atomicity of ++, or
something larger scale?  If its just an atomicity thing, should it be an
atomic_t?

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(get_evtchn_reservation);
> +
> +void put_evtchn_reservation(unsigned int evtchn)
> +{
> +	int irq = evtchn_to_irq[evtchn];
> +	unbind_from_irq(irq);

Hm.

> +}
> +EXPORT_SYMBOL_GPL(put_evtchn_reservation);
> +
>  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..23bd5fd 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 get_evtchn_reservation(unsigned int evtchn);
> +void put_evtchn_reservation(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);

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

[Keir Fraser]

On 17/09/2011 05:50, "Jeremy Fitzhardinge" <jeremy@goop.org> wrote:

> On 09/16/2011 02:14 PM, 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.
> 
> Could you use the refcounting at the irq level?  I was quite pleased to
> have removed the event channel refcounting (and the use of naked event
> channels).
> 
> Oh, is it that userspace allocates an event channel with /dev/evtchn,
> then passes that event channel to the gntalloc/gntdev drivers so they
> can use it to pass events between the two.  That's a bit unfortunate; it
> might have been better to expose those event channels as file
> descriptors so you could use fd refcounting to manage the lifetimes.

The 'event channels' returned by /dev/evtchn are really 32-bit opaque
tokens, and can be whatever you want, as long as all kernel drivers are
updated to treat them consistently.

 -- Keir

> What's the downside of sending the event after the event channel has closed?
> 
> That said:
> 
>> 
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> ---
>>  drivers/xen/events.c |   38 ++++++++++++++++++++++++++++++++++++++
>>  include/xen/events.h |    6 ++++++
>>  2 files changed, 44 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
>> index da70f5c..c9343b9 100644
>> --- a/drivers/xen/events.c
>> +++ b/drivers/xen/events.c
>> @@ -89,6 +89,7 @@ struct irq_info
>>  {
>> struct list_head list;
>> enum xen_irq_type type; /* type */
>> + unsigned short refcount;
> 
> Is short large enough?  Is this something that untrusted userspace could
> end up wrapping?  If short is sufficient, you should pack it next to the
> other short fields to avoid a gap.
> 
>> unsigned irq;
>> unsigned short evtchn; /* event channel */
>> unsigned short cpu; /* cpu bound */
>> @@ -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;
>> + 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(info->refcount > 1);
>> +
>> kfree(info);
>>  
>> /* Legacy IRQ descriptors are managed by the arch. */
>> @@ -912,9 +916,14 @@ 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);
>>  
>> spin_lock(&irq_mapping_update_lock);
>>  
>> + info->refcount--;
>> + if (info->refcount > 0)
>> +  goto out_unlock;
>> +
>> if (VALID_EVTCHN(evtchn)) {
>> close.port = evtchn;
>> if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
>> @@ -943,6 +952,7 @@ static void unbind_from_irq(unsigned int irq)
>>  
>> xen_free_irq(irq);
>>  
>> + out_unlock:
>> spin_unlock(&irq_mapping_update_lock);
>>  }
>>  
>> @@ -1038,6 +1048,34 @@ void unbind_from_irqhandler(unsigned int irq, void
>> *dev_id)
>>  }
>>  EXPORT_SYMBOL_GPL(unbind_from_irqhandler);
>>  
>> +int get_evtchn_reservation(unsigned int evtchn)
> "reservation"?  I think just evtchn_get/put would be more consistent
> with kernel naming conventions.
> 
>> +{
>> + 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;
>> +
>> + spin_lock(&irq_mapping_update_lock);
>> + info->refcount++;
>> + spin_unlock(&irq_mapping_update_lock);
> 
> What is this spinlock protecting against?  The non-atomicity of ++, or
> something larger scale?  If its just an atomicity thing, should it be an
> atomic_t?
> 
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(get_evtchn_reservation);
>> +
>> +void put_evtchn_reservation(unsigned int evtchn)
>> +{
>> + int irq = evtchn_to_irq[evtchn];
>> + unbind_from_irq(irq);
> 
> Hm.
> 
>> +}
>> +EXPORT_SYMBOL_GPL(put_evtchn_reservation);
>> +
>>  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..23bd5fd 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 get_evtchn_reservation(unsigned int evtchn);
>> +void put_evtchn_reservation(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);
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

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

[Daniel De Graaf]

On 09/17/2011 12:50 AM, Jeremy Fitzhardinge wrote:
> On 09/16/2011 02:14 PM, 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.
> 
> Could you use the refcounting at the irq level?  I was quite pleased to
> have removed the event channel refcounting (and the use of naked event
> channels).

This looked more complex: unbind_from_irq has cleanup that happens after
EVTCHNOP_close but before the irq-refcount-protected close operation.
For the IRQ-level refcounting to be useful here, the EVTCHNOP_close would
have to be postponed. The evtchn_to_irq mapping is also maintained here.
 
> Oh, is it that userspace allocates an event channel with /dev/evtchn,
> then passes that event channel to the gntalloc/gntdev drivers so they
> can use it to pass events between the two.  That's a bit unfortunate; it
> might have been better to expose those event channels as file
> descriptors so you could use fd refcounting to manage the lifetimes.

This would also make event channels simpler to use from userspace, avoiding
the extra read() to determine what event channel has fired (and avoiding
almost all userspace knowledge of the local event channel number). However,
this would require practically rewriting the /dev/xen/evtchn userspace API.

With the current event channel API, you would have to pass both the event
channel number and the evtchn file descriptor in order to get an fd reference
which seems redundant and might cause confusion since keeping the fd
reference would also prevent cleanup of other event channels associated with
the shared file descriptor. It would also require changing the notify ioctl
parameters since there is currently no need to pass an evtchn fd.
 
> What's the downside of sending the event after the event channel has closed?

The event won't get sent at all, since the hypervisor sees that the local
end of the event channel is closed. If the local event channel number
happens to get reused, the event could also get sent to the wrong event
channel, although this is generally harmless.

> That said:
> 
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> ---
>>  drivers/xen/events.c |   38 ++++++++++++++++++++++++++++++++++++++
>>  include/xen/events.h |    6 ++++++
>>  2 files changed, 44 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
>> index da70f5c..c9343b9 100644
>> --- a/drivers/xen/events.c
>> +++ b/drivers/xen/events.c
>> @@ -89,6 +89,7 @@ struct irq_info
>>  {
>>  	struct list_head list;
>>  	enum xen_irq_type type;	/* type */
>> +	unsigned short refcount;
> 
> Is short large enough?  Is this something that untrusted userspace could
> end up wrapping?  If short is sufficient, you should pack it next to the
> other short fields to avoid a gap.

This would be quite difficult: since only gntdev/gntalloc use these counts
for now, you would have to map or allocate 64K pages; the gntalloc driver
has a default limit of 1024 pages and gntdev is limited by the hypervisor
max_nr_grant_frames.

Anyway, changing to atomic_t should
 
>>  	unsigned irq;
>>  	unsigned short evtchn;	/* event channel */
>>  	unsigned short cpu;	/* cpu bound */
>> @@ -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;
>> +	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(info->refcount > 1);
>> +
>>  	kfree(info);
>>  
>>  	/* Legacy IRQ descriptors are managed by the arch. */
>> @@ -912,9 +916,14 @@ 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);
>>  
>>  	spin_lock(&irq_mapping_update_lock);
>>  
>> +	info->refcount--;
>> +	if (info->refcount > 0)
>> +		goto out_unlock;
>> +
>>  	if (VALID_EVTCHN(evtchn)) {
>>  		close.port = evtchn;
>>  		if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
>> @@ -943,6 +952,7 @@ static void unbind_from_irq(unsigned int irq)
>>  
>>  	xen_free_irq(irq);
>>  
>> + out_unlock:
>>  	spin_unlock(&irq_mapping_update_lock);
>>  }
>>  
>> @@ -1038,6 +1048,34 @@ void unbind_from_irqhandler(unsigned int irq, void *dev_id)
>>  }
>>  EXPORT_SYMBOL_GPL(unbind_from_irqhandler);
>>  
>> +int get_evtchn_reservation(unsigned int evtchn)
> "reservation"?  I think just evtchn_get/put would be more consistent
> with kernel naming conventions.

Yes.

>> +{
>> +	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;
>> +
>> +	spin_lock(&irq_mapping_update_lock);
>> +	info->refcount++;
>> +	spin_unlock(&irq_mapping_update_lock);
> 
> What is this spinlock protecting against?  The non-atomicity of ++, or
> something larger scale?  If its just an atomicity thing, should it be an
> atomic_t?

Just atomicity.

>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(get_evtchn_reservation);
>> +
>> +void put_evtchn_reservation(unsigned int evtchn)
>> +{
>> +	int irq = evtchn_to_irq[evtchn];
>> +	unbind_from_irq(irq);
> 
> Hm.

Similar name change here.

>> +}
>> +EXPORT_SYMBOL_GPL(put_evtchn_reservation);
>> +
>>  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..23bd5fd 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 get_evtchn_reservation(unsigned int evtchn);
>> +void put_evtchn_reservation(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);
> 


-- 
Daniel De Graaf
National Security Agency

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

[Jeremy Fitzhardinge]

On 09/19/2011 08:50 AM, Daniel De Graaf wrote:
> On 09/17/2011 12:50 AM, Jeremy Fitzhardinge wrote:
>> On 09/16/2011 02:14 PM, 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.
>> Could you use the refcounting at the irq level?  I was quite pleased to
>> have removed the event channel refcounting (and the use of naked event
>> channels).
> This looked more complex: unbind_from_irq has cleanup that happens after
> EVTCHNOP_close but before the irq-refcount-protected close operation.
> For the IRQ-level refcounting to be useful here, the EVTCHNOP_close would
> have to be postponed. The evtchn_to_irq mapping is also maintained here.
>  
>> Oh, is it that userspace allocates an event channel with /dev/evtchn,
>> then passes that event channel to the gntalloc/gntdev drivers so they
>> can use it to pass events between the two.  That's a bit unfortunate; it
>> might have been better to expose those event channels as file
>> descriptors so you could use fd refcounting to manage the lifetimes.
> This would also make event channels simpler to use from userspace, avoiding
> the extra read() to determine what event channel has fired (and avoiding
> almost all userspace knowledge of the local event channel number). However,
> this would require practically rewriting the /dev/xen/evtchn userspace API.

If you had an app which cared about many events, then having an fd for
each would be pretty cumbersome.  But perhaps it would be worth
considering extending the current API a bit to add a general "set of
events fd", where a given event can be the member of multiple sets.

> With the current event channel API, you would have to pass both the event
> channel number and the evtchn file descriptor in order to get an fd reference
> which seems redundant and might cause confusion since keeping the fd
> reference would also prevent cleanup of other event channels associated with
> the shared file descriptor. It would also require changing the notify ioctl
> parameters since there is currently no need to pass an evtchn fd.

Yes, though there's no reason even now one couldn't be careful to bind
your events to a given fd where they share similar lifetimes/uses (even
to the extent of allocating single-event fds).

On the other hand, is there really a need to make a connection between
the gnt and evtchn devices?  Why couldn't the gnt driver just do its own
form of event fd and management independent of /dev/evtchn?  I know it
seems a bit redundant, but coupling the two adds its own complexities.

>> What's the downside of sending the event after the event channel has closed?
> The event won't get sent at all, since the hypervisor sees that the local
> end of the event channel is closed. If the local event channel number
> happens to get reused, the event could also get sent to the wrong event
> channel, although this is generally harmless.

I think /dev/evtchn port numbers are never reused.  And it sounds like
it is purely a usermode problem if they close the event channel prematurely.

    J

>> That said:
>>
>>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>> ---
>>>  drivers/xen/events.c |   38 ++++++++++++++++++++++++++++++++++++++
>>>  include/xen/events.h |    6 ++++++
>>>  2 files changed, 44 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
>>> index da70f5c..c9343b9 100644
>>> --- a/drivers/xen/events.c
>>> +++ b/drivers/xen/events.c
>>> @@ -89,6 +89,7 @@ struct irq_info
>>>  {
>>>  	struct list_head list;
>>>  	enum xen_irq_type type;	/* type */
>>> +	unsigned short refcount;
>> Is short large enough?  Is this something that untrusted userspace could
>> end up wrapping?  If short is sufficient, you should pack it next to the
>> other short fields to avoid a gap.
> This would be quite difficult: since only gntdev/gntalloc use these counts
> for now, you would have to map or allocate 64K pages; the gntalloc driver
> has a default limit of 1024 pages and gntdev is limited by the hypervisor
> max_nr_grant_frames.
>
> Anyway, changing to atomic_t should
>  
>>>  	unsigned irq;
>>>  	unsigned short evtchn;	/* event channel */
>>>  	unsigned short cpu;	/* cpu bound */
>>> @@ -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;
>>> +	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(info->refcount > 1);
>>> +
>>>  	kfree(info);
>>>  
>>>  	/* Legacy IRQ descriptors are managed by the arch. */
>>> @@ -912,9 +916,14 @@ 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);
>>>  
>>>  	spin_lock(&irq_mapping_update_lock);
>>>  
>>> +	info->refcount--;
>>> +	if (info->refcount > 0)
>>> +		goto out_unlock;
>>> +
>>>  	if (VALID_EVTCHN(evtchn)) {
>>>  		close.port = evtchn;
>>>  		if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
>>> @@ -943,6 +952,7 @@ static void unbind_from_irq(unsigned int irq)
>>>  
>>>  	xen_free_irq(irq);
>>>  
>>> + out_unlock:
>>>  	spin_unlock(&irq_mapping_update_lock);
>>>  }
>>>  
>>> @@ -1038,6 +1048,34 @@ void unbind_from_irqhandler(unsigned int irq, void *dev_id)
>>>  }
>>>  EXPORT_SYMBOL_GPL(unbind_from_irqhandler);
>>>  
>>> +int get_evtchn_reservation(unsigned int evtchn)
>> "reservation"?  I think just evtchn_get/put would be more consistent
>> with kernel naming conventions.
> Yes.
>
>>> +{
>>> +	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;
>>> +
>>> +	spin_lock(&irq_mapping_update_lock);
>>> +	info->refcount++;
>>> +	spin_unlock(&irq_mapping_update_lock);
>> What is this spinlock protecting against?  The non-atomicity of ++, or
>> something larger scale?  If its just an atomicity thing, should it be an
>> atomic_t?
> Just atomicity.
>
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(get_evtchn_reservation);
>>> +
>>> +void put_evtchn_reservation(unsigned int evtchn)
>>> +{
>>> +	int irq = evtchn_to_irq[evtchn];
>>> +	unbind_from_irq(irq);
>> Hm.
> Similar name change here.
>
>>> +}
>>> +EXPORT_SYMBOL_GPL(put_evtchn_reservation);
>>> +
>>>  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..23bd5fd 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 get_evtchn_reservation(unsigned int evtchn);
>>> +void put_evtchn_reservation(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);
>

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

[Daniel De Graaf]

On 09/19/2011 12:22 PM, Jeremy Fitzhardinge wrote:
> On 09/19/2011 08:50 AM, Daniel De Graaf wrote:
>> On 09/17/2011 12:50 AM, Jeremy Fitzhardinge wrote:
>>> On 09/16/2011 02:14 PM, 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.
>>> Could you use the refcounting at the irq level?  I was quite pleased to
>>> have removed the event channel refcounting (and the use of naked event
>>> channels).
>> This looked more complex: unbind_from_irq has cleanup that happens after
>> EVTCHNOP_close but before the irq-refcount-protected close operation.
>> For the IRQ-level refcounting to be useful here, the EVTCHNOP_close would
>> have to be postponed. The evtchn_to_irq mapping is also maintained here.
>>  
>>> Oh, is it that userspace allocates an event channel with /dev/evtchn,
>>> then passes that event channel to the gntalloc/gntdev drivers so they
>>> can use it to pass events between the two.  That's a bit unfortunate; it
>>> might have been better to expose those event channels as file
>>> descriptors so you could use fd refcounting to manage the lifetimes.
>> This would also make event channels simpler to use from userspace, avoiding
>> the extra read() to determine what event channel has fired (and avoiding
>> almost all userspace knowledge of the local event channel number). However,
>> this would require practically rewriting the /dev/xen/evtchn userspace API.
> 
> If you had an app which cared about many events, then having an fd for
> each would be pretty cumbersome.  But perhaps it would be worth
> considering extending the current API a bit to add a general "set of
> events fd", where a given event can be the member of multiple sets.
 
This seems to overlap a lot with epoll, which can already do overlapping
file descriptor readiness checks.

>> With the current event channel API, you would have to pass both the event
>> channel number and the evtchn file descriptor in order to get an fd reference
>> which seems redundant and might cause confusion since keeping the fd
>> reference would also prevent cleanup of other event channels associated with
>> the shared file descriptor. It would also require changing the notify ioctl
>> parameters since there is currently no need to pass an evtchn fd.
> 
> Yes, though there's no reason even now one couldn't be careful to bind
> your events to a given fd where they share similar lifetimes/uses (even
> to the extent of allocating single-event fds).
> 
> On the other hand, is there really a need to make a connection between
> the gnt and evtchn devices?  Why couldn't the gnt driver just do its own
> form of event fd and management independent of /dev/evtchn?  I know it
> seems a bit redundant, but coupling the two adds its own complexities.

I don't think there is a real need to make a connection, and even with this
patch there isn't one: only tracking on the event channel, not on the evtchn
device or its FDs.

Unless you're talking about having /dev/xen/gnt* able to manage event channels
independent of /dev/xen/evtchn? To be useful, this would either need to be able
to both send and receive events, or to share local/remote ports with evtchn.

>>> What's the downside of sending the event after the event channel has closed?
>> The event won't get sent at all, since the hypervisor sees that the local
>> end of the event channel is closed. If the local event channel number
>> happens to get reused, the event could also get sent to the wrong event
>> channel, although this is generally harmless.
> 
> I think /dev/evtchn port numbers are never reused.  And it sounds like
> it is purely a usermode problem if they close the event channel prematurely.

The trigger in this case is the kernel closing file descriptors when a
process crashes. In particular:

1. open /dev/xen/evtchn (FD 3) and bind_interdomain an event channel
2. open /dev/xen/gntdev (FD 4) map a granted page
3. use unmap_notify on fd-4 to setup a notification on the event channel
   allocated in step 1
4. segfault, SIGKILL, or some other exit without proper cleanup

At this point, the kernel closes file descriptors in numerical order, which
will cause the event channel to be closed before the gntdev device is able
to send its unmap notification event. In libvchan, I make sure to swap steps
1 and 2 to avoid triggering this bug, but that is clearly a workaround and
not a proper solution.

I have seen /dev/xen/evtchn ports be reused; the comment in evtchn.c is
misleading or incorrect. The port is allocated by the hypervisor in
xen/common/event_channel.c:get_free_port.

>     J
> 
>>> That said:
>>>
>>>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>>> ---
>>>>  drivers/xen/events.c |   38 ++++++++++++++++++++++++++++++++++++++
>>>>  include/xen/events.h |    6 ++++++
>>>>  2 files changed, 44 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
>>>> index da70f5c..c9343b9 100644
>>>> --- a/drivers/xen/events.c
>>>> +++ b/drivers/xen/events.c
>>>> @@ -89,6 +89,7 @@ struct irq_info
>>>>  {
>>>>  	struct list_head list;
>>>>  	enum xen_irq_type type;	/* type */
>>>> +	unsigned short refcount;
>>> Is short large enough?  Is this something that untrusted userspace could
>>> end up wrapping?  If short is sufficient, you should pack it next to the
>>> other short fields to avoid a gap.
>> This would be quite difficult: since only gntdev/gntalloc use these counts
>> for now, you would have to map or allocate 64K pages; the gntalloc driver
>> has a default limit of 1024 pages and gntdev is limited by the hypervisor
>> max_nr_grant_frames.
>>
>> Anyway, changing to atomic_t should
>>  
>>>>  	unsigned irq;
>>>>  	unsigned short evtchn;	/* event channel */
>>>>  	unsigned short cpu;	/* cpu bound */
>>>> @@ -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;
>>>> +	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(info->refcount > 1);
>>>> +
>>>>  	kfree(info);
>>>>  
>>>>  	/* Legacy IRQ descriptors are managed by the arch. */
>>>> @@ -912,9 +916,14 @@ 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);
>>>>  
>>>>  	spin_lock(&irq_mapping_update_lock);
>>>>  
>>>> +	info->refcount--;
>>>> +	if (info->refcount > 0)
>>>> +		goto out_unlock;
>>>> +
>>>>  	if (VALID_EVTCHN(evtchn)) {
>>>>  		close.port = evtchn;
>>>>  		if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
>>>> @@ -943,6 +952,7 @@ static void unbind_from_irq(unsigned int irq)
>>>>  
>>>>  	xen_free_irq(irq);
>>>>  
>>>> + out_unlock:
>>>>  	spin_unlock(&irq_mapping_update_lock);
>>>>  }
>>>>  
>>>> @@ -1038,6 +1048,34 @@ void unbind_from_irqhandler(unsigned int irq, void *dev_id)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(unbind_from_irqhandler);
>>>>  
>>>> +int get_evtchn_reservation(unsigned int evtchn)
>>> "reservation"?  I think just evtchn_get/put would be more consistent
>>> with kernel naming conventions.
>> Yes.
>>
>>>> +{
>>>> +	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;
>>>> +
>>>> +	spin_lock(&irq_mapping_update_lock);
>>>> +	info->refcount++;
>>>> +	spin_unlock(&irq_mapping_update_lock);
>>> What is this spinlock protecting against?  The non-atomicity of ++, or
>>> something larger scale?  If its just an atomicity thing, should it be an
>>> atomic_t?
>> Just atomicity.
>>
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(get_evtchn_reservation);
>>>> +
>>>> +void put_evtchn_reservation(unsigned int evtchn)
>>>> +{
>>>> +	int irq = evtchn_to_irq[evtchn];
>>>> +	unbind_from_irq(irq);
>>> Hm.
>> Similar name change here.
>>
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(put_evtchn_reservation);
>>>> +
>>>>  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..23bd5fd 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 get_evtchn_reservation(unsigned int evtchn);
>>>> +void put_evtchn_reservation(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);
>>
> 


-- 
Daniel De Graaf
National Security Agency

[PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Daniel De Graaf]

When using the unmap notify ioctl, the event channel used for
notification needs to be reserved to avoid it being deallocated prior to
sending the notification.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/gntalloc.c |   14 +++++++++++++-
 drivers/xen/gntdev.c   |   11 +++++++++++
 2 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index f6832f4..cff862b 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -178,8 +178,10 @@ static void __del_gref(struct gntalloc_gref *gref)
 		tmp[gref->notify.pgoff] = 0;
 		kunmap(gref->page);
 	}
-	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
+	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
 		notify_remote_via_evtchn(gref->notify.event);
+		put_evtchn_reservation(gref->notify.event);
+	}
 
 	gref->notify.flags = 0;
 
@@ -396,6 +398,16 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
 		goto unlock_out;
 	}
 
+	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
+		if (get_evtchn_reservation(op.event_channel_port)) {
+			rc = -EINVAL;
+			goto unlock_out;
+		}
+	}
+
+	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
+		put_evtchn_reservation(gref->notify.event);
+
 	gref->notify.flags = op.action;
 	gref->notify.pgoff = pgoff;
 	gref->notify.event = op.event_channel_port;
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index f914b26..8de393c 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -190,6 +190,7 @@ static void gntdev_put_map(struct grant_map *map)
 
 	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
 		notify_remote_via_evtchn(map->notify.event);
+		put_evtchn_reservation(map->notify.event);
 	}
 
 	if (map->pages) {
@@ -596,6 +597,16 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
 		goto unlock_out;
 	}
 
+	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
+		if (get_evtchn_reservation(op.event_channel_port)) {
+			rc = -EINVAL;
+			goto unlock_out;
+		}
+	}
+
+	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
+		put_evtchn_reservation(map->notify.event);
+
 	map->notify.flags = op.action;
 	map->notify.addr = op.index - (map->index << PAGE_SHIFT);
 	map->notify.event = op.event_channel_port;
-- 
1.7.6

[PATCH v2 0/2] Add reference counting to grant notify ioctls

[Daniel De Graaf]

> The current notify ioctls assume that an event channel will not be
> closed prior to the page being unmapped. If the mappings are associated
> with an open file descriptor and the application crashes, the
> notification behavior depends on the close ordering of the file
> descriptors. To avoid this, event channels now have a reference count
> that is used by the grant notify ioctls to postpone the close operation
> until the notification is fired.
 
Changes since v1:
	Rename evtchn_get/put to match kernel naming conventions
	Use atomic_t for refcount

[PATCH 1/2] xen/event: Add reference counting to event channel
[PATCH 2/2] xen/gnt{dev,alloc}: reserve event channels for notify

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

[Daniel De Graaf]

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.

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;
 	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)
+{
+	int irq = evtchn_to_irq[evtchn];
+	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

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

[Ian Campbell]

On Tue, 2011-10-18 at 22:04 +0100, 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.
> 
> 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;
>  	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);

Should this be > 0 ? If we get here with a reference count still held
isn't that indicative of an issue?

Is this just because of the various error paths which call xen_free_irq
to clean up without also dropping the initial reference from
xen_irq_init? I think either these paths should just drop the reference
or the initial refcount should be 0 and the initial reference should be
taken only after the caller of xen_irq_init has actually succeeded.

> +
>  	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)
> +{
> +	int irq = evtchn_to_irq[evtchn];
> +	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);

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

[Konrad Rzeszutek Wilk]

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

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.

 2). Grantdev holding the refcnt forever. That is probably the easiest as it would
     be a bug in the 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

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

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

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

[Daniel De Graaf]

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
> 

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

[Konrad Rzeszutek Wilk]

> >> +	atomic_t refcount;
> > 
> > refcnt
> > 
> 
> Ah yes, vowels are much too valuable to use more than one here...

<laughs> Well, optimization!!

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

[Ian Campbell]

On Mon, 2011-10-24 at 21:22 +0100, Konrad Rzeszutek Wilk wrote:
> 
> > +void evtchn_put(unsigned int evtchn)
> 
> The decleration for 'evtchn' is 'unsigned short' so that can be
> used instead of 'unsigned int'.

I think I nearly made the same comment but then I looked at
drivers/xen/events.c and found that it uses "unsigned", "unsigned
short", "unsigned int" and "int" fairly interchangeably. The externally
visible API (i.e. include/xen/events.h) tends to stick to just "unsigned
int" or "int".

The problem with the short types is that they don't leave room for an
error indication, which is why on the Xen tools (libxc) side we have his
piece of minor ugliness:
        typedef int evtchn_port_or_error_t;

The actual hypercall interface uses "typedef uint32_t evtchn_port_t" but
AIUI real evtchn values are guaranteed to fit in 31 bits.

Ian.

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

[Konrad Rzeszutek Wilk]

On Tue, Oct 25, 2011 at 09:17:15AM +0100, Ian Campbell wrote:
> On Mon, 2011-10-24 at 21:22 +0100, Konrad Rzeszutek Wilk wrote:
> > 
> > > +void evtchn_put(unsigned int evtchn)
> > 
> > The decleration for 'evtchn' is 'unsigned short' so that can be
> > used instead of 'unsigned int'.
> 
> I think I nearly made the same comment but then I looked at
> drivers/xen/events.c and found that it uses "unsigned", "unsigned
> short", "unsigned int" and "int" fairly interchangeably. The externally
> visible API (i.e. include/xen/events.h) tends to stick to just "unsigned
> int" or "int".
> 
> The problem with the short types is that they don't leave room for an
> error indication, which is why on the Xen tools (libxc) side we have his
> piece of minor ugliness:
>         typedef int evtchn_port_or_error_t;
> 
> The actual hypercall interface uses "typedef uint32_t evtchn_port_t" but
> AIUI real evtchn values are guaranteed to fit in 31 bits.


Oh fun! So we actually have a bug with 'unsigned short' cutting it down
from 2^31 to 2^16!

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

[Ian Campbell]

On Tue, 2011-10-25 at 20:09 +0100, Konrad Rzeszutek Wilk wrote:
> On Tue, Oct 25, 2011 at 09:17:15AM +0100, Ian Campbell wrote:
> > On Mon, 2011-10-24 at 21:22 +0100, Konrad Rzeszutek Wilk wrote:
> > > 
> > > > +void evtchn_put(unsigned int evtchn)
> > > 
> > > The decleration for 'evtchn' is 'unsigned short' so that can be
> > > used instead of 'unsigned int'.
> > 
> > I think I nearly made the same comment but then I looked at
> > drivers/xen/events.c and found that it uses "unsigned", "unsigned
> > short", "unsigned int" and "int" fairly interchangeably. The externally
> > visible API (i.e. include/xen/events.h) tends to stick to just "unsigned
> > int" or "int".
> > 
> > The problem with the short types is that they don't leave room for an
> > error indication, which is why on the Xen tools (libxc) side we have his
> > piece of minor ugliness:
> >         typedef int evtchn_port_or_error_t;
> > 
> > The actual hypercall interface uses "typedef uint32_t evtchn_port_t" but
> > AIUI real evtchn values are guaranteed to fit in 31 bits.
> 
> 
> Oh fun! So we actually have a bug with 'unsigned short' cutting it down
> from 2^31 to 2^16!

The actual maximums are 1024 (on x86_32) and 4096 (on x86_64) so
"guaranteed to fit 31 bits" (which I took from the xenctrl.h comment) is
more a case of definitely big enough. The use of 31 is more of an
allusion to errors being negative, I think.

Ian

[PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Daniel De Graaf]

When using the unmap notify ioctl, the event channel used for
notification needs to be reserved to avoid it being deallocated prior to
sending the notification.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/gntalloc.c |   14 +++++++++++++-
 drivers/xen/gntdev.c   |   11 +++++++++++
 2 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index f6832f4..a739fb1 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -178,8 +178,10 @@ static void __del_gref(struct gntalloc_gref *gref)
 		tmp[gref->notify.pgoff] = 0;
 		kunmap(gref->page);
 	}
-	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
+	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
 		notify_remote_via_evtchn(gref->notify.event);
+		evtchn_put(gref->notify.event);
+	}
 
 	gref->notify.flags = 0;
 
@@ -396,6 +398,16 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
 		goto unlock_out;
 	}
 
+	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
+		if (evtchn_get(op.event_channel_port)) {
+			rc = -EINVAL;
+			goto unlock_out;
+		}
+	}
+
+	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
+		evtchn_put(gref->notify.event);
+
 	gref->notify.flags = op.action;
 	gref->notify.pgoff = pgoff;
 	gref->notify.event = op.event_channel_port;
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index f914b26..cfcc890 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -190,6 +190,7 @@ static void gntdev_put_map(struct grant_map *map)
 
 	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
 		notify_remote_via_evtchn(map->notify.event);
+		evtchn_put(map->notify.event);
 	}
 
 	if (map->pages) {
@@ -596,6 +597,16 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
 		goto unlock_out;
 	}
 
+	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
+		if (evtchn_get(op.event_channel_port)) {
+			rc = -EINVAL;
+			goto unlock_out;
+		}
+	}
+
+	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
+		evtchn_put(map->notify.event);
+
 	map->notify.flags = op.action;
 	map->notify.addr = op.index - (map->index << PAGE_SHIFT);
 	map->notify.event = op.event_channel_port;
-- 
1.7.6.4

Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Ian Campbell]

On Tue, 2011-10-18 at 22:04 +0100, Daniel De Graaf wrote:
> When using the unmap notify ioctl, the event channel used for
> notification needs to be reserved to avoid it being deallocated prior to
> sending the notification.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  drivers/xen/gntalloc.c |   14 +++++++++++++-
>  drivers/xen/gntdev.c   |   11 +++++++++++
>  2 files changed, 24 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
> index f6832f4..a739fb1 100644
> --- a/drivers/xen/gntalloc.c
> +++ b/drivers/xen/gntalloc.c
> @@ -178,8 +178,10 @@ static void __del_gref(struct gntalloc_gref *gref)
>  		tmp[gref->notify.pgoff] = 0;
>  		kunmap(gref->page);
>  	}
> -	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
> +	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
>  		notify_remote_via_evtchn(gref->notify.event);
> +		evtchn_put(gref->notify.event);
> +	}
>  
>  	gref->notify.flags = 0;
>  
> @@ -396,6 +398,16 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
>  		goto unlock_out;
>  	}
>  
> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
> +		if (evtchn_get(op.event_channel_port)) {
> +			rc = -EINVAL;
> +			goto unlock_out;
> +		}
> +	}
> +
> +	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
> +		evtchn_put(gref->notify.event);
> +

If the gref gets torn down here won't we notify and drop the reference
on the wrong evtchn, leading to a double free? If we defer the drop
until after gref->notify.event has been updated then this goes away.

Ian.

>  	gref->notify.flags = op.action;
>  	gref->notify.pgoff = pgoff;
>  	gref->notify.event = op.event_channel_port;
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index f914b26..cfcc890 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -190,6 +190,7 @@ static void gntdev_put_map(struct grant_map *map)
>  
>  	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
>  		notify_remote_via_evtchn(map->notify.event);
> +		evtchn_put(map->notify.event);
>  	}
>  
>  	if (map->pages) {
> @@ -596,6 +597,16 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
>  		goto unlock_out;
>  	}
>  
> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
> +		if (evtchn_get(op.event_channel_port)) {
> +			rc = -EINVAL;
> +			goto unlock_out;
> +		}
> +	}
> +
> +	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
> +		evtchn_put(map->notify.event);
> +
>  	map->notify.flags = op.action;
>  	map->notify.addr = op.index - (map->index << PAGE_SHIFT);
>  	map->notify.event = op.event_channel_port;

Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Daniel De Graaf]

On 10/19/2011 05:24 AM, Ian Campbell wrote:
> On Tue, 2011-10-18 at 22:04 +0100, Daniel De Graaf wrote:
>> When using the unmap notify ioctl, the event channel used for
>> notification needs to be reserved to avoid it being deallocated prior to
>> sending the notification.
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> ---
>>  drivers/xen/gntalloc.c |   14 +++++++++++++-
>>  drivers/xen/gntdev.c   |   11 +++++++++++
>>  2 files changed, 24 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
>> index f6832f4..a739fb1 100644
>> --- a/drivers/xen/gntalloc.c
>> +++ b/drivers/xen/gntalloc.c
>> @@ -178,8 +178,10 @@ static void __del_gref(struct gntalloc_gref *gref)
>>  		tmp[gref->notify.pgoff] = 0;
>>  		kunmap(gref->page);
>>  	}
>> -	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
>> +	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
>>  		notify_remote_via_evtchn(gref->notify.event);
>> +		evtchn_put(gref->notify.event);
>> +	}
>>  
>>  	gref->notify.flags = 0;
>>  
>> @@ -396,6 +398,16 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
>>  		goto unlock_out;
>>  	}
>>  
>> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
>> +		if (evtchn_get(op.event_channel_port)) {
>> +			rc = -EINVAL;
>> +			goto unlock_out;
>> +		}
>> +	}
>> +
>> +	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
>> +		evtchn_put(gref->notify.event);
>> +
> 
> If the gref gets torn down here won't we notify and drop the reference
> on the wrong evtchn, leading to a double free? If we defer the drop
> until after gref->notify.event has been updated then this goes away.
> 
> Ian.

This evtchn_put will only be called in the case where the unmap_notify
is being changed and already had an event channel reference. This reference
must be dropped prior to changing gref->notify.event or we will leak
the old event channel.

> 
>>  	gref->notify.flags = op.action;
>>  	gref->notify.pgoff = pgoff;
>>  	gref->notify.event = op.event_channel_port;
>> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
>> index f914b26..cfcc890 100644
>> --- a/drivers/xen/gntdev.c
>> +++ b/drivers/xen/gntdev.c
>> @@ -190,6 +190,7 @@ static void gntdev_put_map(struct grant_map *map)
>>  
>>  	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
>>  		notify_remote_via_evtchn(map->notify.event);
>> +		evtchn_put(map->notify.event);
>>  	}
>>  
>>  	if (map->pages) {
>> @@ -596,6 +597,16 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
>>  		goto unlock_out;
>>  	}
>>  
>> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
>> +		if (evtchn_get(op.event_channel_port)) {
>> +			rc = -EINVAL;
>> +			goto unlock_out;
>> +		}
>> +	}
>> +
>> +	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
>> +		evtchn_put(map->notify.event);
>> +
>>  	map->notify.flags = op.action;
>>  	map->notify.addr = op.index - (map->index << PAGE_SHIFT);
>>  	map->notify.event = op.event_channel_port;
> 
> 

Re: [PATCH 2/2] xen/gnt{dev,alloc}: reserve event channels for notify

[Ian Campbell]

On Wed, 2011-10-19 at 15:23 +0100, Daniel De Graaf wrote:
> On 10/19/2011 05:24 AM, Ian Campbell wrote:
> > On Tue, 2011-10-18 at 22:04 +0100, Daniel De Graaf wrote:
> >> When using the unmap notify ioctl, the event channel used for
> >> notification needs to be reserved to avoid it being deallocated prior to
> >> sending the notification.
> >>
> >> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> >> ---
> >>  drivers/xen/gntalloc.c |   14 +++++++++++++-
> >>  drivers/xen/gntdev.c   |   11 +++++++++++
> >>  2 files changed, 24 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
> >> index f6832f4..a739fb1 100644
> >> --- a/drivers/xen/gntalloc.c
> >> +++ b/drivers/xen/gntalloc.c
> >> @@ -178,8 +178,10 @@ static void __del_gref(struct gntalloc_gref *gref)
> >>  		tmp[gref->notify.pgoff] = 0;
> >>  		kunmap(gref->page);
> >>  	}
> >> -	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
> >> +	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
> >>  		notify_remote_via_evtchn(gref->notify.event);
> >> +		evtchn_put(gref->notify.event);
> >> +	}
> >>  
> >>  	gref->notify.flags = 0;
> >>  
> >> @@ -396,6 +398,16 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
> >>  		goto unlock_out;
> >>  	}
> >>  
> >> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
> >> +		if (evtchn_get(op.event_channel_port)) {
> >> +			rc = -EINVAL;
> >> +			goto unlock_out;
> >> +		}
> >> +	}
> >> +
> >> +	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
> >> +		evtchn_put(gref->notify.event);
> >> +
> > 
> > If the gref gets torn down here won't we notify and drop the reference
> > on the wrong evtchn, leading to a double free? If we defer the drop
> > until after gref->notify.event has been updated then this goes away.
> > 
> > Ian.
> 
> This evtchn_put will only be called in the case where the unmap_notify
> is being changed and already had an event channel reference. This reference
> must be dropped prior to changing gref->notify.event or we will leak
> the old event channel.

More importantly I see now that there is a lock protecting all this
stuff so everything is ok.

Ian.

> 
> > 
> >>  	gref->notify.flags = op.action;
> >>  	gref->notify.pgoff = pgoff;
> >>  	gref->notify.event = op.event_channel_port;
> >> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> >> index f914b26..cfcc890 100644
> >> --- a/drivers/xen/gntdev.c
> >> +++ b/drivers/xen/gntdev.c
> >> @@ -190,6 +190,7 @@ static void gntdev_put_map(struct grant_map *map)
> >>  
> >>  	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
> >>  		notify_remote_via_evtchn(map->notify.event);
> >> +		evtchn_put(map->notify.event);
> >>  	}
> >>  
> >>  	if (map->pages) {
> >> @@ -596,6 +597,16 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
> >>  		goto unlock_out;
> >>  	}
> >>  
> >> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
> >> +		if (evtchn_get(op.event_channel_port)) {
> >> +			rc = -EINVAL;
> >> +			goto unlock_out;
> >> +		}
> >> +	}
> >> +
> >> +	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
> >> +		evtchn_put(map->notify.event);
> >> +
> >>  	map->notify.flags = op.action;
> >>  	map->notify.addr = op.index - (map->index << PAGE_SHIFT);
> >>  	map->notify.event = op.event_channel_port;
> > 
> > 

Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Konrad Rzeszutek Wilk]

On Tue, Oct 18, 2011 at 05:04:11PM -0400, Daniel De Graaf wrote:
> When using the unmap notify ioctl, the event channel used for
> notification needs to be reserved to avoid it being deallocated prior to
> sending the notification.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  drivers/xen/gntalloc.c |   14 +++++++++++++-
>  drivers/xen/gntdev.c   |   11 +++++++++++
>  2 files changed, 24 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
> index f6832f4..a739fb1 100644
> --- a/drivers/xen/gntalloc.c
> +++ b/drivers/xen/gntalloc.c
> @@ -178,8 +178,10 @@ static void __del_gref(struct gntalloc_gref *gref)
>  		tmp[gref->notify.pgoff] = 0;
>  		kunmap(gref->page);
>  	}
> -	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
> +	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
>  		notify_remote_via_evtchn(gref->notify.event);
> +		evtchn_put(gref->notify.event);

So.. I could have some fun by doing this in the userspace:
  for (j = 0; j< 2;j++) {
      for (i = 0; i < 65534; i++) {
         struct ioctl_gntalloc_unmap_notify uarg = {
                 .index = arg.index + offsetof(struct shr_page, notifies[0]),
                 .action =UNMAP_NOTIFY_SEND_EVENT,
		 .event_channel_port = i,
         };
         rv = ioctl(a_fd, IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &uarg);
	}
  }

And cause the event channel refcnt to be set to zero and free it. And then
causing the box to die - as the event channels for the physical IRQ might have
gotten free-ed.


Hm.. Perhaps the gntalloc and gntdev should keep track of which event channels
are OK to refcnt? Something like a whitelist? Granted at that point the refcounting
could as well be done by the API that sets up the event channels from the userspace.

So the evtchn_ioctl is pretty smart. It uses "get_port_user" to get the list
of events that belong to this user (and have been handed out). I think you
need to use that in the gntalloc to double-check that the event channel is not
one of the kernel type.

> +	}
>  
>  	gref->notify.flags = 0;
>  
> @@ -396,6 +398,16 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
>  		goto unlock_out;
>  	}
>  
> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
> +		if (evtchn_get(op.event_channel_port)) {
> +			rc = -EINVAL;
> +			goto unlock_out;
> +		}
> +	}
> +
> +	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
> +		evtchn_put(gref->notify.event);
> +
>  	gref->notify.flags = op.action;
>  	gref->notify.pgoff = pgoff;
>  	gref->notify.event = op.event_channel_port;
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index f914b26..cfcc890 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -190,6 +190,7 @@ static void gntdev_put_map(struct grant_map *map)
>  
>  	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
>  		notify_remote_via_evtchn(map->notify.event);
> +		evtchn_put(map->notify.event);
>  	}
>  
>  	if (map->pages) {
> @@ -596,6 +597,16 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
>  		goto unlock_out;
>  	}
>  
> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
> +		if (evtchn_get(op.event_channel_port)) {
> +			rc = -EINVAL;
> +			goto unlock_out;
> +		}
> +	}
> +
> +	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT)

So notify.flags has not been set yet? That looks to be done later?

Or is this in case of the user doing

 uargs.action = UNMAP_NOTIFY_SEND_EVENT;
 ioctl(.., IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &uarg);
 uargs.action = UNAMP_NOTIFY_CLEAR_BYTE;
 ioctl(.., IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &uarg);

 and we want to preserve the "old" flags before swapping over to the
new?

> +		evtchn_put(map->notify.event);
> +
>  	map->notify.flags = op.action;
>  	map->notify.addr = op.index - (map->index << PAGE_SHIFT);
>  	map->notify.event = op.event_channel_port;
> -- 
> 1.7.6.4

Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Daniel De Graaf]

On 10/24/2011 04:57 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Oct 18, 2011 at 05:04:11PM -0400, Daniel De Graaf wrote:
>> When using the unmap notify ioctl, the event channel used for
>> notification needs to be reserved to avoid it being deallocated prior to
>> sending the notification.
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> ---
>>  drivers/xen/gntalloc.c |   14 +++++++++++++-
>>  drivers/xen/gntdev.c   |   11 +++++++++++
>>  2 files changed, 24 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
>> index f6832f4..a739fb1 100644
>> --- a/drivers/xen/gntalloc.c
>> +++ b/drivers/xen/gntalloc.c
>> @@ -178,8 +178,10 @@ static void __del_gref(struct gntalloc_gref *gref)
>>  		tmp[gref->notify.pgoff] = 0;
>>  		kunmap(gref->page);
>>  	}
>> -	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
>> +	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
>>  		notify_remote_via_evtchn(gref->notify.event);
>> +		evtchn_put(gref->notify.event);
> 
> So.. I could have some fun by doing this in the userspace:
>   for (j = 0; j< 2;j++) {
>       for (i = 0; i < 65534; i++) {
>          struct ioctl_gntalloc_unmap_notify uarg = {
>                  .index = arg.index + offsetof(struct shr_page, notifies[0]),
>                  .action =UNMAP_NOTIFY_SEND_EVENT,
> 		 .event_channel_port = i,
>          };
>          rv = ioctl(a_fd, IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &uarg);
> 	}
>   }
> 
> And cause the event channel refcnt to be set to zero and free it. And then
> causing the box to die - as the event channels for the physical IRQ might have
> gotten free-ed.
> 

Not really. For a given valid event channel E, this will increase the refcnt by one
when i == E, and then decrease refcnt the next time evtchn_get succeeds (for some
other value of i).

> Hm.. Perhaps the gntalloc and gntdev should keep track of which event channels
> are OK to refcnt? Something like a whitelist? Granted at that point the refcounting
> could as well be done by the API that sets up the event channels from the userspace.

Hmm. Perhaps have a magic value for refcount (-1?) that indicates evtchn_get is not
available. That would become the default value of refcnt, and evtchn.c would then
use evtchn_make_refcounted() to change the refcount to 1 and allow _get/_put to work.

> So the evtchn_ioctl is pretty smart. It uses "get_port_user" to get the list
> of events that belong to this user (and have been handed out). I think you
> need to use that in the gntalloc to double-check that the event channel is not
> one of the kernel type.
> 
>> +	}
>>  
>>  	gref->notify.flags = 0;
>>  
>> @@ -396,6 +398,16 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
>>  		goto unlock_out;
>>  	}
>>  
>> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
>> +		if (evtchn_get(op.event_channel_port)) {
>> +			rc = -EINVAL;
>> +			goto unlock_out;
>> +		}
>> +	}
>> +
>> +	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
>> +		evtchn_put(gref->notify.event);
>> +
>>  	gref->notify.flags = op.action;
>>  	gref->notify.pgoff = pgoff;
>>  	gref->notify.event = op.event_channel_port;
>> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
>> index f914b26..cfcc890 100644
>> --- a/drivers/xen/gntdev.c
>> +++ b/drivers/xen/gntdev.c
>> @@ -190,6 +190,7 @@ static void gntdev_put_map(struct grant_map *map)
>>  
>>  	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
>>  		notify_remote_via_evtchn(map->notify.event);
>> +		evtchn_put(map->notify.event);
>>  	}
>>  
>>  	if (map->pages) {
>> @@ -596,6 +597,16 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
>>  		goto unlock_out;
>>  	}
>>  
>> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
>> +		if (evtchn_get(op.event_channel_port)) {
>> +			rc = -EINVAL;
>> +			goto unlock_out;
>> +		}
>> +	}
>> +
>> +	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
> 
> So notify.flags has not been set yet? That looks to be done later?

Yep. It's the previous value (zero if we haven't called the ioctl yet).

> Or is this in case of the user doing
> 
>  uargs.action = UNMAP_NOTIFY_SEND_EVENT;
>  ioctl(.., IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &uarg);
>  uargs.action = UNAMP_NOTIFY_CLEAR_BYTE;
>  ioctl(.., IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &uarg);
> 
>  and we want to preserve the "old" flags before swapping over to the
> new?

No. We acquire the new event channel before releasing the old one so that
if we happen to be the only one holding a reference to this event channel,
a change in the byte-clear portion of the notify does not cause us to drop
the event channel.

>> +		evtchn_put(map->notify.event);
>> +
>>  	map->notify.flags = op.action;
>>  	map->notify.addr = op.index - (map->index << PAGE_SHIFT);
>>  	map->notify.event = op.event_channel_port;
>> -- 
>> 1.7.6.4
> 


-- 
Daniel De Graaf
National Security Agency

Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Konrad Rzeszutek Wilk]

> > And cause the event channel refcnt to be set to zero and free it. And then
> > causing the box to die - as the event channels for the physical IRQ might have
> > gotten free-ed.
> > 
> 
> Not really. For a given valid event channel E, this will increase the refcnt by one
> when i == E, and then decrease refcnt the next time evtchn_get succeeds (for some
> other value of i).

Oh right. Hmm.. I am having this feeling that it still makes sense to seperate the
events that are allocated by grantdev/grantalloc from the ones that are done
for in-kernel uses (such as IRQ, MSI, IPI, etc). Basically not trusting the userland
with its arguments as much as possible.

And yes, I do understand that you need to be a root user to use /dev/gnt*, but
I started thinking about QEMU. And Fedora has this concept of making QEMU run in its
own SELinux container (and own user) - or perhaps I am confusing this with containers..
Anyhow it runs in one of those quasi-root-but-not-root. My thinking is that it could
be possible do with QEMU running under Xen too, but then we have to make sure
that all /dev/gnt* ioctls are secure <hand-waving what secure means>.

It probably involves more than just what we discussed.
> 
> > Hm.. Perhaps the gntalloc and gntdev should keep track of which event channels
> > are OK to refcnt? Something like a whitelist? Granted at that point the refcounting
> > could as well be done by the API that sets up the event channels from the userspace.
> 
> Hmm. Perhaps have a magic value for refcount (-1?) that indicates evtchn_get is not
> available. That would become the default value of refcnt, and evtchn.c would then
> use evtchn_make_refcounted() to change the refcount to 1 and allow _get/_put to work.

How would that work when the IRQ subsystem (so everything is setup in the kernel)
gets an event? Would the refcount be for that -1.. oh. You would only set
the refcnt when the _get/_put calls are made and not when in-kernel calls to setup
IRQs are done?


> 
> > So the evtchn_ioctl is pretty smart. It uses "get_port_user" to get the list
> > of events that belong to this user (and have been handed out). I think you
> > need to use that in the gntalloc to double-check that the event channel is not
> > one of the kernel type.
> > 
> >> +	}
> >>  
> >>  	gref->notify.flags = 0;
> >>  
> >> @@ -396,6 +398,16 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
> >>  		goto unlock_out;
> >>  	}
> >>  
> >> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
> >> +		if (evtchn_get(op.event_channel_port)) {
> >> +			rc = -EINVAL;
> >> +			goto unlock_out;
> >> +		}
> >> +	}
> >> +
> >> +	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
> >> +		evtchn_put(gref->notify.event);
> >> +
> >>  	gref->notify.flags = op.action;
> >>  	gref->notify.pgoff = pgoff;
> >>  	gref->notify.event = op.event_channel_port;
> >> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> >> index f914b26..cfcc890 100644
> >> --- a/drivers/xen/gntdev.c
> >> +++ b/drivers/xen/gntdev.c
> >> @@ -190,6 +190,7 @@ static void gntdev_put_map(struct grant_map *map)
> >>  
> >>  	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
> >>  		notify_remote_via_evtchn(map->notify.event);
> >> +		evtchn_put(map->notify.event);
> >>  	}
> >>  
> >>  	if (map->pages) {
> >> @@ -596,6 +597,16 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
> >>  		goto unlock_out;
> >>  	}
> >>  
> >> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
> >> +		if (evtchn_get(op.event_channel_port)) {
> >> +			rc = -EINVAL;
> >> +			goto unlock_out;
> >> +		}
> >> +	}
> >> +
> >> +	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
> > 
> > So notify.flags has not been set yet? That looks to be done later?
> 
> Yep. It's the previous value (zero if we haven't called the ioctl yet).

OK, can you add a tiny comment so that in a year time the person reading this
will have a warm fuzzy feeling..
> 
> > Or is this in case of the user doing
> > 
> >  uargs.action = UNMAP_NOTIFY_SEND_EVENT;
> >  ioctl(.., IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &uarg);
> >  uargs.action = UNAMP_NOTIFY_CLEAR_BYTE;
> >  ioctl(.., IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &uarg);
> > 
> >  and we want to preserve the "old" flags before swapping over to the
> > new?
> 
> No. We acquire the new event channel before releasing the old one so that
> if we happen to be the only one holding a reference to this event channel,
> a change in the byte-clear portion of the notify does not cause us to drop
> the event channel.

Ok.
> 
> >> +		evtchn_put(map->notify.event);
> >> +
> >>  	map->notify.flags = op.action;
> >>  	map->notify.addr = op.index - (map->index << PAGE_SHIFT);
> >>  	map->notify.event = op.event_channel_port;
> >> -- 
> >> 1.7.6.4
> > 
> 
> 
> -- 
> Daniel De Graaf
> National Security Agency

Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Daniel De Graaf]

On 10/25/2011 03:02 PM, Konrad Rzeszutek Wilk wrote:
>>> And cause the event channel refcnt to be set to zero and free it. And then
>>> causing the box to die - as the event channels for the physical IRQ might have
>>> gotten free-ed.
>>>
>>
>> Not really. For a given valid event channel E, this will increase the refcnt by one
>> when i == E, and then decrease refcnt the next time evtchn_get succeeds (for some
>> other value of i).
> 
> Oh right. Hmm.. I am having this feeling that it still makes sense to seperate the
> events that are allocated by grantdev/grantalloc from the ones that are done
> for in-kernel uses (such as IRQ, MSI, IPI, etc). Basically not trusting the userland
> with its arguments as much as possible.
> 
> And yes, I do understand that you need to be a root user to use /dev/gnt*, but
> I started thinking about QEMU. And Fedora has this concept of making QEMU run in its
> own SELinux container (and own user) - or perhaps I am confusing this with containers..
> Anyhow it runs in one of those quasi-root-but-not-root. My thinking is that it could
> be possible do with QEMU running under Xen too, but then we have to make sure
> that all /dev/gnt* ioctls are secure <hand-waving what secure means>.
> 
> It probably involves more than just what we discussed.

That same SELinux category-based isolation mechanism is also a good solution for xen
qemu-dm processes, although moving qemu to a stubdom provides better isolation since
SELinux currently cannot talk to XSM to determine what domains a particular qemu-dm 
process should be able to manipulate.

Only allowing event channels allocated by userspace to be used in gnt* notify is
a good idea, since there's no reason for userspace to need to manipulate an event
channel set up by the kernel.

>>
>>> Hm.. Perhaps the gntalloc and gntdev should keep track of which event channels
>>> are OK to refcnt? Something like a whitelist? Granted at that point the refcounting
>>> could as well be done by the API that sets up the event channels from the userspace.
>>
>> Hmm. Perhaps have a magic value for refcount (-1?) that indicates evtchn_get is not
>> available. That would become the default value of refcnt, and evtchn.c would then
>> use evtchn_make_refcounted() to change the refcount to 1 and allow _get/_put to work.
> 
> How would that work when the IRQ subsystem (so everything is setup in the kernel)
> gets an event? Would the refcount be for that -1.. oh. You would only set
> the refcnt when the _get/_put calls are made and not when in-kernel calls to setup
> IRQs are done?
> 

Right. The reference count would be a dual-purpose field indicating if the event
channel is kernel-internal (value -1) or userspace-visible (reference count > 0).
New event channels would start out at -1, and evtchn.c would change them to 1.

>>
>>> So the evtchn_ioctl is pretty smart. It uses "get_port_user" to get the list
>>> of events that belong to this user (and have been handed out). I think you
>>> need to use that in the gntalloc to double-check that the event channel is not
>>> one of the kernel type.
>>>
>>>> +	}
>>>>  
>>>>  	gref->notify.flags = 0;
>>>>  
>>>> @@ -396,6 +398,16 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
>>>>  		goto unlock_out;
>>>>  	}
>>>>  
>>>> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
>>>> +		if (evtchn_get(op.event_channel_port)) {
>>>> +			rc = -EINVAL;
>>>> +			goto unlock_out;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
>>>> +		evtchn_put(gref->notify.event);
>>>> +
>>>>  	gref->notify.flags = op.action;
>>>>  	gref->notify.pgoff = pgoff;
>>>>  	gref->notify.event = op.event_channel_port;
>>>> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
>>>> index f914b26..cfcc890 100644
>>>> --- a/drivers/xen/gntdev.c
>>>> +++ b/drivers/xen/gntdev.c
>>>> @@ -190,6 +190,7 @@ static void gntdev_put_map(struct grant_map *map)
>>>>  
>>>>  	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
>>>>  		notify_remote_via_evtchn(map->notify.event);
>>>> +		evtchn_put(map->notify.event);
>>>>  	}
>>>>  
>>>>  	if (map->pages) {
>>>> @@ -596,6 +597,16 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
>>>>  		goto unlock_out;
>>>>  	}
>>>>  
>>>> +	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
>>>> +		if (evtchn_get(op.event_channel_port)) {
>>>> +			rc = -EINVAL;
>>>> +			goto unlock_out;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
>>>
>>> So notify.flags has not been set yet? That looks to be done later?
>>
>> Yep. It's the previous value (zero if we haven't called the ioctl yet).
> 
> OK, can you add a tiny comment so that in a year time the person reading this
> will have a warm fuzzy feeling..

OK

>>
>>> Or is this in case of the user doing
>>>
>>>  uargs.action = UNMAP_NOTIFY_SEND_EVENT;
>>>  ioctl(.., IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &uarg);
>>>  uargs.action = UNAMP_NOTIFY_CLEAR_BYTE;
>>>  ioctl(.., IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &uarg);
>>>
>>>  and we want to preserve the "old" flags before swapping over to the
>>> new?
>>
>> No. We acquire the new event channel before releasing the old one so that
>> if we happen to be the only one holding a reference to this event channel,
>> a change in the byte-clear portion of the notify does not cause us to drop
>> the event channel.
> 
> Ok.
>>
>>>> +		evtchn_put(map->notify.event);
>>>> +
>>>>  	map->notify.flags = op.action;
>>>>  	map->notify.addr = op.index - (map->index << PAGE_SHIFT);
>>>>  	map->notify.event = op.event_channel_port;
>>>> -- 
>>>> 1.7.6.4

Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Konrad Rzeszutek Wilk]

> That same SELinux category-based isolation mechanism is also a good solution for xen
> qemu-dm processes, although moving qemu to a stubdom provides better isolation since
> SELinux currently cannot talk to XSM to determine what domains a particular qemu-dm 
> process should be able to manipulate.

<nods>
> 
> Only allowing event channels allocated by userspace to be used in gnt* notify is
> a good idea, since there's no reason for userspace to need to manipulate an event
> channel set up by the kernel.
> 
.. snip..
> > 
> > How would that work when the IRQ subsystem (so everything is setup in the kernel)
> > gets an event? Would the refcount be for that -1.. oh. You would only set
> > the refcnt when the _get/_put calls are made and not when in-kernel calls to setup
> > IRQs are done?
> > 
> 
> Right. The reference count would be a dual-purpose field indicating if the event
> channel is kernel-internal (value -1) or userspace-visible (reference count > 0).
> New event channels would start out at -1, and evtchn.c would change them to 1.

The tricky bit is going to be with the xen_free_irq which might have to deal with kernel
events and grantdev events... oh wait, the event_put is going to decrement it and then
call xen_free_irq, so that will come out to be the right number.

Looking forward to the patches! Thanks for doing this work.

Re: Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Ian Campbell]

On Tue, 2011-10-25 at 20:41 +0100, Daniel De Graaf wrote:
> 
> >> Hmm. Perhaps have a magic value for refcount (-1?) that indicates
> evtchn_get is not
> >> available. That would become the default value of refcnt, and
> evtchn.c would then
> >> use evtchn_make_refcounted() to change the refcount to 1 and allow
> _get/_put to work.
> > 
> > How would that work when the IRQ subsystem (so everything is setup
> in the kernel)
> > gets an event? Would the refcount be for that -1.. oh. You would
> only set
> > the refcnt when the _get/_put calls are made and not when in-kernel
> calls to setup> IRQs are done?
> > 
> 
> Right. The reference count would be a dual-purpose field indicating if
> the event channel is kernel-internal (value -1) or userspace-visible
> (reference count > 0). New event channels would start out at -1, and
> evtchn.c would change them to 1. 

Is there any way that the reference count could be made part of the
datastructures associated with the /dev/xen/evtchn driver instead of the
core evtchn.c stuff? That wouldreduce the chance of current or futures
users getting something wrong.

Ian.

Re: Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Daniel De Graaf]

On 10/25/2011 04:27 PM, Ian Campbell wrote:
> On Tue, 2011-10-25 at 20:41 +0100, Daniel De Graaf wrote:
>>
>>>> Hmm. Perhaps have a magic value for refcount (-1?) that indicates
>> evtchn_get is not
>>>> available. That would become the default value of refcnt, and
>> evtchn.c would then
>>>> use evtchn_make_refcounted() to change the refcount to 1 and allow
>> _get/_put to work.
>>>
>>> How would that work when the IRQ subsystem (so everything is setup
>> in the kernel)
>>> gets an event? Would the refcount be for that -1.. oh. You would
>> only set
>>> the refcnt when the _get/_put calls are made and not when in-kernel
>> calls to setup> IRQs are done?
>>>
>>
>> Right. The reference count would be a dual-purpose field indicating if
>> the event channel is kernel-internal (value -1) or userspace-visible
>> (reference count > 0). New event channels would start out at -1, and
>> evtchn.c would change them to 1. 
> 
> Is there any way that the reference count could be made part of the
> datastructures associated with the /dev/xen/evtchn driver instead of the
> core evtchn.c stuff? That wouldreduce the chance of current or futures
> users getting something wrong.
> 
> Ian.

This would require that the gntdev and gntalloc modules have a dependency
on the evtchn module, with the evtchn_{get,put} functions moved into that
module. The only other way would be to add a refcount maintenance
function pointer to the IRQ data structure, which seems like it would lead
to more problems than a simple reference count.

-- 
Daniel De Graaf
National Security Agency

Re: Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Ian Campbell]

On Tue, 2011-10-25 at 21:42 +0100, Daniel De Graaf wrote:
> On 10/25/2011 04:27 PM, Ian Campbell wrote:
> > On Tue, 2011-10-25 at 20:41 +0100, Daniel De Graaf wrote:
> >>
> >>>> Hmm. Perhaps have a magic value for refcount (-1?) that indicates
> >> evtchn_get is not
> >>>> available. That would become the default value of refcnt, and
> >> evtchn.c would then
> >>>> use evtchn_make_refcounted() to change the refcount to 1 and allow
> >> _get/_put to work.
> >>>
> >>> How would that work when the IRQ subsystem (so everything is setup
> >> in the kernel)
> >>> gets an event? Would the refcount be for that -1.. oh. You would
> >> only set
> >>> the refcnt when the _get/_put calls are made and not when in-kernel
> >> calls to setup> IRQs are done?
> >>>
> >>
> >> Right. The reference count would be a dual-purpose field indicating if
> >> the event channel is kernel-internal (value -1) or userspace-visible
> >> (reference count > 0). New event channels would start out at -1, and
> >> evtchn.c would change them to 1. 
> > 
> > Is there any way that the reference count could be made part of the
> > datastructures associated with the /dev/xen/evtchn driver instead of the
> > core evtchn.c stuff? That wouldreduce the chance of current or futures
> > users getting something wrong.
> > 
> > Ian.
> 
> This would require that the gntdev and gntalloc modules have a dependency
> on the evtchn module, with the evtchn_{get,put} functions moved into that
> module.

Don't they effectively have that already, since the only evtchns you can
use with them have come from that module?

>  The only other way would be to add a refcount maintenance
> function pointer to the IRQ data structure, which seems like it would lead
> to more problems than a simple reference count.
> 

Re: Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Daniel De Graaf]

On 10/25/2011 04:50 PM, Ian Campbell wrote:
> On Tue, 2011-10-25 at 21:42 +0100, Daniel De Graaf wrote:
>> On 10/25/2011 04:27 PM, Ian Campbell wrote:
>>> On Tue, 2011-10-25 at 20:41 +0100, Daniel De Graaf wrote:
>>>>
>>>>>> Hmm. Perhaps have a magic value for refcount (-1?) that indicates
>>>> evtchn_get is not
>>>>>> available. That would become the default value of refcnt, and
>>>> evtchn.c would then
>>>>>> use evtchn_make_refcounted() to change the refcount to 1 and allow
>>>> _get/_put to work.
>>>>>
>>>>> How would that work when the IRQ subsystem (so everything is setup
>>>> in the kernel)
>>>>> gets an event? Would the refcount be for that -1.. oh. You would
>>>> only set
>>>>> the refcnt when the _get/_put calls are made and not when in-kernel
>>>> calls to setup> IRQs are done?
>>>>>
>>>>
>>>> Right. The reference count would be a dual-purpose field indicating if
>>>> the event channel is kernel-internal (value -1) or userspace-visible
>>>> (reference count > 0). New event channels would start out at -1, and
>>>> evtchn.c would change them to 1. 
>>>
>>> Is there any way that the reference count could be made part of the
>>> datastructures associated with the /dev/xen/evtchn driver instead of the
>>> core evtchn.c stuff? That wouldreduce the chance of current or futures
>>> users getting something wrong.
>>>
>>> Ian.
>>
>> This would require that the gntdev and gntalloc modules have a dependency
>> on the evtchn module, with the evtchn_{get,put} functions moved into that
>> module.
> 
> Don't they effectively have that already, since the only evtchns you can
> use with them have come from that module?
> 

Yes, but the primary purpose of the gntdev/gntalloc modules does not include
using event channels (just mapping memory). In practice, however, this is
true as shared-memory rings need a notification mechanism if you want to avoid
polling.

>>  The only other way would be to add a refcount maintenance
>> function pointer to the IRQ data structure, which seems like it would lead
>> to more problems than a simple reference count.
>>

[PATCH v3 0/3] Add reference counting to grant notify ioctls

[Daniel De Graaf]

> The current notify ioctls assume that an event channel will not be
> closed prior to the page being unmapped. If the mappings are associated
> with an open file descriptor and the application crashes, the
> notification behavior depends on the close ordering of the file
> descriptors. To avoid this, event channels now have a reference count
> that is used by the grant notify ioctls to postpone the close operation
> until the notification is fired.

Changes since v2:
	Avoid possible sleep under spinlock
	Decrease refcount mismatch errors from BUG to WARN
	Use reference count to identify userspace-visible event channels
 
Changes since v1:
 	Rename evtchn_get/put to match kernel naming conventions
	Use atomic_t for refcount

[PATCH 1/3] xen/event: Add reference counting to event channels
[PATCH 2/3] xen/gntalloc: Change gref_lock to a mutex
[PATCH 3/3] xen/gnt{dev,alloc}: reserve event channels for notify

[PATCH 1/3] xen/event: Add reference counting to event channels

[Daniel De Graaf]

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.

The reference count on new event channels defaults to -1 which indicates
the event channel is not referenced outside the kernel; evtchn_get fails
if called on such an event channel. The event channels made visible to
userspace by evtchn have a normal reference count.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/events.c |   62 +++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/xen/evtchn.c |    2 +-
 include/xen/events.h |    7 +++++
 3 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 073c11d..6054580 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -87,6 +87,7 @@ enum xen_irq_type {
  */
 struct irq_info {
 	struct list_head list;
+	atomic_t refcnt;
 	enum xen_irq_type type;	/* type */
 	unsigned irq;
 	unsigned short evtchn;	/* event channel */
@@ -406,6 +407,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->refcnt, -1);
 
 	irq_set_handler_data(irq, info);
 
@@ -469,6 +471,8 @@ static void xen_free_irq(unsigned irq)
 
 	irq_set_handler_data(irq, NULL);
 
+	WARN_ON(atomic_read(&info->refcnt) > 0);
+
 	kfree(info);
 
 	/* Legacy IRQ descriptors are managed by the arch. */
@@ -637,7 +641,7 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	if (irq != -1) {
 		printk(KERN_INFO "xen_map_pirq_gsi: returning irq %d for gsi %u\n",
 		       irq, gsi);
-		goto out;	/* XXX need refcount? */
+		goto out;
 	}
 
 	irq = xen_allocate_irq_gsi(gsi);
@@ -939,6 +943,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_read(&info->refcnt) > 0 && !atomic_dec_and_test(&info->refcnt))
+		return;
 
 	mutex_lock(&irq_mapping_update_lock);
 
@@ -1065,6 +1073,58 @@ void unbind_from_irqhandler(unsigned int irq, void *dev_id)
 }
 EXPORT_SYMBOL_GPL(unbind_from_irqhandler);
 
+int evtchn_make_refcounted(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;
+
+	WARN_ON(atomic_read(&info->refcnt) != -1);
+
+	atomic_set(&info->refcnt, 1);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(evtchn_make_refcounted);
+
+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;
+
+	if (atomic_read(&info->refcnt) < 0)
+		return -EINVAL;
+
+	atomic_inc(&info->refcnt);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(evtchn_get);
+
+void evtchn_put(unsigned int evtchn)
+{
+	int irq = evtchn_to_irq[evtchn];
+	if (WARN_ON(irq == -1))
+		return;
+	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/drivers/xen/evtchn.c b/drivers/xen/evtchn.c
index dbc13e9..b1f60a0 100644
--- a/drivers/xen/evtchn.c
+++ b/drivers/xen/evtchn.c
@@ -268,7 +268,7 @@ static int evtchn_bind_to_user(struct per_user_data *u, int port)
 	rc = bind_evtchn_to_irqhandler(port, evtchn_interrupt, IRQF_DISABLED,
 				       u->name, (void *)(unsigned long)port);
 	if (rc >= 0)
-		rc = 0;
+		rc = evtchn_make_refcounted(port);
 
 	return rc;
 }
diff --git a/include/xen/events.h b/include/xen/events.h
index d287997..0f77370 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -37,6 +37,13 @@ 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_make_refcounted(unsigned int 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

Re: [PATCH 1/3] xen/event: Add reference counting to event channels

[Ian Campbell]

On Wed, 2011-10-26 at 16:47 +0100, Daniel De Graaf wrote:
> @@ -939,6 +943,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_read(&info->refcnt) > 0 && !atomic_dec_and_test(&info->refcnt))
> +               return;

This isn't all that atomic any more...

evtchn_make_refcounted() doesn't seem to have any locking which would
save you...

Perhaps you could always manipulate this flag under the mapping lock
(which perhaps is normally taken around about the sort of place you'd
want to do this anyway) and make it non-atomic?

Or maybe you could build something with cmpxchg?

Ian.

Re: Re: [PATCH 1/3] xen/event: Add reference counting to event channels

[Ian Campbell]

On Wed, 2011-10-26 at 17:51 +0100, Ian Campbell wrote:
> On Wed, 2011-10-26 at 16:47 +0100, Daniel De Graaf wrote:
> > @@ -939,6 +943,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_read(&info->refcnt) > 0 && !atomic_dec_and_test(&info->refcnt))
> > +               return;
> 
> This isn't all that atomic any more...
> 
> evtchn_make_refcounted() doesn't seem to have any locking which would
> save you...
> 
> Perhaps you could always manipulate this flag under the mapping lock
> (which perhaps is normally taken around about the sort of place you'd
> want to do this anyway) and make it non-atomic?
> 
> Or maybe you could build something with cmpxchg?

Or atomic_inc_unless_negative seems to be a primitive but there's no
atomic_dev_unless_negative so unref is still tricky.

Ian.

Re: [PATCH 1/3] xen/event: Add reference counting to event channels

[Daniel De Graaf]

On 10/26/2011 12:51 PM, Ian Campbell wrote:
> On Wed, 2011-10-26 at 16:47 +0100, Daniel De Graaf wrote:
>> @@ -939,6 +943,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_read(&info->refcnt) > 0 && !atomic_dec_and_test(&info->refcnt))
>> +               return;
> 
> This isn't all that atomic any more...
> 
> evtchn_make_refcounted() doesn't seem to have any locking which would
> save you...
> 
> Perhaps you could always manipulate this flag under the mapping lock
> (which perhaps is normally taken around about the sort of place you'd
> want to do this anyway) and make it non-atomic?
> 
> Or maybe you could build something with cmpxchg?
> 
> Ian.
> 

It's atomic for the cases where it needs to be. There are two cases in which
unbind_from_irq can be called:

1. Negative refcnt (to be exact, == -1). This is an internal reference, and
unbind_from_irq is only called once as guaranteed by the caller.

2. Positive refcnt. Once refcnt has been changed to positive, it cannot be
changed back, so "atomic_read(&info->refcnt) > 0" will always be true, for
any number of parallel callers, assuming all callers had a reference to begin
with. In this case, only the atomic_dec_and_test needs atomicity to resolve
the race when parallel callers are running evtchn_put.

The conversion from -1 to positive refcnt is done during the creation of the
event channel, before multiple references to the event channel are allowed.
There is also no way to convert an event channel from positive to negative
(without destroying it completely) so a race from that conversion is also
not possible.

I considered using atomic_dec_if_positive here, but as the existing condition
has no races this is unnecessary.

Should this explanation be copied into a comment to avoid future confusion?

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH 1/3] xen/event: Add reference counting to event channels

[Ian Campbell]

On Wed, 2011-10-26 at 18:28 +0100, Daniel De Graaf wrote:

> It's atomic for the cases where it needs to be. There are two cases in which
> unbind_from_irq can be called:
[...snip...]
> Should this explanation be copied into a comment to avoid future confusion?

I would have said yes but it looks like you found a different problem
and reposted a non-atomic_t/locked version, right?

Ian.

[PATCH 1/3 v3.2] xen/event: Add reference counting to event channels

[Daniel De Graaf]

The previous version had a race where running evtchn_get in parallel
with an event channel's destruction could attempt to manipulate an event
channel that is in the process of being freed. Using the 
irq_mapping_update_lock mutex instead of an atomic refcnt prevents this.

------------------------------------------------------>8

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.

The reference count on new event channels defaults to -1 which indicates
the event channel is not referenced outside the kernel; evtchn_get fails
if called on such an event channel. The event channels made visible to
userspace by evtchn have a normal reference count.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/events.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/xen/evtchn.c |    2 +-
 include/xen/events.h |    7 +++++
 3 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 073c11d..0237629 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -87,6 +87,7 @@ enum xen_irq_type {
  */
 struct irq_info {
 	struct list_head list;
+	int refcnt;
 	enum xen_irq_type type;	/* type */
 	unsigned irq;
 	unsigned short evtchn;	/* event channel */
@@ -406,6 +407,7 @@ static void xen_irq_init(unsigned irq)
 		panic("Unable to allocate metadata for IRQ%d\n", irq);
 
 	info->type = IRQT_UNBOUND;
+	info->refcnt = -1;
 
 	irq_set_handler_data(irq, info);
 
@@ -469,6 +471,8 @@ static void xen_free_irq(unsigned irq)
 
 	irq_set_handler_data(irq, NULL);
 
+	WARN_ON(info->refcnt > 0);
+
 	kfree(info);
 
 	/* Legacy IRQ descriptors are managed by the arch. */
@@ -637,7 +641,7 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	if (irq != -1) {
 		printk(KERN_INFO "xen_map_pirq_gsi: returning irq %d for gsi %u\n",
 		       irq, gsi);
-		goto out;	/* XXX need refcount? */
+		goto out;
 	}
 
 	irq = xen_allocate_irq_gsi(gsi);
@@ -939,9 +943,16 @@ 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);
 
 	mutex_lock(&irq_mapping_update_lock);
 
+	if (info->refcnt > 0) {
+		info->refcnt--;
+		if (info->refcnt != 0)
+			goto done;
+	}
+
 	if (VALID_EVTCHN(evtchn)) {
 		close.port = evtchn;
 		if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
@@ -970,6 +981,7 @@ static void unbind_from_irq(unsigned int irq)
 
 	xen_free_irq(irq);
 
+ done:
 	mutex_unlock(&irq_mapping_update_lock);
 }
 
@@ -1065,6 +1077,66 @@ void unbind_from_irqhandler(unsigned int irq, void *dev_id)
 }
 EXPORT_SYMBOL_GPL(unbind_from_irqhandler);
 
+int evtchn_make_refcounted(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;
+
+	WARN_ON(info->refcnt != -1);
+
+	info->refcnt = 1;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(evtchn_make_refcounted);
+
+int evtchn_get(unsigned int evtchn)
+{
+	int irq;
+	struct irq_info *info;
+	int err = -ENOENT;
+
+	mutex_lock(&irq_mapping_update_lock);
+
+	irq = evtchn_to_irq[evtchn];
+	if (irq == -1)
+		goto done;
+
+	info = irq_get_handler_data(irq);
+
+	if (!info)
+		goto done;
+
+	err = -EINVAL;
+	if (info->refcnt <= 0)
+		goto done;
+
+	info->refcnt++;
+	err = 0;
+ done:
+	mutex_unlock(&irq_mapping_update_lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(evtchn_get);
+
+void evtchn_put(unsigned int evtchn)
+{
+	int irq = evtchn_to_irq[evtchn];
+	if (WARN_ON(irq == -1))
+		return;
+	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/drivers/xen/evtchn.c b/drivers/xen/evtchn.c
index dbc13e9..b1f60a0 100644
--- a/drivers/xen/evtchn.c
+++ b/drivers/xen/evtchn.c
@@ -268,7 +268,7 @@ static int evtchn_bind_to_user(struct per_user_data *u, int port)
 	rc = bind_evtchn_to_irqhandler(port, evtchn_interrupt, IRQF_DISABLED,
 				       u->name, (void *)(unsigned long)port);
 	if (rc >= 0)
-		rc = 0;
+		rc = evtchn_make_refcounted(port);
 
 	return rc;
 }
diff --git a/include/xen/events.h b/include/xen/events.h
index d287997..0f77370 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -37,6 +37,13 @@ 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_make_refcounted(unsigned int 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

[PATCH 2/3] xen/gntalloc: Change gref_lock to a mutex

[Daniel De Graaf]

The event channel release function cannot be called under a spinlock
because it can attempt to acquire a mutex due to the event channel
reference acquired when setting up unmap notifications.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/gntalloc.c |   41 +++++++++++++++++++++--------------------
 1 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index f6832f4..439352d 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -74,7 +74,7 @@ MODULE_PARM_DESC(limit, "Maximum number of grants that may be allocated by "
 		"the gntalloc device");
 
 static LIST_HEAD(gref_list);
-static DEFINE_SPINLOCK(gref_lock);
+static DEFINE_MUTEX(gref_mutex);
 static int gref_size;
 
 struct notify_info {
@@ -143,15 +143,15 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
 	}
 
 	/* Add to gref lists. */
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	list_splice_tail(&queue_gref, &gref_list);
 	list_splice_tail(&queue_file, &priv->list);
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 
 	return 0;
 
 undo:
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	gref_size -= (op->count - i);
 
 	list_for_each_entry(gref, &queue_file, next_file) {
@@ -167,7 +167,7 @@ undo:
 	 */
 	if (unlikely(!list_empty(&queue_gref)))
 		list_splice_tail(&queue_gref, &gref_list);
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 	return rc;
 }
 
@@ -251,7 +251,7 @@ static int gntalloc_release(struct inode *inode, struct file *filp)
 
 	pr_debug("%s: priv %p\n", __func__, priv);
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	while (!list_empty(&priv->list)) {
 		gref = list_entry(priv->list.next,
 			struct gntalloc_gref, next_file);
@@ -261,7 +261,7 @@ static int gntalloc_release(struct inode *inode, struct file *filp)
 			__del_gref(gref);
 	}
 	kfree(priv);
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 
 	return 0;
 }
@@ -286,21 +286,21 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
 		goto out;
 	}
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	/* Clean up pages that were at zero (local) users but were still mapped
 	 * by remote domains. Since those pages count towards the limit that we
 	 * are about to enforce, removing them here is a good idea.
 	 */
 	do_cleanup();
 	if (gref_size + op.count > limit) {
-		spin_unlock(&gref_lock);
+		mutex_unlock(&gref_mutex);
 		rc = -ENOSPC;
 		goto out_free;
 	}
 	gref_size += op.count;
 	op.index = priv->index;
 	priv->index += op.count * PAGE_SIZE;
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 
 	rc = add_grefs(&op, gref_ids, priv);
 	if (rc < 0)
@@ -343,7 +343,7 @@ static long gntalloc_ioctl_dealloc(struct gntalloc_file_private_data *priv,
 		goto dealloc_grant_out;
 	}
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	gref = find_grefs(priv, op.index, op.count);
 	if (gref) {
 		/* Remove from the file list only, and decrease reference count.
@@ -363,7 +363,7 @@ static long gntalloc_ioctl_dealloc(struct gntalloc_file_private_data *priv,
 
 	do_cleanup();
 
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 dealloc_grant_out:
 	return rc;
 }
@@ -383,7 +383,7 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
 	index = op.index & ~(PAGE_SIZE - 1);
 	pgoff = op.index & (PAGE_SIZE - 1);
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 
 	gref = find_grefs(priv, index, 1);
 	if (!gref) {
@@ -400,8 +400,9 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
 	gref->notify.pgoff = pgoff;
 	gref->notify.event = op.event_channel_port;
 	rc = 0;
+
  unlock_out:
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 	return rc;
 }
 
@@ -433,9 +434,9 @@ static void gntalloc_vma_open(struct vm_area_struct *vma)
 	if (!gref)
 		return;
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	gref->users++;
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 }
 
 static void gntalloc_vma_close(struct vm_area_struct *vma)
@@ -444,11 +445,11 @@ static void gntalloc_vma_close(struct vm_area_struct *vma)
 	if (!gref)
 		return;
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	gref->users--;
 	if (gref->users == 0)
 		__del_gref(gref);
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 }
 
 static struct vm_operations_struct gntalloc_vmops = {
@@ -471,7 +472,7 @@ static int gntalloc_mmap(struct file *filp, struct vm_area_struct *vma)
 		return -EINVAL;
 	}
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	gref = find_grefs(priv, vma->vm_pgoff << PAGE_SHIFT, count);
 	if (gref == NULL) {
 		rv = -ENOENT;
@@ -499,7 +500,7 @@ static int gntalloc_mmap(struct file *filp, struct vm_area_struct *vma)
 	rv = 0;
 
 out_unlock:
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 	return rv;
 }
 
-- 
1.7.6.4

[PATCH 3/3] xen/gnt{dev, alloc}: reserve event channels for notify

[Daniel De Graaf]

When using the unmap notify ioctl, the event channel used for
notification needs to be reserved to avoid it being deallocated prior to
sending the notification.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/gntalloc.c |   21 ++++++++++++++++++++-
 drivers/xen/gntdev.c   |   32 ++++++++++++++++++++++++++++++--
 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 439352d..c95181f 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -178,8 +178,10 @@ static void __del_gref(struct gntalloc_gref *gref)
 		tmp[gref->notify.pgoff] = 0;
 		kunmap(gref->page);
 	}
-	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
+	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
 		notify_remote_via_evtchn(gref->notify.event);
+		evtchn_put(gref->notify.event);
+	}
 
 	gref->notify.flags = 0;
 
@@ -396,6 +398,23 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
 		goto unlock_out;
 	}
 
+	/* We need to grab a reference to the event channel we are going to use
+	 * to send the notify before releasing the reference we may already have
+	 * (if someone has called this ioctl twice). This is required so that
+	 * it is possible to change the clear_byte part of the notification
+	 * without disturbing the event channel part, which may now be the last
+	 * reference to that event channel.
+	 */
+	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
+		if (evtchn_get(op.event_channel_port)) {
+			rc = -EINVAL;
+			goto unlock_out;
+		}
+	}
+
+	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
+		evtchn_put(gref->notify.event);
+
 	gref->notify.flags = op.action;
 	gref->notify.pgoff = pgoff;
 	gref->notify.event = op.event_channel_port;
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 5227506..f0bb322 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -193,8 +193,10 @@ static void gntdev_put_map(struct grant_map *map)
 
 	atomic_sub(map->count, &pages_mapped);
 
-	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
+	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
 		notify_remote_via_evtchn(map->notify.event);
+		evtchn_put(map->notify.event);
+	}
 
 	if (map->pages) {
 		if (!use_ptemod)
@@ -600,6 +602,8 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
 	struct ioctl_gntdev_unmap_notify op;
 	struct grant_map *map;
 	int rc;
+	int old_flags;
+	unsigned int old_event;
 
 	if (copy_from_user(&op, u, sizeof(op)))
 		return -EFAULT;
@@ -625,10 +629,34 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
 		goto unlock_out;
 	}
 
+	/* We need to grab a reference to the event channel we are going to use
+	 * to send the notify before releasing the reference we may already have
+	 * (if someone has called this ioctl twice). This is required so that
+	 * it is possible to change the clear_byte part of the notification
+	 * without disturbing the event channel part, which may now be the last
+	 * reference to that event channel.
+	 */
+	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
+		if (evtchn_get(op.event_channel_port)) {
+			rc = -EINVAL;
+			goto unlock_out;
+		}
+	}
+
+	old_flags = map->notify.flags;
+	old_event = map->notify.event;
+
 	map->notify.flags = op.action;
 	map->notify.addr = op.index - (map->index << PAGE_SHIFT);
 	map->notify.event = op.event_channel_port;
-	rc = 0;
+	spin_unlock(&priv->lock);
+
+	/* Cannot execute evtchn_put under spinlock */
+	if (old_flags & UNMAP_NOTIFY_SEND_EVENT)
+		evtchn_put(old_event);
+
+	return 0;
+
  unlock_out:
 	spin_unlock(&priv->lock);
 	return rc;
-- 
1.7.6.4

[PATCH v4 0/3] Add reference counting to grant notify ioctls

[Daniel De Graaf]

> The current notify ioctls assume that an event channel will not be
> closed prior to the page being unmapped. If the mappings are associated
> with an open file descriptor and the application crashes, the
> notification behavior depends on the close ordering of the file
> descriptors. To avoid this, event channels now have a reference count
> that is used by the grant notify ioctls to postpone the close operation
> until the notification is fired.

Changes since v3:
	Avoid race between evtchn_get and evtchn_put
	(reference count changed to an integer protected by mutex)

Changes since v2:
	Avoid possible sleep under spinlock
	Decrease refcount mismatch errors from BUG to WARN
	Use reference count to identify userspace-visible event channels
 
Changes since v1:
 	Rename evtchn_get/put to match kernel naming conventions
	Use atomic_t for refcount

[PATCH 1/3] xen/event: Add reference counting to event channels
[PATCH 2/3] xen/gntalloc: Change gref_lock to a mutex
[PATCH 3/3] xen/gnt{dev,alloc}: reserve event channels for notify

[PATCH 1/3] xen/event: Add reference counting to event channels

[Daniel De Graaf]

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.

The reference count on new event channels defaults to -1 which indicates
the event channel is not referenced outside the kernel; evtchn_get fails
if called on such an event channel. The event channels made visible to
userspace by evtchn have a normal reference count.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/events.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/xen/evtchn.c |    2 +-
 include/xen/events.h |    7 +++++
 3 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 073c11d..0237629 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -87,6 +87,7 @@ enum xen_irq_type {
  */
 struct irq_info {
 	struct list_head list;
+	int refcnt;
 	enum xen_irq_type type;	/* type */
 	unsigned irq;
 	unsigned short evtchn;	/* event channel */
@@ -406,6 +407,7 @@ static void xen_irq_init(unsigned irq)
 		panic("Unable to allocate metadata for IRQ%d\n", irq);
 
 	info->type = IRQT_UNBOUND;
+	info->refcnt = -1;
 
 	irq_set_handler_data(irq, info);
 
@@ -469,6 +471,8 @@ static void xen_free_irq(unsigned irq)
 
 	irq_set_handler_data(irq, NULL);
 
+	WARN_ON(info->refcnt > 0);
+
 	kfree(info);
 
 	/* Legacy IRQ descriptors are managed by the arch. */
@@ -637,7 +641,7 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	if (irq != -1) {
 		printk(KERN_INFO "xen_map_pirq_gsi: returning irq %d for gsi %u\n",
 		       irq, gsi);
-		goto out;	/* XXX need refcount? */
+		goto out;
 	}
 
 	irq = xen_allocate_irq_gsi(gsi);
@@ -939,9 +943,16 @@ 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);
 
 	mutex_lock(&irq_mapping_update_lock);
 
+	if (info->refcnt > 0) {
+		info->refcnt--;
+		if (info->refcnt != 0)
+			goto done;
+	}
+
 	if (VALID_EVTCHN(evtchn)) {
 		close.port = evtchn;
 		if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
@@ -970,6 +981,7 @@ static void unbind_from_irq(unsigned int irq)
 
 	xen_free_irq(irq);
 
+ done:
 	mutex_unlock(&irq_mapping_update_lock);
 }
 
@@ -1065,6 +1077,66 @@ void unbind_from_irqhandler(unsigned int irq, void *dev_id)
 }
 EXPORT_SYMBOL_GPL(unbind_from_irqhandler);
 
+int evtchn_make_refcounted(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;
+
+	WARN_ON(info->refcnt != -1);
+
+	info->refcnt = 1;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(evtchn_make_refcounted);
+
+int evtchn_get(unsigned int evtchn)
+{
+	int irq;
+	struct irq_info *info;
+	int err = -ENOENT;
+
+	mutex_lock(&irq_mapping_update_lock);
+
+	irq = evtchn_to_irq[evtchn];
+	if (irq == -1)
+		goto done;
+
+	info = irq_get_handler_data(irq);
+
+	if (!info)
+		goto done;
+
+	err = -EINVAL;
+	if (info->refcnt <= 0)
+		goto done;
+
+	info->refcnt++;
+	err = 0;
+ done:
+	mutex_unlock(&irq_mapping_update_lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(evtchn_get);
+
+void evtchn_put(unsigned int evtchn)
+{
+	int irq = evtchn_to_irq[evtchn];
+	if (WARN_ON(irq == -1))
+		return;
+	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/drivers/xen/evtchn.c b/drivers/xen/evtchn.c
index dbc13e9..b1f60a0 100644
--- a/drivers/xen/evtchn.c
+++ b/drivers/xen/evtchn.c
@@ -268,7 +268,7 @@ static int evtchn_bind_to_user(struct per_user_data *u, int port)
 	rc = bind_evtchn_to_irqhandler(port, evtchn_interrupt, IRQF_DISABLED,
 				       u->name, (void *)(unsigned long)port);
 	if (rc >= 0)
-		rc = 0;
+		rc = evtchn_make_refcounted(port);
 
 	return rc;
 }
diff --git a/include/xen/events.h b/include/xen/events.h
index d287997..0f77370 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -37,6 +37,13 @@ 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_make_refcounted(unsigned int 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

[PATCH 2/3] xen/gntalloc: Change gref_lock to a mutex

[Daniel De Graaf]

The event channel release function cannot be called under a spinlock
because it can attempt to acquire a mutex due to the event channel
reference acquired when setting up unmap notifications.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/gntalloc.c |   41 +++++++++++++++++++++--------------------
 1 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index f6832f4..439352d 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -74,7 +74,7 @@ MODULE_PARM_DESC(limit, "Maximum number of grants that may be allocated by "
 		"the gntalloc device");
 
 static LIST_HEAD(gref_list);
-static DEFINE_SPINLOCK(gref_lock);
+static DEFINE_MUTEX(gref_mutex);
 static int gref_size;
 
 struct notify_info {
@@ -143,15 +143,15 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
 	}
 
 	/* Add to gref lists. */
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	list_splice_tail(&queue_gref, &gref_list);
 	list_splice_tail(&queue_file, &priv->list);
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 
 	return 0;
 
 undo:
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	gref_size -= (op->count - i);
 
 	list_for_each_entry(gref, &queue_file, next_file) {
@@ -167,7 +167,7 @@ undo:
 	 */
 	if (unlikely(!list_empty(&queue_gref)))
 		list_splice_tail(&queue_gref, &gref_list);
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 	return rc;
 }
 
@@ -251,7 +251,7 @@ static int gntalloc_release(struct inode *inode, struct file *filp)
 
 	pr_debug("%s: priv %p\n", __func__, priv);
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	while (!list_empty(&priv->list)) {
 		gref = list_entry(priv->list.next,
 			struct gntalloc_gref, next_file);
@@ -261,7 +261,7 @@ static int gntalloc_release(struct inode *inode, struct file *filp)
 			__del_gref(gref);
 	}
 	kfree(priv);
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 
 	return 0;
 }
@@ -286,21 +286,21 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
 		goto out;
 	}
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	/* Clean up pages that were at zero (local) users but were still mapped
 	 * by remote domains. Since those pages count towards the limit that we
 	 * are about to enforce, removing them here is a good idea.
 	 */
 	do_cleanup();
 	if (gref_size + op.count > limit) {
-		spin_unlock(&gref_lock);
+		mutex_unlock(&gref_mutex);
 		rc = -ENOSPC;
 		goto out_free;
 	}
 	gref_size += op.count;
 	op.index = priv->index;
 	priv->index += op.count * PAGE_SIZE;
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 
 	rc = add_grefs(&op, gref_ids, priv);
 	if (rc < 0)
@@ -343,7 +343,7 @@ static long gntalloc_ioctl_dealloc(struct gntalloc_file_private_data *priv,
 		goto dealloc_grant_out;
 	}
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	gref = find_grefs(priv, op.index, op.count);
 	if (gref) {
 		/* Remove from the file list only, and decrease reference count.
@@ -363,7 +363,7 @@ static long gntalloc_ioctl_dealloc(struct gntalloc_file_private_data *priv,
 
 	do_cleanup();
 
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 dealloc_grant_out:
 	return rc;
 }
@@ -383,7 +383,7 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
 	index = op.index & ~(PAGE_SIZE - 1);
 	pgoff = op.index & (PAGE_SIZE - 1);
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 
 	gref = find_grefs(priv, index, 1);
 	if (!gref) {
@@ -400,8 +400,9 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
 	gref->notify.pgoff = pgoff;
 	gref->notify.event = op.event_channel_port;
 	rc = 0;
+
  unlock_out:
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 	return rc;
 }
 
@@ -433,9 +434,9 @@ static void gntalloc_vma_open(struct vm_area_struct *vma)
 	if (!gref)
 		return;
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	gref->users++;
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 }
 
 static void gntalloc_vma_close(struct vm_area_struct *vma)
@@ -444,11 +445,11 @@ static void gntalloc_vma_close(struct vm_area_struct *vma)
 	if (!gref)
 		return;
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	gref->users--;
 	if (gref->users == 0)
 		__del_gref(gref);
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 }
 
 static struct vm_operations_struct gntalloc_vmops = {
@@ -471,7 +472,7 @@ static int gntalloc_mmap(struct file *filp, struct vm_area_struct *vma)
 		return -EINVAL;
 	}
 
-	spin_lock(&gref_lock);
+	mutex_lock(&gref_mutex);
 	gref = find_grefs(priv, vma->vm_pgoff << PAGE_SHIFT, count);
 	if (gref == NULL) {
 		rv = -ENOENT;
@@ -499,7 +500,7 @@ static int gntalloc_mmap(struct file *filp, struct vm_area_struct *vma)
 	rv = 0;
 
 out_unlock:
-	spin_unlock(&gref_lock);
+	mutex_unlock(&gref_mutex);
 	return rv;
 }
 
-- 
1.7.6.4

[PATCH 3/3] xen/gnt{dev, alloc}: reserve event channels for notify

[Daniel De Graaf]

When using the unmap notify ioctl, the event channel used for
notification needs to be reserved to avoid it being deallocated prior to
sending the notification.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/xen/gntalloc.c |   21 ++++++++++++++++++++-
 drivers/xen/gntdev.c   |   31 ++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 439352d..c95181f 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -178,8 +178,10 @@ static void __del_gref(struct gntalloc_gref *gref)
 		tmp[gref->notify.pgoff] = 0;
 		kunmap(gref->page);
 	}
-	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
+	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
 		notify_remote_via_evtchn(gref->notify.event);
+		evtchn_put(gref->notify.event);
+	}
 
 	gref->notify.flags = 0;
 
@@ -396,6 +398,23 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
 		goto unlock_out;
 	}
 
+	/* We need to grab a reference to the event channel we are going to use
+	 * to send the notify before releasing the reference we may already have
+	 * (if someone has called this ioctl twice). This is required so that
+	 * it is possible to change the clear_byte part of the notification
+	 * without disturbing the event channel part, which may now be the last
+	 * reference to that event channel.
+	 */
+	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
+		if (evtchn_get(op.event_channel_port)) {
+			rc = -EINVAL;
+			goto unlock_out;
+		}
+	}
+
+	if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
+		evtchn_put(gref->notify.event);
+
 	gref->notify.flags = op.action;
 	gref->notify.pgoff = pgoff;
 	gref->notify.event = op.event_channel_port;
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 5227506..f87ce52 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -193,8 +193,10 @@ static void gntdev_put_map(struct grant_map *map)
 
 	atomic_sub(map->count, &pages_mapped);
 
-	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
+	if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
 		notify_remote_via_evtchn(map->notify.event);
+		evtchn_put(map->notify.event);
+	}
 
 	if (map->pages) {
 		if (!use_ptemod)
@@ -600,6 +602,8 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
 	struct ioctl_gntdev_unmap_notify op;
 	struct grant_map *map;
 	int rc;
+	int out_flags;
+	unsigned int out_event;
 
 	if (copy_from_user(&op, u, sizeof(op)))
 		return -EFAULT;
@@ -607,6 +611,21 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
 	if (op.action & ~(UNMAP_NOTIFY_CLEAR_BYTE|UNMAP_NOTIFY_SEND_EVENT))
 		return -EINVAL;
 
+	/* We need to grab a reference to the event channel we are going to use
+	 * to send the notify before releasing the reference we may already have
+	 * (if someone has called this ioctl twice). This is required so that
+	 * it is possible to change the clear_byte part of the notification
+	 * without disturbing the event channel part, which may now be the last
+	 * reference to that event channel.
+	 */
+	if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
+		if (evtchn_get(op.event_channel_port))
+			return -EINVAL;
+	}
+
+	out_flags = op.action;
+	out_event = op.event_channel_port;
+
 	spin_lock(&priv->lock);
 
 	list_for_each_entry(map, &priv->maps, next) {
@@ -625,12 +644,22 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
 		goto unlock_out;
 	}
 
+	out_flags = map->notify.flags;
+	out_event = map->notify.event;
+
 	map->notify.flags = op.action;
 	map->notify.addr = op.index - (map->index << PAGE_SHIFT);
 	map->notify.event = op.event_channel_port;
+
 	rc = 0;
+
  unlock_out:
 	spin_unlock(&priv->lock);
+
+	/* Drop the reference to the event channel we did not save in the map */
+	if (out_flags & UNMAP_NOTIFY_SEND_EVENT)
+		evtchn_put(out_event);
+
 	return rc;
 }
 
-- 
1.7.6.4

Re: [PATCH v4 0/3] Add reference counting to grant notify ioctls

[Konrad Rzeszutek Wilk]

On Thu, Oct 27, 2011 at 05:58:46PM -0400, Daniel De Graaf wrote:
> > The current notify ioctls assume that an event channel will not be
> > closed prior to the page being unmapped. If the mappings are associated
> > with an open file descriptor and the application crashes, the
> > notification behavior depends on the close ordering of the file
> > descriptors. To avoid this, event channels now have a reference count
> > that is used by the grant notify ioctls to postpone the close operation
> > until the notification is fired.

OK, it all looks sensible to me. Sticking on my 3.3 queue.
Thx!
> 
> Changes since v3:
> 	Avoid race between evtchn_get and evtchn_put
> 	(reference count changed to an integer protected by mutex)
> 
> Changes since v2:
> 	Avoid possible sleep under spinlock
> 	Decrease refcount mismatch errors from BUG to WARN
> 	Use reference count to identify userspace-visible event channels
>  
> Changes since v1:
>  	Rename evtchn_get/put to match kernel naming conventions
> 	Use atomic_t for refcount
> 
> [PATCH 1/3] xen/event: Add reference counting to event channels
> [PATCH 2/3] xen/gntalloc: Change gref_lock to a mutex
> [PATCH 3/3] xen/gnt{dev,alloc}: reserve event channels for notify