| Message ID | 6832dffafd54a6a95b287c4a1ef30250d6b9237a.1624282817.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| 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,
Em Mon, 21 Jun 2021 17:11:46 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > 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 :-) 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. Thanks, Mauro
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 :-)
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.) 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. :-) Alan Stern > Without that guarantee you can't use the returned buffer for > read dma unless the memory accesses are coherent. > > David
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(-)