linux-kernel - Re: [PATCH] usbip: add USBIP_URB

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 19 Aug 2022 11:09:50 -0600
From:   Shuah Khan <skhan@...uxfoundation.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     valentina.manea.m@...il.com, shuah@...nel.org,
        mailhol.vincent@...adoo.fr, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Hongren Zenithal Zheng <i@...ithal.me>,
        Alan Stern <stern@...land.harvard.edu>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH] usbip: add USBIP_URB_* URB transfer flags

On 8/19/22 2:29 AM, Greg KH wrote:
> 
> Overall, nice work, thanks for adding this, minor comments below:
> 
> On Tue, Aug 16, 2022 at 03:57:11PM -0600, Shuah Khan wrote:
>> USBIP driver packs URB transfer flags in PDUs that are exchanged
> 
> What is a "PDU"?

Protocol Data Unit - I will expand or reword to drop it.

[snip]

>>   +-----------+--------+---------------------------------------------------+
>>   | 0x14      | 4      | transfer_flags: possible values depend on the     |
>>   |           |        | URB transfer_flags (refer to URB doc in           |
> 
> You can drop the second "URB" here, as it's an USBIP document being
> referred to, right?

This refers to USB URB document. I can rephrase it to USBIP_URB transfer flags
as it is the API now.

>>   
>> +		/* Unpack the pdu to an urb (map USBIP_URB_* to URB_* flags) */
> 
> No need to mention the mapping everywhere in these comments as the
> function handles that.
> 

These comments were primarily added for making sure right mapping is done.
Will remove them.


>> +	}
>> +	pr_debug("%s: flag = %u map_flags = %u\n", __func__,
>> +		 flags, map_flags);
> 
> When using pr_debug (or dev_dbg()), you already have access to the
> __func__ in the output automatically, so there's never a need to add it
> again, it would just show up twice.
> 

Good point.

> Also, this should be dev_dbg(), as it's a driver, and you have access to
> the device being handled (well, it needs to be passed to the
> function...)
> 

I am going to delete these - debug message isn't really needed.

>> +
>> +	return map_flags;
>> +}
>> +
>> +static unsigned int usbip_to_urb(unsigned int flags)
>> +{
>> +	unsigned int map_flags = 0;
>> +	int loop;
>> +
>> +	for (loop = 0; loop < NUM_USBIP_FLAGS; loop++) {
>> +		if (flags & flag_map[loop].usbip_flag)
>> +			map_flags |= flag_map[loop].urb_flag;
>> +	}
>> +	pr_debug("%s: flag = %u map_flags = %u\n", __func__,
>> +		 flags, map_flags);
> 
> Same debug comment as above.

Same as above. I will just delete the message.

> 

>> + * As a result of coying the transfer flags, they now have become part
> 
> "coying"?  :)

Thanks for catching this :)

> 
>> + * of the API. Any changes to kernel flags now impact the USBIP user-space.
>> + * In addition, it will be compatibility problem between server and client.
>> + * These defines aren't directly referenced and it is an obscure usage
>> + * hidden away in the packets.
> 
> No need to mention the history here, just say "here are the flags for
> the transfer types"
> 

Will do.

> 
>> + *
>> + * Add code to translate the values between the numbers used in userspace
>> + * and the numbers used in the kernel. This will help with any future
>> + * changes to kernel flags breaking the API.
> 
> There's no code being added in this uapi file, so you might want to
> reword all of this.  Possibly by removing most of it, as the history
> here might not be necessary to document.
> 

Will do.

thanks for the review. Will send v2.

thanks,
-- Shuah