Re: git: d464a7698de8 - main - ffs: restore backward compatibility of newfs and makefs with older binaries
Date: Tue, 30 May 2023 02:35:12 UTC
On 30 May 2023, at 03:28, Chuck Silvers <chs@FreeBSD.org> wrote:
>
> The branch main has been updated by chs:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=d464a7698de8fe18530ea65fac90dce56b860a59
>
> 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
>
> ffs: restore backward compatibility of newfs and makefs with older binaries
>
> 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(-)
>
> 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)) == 0)
>
> +/*
> + * 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 = minfpg;
> sblock.fs_ipg = 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 -= sblock.fs_fsize;
> }
> @@ -388,9 +406,11 @@ retry:
> if (Oflag > 1 || (Oflag == 1 && sblock.fs_ipg <= 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) == (unsigned long)sblock.fs_bsize)
> + if (CGSIZE(&sblock) == (unsigned long)sblock.fs_bsize -
> + CGSIZEFUDGE)
> break;
> }
> sblock.fs_fpg -= 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)) == 0)
>
> +/*
> + * 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 = minfpg;
> sblock.fs_ipg = 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 -= 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) == (unsigned long)sblock.fs_bsize)
> + if (CGSIZE(&sblock) == (unsigned long)sblock.fs_bsize -
> + CGSIZEFUDGE)
> break;
> sblock.fs_fpg -= sblock.fs_frag;
> sblock.fs_ipg = roundup(howmany(sblock.fs_fpg, fragsperinode),