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@boot...
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)
----- 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
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
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@boot...
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.
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-stable-mirror@lists.linaro.org
-
Mark Brown -
Miquel Raynal -
Richard Weinberger -
Ronald Wahl