| Message ID | 20231205083102.16946-1-miquel.raynal@bootlin.com |
|---|---|
| State | Accepted |
| Commit | 890188d2d7e4ac6c131ba166ca116cb315e752ee |
| Headers | show |
| Series | spi: atmel: Prevent spi transfers from being killed | expand |
----- 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
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)
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 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
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)
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(-)