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

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

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

When tcm->tcm_ifindex == TCM_IFINDEX_MAGIC_BLOCK, parent is still passed
down but the value is never used. Compiler does not recognize it and
issues a warning. Silence it down initializing parent to 0.

Fixes: 7960d1daf278 ("net: sched: use block index as a handle instead of qdisc when block is shared")
Reported-by: David Miller <davem@davemloft.net>
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- add comment about why the initialization to 0 is ok
- move the initialization into the if block to be near the comment
---
 net/sched/cls_api.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index e500d11da9cd..86d6e9d2cf00 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -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 {
 		const struct Qdisc_class_ops *cops;
 		struct net_device *dev;
-- 
2.14.3

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

[David Miller]

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.

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

[Jiri Pirko]

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.