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: Tue, 25 Oct 2011 15:02:31 -0400 Message-ID: <20111025190231.GD10062@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> <20111024205708.GE2441@phenom.dumpdata.com> <4EA5E024.7040708@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <4EA5E024.7040708@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 > > 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). Oh right. Hmm.. I am having this feeling that it still makes sense to seperate the events that are allocated by grantdev/grantalloc from the ones that are done for in-kernel uses (such as IRQ, MSI, IPI, etc). Basically not trusting the userland with its arguments as much as possible. And yes, I do understand that you need to be a root user to use /dev/gnt*, but I started thinking about QEMU. And Fedora has this concept of making QEMU run in its own SELinux container (and own user) - or perhaps I am confusing this with containers.. Anyhow it runs in one of those quasi-root-but-not-root. My thinking is that it could be possible do with QEMU running under Xen too, but then we have to make sure that all /dev/gnt* ioctls are secure . It probably involves more than just what we discussed. > > > 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. How would that work when the IRQ subsystem (so everything is setup in the kernel) gets an event? Would the refcount be for that -1.. oh. You would only set the refcnt when the _get/_put calls are made and not when in-kernel calls to setup IRQs are done? > > > 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). OK, can you add a tiny comment so that in a year time the person reading this will have a warm fuzzy feeling.. > > > 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. Ok. > > >> + 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