Thu, Jan 18, 2018 at 04:36:58PM CET, da...@davemloft.net wrote:
>From: Jiri Pirko <j...@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.
