| 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
| ||
|
Message-ID: <MW4PR11MB5776A66941471BB2ABF7E99DFD769@MW4PR11MB5776.namprd11.prod.outlook.com>
Date: Tue, 9 May 2023 11:25:40 +0000
From: "Drewek, Wojciech" <wojciech.drewek@...el.com>
To: "Lobakin, Aleksander" <aleksander.lobakin@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "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>
Subject: RE: [PATCH net-next 09/12] ice: implement bridge port vlan
> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin@...el.com>
> Sent: piątek, 21 kwietnia 2023 18:35
> To: Drewek, Wojciech <wojciech.drewek@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; 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>
> Subject: Re: [PATCH net-next 09/12] ice: implement bridge port vlan
>
> From: Wojciech Drewek <wojciech.drewek@...el.com>
> Date: Mon, 17 Apr 2023 11:34:09 +0200
>
> > 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 trafic received by VSI in port VLAN mode
> > - port VLAN mode on uplink port isn't supported
>
> [...]
>
> > @@ -610,11 +612,26 @@ ice_eswitch_br_vlan_filtering_set(struct ice_esw_br *bridge, bool enable)
> > bridge->flags &= ~ICE_ESWITCH_BR_VLAN_FILTERING;
> > }
> >
> > +static void
> > +ice_eswitch_br_clear_pvid(struct ice_esw_br_port *port)
> > +{
> > + struct ice_vsi_vlan_ops *vlan_ops =
> > + ice_get_compat_vsi_vlan_ops(port->vsi);
> > +
>
> Deref in a separate line to avoid breaking?
sure
>
> > + vlan_ops->clear_port_vlan(port->vsi);
> > +
> > + ice_vf_vsi_disable_port_vlan(port->vsi);
> > +
> > + port->pvid = 0;
> > +}
> > +
> > static void
> > ice_eswitch_br_vlan_cleanup(struct ice_esw_br_port *port,
> > struct ice_esw_br_vlan *vlan)
> > {
> > xa_erase(&port->vlans, vlan->vid);
> > + if (port->pvid == vlan->vid)
> > + ice_eswitch_br_clear_pvid(port);
> > kfree(vlan);
> > }
> >
> > @@ -627,9 +644,50 @@ static void ice_eswitch_br_port_vlans_flush(struct ice_esw_br_port *port)
> > ice_eswitch_br_vlan_cleanup(port, vlan);
> > }
> >
> > +static int
> > +ice_eswitch_br_set_pvid(struct ice_esw_br_port *port,
> > + struct ice_esw_br_vlan *vlan)
> > +{
> > + struct ice_vlan port_vlan = ICE_VLAN(ETH_P_8021Q, vlan->vid, 0);
> > + struct device *dev = ice_pf_to_dev(port->vsi->back);
> > + struct ice_vsi_vlan_ops *vlan_ops;
> > + int err;
> > +
> > + if (port->pvid == vlan->vid || vlan->vid == 1)
> > + return 0;
> > +
> > + /* Setting port vlan on uplink isn't supported by hw */
> > + if (port->type == ICE_ESWITCH_BR_UPLINK_PORT)
> > + return -EOPNOTSUPP;
> > +
> > + if (port->pvid) {
> > + dev_info(dev,
>
> dev_err()?
To me it's not an error, port vlan is already configured
>
> > + "Port VLAN (vsi=%u, vid=%u) already exists on the port, remove it before adding new one\n",
> > + port->vsi_idx, port->pvid);
> > + return -EEXIST;
>
> Hmm, isn't -EBUSY more common for such cases?
>
> (below as well)
I don't think so, user is trying to configure something that is already done.
>
> > + }
> > +
> > + ice_vf_vsi_enable_port_vlan(port->vsi);
>
> [...]
>
> > @@ -639,14 +697,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");
>
> (same for dev_err(), as well as below)
Again, is this an error really? We just don't support such case.
>
> > + return ERR_PTR(-EOPNOTSUPP);
> > + }
>
> [...]
>
> > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> > index cf3e2615a62a..b6eef068ea81 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> > @@ -43,6 +43,7 @@ struct ice_esw_br_port {
> > struct ice_vsi *vsi;
> > u16 vsi_idx;
> > struct xarray vlans;
> > + u16 pvid;
>
> Oh, or you can just stack ::vsi_idx with ::pvid here to avoid spawning
> holes.
Sure
>
> > };
> >
> > enum {
> > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_vsi_vlan_ops.c b/drivers/net/ethernet/intel/ice/ice_vf_vsi_vlan_ops.c
> > index b1ffb81893d4..447b4e6ef7e4 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_vf_vsi_vlan_ops.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_vf_vsi_vlan_ops.c
> > @@ -21,6 +21,108 @@ noop_vlan(struct ice_vsi __always_unused *vsi)
> > return 0;
> > }
> >
> > +static void ice_port_vlan_on(struct ice_vsi *vsi)
> > +{
> > + struct ice_vsi_vlan_ops *vlan_ops;
> > + struct ice_pf *pf = vsi->back;
> > +
> > + if (ice_is_dvm_ena(&pf->hw)) {
> > + vlan_ops = &vsi->outer_vlan_ops;
> > +
> > + /* setup outer VLAN ops */
> > + vlan_ops->set_port_vlan = ice_vsi_set_outer_port_vlan;
> > + vlan_ops->clear_port_vlan = ice_vsi_clear_outer_port_vlan;
> > + vlan_ops->clear_port_vlan = ice_vsi_clear_outer_port_vlan;
> > + vlan_ops->ena_rx_filtering =
> > + ice_vsi_ena_rx_vlan_filtering;
> > +
> > + /* setup inner VLAN ops */
> > + vlan_ops = &vsi->inner_vlan_ops;
> > + vlan_ops->add_vlan = noop_vlan_arg;
> > + vlan_ops->del_vlan = noop_vlan_arg;
> > + vlan_ops->ena_stripping = ice_vsi_ena_inner_stripping;
> > + vlan_ops->dis_stripping = ice_vsi_dis_inner_stripping;
> > + vlan_ops->ena_insertion = ice_vsi_ena_inner_insertion;
> > + vlan_ops->dis_insertion = ice_vsi_dis_inner_insertion;
> > + } else {
> > + vlan_ops = &vsi->inner_vlan_ops;
> > +
> > + vlan_ops->set_port_vlan = ice_vsi_set_inner_port_vlan;
> > + vlan_ops->clear_port_vlan = ice_vsi_clear_inner_port_vlan;
> > + vlan_ops->clear_port_vlan = ice_vsi_clear_inner_port_vlan;
> > + vlan_ops->ena_rx_filtering =
> > + ice_vsi_ena_rx_vlan_filtering;
> > + }
>
> ->ena_rx_filtering is filled with just one possible value, so it could
> be done outside ifs.
Agree
>
> > +}
> > +
> > +static void ice_port_vlan_off(struct ice_vsi *vsi)
> > +{
> > + struct ice_vsi_vlan_ops *vlan_ops;
> > + struct ice_pf *pf = vsi->back;
> > +
> > + if (ice_is_dvm_ena(&pf->hw)) {
> > + /* setup inner VLAN ops */
> > + vlan_ops = &vsi->inner_vlan_ops;
> > +
> > + vlan_ops->ena_stripping = ice_vsi_ena_inner_stripping;
> > + vlan_ops->dis_stripping = ice_vsi_dis_inner_stripping;
> > + vlan_ops->ena_insertion = ice_vsi_ena_inner_insertion;
> > + vlan_ops->dis_insertion = ice_vsi_dis_inner_insertion;
> > +
> > + vlan_ops = &vsi->outer_vlan_ops;
> > +
> > + vlan_ops->del_vlan = ice_vsi_del_vlan;
> > + vlan_ops->ena_stripping = ice_vsi_ena_outer_stripping;
> > + vlan_ops->dis_stripping = ice_vsi_dis_outer_stripping;
> > + vlan_ops->ena_insertion = ice_vsi_ena_outer_insertion;
> > + vlan_ops->dis_insertion = ice_vsi_dis_outer_insertion;
> > + } else {
> > + vlan_ops = &vsi->inner_vlan_ops;
> > +
> > + vlan_ops->del_vlan = ice_vsi_del_vlan;
> > + vlan_ops->ena_stripping = ice_vsi_ena_inner_stripping;
> > + vlan_ops->dis_stripping = ice_vsi_dis_inner_stripping;
> > + vlan_ops->ena_insertion = ice_vsi_ena_inner_insertion;
> > + vlan_ops->dis_insertion = ice_vsi_dis_inner_insertion;
> > + }
>
> The whole ->inner_vlan_ops is filled with the same values, the only
> difference is ->del_vlan, which can be left in `else`, the rest can be
> set up unconditionally.
Makes sense
>
> > +
> > + if (!test_bit(ICE_FLAG_VF_VLAN_PRUNING, pf->flags))
> > + vlan_ops->ena_rx_filtering = noop_vlan;
> > + else
> > + vlan_ops->ena_rx_filtering =
> > + ice_vsi_ena_rx_vlan_filtering;
> > +}
> > +
> > +/**
> > + * ice_vf_vsi_enable_port_vlan - Set VSI VLAN ops to support port VLAN
> > + * @vsi: VF's VSI being configured
> > + *
> > + * The function won't create port VLAN, it only allows to create port VLAN
> > + * using VLAN ops on the VF VSI.
> > + */
> > +void ice_vf_vsi_enable_port_vlan(struct ice_vsi *vsi)
> > +{
> > + if (WARN_ON(!vsi->vf))
>
> I'd use WARN_ON_ONCE(). Otherwise, it may be possible to flood kernel
> log buffer (-> CPU) from the userspace.
Sure
>
> > + return;
> > +
> > + ice_port_vlan_on(vsi);
> > +}
> > +
> > +/**
> > + * ice_vf_vsi_disable_port_vlan - Clear VSI support for creating port VLAN
> > + * @vsi: VF's VSI being configured
> > + *
> > + * The function should be called after removing port VLAN on VSI
> > + * (using VLAN ops)
> > + */
> > +void ice_vf_vsi_disable_port_vlan(struct ice_vsi *vsi)
> > +{
> > + if (WARN_ON(!vsi->vf))
>
> (same)
>
> > + return;
> > +
> > + ice_port_vlan_off(vsi);
> > +}
>
> [...]
>
> > + info->valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_VLAN_VALID |
> > + ICE_AQ_VSI_PROP_SW_VALID);
> > +
> > + ret = ice_update_vsi(hw, vsi->idx, ctxt, NULL);
> > + if (ret)
> > + dev_info(ice_hw_to_dev(hw), "update VSI for port VLAN failed, err %d aq_err %s\n",
>
> (dev_err())
> (+ %pe)
>
> > + ret, ice_aq_str(hw->adminq.sq_last_status));
> > +
> > + kfree(ctxt);
> > + return ret;
> > +}
> [...]
>
> Thanks,
> Olek
Powered by blists - more mailing lists