Re: [patch net-next v2] net: sched: silence uninitialized parent variable warning in tc_dump_tfilter

[Jiri Pirko <jiri@resnulli.us>]

Thu, Jan 18, 2018 at 04:36:58PM CET, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Thu, 18 Jan 2018 16:14:49 +0100
>
>> @@ -1317,6 +1317,13 @@ static int tc_dump_tfilter(struct sk_buff *skb, struct netlink_callback *cb)
>>  		block = tcf_block_lookup(net, tcm->tcm_block_index);
>>  		if (!block)
>>  			goto out;
>> +		/* If we work with block index, q is NULL and parent value
>> +		 * will never be used in the following code. The check
>> +		 * in tcf_fill_node prevents it. However, compiler does not
>> +		 * see that far, so set parent to zero to silence the warning
>> +		 * about parent being uninitialized.
>> +		 */
>> +		parent = 0;
>>  	} else {
>
>Ugh....
>
>Jiri, if you need to add such a verbose comment to explain a compiler
>warning fix, then this code is too complicated for humans to
>understand and audit properly.
>
>And from this perspective I really don't blame the compiler.  Even
>I am still having trouble putting all of these invariants together,
>even considering the information in this comment, in order to see
>how this is "ok".
>
>And even if tcf_fill_node() doesn't access parent, tcf_chain_dump()
>does and stores this uninitialized value into the 'args' if we
>run out of space during the dump.
>
>Yes, I understand that this value will never be used, but wow that
>is propagating an uninitialized value across dump passes.
>
>I've applied this, but please look into restructuring this code
>so that it is a bit more sane in this regard.

Ack. Will try to figure out how to make this saner.

Thanks.