Re: [PATCH net 2/2] net: dsa: tag_sja1105: always prefer source port information from INCL_SRCPT

On Tue, Jun 27, 2023 at 01:18:28AM +0300, Vladimir Oltean wrote:

Hi Simon,

On Mon, Jun 26, 2023 at 08:11:53PM +0200, Simon Horman wrote:

quoted

Hi Vladimir,

A similar comment to that made for [1], though the code is somewhat
different to that case: are you sure vid is initialised here?
GCC 12 and Smatch seem unsure about it.

[1] Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q
    https://lore.kernel.org/all/ZJg2M+Qvg3Fv73CH@corigine.com/ (local)

"vid" can be uninitialized if the tagger is fed a junk packet (a
non-link-local, non-meta packet that also has no tag_8021q header).

The immediate answer that comes to mind is: it depends on how the driver
configures the hardware to send packets to the CPU (and it will never
configure the switch in that way).

But, between the sja1105 driver configuring the switch in a certain way
and the tag_sja1105 driver seeing the results of that, there's also the
DSA master driver (can be any net_device) which can alter the packet in
a nonsensical way, like remove the VLAN header for some reason.

Considering the fact that the DSA master can have tc rules on its
ingress path which do just that, it would probably be wise to be
defensive about this. So I can probably add:

	if (sja1105_skb_has_tag_8021q(skb)) {
		... // existing call to sja1105_vlan_rcv() here
	} else if (source_port == -1 && switch_id == -1) {
		/* Packets with no source information have no chance of
		 * getting accepted, drop them straight away.
		 */
		return NULL;
	}

This "else if" block should ensure that when "vid" is uninitialized,
either "source_port" and "switch_id", or "vbid", always have valid values.

This is kind of complex :)

Can I clarify that either:

1. Both source_port and switch_id are -1; or
2. Neither source_port nor switch_id are -1

If so, I agree with your proposal.