Currently stripe unit/swidth checking logic is all over xfsprogs.
So, refactor the same code snippet into a single validation helper
xfs_validate_stripe_factors(), including:
- integer overflows of either value
- sunit and swidth alignment wrt sector size
- if either sunit or swidth are zero, both should be zero
- swidth must be a multiple of sunit
Signed-off-by: Gao Xiang <hsiangkao@xxxxxxxxxx>
---
changes since v2:
- try to cover xfs_validate_sb_common() case as well suggested by Eric,
so move to libxfs as an attempt for review;
- use an static inline helper in xfs_sb.h so compilers can do their
best to eliminate unneeded branches / rearrange code according to
each individual caller;
- get back to use "int error" expression since it's compatible with
"enum xfs_stripeval" and it can be laterly reused for future uses
in these callers instead of introducing another error code variables;
just for review/comments for now, if it looks almost fine, I'd like to
go further to stage up related functions to kernel.
libfrog/topology.c | 15 +++++++++++
libxfs/xfs_sb.c | 32 ++++++++++++++++--------
libxfs/xfs_sb.h | 54 ++++++++++++++++++++++++++++++++++++++++
mkfs/xfs_mkfs.c | 62 ++++++++++++++++++++++++----------------------
po/pl.po | 4 +--
5 files changed, 124 insertions(+), 43 deletions(-)
diff --git a/libfrog/topology.c b/libfrog/topology.c
index b1b470c9..554afbfc 100644
--- a/libfrog/topology.c
+++ b/libfrog/topology.c
@@ -187,6 +187,7 @@ static void blkid_get_topology(
blkid_probe pr;
unsigned long val;
struct stat statbuf;
+ int error;
/* can't get topology info from a file */
if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode)) {
@@ -230,6 +231,20 @@ static void blkid_get_topology(
*sunit = *sunit >> 9;
*swidth = *swidth >> 9;
+ error = xfs_validate_stripe_factors(0, sunit, swidth);
+ if (error) {
+ fprintf(stderr,
+_("%s: Volume reports invalid sunit (%d bytes) and swidth (%d bytes) %s, ignoring.\n"),
+ progname, BBTOB(*sunit), BBTOB(*swidth),
+ xfs_stripeval_str[error]);
+ /*
+ * if firmware is broken, just give up and set both to zero,
+ * we can't trust information from this device.
+ */
+ *sunit = 0;
+ *swidth = 0;
+ }
+
if (blkid_topology_get_alignment_offset(tp) != 0) {
fprintf(stderr,
_("warning: device is not properly aligned %s\n"),
diff --git a/libxfs/xfs_sb.c b/libxfs/xfs_sb.c
index d37d60b3..0c0f5f90 100644
--- a/libxfs/xfs_sb.c
+++ b/libxfs/xfs_sb.c
@@ -210,6 +210,15 @@ xfs_validate_sb_write(
return 0;
}
+const char *xfs_stripeval_str[] = {
+ "OK",
+ "SUNIT_MISALIGN",
+ "SWIDTH_OVERFLOW",
+ "PARTIAL_VALID",
+ "SUNIT_TOO_LARGE",
+ "SWIDTH_MISALIGN",
+};
+
/* Check the validity of the SB. */
STATIC int
xfs_validate_sb_common(
@@ -220,6 +229,8 @@ xfs_validate_sb_common(
struct xfs_dsb *dsb = bp->b_addr;
uint32_t agcount = 0;
uint32_t rem;
+ int sunit, swidth;
+ int error;
if (!xfs_verify_magic(bp, dsb->sb_magicnum)) {
xfs_warn(mp, "bad magic number");
@@ -357,21 +368,20 @@ xfs_validate_sb_common(
}
}
- if (sbp->sb_unit) {
- if (!xfs_sb_version_hasdalign(sbp) ||
- sbp->sb_unit > sbp->sb_width ||
- (sbp->sb_width % sbp->sb_unit) != 0) {
- xfs_notice(mp, "SB stripe unit sanity check failed");
- return -EFSCORRUPTED;
- }
- } else if (xfs_sb_version_hasdalign(sbp)) {
+ sunit = sbp->sb_unit;
+ swidth = sbp->sb_width;
+
+ if (!sunit ^ !xfs_sb_version_hasdalign(sbp)) {
xfs_notice(mp, "SB stripe alignment sanity check failed");
return -EFSCORRUPTED;
- } else if (sbp->sb_width) {
- xfs_notice(mp, "SB stripe width sanity check failed");
- return -EFSCORRUPTED;
}
+ error = xfs_validate_stripe_factors(0, &sunit, &swidth);
+ if (error) {
+ xfs_notice(mp, "SB stripe sanity check failed %s",
+ xfs_stripeval_str[error]);
+ return -EFSCORRUPTED;
+ }
if (xfs_sb_version_hascrc(&mp->m_sb) &&
sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) {
diff --git a/libxfs/xfs_sb.h b/libxfs/xfs_sb.h
index 92465a9a..c4524bbc 100644
--- a/libxfs/xfs_sb.h
+++ b/libxfs/xfs_sb.h
@@ -41,5 +41,59 @@ extern int xfs_sb_read_secondary(struct xfs_mount *mp,
extern int xfs_sb_get_secondary(struct xfs_mount *mp,
struct xfs_trans *tp, xfs_agnumber_t agno,
struct xfs_buf **bpp);
+enum xfs_stripeval {
+ XFS_STRIPEVAL_OK = 0,
+ XFS_STRIPEVAL_SUNIT_MISALIGN,
+ XFS_STRIPEVAL_SWIDTH_OVERFLOW,
+ XFS_STRIPEVAL_PARTIAL_VALID,
+ XFS_STRIPEVAL_SUNIT_TOO_LARGE,
+ XFS_STRIPEVAL_SWIDTH_MISALIGN,
+};
+
+extern const char *xfs_stripeval_str[];
+
+/*
+ * This accepts either
+ * - (sectersize != 0) dsu (in bytes) / dsw (which is multiplier of dsu)
+ * or
+ * - (sectersize == 0) sunit / swidth (in 512B sectors)
+ * and return sunit/swidth in sectors.
+ */
+static inline enum xfs_stripeval
+xfs_validate_stripe_factors(
+ int sectorsize,
+ int *sunitp,
+ int *swidthp)
+{
+ int sunit = *sunitp;
+ int swidth = *swidthp;
+
+ if (sectorsize) {
+ long long big_swidth;
+
+ if (sunit % sectorsize)
+ return XFS_STRIPEVAL_SUNIT_MISALIGN;
+
+ sunit = (int)BTOBBT(sunit);
+ big_swidth = (long long)sunit * swidth;
+
+ if (big_swidth > INT_MAX)
+ return XFS_STRIPEVAL_SWIDTH_OVERFLOW;
+ swidth = big_swidth;
+ }
+
+ if ((sunit && !swidth) || (!sunit && swidth))
+ return XFS_STRIPEVAL_PARTIAL_VALID;
+
+ if (sunit > swidth)
+ return XFS_STRIPEVAL_SUNIT_TOO_LARGE;
+
+ if (sunit && (swidth % sunit))
+ return XFS_STRIPEVAL_SWIDTH_MISALIGN;
+
+ *sunitp = sunit;
+ *swidthp = swidth;
+ return XFS_STRIPEVAL_OK;
+}
#endif /* __XFS_SB_H__ */
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 2e6cd280..8fdf17d7 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2255,7 +2255,6 @@ calc_stripe_factors(
struct cli_params *cli,
struct fs_topology *ft)
{
- long long int big_dswidth;
int dsunit = 0;
int dswidth = 0;
int lsunit = 0;
@@ -2263,6 +2262,7 @@ calc_stripe_factors(
int dsw = 0;
int lsu = 0;
bool use_dev = false;
+ int error;
if (cli_opt_set(&dopts, D_SUNIT))
dsunit = cli->dsunit;
@@ -2289,29 +2289,40 @@ _("both data su and data sw options must be specified\n"));
usage();
}
- if (dsu % cfg->sectorsize) {
- fprintf(stderr,
-_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
- usage();
- }
-
- dsunit = (int)BTOBBT(dsu);
- big_dswidth = (long long int)dsunit * dsw;
- if (big_dswidth > INT_MAX) {
- fprintf(stderr,
-_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
- big_dswidth, dsunit);
- usage();
- }
- dswidth = big_dswidth;
+ dsunit = dsu;
+ dswidth = dsw;
+ error = xfs_validate_stripe_factors(cfg->sectorsize,
+ &dsunit, &dswidth);
+ } else {
+ error = xfs_validate_stripe_factors(0, &dsunit, &dswidth);
}
- if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
- (dsunit && (dswidth % dsunit != 0))) {
+ switch (error) {
+ case XFS_STRIPEVAL_OK:
+ break;
+ case XFS_STRIPEVAL_SUNIT_MISALIGN:
+ fprintf(stderr,
+_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
+ usage();
+ break;
+ case XFS_STRIPEVAL_SWIDTH_OVERFLOW:
+ fprintf(stderr,
+_("data stripe width (%d) is too large of a multiple of the data stripe unit (%d)\n"),
+ dsw, dsunit);
+ usage();
+ break;
+ case XFS_STRIPEVAL_PARTIAL_VALID:
+ case XFS_STRIPEVAL_SWIDTH_MISALIGN:
fprintf(stderr,
_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
dswidth, dsunit);
usage();
+ break;
+ default:
+ fprintf(stderr,
+_("invalid data stripe unit (%d), width (%d) %s\n"),
+ dsunit, dswidth, xfs_stripeval_str[error]);
+ usage();
}
/* If sunit & swidth were manually specified as 0, same as noalign */
@@ -2328,18 +2339,9 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
/* if no stripe config set, use the device default */
if (!dsunit) {
- /* Ignore nonsense from device. XXX add more validation */
- if (ft->dsunit && ft->dswidth == 0) {
- fprintf(stderr,
-_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
- progname, BBTOB(ft->dsunit));
- ft->dsunit = 0;
- ft->dswidth = 0;
- } else {
- dsunit = ft->dsunit;
- dswidth = ft->dswidth;
- use_dev = true;
- }
+ dsunit = ft->dsunit;
+ dswidth = ft->dswidth;
+ use_dev = true;
} else {
/* check and warn if user-specified alignment is sub-optimal */
if (ft->dsunit && ft->dsunit != dsunit) {
diff --git a/po/pl.po b/po/pl.po
index 87109f6b..02d2258f 100644
--- a/po/pl.po
+++ b/po/pl.po
@@ -9085,10 +9085,10 @@ msgstr "su danych musi być wielokrotnością rozmiaru sektora (%d)\n"
#: .././mkfs/xfs_mkfs.c:2267
#, c-format
msgid ""
-"data stripe width (%lld) is too large of a multiple of the data stripe unit "
+"data stripe width (%d) is too large of a multiple of the data stripe unit "
"(%d)\n"
msgstr ""
-"szerokość pasa danych (%lld) jest zbyt dużą wielokrotnością jednostki pasa "
+"szerokość pasa danych (%d) jest zbyt dużą wielokrotnością jednostki pasa "
"danych (%d)\n"
#: .././mkfs/xfs_mkfs.c:2276
--
2.18.1
