| Message ID | 20230615030837.8518-3-schmitzmic@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Amiga RDB partition support fixes | expand |
On Thu, Jun 15, 2023 at 03:08:36PM +1200, Michael Schmitz wrote: > +/* MSch 20230615: any field used by the Linux kernel must be > + * annotated __be32! If any fields require increase to 64 > + * bit size, rdb_ID _must_ be changed! > + */ This is a really weird comment. If you change on-disk format it is a different format and needs to be marked as such, sure. And as far as I can tell everything that is a __u32 here should be an __be32 because it is a big endian on-disk format. Why would you change only a single field?
Hi Christoph, thanks for your comments! Am 15.06.2023 um 16:17 schrieb Christoph Hellwig: > On Thu, Jun 15, 2023 at 03:08:36PM +1200, Michael Schmitz wrote: >> +/* MSch 20230615: any field used by the Linux kernel must be >> + * annotated __be32! If any fields require increase to 64 >> + * bit size, rdb_ID _must_ be changed! >> + */ > > This is a really weird comment. If you change on-disk format it > is a different format and needs to be marked as such, sure. I concede this isn't relevant to the matter at hand, and it's something that _should_ be obvious. This came up in my off-list discussion with Joanne Dow as a caveat for future huge disk sizes, so I thought I'd add it to the comment. > And as far as I can tell everything that is a __u32 here should > be an __be32 because it is a big endian on-disk format. Why > would you change only a single field? Because that's all I needed, and wanted to avoid excess patch churn. Plus (appeal to authority here :-)) it's in keeping with what Al Viro did when the __be32 annotations were first added. I can change all __u32 to __be32 and drop the comment if that's preferred. Cheers, Michael
On Thu, Jun 15, 2023 at 04:50:45PM +1200, Michael Schmitz wrote: >> And as far as I can tell everything that is a __u32 here should >> be an __be32 because it is a big endian on-disk format. Why >> would you change only a single field? > > Because that's all I needed, and wanted to avoid excess patch churn. Plus > (appeal to authority here :-)) it's in keeping with what Al Viro did when > the __be32 annotations were first added. > > I can change all __u32 to __be32 and drop the comment if that's preferred. That would be great!
Hi Christoph, Am 15.06.2023 um 17:53 schrieb Christoph Hellwig: > On Thu, Jun 15, 2023 at 04:50:45PM +1200, Michael Schmitz wrote: >>> And as far as I can tell everything that is a __u32 here should >>> be an __be32 because it is a big endian on-disk format. Why >>> would you change only a single field? >> >> Because that's all I needed, and wanted to avoid excess patch churn. Plus >> (appeal to authority here :-)) it's in keeping with what Al Viro did when >> the __be32 annotations were first added. >> >> I can change all __u32 to __be32 and drop the comment if that's preferred. > > That would be great! OK, will respin tomorrow. Cheers, Michael
Hi Michael, Thanks for your patch! On Thu, Jun 15, 2023 at 7:53 AM Christoph Hellwig <hch@lst.de> wrote: > On Thu, Jun 15, 2023 at 04:50:45PM +1200, Michael Schmitz wrote: > >> And as far as I can tell everything that is a __u32 here should > >> be an __be32 because it is a big endian on-disk format. Why > >> would you change only a single field? > > > > Because that's all I needed, and wanted to avoid excess patch churn. Plus > > (appeal to authority here :-)) it's in keeping with what Al Viro did when > > the __be32 annotations were first added. > > > > I can change all __u32 to __be32 and drop the comment if that's preferred. > > That would be great! I totally agree with Christoph. Gr{oetje,eeting}s, Geert
Hi Geert, On 15/06/23 19:21, Geert Uytterhoeven wrote: > Hi Michael, > > Thanks for your patch! > > On Thu, Jun 15, 2023 at 7:53 AM Christoph Hellwig <hch@lst.de> wrote: >> On Thu, Jun 15, 2023 at 04:50:45PM +1200, Michael Schmitz wrote: >>>> And as far as I can tell everything that is a __u32 here should >>>> be an __be32 because it is a big endian on-disk format. Why >>>> would you change only a single field? >>> Because that's all I needed, and wanted to avoid excess patch churn. Plus >>> (appeal to authority here :-)) it's in keeping with what Al Viro did when >>> the __be32 annotations were first added. >>> >>> I can change all __u32 to __be32 and drop the comment if that's preferred. >> That would be great! > I totally agree with Christoph. Thanks - now there's two __s32 fields in that header - one checksum each for RDB and PB. No one has so far seen the need for a 'signed big endian 32 bit' type, and I'd rather avoid adding one to types.h. I'll leave those as they are (with the tacit understanding that they are equally meant to be big endian). Cheers, Michael > Gr{oetje,eeting}s, > > Geert >
On Fri, Jun 16, 2023 at 07:53:11AM +1200, Michael Schmitz wrote: > Thanks - now there's two __s32 fields in that header - one checksum each > for RDB and PB. No one has so far seen the need for a 'signed big endian 32 > bit' type, and I'd rather avoid adding one to types.h. I'll leave those as > they are (with the tacit understanding that they are equally meant to be > big endian). We have those in a few other pleases and store them as __be32 as well. The (implicit) cast to s32 will make them signed again.
Hi Christoph, Am 16.06.2023 um 17:48 schrieb Christoph Hellwig: > On Fri, Jun 16, 2023 at 07:53:11AM +1200, Michael Schmitz wrote: >> Thanks - now there's two __s32 fields in that header - one checksum each >> for RDB and PB. No one has so far seen the need for a 'signed big endian 32 >> bit' type, and I'd rather avoid adding one to types.h. I'll leave those as >> they are (with the tacit understanding that they are equally meant to be >> big endian). > > We have those in a few other pleases and store them as __be32 as well. The > (implicit) cast to s32 will make them signed again. Where's that cast to s32 hidden? I've only seen #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) which would make the checksums unsigned if __be32 was used. Whether the checksum code uses signed or unsigned math would require inspection of the Amiga partitioning tool source which I don't have, so I've kept __s32 to be safe. Thanks for reviewing v11! Cheers, Michael
On Fri, Jun 16, 2023 at 07:20:52PM +1200, Michael Schmitz wrote: > Am 16.06.2023 um 17:48 schrieb Christoph Hellwig: >> On Fri, Jun 16, 2023 at 07:53:11AM +1200, Michael Schmitz wrote: >>> Thanks - now there's two __s32 fields in that header - one checksum each >>> for RDB and PB. No one has so far seen the need for a 'signed big endian 32 >>> bit' type, and I'd rather avoid adding one to types.h. I'll leave those as >>> they are (with the tacit understanding that they are equally meant to be >>> big endian). >> >> We have those in a few other pleases and store them as __be32 as well. The >> (implicit) cast to s32 will make them signed again. > > Where's that cast to s32 hidden? I've only seen > > #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) > > which would make the checksums unsigned if __be32 was used. > > Whether the checksum code uses signed or unsigned math would require > inspection of the Amiga partitioning tool source which I don't have, so > I've kept __s32 to be safe. Well, the return value of be32_to_cpu is going to be a assigned to a, presumably signed, variable. With that you get an implicit cast.
Hi Michael, On Fri, Jun 16, 2023 at 9:21 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > Am 16.06.2023 um 17:48 schrieb Christoph Hellwig: > > On Fri, Jun 16, 2023 at 07:53:11AM +1200, Michael Schmitz wrote: > >> Thanks - now there's two __s32 fields in that header - one checksum each > >> for RDB and PB. No one has so far seen the need for a 'signed big endian 32 > >> bit' type, and I'd rather avoid adding one to types.h. I'll leave those as > >> they are (with the tacit understanding that they are equally meant to be > >> big endian). > > > > We have those in a few other pleases and store them as __be32 as well. The > > (implicit) cast to s32 will make them signed again. > > Where's that cast to s32 hidden? I've only seen > > #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) > > which would make the checksums unsigned if __be32 was used. > > Whether the checksum code uses signed or unsigned math would require > inspection of the Amiga partitioning tool source which I don't have, so > I've kept __s32 to be safe. Unsurprisingly, block/partitions/amiga.c:checksum_block() calculates a checksum over __be32 words. The actual signedness of the checksum field doesn't matter much[*], as using two-complement numbers, you can just assign a signed value to an unsigned field. It should definitely be __be32. [*] I guess it was made signed because the procedure to update the check goes like this: 1. set checksum field to zero, 2. calculate sum, 3. store negated sum in checksum field. Gr{oetje,eeting}s, Geert
Hi Geert, Am 16.06.2023 um 19:28 schrieb Geert Uytterhoeven: > Hi Michael, > > On Fri, Jun 16, 2023 at 9:21 AM Michael Schmitz <schmitzmic@gmail.com> wrote: >> Am 16.06.2023 um 17:48 schrieb Christoph Hellwig: >>> On Fri, Jun 16, 2023 at 07:53:11AM +1200, Michael Schmitz wrote: >>>> Thanks - now there's two __s32 fields in that header - one checksum each >>>> for RDB and PB. No one has so far seen the need for a 'signed big endian 32 >>>> bit' type, and I'd rather avoid adding one to types.h. I'll leave those as >>>> they are (with the tacit understanding that they are equally meant to be >>>> big endian). >>> >>> We have those in a few other pleases and store them as __be32 as well. The >>> (implicit) cast to s32 will make them signed again. >> >> Where's that cast to s32 hidden? I've only seen >> >> #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) >> >> which would make the checksums unsigned if __be32 was used. >> >> Whether the checksum code uses signed or unsigned math would require >> inspection of the Amiga partitioning tool source which I don't have, so >> I've kept __s32 to be safe. > > Unsurprisingly, block/partitions/amiga.c:checksum_block() calculates > a checksum over __be32 words. The actual signedness of the checksum > field doesn't matter much[*], as using two-complement numbers, you can > just assign a signed value to an unsigned field. > It should definitely be __be32. > > [*] I guess it was made signed because the procedure to update the > check goes like this: > 1. set checksum field to zero, > 2. calculate sum, > 3. store negated sum in checksum field. Thanks, that explains why the result of checksum_block() is tested against zero. Makes sense now. Will fix in v12 ... Cheers, Michael > > Gr{oetje,eeting}s, > > Geert >
Hi Christoph, Am 16.06.2023 um 19:25 schrieb Christoph Hellwig: >>> We have those in a few other pleases and store them as __be32 as well. The >>> (implicit) cast to s32 will make them signed again. >> >> Where's that cast to s32 hidden? I've only seen >> >> #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) >> >> which would make the checksums unsigned if __be32 was used. >> >> Whether the checksum code uses signed or unsigned math would require >> inspection of the Amiga partitioning tool source which I don't have, so >> I've kept __s32 to be safe. > > Well, the return value of be32_to_cpu is going to be a assigned to a, > presumably signed, variable. With that you get an implicit cast. I see - as Geert explained, signed or unsigned doesn't matter for the type of checksum algorithm used in this case, so there is no potential for confusion once you think about it (which I didn't, obviously). Cheers, Michael
diff --git a/include/uapi/linux/affs_hardblocks.h b/include/uapi/linux/affs_hardblocks.h index 5e2fb8481252..9da5bc607939 100644 --- a/include/uapi/linux/affs_hardblocks.h +++ b/include/uapi/linux/affs_hardblocks.h @@ -6,6 +6,11 @@ /* Just the needed definitions for the RDB of an Amiga HD. */ +/* MSch 20230615: any field used by the Linux kernel must be + * annotated __be32! If any fields require increase to 64 + * bit size, rdb_ID _must_ be changed! + */ + struct RigidDiskBlock { __u32 rdb_ID; __be32 rdb_SummedLongs; @@ -32,7 +37,7 @@ struct RigidDiskBlock { __u32 rdb_RDBBlocksHi; __u32 rdb_LoCylinder; __u32 rdb_HiCylinder; - __u32 rdb_CylBlocks; + __be32 rdb_CylBlocks; __u32 rdb_AutoParkSeconds; __u32 rdb_HighRDSKBlock; __u32 rdb_Reserved4;
The Amiga partition parser module uses signed int for partition sector address and count, which will overflow for disks larger than 1 TB. Use u64 as type for sector address and size to allow using disks up to 2 TB without LBD support, and disks larger than 2 TB with LBD. The RBD format allows to specify disk sizes up to 2^128 bytes (though native OS limitations reduce this somewhat, to max 2^68 bytes), so check for u64 overflow carefully to protect against overflowing sector_t. This bug was reported originally in 2012, and the fix was created by the RDB author, Joanne Dow <jdow@earthlink.net>. A patch had been discussed and reviewed on linux-m68k at that time but never officially submitted (now resubmitted as patch 1 of this series). Patch 3 (this series) adds additional error checking and warning messages. One of the error checks now makes use of the previously unused rdb_CylBlocks field, which causes a 'sparse' warning (cast to restricted __be32). Annotate rdb_CylBlocks field as big endian to shut up the warning. Add comment to document that the same annotation is going to be needed for any other fields that may be used by the kernel in future. Reported-by: Martin Steigerwald <Martin@lichtvoll.de> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=43511 Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Message-ID: <201206192146.09327.Martin@lichtvoll.de> Cc: <stable@vger.kernel.org> # 5.2 Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> --- include/uapi/linux/affs_hardblocks.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)