From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Miller <davem@davemloft.net>
Subject: Re: [patch net-next v2] net: sched: silence uninitialized parent
 variable warning in tc_dump_tfilter
Date: Thu, 18 Jan 2018 10:36:58 -0500 (EST)
Message-ID: <20180118.103658.1668529956898388197.davem@davemloft.net>
References: <20180118151449.2803-1-jiri@resnulli.us>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Cc: netdev@vger.kernel.org, jhs@mojatatu.com, xiyou.wangcong@gmail.com,
        mlxsw@mellanox.com, sfr@canb.auug.org.au, arnd@arndb.de
To: jiri@resnulli.us
Return-path: <netdev-owner@vger.kernel.org>
Received: from shards.monkeyblade.net ([184.105.139.130]:52608 "EHLO
        shards.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S932640AbeARPhC (ORCPT
        <rfc822;netdev@vger.kernel.org>); Thu, 18 Jan 2018 10:37:02 -0500
In-Reply-To: <20180118151449.2803-1-jiri@resnulli.us>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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.

Thank you.