| Message ID | 6832dffafd54a6a95b287c4a1ef30250d6b9237a.1624282817.git.mchehab+huawei@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] media: uvc: don't do DMA on stack | expand |
Hi Mauro, Thank you for the patch. On Mon, Jun 21, 2021 at 03:40:19PM +0200, Mauro Carvalho Chehab wrote: > As warned by smatch: > drivers/media/usb/uvc/uvc_v4l2.c:911 uvc_ioctl_g_input() error: doing dma on the stack (&i) > drivers/media/usb/uvc/uvc_v4l2.c:943 uvc_ioctl_s_input() error: doing dma on the stack (&i) > > those two functions call uvc_query_ctrl passing a pointer to > a data at the DMA stack. those are used to send URBs via > usb_control_msg(). Using DMA stack is not supported and should > not work anymore on modern Linux versions. > > So, use a kmalloc'ed buffer. > > Cc: stable@vger.kernel.org # Kernel 4.9 and upper > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > --- > drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 252136cc885c..a95bf7318848 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -899,8 +899,8 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input) > { > struct uvc_fh *handle = fh; > struct uvc_video_chain *chain = handle->chain; > + u8 *buf; > int ret; > - u8 i; > > if (chain->selector == NULL || > (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) { > @@ -908,13 +908,20 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input) > return 0; > } > > + buf = kmalloc(1, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, chain->selector->id, > chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL, > - &i, 1); > + buf, 1); > if (ret < 0) > return ret; Memory leak :-) if (!ret) *input = *buf - 1; kfree(buf); return ret; > > - *input = i - 1; > + *input = *buf - 1; > + > + kfree(buf); > + > return 0; > } > > @@ -922,8 +929,8 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input) > { > struct uvc_fh *handle = fh; > struct uvc_video_chain *chain = handle->chain; > + char *buf; u8 *buf; With these two changes, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Do I need to take the patch in my tree ? > int ret; > - u32 i; > > ret = uvc_acquire_privileges(handle); > if (ret < 0) > @@ -939,10 +946,17 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input) > if (input >= chain->selector->bNrInPins) > return -EINVAL; > > - i = input + 1; > - return uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id, > - chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL, > - &i, 1); > + buf = kmalloc(1, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + *buf = input + 1; > + ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id, > + chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL, > + buf, 1); > + kfree(buf); > + > + return ret; > } > > static int uvc_ioctl_queryctrl(struct file *file, void *fh,
From: Mauro Carvalho Chehab > Sent: 21 June 2021 14:40 > > As warned by smatch: > drivers/media/usb/uvc/uvc_v4l2.c:911 uvc_ioctl_g_input() error: doing dma on the stack (&i) > drivers/media/usb/uvc/uvc_v4l2.c:943 uvc_ioctl_s_input() error: doing dma on the stack (&i) > > those two functions call uvc_query_ctrl passing a pointer to > a data at the DMA stack. those are used to send URBs via > usb_control_msg(). Using DMA stack is not supported and should > not work anymore on modern Linux versions. > > So, use a kmalloc'ed buffer. ... > + buf = kmalloc(1, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, chain->selector->id, > chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL, > - &i, 1); > + buf, 1); Thought... Is kmalloc(1, GFP_KERNEL) guaranteed to return a pointer into a cache line that will not be accessed by any other code? (This is slightly weaker than requiring a cache-line aligned pointer - but very similar.) Without that guarantee you can't use the returned buffer for read dma unless the memory accesses are coherent. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Jun 22, 2021 at 08:07:12AM +0000, David Laight wrote: > From: Mauro Carvalho Chehab > > Sent: 21 June 2021 14:40 > > > > As warned by smatch: > > drivers/media/usb/uvc/uvc_v4l2.c:911 uvc_ioctl_g_input() error: doing dma on the stack (&i) > > drivers/media/usb/uvc/uvc_v4l2.c:943 uvc_ioctl_s_input() error: doing dma on the stack (&i) > > > > those two functions call uvc_query_ctrl passing a pointer to > > a data at the DMA stack. those are used to send URBs via > > usb_control_msg(). Using DMA stack is not supported and should > > not work anymore on modern Linux versions. > > > > So, use a kmalloc'ed buffer. > ... > > + buf = kmalloc(1, GFP_KERNEL); > > + if (!buf) > > + return -ENOMEM; > > + > > ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, chain->selector->id, > > chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL, > > - &i, 1); > > + buf, 1); > > Thought... > > Is kmalloc(1, GFP_KERNEL) guaranteed to return a pointer into > a cache line that will not be accessed by any other code? > > (This is slightly weaker than requiring a cache-line aligned > pointer - but very similar.) > > Without that guarantee you can't use the returned buffer for > read dma unless the memory accesses are coherent. For USB buffers, that should be fine, we have been doing this for decades now... thanks, greg k-h
Hi Mauro, On Mon, Jun 21, 2021 at 04:34:08PM +0200, Mauro Carvalho Chehab wrote: > Em Mon, 21 Jun 2021 17:11:46 +0300 Laurent Pinchart escreveu: > > On Mon, Jun 21, 2021 at 03:40:19PM +0200, Mauro Carvalho Chehab wrote: > > > As warned by smatch: > > > drivers/media/usb/uvc/uvc_v4l2.c:911 uvc_ioctl_g_input() error: doing dma on the stack (&i) > > > drivers/media/usb/uvc/uvc_v4l2.c:943 uvc_ioctl_s_input() error: doing dma on the stack (&i) > > > > > > those two functions call uvc_query_ctrl passing a pointer to > > > a data at the DMA stack. those are used to send URBs via > > > usb_control_msg(). Using DMA stack is not supported and should > > > not work anymore on modern Linux versions. > > > > > > So, use a kmalloc'ed buffer. > > > > > > Cc: stable@vger.kernel.org # Kernel 4.9 and upper > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > > --- > > > drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++-------- > > > 1 file changed, 22 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > > index 252136cc885c..a95bf7318848 100644 > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > > @@ -899,8 +899,8 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input) > > > { > > > struct uvc_fh *handle = fh; > > > struct uvc_video_chain *chain = handle->chain; > > > + u8 *buf; > > > int ret; > > > - u8 i; > > > > > > if (chain->selector == NULL || > > > (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) { > > > @@ -908,13 +908,20 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input) > > > return 0; > > > } > > > > > > + buf = kmalloc(1, GFP_KERNEL); > > > + if (!buf) > > > + return -ENOMEM; > > > + > > > ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, chain->selector->id, > > > chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL, > > > - &i, 1); > > > + buf, 1); > > > if (ret < 0) > > > return ret; > > > > Memory leak :-) > > Argh ;-) > > Clearly, I'm needing more caffeine today, but it is too damn hot > here... > > > > > if (!ret) > > *input = *buf - 1; > > > > kfree(buf); > > > > return ret; > > > > > > > > - *input = i - 1; > > > + *input = *buf - 1; > > > + > > > + kfree(buf); > > > + > > > return 0; > > > } > > > > > > @@ -922,8 +929,8 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input) > > > { > > > struct uvc_fh *handle = fh; > > > struct uvc_video_chain *chain = handle->chain; > > > + char *buf; > > > > u8 *buf; > > > > With these two changes, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Thanks! > > > Do I need to take the patch in my tree ? > > It is up to you. > > I suspect that it would be easier to just merge it at media_stage, > together with the other patches from the smatch series, but it is > up to you. > > Just let me know if you prefer to merge it via your tree, and I'll drop > it from my queue, or otherwise I'll merge directly at media_stage, > after waiting for a while on feedbacks on the remaining patches. Please merge it directly, it's less work for me :-) -- Regards, Laurent Pinchart
From: Alan Stern > Sent: 22 June 2021 14:29 ... > > Thought... > > > > Is kmalloc(1, GFP_KERNEL) guaranteed to return a pointer into > > a cache line that will not be accessed by any other code? > > (This is slightly weaker than requiring a cache-line aligned > > pointer - but very similar.) > > As I understand it, on architectures that do not have cache-coherent > I/O, kmalloc is guaranteed to return a buffer that is > cacheline-aligned and whose length is a multiple of the cacheline > size. > > Now, whether that buffer ends up being accessed by any other code > depends on what your driver does with the pointer it gets from > kmalloc. :-) Thanks for the clarification. Most of the small allocates in the usb stack are for transmits where it is only necessary to ensure a cache write-back. I know there has been some confusion because one of the allocators can add a small header to every allocation. This can lead to unexpectedly inadequately aligned pointers. If it is updated when the preceding block is freed (as some user-space mallocs do) then it would need to be in a completely separate cache line. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Jun 22, 2021 at 02:21:27PM +0000, David Laight wrote: > From: Alan Stern > > Sent: 22 June 2021 14:29 > ... > > > Thought... > > > > > > Is kmalloc(1, GFP_KERNEL) guaranteed to return a pointer into > > > a cache line that will not be accessed by any other code? > > > (This is slightly weaker than requiring a cache-line aligned > > > pointer - but very similar.) > > > > As I understand it, on architectures that do not have cache-coherent > > I/O, kmalloc is guaranteed to return a buffer that is > > cacheline-aligned and whose length is a multiple of the cacheline > > size. > > > > Now, whether that buffer ends up being accessed by any other code > > depends on what your driver does with the pointer it gets from > > kmalloc. :-) > > Thanks for the clarification. > > Most of the small allocates in the usb stack are for transmits > where it is only necessary to ensure a cache write-back. > > I know there has been some confusion because one of the > allocators can add a small header to every allocation. > This can lead to unexpectedly inadequately aligned pointers. > If it is updated when the preceding block is freed (as some > user-space mallocs do) then it would need to be in a > completely separate cache line. If you really want to find out what the true story is, you should ask on the linux-mm mailing list. The rest of us are not experts on this stuff. Alan Stern
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 252136cc885c..a95bf7318848 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -899,8 +899,8 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input) { struct uvc_fh *handle = fh; struct uvc_video_chain *chain = handle->chain; + u8 *buf; int ret; - u8 i; if (chain->selector == NULL || (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) { @@ -908,13 +908,20 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input) return 0; } + buf = kmalloc(1, GFP_KERNEL); + if (!buf) + return -ENOMEM; + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, chain->selector->id, chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL, - &i, 1); + buf, 1); if (ret < 0) return ret; - *input = i - 1; + *input = *buf - 1; + + kfree(buf); + return 0; } @@ -922,8 +929,8 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input) { struct uvc_fh *handle = fh; struct uvc_video_chain *chain = handle->chain; + char *buf; int ret; - u32 i; ret = uvc_acquire_privileges(handle); if (ret < 0) @@ -939,10 +946,17 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input) if (input >= chain->selector->bNrInPins) return -EINVAL; - i = input + 1; - return uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id, - chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL, - &i, 1); + buf = kmalloc(1, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + *buf = input + 1; + ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id, + chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL, + buf, 1); + kfree(buf); + + return ret; } static int uvc_ioctl_queryctrl(struct file *file, void *fh,
As warned by smatch: drivers/media/usb/uvc/uvc_v4l2.c:911 uvc_ioctl_g_input() error: doing dma on the stack (&i) drivers/media/usb/uvc/uvc_v4l2.c:943 uvc_ioctl_s_input() error: doing dma on the stack (&i) those two functions call uvc_query_ctrl passing a pointer to a data at the DMA stack. those are used to send URBs via usb_control_msg(). Using DMA stack is not supported and should not work anymore on modern Linux versions. So, use a kmalloc'ed buffer. Cc: stable@vger.kernel.org # Kernel 4.9 and upper Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)