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: Mon, 24 Oct 2011 18:01:08 -0400 Message-ID: <4EA5E024.7040708@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> <20111024205708.GE2441@phenom.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20111024205708.GE2441@phenom.dumpdata.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: Konrad Rzeszutek Wilk 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 10/24/2011 04:57 PM, Konrad Rzeszutek Wilk wrote: > 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. > Not really. For a given valid event channel E, this will increase the refcnt by one when i == E, and then decrease refcnt the next time evtchn_get succeeds (for some other value of i). > 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. Hmm. Perhaps have a magic value for refcount (-1?) that indicates evtchn_get is not available. That would become the default value of refcnt, and evtchn.c would then use evtchn_make_refcounted() to change the refcount to 1 and allow _get/_put to work. > 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? Yep. It's the previous value (zero if we haven't called the ioctl yet). > 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? No. We acquire the new event channel before releasing the old one so that if we happen to be the only one holding a reference to this event channel, a change in the byte-clear portion of the notify does not cause us to drop the event channel. >> + 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 > -- Daniel De Graaf National Security Agency