[PATCH] sh: dma: Add missing IS_ERR test

[PATCH] sh: dma: Add missing IS_ERR test

[Rolf Evers-Fischer]

get_dma_channel may return ERR_PTR, so a check is added.

Signed-off-by: Rolf Evers-Fischer <embedded24@evers-fischer.de>
---
 arch/sh/drivers/dma/dma-api.c   | 20 +++++++++++++++++++-
 arch/sh/drivers/dma/dma-sysfs.c |  2 +-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/sh/drivers/dma/dma-api.c b/arch/sh/drivers/dma/dma-api.c
index ab9170494dcc..5d6f1a46cc5e 100644
--- a/arch/sh/drivers/dma/dma-api.c
+++ b/arch/sh/drivers/dma/dma-api.c
@@ -94,7 +94,7 @@ int get_dma_residue(unsigned int chan)
 	struct dma_info *info = get_dma_info(chan);
 	struct dma_channel *channel = get_dma_channel(chan);
 
-	if (info->ops->get_residue)
+	if (!IS_ERR(channel) && (info->ops->get_residue))
 		return info->ops->get_residue(channel);
 
 	return 0;
@@ -195,6 +195,9 @@ int request_dma(unsigned int chan, const char *dev_id)
 	int result;
 
 	channel = get_dma_channel(chan);
+	if (IS_ERR(channel))
+		return PTR_ERR(channel);
+
 	if (atomic_xchg(&channel->busy, 1))
 		return -EBUSY;
 
@@ -217,6 +220,9 @@ void free_dma(unsigned int chan)
 	struct dma_info *info = get_dma_info(chan);
 	struct dma_channel *channel = get_dma_channel(chan);
 
+	if (IS_ERR(channel))
+		return;
+
 	if (info->ops->free)
 		info->ops->free(channel);
 
@@ -229,6 +235,9 @@ void dma_wait_for_completion(unsigned int chan)
 	struct dma_info *info = get_dma_info(chan);
 	struct dma_channel *channel = get_dma_channel(chan);
 
+	if (IS_ERR(channel))
+		return;
+
 	if (channel->flags & DMA_TEI_CAPABLE) {
 		wait_event(channel->wait_queue,
 			   (info->ops->get_residue(channel) == 0));
@@ -274,6 +283,9 @@ void dma_configure_channel(unsigned int chan, unsigned long flags)
 	struct dma_info *info = get_dma_info(chan);
 	struct dma_channel *channel = get_dma_channel(chan);
 
+	if (IS_ERR(channel))
+		return;
+
 	if (info->ops->configure)
 		info->ops->configure(channel, flags);
 }
@@ -285,6 +297,9 @@ int dma_xfer(unsigned int chan, unsigned long from,
 	struct dma_info *info = get_dma_info(chan);
 	struct dma_channel *channel = get_dma_channel(chan);
 
+	if (IS_ERR(channel))
+		return PTR_ERR(channel);
+
 	channel->sar	= from;
 	channel->dar	= to;
 	channel->count	= size;
@@ -299,6 +314,9 @@ int dma_extend(unsigned int chan, unsigned long op, void *param)
 	struct dma_info *info = get_dma_info(chan);
 	struct dma_channel *channel = get_dma_channel(chan);
 
+	if (IS_ERR(channel))
+		return PTR_ERR(channel);
+
 	if (info->ops->extend)
 		return info->ops->extend(channel, op, param);
 
diff --git a/arch/sh/drivers/dma/dma-sysfs.c b/arch/sh/drivers/dma/dma-sysfs.c
index 8ef318150f84..6ba5b569d446 100644
--- a/arch/sh/drivers/dma/dma-sysfs.c
+++ b/arch/sh/drivers/dma/dma-sysfs.c
@@ -30,7 +30,7 @@ static ssize_t dma_show_devices(struct device *dev,
 		struct dma_info *info = get_dma_info(i);
 		struct dma_channel *channel = get_dma_channel(i);
 
-		if (unlikely(!info) || !channel)
+		if (unlikely(!info) || IS_ERR_OR_NULL(channel))
 			continue;
 
 		len += sprintf(buf + len, "%2d: %14s    %s\n",
-- 
2.21.0

Re: [PATCH] sh: dma: Add missing IS_ERR test

[Sergei Shtylyov]

Hello!

On 07.06.2019 14:54, Rolf Evers-Fischer wrote:

> get_dma_channel may return ERR_PTR, so a check is added.
> 
> Signed-off-by: Rolf Evers-Fischer <embedded24@evers-fischer.de>
> ---
>   arch/sh/drivers/dma/dma-api.c   | 20 +++++++++++++++++++-
>   arch/sh/drivers/dma/dma-sysfs.c |  2 +-
>   2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/sh/drivers/dma/dma-api.c b/arch/sh/drivers/dma/dma-api.c
> index ab9170494dcc..5d6f1a46cc5e 100644
> --- a/arch/sh/drivers/dma/dma-api.c
> +++ b/arch/sh/drivers/dma/dma-api.c
> @@ -94,7 +94,7 @@ int get_dma_residue(unsigned int chan)
>   	struct dma_info *info = get_dma_info(chan);
>   	struct dma_channel *channel = get_dma_channel(chan);
>   
> -	if (info->ops->get_residue)
> +	if (!IS_ERR(channel) && (info->ops->get_residue))

    Extra parens not needed here.

[...]

MBR, Sergei

Re: [PATCH] sh: dma: Add missing IS_ERR test

[Geert Uytterhoeven]

Hi Rolf,

Thanks for your patch!

On Fri, Jun 7, 2019 at 2:04 PM Rolf Evers-Fischer
<embedded24@evers-fischer.de> wrote:
> get_dma_channel may return ERR_PTR, so a check is added.

It may also return NULL...

> --- a/arch/sh/drivers/dma/dma-api.c
> +++ b/arch/sh/drivers/dma/dma-api.c
> @@ -94,7 +94,7 @@ int get_dma_residue(unsigned int chan)
>         struct dma_info *info = get_dma_info(chan);
>         struct dma_channel *channel = get_dma_channel(chan);
>
> -       if (info->ops->get_residue)
> +       if (!IS_ERR(channel) && (info->ops->get_residue))
>                 return info->ops->get_residue(channel);

... in which case .get_residue() may crash, as some implementations
dereference the passed channel pointer.

Hence !IS_ERR_OR_NULL()?

I didn't check the other callers.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] sh: dma: Add missing IS_ERR test

[Rolf Evers-Fischer]

Hello Sergei,

thanks for your feedback.

On Sat, 8 Jun 2019, Sergei Shtylyov wrote:

> Hello!
> 
> On 07.06.2019 14:54, Rolf Evers-Fischer wrote:
> 
> > get_dma_channel may return ERR_PTR, so a check is added.
> > 
> > Signed-off-by: Rolf Evers-Fischer <embedded24@evers-fischer.de>
> > ---
> >   arch/sh/drivers/dma/dma-api.c   | 20 +++++++++++++++++++-
> >   arch/sh/drivers/dma/dma-sysfs.c |  2 +-
> >   2 files changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/sh/drivers/dma/dma-api.c b/arch/sh/drivers/dma/dma-api.c
> > index ab9170494dcc..5d6f1a46cc5e 100644
> > --- a/arch/sh/drivers/dma/dma-api.c
> > +++ b/arch/sh/drivers/dma/dma-api.c
> > @@ -94,7 +94,7 @@ int get_dma_residue(unsigned int chan)
> >   	struct dma_info *info = get_dma_info(chan);
> >   	struct dma_channel *channel = get_dma_channel(chan);
> >   -	if (info->ops->get_residue)
> > +	if (!IS_ERR(channel) && (info->ops->get_residue))
> 
>    Extra parens not needed here.
> 
> [...]

I agree with you. They should better be removed.

Kind regards,
 Rolf

Re: [PATCH] sh: dma: Add missing IS_ERR test

[Rolf Evers-Fischer]

On Sat, 8 Jun 2019, Geert Uytterhoeven wrote:
Hi Geert,

thank you for your reply and your additional findings.

> Hi Rolf,
> 
> Thanks for your patch!
> 
> On Fri, Jun 7, 2019 at 2:04 PM Rolf Evers-Fischer
> <embedded24@evers-fischer.de> wrote:
> > get_dma_channel may return ERR_PTR, so a check is added.
> 
> It may also return NULL...

Good catch. I must have missed this.

> 
> > --- a/arch/sh/drivers/dma/dma-api.c
> > +++ b/arch/sh/drivers/dma/dma-api.c
> > @@ -94,7 +94,7 @@ int get_dma_residue(unsigned int chan)
> >         struct dma_info *info = get_dma_info(chan);
> >         struct dma_channel *channel = get_dma_channel(chan);
> >
> > -       if (info->ops->get_residue)
> > +       if (!IS_ERR(channel) && (info->ops->get_residue))
> >                 return info->ops->get_residue(channel);
> 
> ... in which case .get_residue() may crash, as some implementations
> dereference the passed channel pointer.
> 
> Hence !IS_ERR_OR_NULL()?

Yes, in fact. IS_ERR_OR_NULL is the better choice here. 
I will resend a reworked patch immediately.

> 
> I didn't check the other callers.

Well, I did. And I found that none of the implementations checks the 
passed pointer. However, no in-tree driver is using the .extend() op, but 
as long as we don't know, if any out-of-tree drivers are using it without 
any additional check, I would prefer to check for NULL or error in 
dma_extend() as well.

Kind regards,
 Rolf