netdev - Re: [PATCH net] selftests: bonding: do not set port down before adding to bond

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: <ZN34sjMg+SkG4Yz+@orbyte.nwl.cc>
Date: Thu, 17 Aug 2023 12:38:42 +0200
From: Phil Sutter <phil@....cc>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, Jay Vosburgh <j.vosburgh@...il.com>,
	"David S . Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Eric Dumazet <edumazet@...gle.com>, Liang Li <liali@...hat.com>,
	Jiri Pirko <jiri@...dia.com>,
	Nikolay Aleksandrov <razor@...ckwall.org>,
	Zhengchao Shao <shaozhengchao@...wei.com>
Subject: Re: [PATCH net] selftests: bonding: do not set port down before
 adding to bond

On Thu, Aug 17, 2023 at 04:24:59PM +0800, Hangbin Liu wrote:
> Before adding a port to bond, it need to be set down first. In the
> lacpdu test the author set the port down specifically. But commit
> a4abfa627c38 ("net: rtnetlink: Enslave device before bringing it up")
> changed the operation order, the kernel will set the port down _after_
> adding to bond. So all the ports will be down at last and the test failed.
> 
> In fact, the veth interfaces are already inactive when added. This
> means there's no need to set them down again before adding to the bond.
> Let's just remove the link down operation.
> 
> Reported-by: Zhengchao Shao <shaozhengchao@...wei.com>
> Closes: https://lore.kernel.org/netdev/a0ef07c7-91b0-94bd-240d-944a330fcabd@huawei.com/
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
> PS: I'm not sure if this should be a regression of a4abfa627c38.

Well, in theory it might be as ip-link's behaviour changed in this
detail. Yet:

> -ip link set veth1-bond down master fbond

Without prior knowledge of kernel interna[1], one would expect this
command to result in veth1-bond being enslaved and down, irrespective of
in which order the link changes happen.

The command my patch enables, namely:

| ip link set veth1-bond up master fbond

is actually intuitive. OK, it won't work if veth1-bond is up already.
But I guess that's rather a missing feature (bridge driver supports it
for instance).

Cheers, Phil

[1] - link-state change happens before master assignment
    - bond driver "ups" newly attached ports