Le 19/10/2018 à 04:16, Cortland Setlow Tölva a écrit :
> 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.
OK, it's clearer now. Thank you for the explanation.
Laurent
