Re: [Xen-devel] [PATCHv1 2/2] xen/privcmd: add ioctls for locking/unlocking hypercall buffers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.08.16 at 19:03, <david.vrabel@xxxxxxxxxx> wrote:
> Locked buffers are stored in a rbtree for faster lookup during UNLOCK,
> and stored in a list so all buffers can be unlocked when the file
> descriptor is closed.

Is there no sufficiently easy way to walk an rbtree, so the duplicate
putting on a list could be avoided?

> +static int privcmd_hbuf_compare(struct privcmd_hcall_buf *a,
> +                             struct privcmd_hcall_buf *b)
> +{
> +     if (b->start == a->start)
> +             return b->len - a->len;
> +     return b->start - a->start;

I don't think subtraction can be used for either of these, or truncation
may result on 64-bit arches. Or, since the function only gets called
directly, its return type (and respective variables in callers) would
need to change to long.

> +static void privcmd_hbuf_unlock(struct privcmd_data *priv,
> +                             struct privcmd_hbuf *hbuf)
> +{
> +     unsigned int i;
> +
> +     for (i = 0; i < hbuf->nr_pages; i++)
> +             put_page(hbuf->pages[i]);

Is taking extra references on each lock (and hence dropping them
here on each unlock) really worthwhile, considering that you refcount
the hbuf structures anyway? Taking just a single reference would
not only eliminate no matter how small refcount overflow concerns,
but also ...

> +static void privcmd_hbuf_erase_all(struct privcmd_data *priv)
> +{
> +     while (!list_empty(&priv->hbuf_list)) {
> +             struct privcmd_hbuf *hbuf;
> +
> +             hbuf = list_first_entry(&priv->hbuf_list,
> +                                     struct privcmd_hbuf, release_node);
> +             privcmd_hbuf_unlock(priv, hbuf);
> +     }
> +}

... decrease the iteration count needed here.


> +     case IOCTL_PRIVCMD_HCALL_BUF_UNLOCK:
> +             ret = privcmd_ioctl_hcall_buf_unlock(priv, udata);

Looking at the function's implementation, this yields success even
if there was no matching structure in the tree. Shouldn't the caller
rather be made aware of the bad request?

> --- a/include/uapi/xen/privcmd.h
> +++ b/include/uapi/xen/privcmd.h
> @@ -77,6 +77,11 @@ struct privcmd_mmapbatch_v2 {
>       int __user *err;  /* array of error codes */
>  };
>  
> +struct privcmd_hcall_buf {
> +     void *start;

Even if you don't reref this pointer, shouldn't it be __user nevertheless?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel