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

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

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