| Message ID | 20220610213335.3077375-2-rhett.aultman@samsara.com |
|---|---|
| State | New |
| Headers | show |
| Series | URB_FREE_COHERENT gs_usb memory leak fix | expand |
On Fri, Jun 10, 2022 at 05:33:35PM -0400, Rhett Aultman wrote: > > In order to have all the flags in numerical order, URB_DIR_IN is > renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse > value 0x0200. > #define URB_FREE_BUFFER 0x0100 /* Free transfer buffer with the URB */ > +#define URB_FREE_COHERENT 0x0200 /* Free DMA memory of transfer buffer */ > > /* The following flags are used internally by usbcore and HCDs */ > -#define URB_DIR_IN 0x0200 /* Transfer from device to host */ > +#define URB_DIR_IN 0x0400 /* Transfer from device to host */ > #define URB_DIR_OUT 0 > #define URB_DIR_MASK URB_DIR_IN > > -- > 2.30.2 > I'm afraid this is a change of uapi as this field is, unfortunately, exported by usbip to userspace as TCP packets. This may also cause incompatibility (surprisingly not for this case, detailed below) between usbip server and client when one kernel is using the new flags and the other one is not. If we do change this, we may need to bump usbip protocol version accordingly. A copy of Alan Stern's suggestion here for reference > I don't see anything wrong with this, except that it would be nice to keep > the flag values in numerical order. In other words, set URB_FREE_COHERENT > to 0x0200 and change URB_DIR_IN to 0x0400. > > Alan Stern In usbip, an URB is packed by client (vhci) into a TCP packet by usbip_pack_cmd_submit, in which transfer_flags is copied almost as-is, except it clears the DMA flag. Then the server (usbip-host) would accept the packet, mask some flags and re-submit_urb to usbcore. In usbip protocol, URB_DIR_IN is not used as it has a `direction' field, the in-kernel implementation (usbip-host) also does not use it as when re-submit_urb the usb_submit_urb will calculate this specific bit again. For FREE_COHERENT, since DMA flag is cleared, it does not affect the protocol if both server and client are using the new version. If we are using vhci in newer kernel and the other side is using the old version, the USB_FREE_COHERENT flag would be 0x0200, and will be transmitted via TCP/IP to usbip-host or a userspace implementation (there are many of them), which will perceive it as URB_DIR_IN. usbip-host is not affected, but userspace program might be affected. If we are using vhci in old kernel, and the other side is using the new version, all the URB_DIR_IN would be conceived by usbip-host as USB_FREE_COHERENT. Fortunately, it will be masked by masking_bogus_flags in stub_rx.c, and usbip-host will behave correctly, but userspace program might be affected.
On 6/23/22 11:30 AM, Hongren Zenithal Zheng wrote: > On Fri, Jun 10, 2022 at 05:33:35PM -0400, Rhett Aultman wrote: >> >> In order to have all the flags in numerical order, URB_DIR_IN is >> renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse >> value 0x0200. > >> #define URB_FREE_BUFFER 0x0100 /* Free transfer buffer with the URB */ >> +#define URB_FREE_COHERENT 0x0200 /* Free DMA memory of transfer buffer */ >> >> /* The following flags are used internally by usbcore and HCDs */ >> -#define URB_DIR_IN 0x0200 /* Transfer from device to host */ >> +#define URB_DIR_IN 0x0400 /* Transfer from device to host */ >> #define URB_DIR_OUT 0 >> #define URB_DIR_MASK URB_DIR_IN >> >> -- >> 2.30.2 >> > > I'm afraid this is a change of uapi as this field is, unfortunately, > exported by usbip to userspace as TCP packets. > > This may also cause incompatibility (surprisingly not for this case, > detailed below) between usbip server and client > when one kernel is using the new flags and the other one is not. > > If we do change this, we may need to bump usbip protocol version > accordingly. > > A copy of Alan Stern's suggestion here for reference >> I don't see anything wrong with this, except that it would be nice to keep >> the flag values in numerical order. In other words, set URB_FREE_COHERENT >> to 0x0200 and change URB_DIR_IN to 0x0400. >> >> Alan Stern Thank you Alan for this detailed analysis of uapi impacts and usbip host side and vhci incompatibilities. Userspace is going to be affected. In addition to the usbip tool in the kernel repo, there are other versions floating around that would break if we were to change the flags. > One way to solve this issue for usbip > is to add some boilerplate transform > from URB_* to USBIP_FLAGS_* > as it is de facto uapi now. It doesn't sound like a there is a compelling reason other than "it would be nice to keep the flag values in numerical order". I would not recommend this option. I am not seeing any value to adding change URB_* to USBIP_FLAGS_* layer without some serious techinical concerns. > > Another way is to use 0x0400 for FREE_COHERENT. > usbip will not take care of this bit as > it would be masked. > I would go with this option adding a clear comment with link to this discussion. > Cc Shuah Khan here since she is the maintainer > on usbip. > Thank you adding me to the discussion. thanks, -- Shuah
On Thu, Jun 23, 2022 at 11:45:13AM -0600, Shuah Khan wrote: > On 6/23/22 11:30 AM, Hongren Zenithal Zheng wrote: > > On Fri, Jun 10, 2022 at 05:33:35PM -0400, Rhett Aultman wrote: > > > > > > In order to have all the flags in numerical order, URB_DIR_IN is > > > renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse > > > value 0x0200. > > > > > #define URB_FREE_BUFFER 0x0100 /* Free transfer buffer with the URB */ > > > +#define URB_FREE_COHERENT 0x0200 /* Free DMA memory of transfer buffer */ > > > /* The following flags are used internally by usbcore and HCDs */ > > > -#define URB_DIR_IN 0x0200 /* Transfer from device to host */ > > > +#define URB_DIR_IN 0x0400 /* Transfer from device to host */ > > > #define URB_DIR_OUT 0 > > > #define URB_DIR_MASK URB_DIR_IN > > > -- > > > 2.30.2 > > > > > > > I'm afraid this is a change of uapi as this field is, unfortunately, > > exported by usbip to userspace as TCP packets. > > > > This may also cause incompatibility (surprisingly not for this case, > > detailed below) between usbip server and client > > when one kernel is using the new flags and the other one is not. > > > > If we do change this, we may need to bump usbip protocol version > > accordingly. > > > > > > A copy of Alan Stern's suggestion here for reference > > > I don't see anything wrong with this, except that it would be nice to keep > > > the flag values in numerical order. In other words, set URB_FREE_COHERENT > > > to 0x0200 and change URB_DIR_IN to 0x0400. > > > > > > Alan Stern > > Thank you Alan for this detailed analysis of uapi impacts and > usbip host side and vhci incompatibilities. Userspace is going > to be affected. In addition to the usbip tool in the kernel repo, > there are other versions floating around that would break if we > were to change the flags. > > > One way to solve this issue for usbip > > is to add some boilerplate transform > > from URB_* to USBIP_FLAGS_* > > as it is de facto uapi now. > > It doesn't sound like a there is a compelling reason other than > "it would be nice to keep the flag values in numerical order". > > I would not recommend this option. I am not seeing any value to adding > change URB_* to USBIP_FLAGS_* layer without some serious techinical > concerns. > > > > > Another way is to use 0x0400 for FREE_COHERENT. > > usbip will not take care of this bit as > > it would be masked. > > > > I would go with this option adding a clear comment with link to this > discussion. > > > Cc Shuah Khan here since she is the maintainer > > on usbip. > > > > Thank you adding me to the discussion. I can see this causing more problems in the future. There's no hint in include/linux/usb.h that any of the values it defines are part of a user API. If they are, they should be moved to include/uapi/linux/usb/. In general, if a user program depends on kernel details that are not designed to be part of a user API, you should expect that the program will sometimes break from one kernel version to another. Yes, I know Linus insists that kernel changes should not cause regressions in userspace, but the line has to be drawn somewhere. Otherwise the kernel could never change at all. Alan Stern
On Fri, Jun 24, 2022 at 10:43:34AM -0400, Alan Stern wrote: > > > > > One way to solve this issue for usbip > > > is to add some boilerplate transform > > > from URB_* to USBIP_FLAGS_* > > > as it is de facto uapi now. > > > > It doesn't sound like a there is a compelling reason other than > > "it would be nice to keep the flag values in numerical order". > > > > I would not recommend this option. I am not seeing any value to adding > > change URB_* to USBIP_FLAGS_* layer without some serious techinical > > concerns. The transfer_flag in usbip is de facto uapi, That's why I'm proposing the USBIP_FLAGS_* way and further more I think usbip could move some flags/structs in usbip_common.h to include/uapi/linux/usb/usbip.h, instead of the userspace copying them into their own header. I will start a new thread if Shuah think that is acceptable. If this patch is to be landed, I think it should be landed along with the usbip change so there would be no userspace change; Even without this patch, making usbip flags/structs uapi alone is still worth doing. > > > > > > > > Another way is to use 0x0400 for FREE_COHERENT. > > > usbip will not take care of this bit as > > > it would be masked. > > > > > > > I would go with this option adding a clear comment with link to this > > discussion. > > > > > Cc Shuah Khan here since she is the maintainer > > > on usbip. > > > > > > > Thank you adding me to the discussion. > > I can see this causing more problems in the future. There's no hint in > include/linux/usb.h that any of the values it defines are part of a user > API. If they are, they should be moved to include/uapi/linux/usb/. I agree with this argument. > > In general, if a user program depends on kernel details that are not > designed to be part of a user API, you should expect that the program > will sometimes break from one kernel version to another. > > Yes, I know Linus insists that kernel changes should not cause > regressions in userspace, but the line has to be drawn somewhere. > Otherwise the kernel could never change at all. > > Alan Stern
On 6/24/22 8:43 AM, Alan Stern wrote: > On Thu, Jun 23, 2022 at 11:45:13AM -0600, Shuah Khan wrote: >> On 6/23/22 11:30 AM, Hongren Zenithal Zheng wrote: >>> On Fri, Jun 10, 2022 at 05:33:35PM -0400, Rhett Aultman wrote: >>>> >>>> In order to have all the flags in numerical order, URB_DIR_IN is >>>> renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse >>>> value 0x0200. >>> >>>> #define URB_FREE_BUFFER 0x0100 /* Free transfer buffer with the URB */ >>>> +#define URB_FREE_COHERENT 0x0200 /* Free DMA memory of transfer buffer */ >>>> /* The following flags are used internally by usbcore and HCDs */ >>>> -#define URB_DIR_IN 0x0200 /* Transfer from device to host */ >>>> +#define URB_DIR_IN 0x0400 /* Transfer from device to host */ >>>> #define URB_DIR_OUT 0 >>>> #define URB_DIR_MASK URB_DIR_IN >>>> -- >>>> 2.30.2 >>>> >>> >>> I'm afraid this is a change of uapi as this field is, unfortunately, >>> exported by usbip to userspace as TCP packets. >>> >>> This may also cause incompatibility (surprisingly not for this case, >>> detailed below) between usbip server and client >>> when one kernel is using the new flags and the other one is not. >>> >>> If we do change this, we may need to bump usbip protocol version >>> accordingly. >>> >> >> >>> A copy of Alan Stern's suggestion here for reference >>>> I don't see anything wrong with this, except that it would be nice to keep >>>> the flag values in numerical order. In other words, set URB_FREE_COHERENT >>>> to 0x0200 and change URB_DIR_IN to 0x0400. >>>> >>>> Alan Stern >> >> Thank you Alan for this detailed analysis of uapi impacts and >> usbip host side and vhci incompatibilities. Userspace is going >> to be affected. In addition to the usbip tool in the kernel repo, >> there are other versions floating around that would break if we >> were to change the flags. >> >>> One way to solve this issue for usbip >>> is to add some boilerplate transform >>> from URB_* to USBIP_FLAGS_* >>> as it is de facto uapi now. >> >> It doesn't sound like a there is a compelling reason other than >> "it would be nice to keep the flag values in numerical order". >> >> I would not recommend this option. I am not seeing any value to adding >> change URB_* to USBIP_FLAGS_* layer without some serious techinical >> concerns. >> >>> >>> Another way is to use 0x0400 for FREE_COHERENT. >>> usbip will not take care of this bit as >>> it would be masked. >>> >> >> I would go with this option adding a clear comment with link to this >> discussion. >> >>> Cc Shuah Khan here since she is the maintainer >>> on usbip. >>> >> >> Thank you adding me to the discussion. > > I can see this causing more problems in the future. There's no hint in > include/linux/usb.h that any of the values it defines are part of a user > API. If they are, they should be moved to include/uapi/linux/usb/. > Please elaborate on more problems in the future. > In general, if a user program depends on kernel details that are not > designed to be part of a user API, you should expect that the program > will sometimes break from one kernel version to another. > > Yes, I know Linus insists that kernel changes should not cause > regressions in userspace, but the line has to be drawn somewhere. > Otherwise the kernel could never change at all. > I have had to change the usbip sysfs interface api in the past to address security bugs related to information leaks. I am not saying no. I am asking if there is a good reason to do this. So far I haven't heard one. thanks, -- Shuah
On Fri, Jun 24, 2022 at 10:31:06AM -0600, Shuah Khan wrote: > On 6/24/22 8:43 AM, Alan Stern wrote: > > > It doesn't sound like a there is a compelling reason other than > > > "it would be nice to keep the flag values in numerical order". > > > > > > I would not recommend this option. I am not seeing any value to adding > > > change URB_* to USBIP_FLAGS_* layer without some serious techinical > > > concerns. > > > > > > > > > > > Another way is to use 0x0400 for FREE_COHERENT. > > > > usbip will not take care of this bit as > > > > it would be masked. > > > > > > > > > > I would go with this option adding a clear comment with link to this > > > discussion. > > > > > > > Cc Shuah Khan here since she is the maintainer > > > > on usbip. > > > > > > > > > > Thank you adding me to the discussion. > > > > I can see this causing more problems in the future. There's no hint in > > include/linux/usb.h that any of the values it defines are part of a user > > API. If they are, they should be moved to include/uapi/linux/usb/. > > > > Please elaborate on more problems in the future. In the future people will want to make other changes to include/linux/usb.h and they will not be aware that those changes will adversely affect usbip, because there is no documentation saying that the values defined in usb.h are part of a user API. That will be a problem, because those changes may be serious and important ones, not just decorative or stylistic as in this case. > > In general, if a user program depends on kernel details that are not > > designed to be part of a user API, you should expect that the program > > will sometimes break from one kernel version to another. > > > > Yes, I know Linus insists that kernel changes should not cause > > regressions in userspace, but the line has to be drawn somewhere. > > Otherwise the kernel could never change at all. > > > > I have had to change the usbip sysfs interface api in the past to > address security bugs related to information leaks. I am not saying > no. I am asking if there is a good reason to do this. So far I haven't > heard one. I agree with Hongren that values defined in include/linux/ should not be part of a user API. There are two choices: Move the definitions into include/uapi/linux/, or Add code to translate the values between the numbers used in userspace and the numbers used in the kernel. (This is what was done for urb->transfer_flags in devio.c:proc_do_submiturb() near line 1862.) Alan Stern
On 6/24/22 12:07 PM, Alan Stern wrote: > On Fri, Jun 24, 2022 at 10:31:06AM -0600, Shuah Khan wrote: >> On 6/24/22 8:43 AM, Alan Stern wrote: >>>> It doesn't sound like a there is a compelling reason other than >>>> "it would be nice to keep the flag values in numerical order". >>>> >>>> I would not recommend this option. I am not seeing any value to adding >>>> change URB_* to USBIP_FLAGS_* layer without some serious techinical >>>> concerns. >>>> >>>>> >>>>> Another way is to use 0x0400 for FREE_COHERENT. >>>>> usbip will not take care of this bit as >>>>> it would be masked. >>>>> >>>> >>>> I would go with this option adding a clear comment with link to this >>>> discussion. >>>> >>>>> Cc Shuah Khan here since she is the maintainer >>>>> on usbip. >>>>> >>>> >>>> Thank you adding me to the discussion. >>> >>> I can see this causing more problems in the future. There's no hint in >>> include/linux/usb.h that any of the values it defines are part of a user >>> API. If they are, they should be moved to include/uapi/linux/usb/. >>> >> >> Please elaborate on more problems in the future. > > In the future people will want to make other changes to > include/linux/usb.h and they will not be aware that those changes will > adversely affect usbip, because there is no documentation saying that > the values defined in usb.h are part of a user API. That will be a > problem, because those changes may be serious and important ones, not > just decorative or stylistic as in this case. > How often do these values change based on our past experience with these fields? >>> In general, if a user program depends on kernel details that are not >>> designed to be part of a user API, you should expect that the program >>> will sometimes break from one kernel version to another. >>> >>> Yes, I know Linus insists that kernel changes should not cause >>> regressions in userspace, but the line has to be drawn somewhere. >>> Otherwise the kernel could never change at all. >>> >> >> I have had to change the usbip sysfs interface api in the past to >> address security bugs related to information leaks. I am not saying >> no. I am asking if there is a good reason to do this. So far I haven't >> heard one. > > I agree with Hongren that values defined in include/linux/ should not be > part of a user API. There are two choices: > I agree with this in general. I don't think this is an explicit decision to make them part of API. It is a consequence of simply copying the transfer_flags. I am with you both on not being able to recognize the impact until as this is rather obscure usage hidden away in the packets. These defines aren't directly referenced. > Move the definitions into include/uapi/linux/, or > Wouldn't this be easier way to handle the change? With this option the uapi will be well documented. > Add code to translate the values between the numbers used in > userspace and the numbers used in the kernel. (This is what > was done for urb->transfer_flags in devio.c:proc_do_submiturb() > near line 1862.) > I looked at the code and looks simple enough. I am okay going this route if we see issues with the option 1. thanks, -- Shuah
On 6/30/22 8:10 PM, Shuah Khan wrote: > On 6/27/22 7:35 PM, Alan Stern wrote: >> On Mon, Jun 27, 2022 at 04:54:17PM -0600, Shuah Khan wrote: >>> On 6/24/22 12:07 PM, Alan Stern wrote: >>>> In the future people will want to make other changes to >>>> include/linux/usb.h and they will not be aware that those changes will >>>> adversely affect usbip, because there is no documentation saying that >>>> the values defined in usb.h are part of a user API. That will be a >>>> problem, because those changes may be serious and important ones, not >>>> just decorative or stylistic as in this case. >>>> >>> >>> How often do these values change based on our past experience with these >>> fields? >> >> I don't know. You could check the git history to find out for certain. >> My guess would be every eight or ten years. >> >>>> I agree with Hongren that values defined in include/linux/ should not be >>>> part of a user API. There are two choices: >>>> >>> >>> I agree with this in general. I don't think this is an explicit decision >>> to make them part of API. It is a consequence of simply copying the >>> transfer_flags. I am with you both on not being able to recognize the >>> impact until as this is rather obscure usage hidden away in the packets. >>> These defines aren't directly referenced. >>> >>>> Move the definitions into include/uapi/linux/, or >>>> >>> >>> Wouldn't this be easier way to handle the change? With this option >>> the uapi will be well documented. >>> >>>> Add code to translate the values between the numbers used in >>>> userspace and the numbers used in the kernel. (This is what >>>> was done for urb->transfer_flags in devio.c:proc_do_submiturb() >>>> near line 1862.) >>>> >>> >>> I looked at the code and looks simple enough. I am okay going this route >>> if we see issues with the option 1. >> >> It's up to you; either approach is okay with me. However, I do think >> that the second option is a little better; I don't see any good reason >> why the kernel should be forced to use the same numeric values for these >> flags forever. Especially since the only user program that needs to >> know them is usbip, which is fairly closely tied to the kernel; if there >> were more programs using those values then they would constitute a good >> reason for choosing the first option. >> > > Thank you Alan and Hongren for your help with this problem. Since there > are no changes to the flags for the time being, I am comfortable going > with the second option. > > I will send a patch soon. > Patch is almost ready to be sent out. Changes aren't bad at all. Hoping to get this done sooner - summer vacations didn't cooperate. Just an update that I haven't forgotten and it will taken care of. thanks, -- Shuah
On Tue. 2 Aug. 2022 at 02:48, Shuah Khan <skhan@linuxfoundation.org> wrote: > On 6/30/22 8:10 PM, Shuah Khan wrote: > > On 6/27/22 7:35 PM, Alan Stern wrote: > >> On Mon, Jun 27, 2022 at 04:54:17PM -0600, Shuah Khan wrote: > >>> On 6/24/22 12:07 PM, Alan Stern wrote: > >>>> In the future people will want to make other changes to > >>>> include/linux/usb.h and they will not be aware that those changes will > >>>> adversely affect usbip, because there is no documentation saying that > >>>> the values defined in usb.h are part of a user API. That will be a > >>>> problem, because those changes may be serious and important ones, not > >>>> just decorative or stylistic as in this case. > >>>> > >>> > >>> How often do these values change based on our past experience with these > >>> fields? > >> > >> I don't know. You could check the git history to find out for certain. > >> My guess would be every eight or ten years. > >> > >>>> I agree with Hongren that values defined in include/linux/ should not be > >>>> part of a user API. There are two choices: > >>>> > >>> > >>> I agree with this in general. I don't think this is an explicit decision > >>> to make them part of API. It is a consequence of simply copying the > >>> transfer_flags. I am with you both on not being able to recognize the > >>> impact until as this is rather obscure usage hidden away in the packets. > >>> These defines aren't directly referenced. > >>> > >>>> Move the definitions into include/uapi/linux/, or > >>>> > >>> > >>> Wouldn't this be easier way to handle the change? With this option > >>> the uapi will be well documented. > >>> > >>>> Add code to translate the values between the numbers used in > >>>> userspace and the numbers used in the kernel. (This is what > >>>> was done for urb->transfer_flags in devio.c:proc_do_submiturb() > >>>> near line 1862.) > >>>> > >>> > >>> I looked at the code and looks simple enough. I am okay going this route > >>> if we see issues with the option 1. > >> > >> It's up to you; either approach is okay with me. However, I do think > >> that the second option is a little better; I don't see any good reason > >> why the kernel should be forced to use the same numeric values for these > >> flags forever. Especially since the only user program that needs to > >> know them is usbip, which is fairly closely tied to the kernel; if there > >> were more programs using those values then they would constitute a good > >> reason for choosing the first option. > >> > > > > Thank you Alan and Hongren for your help with this problem. Since there > > are no changes to the flags for the time being, I am comfortable going > > with the second option. > > > > I will send a patch soon. > > > > Patch is almost ready to be sent out. Changes aren't bad at all. Hoping to > get this done sooner - summer vacations didn't cooperate. > > Just an update that I haven't forgotten and it will taken care of. > thanks, Thanks for keeping this under your radar. I also have on my TODO list to send a new version of my patch to add the `URB_FREE_COHERENT' flag but this time adding an `allocated_length' field to struct urb. I will wait for your patch to go first. By the way, I will be out for summer holiday for the next couple of weeks so I wasn't planning to submit anything soon regardless. Yours sincerely, Vincent Mailhol
On 8/1/22 12:28 PM, Vincent MAILHOL wrote: > On Tue. 2 Aug. 2022 at 02:48, Shuah Khan <skhan@linuxfoundation.org> wrote: >> On 6/30/22 8:10 PM, Shuah Khan wrote: >>> On 6/27/22 7:35 PM, Alan Stern wrote: >>>> On Mon, Jun 27, 2022 at 04:54:17PM -0600, Shuah Khan wrote: >>>>> On 6/24/22 12:07 PM, Alan Stern wrote: >>>>>> In the future people will want to make other changes to >>>>>> include/linux/usb.h and they will not be aware that those changes will >>>>>> adversely affect usbip, because there is no documentation saying that >>>>>> the values defined in usb.h are part of a user API. That will be a >>>>>> problem, because those changes may be serious and important ones, not >>>>>> just decorative or stylistic as in this case. >>>>>> >>>>> >>>>> How often do these values change based on our past experience with these >>>>> fields? >>>> >>>> I don't know. You could check the git history to find out for certain. >>>> My guess would be every eight or ten years. >>>> >>>>>> I agree with Hongren that values defined in include/linux/ should not be >>>>>> part of a user API. There are two choices: >>>>>> >>>>> >>>>> I agree with this in general. I don't think this is an explicit decision >>>>> to make them part of API. It is a consequence of simply copying the >>>>> transfer_flags. I am with you both on not being able to recognize the >>>>> impact until as this is rather obscure usage hidden away in the packets. >>>>> These defines aren't directly referenced. >>>>> >>>>>> Move the definitions into include/uapi/linux/, or >>>>>> >>>>> >>>>> Wouldn't this be easier way to handle the change? With this option >>>>> the uapi will be well documented. >>>>> >>>>>> Add code to translate the values between the numbers used in >>>>>> userspace and the numbers used in the kernel. (This is what >>>>>> was done for urb->transfer_flags in devio.c:proc_do_submiturb() >>>>>> near line 1862.) >>>>>> >>>>> >>>>> I looked at the code and looks simple enough. I am okay going this route >>>>> if we see issues with the option 1. >>>> >>>> It's up to you; either approach is okay with me. However, I do think >>>> that the second option is a little better; I don't see any good reason >>>> why the kernel should be forced to use the same numeric values for these >>>> flags forever. Especially since the only user program that needs to >>>> know them is usbip, which is fairly closely tied to the kernel; if there >>>> were more programs using those values then they would constitute a good >>>> reason for choosing the first option. >>>> >>> >>> Thank you Alan and Hongren for your help with this problem. Since there >>> are no changes to the flags for the time being, I am comfortable going >>> with the second option. >>> >>> I will send a patch soon. >>> >> >> Patch is almost ready to be sent out. Changes aren't bad at all. Hoping to >> get this done sooner - summer vacations didn't cooperate. >> >> Just an update that I haven't forgotten and it will taken care of. >> thanks, > > Thanks for keeping this under your radar. I also have on my TODO list > to send a new version of my patch to add the `URB_FREE_COHERENT' flag > but this time adding an `allocated_length' field to struct urb. I will > wait for your patch to go first. By the way, I will be out for summer > holiday for the next couple of weeks so I wasn't planning to submit > anything soon regardless. > Sounds good. I now have the patch ready to be sent out. I will wait for the merge window to close before I send it out. thanks, -- Shuah
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c index 33d62d7e3929..36c48fb196e0 100644 --- a/drivers/usb/core/urb.c +++ b/drivers/usb/core/urb.c @@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref) if (urb->transfer_flags & URB_FREE_BUFFER) kfree(urb->transfer_buffer); + else if (urb->transfer_flags & URB_FREE_COHERENT) + usb_free_coherent(urb->dev, urb->transfer_buffer_length, + urb->transfer_buffer, urb->transfer_dma); kfree(urb); } @@ -504,7 +507,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) /* Check against a simple/standard policy */ allowed = (URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT | URB_DIR_MASK | - URB_FREE_BUFFER); + URB_FREE_BUFFER | URB_FREE_COHERENT); switch (xfertype) { case USB_ENDPOINT_XFER_BULK: case USB_ENDPOINT_XFER_INT: diff --git a/include/linux/usb.h b/include/linux/usb.h index 60bee864d897..945d68ea1d76 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -1328,9 +1328,10 @@ extern int usb_disabled(void); #define URB_NO_INTERRUPT 0x0080 /* HINT: no non-error interrupt * needed */ #define URB_FREE_BUFFER 0x0100 /* Free transfer buffer with the URB */ +#define URB_FREE_COHERENT 0x0200 /* Free DMA memory of transfer buffer */ /* The following flags are used internally by usbcore and HCDs */ -#define URB_DIR_IN 0x0200 /* Transfer from device to host */ +#define URB_DIR_IN 0x0400 /* Transfer from device to host */ #define URB_DIR_OUT 0 #define URB_DIR_MASK URB_DIR_IN