* [PATCH 1/1] xfsprogs: Fix uninitialized cfg->lsunit
@ 2019-07-01 17:35 Allison Collins
2019-07-02 8:26 ` Carlos Maiolino
0 siblings, 1 reply; 6+ messages in thread
From: Allison Collins @ 2019-07-01 17:35 UTC (permalink / raw)
To: linux-xfs
While investigating another mkfs bug, noticed that cfg->lsunit is sometimes
left uninitialized when it should not. This is because calc_stripe_factors
in some cases needs cfg->loginternal to be set first. This is done in
validate_logdev. So move calc_stripe_factors below validate_logdev while
parsing configs.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
mkfs/xfs_mkfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index ddb25ec..f4a5e4b 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3995,7 +3995,6 @@ main(
cfg.rtblocks = calc_dev_size(cli.rtsize, &cfg, &ropts, R_SIZE, "rt");
validate_rtextsize(&cfg, &cli, &ft);
- calc_stripe_factors(&cfg, &cli, &ft);
/*
* Open and validate the device configurations
@@ -4005,6 +4004,7 @@ main(
validate_datadev(&cfg, &cli);
validate_logdev(&cfg, &cli, &logfile);
validate_rtdev(&cfg, &cli, &rtfile);
+ calc_stripe_factors(&cfg, &cli, &ft);
/*
* At this point when know exactly what size all the devices are,
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] xfsprogs: Fix uninitialized cfg->lsunit
2019-07-01 17:35 [PATCH 1/1] xfsprogs: Fix uninitialized cfg->lsunit Allison Collins
@ 2019-07-02 8:26 ` Carlos Maiolino
2019-07-02 22:15 ` Allison Collins
0 siblings, 1 reply; 6+ messages in thread
From: Carlos Maiolino @ 2019-07-02 8:26 UTC (permalink / raw)
To: Allison Collins; +Cc: linux-xfs
Hi,
On Mon, Jul 01, 2019 at 10:35:38AM -0700, Allison Collins wrote:
> While investigating another mkfs bug, noticed that cfg->lsunit is sometimes
> left uninitialized when it should not. This is because calc_stripe_factors
> in some cases needs cfg->loginternal to be set first. This is done in
> validate_logdev. So move calc_stripe_factors below validate_logdev while
> parsing configs.
>
I believe cfg->lsunit will be left 'uninitialized' every time if it is not
explicitly set in mkfs command line.
I believe you are referring to this specific part of the code here:
┆ if (lsunit) {
┆ ┆ /* convert from 512 byte blocks to fs blocks */
┆ ┆ cfg->lsunit = DTOBT(lsunit, cfg->blocklog);
┆ } else if (cfg->sb_feat.log_version == 2 &&
┆ ┆ cfg->loginternal && cfg->dsunit) {
┆ ┆ /* lsunit and dsunit now in fs blocks */
┆ ┆ cfg->lsunit = cfg->dsunit;
┆ }
Which, well, unless we set lsunit at command line, we will always fall into the
else if and leave cfg->lsunit uninitialized, once we still don't have
cfg->loginternal set.
This is 'okayish' because we initialize the cfg structure here in main:
struct mkfs_params┆ cfg = {};
By default (IIRC), GCC will initialize to 0 all members of the struct, so, we
are 'safe' here in any case. But, at the same time, (also IIRC), structs should
not be initialized by empty braces (according to ISO C).
So, while I agree with your patch, while you're still on it, could you please
also (and if others agree), properly initialize the structs in main(){}?
Like:
@@ -3848,15 +3849,15 @@ main(
.isdirect = LIBXFS_DIRECT,
.isreadonly = LIBXFS_EXCLUSIVELY,
};
- struct xfs_mount mbuf = {};
+ struct xfs_mount mbuf = {0};
struct xfs_mount *mp = &mbuf;
struct xfs_sb *sbp = &mp->m_sb;
- struct fs_topology ft = {};
+ struct fs_topology ft = {0};
struct cli_params cli = {
.xi = &xi,
.loginternal = 1,
};
- struct mkfs_params cfg = {};
+ struct mkfs_params cfg = {0};
Anyway, this is more a suggestion due ISO C 'formalities' (which I *think* GCC
would complain if -Wpedantic was enabled), otherwise I can send a patch later
changing that, if you decide to go with your patch as-is, you can add:
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Cheers
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> ---
> mkfs/xfs_mkfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index ddb25ec..f4a5e4b 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3995,7 +3995,6 @@ main(
> cfg.rtblocks = calc_dev_size(cli.rtsize, &cfg, &ropts, R_SIZE, "rt");
>
> validate_rtextsize(&cfg, &cli, &ft);
> - calc_stripe_factors(&cfg, &cli, &ft);
>
> /*
> * Open and validate the device configurations
> @@ -4005,6 +4004,7 @@ main(
> validate_datadev(&cfg, &cli);
> validate_logdev(&cfg, &cli, &logfile);
> validate_rtdev(&cfg, &cli, &rtfile);
> + calc_stripe_factors(&cfg, &cli, &ft);
>
> /*
> * At this point when know exactly what size all the devices are,
> --
> 2.7.4
>
--
Carlos
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] xfsprogs: Fix uninitialized cfg->lsunit
2019-07-02 8:26 ` Carlos Maiolino
@ 2019-07-02 22:15 ` Allison Collins
0 siblings, 0 replies; 6+ messages in thread
From: Allison Collins @ 2019-07-02 22:15 UTC (permalink / raw)
To: linux-xfs
On 7/2/19 1:26 AM, Carlos Maiolino wrote:
> Hi,
>
> On Mon, Jul 01, 2019 at 10:35:38AM -0700, Allison Collins wrote:
>> While investigating another mkfs bug, noticed that cfg->lsunit is sometimes
>> left uninitialized when it should not. This is because calc_stripe_factors
>> in some cases needs cfg->loginternal to be set first. This is done in
>> validate_logdev. So move calc_stripe_factors below validate_logdev while
>> parsing configs.
>>
>
> I believe cfg->lsunit will be left 'uninitialized' every time if it is not
> explicitly set in mkfs command line.
>
> I believe you are referring to this specific part of the code here:
>
> ┆ if (lsunit) {
> ┆ ┆ /* convert from 512 byte blocks to fs blocks */
> ┆ ┆ cfg->lsunit = DTOBT(lsunit, cfg->blocklog);
> ┆ } else if (cfg->sb_feat.log_version == 2 &&
> ┆ ┆ cfg->loginternal && cfg->dsunit) {
> ┆ ┆ /* lsunit and dsunit now in fs blocks */
> ┆ ┆ cfg->lsunit = cfg->dsunit;
> ┆ }
>
> Which, well, unless we set lsunit at command line, we will always fall into the
> else if and leave cfg->lsunit uninitialized, once we still don't have
> cfg->loginternal set.
>
> This is 'okayish' because we initialize the cfg structure here in main:
>
> struct mkfs_params┆ cfg = {};
>
Yeah, it's worth mentioning too that I actually found this while trying
to fix a corrupted log ticket that was reported to have popped up after
upgrading xfsprogs. A lot of trial and error later I found the
corruption correlated with this bug, but I haven't found out exactly why
it has that effect yet. Something not right with how kernel space is
handling the config I suspect, but I'm still looking at it.
>
> By default (IIRC), GCC will initialize to 0 all members of the struct, so, we
> are 'safe' here in any case. But, at the same time, (also IIRC), structs should
> not be initialized by empty braces (according to ISO C).
>
> So, while I agree with your patch, while you're still on it, could you please
> also (and if others agree), properly initialize the structs in main(){}?
>
> Like:
>
> @@ -3848,15 +3849,15 @@ main(
> .isdirect = LIBXFS_DIRECT,
> .isreadonly = LIBXFS_EXCLUSIVELY,
> };
> - struct xfs_mount mbuf = {};
> + struct xfs_mount mbuf = {0};
> struct xfs_mount *mp = &mbuf;
> struct xfs_sb *sbp = &mp->m_sb;
> - struct fs_topology ft = {};
> + struct fs_topology ft = {0};
> struct cli_params cli = {
> .xi = &xi,
> .loginternal = 1,
> };
> - struct mkfs_params cfg = {};
> + struct mkfs_params cfg = {0};
>
>
>
>
> Anyway, this is more a suggestion due ISO C 'formalities' (which I *think* GCC
> would complain if -Wpedantic was enabled), otherwise I can send a patch later
> changing that, if you decide to go with your patch as-is, you can add:
>
Ok, that looks reasonable. I can add that in a v2 and send it out. Thanks!
Allison
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
>
> Cheers
>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> ---
>> mkfs/xfs_mkfs.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index ddb25ec..f4a5e4b 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -3995,7 +3995,6 @@ main(
>> cfg.rtblocks = calc_dev_size(cli.rtsize, &cfg, &ropts, R_SIZE, "rt");
>>
>> validate_rtextsize(&cfg, &cli, &ft);
>> - calc_stripe_factors(&cfg, &cli, &ft);
>>
>> /*
>> * Open and validate the device configurations
>> @@ -4005,6 +4004,7 @@ main(
>> validate_datadev(&cfg, &cli);
>> validate_logdev(&cfg, &cli, &logfile);
>> validate_rtdev(&cfg, &cli, &rtfile);
>> + calc_stripe_factors(&cfg, &cli, &ft);
>>
>> /*
>> * At this point when know exactly what size all the devices are,
>> --
>> 2.7.4
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] xfsprogs: Fix uninitialized cfg->lsunit
@ 2019-06-19 18:28 Allison Collins
2019-06-20 15:32 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Allison Collins @ 2019-06-19 18:28 UTC (permalink / raw)
To: linux-xfs
While investigating another mkfs bug, noticed that cfg->lsunit is sometimes
left uninitialized when it should not. This is because calc_stripe_factors
in some cases needs cfg->loginternal to be set first. This is done in
validate_logdev. So move calc_stripe_factors below validate_logdev while
parsing configs.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
---
mkfs/xfs_mkfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index ddb25ec..f4a5e4b 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3995,7 +3995,6 @@ main(
cfg.rtblocks = calc_dev_size(cli.rtsize, &cfg, &ropts, R_SIZE, "rt");
validate_rtextsize(&cfg, &cli, &ft);
- calc_stripe_factors(&cfg, &cli, &ft);
/*
* Open and validate the device configurations
@@ -4005,6 +4004,7 @@ main(
validate_datadev(&cfg, &cli);
validate_logdev(&cfg, &cli, &logfile);
validate_rtdev(&cfg, &cli, &rtfile);
+ calc_stripe_factors(&cfg, &cli, &ft);
/*
* At this point when know exactly what size all the devices are,
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] xfsprogs: Fix uninitialized cfg->lsunit
2019-06-19 18:28 Allison Collins
@ 2019-06-20 15:32 ` Darrick J. Wong
2019-06-21 1:36 ` Allison Collins
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2019-06-20 15:32 UTC (permalink / raw)
To: Allison Collins; +Cc: linux-xfs
On Wed, Jun 19, 2019 at 11:28:57AM -0700, Allison Collins wrote:
> While investigating another mkfs bug, noticed that cfg->lsunit is sometimes
> left uninitialized when it should not. This is because calc_stripe_factors
> in some cases needs cfg->loginternal to be set first. This is done in
> validate_logdev. So move calc_stripe_factors below validate_logdev while
> parsing configs.
<grumble> The cfg in main() is not (in a manner easily detectable by
toolz) uninitialized, it's zero-initialized by default and we haven't
set cfg->loginternal correctly yet...
...what we really need here is enum { FALSE, TRUE, FILENOTFOUND } to
detect that we're using incorrect garbage data. :P
(Really, someone should take a closer look at whether or not there are
other places where we do things like this...)
Anyway, this does solve a problem, so
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
>
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
> mkfs/xfs_mkfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index ddb25ec..f4a5e4b 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3995,7 +3995,6 @@ main(
> cfg.rtblocks = calc_dev_size(cli.rtsize, &cfg, &ropts, R_SIZE, "rt");
>
> validate_rtextsize(&cfg, &cli, &ft);
> - calc_stripe_factors(&cfg, &cli, &ft);
>
> /*
> * Open and validate the device configurations
> @@ -4005,6 +4004,7 @@ main(
> validate_datadev(&cfg, &cli);
> validate_logdev(&cfg, &cli, &logfile);
> validate_rtdev(&cfg, &cli, &rtfile);
> + calc_stripe_factors(&cfg, &cli, &ft);
>
> /*
> * At this point when know exactly what size all the devices are,
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] xfsprogs: Fix uninitialized cfg->lsunit
2019-06-20 15:32 ` Darrick J. Wong
@ 2019-06-21 1:36 ` Allison Collins
0 siblings, 0 replies; 6+ messages in thread
From: Allison Collins @ 2019-06-21 1:36 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On 6/20/19 8:32 AM, Darrick J. Wong wrote:
> On Wed, Jun 19, 2019 at 11:28:57AM -0700, Allison Collins wrote:
>> While investigating another mkfs bug, noticed that cfg->lsunit is sometimes
>> left uninitialized when it should not. This is because calc_stripe_factors
>> in some cases needs cfg->loginternal to be set first. This is done in
>> validate_logdev. So move calc_stripe_factors below validate_logdev while
>> parsing configs.
>
> <grumble> The cfg in main() is not (in a manner easily detectable by
> toolz) uninitialized, it's zero-initialized by default and we haven't
> set cfg->loginternal correctly yet...
>
> ...what we really need here is enum { FALSE, TRUE, FILENOTFOUND } to
> detect that we're using incorrect garbage data. :P
>
> (Really, someone should take a closer look at whether or not there are
> other places where we do things like this...)
>
> Anyway, this does solve a problem, so
>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> --D
Alrighty, thx for the review!
Yes, I gave it a quick look over and didn't notice any other out of
order usage, but it is really difficult to tell when one function has a
dependency of another without just pouring through it.
Do you mean to add an enum for every member of the cfg? That would
work, but I guess the question then is the added complexity worth just
making sure everything is configured linearly?
Another approach could be to initialize things to some sort of
uninitialized signature. Like memset(&cfg, 0xFF, sizeof(struct
mkfs_params), assuming 0xFF would never otherwise be set. Then other
subroutines could check to see if the configs they need are not foxed out.
I think both of these solutions have the effect of scattering default
config handling and asserts all over the place though. Which is
something to consider. I notice there's been a lot of patches to just
try and organize things. Technically it shouldn't be needed if we can
be diligent about making sure every thing flows forward, but I guess
deciding which of them is the lesser burden is really the question here.
:-)
Thoughts?
Allison
>
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>> mkfs/xfs_mkfs.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index ddb25ec..f4a5e4b 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -3995,7 +3995,6 @@ main(
>> cfg.rtblocks = calc_dev_size(cli.rtsize, &cfg, &ropts, R_SIZE, "rt");
>>
>> validate_rtextsize(&cfg, &cli, &ft);
>> - calc_stripe_factors(&cfg, &cli, &ft);
>>
>> /*
>> * Open and validate the device configurations
>> @@ -4005,6 +4004,7 @@ main(
>> validate_datadev(&cfg, &cli);
>> validate_logdev(&cfg, &cli, &logfile);
>> validate_rtdev(&cfg, &cli, &rtfile);
>> + calc_stripe_factors(&cfg, &cli, &ft);
>>
>> /*
>> * At this point when know exactly what size all the devices are,
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-03 1:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-01 17:35 [PATCH 1/1] xfsprogs: Fix uninitialized cfg->lsunit Allison Collins
2019-07-02 8:26 ` Carlos Maiolino
2019-07-02 22:15 ` Allison Collins
-- strict thread matches above, loose matches on Subject: below --
2019-06-19 18:28 Allison Collins
2019-06-20 15:32 ` Darrick J. Wong
2019-06-21 1:36 ` Allison Collins
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).