| 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: <72f95cf5-d922-1f3d-2495-e8bdea6de247@intel.com>
Date: Fri, 21 Apr 2023 18:35:28 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
<david.m.ertman@...el.com>, <michal.swiatkowski@...ux.intel.com>,
<marcin.szycik@...ux.intel.com>, <pawel.chmielewski@...el.com>,
<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?
> + 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()?
> + "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)
> + }
> +
> + 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)
> + 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.
> };
>
> 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.
> +}
> +
> +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.
> +
> + 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.
> + 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