On 2024-01-03 17:39, Jakub Kicinski wrote:

On Wed, 27 Dec 2023 17:46:30 -0800 David Wei wrote:

quoted

+static ssize_t nsim_dev_peer_write(struct file *file,
+				   const char __user *data,
+				   size_t count, loff_t *ppos)
+{
+	struct nsim_dev_port *nsim_dev_port, *peer_dev_port;
+	struct nsim_dev *peer_dev;
+	unsigned int id, port;
+	char buf[22];
+	ssize_t ret;
+
+	if (count >= sizeof(buf))
+		return -ENOSPC;
+
+	ret = copy_from_user(buf, data, count);
+	if (ret)
+		return -EFAULT;
+	buf[count] = '\0';
+
+	ret = sscanf(buf, "%u %u", &id, &port);
+	if (ret != 2) {
+		pr_err("Format is peer netdevsim \"id port\" (uint uint)\n");

netif_err() or dev_err() ? Granted the rest of the file seems to use
pr_err(), but I'm not sure why...

I can change it to use one of these two in this patchset, then I can
chnage the others separately in another patch. How does that sound?

quoted

+		return -EINVAL;
+	}

Could you put a sleep() here and test removing the device while some
thread is stuck here? I don't recall exactly but I thought debugfs
remove waits for concurrent reads and writes which could be problematic
given we take all the locks under the sun here..

Yep, I'll test this.

quoted

+	ret = -EINVAL;
+	mutex_lock(&nsim_dev_list_lock);
+	peer_dev = nsim_dev_find_by_id(id);
+	if (!peer_dev) {
+		pr_err("Peer netdevsim %u does not exist\n", id);
+		goto out_mutex;
+	}
+
+	devl_lock(priv_to_devlink(peer_dev));
+	rtnl_lock();
+	nsim_dev_port = file->private_data;
+	peer_dev_port = __nsim_dev_port_lookup(peer_dev, NSIM_DEV_PORT_TYPE_PF,
+					       port);
+	if (!peer_dev_port) {
+		pr_err("Peer netdevsim %u port %u does not exist\n", id, port);
+		goto out_devl;
+	}
+
+	if (nsim_dev_port == peer_dev_port) {
+		pr_err("Cannot link netdevsim to itself\n");
+		goto out_devl;
+	}
+
+	rcu_assign_pointer(nsim_dev_port->ns->peer, peer_dev_port->ns);
+	rcu_assign_pointer(peer_dev_port->ns->peer, nsim_dev_port->ns);
+	ret = count;
+
+out_devl:

out_unlock_rtnl

quoted

+	rtnl_unlock();
+	devl_unlock(priv_to_devlink(peer_dev));
+out_mutex:

out_unlock_dev_list

quoted

+	mutex_unlock(&nsim_dev_list_lock);
+
+	return ret;
+}
+
+static const struct file_operations nsim_dev_peer_fops = {
+	.open = simple_open,
+	.read = nsim_dev_peer_read,
+	.write = nsim_dev_peer_write,
+	.llseek = generic_file_llseek,

You don't support seek, you want some form of no_seek here.

quoted

+	.owner = THIS_MODULE,
+};