| 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: <d3cc5703-bc8d-9e5e-c354-94dde3b1e91c@intel.com>
Date: Fri, 21 Apr 2023 17:25:23 +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>,
<alexandr.lobakin@...el.com>, <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 08/12] ice: Add VLAN FDB support in switchdev
mode
From: Wojciech Drewek <wojciech.drewek@...el.com>
Date: Mon, 17 Apr 2023 11:34:08 +0200
> From: Marcin Szycik <marcin.szycik@...el.com>
>
> Add support for matching on VLAN tag in bridge offloads.
> Currently only trunk mode is supported.
>
> To enable VLAN filtering (existing FDB entries will be deleted):
> ip link set $BR type bridge vlan_filtering 1
>
> To add VLANs to bridge in trunk mode:
> bridge vlan add dev $PF1 vid 110-111
> bridge vlan add dev $VF1_PR vid 110-111
>
> Signed-off-by: Marcin Szycik <marcin.szycik@...el.com>
> ---
> .../net/ethernet/intel/ice/ice_eswitch_br.c | 319 +++++++++++++++++-
> .../net/ethernet/intel/ice/ice_eswitch_br.h | 12 +
> 2 files changed, 317 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> index 49381e4bf62a..56d36e397b12 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> @@ -59,13 +59,19 @@ ice_eswitch_br_netdev_to_port(struct net_device *dev)
> static void
> ice_eswitch_br_ingress_rule_setup(struct ice_adv_lkup_elem *list,
> struct ice_adv_rule_info *rule_info,
> - const unsigned char *mac,
> + const unsigned char *mac, bool vlan, u16 vid,
Could we use one combined argument? Doesn't `!!vid == !!vlan`? VID 0 is
reserved IIRC...
(same in all the places below)
> u8 pf_id, u16 vf_vsi_idx)
> {
> list[0].type = ICE_MAC_OFOS;
> ether_addr_copy(list[0].h_u.eth_hdr.dst_addr, mac);
> eth_broadcast_addr(list[0].m_u.eth_hdr.dst_addr);
[...]
> @@ -344,10 +389,33 @@ ice_eswitch_br_fdb_entry_create(struct net_device *netdev,
> struct device *dev = ice_pf_to_dev(pf);
> struct ice_esw_br_fdb_entry *fdb_entry;
> struct ice_esw_br_flow *flow;
> + struct ice_esw_br_vlan *vlan;
> struct ice_hw *hw = &pf->hw;
> + bool add_vlan = false;
> unsigned long event;
> int err;
>
> + /* FIXME: untagged filtering is not yet supported
> + */
Shouldn't be present in release code I believe. I mean, the sentence is
fine (just don't forget dot at the end), but without "FIXME:". And it
can be one-liner.
> + if (!(bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING) && vid)
> + return;
[...]
> +static void
> +ice_eswitch_br_vlan_filtering_set(struct ice_esw_br *bridge, bool enable)
> +{
> + bool filtering = bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING;
> +
> + if (filtering == enable)
> + return;
if (enable == !!(bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING))
?
> +
> + ice_eswitch_br_fdb_flush(bridge);
> + if (enable)
> + bridge->flags |= ICE_ESWITCH_BR_VLAN_FILTERING;
> + else
> + bridge->flags &= ~ICE_ESWITCH_BR_VLAN_FILTERING;
> +}
[...]
> + port = xa_load(&bridge->ports, vsi_idx);
> + if (!port)
> + return -EINVAL;
> +
> + vlan = xa_load(&port->vlans, vid);
> + if (vlan) {
> + if (vlan->flags == flags)
> + return 0;
> +
> + ice_eswitch_br_vlan_cleanup(port, vlan);
> + }
> +
> + vlan = ice_eswitch_br_vlan_create(vid, flags, port);
> + if (IS_ERR(vlan)) {
> + NL_SET_ERR_MSG_MOD(extack, "Failed to create VLAN entry");
FYI, there's NL_SET_ERR_MSG_FMT_MOD() landed recently (a couple releases
back), which supports format strings. E.g. you could pass VID, VSI ID,
flags etc. there to have more meaningful output (right in userspace).
> + return PTR_ERR(vlan);
> + }
> +
> + return 0;
> +}
[...]
> +static int
> +ice_eswitch_br_port_obj_add(struct net_device *netdev, const void *ctx,
> + const struct switchdev_obj *obj,
> + struct netlink_ext_ack *extack)
> +{
> + struct ice_esw_br_port *br_port = ice_eswitch_br_netdev_to_port(netdev);
> + struct switchdev_obj_port_vlan *vlan;
> + int err;
> +
> + if (!br_port)
> + return -EINVAL;
> +
> + switch (obj->id) {
> + case SWITCHDEV_OBJ_ID_PORT_VLAN:
> + vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
> + err = ice_eswitch_br_port_vlan_add(br_port->bridge,
> + br_port->vsi_idx, vlan->vid,
> + vlan->flags, extack);
return right here? You have `default` in the switch block, so the
compiler shouldn't complain if you remove it from the end of the func.
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return err;
> +}
> +
> +static int
> +ice_eswitch_br_port_obj_del(struct net_device *netdev, const void *ctx,
> + const struct switchdev_obj *obj)
> +{
> + struct ice_esw_br_port *br_port = ice_eswitch_br_netdev_to_port(netdev);
> + struct switchdev_obj_port_vlan *vlan;
> +
> + if (!br_port)
> + return -EINVAL;
> +
> + switch (obj->id) {
> + case SWITCHDEV_OBJ_ID_PORT_VLAN:
> + vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
> + ice_eswitch_br_port_vlan_del(br_port->bridge, br_port->vsi_idx,
> + vlan->vid);
(same)
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +ice_eswitch_br_port_obj_attr_set(struct net_device *netdev, const void *ctx,
> + const struct switchdev_attr *attr,
> + struct netlink_ext_ack *extack)
> +{
> + struct ice_esw_br_port *br_port = ice_eswitch_br_netdev_to_port(netdev);
> +
> + if (!br_port)
> + return -EINVAL;
> +
> + switch (attr->id) {
> + case SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING:
> + ice_eswitch_br_vlan_filtering_set(br_port->bridge,
> + attr->u.vlan_filtering);
(and here)
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
[...]
> + br_offloads->switchdev_blk.notifier_call =
> + ice_eswitch_br_event_blocking;
Oh, you have two usages of ->switchdev_blk here, so you can add an
intermediate variable to avoid line breaking, which would also shorten
the line below :D
nb = &br_offloads->switchdev_blk;
nb->notifier_call = ice_eswitch_br_event_blocking;
...
> + err = register_switchdev_blocking_notifier(&br_offloads->switchdev_blk);
> + if (err) {
> + dev_err(dev,
> + "Failed to register bridge blocking switchdev notifier\n");
> + goto err_reg_switchdev_blk;
> + }
> +
> br_offloads->netdev_nb.notifier_call = ice_eswitch_br_port_event;
> err = register_netdevice_notifier(&br_offloads->netdev_nb);
(here the same, but no line breaks, so up to you. You could reuse the
same variable or leave it as it is)
> if (err) {
[...]
> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> index 73ad81bad655..cf3e2615a62a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> @@ -42,10 +42,16 @@ struct ice_esw_br_port {
> enum ice_esw_br_port_type type;
> struct ice_vsi *vsi;
> u16 vsi_idx;
> + struct xarray vlans;
Hmm, I feel like you can make ::type u16 and then stack it with
::vsi_idx, so that you avoid a hole here.
> +};
> +
> +enum {
> + ICE_ESWITCH_BR_VLAN_FILTERING = BIT(0),
> };
>
> struct ice_esw_br {
> struct ice_esw_br_offloads *br_offloads;
> + int flags;
Unsigned types fit flags better I think?
> int ifindex;
(BTW, ifindex is also usually unsigned unless it's not an error)
>
> struct xarray ports;
[...]
Thanks,
Olek
Powered by blists - more mailing lists