| Message ID | 20230704233808.25166-2-schmitzmic@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bugfix for Amiga partition fixes | expand |
On Wed, Jul 5, 2023 at 1:38 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. > > Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de> > Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support") > Message-ID: 024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de Please drop this line. > Cc: <stable@vger.kernel.org> # 5.2 > Link: https://lore.kernel.org/r/024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> > Reviewed-by: Martin Steigerwald <martin@lichtvoll.de> > Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> Gr{oetje,eeting}s, Geert
Hi Geert, thanks for the review! Am 05.07.2023 um 19:28 schrieb Geert Uytterhoeven: > On Wed, Jul 5, 2023 at 1:38 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. >> >> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de> >> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support") >> Message-ID: 024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de > > Please drop this line. Because it's redundant, as I've also used Link:? Cheers, Michael > >> Cc: <stable@vger.kernel.org> # 5.2 >> Link: https://lore.kernel.org/r/024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de >> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> >> Reviewed-by: Martin Steigerwald <martin@lichtvoll.de> >> Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de> > > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Gr{oetje,eeting}s, > > Geert >
Hi Michael, On Wed, Jul 5, 2023 at 10:53 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > Am 05.07.2023 um 19:28 schrieb Geert Uytterhoeven: > > On Wed, Jul 5, 2023 at 1:38 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. > >> > >> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de> > >> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support") > >> Message-ID: 024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de > > > > Please drop this line. > > Because it's redundant, as I've also used Link:? (That, too ;-) Because the use of the Message-ID: tag in patches is not documented. IIRC, it might also cause issues when applying, as the downloaded patch will appear to have two Message-IDs. I'm not sure the sample git hook in Documentation/maintainer/configure-git.rst (and all variants the various maintainers are using) handles this correctly. Gr{oetje,eeting}s, Geert
Hi Geert, On 5/07/23 21:08, Geert Uytterhoeven wrote: > >>>> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support") >>>> Message-ID: 024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de >>> Please drop this line. >> Because it's redundant, as I've also used Link:? > (That, too ;-) > > Because the use of the Message-ID: tag in patches is not documented. Now I wonder where I picked up that habit ... > IIRC, it might also cause issues when applying, as the downloaded patch > will appear to have two Message-IDs. That's correct (if you refer to a patch in mbox format), but from the context of the two Message-ID lines, it ought to be clear which one matters. > I'm not sure the sample git hook in Documentation/maintainer/configure-git.rst > (and all variants the various maintainers are using) handles this correctly. You're right, it won't check for context there. In this particular instance, it won't use my Message-ID tag anyway though (forgot the angle brackets). I'll fix that in v5 later. Cheers, Michael > > Gr{oetje,eeting}s, > > Geert >
On 7/4/23 5:38?PM, Michael Schmitz wrote: > Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de> > Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support") > Cc: <stable@vger.kernel.org> # 5.2 This is confusing - it's being marked for stable, but also labeled as fixing a commit that isn't even a release yet?
Hi Jens, On 6/07/23 08:42, Jens Axboe wrote: > On 7/4/23 5:38?PM, Michael Schmitz wrote: >> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de> >> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support") >> Cc: <stable@vger.kernel.org> # 5.2 > This is confusing - it's being marked for stable, but also labeled as > fixing a commit that isn't even a release yet? True - but as you had pointed out, the commit this fixes is set in stone. How do we ensure this bugfix is picked up as well when the other patches are backported? Does that happen automatically, or do I need to add a Link: tag to the patch being fixed? (Greg didn't seem to object to the Fixes: as such, just to the incorrect version prereq) Cheers, Michael
On 7/5/23 3:41?PM, Michael Schmitz wrote: > Hi Jens, > > On 6/07/23 08:42, Jens Axboe wrote: >> On 7/4/23 5:38?PM, Michael Schmitz wrote: >>> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de> >>> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support") >>> Cc: <stable@vger.kernel.org> # 5.2 >> This is confusing - it's being marked for stable, but also labeled as >> fixing a commit that isn't even a release yet? > > True - but as you had pointed out, the commit this fixes is set in > stone. How do we ensure this bugfix is picked up as well when the > other patches are backported? Does that happen automatically, or do I > need to add a Link: tag to the patch being fixed? This: Cc: <stable@vger.kernel.org> # 5.2 should be enough for it to go into stable from 5.2 and onwards. > (Greg didn't seem to object to the Fixes: as such, just to the > incorrect version prereq) I think it's really confusing... A patch should only have a Fixes tag if it's fixing a specific bug in that patch. Either it is, in which case you would not need Cc stable at all since it's only in 6.5-rc, or it isn't and you should just have the stable tag with 5.2+ as you already have. I'll apply this and remove the Fixes line, and the message id thing too.
Hi Jens, On 6/07/23 09:44, Jens Axboe wrote: > On 7/5/23 3:41?PM, Michael Schmitz wrote: >> Hi Jens, >> >> On 6/07/23 08:42, Jens Axboe wrote: >>> On 7/4/23 5:38?PM, Michael Schmitz wrote: >>>> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de> >>>> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support") >>>> Cc: <stable@vger.kernel.org> # 5.2 >>> This is confusing - it's being marked for stable, but also labeled as >>> fixing a commit that isn't even a release yet? >> True - but as you had pointed out, the commit this fixes is set in >> stone. How do we ensure this bugfix is picked up as well when the >> other patches are backported? Does that happen automatically, or do I >> need to add a Link: tag to the patch being fixed? > This: > > Cc: <stable@vger.kernel.org> # 5.2 > > should be enough for it to go into stable from 5.2 and onwards. OK - I wasn't certain whether you wanted the Fixes or stable tag dropped. >> (Greg didn't seem to object to the Fixes: as such, just to the >> incorrect version prereq) > I think it's really confusing... A patch should only have a Fixes tag if > it's fixing a specific bug in that patch. Either it is, in which case > you would not need Cc stable at all since it's only in 6.5-rc, or it It is fixing a bug in b6f3f28f60. I should have checked whether the patch series had already gone to release, not just -rc, instead of just adding the stable tag out of caution. > isn't and you should just have the stable tag with 5.2+ as you already > have. > > I'll apply this and remove the Fixes line, and the message id thing > too. Thanks - whatever is least confusing is fine, as long as it's backported to stable in the end. Won't be sending v5 then... Cheers, Michael
>> should be enough for it to go into stable from 5.2 and onwards. > OK - I wasn't certain whether you wanted the Fixes or stable tag dropped. >>> (Greg didn't seem to object to the Fixes: as such, just to the >>> incorrect version prereq) >> I think it's really confusing... A patch should only have a Fixes tag if >> it's fixing a specific bug in that patch. Either it is, in which case >> you would not need Cc stable at all since it's only in 6.5-rc, or it > > It is fixing a bug in b6f3f28f60. I should have checked whether the > patch series had already gone to release, not just -rc, instead of > just adding the stable tag out of caution. But this is the confusion - if it's fixing a bug in b6f3f28f60, then why is it marked as needing to get backported much further back, predating that commit?
Hi Jens, On 6/07/23 10:13, Jens Axboe wrote: >>> should be enough for it to go into stable from 5.2 and onwards. >> OK - I wasn't certain whether you wanted the Fixes or stable tag dropped. >>>> (Greg didn't seem to object to the Fixes: as such, just to the >>>> incorrect version prereq) >>> I think it's really confusing... A patch should only have a Fixes tag if >>> it's fixing a specific bug in that patch. Either it is, in which case >>> you would not need Cc stable at all since it's only in 6.5-rc, or it >> It is fixing a bug in b6f3f28f60. I should have checked whether the >> patch series had already gone to release, not just -rc, instead of >> just adding the stable tag out of caution. > But this is the confusion - if it's fixing a bug in b6f3f28f60, then why > is it marked as needing to get backported much further back, predating > that commit? I see - it doesn't need to be backported that far back _alone_. It only needs to be applied after b6f3f28f60 once that one has been backported. Cheers, Michael
On 7/5/23 4:25?PM, Michael Schmitz wrote: > Hi Jens, > > On 6/07/23 10:13, Jens Axboe wrote: >>>> should be enough for it to go into stable from 5.2 and onwards. >>> OK - I wasn't certain whether you wanted the Fixes or stable tag dropped. >>>>> (Greg didn't seem to object to the Fixes: as such, just to the >>>>> incorrect version prereq) >>>> I think it's really confusing... A patch should only have a Fixes tag if >>>> it's fixing a specific bug in that patch. Either it is, in which case >>>> you would not need Cc stable at all since it's only in 6.5-rc, or it >>> It is fixing a bug in b6f3f28f60. I should have checked whether the >>> patch series had already gone to release, not just -rc, instead of >>> just adding the stable tag out of caution. >> But this is the confusion - if it's fixing a bug in b6f3f28f60, then why >> is it marked as needing to get backported much further back, predating >> that commit? > > I see - it doesn't need to be backported that far back _alone_. It > only needs to be applied after b6f3f28f60 once that one has been > backported. OK I see - I think there's some serious misunderstandings here then :-) It sounds like it fixes a bug in b6f3f28f60 alone, and it has no business going into stable. The commit should _just_ be marked with it fixing that. If someone were to backport that previous series, then their tooling or diligence should notice this dependency and this current commit should be picked as well. There should be no Cc: stable on this patch at all, I'll fix it up.
On 7/5/23 4:34?PM, Jens Axboe wrote: > On 7/5/23 4:25?PM, Michael Schmitz wrote: >> Hi Jens, >> >> On 6/07/23 10:13, Jens Axboe wrote: >>>>> should be enough for it to go into stable from 5.2 and onwards. >>>> OK - I wasn't certain whether you wanted the Fixes or stable tag dropped. >>>>>> (Greg didn't seem to object to the Fixes: as such, just to the >>>>>> incorrect version prereq) >>>>> I think it's really confusing... A patch should only have a Fixes tag if >>>>> it's fixing a specific bug in that patch. Either it is, in which case >>>>> you would not need Cc stable at all since it's only in 6.5-rc, or it >>>> It is fixing a bug in b6f3f28f60. I should have checked whether the >>>> patch series had already gone to release, not just -rc, instead of >>>> just adding the stable tag out of caution. >>> But this is the confusion - if it's fixing a bug in b6f3f28f60, then why >>> is it marked as needing to get backported much further back, predating >>> that commit? >> >> I see - it doesn't need to be backported that far back _alone_. It >> only needs to be applied after b6f3f28f60 once that one has been >> backported. > > OK I see - I think there's some serious misunderstandings here then :-) > > It sounds like it fixes a bug in b6f3f28f60 alone, and it has no > business going into stable. The commit should _just_ be marked with it > fixing that. If someone were to backport that previous series, then > their tooling or diligence should notice this dependency and this > current commit should be picked as well. > > There should be no Cc: stable on this patch at all, I'll fix it up. Here's what I have: https://git.kernel.dk/cgit/linux/commit/?h=block-6.5&id=7eb1e47696aa231b1a567846bbe3a1e1befe1854 which has the following manual edits: 1) Change the title/subject line of the patch. "bugfix for Amiga partition overflow check patch" means very little. The fact that this patch is a bug fix for a previous commit is explicit with the Fixes line. 2) Break lines at 72-74 chars, yours were very short. 3) Drop message-id 4) Drop cc stable tag 5) Drop the revision history. This should be behind three '---' lines and then it's dropped automatically. Let's hope this is it for Amiga partition handling!
Hi Jens, thanks for patching this all up! On 6/07/23 10:38, Jens Axboe wrote: > On 7/5/23 4:34?PM, Jens Axboe wrote: >> On 7/5/23 4:25?PM, Michael Schmitz wrote: >>> Hi Jens, >>> >>> On 6/07/23 10:13, Jens Axboe wrote: >>>>>> should be enough for it to go into stable from 5.2 and onwards. >>>>> OK - I wasn't certain whether you wanted the Fixes or stable tag dropped. >>>>>>> (Greg didn't seem to object to the Fixes: as such, just to the >>>>>>> incorrect version prereq) >>>>>> I think it's really confusing... A patch should only have a Fixes tag if >>>>>> it's fixing a specific bug in that patch. Either it is, in which case >>>>>> you would not need Cc stable at all since it's only in 6.5-rc, or it >>>>> It is fixing a bug in b6f3f28f60. I should have checked whether the >>>>> patch series had already gone to release, not just -rc, instead of >>>>> just adding the stable tag out of caution. >>>> But this is the confusion - if it's fixing a bug in b6f3f28f60, then why >>>> is it marked as needing to get backported much further back, predating >>>> that commit? >>> I see - it doesn't need to be backported that far back _alone_. It >>> only needs to be applied after b6f3f28f60 once that one has been >>> backported. >> OK I see - I think there's some serious misunderstandings here then :-) That's probably understating it. I need to reread the patch submission notes. >> It sounds like it fixes a bug in b6f3f28f60 alone, and it has no >> business going into stable. The commit should _just_ be marked with it >> fixing that. If someone were to backport that previous series, then >> their tooling or diligence should notice this dependency and this >> current commit should be picked as well. >> >> There should be no Cc: stable on this patch at all, I'll fix it up. > Here's what I have: > > https://git.kernel.dk/cgit/linux/commit/?h=block-6.5&id=7eb1e47696aa231b1a567846bbe3a1e1befe1854 > > which has the following manual edits: > > 1) Change the title/subject line of the patch. "bugfix for Amiga > partition overflow check patch" means very little. The fact that this > patch is a bug fix for a previous commit is explicit with the Fixes > line. > > 2) Break lines at 72-74 chars, yours were very short. Somehow a limit of 60 had stuck in my mind. > > 3) Drop message-id > > 4) Drop cc stable tag > > 5) Drop the revision history. This should be behind three '---' lines > and then it's dropped automatically. Argh - looks like I've made that mistake more often than not in the past few years. I'll remember that now. > > Let's hope this is it for Amiga partition handling! I should certainly hope so! Cheers, Michael
Michael Schmitz - 06.07.23, 01:54:49 CEST: > Let's hope this is it for Amiga partition handling! > > I should certainly hope so! Yeah, really. For good. Thank you, Michael and Jens!
diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c index ed222b9c901b..506921095412 100644 --- a/block/partitions/amiga.c +++ b/block/partitions/amiga.c @@ -90,7 +90,7 @@ int amiga_partition(struct parsed_partitions *state) } blk = be32_to_cpu(rdb->rdb_PartitionList); put_dev_sector(sect); - for (part = 1; blk>0 && part<=16; part++, put_dev_sector(sect)) { + for (part = 1; (s32) blk>0 && part<=16; part++, put_dev_sector(sect)) { /* Read in terms partition table understands */ if (check_mul_overflow(blk, (sector_t) blksize, &blk)) { pr_err("Dev %s: overflow calculating partition block %llu! Skipping partitions %u and beyond\n",
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. Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support") Message-ID: 024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de Cc: <stable@vger.kernel.org> # 5.2 Link: https://lore.kernel.org/r/024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> Reviewed-by: Martin Steigerwald <martin@lichtvoll.de> Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de> -- Changes since v1: - corrected Fixes: tag - added Tested-by: - reworded commit message to describe filesystem partition size mismatch problem Changes since v2: Adrian Glaubitz: - fix typo in commit message Changes since v3: Greg KH: - fix stable tag Geert Uytterhoeven: - revert changes to commit message since v1 --- block/partitions/amiga.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)