From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jiri Pirko <jiri@resnulli.us>
Subject: Re: [patch net-next v2] net: sched: silence uninitialized parent
 variable warning in tc_dump_tfilter
Date: Thu, 18 Jan 2018 17:02:17 +0100
Message-ID: <20180118160217.GC2063@nanopsycho.orion>
References: <20180118151449.2803-1-jiri@resnulli.us>
 <20180118.103658.1668529956898388197.davem@davemloft.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: netdev@vger.kernel.org, jhs@mojatatu.com, xiyou.wangcong@gmail.com,
        mlxsw@mellanox.com, sfr@canb.auug.org.au, arnd@arndb.de
To: David Miller <davem@davemloft.net>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-wr0-f195.google.com ([209.85.128.195]:46102 "EHLO
        mail-wr0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S932670AbeARQCT (ORCPT
        <rfc822;netdev@vger.kernel.org>); Thu, 18 Jan 2018 11:02:19 -0500
Received: by mail-wr0-f195.google.com with SMTP id g21so23054752wrb.13
        for <netdev@vger.kernel.org>; Thu, 18 Jan 2018 08:02:19 -0800 (PST)
Content-Disposition: inline
In-Reply-To: <20180118.103658.1668529956898388197.davem@davemloft.net>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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.