| Message ID | 20180118151449.2803-1-jiri@resnulli.us |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,v2] net: sched: silence uninitialized parent variable warning in tc_dump_tfilter | expand |
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.
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.
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;