Date: Tue, 30 May 2023 03:35:12 +0100 From: Jessica Clarke <jrtc27@freebsd.org> To: Chuck Silvers <chs@FreeBSD.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: d464a7698de8 - main - ffs: restore backward compatibility of newfs and makefs with older binaries Message-ID: <31C0370D-59E1-47FF-9A27-AB08F19C3F8E@freebsd.org> In-Reply-To: <202305300228.34U2S2QI062128@gitrepo.freebsd.org> References: <202305300228.34U2S2QI062128@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 30 May 2023, at 03:28, Chuck Silvers <chs@FreeBSD.org> wrote: >=20 > The branch main has been updated by chs: >=20 > URL: = https://cgit.FreeBSD.org/src/commit/?id=3Dd464a7698de8fe18530ea65fac90dce5= 6b860a59 >=20 > commit d464a7698de8fe18530ea65fac90dce56b860a59 > Author: Chuck Silvers <chs@FreeBSD.org> > AuthorDate: 2023-05-30 02:26:28 +0000 > Commit: Chuck Silvers <chs@FreeBSD.org> > CommitDate: 2023-05-30 02:26:28 +0000 >=20 > ffs: restore backward compatibility of newfs and makefs with older = binaries >=20 > The previous change to CGSIZE had the unintended side-effect of = allowing > newfs and makefs to create file systems that would fail validation = when > examined by older commands and kernels, by allowing newfs/makefs to = pack > slightly more blocks into a CG than those older binaries think is = valid. > Fix this by having newfs/makefs artificially restrict the number of = blocks > in a CG to the slightly smaller value that those older binaries = will accept. > The validation code will continue to accept the slightly larger = value > that the current newfs/makefs (before this change) could create. Can we ditch this once 13 is EOL? Jess > Fixes: 0a6e34e950cd5889122a199c34519b67569be9cc > Reviewed by: mckusick > MFC after: 3 days > Sponsored by: Netflix > --- > sbin/newfs/mkfs.c | 26 +++++++++++++++++++++++--- > usr.sbin/makefs/ffs/mkfs.c | 26 +++++++++++++++++++++++--- > 2 files changed, 46 insertions(+), 6 deletions(-) >=20 > diff --git a/sbin/newfs/mkfs.c b/sbin/newfs/mkfs.c > index adc64f47cac6..28b02d250e17 100644 > --- a/sbin/newfs/mkfs.c > +++ b/sbin/newfs/mkfs.c > @@ -76,6 +76,23 @@ __FBSDID("$FreeBSD$"); > #define UMASK 0755 > #define POWEROF2(num) (((num) & ((num) - 1)) =3D=3D 0) >=20 > +/* > + * The definition of "struct cg" used to contain an extra field at = the end > + * to represent the variable-length data that followed the fixed = structure. > + * This had the effect of artificially limiting the number of blocks = that > + * newfs would put in a CG, since newfs thought that the fixed-size = header > + * was bigger than it really was. When we started validating that = the CG > + * header data actually fit into one fs block, the placeholder field = caused > + * a problem because it caused struct cg to be a different size = depending on > + * platform. The placeholder field was later removed, but this = caused a > + * backward compatibility problem with older binaries that still = thought > + * struct cg was larger, and a new file system could fail validation = if > + * viewed by the older binaries. To avoid this compatibility = problem, we > + * now artificially reduce the amount of space that the = variable-length data > + * can use such that new file systems will pass validation by older = binaries. > + */ > +#define CGSIZEFUDGE 8 > + > static struct csum *fscs; > #define sblock disk.d_fs > #define acg disk.d_cg > @@ -369,7 +386,8 @@ retry: > sblock.fs_fpg =3D minfpg; > sblock.fs_ipg =3D roundup(howmany(sblock.fs_fpg, fragsperinode), > INOPB(&sblock)); > - if (CGSIZE(&sblock) < (unsigned long)sblock.fs_bsize) > + if (CGSIZE(&sblock) < (unsigned long)sblock.fs_bsize - > + CGSIZEFUDGE) > break; > density -=3D sblock.fs_fsize; > } > @@ -388,9 +406,11 @@ retry: > if (Oflag > 1 || (Oflag =3D=3D 1 && sblock.fs_ipg <=3D 0x7fff)) { > if (sblock.fs_size / sblock.fs_fpg < MINCYLGRPS) > break; > - if (CGSIZE(&sblock) < (unsigned long)sblock.fs_bsize) > + if (CGSIZE(&sblock) < (unsigned long)sblock.fs_bsize - > + CGSIZEFUDGE) > continue; > - if (CGSIZE(&sblock) =3D=3D (unsigned long)sblock.fs_bsize) > + if (CGSIZE(&sblock) =3D=3D (unsigned long)sblock.fs_bsize - > + CGSIZEFUDGE) > break; > } > sblock.fs_fpg -=3D sblock.fs_frag; > diff --git a/usr.sbin/makefs/ffs/mkfs.c b/usr.sbin/makefs/ffs/mkfs.c > index d48dc65aac68..4e88dae7aae8 100644 > --- a/usr.sbin/makefs/ffs/mkfs.c > +++ b/usr.sbin/makefs/ffs/mkfs.c > @@ -80,6 +80,23 @@ static int count_digits(int); > #define UMASK 0755 > #define POWEROF2(num) (((num) & ((num) - 1)) =3D=3D 0) >=20 > +/* > + * The definition of "struct cg" used to contain an extra field at = the end > + * to represent the variable-length data that followed the fixed = structure. > + * This had the effect of artificially limiting the number of blocks = that > + * newfs would put in a CG, since newfs thought that the fixed-size = header > + * was bigger than it really was. When we started validating that = the CG > + * header data actually fit into one fs block, the placeholder field = caused > + * a problem because it caused struct cg to be a different size = depending on > + * platform. The placeholder field was later removed, but this = caused a > + * backward compatibility problem with older binaries that still = thought > + * struct cg was larger, and a new file system could fail validation = if > + * viewed by the older binaries. To avoid this compatibility = problem, we > + * now artificially reduce the amount of space that the = variable-length data > + * can use such that new file systems will pass validation by older = binaries. > + */ > +#define CGSIZEFUDGE 8 > + > static union { > struct fs fs; > char pad[SBLOCKSIZE]; > @@ -347,7 +364,8 @@ ffs_mkfs(const char *fsys, const fsinfo_t *fsopts, = time_t tstamp) > sblock.fs_fpg =3D minfpg; > sblock.fs_ipg =3D roundup(howmany(sblock.fs_fpg, fragsperinode), > INOPB(&sblock)); > - if (CGSIZE(&sblock) < (unsigned long)sblock.fs_bsize) > + if (CGSIZE(&sblock) < (unsigned long)sblock.fs_bsize - > + CGSIZEFUDGE) > break; > density -=3D sblock.fs_fsize; > } > @@ -366,9 +384,11 @@ ffs_mkfs(const char *fsys, const fsinfo_t = *fsopts, time_t tstamp) > INOPB(&sblock)); > if (sblock.fs_size / sblock.fs_fpg < 1) > break; > - if (CGSIZE(&sblock) < (unsigned long)sblock.fs_bsize) > + if (CGSIZE(&sblock) < (unsigned long)sblock.fs_bsize - > + CGSIZEFUDGE) > continue; > - if (CGSIZE(&sblock) =3D=3D (unsigned long)sblock.fs_bsize) > + if (CGSIZE(&sblock) =3D=3D (unsigned long)sblock.fs_bsize - > + CGSIZEFUDGE) > break; > sblock.fs_fpg -=3D sblock.fs_frag; > sblock.fs_ipg =3D roundup(howmany(sblock.fs_fpg, fragsperinode),
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?31C0370D-59E1-47FF-9A27-AB08F19C3F8E>