Re: [PATCH net-next 06/12] ice: Implement basic eswitch bridge setup

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Mon, Jun 26, 2023 at 05:31:14PM +0300, Vlad Buslov wrote:
> On Mon 26 Jun 2023 at 16:26, Michal Swiatkowski <michal.swiatkowski@linux.intel.com> wrote:
> > On Tue, Jun 20, 2023 at 10:44:17AM -0700, Tony Nguyen wrote:
> >> From: Wojciech Drewek <wojciech.drewek@intel.com>
> >> 
> >> With this patch, ice driver is able to track if the port
> >> representors or uplink port were added to the linux bridge in
> >> switchdev mode. Listen for NETDEV_CHANGEUPPER events in order to
> >> detect this. ice_esw_br data structure reflects the linux bridge
> >> and stores all the ports of the bridge (ice_esw_br_port) in
> >> xarray, it's created when the first port is added to the bridge and
> >> freed once the last port is removed. Note that only one bridge is
> >> supported per eswitch.
> >> 
> >> Bridge port (ice_esw_br_port) can be either a VF port representor
> >> port or uplink port (ice_esw_br_port_type). In both cases bridge port
> >> holds a reference to the VSI, VF's VSI in case of the PR and uplink
> >> VSI in case of the uplink. VSI's index is used as an index to the
> >> xarray in which ports are stored.
> >> 
> >> Add a check which prevents configuring switchdev mode if uplink is
> >> already added to any bridge. This is needed because we need to listen
> >> for NETDEV_CHANGEUPPER events to record if the uplink was added to
> >> the bridge. Netdevice notifier is registered after eswitch mode
> >> is changed to switchdev.
> >> 
> >> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> >> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> >> Tested-by: Sujai Buvaneswaran <sujai.buvaneswaran@intel.com>
> >> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> >> ---
> >>  drivers/net/ethernet/intel/ice/Makefile       |   2 +-
> >>  drivers/net/ethernet/intel/ice/ice.h          |   4 +-
> >>  drivers/net/ethernet/intel/ice/ice_eswitch.c  |  26 +-
> >>  .../net/ethernet/intel/ice/ice_eswitch_br.c   | 384 ++++++++++++++++++
> >>  .../net/ethernet/intel/ice/ice_eswitch_br.h   |  42 ++
> >>  drivers/net/ethernet/intel/ice/ice_main.c     |   2 +-
> >>  drivers/net/ethernet/intel/ice/ice_repr.c     |   2 +-
> >>  drivers/net/ethernet/intel/ice/ice_repr.h     |   3 +-
> >>  8 files changed, 456 insertions(+), 9 deletions(-)
> >>  create mode 100644 drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> >>  create mode 100644 drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> >> +
> >> +static int
> >> +ice_eswitch_br_port_changeupper(struct notifier_block *nb, void *ptr)
> >> +{
> >> +	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> >> +	struct netdev_notifier_changeupper_info *info = ptr;
> >> +	struct ice_esw_br_offloads *br_offloads;
> >> +	struct netlink_ext_ack *extack;
> >> +	struct net_device *upper;
> >> +
> >> +	br_offloads = ice_nb_to_br_offloads(nb, netdev_nb);
> >> +
> >> +	if (!ice_eswitch_br_is_dev_valid(dev))
> >> +		return 0;
> >> +
> >> +	upper = info->upper_dev;
> >> +	if (!netif_is_bridge_master(upper))
> >> +		return 0;
> >> +
> >> +	extack = netdev_notifier_info_to_extack(&info->info);
> >> +
> >> +	if (info->linking)
> >> +		return ice_eswitch_br_port_link(br_offloads, dev,
> >> +						upper->ifindex, extack);
> >> +	else
> >> +		return ice_eswitch_br_port_unlink(br_offloads, dev,
> >> +						  upper->ifindex, extack);
> >> +}
> >> +
> >> +static int
> >> +ice_eswitch_br_port_event(struct notifier_block *nb,
> >> +			  unsigned long event, void *ptr)
> >> +{
> >> +	int err = 0;
> >> +
> >> +	switch (event) {
> >> +	case NETDEV_CHANGEUPPER:
> >> +		err = ice_eswitch_br_port_changeupper(nb, ptr);
> >> +		break;
> >> +	}
> >> +
> >> +	return notifier_from_errno(err);
> >> +}
> > Hi Vlad,
> >
> > We found out that adding VF and corresponding port representor to the
> > bridge cause loop in the bridge. Packets are looping through the bridge.
> > I know that it isn't valid configuration, howevere, it can happen and
> > after that the server is quite unstable.
> >
> > Does mellanox validate the port for this scenario? Or we should assume
> > that user will add port wisely? I was looking at your code, but didn't
> > find that. You are using NETDEV_PRECHANGEUPPER, do you think we should
> > validate if user is trying to add VF when his PR is currently added?
> 
> Hmm, no, it is not something we validate. Also, I assume it will be
> quite tricky to properly test for it, since user could try to add some
> other netdevice connected to the VF (VLAN, tunneling dev, bonding, etc.)
> which will probably lead to same result.
> 

Agree, thanks. As Jakub wrote, STP should be turned on to prevent this kind
of problem.