Re: [PATCH v3] block: bugfix for Amiga partition overflow check patch

Geert Uytterhoeven
2023-07-05T07:25:08Z

Hi Michael,

On Tue, Jul 4, 2023 at 9:30 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
> On 4/07/23 19:20, Geert Uytterhoeven wrote:
> > On Tue, Jul 4, 2023 at 7:50 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> >> Making 'blk' sector_t (i.e. 64 bit if LBD support is active)
> >> fails the 'blk>0' test in the partition block loop if a
> >> value of (signed int) -1 is used to mark the end of the
> >> partition block list.
> >>
> >> This bug was introduced in patch 3 of my prior Amiga partition
> >> support fixes series, and spotted by Christian Zigotzky when
> >> testing the latest block updates.
> >>
> >> Explicitly cast 'blk' to signed int to allow use of -1 to
> >> terminate the partition block linked list.
> > That's the explanation for what this patch does.
> >
> > The below is not directly related to that, so IMHO it does not
> > belong in the description of this patch.
>
> Yes, I realize that. I had hoped that by way of the Fixes: tag, people
> would be able to relate that comment to the correct commit. Might be a
> little circuitous ...
>
> > We do not really have a way to record comments in git history
> > after the fact. The best you can do is to reply to the email thread
> > where the patch was submitted. When people follow the Link:
> > tag to the lore archive in the original commit, they can read any follow-ups.
>
> Does lore pick up related patches through the In-Reply-To header? In
> that case it would be easiest for me to to put this comment in a cover
> letter to the bugfix patch.

Lore does not do that (b4 (the tool to download patch series from lore)
usually can link a series to its previous version, though).
New replies sent to a patch submission do end up in the right thread,
so any later comments (bug reports, Reviewed/Tested-by tags, ...)
can be found easily by following the Link: tag in the commit.

> >> Testing by Christian also exposed another aspect of the old
> >> bug fixed in commits fc3d092c6b ("block: fix signed int
> >> overflow in Amiga partition support") and b6f3f28f60
> >> ("block: add overflow checks for Amiga partition support"):
> >>
> >> Partitions that did overflow the disk size (due to 32 bit int
> >> overflow) were not skipped but truncated to the end of the
> >> disk. Users who missed the warning message during boot would
> > I am confused. So before, the partition size as seen by Linux after
> > the truncation, was correct?
>
> No, it was incorrect (though valid).
>
> On a 2 TB disk, a partition of 1.3 TB at the end of the disk (but not
> extending to the very end!) would trigger a overflow in the size
> calculation:
>
> sda: p4 size 18446744071956107760 extends beyond EOD,

Oh, so they were not "truncated to the end of the disk"?

> That's only noted somewhere inside put_partition. The effective
> partition size seen by the kernel and user tools is then that of a
> partition extending to EOD (in Christian's case a full 8 GB more than
> recorded in the partition table).
>
> >> go on to create a filesystem with a size exceeding the
> >> actual partition size. Now that the 32 bit overflow has been
> > But if Linux did see the correct size, mkfs would have used the correct
> > size, too, and the size in the recorded file system should be correct?
>
> mkfs used what the old kernel code gave as partition size. That did
> 'seem' correct at that time, but after the overflow fixes (which prevent
> other partition miscalculations, which in Martin's case caused
> partitions to overlap), the partitions size is actually correct and
> smaller than the filesystem size.
>
> I have a hunch I don't explain myself very well.

;-)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds