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

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

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