Re: [Qemu-devel] [PATCH v3 3/3] linux-user: Implement special usbfs ioctls.

[Laurent Vivier <laurent@vivier.eu>]

Le 08/10/2018 à 18:35, Cortland Tölva a écrit :
> Userspace submits a USB Request Buffer to the kernel, optionally
> discards it, and finally reaps the URB.  Thunk buffers from target
> to host and back.
> 
> Tested by running an i386 scanner driver on ARMv7 and by running
> the PowerPC lsusb utility on x86_64.  The discardurb ioctl is
> not exercised in these tests.
> 
> Signed-off-by: Cortland Tölva <cst@tolva.net>
> ---
> There are two alternatives for the strategy of holding lock_user on
> memory from submit until reap.  v3 of this series tries to determine
> the access permissions for user memory from endpoint direction, but
> the logic for this is complex.  The first alternative is to request
> write access.  If that fails, request read access.  If that fails, try
> to submit the ioctl with no buffer - perhaps the user code filled in
> fields the kernel will ignore.  The second alternative is to read user
> memory into an allocated buffer, pass it to the kernel, and write back
> to target memory only if the kernel indicates that writes occurred.
> 
> Changes from v1:
>   improve pointer cast to int compatibility
>   remove unimplemented types for usb streams
>   struct definitions moved to this patch where possible
> 
> Changes from v2:
>  organize urb thunk metadata in a struct
>  hold lock_user from submit until discard
>  fixes for 64-bit hosts
> 
>  linux-user/ioctls.h        |   8 ++
>  linux-user/syscall.c       | 177 +++++++++++++++++++++++++++++++++++++++++++++
>  linux-user/syscall_defs.h  |   4 +
>  linux-user/syscall_types.h |  20 +++++
>  4 files changed, 209 insertions(+)
> 
> diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
> index 92f6177f1d..ae8951625f 100644
...
> index 2641260186..9b7ea96cfb 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -96,6 +96,7 @@
>  #include <linux/fb.h>
>  #if defined(CONFIG_USBFS)
>  #include <linux/usbdevice_fs.h>
> +#include <linux/usb/ch9.h>
>  #endif
>  #include <linux/vt.h>
>  #include <linux/dm-ioctl.h>
> @@ -4199,6 +4200,182 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
>      return ret;
>  }
>  
> +#if defined(CONFIG_USBFS)
> +#if HOST_LONG_BITS > 64
> +#error USBDEVFS thunks do not support >64 bit hosts yet.
> +#endif
> +struct live_urb {
> +    uint64_t target_urb_adr;
> +    uint64_t target_buf_adr;
> +    char *target_buf_ptr;
> +    struct usbdevfs_urb host_urb;
> +};
> +
> +static GHashTable *usbdevfs_urb_hashtable(void)
> +{
> +    static GHashTable *urb_hashtable;
> +
> +    if (!urb_hashtable) {
> +        urb_hashtable = g_hash_table_new(g_int64_hash, g_int64_equal);
> +    }
> +    return urb_hashtable;
> +}
> +
> +static void urb_hashtable_insert(struct live_urb *urb)
> +{
> +    GHashTable *urb_hashtable = usbdevfs_urb_hashtable();
> +    g_hash_table_insert(urb_hashtable, urb, urb);

Here the key of the hashtable seems to be the pointer to the host live_urb.

> +}
> +
> +static struct live_urb *urb_hashtable_lookup(uint64_t target_urb_adr)
> +{
> +    GHashTable *urb_hashtable = usbdevfs_urb_hashtable();
> +    return g_hash_table_lookup(urb_hashtable, &target_urb_adr);

And here the key is the pointer to the target_urb_adr

So I think urb_hashtable_insert()  should be:

    g_hash_table_insert(urb_hashtable, urb->target_urb_adr, urb);

and urb_hashtable_lookup() should be:

    return g_hash_table_lookup(urb_hashtable, target_urb_adr);
...
Did I miss something?

Thanks,
Laurent