Messages in this thread | | | Date | Tue, 27 Jun 2023 13:15:03 +0200 | From | Simon Horman <> | Subject | 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: > > 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/ > > "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.
| |