[Qemu-devel] [PATCH] hw/block/nand: Fix bad offset in nand_blk_load for on-drive OOB

[Qemu-devel] [PATCH] hw/block/nand: Fix bad offset in nand_blk_load for on-drive OOB

[Karl Beldan]

The logic wants 512-byte aligned blk ops.
To switch to byte-based block accesses, the fixed commit changed the
blk read offset,
	PAGE_START(addr) >> 9
with
	PAGE_START(addr)
which min alignment, for on-drive OOB, is the min OOB size.
Consequently the reads are offset by PAGE_START(addr) & 0x1ff.

Fixes: 9fc0d361cc41 ("nand: Switch to byte-based block access")
Cc: Eric Blake <eblake@redhat.com>
Signed-off-by: Karl Beldan <karl.beldan+oss@gmail.com>
---
 hw/block/nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index 919cb9b803..ed587f60f0 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -788,7 +788,7 @@ static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState *s,
                             OOB_SIZE);
             s->ioaddr = s->io + SECTOR_OFFSET(s->addr) + offset;
         } else {
-            if (blk_pread(s->blk, PAGE_START(addr), s->io,
+            if (blk_pread(s->blk, PAGE_START(addr) & ~0x1ff, s->io,
                           (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS) < 0) {
                 printf("%s: read error in sector %" PRIu64 "\n",
                                 __func__, PAGE_START(addr) >> 9);
-- 
2.16.1.72.g5be1f00

Re: [Qemu-devel] [PATCH] hw/block/nand: Fix bad offset in nand_blk_load for on-drive OOB

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3980 bytes --]

On 04/23/2018 04:14 PM, Karl Beldan wrote:
> The logic wants 512-byte aligned blk ops.

The commit you are referencing mentions that the code permits 256, 512,
or 2048-byte alignment, based on the configuration of the hardware it is
emulating.  The whole file is hard to read, and I'm not surprised if
what I thought was a patch with no semantic change didn't quite succeed.

> To switch to byte-based block accesses, the fixed commit changed the
> blk read offset,
> 	PAGE_START(addr) >> 9
> with
> 	PAGE_START(addr)
> which min alignment, for on-drive OOB, is the min OOB size.
> Consequently the reads are offset by PAGE_START(addr) & 0x1ff.

Do you have a call trace where a caller is passing in an addr that is
not aligned to 512?  I agree that the old code was 512-aligned by virtue
of the old interface; but the new code SHOULD be able to call into the
block layer with whatever alignment it needs, where the block layer will
do a larger read to satisfy block constraints, as needed (that is, if a
caller requests a 256-byte read of a device that requires 512-alignment,
blk_pread() will automatically widen out to a 512-byte read).

> 
> Fixes: 9fc0d361cc41 ("nand: Switch to byte-based block access")
> Cc: Eric Blake <eblake@redhat.com>
> Signed-off-by: Karl Beldan <karl.beldan+oss@gmail.com>

CC: qemu-stable@nongnu.org

> ---
>  hw/block/nand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/nand.c b/hw/block/nand.c
> index 919cb9b803..ed587f60f0 100644
> --- a/hw/block/nand.c
> +++ b/hw/block/nand.c
> @@ -788,7 +788,7 @@ static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState *s,
>                              OOB_SIZE);
>              s->ioaddr = s->io + SECTOR_OFFSET(s->addr) + offset;
>          } else {
> -            if (blk_pread(s->blk, PAGE_START(addr), s->io,
> +            if (blk_pread(s->blk, PAGE_START(addr) & ~0x1ff, s->io,
>                            (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS) < 0) {

I don't like the magic number masking.  This should probably be written
QEMU_ALIGN_DOWN(PAGE_START(addr), BDRV_SECTOR_SIZE).

I would like to see some actual numbers from a backtrace to make sure
we're doing this right, but my initial evaluation is done by assuming,
for the sake of argument, that we're using a 256-byte page size (as that
is most likely to be unaligned to a 512-byte boundary).  I see that we
can call into nand_blk_load_256() during processing of
NAND_CMD_RANDOMREAD2 from nand_command(), although it gets harder to
audit without an actual call trace whether addr and offset have 512-byte
alignments at that point.  But without analysis of whether this is an
actual possibility, let's assume we can somehow trigger a code path that
calls nand_blk_load_256(s, 768, 1).

Before 9fc0d361cc41, this called:

            if (blk_read(s->blk, PAGE_START(addr) >> 9,
                         s->io, (PAGE_SECTORS + 2)) < 0) {

or, tracing macro-expansion

blk_read(s->blk, (PAGE(addr) * (PAGE_SIZE + OOB_SIZE)) >> 9,
                  s->io, (1 + 2))

blk_read(s->blk, (((addr) >> 8) * (256 + (1 << (PAGE_SHIFT - 5)))) >> 9,
                  s->io, (1 + 2))

blk_read(s->blk, (((addr) >> 8) * (256 + 8)) >> 9,
                  s->io, 3)


which, for our given addr of 768, results in
blk_read(s->blk, 792 >> 9, s->io, 3)

or the 1536 bytes starting at offset 512 (which includes offset 792 that
we are interested in).

And indeed, post-patch, it now results in trying to read 1536 bytes, but
starting at offset 792 rather than offset 512.

So it looks like the fix is correct, once it is spelled in a more
obvious way rather than with a magic number, and that the fact that this
has been broken since 2.7 means that this device is not getting much
actual testing :(

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] hw/block/nand: Fix bad offset in nand_blk_load for on-drive OOB

[Karl Beldan]

On Mon, Apr 23, 2018 at 05:14:03PM -0500, Eric Blake wrote:
> On 04/23/2018 04:14 PM, Karl Beldan wrote:
> > The logic wants 512-byte aligned blk ops.
> 
> The commit you are referencing mentions that the code permits 256, 512,
> or 2048-byte alignment, based on the configuration of the hardware it is
> emulating.  The whole file is hard to read, and I'm not surprised if
> what I thought was a patch with no semantic change didn't quite succeed.
> 
> > To switch to byte-based block accesses, the fixed commit changed the
> > blk read offset,
> > 	PAGE_START(addr) >> 9
> > with
> > 	PAGE_START(addr)
> > which min alignment, for on-drive OOB, is the min OOB size.
> > Consequently the reads are offset by PAGE_START(addr) & 0x1ff.
> 
> Do you have a call trace where a caller is passing in an addr that is
> not aligned to 512?  I agree that the old code was 512-aligned by virtue

PAGE(addr)       is ((addr) >> ADDR_SHIFT)
PAGE_START(page) is (PAGE(page) * (PAGE_SIZE + OOB_SIZE))

Take the 2nd page, i.e page 1.


> of the old interface; but the new code SHOULD be able to call into the
> block layer with whatever alignment it needs, where the block layer will
> do a larger read to satisfy block constraints, as needed (that is, if a
> caller requests a 256-byte read of a device that requires 512-alignment,
> blk_pread() will automatically widen out to a 512-byte read).
> 

Indeed the block layer handles said alignements, but then you'd have to
not add the offset twice. Keeping the logic homogenous seemed natural.


> > 
> > Fixes: 9fc0d361cc41 ("nand: Switch to byte-based block access")
> > Cc: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Karl Beldan <karl.beldan+oss@gmail.com>
> 
> CC: qemu-stable@nongnu.org
> 
> > ---
> >  hw/block/nand.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/nand.c b/hw/block/nand.c
> > index 919cb9b803..ed587f60f0 100644
> > --- a/hw/block/nand.c
> > +++ b/hw/block/nand.c
> > @@ -788,7 +788,7 @@ static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState *s,
> >                              OOB_SIZE);
> >              s->ioaddr = s->io + SECTOR_OFFSET(s->addr) + offset;
> >          } else {
> > -            if (blk_pread(s->blk, PAGE_START(addr), s->io,
> > +            if (blk_pread(s->blk, PAGE_START(addr) & ~0x1ff, s->io,
> >                            (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS) < 0) {
> 
> I don't like the magic number masking.  This should probably be written
> QEMU_ALIGN_DOWN(PAGE_START(addr), BDRV_SECTOR_SIZE).
> 

There are many other occurences in this file of said "magic" number.
In general or for ulterior "rework" of the driver everybody would agree
on this , however doing that for _this_ fix would convey another intent
as some other places in the code where this sector size mask is used,
which isn't the case, so I'd very much keep it homogenous/coherent.

> I would like to see some actual numbers from a backtrace to make sure
> we're doing this right, but my initial evaluation is done by assuming,
> for the sake of argument, that we're using a 256-byte page size (as that
> is most likely to be unaligned to a 512-byte boundary).  I see that we
> can call into nand_blk_load_256() during processing of
> NAND_CMD_RANDOMREAD2 from nand_command(), although it gets harder to
> audit without an actual call trace whether addr and offset have 512-byte
> alignments at that point.  But without analysis of whether this is an
> actual possibility, let's assume we can somehow trigger a code path that
> calls nand_blk_load_256(s, 768, 1).
> 
> Before 9fc0d361cc41, this called:
> 
>             if (blk_read(s->blk, PAGE_START(addr) >> 9,
>                          s->io, (PAGE_SECTORS + 2)) < 0) {
> 
> or, tracing macro-expansion
> 
> blk_read(s->blk, (PAGE(addr) * (PAGE_SIZE + OOB_SIZE)) >> 9,
>                   s->io, (1 + 2))
> 
> blk_read(s->blk, (((addr) >> 8) * (256 + (1 << (PAGE_SHIFT - 5)))) >> 9,
>                   s->io, (1 + 2))
> 
> blk_read(s->blk, (((addr) >> 8) * (256 + 8)) >> 9,
>                   s->io, 3)
> 
> 
> which, for our given addr of 768, results in
> blk_read(s->blk, 792 >> 9, s->io, 3)
> 
> or the 1536 bytes starting at offset 512 (which includes offset 792 that
> we are interested in).
> 
> And indeed, post-patch, it now results in trying to read 1536 bytes, but
> starting at offset 792 rather than offset 512.
> 
> So it looks like the fix is correct, once it is spelled in a more
> obvious way rather than with a magic number, and that the fact that this
> has been broken since 2.7 means that this device is not getting much
> actual testing :(
> 

Still on my initial remark wrt magic.

The pitfall becomes apparent very quickly, but only happens for on-disk
OOB, and Linux/nandsim is a hard contender.


Regards, 
Karl