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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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

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



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