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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

> > 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel