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:   Mon, 26 Jun 2023 13:37:41 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     Simon Horman <simon.horman@corigine.com>
Cc:     Pawel Dembicki <paweldembicki@gmail.com>, netdev@vger.kernel.org,
        Linus Walleij <linus.walleij@linaro.org>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@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-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based
 on 8021q
Message-ID: <8b34df96-daed-4b71-9281-3a7b8a315c90@kadam.mountain>
References: <20230625115343.1603330-1-paweldembicki@gmail.com>
 <20230625115343.1603330-4-paweldembicki@gmail.com>
 <ZJg2M+Qvg3Fv73CH@corigine.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZJg2M+Qvg3Fv73CH@corigine.com>
Precedence: bulk

On Sun, Jun 25, 2023 at 02:42:27PM +0200, Simon Horman wrote:
> + Dan Carpenter
> 
> On Sun, Jun 25, 2023 at 01:53:39PM +0200, Pawel Dembicki wrote:
> > This patch is simple implementation of 8021q tagging in vsc73xx driver.
> > At this moment devices with DSA_TAG_PROTO_NONE are useless. VSC73XX
> > family doesn't provide any tag support for external ethernet ports.
> > 
> > The only way is vlan-based tagging. It require constant hardware vlan
> > filtering. VSC73XX family support provider bridging but QinQ only without
> > fully implemented 802.1AD. It allow only doubled 0x8100 TPID.
> > 
> > In simple port mode QinQ is enabled to preserve forwarding vlan tagged
> > frames.
> > 
> > Tag driver introduce most simple funcionality required for proper taging
> > support.
> > 
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> 
> ...
> 
> > +static void vsc73xx_vlan_rcv(struct sk_buff *skb, int *source_port,
> > +			     int *switch_id, int *vbid, u16 *vid)
> > +{
> > +	if (vid_is_dsa_8021q(skb_vlan_tag_get(skb) & VLAN_VID_MASK))
> > +		return dsa_8021q_rcv(skb, source_port, switch_id, vbid);
> > +
> > +	/* Try our best with imprecise RX */
> > +	*vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK;
> > +}
> > +
> > +static struct sk_buff *vsc73xx_rcv(struct sk_buff *skb,
> > +				   struct net_device *netdev)
> > +{
> > +	int src_port = -1, switch_id = -1, vbid = -1;
> > +	u16 vid;
> > +
> > +	if (skb_vlan_tag_present(skb))
> > +		/* Normal traffic path. */
> > +		vsc73xx_vlan_rcv(skb, &src_port, &switch_id, &vbid, &vid);
> > +
> > +	if (vbid >= 1)
> > +		skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
> > +	else if (src_port == -1 || switch_id == -1)
> > +		skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
> 
> Hi Pawel,
> 
> Smatch warns that vid may be used uninitialised here.
> And it's not clear to me why that cannot be the case.
> 

The problem is only when skb_vlan_tag_present() is false.

If vsc73xx_vlan_rcv() is called then actually it's fine.  Either vbid,
src_port and switch_id will be set or vid will be initialized.  Smatch
thinks the vbid is set to 0-7, src_port is set to 0-15 and
switch_id is set to 0-7.  Smatch kind of ignores the switch_id == -1
condition because it's known to be false given that we already checked
src_port == -1.

But the concern again is the skb_vlan_tag_present() is false and then
vbid, src_port and switch_id would all be set to -1.

regards,
dan carpenter