| 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: <MW4PR11MB5776844315E90B76E1DBCD64FD6A9@MW4PR11MB5776.namprd11.prod.outlook.com>
Date: Thu, 27 Apr 2023 10:28:21 +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 08/12] ice: Add VLAN FDB support in switchdev
mode
> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin@...el.com>
> Sent: piątek, 21 kwietnia 2023 17:25
> 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>
> 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)
Makes sense
>
> > 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.
Sure
>
> > + 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))
>
> ?
I like it
>
> > +
> > + 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).
Sure, I guess I can improve log msgs in other patches now.
>
> > + 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.
Sure
>
> > + 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;
> ...
Hmmm, I feel like it is more readable right now. It's clear that we're registering
switchdev blocking notifier block (switchdev_blk). Introducing generic variable (nb)
might a bit ambiguous IMO. So if you have nothing against it I'd leave it as it is.
>
> > + 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)
(same here)
>
> > 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?
Yup
>
> > int ifindex;
>
> (BTW, ifindex is also usually unsigned unless it's not an error)
It is defined as int in net_device, so I don’t know
>
> >
> > struct xarray ports;
> [...]
>
> Thanks,
> Olek
Powered by blists - more mailing lists