linux-kernel - Re: [PATCH net] net: lan966x: Make sure to insert the vlan tags also in host mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250528085451.bindv4477g56mfcu@DEN-DL-M31836.microchip.com>
Date: Wed, 28 May 2025 10:54:51 +0200
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
CC: <UNGLinuxDriver@...rochip.com>, <andrew+netdev@...n.ch>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] net: lan966x: Make sure to insert the vlan tags also
 in host mode

The 05/27/2025 10:37, Maxime Chevallier wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hello Horatiu,

Hi Maxime,

> 
> On Tue, 27 May 2025 09:08:50 +0200
> Horatiu Vultur <horatiu.vultur@...rochip.com> wrote:
> 
> > When running these commands on DUT (and similar at the other end)
> > ip link set dev eth0 up
> > ip link add link eth0 name eth0.10 type vlan id 10
> > ip addr add 10.0.0.1/24 dev eth0.10
> > ip link set dev eth0.10 up
> > ping 10.0.0.2/24
> >
> > The ping will fail.
> >
> > The reason why is failing is because, the network interfaces for lan966x
> > have a flag saying that the HW can insert the vlan tags into the
> > frames(NETIF_F_HW_VLAN_CTAG_TX). Meaning that the frames that are
> > transmitted don't have the vlan tag inside the skb data, but they have
> > it inside the skb. We already get that vlan tag and put it in the IFH
> > but the problem is that we don't configure the HW to rewrite the frame
> > when the interface is in host mode.
> > The fix consists in actually configuring the HW to insert the vlan tag
> > if it is different than 0.
> >
> > Fixes: 6d2c186afa5d ("net: lan966x: Add vlan support.")
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@...rochip.com>
> > ---
> >  .../ethernet/microchip/lan966x/lan966x_main.c |  1 +
> >  .../ethernet/microchip/lan966x/lan966x_main.h |  1 +
> >  .../microchip/lan966x/lan966x_switchdev.c     |  1 +
> >  .../ethernet/microchip/lan966x/lan966x_vlan.c | 21 +++++++++++++++++++
> >  4 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > index 427bdc0e4908c..7001584f1b7a6 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > @@ -879,6 +879,7 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
> >       lan966x_vlan_port_set_vlan_aware(port, 0);
> >       lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
> >       lan966x_vlan_port_apply(port);
> > +     lan966x_vlan_port_rew_host(port);
> >
> >       return 0;
> >  }
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > index 1f9df67f05044..4f75f06883693 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > @@ -497,6 +497,7 @@ void lan966x_vlan_port_apply(struct lan966x_port *port);
> >  bool lan966x_vlan_cpu_member_cpu_vlan_mask(struct lan966x *lan966x, u16 vid);
> >  void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
> >                                     bool vlan_aware);
> > +void lan966x_vlan_port_rew_host(struct lan966x_port *port);
> >  int lan966x_vlan_port_set_vid(struct lan966x_port *port,
> >                             u16 vid,
> >                             bool pvid,
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > index 1c88120eb291a..bcb4db76b75cd 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > @@ -297,6 +297,7 @@ static void lan966x_port_bridge_leave(struct lan966x_port *port,
> >       lan966x_vlan_port_set_vlan_aware(port, false);
> >       lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
> >       lan966x_vlan_port_apply(port);
> > +     lan966x_vlan_port_rew_host(port);
> >  }
> >
> >  int lan966x_port_changeupper(struct net_device *dev,
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> > index fa34a739c748e..f158ec6ab10cc 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> > @@ -149,6 +149,27 @@ void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
> >       port->vlan_aware = vlan_aware;
> >  }
> >
> > +/* When the interface is in host mode, the interface should not be vlan aware
> > + * but it should insert all the tags that it gets from the network stack.
> > + * The tags are no in the data of the frame but actually in the skb and the ifh
>                    not
> > + * is confiured already to get this tag. So what we need to do is to update the
>          configured
> > + * rewriter to insert the vlan tag for all frames which have a vlan tag
> > + * different than 0.

Well spotted!

> 
> Just to be extra clear, the doc seems to say that
> 
>         REW_TAG_CFG_TAG_CFG_SET(1);
> 
> means "Tag all frames, except when VID=PORT_VLAN_CFG.PORT_VID or
> VID=0."
> 
> Another setting for these bits are "Tag all frames except when VID=0",
> which is what you document in the above comment.
> 
> In this case, is there any chance that it would make a difference ?

I don't see how will this make a difference but I will update it to set
the value of 2 which means VID=0 and also update the comment.

> 
> > + */
> > +void lan966x_vlan_port_rew_host(struct lan966x_port *port)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +     u32 val;
> > +
> > +     /* Tag all frames except when VID == DEFAULT_VLAN */
> > +     val = REW_TAG_CFG_TAG_CFG_SET(1);
> > +
> > +     /* Update only some bits in the register */
> > +     lan_rmw(val,
> > +             REW_TAG_CFG_TAG_CFG,
> > +             lan966x, REW_TAG_CFG(port->chip_port));
> > +}
> > +
> >  void lan966x_vlan_port_apply(struct lan966x_port *port)
> >  {
> >       struct lan966x *lan966x = port->lan966x;
> 
> Sorry for the typo nitpicking, but with that :
> 
> Reviewed-by: Maxime Chevallier <maxime.chevallier@...tlin.com>
> 
> Maxime

-- 
/Horatiu