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

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

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

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