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
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
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
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
>