Re: [PATCH] mtd: core: always verify OOB offset in mtd_check_oob_ops()

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Gabor,

On 31/08/2025 at 16:40:10 +02, Gabor Juhos <j4g8y7@gmail.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@gmail.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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/