| 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: <424fbf9a-f060-87d1-cad2-36e2ef732bd6@gmail.com>
Date: Wed, 5 Jul 2017 12:35:46 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Arkadi Sharshevsky <arkadis@...lanox.com>, netdev@...r.kernel.org
Cc: davem@...emloft.net, jiri@...nulli.us, ivecera@...hat.com,
andrew@...n.ch, vivien.didelot@...oirfairelinux.com,
Woojung.Huh@...rochip.com, stephen@...workplumber.org,
mlxsw@...lanox.com
Subject: Re: [PATCH net-next RFC 05/12] net: dsa: Add support for learning FDB
through notification
On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote:
> Add support for learning FDB through notification. The driver defers
> the hardware update via ordered work queue. In case of a successful
> FDB add a notification is sent back to bridge.
>
> Signed-off-by: Arkadi Sharshevsky <arkadis@...lanox.com>
> ---
> net/dsa/slave.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 122 insertions(+), 2 deletions(-)
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 19395cc..d0592f2 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1263,19 +1263,139 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
> return NOTIFY_DONE;
> }
>
> +struct dsa_switchdev_event_work {
> + struct work_struct work;
> + struct switchdev_notifier_fdb_info fdb_info;
> + struct net_device *dev;
> + unsigned long event;
> +};
> +
> +static void dsa_switchdev_event_work(struct work_struct *work)
> +{
> + struct dsa_switchdev_event_work *switchdev_work =
> + container_of(work, struct dsa_switchdev_event_work, work);
> + struct net_device *dev = switchdev_work->dev;
> + struct switchdev_notifier_fdb_info *fdb_info;
> + struct dsa_slave_priv *p = netdev_priv(dev);
> + int err;
> +
> + rtnl_lock();
> + switch (switchdev_work->event) {
> + case SWITCHDEV_FDB_ADD_TO_DEVICE:
> + fdb_info = &switchdev_work->fdb_info;
You could probably move this assignment outside of the switch()/case
statement since it is repeated
> + err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid);
> + if (err) {
> + netdev_dbg(dev, "fdb add failed err=%d\n", err);
> + break;
> + }
> + call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
> + &fdb_info->info);
> + break;
> +
> + case SWITCHDEV_FDB_DEL_TO_DEVICE:
> + fdb_info = &switchdev_work->fdb_info;
> + err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid);
> + if (err)
> + netdev_dbg(dev, "fdb del failed err=%d\n", err);
OK I must have missed from the off-list discussion why we are not
calling the switchdev notifier here?
> + break;
> + }
> + rtnl_unlock();
> +
> + kfree(switchdev_work->fdb_info.addr);
> + kfree(switchdev_work);
> + dev_put(dev);
> +}
> +
> +static int
> +dsa_slave_switchdev_fdb_work_init(struct dsa_switchdev_event_work *
> + switchdev_work,
> + const struct switchdev_notifier_fdb_info *
> + fdb_info)
> +{
> + memcpy(&switchdev_work->fdb_info, fdb_info,
> + sizeof(switchdev_work->fdb_info));
> + switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
> + if (!switchdev_work->fdb_info.addr)
> + return -ENOMEM;
Can't we have the fdb_info structure contain pre-allocated 6 bytes
storage for address, it's really silly to do this here and every time we
need to offload an FDB entry.
> + ether_addr_copy((u8 *)switchdev_work->fdb_info.addr,
> + fdb_info->addr);
> + return 0;
> +}
> +
> +/* Called under rcu_read_lock() */
> +static int dsa_slave_switchdev_event(struct notifier_block *unused,
> + unsigned long event, void *ptr)
> +{
> + struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
> + struct dsa_switchdev_event_work *switchdev_work;
> +
> + if (!dsa_slave_dev_check(dev))
> + return NOTIFY_DONE;
> +
> + switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
> + if (!switchdev_work)
> + return NOTIFY_BAD;
Same here, can we try to re-use a heap allocated workqueue item and we
just re-initialize it when we have to?
> +
> + INIT_WORK(&switchdev_work->work, dsa_switchdev_event_work);
> + switchdev_work->dev = dev;
> + switchdev_work->event = event;
> +
> + switch (event) {
> + case SWITCHDEV_FDB_ADD_TO_DEVICE: /* fall through */
> + case SWITCHDEV_FDB_DEL_TO_DEVICE:
> + if (dsa_slave_switchdev_fdb_work_init(switchdev_work,
> + ptr))
> + goto err_fdb_work_init;
> + dev_hold(dev);
> + break;
> + default:
> + kfree(switchdev_work);
> + return NOTIFY_DONE;
> + }
> +
> + dsa_schedule_work(&switchdev_work->work);
> + return NOTIFY_OK;
> +
> +err_fdb_work_init:
> + kfree(switchdev_work);
> + return NOTIFY_BAD;
> +}
> +
> static struct notifier_block dsa_slave_nb __read_mostly = {
> - .notifier_call = dsa_slave_netdevice_event,
> + .notifier_call = dsa_slave_netdevice_event,
> +};
> +
> +static struct notifier_block dsa_slave_switchdev_notifier = {
> + .notifier_call = dsa_slave_switchdev_event,
> };
>
> int dsa_slave_register_notifier(void)
> {
> - return register_netdevice_notifier(&dsa_slave_nb);
> + int err;
> +
> + err = register_netdevice_notifier(&dsa_slave_nb);
> + if (err)
> + return err;
> +
> + err = register_switchdev_notifier(&dsa_slave_switchdev_notifier);
> + if (err)
> + goto err_register_switchdev;
this label is a bit misnamed since it really deals with the slave dsa
notifier, how about no label or just renaming it "error_slave_nb"?
> +
> + return 0;
> +
> +err_register_switchdev:
> + unregister_netdevice_notifier(&dsa_slave_nb);
> + return err;
> }
>
> void dsa_slave_unregister_notifier(void)
> {
> int err;
>
> + err = unregister_netdevice_notifier(&dsa_slave_switchdev_notifier);
> + if (err)
> + pr_err("DSA: failed to unregister switchdev notifier (%d)\n", err);
> +
> err = unregister_netdevice_notifier(&dsa_slave_nb);
> if (err)
> pr_err("DSA: failed to unregister slave notifier (%d)\n", err);
>
--
Florian
Powered by blists - more mailing lists