netdev - Re: [PATCHi next] veth: advertise peer link relationship for both devices

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: <20160611.154340.1340829899277605940.davem@davemloft.net>
Date:	Sat, 11 Jun 2016 15:43:40 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	lrichard@...hat.com
Cc:	netdev@...r.kernel.org, nicolas.dichtel@...nd.com
Subject: Re: [PATCHi next] veth: advertise peer link relationship for both
 devices

From: Lance Richardson <lrichard@...hat.com>
Date: Fri, 10 Jun 2016 12:32:19 -0400

> Currently, when creating a veth pair, notfications to user
> space only include link peer for one end of the veth pair:
>    # ip monitor link &
>    # ip link add dev vm1 type veth peer name vm2
>    30: vm2@...E: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
>        link/ether be:e3:b7:0e:14:52 brd ff:ff:ff:ff:ff:ff
>    31: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
>        link/ether da:e6:a6:c5:42:54 brd ff:ff:ff:ff:ff:ff
> 
> With this change, netlink notifications are sent with complete
> information for both interfaces of the veth pair:
> 
>    # 3: vm2@...E: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
>        link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff
>    4: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
>        link/ether b2:05:70:e0:fc:35 brd ff:ff:ff:ff:ff:ff
>    3: vm2@vm1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
>        link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff
> 
> Signed-off-by: Lance Richardson <lrichard@...hat.com>

I don't know about this.

First of all, those notifications you get above tell you everything you
need to know in order to figure out what both ends of the veth pair are.

In fact, I would say that the vm1@vm2 notification #31 above is the _only_
one you absolutely need.

> @@ -466,8 +466,16 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>  
>  	priv = netdev_priv(peer);
>  	rcu_assign_pointer(priv->peer, dev);
> +
> +	err = rtnl_configure_link(dev, NULL);
> +	if (err < 0)
> +		goto err_configure_dev;
> +
> +	rtmsg_ifinfo(RTM_NEWLINK, peer, 0, GFP_KERNEL);
>  	return 0;
>  
> +err_configure_dev:
> +	/* nothing to do */
>  err_register_dev:
>  	/* nothing to do */
>  err_configure_peer:

If you're registering the peer here explicitly, this means a link configure
somewhere else is now superfluous.

I really don't like this change at all, both from a necessity perspective as
well as from it's implementation.