Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 27 Jun 2023 14:41:48 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Simon Horman <simon.horman@corigine.com>
Cc:     Vladimir Oltean <vladimir.oltean@nxp.com>, netdev@vger.kernel.org,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net 2/2] net: dsa: tag_sja1105: always prefer source port
 information from INCL_SRCPT
Message-ID: <20230627114148.lpyopy6ttuvvciww@skbuf>
References: <20230626155112.3155993-1-vladimir.oltean@nxp.com>
 <20230626155112.3155993-3-vladimir.oltean@nxp.com>
 <ZJnU6WntVQW2AgvZ@corigine.com>
 <20230626221828.qzjeo6dedjnyme6k@skbuf>
 <ZJrEtw15g3a7nyLN@corigine.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZJrEtw15g3a7nyLN@corigine.com>
Precedence: bulk

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.