Messages in this thread | | | Date | Tue, 27 Jun 2023 14:41:48 +0300 | From | Vladimir Oltean <> | 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:15:03PM +0200, Simon Horman wrote: > 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.
They are integers assigned from the same code blocks in all cases, starting with -1 and later being assigned rvalues either from u64 fields limited to 0-255 (meta->source_port, meta->switch_id) or from unsigned char fields (hdr->h_dest[3], hdr->h_dest[4]), or from dsa_8021q_rx_source_port() and dsa_8021q_rx_switch_id() which return limited-size positive integers due to their implementation.
| |