On Sat, Oct 22, 2016 at 12:16:29AM +0200, Arnd Bergmann wrote:

On Friday, October 21, 2016 11:05:45 PM CEST Arnd Bergmann wrote:

quoted

Can you explain why "dissector_uses_key(flow_dissector,
FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies
"eth_type_vlan(proto))"?

If I add uninitialized_var() here, I would at least put that in
a comment here.

Found it now myself: if skb_vlan_tag_present(skb), then we don't
access 'vlan', otherwise we know it is initialized because
eth_type_vlan(proto) has to be true.
 

quoted

On a related note, I also don't see how
"dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)"
implies that skb is non-NULL. I guess this is related to the
first one.

I'm still unsure about this one.

Only skb_flow_dissect_flow_keys_buf() calls this function with skb ==
NULL. It uses flow_keys_buf_dissector_keys which does not specify
FLOW_DISSECTOR_KEY_VLAN, so the if statement is false.

A similar assumption is made for FLOW_DISSECTOR_KEY_ETH_ADDRS higher up.

I also found something else that is suspicious: 'vlan' points
to the local _vlan variable, but that has gone out of scope
by the time we access the pointer, which doesn't seem safe.

I see no harm in moving _vlan to the same scope as vlan.