| Message ID | 20230730153343.22033-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] media: atomisp: Fix smatch warnings caused by atomisp custom assert() usage | expand |
Hi Hans, On Sun, Jul 30, 2023 at 11:33 PM Hans de Goede <hdegoede@redhat.com> wrote: > > The atomisp code base has a custom assert() macro, a couple of functions > use this in a construction like the following: > > assert(pipe); > assert(pipe->stream); > if ((!pipe) || (!pipe->stream)) { > ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > "allocate_mipi_frames(%p) exit: ...\n", > pipe); > return -EINVAL; > } > > The second assert is seen by smatch as dereferencing "pipe" in the above > example (and dereferencing "dvs_6axis_config" in the other case). > > Following by the dereferenced variable being checked (a second time) > in the following if () statement. > > This triggers the following smatch warnings: > drivers/staging/media/atomisp/pci/sh_css_mipi.c:356 allocate_mipi_frames() warn: variable dereferenced before check 'pipe' (see line 355) > drivers/staging/media/atomisp/pci/sh_css_mipi.c:562 send_mipi_frames() warn: variable dereferenced before check 'pipe' (see line 561) > drivers/staging/media/atomisp/pci/sh_css_param_dvs.c:208 free_dvs_6axis_table() warn: variable dereferenced before check 'dvs_6axis_config' (see line 206) > > The custom assert() macro actually expands to a BUG() call and BUG() > calls should not be used in the kernel. > > Remove the assert() calls to fix the smatch warnings and in case of > [allocate|send]_mipi_frames() also remove the if () return -EINVAL > block since these functions are never called with a NULL pipe. > > Reported-by: Hans Verkuil <hverkuil@xs4all.nl> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Note: > 1. No Closes: tag since Hans Verkuil reported this in an offlist email. > 2. All the other assert() calls really should also be removed. > I've put this on my todo list. > --- > Changes in v2: > - Drop the sh_css_sp.c bits, just dropping the assert() calls is > not enough there > --- > drivers/staging/media/atomisp/pci/sh_css_mipi.c | 16 ---------------- > .../staging/media/atomisp/pci/sh_css_param_dvs.c | 3 --- > 2 files changed, 19 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c > index b20acaab0595..ced21dedf7ac 100644 > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c > @@ -351,15 +351,6 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, > ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > "allocate_mipi_frames(%p) enter:\n", pipe); > > - assert(pipe); > - assert(pipe->stream); > - if ((!pipe) || (!pipe->stream)) { > - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > - "allocate_mipi_frames(%p) exit: pipe or stream is null.\n", > - pipe); > - return -EINVAL; > - } Thank you for working on this. The NULL test for pipe could be kept here since the caller only tests "stream" but not test "main_pipe" is NULL. (sh_css.c line: 1799) The rest of the part is ok for me. > - > if (IS_ISP2401 && pipe->stream->config.online) { > ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > "allocate_mipi_frames(%p) exit: no buffers needed for 2401 pipe mode.\n", > @@ -557,13 +548,6 @@ send_mipi_frames(struct ia_css_pipe *pipe) > > IA_CSS_ENTER_PRIVATE("pipe=%p", pipe); > > - assert(pipe); > - assert(pipe->stream); > - if (!pipe || !pipe->stream) { > - IA_CSS_ERROR("pipe or stream is null"); > - return -EINVAL; > - } > - > /* multi stream video needs mipi buffers */ > /* nothing to be done in other cases. */ > if (pipe->stream->config.mode != IA_CSS_INPUT_MODE_BUFFERED_SENSOR) { > diff --git a/drivers/staging/media/atomisp/pci/sh_css_param_dvs.c b/drivers/staging/media/atomisp/pci/sh_css_param_dvs.c > index ff0082d02af3..5174bc210ae1 100644 > --- a/drivers/staging/media/atomisp/pci/sh_css_param_dvs.c > +++ b/drivers/staging/media/atomisp/pci/sh_css_param_dvs.c > @@ -202,9 +202,6 @@ generate_dvs_6axis_table_from_config(struct ia_css_dvs_6axis_config > void > free_dvs_6axis_table(struct ia_css_dvs_6axis_config **dvs_6axis_config) > { > - assert(dvs_6axis_config); > - assert(*dvs_6axis_config); > - > if ((dvs_6axis_config) && (*dvs_6axis_config)) { > IA_CSS_ENTER_PRIVATE("dvs_6axis_config %p", (*dvs_6axis_config)); > if ((*dvs_6axis_config)->xcoords_y) { > -- > 2.41.0 >
On Tue, Aug 01, 2023 at 08:32:18PM +0800, Kate Hsuan wrote: > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c > > index b20acaab0595..ced21dedf7ac 100644 > > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c > > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c > > @@ -351,15 +351,6 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, > > ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > > "allocate_mipi_frames(%p) enter:\n", pipe); > > > > - assert(pipe); > > - assert(pipe->stream); > > - if ((!pipe) || (!pipe->stream)) { > > - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, > > - "allocate_mipi_frames(%p) exit: pipe or stream is null.\n", > > - pipe); > > - return -EINVAL; > > - } > > Thank you for working on this. > > The NULL test for pipe could be kept here since the caller only tests > "stream" but not test "main_pipe" is NULL. (sh_css.c line: 1799) > It dereferences main_pipe on the next line so this check would be too late. drivers/staging/media/atomisp/pci/sh_css.c 1799 main_pipe = stream->last_pipe; 1800 pipe_id = main_pipe->mode; Smatch says that ->last_pipe is known to be non-NULL as well. $ smdb.py create_host_pipeline drivers/staging/media/atomisp/pci/sh_css.c | ia_css_stream_start | create_host_pipeline | PARAM_VALUE | 0 | stream->last_pipe | 4096-ptr_max regards, dan carpenter
diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c index b20acaab0595..ced21dedf7ac 100644 --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c @@ -351,15 +351,6 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, "allocate_mipi_frames(%p) enter:\n", pipe); - assert(pipe); - assert(pipe->stream); - if ((!pipe) || (!pipe->stream)) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, - "allocate_mipi_frames(%p) exit: pipe or stream is null.\n", - pipe); - return -EINVAL; - } - if (IS_ISP2401 && pipe->stream->config.online) { ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, "allocate_mipi_frames(%p) exit: no buffers needed for 2401 pipe mode.\n", @@ -557,13 +548,6 @@ send_mipi_frames(struct ia_css_pipe *pipe) IA_CSS_ENTER_PRIVATE("pipe=%p", pipe); - assert(pipe); - assert(pipe->stream); - if (!pipe || !pipe->stream) { - IA_CSS_ERROR("pipe or stream is null"); - return -EINVAL; - } - /* multi stream video needs mipi buffers */ /* nothing to be done in other cases. */ if (pipe->stream->config.mode != IA_CSS_INPUT_MODE_BUFFERED_SENSOR) { diff --git a/drivers/staging/media/atomisp/pci/sh_css_param_dvs.c b/drivers/staging/media/atomisp/pci/sh_css_param_dvs.c index ff0082d02af3..5174bc210ae1 100644 --- a/drivers/staging/media/atomisp/pci/sh_css_param_dvs.c +++ b/drivers/staging/media/atomisp/pci/sh_css_param_dvs.c @@ -202,9 +202,6 @@ generate_dvs_6axis_table_from_config(struct ia_css_dvs_6axis_config void free_dvs_6axis_table(struct ia_css_dvs_6axis_config **dvs_6axis_config) { - assert(dvs_6axis_config); - assert(*dvs_6axis_config); - if ((dvs_6axis_config) && (*dvs_6axis_config)) { IA_CSS_ENTER_PRIVATE("dvs_6axis_config %p", (*dvs_6axis_config)); if ((*dvs_6axis_config)->xcoords_y) {
The atomisp code base has a custom assert() macro, a couple of functions use this in a construction like the following: assert(pipe); assert(pipe->stream); if ((!pipe) || (!pipe->stream)) { ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, "allocate_mipi_frames(%p) exit: ...\n", pipe); return -EINVAL; } The second assert is seen by smatch as dereferencing "pipe" in the above example (and dereferencing "dvs_6axis_config" in the other case). Following by the dereferenced variable being checked (a second time) in the following if () statement. This triggers the following smatch warnings: drivers/staging/media/atomisp/pci/sh_css_mipi.c:356 allocate_mipi_frames() warn: variable dereferenced before check 'pipe' (see line 355) drivers/staging/media/atomisp/pci/sh_css_mipi.c:562 send_mipi_frames() warn: variable dereferenced before check 'pipe' (see line 561) drivers/staging/media/atomisp/pci/sh_css_param_dvs.c:208 free_dvs_6axis_table() warn: variable dereferenced before check 'dvs_6axis_config' (see line 206) The custom assert() macro actually expands to a BUG() call and BUG() calls should not be used in the kernel. Remove the assert() calls to fix the smatch warnings and in case of [allocate|send]_mipi_frames() also remove the if () return -EINVAL block since these functions are never called with a NULL pipe. Reported-by: Hans Verkuil <hverkuil@xs4all.nl> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Note: 1. No Closes: tag since Hans Verkuil reported this in an offlist email. 2. All the other assert() calls really should also be removed. I've put this on my todo list. --- Changes in v2: - Drop the sh_css_sp.c bits, just dropping the assert() calls is not enough there --- drivers/staging/media/atomisp/pci/sh_css_mipi.c | 16 ---------------- .../staging/media/atomisp/pci/sh_css_param_dvs.c | 3 --- 2 files changed, 19 deletions(-)