netdev - RE: [PATCH iwl-next v3 08/10] ice: implement bridge port vlan

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: <MW4PR11MB57762D825AD01B9A8527EE61FD409@MW4PR11MB5776.namprd11.prod.outlook.com>
Date: Tue, 23 May 2023 10:10:46 +0000
From: "Drewek, Wojciech" <wojciech.drewek@...el.com>
To: Simon Horman <simon.horman@...igine.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Lobakin, Aleksander"
	<aleksander.lobakin@...el.com>, "Ertman, David M" <david.m.ertman@...el.com>,
	"michal.swiatkowski@...ux.intel.com" <michal.swiatkowski@...ux.intel.com>,
	"marcin.szycik@...ux.intel.com" <marcin.szycik@...ux.intel.com>,
	"Chmielewski, Pawel" <pawel.chmielewski@...el.com>, "Samudrala, Sridhar"
	<sridhar.samudrala@...el.com>, Dan Carpenter <dan.carpenter@...aro.org>
Subject: RE: [PATCH iwl-next v3 08/10] ice: implement bridge port vlan



> -----Original Message-----
> From: Simon Horman <simon.horman@...igine.com>
> Sent: poniedziałek, 22 maja 2023 13:28
> To: Drewek, Wojciech <wojciech.drewek@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Lobakin, Aleksander <aleksander.lobakin@...el.com>; Ertman, David M
> <david.m.ertman@...el.com>; michal.swiatkowski@...ux.intel.com; marcin.szycik@...ux.intel.com; Chmielewski, Pawel
> <pawel.chmielewski@...el.com>; Samudrala, Sridhar <sridhar.samudrala@...el.com>; Dan Carpenter <dan.carpenter@...aro.org>
> Subject: Re: [PATCH iwl-next v3 08/10] ice: implement bridge port vlan
> 
> +Dan Carpenter
> 
> On Mon, May 22, 2023 at 11:05:40AM +0200, Wojciech Drewek wrote:
> > From: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> >
> > Port VLAN in this case means push and pop VLAN action on specific vid.
> > There are a few limitation in hardware:
> > - push and pop can't be used separately
> > - if port VLAN is used there can't be any trunk VLANs, because pop
> >   action is done on all traffic received by VSI in port VLAN mode
> > - port VLAN mode on uplink port isn't supported
> >
> > Reflect these limitations in code using dev_info to inform the user
> > about unsupported configuration.
> >
> > In bridge mode there is a need to configure port vlan without resetting
> > VFs. To do that implement ice_port_vlan_on/off() functions. They are
> > only configuring correct vlan_ops to allow setting port vlan.
> >
> > We also need to clear port vlan without resetting the VF which is not
> > supported right now. Change it by implementing clear_port_vlan ops.
> > As previous VLAN configuration isn't always the same, store current
> > config while creating port vlan and restore it in clear function.
> >
> > Configuration steps:
> > - configure switchdev with bridge
> > - #bridge vlan add dev eth0 vid 120 pvid untagged
> > - #bridge vlan add dev eth1 vid 120 pvid untagged
> > - ping from VF0 to VF1
> >
> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> > Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.com>
> 
> ...
> 
> > @@ -639,14 +698,29 @@ ice_eswitch_br_vlan_create(u16 vid, u16 flags, struct ice_esw_br_port *port)
> >
> >  	vlan->vid = vid;
> >  	vlan->flags = flags;
> > +	if ((flags & BRIDGE_VLAN_INFO_PVID) &&
> > +	    (flags & BRIDGE_VLAN_INFO_UNTAGGED)) {
> > +		err = ice_eswitch_br_set_pvid(port, vlan);
> > +		if (err)
> > +			goto err_set_pvid;
> > +	} else if ((flags & BRIDGE_VLAN_INFO_PVID) ||
> > +		   (flags & BRIDGE_VLAN_INFO_UNTAGGED)) {
> > +		dev_info(dev, "VLAN push and pop are supported only simultaneously\n");
> > +		return ERR_PTR(-EOPNOTSUPP);
> 
> Hi Wojciech,
> 
> Smatch thinks that vlan is being leaked here.
> So perhaps:
> 
> 		err = -EOPNOTSUPP;
> 		goto err_set_pvid;
> 
> .../ice_eswitch_br.c:709 ice_eswitch_br_vlan_create() warn: possible memory leak of 'vlan'

Thanks,
I'll fix that

> 
> 
> > +	}
> >
> >  	err = xa_insert(&port->vlans, vlan->vid, vlan, GFP_KERNEL);
> > -	if (err) {
> > -		kfree(vlan);
> > -		return ERR_PTR(err);
> > -	}
> > +	if (err)
> > +		goto err_insert;
> >
> >  	return vlan;
> > +
> > +err_insert:
> > +	if (port->pvid)
> > +		ice_eswitch_br_clear_pvid(port);
> > +err_set_pvid:
> > +	kfree(vlan);
> > +	return ERR_PTR(err);
> >  }
> >
> >  static int
> 
> ...