On Thu, Oct 18, 2018 at 11:48 AM Laurent Vivier <laur...@vivier.eu> 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 <c...@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);
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.
