netdev - Re: [PATCH v2 net-next 1/5] net: switchdev: move SWITCHDEV_FDB_{ADD,DEL}_TO

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 20 Aug 2021 10:36:41 +0300
From:   Vlad Buslov <vladbu@...dia.com>
To:     Vladimir Oltean <olteanv@...il.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

On Fri 20 Aug 2021 at 02:18, Vladimir Oltean <olteanv@...il.com> wrote:
> 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.

Thanks for your advice! I'll start looking into it as soon as this
series is accepted.