> Il giorno 13 dic 2016, alle ore 05:22, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> ha scritto:
>
>> On Mon, 12 Dec 2016, Mateusz Berezecki wrote:
>>
>> Promote a variable keeping track of USB transfer memory usage to a
>> wider data type and allow for higher bandwidth transfers from a large
>> number of USB devices connected to a single host.
>> ---
>> drivers/usb/core/devio.c | 34 ++++++++++++----------------------
>> 1 file changed, 12 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
>> index 4016dae..bead975 100644
>> --- a/drivers/usb/core/devio.c
>> +++ b/drivers/usb/core/devio.c
>> @@ -134,42 +134,32 @@ enum snoop_when {
>> #define USB_DEVICE_DEV MKDEV(USB_DEVICE_MAJOR, 0)
>>
>> /* Limit on the total amount of memory we can allocate for transfers */
>> -static unsigned usbfs_memory_mb = 16;
>> +static u32 usbfs_memory_mb = 16;
>> module_param(usbfs_memory_mb, uint, 0644);
>> MODULE_PARM_DESC(usbfs_memory_mb,
>> "maximum MB allowed for usbfs buffers (0 = no limit)");
>>
>> -/* Hard limit, necessary to avoid arithmetic overflow */
>> -#define USBFS_XFER_MAX (UINT_MAX / 2 - 1000000)
>> -
>> -static atomic_t usbfs_memory_usage; /* Total memory currently allocated */
>> +static atomic64_t usbfs_memory_usage; /* Total memory currently allocated */
>>
>> /* Check whether it's okay to allocate more memory for a transfer */
>> -static int usbfs_increase_memory_usage(unsigned amount)
>> +static int usbfs_increase_memory_usage(u64 amount)
>> {
>> - unsigned lim;
>> + u64 lim;
>>
>> - /*
>> - * Convert usbfs_memory_mb to bytes, avoiding overflows.
>> - * 0 means use the hard limit (effectively unlimited).
>> - */
>> lim = ACCESS_ONCE(usbfs_memory_mb);
>> - if (lim == 0 || lim > (USBFS_XFER_MAX >> 20))
>> - lim = USBFS_XFER_MAX;
>> - else
>> - lim <<= 20;
>> + lim <<= 20;
>
> You need to handle the case where lim == 0 (see the MODULE_PARM_DESC
> text).
Good catch.
>
>>
>> - atomic_add(amount, &usbfs_memory_usage);
>> - if (atomic_read(&usbfs_memory_usage) <= lim)
>> + atomic64_add(amount, &usbfs_memory_usage);
>> + if (atomic64_read(&usbfs_memory_usage) <= lim)
>> return 0;
>> - atomic_sub(amount, &usbfs_memory_usage);
>> + atomic64_sub(amount, &usbfs_memory_usage);
>> return -ENOMEM;
>> }
>>
>> /* Memory for a transfer is being deallocated */
>> -static void usbfs_decrease_memory_usage(unsigned amount)
>> +static void usbfs_decrease_memory_usage(u64 amount)
>> {
>> - atomic_sub(amount, &usbfs_memory_usage);
>> + atomic64_sub(amount, &usbfs_memory_usage);
>> }
>>
>> static int connected(struct usb_dev_state *ps)
>> @@ -1191,7 +1181,7 @@ static int proc_bulk(struct usb_dev_state *ps, void __user *arg)
>> if (!usb_maxpacket(dev, pipe, !(bulk.ep & USB_DIR_IN)))
>> return -EINVAL;
>> len1 = bulk.len;
>> - if (len1 >= USBFS_XFER_MAX)
>> + if (len1 >= (INT_MAX - sizeof(struct urb)))
>> return -EINVAL;
>> ret = usbfs_increase_memory_usage(len1 + sizeof(struct urb));
>> if (ret)
>> @@ -1584,7 +1574,7 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
>> return -EINVAL;
>> }
>>
>> - if (uurb->buffer_length >= USBFS_XFER_MAX) {
>> + if (uurb->buffer_length >= (INT_MAX - sizeof(struct urb))) {
>> ret = -EINVAL;
>> goto error;
>> }
>
> This isn't right. See how the u variable gets used in this routine.
> You have to guarantee that the computation of u won't overflow.
Ok. Let me revise, test and resubmit in a few hours.
Thanks for the feedback and reviewing!
Mateusz
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
