| 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
| ||
|
Date: Fri, 20 Aug 2021 02:18:49 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Vlad Buslov <vladbu@...dia.com>
Cc: Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org,
Jakub Kicinski <kuba@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Roopa Prabhu <roopa@...dia.com>,
Nikolay Aleksandrov <nikolay@...dia.com>,
Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
Vivien Didelot <vivien.didelot@...il.com>,
Vadym Kochan <vkochan@...vell.com>,
Taras Chornyi <tchornyi@...vell.com>,
Jiri Pirko <jiri@...dia.com>, Ido Schimmel <idosch@...dia.com>,
UNGLinuxDriver@...rochip.com,
Grygorii Strashko <grygorii.strashko@...com>,
Marek Behun <kabel@...ckhole.sk>,
DENG Qingfang <dqfext@...il.com>,
Kurt Kanzenbach <kurt@...utronix.de>,
Hauke Mehrtens <hauke@...ke-m.de>,
Woojung Huh <woojung.huh@...rochip.com>,
Sean Wang <sean.wang@...iatek.com>,
Landen Chao <Landen.Chao@...iatek.com>,
Claudiu Manoil <claudiu.manoil@....com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
George McCollister <george.mccollister@...il.com>,
Ioana Ciornei <ioana.ciornei@....com>,
Saeed Mahameed <saeedm@...dia.com>,
Leon Romanovsky <leon@...nel.org>,
Lars Povlsen <lars.povlsen@...rochip.com>,
Steen Hegelund <Steen.Hegelund@...rochip.com>,
Julian Wiedmann <jwi@...ux.ibm.com>,
Karsten Graul <kgraul@...ux.ibm.com>,
Heiko Carstens <hca@...ux.ibm.com>,
Vasily Gorbik <gor@...ux.ibm.com>,
Christian Borntraeger <borntraeger@...ibm.com>,
Ivan Vecera <ivecera@...hat.com>,
Jianbo Liu <jianbol@...dia.com>,
Mark Bloch <mbloch@...dia.com>, Roi Dayan <roid@...dia.com>,
Tobias Waldekranz <tobias@...dekranz.com>,
Vignesh Raghavendra <vigneshr@...com>,
Jesse Brandeburg <jesse.brandeburg@...el.com>
Subject: Re: [PATCH v2 net-next 1/5] net: switchdev: move
SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE to the blocking notifier chain
Hi Vlad,
On Thu, Aug 19, 2021 at 09:15:17PM +0300, Vlad Buslov wrote:
> On Thu 19 Aug 2021 at 19:07, Vladimir Oltean <vladimir.oltean@....com> wrote:
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c
> > index 0c38c2e319be..ea7c3f07f6fe 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c
> > @@ -276,6 +276,55 @@ mlx5_esw_bridge_port_obj_attr_set(struct net_device *dev,
> > return err;
> > }
> >
> > +static struct mlx5_bridge_switchdev_fdb_work *
> > +mlx5_esw_bridge_init_switchdev_fdb_work(struct net_device *dev, bool add,
> > + struct switchdev_notifier_fdb_info *fdb_info,
> > + struct mlx5_esw_bridge_offloads *br_offloads);
> > +
> > +static int
> > +mlx5_esw_bridge_fdb_event(struct net_device *dev, unsigned long event,
> > + struct switchdev_notifier_info *info,
> > + struct mlx5_esw_bridge_offloads *br_offloads)
> > +{
> > + struct switchdev_notifier_fdb_info *fdb_info;
> > + struct mlx5_bridge_switchdev_fdb_work *work;
> > + struct mlx5_eswitch *esw = br_offloads->esw;
> > + u16 vport_num, esw_owner_vhca_id;
> > + struct net_device *upper, *rep;
> > +
> > + upper = netdev_master_upper_dev_get_rcu(dev);
> > + if (!upper)
> > + return 0;
> > + if (!netif_is_bridge_master(upper))
> > + return 0;
> > +
> > + rep = mlx5_esw_bridge_rep_vport_num_vhca_id_get(dev, esw,
> > + &vport_num,
> > + &esw_owner_vhca_id);
> > + if (!rep)
> > + return 0;
> > +
> > + /* only handle the event on peers */
> > + if (mlx5_esw_bridge_is_local(dev, rep, esw))
> > + return 0;
>
> This check is only needed for SWITCHDEV_FDB_DEL_TO_BRIDGE case. Here it
> breaks the offload.
Very good point, thanks for looking. I copied the entire atomic notifier
handler and deleted the code which wasn't needed, but I actually took a
break while converting mlx5, and so I forgot to delete this part when I
came back.
> > +
> > + fdb_info = container_of(info, struct switchdev_notifier_fdb_info, info);
> > +
> > + work = mlx5_esw_bridge_init_switchdev_fdb_work(dev,
> > + event == SWITCHDEV_FDB_ADD_TO_DEVICE,
> > + fdb_info,
>
> Here FDB info can already be deallocated[1] since this is now executing
> asynchronously and races with fdb_rcu_free() that is scheduled to be
> called after rcu grace period by fdb_delete().
I am incredibly lucky that you caught this, apparently I needed to add
an msleep(1000) to see it as well.
It is not the struct switchdev_notifier_fdb_info *fdb_info that gets
freed under RCU. It is fdb_info->addr (the MAC address), since
switchdev_deferred_enqueue only performs a shallow copy. I will address
that in v3.
> > @@ -415,9 +470,7 @@ static int mlx5_esw_bridge_switchdev_event(struct notifier_block *nb,
> > /* only handle the event on peers */
> > if (mlx5_esw_bridge_is_local(dev, rep, esw))
> > break;
>
> I really like the idea of completely remove the driver wq from FDB
> handling code, but I'm not yet too familiar with bridge internals to
> easily determine whether same approach can be applied to
> SWITCHDEV_FDB_{ADD|DEL}_TO_BRIDGE event after this series is accepted.
> It seems that all current users already generate these events from
> blocking context, so would it be a trivial change for me to do in your
> opinion? That would allow me to get rid of mlx5_esw_bridge_offloads->wq
> in our driver.
If all callers really are in blocking context (and they do appear to be)
you can even forgo the switchdev_deferred_enqueue that switchdev_fdb_add_to_device
does, and just call_switchdev_blocking_notifiers() directly. Then you
move the bridge handler from br_switchdev_event() to br_switchdev_blocking_event().
It should be even simpler than this conversion.
Powered by blists - more mailing lists