netdev - Re: [PATCH net] net: Have netpoll bring-up DSA management interface

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: <58b07285-bb70-3115-eb03-5e43a4abeae6@gmail.com>
Date:   Mon, 19 Oct 2020 14:03:40 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     netdev@...r.kernel.org, kuba@...nel.org,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Yunjian Wang <wangyunjian@...wei.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] net: Have netpoll bring-up DSA management interface

On 10/19/20 1:02 PM, Vladimir Oltean wrote:
> On Mon, Oct 19, 2020 at 10:17:44AM -0700, Florian Fainelli wrote:
>> These devices also do not utilize the upper/lower linking so the
>> check about the netpoll device having upper is not going to be a
>> problem.
> 
> They do as of 2f1e8ea726e9 ("net: dsa: link interfaces with the DSA
> master to get rid of lockdep warnings"), don't they? The question is why
> that doesn't work, and the answer is, I believe, that the linkage needs
> to be the other way around than DSA has it.



> 
>>
>> The solution adopted here is identical to the one done for
>> net/ipv4/ipconfig.c with 728c02089a0e ("net: ipv4: handle DSA enabled
>> master network devices"), with the network namespace scope being
>> restricted to that of the process configuring netpoll.
> 
> ... and further restricted to the only network namespace that DSA
> supports. As a side note, we should declare NETIF_F_NETNS_LOCAL_BIT for
> DSA interfaces.
> 
>>
>> Fixes: 04ff53f96a93 ("net: dsa: Add netconsole support")
>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>> ---
>>  net/core/netpoll.c | 22 ++++++++++++++++++----
>>  1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
>> index c310c7c1cef7..960948290001 100644
>> --- a/net/core/netpoll.c
>> +++ b/net/core/netpoll.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/export.h>
>>  #include <linux/if_vlan.h>
>> +#include <net/dsa.h>
>>  #include <net/tcp.h>
>>  #include <net/udp.h>
>>  #include <net/addrconf.h>
>> @@ -657,15 +658,15 @@ EXPORT_SYMBOL_GPL(__netpoll_setup);
>>  
>>  int netpoll_setup(struct netpoll *np)
>>  {
>> -	struct net_device *ndev = NULL;
>> +	struct net_device *ndev = NULL, *dev = NULL;
>> +	struct net *net = current->nsproxy->net_ns;
>>  	struct in_device *in_dev;
>>  	int err;
>>  
>>  	rtnl_lock();
>> -	if (np->dev_name[0]) {
>> -		struct net *net = current->nsproxy->net_ns;
>> +	if (np->dev_name[0])
>>  		ndev = __dev_get_by_name(net, np->dev_name);
>> -	}
>> +
>>  	if (!ndev) {
>>  		np_err(np, "%s doesn't exist, aborting\n", np->dev_name);
>>  		err = -ENODEV;
>> @@ -673,6 +674,19 @@ int netpoll_setup(struct netpoll *np)
>>  	}
>>  	dev_hold(ndev);
>>  
>> +	/* bring up DSA management network devices up first */
>> +	for_each_netdev(net, dev) {
>> +		if (!netdev_uses_dsa(dev))
>> +			continue;
>> +
>> +		err = dev_change_flags(dev, dev->flags | IFF_UP, NULL);
>> +		if (err < 0) {
>> +			np_err(np, "%s failed to open %s\n",
>> +			       np->dev_name, dev->name);
>> +			goto put;
>> +		}
>> +	}
>> +
> 
> Completely crazy and outlandish idea, I know, but what's wrong with
> doing this in DSA?

I really do not have a problem with that approach however other stacked
devices like 802.1Q do not do that. It certainly scales a lot better to
do this within DSA rather than sprinkling DSA specific knowledge
throughout the network stack. Maybe for "configuration less" stacked
devices such as DSA, 802.1Q (bridge ports?), bond etc. it would be
acceptable to ensure that the lower device is always brought up?

PS: if you quote below the git version, it would appear that Thunderbird
just eats that part of the mail... another bug to submit there.
-- 
Florian