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

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

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 <dgdegra@tycho.nsa.gov>
> ---
>  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