From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify Date: Wed, 19 Oct 2011 10:23:55 -0400 Message-ID: <4E9EDD7B.10202@tycho.nsa.gov> References: <1316207684-19860-1-git-send-email-dgdegra@tycho.nsa.gov> <1318971851-12809-1-git-send-email-dgdegra@tycho.nsa.gov> <1318971851-12809-3-git-send-email-dgdegra@tycho.nsa.gov> <1319016278.3385.88.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1319016278.3385.88.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Campbell Cc: "Keir (Xen.org)" , "jeremy@goop.org" , "xen-devel@lists.xensource.com" , Ian Jackson , "konrad.wilk@oracle.com" List-Id: xen-devel@lists.xenproject.org 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 >> --- >> 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; > >