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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

> > 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 <hand-waving what secure means>.

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