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

[Cortland Setlow Tölva]

On Thu, Oct 18, 2018 at 11:48 AM Laurent Vivier <address@hidden> wrote:
>
> 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 <address@hidden>
> > ---
> > 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);

g_int64_hash means this GHashTable expects to be given a pointer to an int64_t,
and the int64_t is used as the key.

> > +    }
> > +    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.
>

The key is pointed to by urb. It is the first member of the struct,
target_urb_adr.

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

target_urb_adr is on the stack here, and it contains the key.  Both
g_hash_table_lookup
and g_hash_table_insert take a pointer to the key, which is an int64_t
in this code.

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

My code might have been clearer if urb_hashtable_insert and
urb_hashtable_lookup had the same argument type.  However,
I did not want to treat target_urb_adr as the first element in a
struct live_urb until checking that it was tracked in the hashtable.

The thing we are looking up has to be a target-sized pointer, so I
cannot use the g_direct_hash with host-sized pointers as keys.
The next best option was to use int64_t as the key.

>
> Thanks,
> Laurent

Thanks,
Cortland.