[PATCH] spi: atmel: Prevent spi transfers from being killed

[PATCH] spi: atmel: Prevent spi transfers from being killed

[Miquel Raynal]

Upstream commit e0205d6203c2 ("spi: atmel: Prevent false timeouts on
long transfers") has tried to mitigate the problem of getting spi
transfers canceled because they were lasting too long. On slow buses,
transfers in the MiB range can take more than one second and thus a
calculation was added to progressively increment the timeout value. In
order to not be too problematic from a user point of view (waiting dozen
of seconds or even minutes), the wait call was turned interruptible.

Turning the wait interruptible was a mistake as what we really wanted to
do was to be able to kill a transfer. Any signal interrupting our
transfer would not be suitable at all so a second attempt was made at
turning the wait killable instead.

Link: https://lore.kernel.org/linux-spi/20231127095842.389631-1-miquel.raynal@bootlin.com/

All being well, it was reported that JFFS2 was showing a splat when
interrupting a transfer. After some more debate about whether JFFS2
should be fixed and how, it was also pointed out that the whole
consistency of the filesystem in case of parallel I/O would be
compromised. Changing JFFS2 behavior would in theory be possible but
nobody has the energy and time and knowledge to do this now, so better
prevent spi transfers to be interrupted by the user.

Partially revert the blamed commit to no longer use the interruptible
nor the killable variant of wait_for_completion().

Fixes: e0205d6203c2 ("spi: atmel: Prevent false timeouts on long transfers")
Cc: stable@vger.kernel.org
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-atmel.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 0197c25f5029..54277de30161 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -1333,12 +1333,10 @@ static int atmel_spi_one_transfer(struct spi_controller *host,
 		}
 
 		dma_timeout = msecs_to_jiffies(spi_controller_xfer_timeout(host, xfer));
-		ret_timeout = wait_for_completion_killable_timeout(&as->xfer_completion,
-								   dma_timeout);
-		if (ret_timeout <= 0) {
-			dev_err(&spi->dev, "spi transfer %s\n",
-				!ret_timeout ? "timeout" : "canceled");
-			as->done_status = ret_timeout < 0 ? ret_timeout : -EIO;
+		ret_timeout = wait_for_completion_timeout(&as->xfer_completion, dma_timeout);
+		if (!ret_timeout) {
+			dev_err(&spi->dev, "spi transfer timeout\n");
+			as->done_status = -EIO;
 		}
 
 		if (as->done_status)
-- 
2.34.1

[PATCH] spi: atmel: Prevent spi transfers from being killed

[Miquel Raynal]

Upstream commit e0205d6203c2 ("spi: atmel: Prevent false timeouts on
long transfers") has tried to mitigate the problem of getting spi
transfers canceled because they were lasting too long. On slow buses,
transfers in the MiB range can take more than one second and thus a
calculation was added to progressively increment the timeout value. In
order to not be too problematic from a user point of view (waiting dozen
of seconds or even minutes), the wait call was turned interruptible.

Turning the wait interruptible was a mistake as what we really wanted to
do was to be able to kill a transfer. Any signal interrupting our
transfer would not be suitable at all so a second attempt was made at
turning the wait killable instead.

Link: https://lore.kernel.org/linux-spi/20231127095842.389631-1-miquel.raynal@bootlin.com/

All being well, it was reported that JFFS2 was showing a splat when
interrupting a transfer. After some more debate about whether JFFS2
should be fixed and how, it was also pointed out that the whole
consistency of the filesystem in case of parallel I/O would be
compromised. Changing JFFS2 behavior would in theory be possible but
nobody has the energy and time and knowledge to do this now, so better
prevent spi transfers to be interrupted by the user.

Partially revert the blamed commit to no longer use the interruptible
nor the killable variant of wait_for_completion().

Fixes: e0205d6203c2 ("spi: atmel: Prevent false timeouts on long transfers")
Cc: stable@vger.kernel.org
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-atmel.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 0197c25f5029..54277de30161 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -1333,12 +1333,10 @@ static int atmel_spi_one_transfer(struct spi_controller *host,
 		}
 
 		dma_timeout = msecs_to_jiffies(spi_controller_xfer_timeout(host, xfer));
-		ret_timeout = wait_for_completion_killable_timeout(&as->xfer_completion,
-								   dma_timeout);
-		if (ret_timeout <= 0) {
-			dev_err(&spi->dev, "spi transfer %s\n",
-				!ret_timeout ? "timeout" : "canceled");
-			as->done_status = ret_timeout < 0 ? ret_timeout : -EIO;
+		ret_timeout = wait_for_completion_timeout(&as->xfer_completion, dma_timeout);
+		if (!ret_timeout) {
+			dev_err(&spi->dev, "spi transfer timeout\n");
+			as->done_status = -EIO;
 		}
 
 		if (as->done_status)
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] spi: atmel: Prevent spi transfers from being killed

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
> All being well, it was reported that JFFS2 was showing a splat when
> interrupting a transfer. After some more debate about whether JFFS2
> should be fixed and how, it was also pointed out that the whole
> consistency of the filesystem in case of parallel I/O would be
> compromised. Changing JFFS2 behavior would in theory be possible but
> nobody has the energy and time and knowledge to do this now, so better
> prevent spi transfers to be interrupted by the user.

Well, it's not really an JFFS2 issue.
The real problem is, that with the said change anyone can abort an IO.
Usually file systems assume that an IO can only fail in fatal situations.
That's why UBIFS, for example, switches immediately to read-only mode.
So, an unprivileged user can force UBIFS into read-only mode, which is a
local DoS attack vector.

JFFS2, on the other hand, dies a different death. If you abort one IO,
another IO path can still be active and will violate the order of written
data.

Long story short, aborting pure user inflicted IO is fine. This is the "dd"
use case.
But as soon a filesystem is on top, things get complicated.

Maybe it is possible to teach the SPI subsystem whether an IO comes from spidev
or the kernel itself?

Thanks,
//richard

Re: [PATCH] spi: atmel: Prevent spi transfers from being killed

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
> All being well, it was reported that JFFS2 was showing a splat when
> interrupting a transfer. After some more debate about whether JFFS2
> should be fixed and how, it was also pointed out that the whole
> consistency of the filesystem in case of parallel I/O would be
> compromised. Changing JFFS2 behavior would in theory be possible but
> nobody has the energy and time and knowledge to do this now, so better
> prevent spi transfers to be interrupted by the user.

Well, it's not really an JFFS2 issue.
The real problem is, that with the said change anyone can abort an IO.
Usually file systems assume that an IO can only fail in fatal situations.
That's why UBIFS, for example, switches immediately to read-only mode.
So, an unprivileged user can force UBIFS into read-only mode, which is a
local DoS attack vector.

JFFS2, on the other hand, dies a different death. If you abort one IO,
another IO path can still be active and will violate the order of written
data.

Long story short, aborting pure user inflicted IO is fine. This is the "dd"
use case.
But as soon a filesystem is on top, things get complicated.

Maybe it is possible to teach the SPI subsystem whether an IO comes from spidev
or the kernel itself?

Thanks,
//richard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] spi: atmel: Prevent spi transfers from being killed

[Ronald Wahl]

On 05.12.23 09:31, Miquel Raynal wrote:
> Upstream commit e0205d6203c2 ("spi: atmel: Prevent false timeouts on
> long transfers") has tried to mitigate the problem of getting spi
> transfers canceled because they were lasting too long. On slow buses,
> transfers in the MiB range can take more than one second and thus a
> calculation was added to progressively increment the timeout value. In
> order to not be too problematic from a user point of view (waiting dozen
> of seconds or even minutes), the wait call was turned interruptible.
>
> Turning the wait interruptible was a mistake as what we really wanted to
> do was to be able to kill a transfer. Any signal interrupting our
> transfer would not be suitable at all so a second attempt was made at
> turning the wait killable instead.
>
> Link: https://lore.kernel.org/linux-spi/20231127095842.389631-1-miquel.raynal@bootlin.com/
>
> All being well, it was reported that JFFS2 was showing a splat when
> interrupting a transfer. After some more debate about whether JFFS2
> should be fixed and how, it was also pointed out that the whole
> consistency of the filesystem in case of parallel I/O would be
> compromised. Changing JFFS2 behavior would in theory be possible but
> nobody has the energy and time and knowledge to do this now, so better
> prevent spi transfers to be interrupted by the user.
>
> Partially revert the blamed commit to no longer use the interruptible
> nor the killable variant of wait_for_completion().
>
> Fixes: e0205d6203c2 ("spi: atmel: Prevent false timeouts on long transfers")
> Cc: stable@vger.kernel.org

Tested-by: Ronald Wahl <ronald.wahl@raritan.com>

> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   drivers/spi/spi-atmel.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
> index 0197c25f5029..54277de30161 100644
> --- a/drivers/spi/spi-atmel.c
> +++ b/drivers/spi/spi-atmel.c
> @@ -1333,12 +1333,10 @@ static int atmel_spi_one_transfer(struct spi_controller *host,
>               }
>
>               dma_timeout = msecs_to_jiffies(spi_controller_xfer_timeout(host, xfer));
> -             ret_timeout = wait_for_completion_killable_timeout(&as->xfer_completion,
> -                                                                dma_timeout);
> -             if (ret_timeout <= 0) {
> -                     dev_err(&spi->dev, "spi transfer %s\n",
> -                             !ret_timeout ? "timeout" : "canceled");
> -                     as->done_status = ret_timeout < 0 ? ret_timeout : -EIO;
> +             ret_timeout = wait_for_completion_timeout(&as->xfer_completion, dma_timeout);
> +             if (!ret_timeout) {
> +                     dev_err(&spi->dev, "spi transfer timeout\n");
> +                     as->done_status = -EIO;
>               }
>
>               if (as->done_status)


________________________________

Ce message, ainsi que tous les fichiers joints à ce message, peuvent contenir des informations sensibles et/ ou confidentielles ne devant pas être divulguées. Si vous n'êtes pas le destinataire de ce message (ou que vous recevez ce message par erreur), nous vous remercions de le notifier immédiatement à son expéditeur, et de détruire ce message. Toute copie, divulgation, modification, utilisation ou diffusion, non autorisée, directe ou indirecte, de tout ou partie de ce message, est strictement interdite.


This e-mail, and any document attached hereby, may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorized, direct or indirect, copying, disclosure, distribution or other use of the material or parts thereof is strictly forbidden.

Re: [PATCH] spi: atmel: Prevent spi transfers from being killed

[Ronald Wahl]

On 05.12.23 09:31, Miquel Raynal wrote:
> Upstream commit e0205d6203c2 ("spi: atmel: Prevent false timeouts on
> long transfers") has tried to mitigate the problem of getting spi
> transfers canceled because they were lasting too long. On slow buses,
> transfers in the MiB range can take more than one second and thus a
> calculation was added to progressively increment the timeout value. In
> order to not be too problematic from a user point of view (waiting dozen
> of seconds or even minutes), the wait call was turned interruptible.
>
> Turning the wait interruptible was a mistake as what we really wanted to
> do was to be able to kill a transfer. Any signal interrupting our
> transfer would not be suitable at all so a second attempt was made at
> turning the wait killable instead.
>
> Link: https://lore.kernel.org/linux-spi/20231127095842.389631-1-miquel.raynal@bootlin.com/
>
> All being well, it was reported that JFFS2 was showing a splat when
> interrupting a transfer. After some more debate about whether JFFS2
> should be fixed and how, it was also pointed out that the whole
> consistency of the filesystem in case of parallel I/O would be
> compromised. Changing JFFS2 behavior would in theory be possible but
> nobody has the energy and time and knowledge to do this now, so better
> prevent spi transfers to be interrupted by the user.
>
> Partially revert the blamed commit to no longer use the interruptible
> nor the killable variant of wait_for_completion().
>
> Fixes: e0205d6203c2 ("spi: atmel: Prevent false timeouts on long transfers")
> Cc: stable@vger.kernel.org

Tested-by: Ronald Wahl <ronald.wahl@raritan.com>

> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   drivers/spi/spi-atmel.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
> index 0197c25f5029..54277de30161 100644
> --- a/drivers/spi/spi-atmel.c
> +++ b/drivers/spi/spi-atmel.c
> @@ -1333,12 +1333,10 @@ static int atmel_spi_one_transfer(struct spi_controller *host,
>               }
>
>               dma_timeout = msecs_to_jiffies(spi_controller_xfer_timeout(host, xfer));
> -             ret_timeout = wait_for_completion_killable_timeout(&as->xfer_completion,
> -                                                                dma_timeout);
> -             if (ret_timeout <= 0) {
> -                     dev_err(&spi->dev, "spi transfer %s\n",
> -                             !ret_timeout ? "timeout" : "canceled");
> -                     as->done_status = ret_timeout < 0 ? ret_timeout : -EIO;
> +             ret_timeout = wait_for_completion_timeout(&as->xfer_completion, dma_timeout);
> +             if (!ret_timeout) {
> +                     dev_err(&spi->dev, "spi transfer timeout\n");
> +                     as->done_status = -EIO;
>               }
>
>               if (as->done_status)


________________________________

Ce message, ainsi que tous les fichiers joints à ce message, peuvent contenir des informations sensibles et/ ou confidentielles ne devant pas être divulguées. Si vous n'êtes pas le destinataire de ce message (ou que vous recevez ce message par erreur), nous vous remercions de le notifier immédiatement à son expéditeur, et de détruire ce message. Toute copie, divulgation, modification, utilisation ou diffusion, non autorisée, directe ou indirecte, de tout ou partie de ce message, est strictement interdite.


This e-mail, and any document attached hereby, may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorized, direct or indirect, copying, disclosure, distribution or other use of the material or parts thereof is strictly forbidden.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] spi: atmel: Prevent spi transfers from being killed

[Miquel Raynal]

Hi Richard,

richard@nod.at wrote on Tue, 5 Dec 2023 10:22:56 +0100 (CET):

> ----- Ursprüngliche Mail -----
> > Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
> > All being well, it was reported that JFFS2 was showing a splat when
> > interrupting a transfer. After some more debate about whether JFFS2
> > should be fixed and how, it was also pointed out that the whole
> > consistency of the filesystem in case of parallel I/O would be
> > compromised. Changing JFFS2 behavior would in theory be possible but
> > nobody has the energy and time and knowledge to do this now, so better
> > prevent spi transfers to be interrupted by the user.  
> 
> Well, it's not really an JFFS2 issue.
> The real problem is, that with the said change anyone can abort an IO.
> Usually file systems assume that an IO can only fail in fatal situations.
> That's why UBIFS, for example, switches immediately to read-only mode.
> So, an unprivileged user can force UBIFS into read-only mode, which is a
> local DoS attack vector.

Right.

> JFFS2, on the other hand, dies a different death. If you abort one IO,
> another IO path can still be active and will violate the order of written
> data.
> 
> Long story short, aborting pure user inflicted IO is fine. This is the "dd"
> use case.
> But as soon a filesystem is on top, things get complicated.
> 
> Maybe it is possible to teach the SPI subsystem whether an IO comes from spidev
> or the kernel itself?

Well, it would only partially fix the problem, I was playing with a
spi-nor or spi-nand chip (don't remember) which was supported in the
kernel, just making big reads/writes (without fs, at this time). I
don't think deliberating on whether the driver requesting the transfer
is in the kernel or in userspace matters, but whether there is a
filesystem on top or not. But TBH I don't think this can be solved
without ugly hacks...

Thanks,
Miquèl

Re: [PATCH] spi: atmel: Prevent spi transfers from being killed

[Miquel Raynal]

Hi Richard,

richard@nod.at wrote on Tue, 5 Dec 2023 10:22:56 +0100 (CET):

> ----- Ursprüngliche Mail -----
> > Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
> > All being well, it was reported that JFFS2 was showing a splat when
> > interrupting a transfer. After some more debate about whether JFFS2
> > should be fixed and how, it was also pointed out that the whole
> > consistency of the filesystem in case of parallel I/O would be
> > compromised. Changing JFFS2 behavior would in theory be possible but
> > nobody has the energy and time and knowledge to do this now, so better
> > prevent spi transfers to be interrupted by the user.  
> 
> Well, it's not really an JFFS2 issue.
> The real problem is, that with the said change anyone can abort an IO.
> Usually file systems assume that an IO can only fail in fatal situations.
> That's why UBIFS, for example, switches immediately to read-only mode.
> So, an unprivileged user can force UBIFS into read-only mode, which is a
> local DoS attack vector.

Right.

> JFFS2, on the other hand, dies a different death. If you abort one IO,
> another IO path can still be active and will violate the order of written
> data.
> 
> Long story short, aborting pure user inflicted IO is fine. This is the "dd"
> use case.
> But as soon a filesystem is on top, things get complicated.
> 
> Maybe it is possible to teach the SPI subsystem whether an IO comes from spidev
> or the kernel itself?

Well, it would only partially fix the problem, I was playing with a
spi-nor or spi-nand chip (don't remember) which was supported in the
kernel, just making big reads/writes (without fs, at this time). I
don't think deliberating on whether the driver requesting the transfer
is in the kernel or in userspace matters, but whether there is a
filesystem on top or not. But TBH I don't think this can be solved
without ugly hacks...

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] spi: atmel: Prevent spi transfers from being killed

[Mark Brown]

On Tue, 05 Dec 2023 09:31:02 +0100, Miquel Raynal wrote:
> Upstream commit e0205d6203c2 ("spi: atmel: Prevent false timeouts on
> long transfers") has tried to mitigate the problem of getting spi
> transfers canceled because they were lasting too long. On slow buses,
> transfers in the MiB range can take more than one second and thus a
> calculation was added to progressively increment the timeout value. In
> order to not be too problematic from a user point of view (waiting dozen
> of seconds or even minutes), the wait call was turned interruptible.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: atmel: Prevent spi transfers from being killed
      commit: 890188d2d7e4ac6c131ba166ca116cb315e752ee

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH] spi: atmel: Prevent spi transfers from being killed

[Mark Brown]

On Tue, 05 Dec 2023 09:31:02 +0100, Miquel Raynal wrote:
> Upstream commit e0205d6203c2 ("spi: atmel: Prevent false timeouts on
> long transfers") has tried to mitigate the problem of getting spi
> transfers canceled because they were lasting too long. On slow buses,
> transfers in the MiB range can take more than one second and thus a
> calculation was added to progressively increment the timeout value. In
> order to not be too problematic from a user point of view (waiting dozen
> of seconds or even minutes), the wait call was turned interruptible.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: atmel: Prevent spi transfers from being killed
      commit: 890188d2d7e4ac6c131ba166ca116cb315e752ee

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel