diff mbox series

[v2] i2c: tegra: Fix failure during probe deferral cleanup

Message ID 20230707132619.2998382-1-thierry.reding@gmail.com
State New
Headers show
Series [v2] i2c: tegra: Fix failure during probe deferral cleanup | expand

Commit Message

Thierry Reding July 7, 2023, 1:26 p.m. UTC
From: Thierry Reding <treding@nvidia.com>

If the driver fails to obtain a DMA channel, it will initiate cleanup
and try to release the DMA channel that couldn't be retrieved. This will
cause a crash because the cleanup will try to dereference an ERR_PTR()-
encoded error code.

However, there's nothing to clean up at this point yet, so we can avoid
this by simply resetting the DMA channel to NULL instead of storing the
error code.

Fixes: fcc8a89a1c83 ("i2c: tegra: Share same DMA channel for RX and TX")
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- simplify patch by setting dma_chan = NULL on channel request failure

 drivers/i2c/busses/i2c-tegra.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Akhil R July 20, 2023, 3:42 p.m. UTC | #1
>If the driver fails to obtain a DMA channel, it will initiate cleanup 
>and try to release the DMA channel that couldn't be retrieved. This 
>will cause a crash because the cleanup will try to dereference an 
>ERR_PTR()- encoded error code.
>
>However, there's nothing to clean up at this point yet, so we can avoid 
>this by simply resetting the DMA channel to NULL instead of storing the 
>error code.
>
>Fixes: fcc8a89a1c83 ("i2c: tegra: Share same DMA channel for RX and 
>TX")
>Signed-off-by: Thierry Reding mailto:treding@nvidia.com

This fixed the crash issue seen when there is no dmas property in
the device tree.

Resending this with the correct headers to get it recorded by
patchwork.

Tested-by: Akhil R <akhilrajeev@nvidia.com>
Thierry Reding July 25, 2023, 4:25 p.m. UTC | #2
On Fri, Jul 07, 2023 at 03:26:19PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> If the driver fails to obtain a DMA channel, it will initiate cleanup
> and try to release the DMA channel that couldn't be retrieved. This will
> cause a crash because the cleanup will try to dereference an ERR_PTR()-
> encoded error code.
> 
> However, there's nothing to clean up at this point yet, so we can avoid
> this by simply resetting the DMA channel to NULL instead of storing the
> error code.
> 
> Fixes: fcc8a89a1c83 ("i2c: tegra: Share same DMA channel for RX and TX")
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - simplify patch by setting dma_chan = NULL on channel request failure
> 
>  drivers/i2c/busses/i2c-tegra.c | 1 +
>  1 file changed, 1 insertion(+)

Hi Andy, Wolfram,

do you have any reservations about this? This fixes a bug that was
introduced in v6.5-rc1, so it'd be great if this could make v6.5.

Thanks,
Thierry

> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index f155e9028f94..2a13f11edfd1 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -460,6 +460,7 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
>  	i2c_dev->dma_chan = dma_request_chan(i2c_dev->dev, "tx");
>  	if (IS_ERR(i2c_dev->dma_chan)) {
>  		err = PTR_ERR(i2c_dev->dma_chan);
> +		i2c_dev->dma_chan = NULL;
>  		goto err_out;
>  	}
>  
> -- 
> 2.41.0
>
Andi Shyti July 25, 2023, 9:34 p.m. UTC | #3
Hi Thierry,

On Fri, Jul 07, 2023 at 03:26:19PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> If the driver fails to obtain a DMA channel, it will initiate cleanup
> and try to release the DMA channel that couldn't be retrieved. This will
> cause a crash because the cleanup will try to dereference an ERR_PTR()-
> encoded error code.

while this is a valid solution I think the best thing here is to
clear the exit path by adding another goto label.

By setting dma_chan = NULL you would go through some extra checks
that are not needed.

I guess that by doing this we could even remove the

	if (i2c_dev->dma_buf)
	if (i2c_dev->dma_chan)

in tegra_i2c_release_dma(). However you see it cleaner. I'm not
going to be picky, though. Let me know if you are up for some
more clean up, otherwise I can give you an r-b... after a little
clarification...

> However, there's nothing to clean up at this point yet, so we can avoid
> this by simply resetting the DMA channel to NULL instead of storing the
> error code.
> 
> Fixes: fcc8a89a1c83 ("i2c: tegra: Share same DMA channel for RX and TX")

... is this the correct commit that is getting fixed? I think
it's this one:

Fixes: 86c92b9965ff ("i2c: tegra: Add DMA support")
Cc: <stable@vger.kernel.org> # v5.1+

Thanks,
Andi

> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - simplify patch by setting dma_chan = NULL on channel request failure
> 
>  drivers/i2c/busses/i2c-tegra.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index f155e9028f94..2a13f11edfd1 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -460,6 +460,7 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
>  	i2c_dev->dma_chan = dma_request_chan(i2c_dev->dev, "tx");
>  	if (IS_ERR(i2c_dev->dma_chan)) {
>  		err = PTR_ERR(i2c_dev->dma_chan);
> +		i2c_dev->dma_chan = NULL;
>  		goto err_out;
>  	}
>  
> -- 
> 2.41.0
>
Thierry Reding July 27, 2023, 3:05 p.m. UTC | #4
On Tue, Jul 25, 2023 at 11:34:04PM +0200, Andi Shyti wrote:
> Hi Thierry,
> 
> On Fri, Jul 07, 2023 at 03:26:19PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > If the driver fails to obtain a DMA channel, it will initiate cleanup
> > and try to release the DMA channel that couldn't be retrieved. This will
> > cause a crash because the cleanup will try to dereference an ERR_PTR()-
> > encoded error code.
> 
> while this is a valid solution I think the best thing here is to
> clear the exit path by adding another goto label.
> 
> By setting dma_chan = NULL you would go through some extra checks
> that are not needed.
> 
> I guess that by doing this we could even remove the
> 
> 	if (i2c_dev->dma_buf)
> 	if (i2c_dev->dma_chan)
> 
> in tegra_i2c_release_dma(). However you see it cleaner. I'm not
> going to be picky, though. Let me know if you are up for some
> more clean up, otherwise I can give you an r-b... after a little
> clarification...

The problem is that DMA support is optional, so we will typically
succeed probe even when the DMA channel cannot be retrieved. The
tegra_i2c_release_dma() is going to get called in any case and if
we were to remove those checks, it would try and release a NULL
buffer and a NULL channel for the non-DMA case.

That's also the reason why we set dma_chan = NULL rather than use
an error label. We could technically skip tegra_i2c_release_dma()
when we fail to get the channel, but we do want to run it when we
fail to allocate the DMA buffer. So that would mean we end up with
two different cleanup paths rather than just one. So overall the
cleanup is simpler if we treat both code paths the same.

> > However, there's nothing to clean up at this point yet, so we can avoid
> > this by simply resetting the DMA channel to NULL instead of storing the
> > error code.
> > 
> > Fixes: fcc8a89a1c83 ("i2c: tegra: Share same DMA channel for RX and TX")
> 
> ... is this the correct commit that is getting fixed? I think
> it's this one:
> 
> Fixes: 86c92b9965ff ("i2c: tegra: Add DMA support")
> Cc: <stable@vger.kernel.org> # v5.1+

The original DMA support patch didn't have this issue because it was
storing the DMA channel (or error code) in a local variable first and
only assigned it to the i2c_dev->{rx,tx}_dma_channel fields after
checking for errors. Hence, those fields would never end up with an
error code and therefore this wasn't causing any issues previously.

I hope that answers all your questions.

Thanks,
Thierry
Andi Shyti July 27, 2023, 7:57 p.m. UTC | #5
Hi Thierry,

> > > If the driver fails to obtain a DMA channel, it will initiate cleanup
> > > and try to release the DMA channel that couldn't be retrieved. This will
> > > cause a crash because the cleanup will try to dereference an ERR_PTR()-
> > > encoded error code.
> > 
> > while this is a valid solution I think the best thing here is to
> > clear the exit path by adding another goto label.
> > 
> > By setting dma_chan = NULL you would go through some extra checks
> > that are not needed.
> > 
> > I guess that by doing this we could even remove the
> > 
> > 	if (i2c_dev->dma_buf)
> > 	if (i2c_dev->dma_chan)
> > 
> > in tegra_i2c_release_dma(). However you see it cleaner. I'm not
> > going to be picky, though. Let me know if you are up for some
> > more clean up, otherwise I can give you an r-b... after a little
> > clarification...
> 
> The problem is that DMA support is optional, so we will typically
> succeed probe even when the DMA channel cannot be retrieved. The
> tegra_i2c_release_dma() is going to get called in any case and if
> we were to remove those checks, it would try and release a NULL
> buffer and a NULL channel for the non-DMA case.
> 
> That's also the reason why we set dma_chan = NULL rather than use
> an error label. We could technically skip tegra_i2c_release_dma()
> when we fail to get the channel, but we do want to run it when we
> fail to allocate the DMA buffer. So that would mean we end up with
> two different cleanup paths rather than just one. So overall the
> cleanup is simpler if we treat both code paths the same.

that's indeed an easy one-liner fix... that's why I proposed my
r-b in the earlier mail.

> > > However, there's nothing to clean up at this point yet, so we can avoid
> > > this by simply resetting the DMA channel to NULL instead of storing the
> > > error code.
> > > 
> > > Fixes: fcc8a89a1c83 ("i2c: tegra: Share same DMA channel for RX and TX")
> > 
> > ... is this the correct commit that is getting fixed? I think
> > it's this one:
> > 
> > Fixes: 86c92b9965ff ("i2c: tegra: Add DMA support")
> > Cc: <stable@vger.kernel.org> # v5.1+
> 
> The original DMA support patch didn't have this issue because it was
> storing the DMA channel (or error code) in a local variable first and
> only assigned it to the i2c_dev->{rx,tx}_dma_channel fields after
> checking for errors. Hence, those fields would never end up with an
> error code and therefore this wasn't causing any issues previously.

Yes, you are right! the patch commit you mentioned is actually
releasing the channel by checking i2c_dev->dma_chan which might
store the error number and therefore is not NULL.

Thanks for the clarification!

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

> I hope that answers all your questions.
> 
> Thanks,
> Thierry
Wolfram Sang Aug. 14, 2023, 1:34 p.m. UTC | #6
On Fri, Jul 07, 2023 at 03:26:19PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> If the driver fails to obtain a DMA channel, it will initiate cleanup
> and try to release the DMA channel that couldn't be retrieved. This will
> cause a crash because the cleanup will try to dereference an ERR_PTR()-
> encoded error code.
> 
> However, there's nothing to clean up at this point yet, so we can avoid
> this by simply resetting the DMA channel to NULL instead of storing the
> error code.
> 
> Fixes: fcc8a89a1c83 ("i2c: tegra: Share same DMA channel for RX and TX")
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Applied to for-current, thanks!
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index f155e9028f94..2a13f11edfd1 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -460,6 +460,7 @@  static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
 	i2c_dev->dma_chan = dma_request_chan(i2c_dev->dev, "tx");
 	if (IS_ERR(i2c_dev->dma_chan)) {
 		err = PTR_ERR(i2c_dev->dma_chan);
+		i2c_dev->dma_chan = NULL;
 		goto err_out;
 	}