[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <874itmpcrs.fsf@bootlin.com>
Date: Mon, 01 Sep 2025 14:39:51 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Gabor Juhos <j4g8y7@...il.com>
Cc: Richard Weinberger <richard@....at>, Vignesh Raghavendra
<vigneshr@...com>, linux-mtd@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mtd: core: always verify OOB offset in mtd_check_oob_ops()
Hi Gabor,
On 31/08/2025 at 16:40:10 +02, Gabor Juhos <j4g8y7@...il.com> wrote:
> Using an OOB offset past end of the available OOB data is invalid,
> irregardless of whether the 'ooblen' is set in the ops or not. Move
> the relevant check out from the if statement to always verify that.
>
> The 'oobtest' module executes four tests to verify how reading/writing
> OOB data past end of the devices is handled. It expects errors in case
> of these tests, but this expectation fails in the last two tests on
> MTD devices, which have no OOB bytes available.
>
> This is indicated in the test output like the following:
>
> [ 212.059416] mtd_oobtest: attempting to write past end of device
> [ 212.060379] mtd_oobtest: an error is expected...
> [ 212.066353] mtd_oobtest: error: wrote past end of device
> [ 212.071142] mtd_oobtest: attempting to read past end of device
> [ 212.076507] mtd_oobtest: an error is expected...
> [ 212.082080] mtd_oobtest: error: read past end of device
> ...
> [ 212.330508] mtd_oobtest: finished with 2 errors
>
> For reference, here is the corresponding code from the oobtest module:
>
> /* Attempt to write off end of device */
> ops.mode = MTD_OPS_AUTO_OOB;
> ops.len = 0;
> ops.retlen = 0;
> ops.ooblen = mtd->oobavail;
> ops.oobretlen = 0;
> ops.ooboffs = 1;
> ops.datbuf = NULL;
> ops.oobbuf = writebuf;
> pr_info("attempting to write past end of device\n");
> pr_info("an error is expected...\n");
> err = mtd_write_oob(mtd, mtd->size - mtd->writesize, &ops);
> if (err) {
> pr_info("error occurred as expected\n");
> } else {
> pr_err("error: wrote past end of device\n");
> errcnt += 1;
> }
>
> As it can be seen, the code sets 'ooboffs' to 1, and 'ooblen' to
> mtd->oobavail which is zero in our case.
>
> Since the mtd_check_oob_ops() function only verifies 'ooboffs' if 'ooblen'
> is not zero, the 'ooboffs' value does not gets validated and the function
> returns success whereas it should fail.
>
> After the change, the oobtest module will bail out early with an error if
> there are no OOB bytes available on the MDT device under test:
>
> # cat /sys/class/mtd/mtd0/oobavail
> 0
> # insmod mtd_test; insmod mtd_oobtest dev=0
> [ 943.606228]
> [ 943.606259] =================================================
> [ 943.606784] mtd_oobtest: MTD device: 0
> [ 943.612660] mtd_oobtest: MTD device size 524288, eraseblock size 131072, page size 2048, count of eraseblocks 4, pages per eraseblock 64, OOB size 128
> [ 943.616091] mtd_test: scanning for bad eraseblocks
> [ 943.629571] mtd_test: scanned 4 eraseblocks, 0 are bad
> [ 943.634313] mtd_oobtest: test 1 of 5
> [ 943.653402] mtd_oobtest: writing OOBs of whole device
> [ 943.653424] mtd_oobtest: error: writeoob failed at 0x0
> [ 943.657419] mtd_oobtest: error: use_len 0, use_offset 0
> [ 943.662493] mtd_oobtest: error -22 occurred
> [ 943.667574] =================================================
>
> This behaviour is more accurate than the current one where most tests
> are indicating successful writing of OOB data even that in fact nothing
> gets written into the device, which is quite misleading.
>
> Signed-off-by: Gabor Juhos <j4g8y7@...il.com>
Thanks a lot for this contribution, I'm ready to take it. Just one
question, do you consider it should be backported? I would tend to
answer yes to this question, which would involve you sending a v2 with:
Fixes:
Cc: stable...
Otherwise I can take it as-is if you convince me it is not so relevant
:-)
Cheers,
Miquèl
Powered by blists - more mailing lists