[bug report] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00

[bug report] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00

[Dan Carpenter]

Hello Green Wan,

This is a semi-automatic email about new static checker warnings.

The patch 6973886ad58e: "dmaengine: sf-pdma: add platform DMA support 
for HiFive Unleashed A00" from Nov 7, 2019, leads to the following 
Smatch complaint:

    drivers/dma/sf-pdma/sf-pdma.c:103 sf_pdma_prep_dma_memcpy()
     error: we previously assumed 'chan' could be null (see line 97)

drivers/dma/sf-pdma/sf-pdma.c
    96	
    97		if (chan && (!len || !dest || !src)) {
                    ^^^^
Check for NULL

    98			dev_err(chan->pdma->dma_dev.dev,
    99				"Please check dma len, dest, src!\n");
   100			return NULL;
   101		}
   102	
   103		desc = sf_pdma_alloc_desc(chan);
                                          ^^^^
Unchecked dereference inside the function.

   104		if (!desc)
   105			return NULL;

regards,
dan carpenter

Re: [bug report] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00

[Green Wan]

Hi Dan,

Thanks for reaching out. I do have the same question before. If I
remember it correctly, the 'chan' is returned by to_sf_pdma_chan()
which contains 'dchan'. 'chan' s unlikely to be NULL since it's
container.

I agreed that the code logic causes confused easily. Now the driver is
in -next branch. I can check with maintainer where/when to fix it.

--
Green


On Sat, Nov 16, 2019 at 4:23 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Green Wan,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 6973886ad58e: "dmaengine: sf-pdma: add platform DMA support
> for HiFive Unleashed A00" from Nov 7, 2019, leads to the following
> Smatch complaint:
>
>     drivers/dma/sf-pdma/sf-pdma.c:103 sf_pdma_prep_dma_memcpy()
>      error: we previously assumed 'chan' could be null (see line 97)
>
> drivers/dma/sf-pdma/sf-pdma.c
>     96
>     97          if (chan && (!len || !dest || !src)) {
>                     ^^^^
> Check for NULL
>
>     98                  dev_err(chan->pdma->dma_dev.dev,
>     99                          "Please check dma len, dest, src!\n");
>    100                  return NULL;
>    101          }
>    102
>    103          desc = sf_pdma_alloc_desc(chan);
>                                           ^^^^
> Unchecked dereference inside the function.
>
>    104          if (!desc)
>    105                  return NULL;
>
> regards,
> dan carpenter

Re: [bug report] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00

[Dan Carpenter]

On Mon, Nov 18, 2019 at 12:25:14PM +0800, Green Wan wrote:
> Hi Dan,
> 
> Thanks for reaching out. I do have the same question before. If I
> remember it correctly, the 'chan' is returned by to_sf_pdma_chan()
> which contains 'dchan'. 'chan' s unlikely to be NULL since it's
> container.

to_sf_pdma_chan() is really a no-op when you read it carefully.  "chan"
and "dchan" are equivalent.  Smatch parses it correctly.

You are obviously correct that people should check the original "dchan"
instead of the returned "chan", but I feel like some people are clever
enough to know when container_of() is a no-op and deliberately check the
returned value...  These are sorts of people you don't want to get into
a debate with because they're an ultra annoying blend of clever and
dumb.

My other comment here is that Smatch doesn't warn about inconsistent
NULL checking when the pointer is provably non-NULL.  In this case, the
only caller that Smatch doesn't parse correctly is ioat_dma_self_test().
For all the other callers it knows that "dchan" is non-NULL.  In
ioat_dma_self_test() the code looks like:

   330          /* Start copy, using first DMA channel */
   331          dma_chan = container_of(dma->channels.next, struct dma_chan,
   332                                  device_node);

Smatch says that both "dma->channels.next" and "dma_chan" are complete
unknowns.

I could probably safely mark dma_chan as a valid pointer in this
instance which would make the warning disappear...  Maybe I will add a
check to see if "dma->channels.next" can be controlled by the user.

regards,
dan carpenter

Re: [bug report] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00

[Green Wan]

Hi Dan,

Thanks for the comment. I'd like to do check and fix it. And to be
honest, I don't use smatch tool to do static analysis. Looks
interesting. Let me enable it to see how it helps/works.

Thanks a lot.





On Mon, Nov 18, 2019 at 3:36 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, Nov 18, 2019 at 12:25:14PM +0800, Green Wan wrote:
> > Hi Dan,
> >
> > Thanks for reaching out. I do have the same question before. If I
> > remember it correctly, the 'chan' is returned by to_sf_pdma_chan()
> > which contains 'dchan'. 'chan' s unlikely to be NULL since it's
> > container.
>
> to_sf_pdma_chan() is really a no-op when you read it carefully.  "chan"
> and "dchan" are equivalent.  Smatch parses it correctly.
>
> You are obviously correct that people should check the original "dchan"
> instead of the returned "chan", but I feel like some people are clever
> enough to know when container_of() is a no-op and deliberately check the
> returned value...  These are sorts of people you don't want to get into
> a debate with because they're an ultra annoying blend of clever and
> dumb.
>
> My other comment here is that Smatch doesn't warn about inconsistent
> NULL checking when the pointer is provably non-NULL.  In this case, the
> only caller that Smatch doesn't parse correctly is ioat_dma_self_test().
> For all the other callers it knows that "dchan" is non-NULL.  In
> ioat_dma_self_test() the code looks like:
>
>    330          /* Start copy, using first DMA channel */
>    331          dma_chan = container_of(dma->channels.next, struct dma_chan,
>    332                                  device_node);
>
> Smatch says that both "dma->channels.next" and "dma_chan" are complete
> unknowns.
>
> I could probably safely mark dma_chan as a valid pointer in this
> instance which would make the warning disappear...  Maybe I will add a
> check to see if "dma->channels.next" can be controlled by the user.
>
> regards,
> dan carpenter
>