Re: [PATCH 1/1] xfsprogs: Fix uninitialized cfg->lsunit

[Allison Collins <allison.henderson@oracle.com>]

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
>>
>