From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify Date: Mon, 24 Oct 2011 16:57:08 -0400 Message-ID: <20111024205708.GE2441@phenom.dumpdata.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1318971851-12809-3-git-send-email-dgdegra@tycho.nsa.gov> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Daniel De Graaf Cc: Ian.Jackson@eu.citrix.com, jeremy@goop.org, xen-devel@lists.xensource.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org 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 > --- > 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. 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. 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? 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? > + 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