netdev - Re: [PATCH net-next v8 1/4] netdevsim: allow two netdevsim ports to be connected

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: <20240131165739.53cf301e@kernel.org>
Date: Wed, 31 Jan 2024 16:57:39 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: David Wei <dw@...idwei.uk>
Cc: Jiri Pirko <jiri@...nulli.us>, Sabrina Dubroca <sd@...asysnail.net>,
 netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v8 1/4] netdevsim: allow two netdevsim ports to
 be connected

On Tue, 30 Jan 2024 13:46:17 -0800 David Wei wrote:
> +	err = -EINVAL;
> +	rtnl_lock();
> +	ns_a = get_net_ns_by_id(current->nsproxy->net_ns, netnsid_a);

I think we should support local netns, i.e. the one which we are
currently in. But by definition it has no id. How about we make the
netns id signed and make -1 mean "use current->nsproxy->net_ns
directly"?

Also you can look up the netns before taking rtnl_lock, they are 
not protected by rtnl_lock.

> +	if (!ns_a) {
> +		pr_err("Could not find netns with id: %u\n", netnsid_a);
> +		goto out_unlock_rtnl;
> +	}
> +
> +	dev_a = __dev_get_by_index(ns_a, ifidx_a);
> +	if (!dev_a) {
> +		pr_err("Could not find device with ifindex %u in netnsid %u\n", ifidx_a, netnsid_a);
> +		goto out_put_netns_a;
> +	}
> +
> +	if (!netdev_is_nsim(dev_a)) {
> +		pr_err("Device with ifindex %u in netnsid %u is not a netdevsim\n", ifidx_a, netnsid_a);
> +		goto out_put_netns_a;
> +	}
> +
> +	ns_b = get_net_ns_by_id(current->nsproxy->net_ns, netnsid_b);
> +	if (!ns_b) {
> +		pr_err("Could not find netns with id: %u\n", netnsid_b);
> +		goto out_put_netns_a;
> +	}
> +
> +	dev_b = __dev_get_by_index(ns_b, ifidx_b);
> +	if (!dev_b) {
> +		pr_err("Could not find device with ifindex %u in netnsid %u\n", ifidx_b, netnsid_b);
> +		goto out_put_netns_b;
> +	}
> +
> +	if (!netdev_is_nsim(dev_b)) {
> +		pr_err("Device with ifindex %u in netnsid %u is not a netdevsim\n", ifidx_b, netnsid_b);
> +		goto out_put_netns_b;
> +	}

You're missing if dev_a == dev_b return goto out..; ?
Actually I can't think of a reason why looping would explode.
Could you test it and if it indeed doesn't splat add a comment
here that loops are okay?

> +	err = 0;
> +	nsim_a = netdev_priv(dev_a);
> +	peer = rtnl_dereference(nsim_a->peer);
> +	if (peer) {
> +		pr_err("Netdevsim %u:%u is already linked\n", netnsid_a, ifidx_a);
> +		goto out_put_netns_b;
> +	}
> +
> +	nsim_b = netdev_priv(dev_b);
> +	peer = rtnl_dereference(nsim_b->peer);
> +	if (peer) {
> +		pr_err("Netdevsim %u:%u is already linked\n", netnsid_b, ifidx_b);
> +		goto out_put_netns_b;
> +	}
> +
> +	rcu_assign_pointer(nsim_a->peer, nsim_b);
> +	rcu_assign_pointer(nsim_b->peer, nsim_a);

> @@ -333,6 +333,7 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
>  		goto err_phc_destroy;
>  
>  	rtnl_lock();
> +	RCU_INIT_POINTER(ns->peer, NULL);

since you have to repost - pretty sure ns is zalloc'ed so you don't
have to do this?

>  	err = nsim_bpf_init(ns);
>  	if (err)
>  		goto err_utn_destroy;