| Message ID | 20201013040627.13932-4-hsiangkao@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfsprogs: consolidate stripe validation | expand |
On Tue, Oct 13, 2020 at 12:06:27PM +0800, Gao Xiang wrote: > Check stripe numbers in calc_stripe_factors() by using > xfs_validate_stripe_geometry(). > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > libxfs/libxfs_api_defs.h | 1 + > mkfs/xfs_mkfs.c | 23 +++++++---------------- > 2 files changed, 8 insertions(+), 16 deletions(-) > > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h > index e7e42e93..306d0deb 100644 > --- a/libxfs/libxfs_api_defs.h > +++ b/libxfs/libxfs_api_defs.h > @@ -188,6 +188,7 @@ > #define xfs_trans_roll_inode libxfs_trans_roll_inode > #define xfs_trans_roll libxfs_trans_roll > > +#define xfs_validate_stripe_geometry libxfs_validate_stripe_geometry > #define xfs_verify_agbno libxfs_verify_agbno > #define xfs_verify_agino libxfs_verify_agino > #define xfs_verify_cksum libxfs_verify_cksum > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 8fe149d7..aec40c1f 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -2305,12 +2305,6 @@ _("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) { > @@ -2322,13 +2316,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit ( > dswidth = big_dswidth; > } > > - if ((dsunit && !dswidth) || (!dsunit && dswidth) || > - (dsunit && (dswidth % dsunit != 0))) { > - fprintf(stderr, > -_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), > - dswidth, dsunit); > + if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), BBTOB(dswidth), > + cfg->sectorsize, false)) > usage(); > - } > > /* If sunit & swidth were manually specified as 0, same as noalign */ > if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) && > @@ -2344,11 +2334,12 @@ _("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) { > + /* Ignore nonsense from device report. */ > + if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit), > + BBTOB(ft->dswidth), 0, true)) { > fprintf(stderr, > -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"), > - progname, BBTOB(ft->dsunit)); > +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"), > + progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth)); > ft->dsunit = 0; > ft->dswidth = 0; > } else { > -- > 2.18.1 >
On Tue, Oct 13, 2020 at 12:06:27PM +0800, Gao Xiang wrote: > Check stripe numbers in calc_stripe_factors() by using > xfs_validate_stripe_geometry(). > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> Looks good to me, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > libxfs/libxfs_api_defs.h | 1 + > mkfs/xfs_mkfs.c | 23 +++++++---------------- > 2 files changed, 8 insertions(+), 16 deletions(-) > > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h > index e7e42e93..306d0deb 100644 > --- a/libxfs/libxfs_api_defs.h > +++ b/libxfs/libxfs_api_defs.h > @@ -188,6 +188,7 @@ > #define xfs_trans_roll_inode libxfs_trans_roll_inode > #define xfs_trans_roll libxfs_trans_roll > > +#define xfs_validate_stripe_geometry libxfs_validate_stripe_geometry > #define xfs_verify_agbno libxfs_verify_agbno > #define xfs_verify_agino libxfs_verify_agino > #define xfs_verify_cksum libxfs_verify_cksum > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 8fe149d7..aec40c1f 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -2305,12 +2305,6 @@ _("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) { > @@ -2322,13 +2316,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit ( > dswidth = big_dswidth; > } > > - if ((dsunit && !dswidth) || (!dsunit && dswidth) || > - (dsunit && (dswidth % dsunit != 0))) { > - fprintf(stderr, > -_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), > - dswidth, dsunit); > + if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), BBTOB(dswidth), > + cfg->sectorsize, false)) > usage(); > - } > > /* If sunit & swidth were manually specified as 0, same as noalign */ > if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) && > @@ -2344,11 +2334,12 @@ _("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) { > + /* Ignore nonsense from device report. */ > + if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit), > + BBTOB(ft->dswidth), 0, true)) { > fprintf(stderr, > -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"), > - progname, BBTOB(ft->dsunit)); > +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"), > + progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth)); > ft->dsunit = 0; > ft->dswidth = 0; > } else { > -- > 2.18.1 >
On 10/12/20 11:06 PM, Gao Xiang wrote: > Check stripe numbers in calc_stripe_factors() by using > xfs_validate_stripe_geometry(). > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> Hm, unless I have made a mistake, this seems to allow an invalid stripe specification. Without this patch, this fails: # mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0 data su must be a multiple of the sector size (512) With the patch: # mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0 meta-data=/dev/loop0 isize=512 agcount=8, agsize=32768 blks = sectsz=512 attr=2, projid32bit=1 = crc=1 finobt=1, sparse=1, rmapbt=0 = reflink=1 bigtime=0 data = bsize=4096 blocks=262144, imaxpct=25 = sunit=1 swidth=1 blks naming =version 2 bsize=4096 ascii-ci=0, ftype=1 log =internal log bsize=4096 blocks=2560, version=2 = sectsz=512 sunit=1 blks, lazy-count=1 realtime =none extsz=4096 blocks=0, rtextents=0 Discarding blocks...Done. When you are back from holiday, can you check? No big rush. Thanks, -Eric > --- > libxfs/libxfs_api_defs.h | 1 + > mkfs/xfs_mkfs.c | 23 +++++++---------------- > 2 files changed, 8 insertions(+), 16 deletions(-) > > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h > index e7e42e93..306d0deb 100644 > --- a/libxfs/libxfs_api_defs.h > +++ b/libxfs/libxfs_api_defs.h > @@ -188,6 +188,7 @@ > #define xfs_trans_roll_inode libxfs_trans_roll_inode > #define xfs_trans_roll libxfs_trans_roll > > +#define xfs_validate_stripe_geometry libxfs_validate_stripe_geometry > #define xfs_verify_agbno libxfs_verify_agbno > #define xfs_verify_agino libxfs_verify_agino > #define xfs_verify_cksum libxfs_verify_cksum > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 8fe149d7..aec40c1f 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -2305,12 +2305,6 @@ _("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) { > @@ -2322,13 +2316,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit ( > dswidth = big_dswidth; > } > > - if ((dsunit && !dswidth) || (!dsunit && dswidth) || > - (dsunit && (dswidth % dsunit != 0))) { > - fprintf(stderr, > -_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), > - dswidth, dsunit); > + if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), BBTOB(dswidth), > + cfg->sectorsize, false)) > usage(); > - } > > /* If sunit & swidth were manually specified as 0, same as noalign */ > if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) && > @@ -2344,11 +2334,12 @@ _("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) { > + /* Ignore nonsense from device report. */ > + if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit), > + BBTOB(ft->dswidth), 0, true)) { > fprintf(stderr, > -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"), > - progname, BBTOB(ft->dsunit)); > +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"), > + progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth)); > ft->dsunit = 0; > ft->dswidth = 0; > } else { >
Hi Eric, On Mon, Feb 15, 2021 at 07:04:25PM -0600, Eric Sandeen wrote: > On 10/12/20 11:06 PM, Gao Xiang wrote: > > Check stripe numbers in calc_stripe_factors() by using > > xfs_validate_stripe_geometry(). > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > Hm, unless I have made a mistake, this seems to allow an invalid > stripe specification. > > Without this patch, this fails: > > # mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0 > data su must be a multiple of the sector size (512) > > With the patch: > > # mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0 > meta-data=/dev/loop0 isize=512 agcount=8, agsize=32768 blks > = sectsz=512 attr=2, projid32bit=1 > = crc=1 finobt=1, sparse=1, rmapbt=0 > = reflink=1 bigtime=0 > data = bsize=4096 blocks=262144, imaxpct=25 > = sunit=1 swidth=1 blks > naming =version 2 bsize=4096 ascii-ci=0, ftype=1 > log =internal log bsize=4096 blocks=2560, version=2 > = sectsz=512 sunit=1 blks, lazy-count=1 > realtime =none extsz=4096 blocks=0, rtextents=0 > Discarding blocks...Done. > > When you are back from holiday, can you check? No big rush. I'm back from holiday today. I think the problem is in "if (dsu || dsw) {" it turns into "dsunit = (int)BTOBBT(dsu);" anyway, and then if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), BBTOB(dswidth), cfg->sectorsize, false)) so dsu isn't checked with sectorsize in advance before it turns into BB. the fix seems simple though, 1) turn dsunit and dswidth into bytes rather than BB, but I have no idea the range of these 2 varibles, since I saw "if (big_dswidth > INT_MAX) {" but the big_dswidth was also in BB as well, if we turn these into bytes, and such range cannot be guarunteed... 2) recover the previous code snippet and check dsu in advance: if (dsu % cfg->sectorsize) { fprintf(stderr, _("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize); usage(); } btw, do we have some range test about these variables? I could rearrange the code snippet, but I'm not sure if it could introduce some new potential regression as well... Thanks, Gao Xiang > > Thanks, > -Eric > > > --- > > libxfs/libxfs_api_defs.h | 1 + > > mkfs/xfs_mkfs.c | 23 +++++++---------------- > > 2 files changed, 8 insertions(+), 16 deletions(-) > > > > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h > > index e7e42e93..306d0deb 100644 > > --- a/libxfs/libxfs_api_defs.h > > +++ b/libxfs/libxfs_api_defs.h > > @@ -188,6 +188,7 @@ > > #define xfs_trans_roll_inode libxfs_trans_roll_inode > > #define xfs_trans_roll libxfs_trans_roll > > > > +#define xfs_validate_stripe_geometry libxfs_validate_stripe_geometry > > #define xfs_verify_agbno libxfs_verify_agbno > > #define xfs_verify_agino libxfs_verify_agino > > #define xfs_verify_cksum libxfs_verify_cksum > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > > index 8fe149d7..aec40c1f 100644 > > --- a/mkfs/xfs_mkfs.c > > +++ b/mkfs/xfs_mkfs.c > > @@ -2305,12 +2305,6 @@ _("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) { > > @@ -2322,13 +2316,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit ( > > dswidth = big_dswidth; > > } > > > > - if ((dsunit && !dswidth) || (!dsunit && dswidth) || > > - (dsunit && (dswidth % dsunit != 0))) { > > - fprintf(stderr, > > -_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), > > - dswidth, dsunit); > > + if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), BBTOB(dswidth), > > + cfg->sectorsize, false)) > > usage(); > > - } > > > > /* If sunit & swidth were manually specified as 0, same as noalign */ > > if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) && > > @@ -2344,11 +2334,12 @@ _("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) { > > + /* Ignore nonsense from device report. */ > > + if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit), > > + BBTOB(ft->dswidth), 0, true)) { > > fprintf(stderr, > > -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"), > > - progname, BBTOB(ft->dsunit)); > > +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"), > > + progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth)); > > ft->dsunit = 0; > > ft->dswidth = 0; > > } else { > > >
On Thu, Feb 18, 2021 at 10:41:59AM +0800, Gao Xiang wrote: > Hi Eric, > > On Mon, Feb 15, 2021 at 07:04:25PM -0600, Eric Sandeen wrote: > > On 10/12/20 11:06 PM, Gao Xiang wrote: > > > Check stripe numbers in calc_stripe_factors() by using > > > xfs_validate_stripe_geometry(). > > > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > > > Hm, unless I have made a mistake, this seems to allow an invalid > > stripe specification. > > > > Without this patch, this fails: > > > > # mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0 > > data su must be a multiple of the sector size (512) > > > > With the patch: > > > > # mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0 > > meta-data=/dev/loop0 isize=512 agcount=8, agsize=32768 blks > > = sectsz=512 attr=2, projid32bit=1 > > = crc=1 finobt=1, sparse=1, rmapbt=0 > > = reflink=1 bigtime=0 > > data = bsize=4096 blocks=262144, imaxpct=25 > > = sunit=1 swidth=1 blks > > naming =version 2 bsize=4096 ascii-ci=0, ftype=1 > > log =internal log bsize=4096 blocks=2560, version=2 > > = sectsz=512 sunit=1 blks, lazy-count=1 > > realtime =none extsz=4096 blocks=0, rtextents=0 > > Discarding blocks...Done. > > > > When you are back from holiday, can you check? No big rush. > > I'm back from holiday today. I think the problem is in > "if (dsu || dsw) {" it turns into "dsunit = (int)BTOBBT(dsu);" anyway, > and then if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), > BBTOB(dswidth), cfg->sectorsize, false)) > > so dsu isn't checked with sectorsize in advance before it turns into BB. > > the fix seems simple though, > 1) turn dsunit and dswidth into bytes rather than BB, but I have no idea the range of > these 2 varibles, since I saw "if (big_dswidth > INT_MAX) {" but the big_dswidth > was also in BB as well, if we turn these into bytes, and such range cannot be > guarunteed... > 2) recover the previous code snippet and check dsu in advance: > if (dsu % cfg->sectorsize) { > fprintf(stderr, > _("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize); > usage(); > } > > btw, do we have some range test about these variables? I could rearrange the code > snippet, but I'm not sure if it could introduce some new potential regression as well... > > Thanks, > Gao Xiang Or how about applying the following incremental patch, although the maximum dswidth would be smaller I think, but considering libxfs_validate_stripe_geometry() accepts dswidth in 64-bit bytes as well. I think that would be fine. Does that make sense? I've confirmed "# mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0" now report: stripe unit (4097) must be a multiple of the sector size (512) and xfs/191-input-validation passes now... diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index f152d5c7..80405790 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -2361,20 +2361,24 @@ _("both data su and data sw options must be specified\n")); usage(); } - dsunit = (int)BTOBBT(dsu); - big_dswidth = (long long int)dsunit * dsw; + big_dswidth = (long long int)dsu * 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; - } - if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), BBTOB(dswidth), - cfg->sectorsize, false)) + if (!libxfs_validate_stripe_geometry(NULL, dsu, big_dswidth, + cfg->sectorsize, false)) + usage(); + + dsunit = BTOBBT(dsu); + dswidth = BTOBBT(big_dswidth); + } else if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), + BBTOB(dswidth), cfg->sectorsize, false)) { usage(); + } /* If sunit & swidth were manually specified as 0, same as noalign */ if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) &&
On 2/17/21 11:24 PM, Gao Xiang wrote: > On Thu, Feb 18, 2021 at 10:41:59AM +0800, Gao Xiang wrote: >> Hi Eric, >> >> On Mon, Feb 15, 2021 at 07:04:25PM -0600, Eric Sandeen wrote: >>> On 10/12/20 11:06 PM, Gao Xiang wrote: >>>> Check stripe numbers in calc_stripe_factors() by using >>>> xfs_validate_stripe_geometry(). >>>> >>>> Signed-off-by: Gao Xiang <hsiangkao@redhat.com> >>> >>> Hm, unless I have made a mistake, this seems to allow an invalid >>> stripe specification. >>> >>> Without this patch, this fails: >>> >>> # mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0 >>> data su must be a multiple of the sector size (512) >>> >>> With the patch: >>> >>> # mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0 >>> meta-data=/dev/loop0 isize=512 agcount=8, agsize=32768 blks >>> = sectsz=512 attr=2, projid32bit=1 >>> = crc=1 finobt=1, sparse=1, rmapbt=0 >>> = reflink=1 bigtime=0 >>> data = bsize=4096 blocks=262144, imaxpct=25 >>> = sunit=1 swidth=1 blks >>> naming =version 2 bsize=4096 ascii-ci=0, ftype=1 >>> log =internal log bsize=4096 blocks=2560, version=2 >>> = sectsz=512 sunit=1 blks, lazy-count=1 >>> realtime =none extsz=4096 blocks=0, rtextents=0 >>> Discarding blocks...Done. >>> >>> When you are back from holiday, can you check? No big rush. >> >> I'm back from holiday today. I think the problem is in >> "if (dsu || dsw) {" it turns into "dsunit = (int)BTOBBT(dsu);" anyway, >> and then if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), >> BBTOB(dswidth), cfg->sectorsize, false)) >> >> so dsu isn't checked with sectorsize in advance before it turns into BB. >> >> the fix seems simple though, >> 1) turn dsunit and dswidth into bytes rather than BB, but I have no idea the range of >> these 2 varibles, since I saw "if (big_dswidth > INT_MAX) {" but the big_dswidth >> was also in BB as well, if we turn these into bytes, and such range cannot be >> guarunteed... >> 2) recover the previous code snippet and check dsu in advance: >> if (dsu % cfg->sectorsize) { >> fprintf(stderr, >> _("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize); >> usage(); >> } since we have this check already in xfs_validate_stripe_geometry, it seems best to keep using it there, and not copy it ... which I think you accomplish below. >> btw, do we have some range test about these variables? I could rearrange the code >> snippet, but I'm not sure if it could introduce some new potential regression as well... >> >> Thanks, >> Gao Xiang > > Or how about applying the following incremental patch, although the maximum dswidth > would be smaller I think, but considering libxfs_validate_stripe_geometry() accepts > dswidth in 64-bit bytes as well. I think that would be fine. Does that make sense? > > I've confirmed "# mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0" now report: > stripe unit (4097) must be a multiple of the sector size (512) > > and xfs/191-input-validation passes now... > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index f152d5c7..80405790 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -2361,20 +2361,24 @@ _("both data su and data sw options must be specified\n")); > usage(); > } Just thinking through this... I think this is the right idea. > - dsunit = (int)BTOBBT(dsu); > - big_dswidth = (long long int)dsunit * dsw; > + big_dswidth = (long long int)dsu * dsw; dsu is in bytes; this would mean big_dswidth is now also in bytes... the original goal here, I think, is to not overflow the 32-bit superblock value for dswidth. > 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); so this used to test big_dswidth in BB (sectors); but now it tests in bytes. Perhaps this should change to check and report sectors again: if (BTOBBT(big_dswidth) > INT_MAX) { fprintf(stderr, _("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"), BTOBBT(big_dswidth), dsunit); I think the goal is to not overflow the 32-bit on-disk values, which would be easy to do with "dsw" specified as a /multiplier/ of "dsu" So I think that if we keep range checking the value in BB units, it will be OK. > usage(); > } > - dswidth = big_dswidth; > - } > > - if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), BBTOB(dswidth), > - cfg->sectorsize, false)) > + if (!libxfs_validate_stripe_geometry(NULL, dsu, big_dswidth, > + cfg->sectorsize, false)) > + usage(); > + > + dsunit = BTOBBT(dsu); > + dswidth = BTOBBT(big_dswidth); > + } else if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), > + BBTOB(dswidth), cfg->sectorsize, false)) { > usage(); > + } Otherwise this looks reasonable to me; now it's basically: 1) If we got geometry in bytes, validate them directly 2) If we got geometry in BB, convert to bytes, and validate 3) If we got no geometry, validate the device-reported defaults Thanks, -Eric > /* If sunit & swidth were manually specified as 0, same as noalign */ > if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) && >
On Thu, Feb 18, 2021 at 10:38:17AM -0600, Eric Sandeen wrote: > On 2/17/21 11:24 PM, Gao Xiang wrote: > ... > since we have this check already in xfs_validate_stripe_geometry, it seems best to > keep using it there, and not copy it ... which I think you accomplish below. > > >> btw, do we have some range test about these variables? I could rearrange the code > >> snippet, but I'm not sure if it could introduce some new potential regression as well... > >> > >> Thanks, > >> Gao Xiang > > > > Or how about applying the following incremental patch, although the maximum dswidth > > would be smaller I think, but considering libxfs_validate_stripe_geometry() accepts > > dswidth in 64-bit bytes as well. I think that would be fine. Does that make sense? > > > > I've confirmed "# mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0" now report: > > stripe unit (4097) must be a multiple of the sector size (512) > > > > and xfs/191-input-validation passes now... > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > > index f152d5c7..80405790 100644 > > --- a/mkfs/xfs_mkfs.c > > +++ b/mkfs/xfs_mkfs.c > > @@ -2361,20 +2361,24 @@ _("both data su and data sw options must be specified\n")); > > usage(); > > } > > Just thinking through this... I think this is the right idea. > > > - dsunit = (int)BTOBBT(dsu); > > - big_dswidth = (long long int)dsunit * dsw; > > + big_dswidth = (long long int)dsu * dsw; > > dsu is in bytes; this would mean big_dswidth is now also in bytes... > the original goal here, I think, is to not overflow the 32-bit superblock value > for dswidth. Yeah, agreed. Thanks for catching this. > > > 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); > > so this used to test big_dswidth in BB (sectors); but now it tests in bytes. > > Perhaps this should change to check and report sectors again: > > if (BTOBBT(big_dswidth) > INT_MAX) { > fprintf(stderr, > _("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"), > BTOBBT(big_dswidth), dsunit); > > I think the goal is to not overflow the 32-bit on-disk values, which would be > easy to do with "dsw" specified as a /multiplier/ of "dsu" > > So I think that if we keep range checking the value in BB units, it will be > OK. > > > usage(); > > } > > - dswidth = big_dswidth; > > - } > > > > - if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), BBTOB(dswidth), > > - cfg->sectorsize, false)) > > + if (!libxfs_validate_stripe_geometry(NULL, dsu, big_dswidth, > > + cfg->sectorsize, false)) > > + usage(); > > + > > + dsunit = BTOBBT(dsu); > > + dswidth = BTOBBT(big_dswidth); > > + } else if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), > > + BBTOB(dswidth), cfg->sectorsize, false)) { > > usage(); > > + } > Otherwise this looks reasonable to me; now it's basically: > > 1) If we got geometry in bytes, validate them directly > 2) If we got geometry in BB, convert to bytes, and validate > 3) If we got no geometry, validate the device-reported defaults > Ok, let me send the next version. Thanks, Gao Xiang > Thanks, > -Eric > > > /* If sunit & swidth were manually specified as 0, same as noalign */ > > if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) && > > >
diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h index e7e42e93..306d0deb 100644 --- a/libxfs/libxfs_api_defs.h +++ b/libxfs/libxfs_api_defs.h @@ -188,6 +188,7 @@ #define xfs_trans_roll_inode libxfs_trans_roll_inode #define xfs_trans_roll libxfs_trans_roll +#define xfs_validate_stripe_geometry libxfs_validate_stripe_geometry #define xfs_verify_agbno libxfs_verify_agbno #define xfs_verify_agino libxfs_verify_agino #define xfs_verify_cksum libxfs_verify_cksum diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 8fe149d7..aec40c1f 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -2305,12 +2305,6 @@ _("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) { @@ -2322,13 +2316,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit ( dswidth = big_dswidth; } - if ((dsunit && !dswidth) || (!dsunit && dswidth) || - (dsunit && (dswidth % dsunit != 0))) { - fprintf(stderr, -_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"), - dswidth, dsunit); + if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), BBTOB(dswidth), + cfg->sectorsize, false)) usage(); - } /* If sunit & swidth were manually specified as 0, same as noalign */ if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) && @@ -2344,11 +2334,12 @@ _("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) { + /* Ignore nonsense from device report. */ + if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit), + BBTOB(ft->dswidth), 0, true)) { fprintf(stderr, -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"), - progname, BBTOB(ft->dsunit)); +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"), + progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth)); ft->dsunit = 0; ft->dswidth = 0; } else {
Check stripe numbers in calc_stripe_factors() by using xfs_validate_stripe_geometry(). Signed-off-by: Gao Xiang <hsiangkao@redhat.com> --- libxfs/libxfs_api_defs.h | 1 + mkfs/xfs_mkfs.c | 23 +++++++---------------- 2 files changed, 8 insertions(+), 16 deletions(-)