[v2,1/1] xfsprogs: Fix uninitialized cfg->lsunit

Commit Message

Allison Henderson July 2, 2019, 11:27 p.m. UTC

From: Allison Henderson <[e-mail]>

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 <[e-mail]>
Reviewed-by: Darrick J. Wong <[e-mail]>
Reviewed-by: Carlos Maiolino <[e-mail]>
---
 mkfs/xfs_mkfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Eric Sandeen July 3, 2019, 5:47 p.m. UTC | #1

On 7/2/19 6:27 PM, Allison Collins wrote:
> From: Allison Henderson <[e-mail]>
> 
> 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 <[e-mail]>
> Reviewed-by: Darrick J. Wong <[e-mail]>
> Reviewed-by: Carlos Maiolino <[e-mail]>

Ok, while I appreciate you taking Carlos's input, the patch now does
far more than the commit log says, with no explanation of why it's doing
so.  (and there's no indication of what actually changed in V2: - putting
that info below the "---" line is helpful)

I'd prefer to take the original patch and if we really want to change
how we initialize empty structures, that should be a separate patch, and
should hit everywhere we do it, not just mkfs.

But to Carlos's point, cfg->lsunit isn't exactly "uninitialized"
(to me, uninitialized means that it was never set, when in fact it was
initialized, to zero, right?)

So it's not quite clear to me what's happening here; I guess this test:

        /*
         * check that log sunit is modulo fsblksize or default it to dsunit.
         */
        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;
        }

is doing the wrong thing because cfg->loginternal hasn't actually been
evaluated yet?  Is there a mkfs command that demonstrates the problem
which could be included in the changelog?  Does it only happen with
external logs?  If you can provide a bit more information about when and
how this actually fails, that would improve the changelog for future
generations.

(also agreeing w/ darrick that these seem like little time bombs...)

Thanks,
-Eric

> ---
>  mkfs/xfs_mkfs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 468b8fd..6e32403 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3861,15 +3861,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};
>  
>  	/* build time defaults */
>  	struct mkfs_default_params	dft = {
> @@ -4008,7 +4008,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
> @@ -4018,6 +4017,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,
>

Eric Sandeen July 3, 2019, 6:14 p.m. UTC | #2

On 7/3/19 12:47 PM, Eric Sandeen wrote:
> On 7/2/19 6:27 PM, Allison Collins wrote:
>> From: Allison Henderson <[e-mail]>
>>
>> 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 <[e-mail]>
>> Reviewed-by: Darrick J. Wong <[e-mail]>
>> Reviewed-by: Carlos Maiolino <[e-mail]>
> 
> Ok, while I appreciate you taking Carlos's input, the patch now does
> far more than the commit log says, with no explanation of why it's doing
> so.  (and there's no indication of what actually changed in V2: - putting
> that info below the "---" line is helpful)
> 
> I'd prefer to take the original patch and if we really want to change
> how we initialize empty structures, that should be a separate patch, and
> should hit everywhere we do it, not just mkfs.

Ok so I was on track up to here

> But to Carlos's point, cfg->lsunit isn't exactly "uninitialized"
> (to me, uninitialized means that it was never set, when in fact it was
> initialized, to zero, right?)

and I guess this is just semantics...

> So it's not quite clear to me what's happening here; I guess this test:
> 
>         /*
>          * check that log sunit is modulo fsblksize or default it to dsunit.
>          */
>         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;
>         }
> 
> is doing the wrong thing because cfg->loginternal hasn't actually been
> evaluated yet?  Is there a mkfs command that demonstrates the problem
> which could be included in the changelog?  Does it only happen with
> external logs?  If you can provide a bit more information about when and
> how this actually fails, that would improve the changelog for future
> generations.

OK, TBH I had confused cfg-> with cli-> (derp) and I see that as you said,
validate_logdev() sets up cfg->loginternal, sorry.  So I'm ok with V1 and
its changelog as it stands, I think.

Sorry for my confusion and the noise,

-Eric

> (also agreeing w/ darrick that these seem like little time bombs...)
> 
> Thanks,
> -Eric
> 
>> ---
>>  mkfs/xfs_mkfs.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index 468b8fd..6e32403 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -3861,15 +3861,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};
>>  
>>  	/* build time defaults */
>>  	struct mkfs_default_params	dft = {
>> @@ -4008,7 +4008,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
>> @@ -4018,6 +4017,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,
>>
>

Allison Henderson July 3, 2019, 7:01 p.m. UTC | #3

On 7/3/19 11:14 AM, Eric Sandeen wrote:
> On 7/3/19 12:47 PM, Eric Sandeen wrote:
>> On 7/2/19 6:27 PM, Allison Collins wrote:
>>> From: Allison Henderson <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Reviewed-by: Darrick J. Wong <[e-mail]>
>>> Reviewed-by: Carlos Maiolino <[e-mail]>
>>
>> Ok, while I appreciate you taking Carlos's input, the patch now does
>> far more than the commit log says, with no explanation of why it's doing
>> so.  (and there's no indication of what actually changed in V2: - putting
>> that info below the "---" line is helpful)
>>
>> I'd prefer to take the original patch and if we really want to change
>> how we initialize empty structures, that should be a separate patch, and
>> should hit everywhere we do it, not just mkfs.
> 
> Ok so I was on track up to here
> 
>> But to Carlos's point, cfg->lsunit isn't exactly "uninitialized"
>> (to me, uninitialized means that it was never set, when in fact it was
>> initialized, to zero, right?)
> 
> and I guess this is just semantics...
> 
>> So it's not quite clear to me what's happening here; I guess this test:
>>
>>          /*
>>           * check that log sunit is modulo fsblksize or default it to dsunit.
>>           */
>>          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;
>>          }
>>
>> is doing the wrong thing because cfg->loginternal hasn't actually been
>> evaluated yet?  Is there a mkfs command that demonstrates the problem
>> which could be included in the changelog?  Does it only happen with
>> external logs?  If you can provide a bit more information about when and
>> how this actually fails, that would improve the changelog for future
>> generations.
> 
> OK, TBH I had confused cfg-> with cli-> (derp) and I see that as you said,
> validate_logdev() sets up cfg->loginternal, sorry.  So I'm ok with V1 and
> its changelog as it stands, I think.
> 
> Sorry for my confusion and the noise,
> 
> -Eric

Alrighty then, I think I answered all your other questions in the IRC 
chat.  Thank you!!

Allison

> 
>> (also agreeing w/ darrick that these seem like little time bombs...)
>>
>> Thanks,
>> -Eric
>>
>>> ---
>>>   mkfs/xfs_mkfs.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>>> index 468b8fd..6e32403 100644
>>> --- a/mkfs/xfs_mkfs.c
>>> +++ b/mkfs/xfs_mkfs.c
>>> @@ -3861,15 +3861,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};
>>>   
>>>   	/* build time defaults */
>>>   	struct mkfs_default_params	dft = {
>>> @@ -4008,7 +4008,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
>>> @@ -4018,6 +4017,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,
>>>
>>

Patch

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 468b8fd..6e32403 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3861,15 +3861,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};
 
 	/* build time defaults */
 	struct mkfs_default_params	dft = {
@@ -4008,7 +4008,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
@@ -4018,6 +4017,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,