linux-kernel - Re: [net-next v3 5/6] net: marvell: prestera: Add Switchdev driver implementation

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]

Message-ID: <20200726083414.GD2216@nanopsycho>
Date:   Sun, 26 Jul 2020 10:34:14 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Vadym Kochan <vadym.kochan@...ision.eu>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        Ido Schimmel <idosch@...lanox.com>,
        Andrew Lunn <andrew@...n.ch>,
        Oleksandr Mazur <oleksandr.mazur@...ision.eu>,
        Serhiy Boiko <serhiy.boiko@...ision.eu>,
        Serhiy Pshyk <serhiy.pshyk@...ision.eu>,
        Volodymyr Mytnyk <volodymyr.mytnyk@...ision.eu>,
        Taras Chornyi <taras.chornyi@...ision.eu>,
        Andrii Savka <andrii.savka@...ision.eu>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Mickey Rachamim <mickeyr@...vell.com>
Subject: Re: [net-next v3 5/6] net: marvell: prestera: Add Switchdev driver
 implementation

Sat, Jul 25, 2020 at 05:06:50PM CEST, vadym.kochan@...ision.eu wrote:
>The following features are supported:
>
>    - VLAN-aware bridge offloading
>    - VLAN-unaware bridge offloading
>    - FDB offloading (learning, ageing)
>    - Switchport configuration
>
>Currently there are some limitations like:
>
>    - Only 1 VLAN-aware bridge instance supported
>    - FDB ageing timeout parameter is set globally per device
>
>Signed-off-by: Serhiy Boiko <serhiy.boiko@...ision.eu>
>Signed-off-by: Serhiy Pshyk <serhiy.pshyk@...ision.eu>
>Signed-off-by: Taras Chornyi <taras.chornyi@...ision.eu>
>Signed-off-by: Vadym Kochan <vadym.kochan@...ision.eu>
>---

[...]


>+static void prestera_fdb_event_work(struct work_struct *work)
>+{
>+	struct switchdev_notifier_fdb_info *fdb_info;
>+	struct prestera_fdb_event_work *swdev_work;
>+	struct prestera_port *port;
>+	struct net_device *dev;
>+	int err = 0;
>+
>+	swdev_work = container_of(work, struct prestera_fdb_event_work, work);
>+	dev = swdev_work->dev;
>+
>+	rtnl_lock();
>+
>+	port = prestera_port_dev_lower_find(dev);
>+	if (!port)
>+		goto out;
>+
>+	switch (swdev_work->event) {
>+	case SWITCHDEV_FDB_ADD_TO_DEVICE:
>+		fdb_info = &swdev_work->fdb_info;
>+		if (!fdb_info->added_by_user)
>+			break;
>+
>+		err = prestera_port_fdb_set(port, fdb_info, true);
>+		if (err)
>+			break;
>+
>+		prestera_fdb_offload_notify(port, fdb_info);
>+		break;
>+
>+	case SWITCHDEV_FDB_DEL_TO_DEVICE:
>+		fdb_info = &swdev_work->fdb_info;
>+		prestera_port_fdb_set(port, fdb_info, false);
>+		break;
>+	}
>+
>+out:
>+	rtnl_unlock();
>+
>+	kfree(swdev_work->fdb_info.addr);
>+	kfree(swdev_work);
>+	dev_put(dev);
>+}
>+
>+static int prestera_switchdev_event(struct notifier_block *unused,
>+				    unsigned long event, void *ptr)
>+{
>+	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
>+	struct switchdev_notifier_fdb_info *fdb_info;
>+	struct switchdev_notifier_info *info = ptr;
>+	struct prestera_fdb_event_work *swdev_work;
>+	struct net_device *upper;
>+	int err = 0;
>+
>+	if (event == SWITCHDEV_PORT_ATTR_SET) {
>+		err = switchdev_handle_port_attr_set(dev, ptr,
>+						     prestera_netdev_check,
>+						     prestera_port_obj_attr_set);
>+		return notifier_from_errno(err);
>+	}
>+
>+	upper = netdev_master_upper_dev_get_rcu(dev);
>+	if (!upper)
>+		return NOTIFY_DONE;
>+
>+	if (!netif_is_bridge_master(upper))
>+		return NOTIFY_DONE;

Okay, you support upper bridge. Of which interface? I believe you should
put prestera_netdev_check(dev) check here and avoid the lookup in the
work. Otherwise any chain of intermediate lower devices would be
supported, which is wrong.


>+
>+	swdev_work = kzalloc(sizeof(*swdev_work), GFP_ATOMIC);
>+	if (!swdev_work)
>+		return NOTIFY_BAD;
>+
>+	swdev_work->event = event;
>+	swdev_work->dev = dev;
>+
>+	switch (event) {
>+	case SWITCHDEV_FDB_ADD_TO_DEVICE:
>+	case SWITCHDEV_FDB_DEL_TO_DEVICE:
>+		fdb_info = container_of(info,
>+					struct switchdev_notifier_fdb_info,
>+					info);
>+
>+		INIT_WORK(&swdev_work->work, prestera_fdb_event_work);
>+		memcpy(&swdev_work->fdb_info, ptr,
>+		       sizeof(swdev_work->fdb_info));
>+
>+		swdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
>+		if (!swdev_work->fdb_info.addr)
>+			goto out;
>+
>+		ether_addr_copy((u8 *)swdev_work->fdb_info.addr,
>+				fdb_info->addr);
>+		dev_hold(dev);
>+
>+		break;
>+
>+	default:
>+		kfree(swdev_work);
>+		return NOTIFY_DONE;
>+	}
>+
>+	queue_work(swdev_wq, &swdev_work->work);
>+	return NOTIFY_DONE;
>+out:
>+	kfree(swdev_work);
>+	return NOTIFY_BAD;
>+}

[...]