[PATCH net-next v2] netconsole: Enable live renaming for network interfaces used by netconsole

[PATCH net-next v2] netconsole: Enable live renaming for network interfaces used by netconsole

[Andy Ren]

This patch enables support for live renaming of network interfaces
initialized by netconsole.

This resolves an issue seen when netconsole is configured to boot as a
built-in kernel module with a kernel boot argument. As stated in the
kernel man page - As a built-in, netconsole initializes immediately
after NIC cards and will bring up the specified interface as soon as
possible. Consequently, the renaming of specified interfaces will fail
and return EBUSY. This is because by default, the kernel disallows live
renaming unless the device explicitly sets a priv_flags bit
(e.g: IFF_LIVE_RENAME_OK or IFF_LIVE_ADDR_CHANGE), and so renaming after
a network interface is up returns EBUSY.

The changes to the kernel are as of following:

- Addition of a iface_live_renaming boolean flag to the netpoll struct,
used to enable/disable interface live renaming. False by default
- Changes to check for the aforementioned flag in network and ethernet
driver interface renaming code
- Adds a new optional "*" parameter to the netconsole configuration
string that enables interface live renaming when included
(e.g. netconsole=+*....). When this optional parameter is included,
"iface_live_renaming" is set to true

Signed-off-by: Andy Ren <andy.ren@getcruise.com>
---
 Documentation/networking/netconsole.rst |  7 ++++---
 drivers/net/netconsole.c                |  5 +++++
 include/linux/netpoll.h                 |  3 +++
 net/core/dev.c                          |  3 ++-
 net/core/netpoll.c                      | 15 +++++++++++++++
 net/ethernet/eth.c                      |  5 ++++-
 6 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst
index 1f5c4a04027c..01a45f38ce3f 100644
--- a/Documentation/networking/netconsole.rst
+++ b/Documentation/networking/netconsole.rst
@@ -34,10 +34,11 @@ Sender and receiver configuration:
 It takes a string configuration parameter "netconsole" in the
 following format::
 
- netconsole=[+][src-port]@[src-ip]/[<dev>],[tgt-port]@<tgt-ip>/[tgt-macaddr]
+ netconsole=[+][*][src-port]@[src-ip]/[<dev>],[tgt-port]@<tgt-ip>/[tgt-macaddr]
 
    where
 	+             if present, enable extended console support
+	*             if present, allow runtime network interface renaming
 	src-port      source for UDP packets (defaults to 6665)
 	src-ip        source IP to use (interface address)
 	dev           network interface (eth0)
@@ -47,7 +48,7 @@ following format::
 
 Examples::
 
- linux netconsole=4444@10.0.0.1/eth1,9353@10.0.0.2/12:34:56:78:9a:bc
+ linux netconsole=*4444@10.0.0.1/eth1,9353@10.0.0.2/12:34:56:78:9a:bc
 
 or::
 
@@ -158,7 +159,7 @@ If '+' is prefixed to the configuration line or "extended" config file
 is set to 1, extended console support is enabled. An example boot
 param follows::
 
- linux netconsole=+4444@10.0.0.1/eth1,9353@10.0.0.2/12:34:56:78:9a:bc
+ linux netconsole=+*4444@10.0.0.1/eth1,9353@10.0.0.2/12:34:56:78:9a:bc
 
 Log messages are transmitted with extended metadata header in the
 following format which is the same as /dev/kmsg::
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index bdff9ac5056d..dea5b783744f 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -188,6 +188,11 @@ static struct netconsole_target *alloc_param_target(char *target_config)
 		target_config++;
 	}
 
+	if (*target_config == '*') {
+		nt->np.iface_live_renaming = true;
+		target_config++;
+	}
+
 	/* Parse parameters and setup netpoll */
 	err = netpoll_parse_options(&nt->np, target_config);
 	if (err)
diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index bd19c4b91e31..f2ebdabf0959 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -32,6 +32,7 @@ struct netpoll {
 	bool ipv6;
 	u16 local_port, remote_port;
 	u8 remote_mac[ETH_ALEN];
+	bool iface_live_renaming;
 };
 
 struct netpoll_info {
@@ -51,9 +52,11 @@ struct netpoll_info {
 void netpoll_poll_dev(struct net_device *dev);
 void netpoll_poll_disable(struct net_device *dev);
 void netpoll_poll_enable(struct net_device *dev);
+bool netpoll_live_renaming_enabled(struct net_device *dev);
 #else
 static inline void netpoll_poll_disable(struct net_device *dev) { return; }
 static inline void netpoll_poll_enable(struct net_device *dev) { return; }
+static inline bool netpoll_live_renaming_enabled(struct net_device *dev) { return false; }
 #endif
 
 void netpoll_send_udp(struct netpoll *np, const char *msg, int len);
diff --git a/net/core/dev.c b/net/core/dev.c
index 2e4f1c97b59e..90e6870d38d0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1176,7 +1176,8 @@ int dev_change_name(struct net_device *dev, const char *newname)
 	 * directly.
 	 */
 	if (dev->flags & IFF_UP &&
-	    likely(!(dev->priv_flags & IFF_LIVE_RENAME_OK)))
+	    likely(!(dev->priv_flags & IFF_LIVE_RENAME_OK) &&
+		   !netpoll_live_renaming_enabled(dev)))
 		return -EBUSY;
 
 	down_write(&devnet_rename_sem);
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 9be762e1d042..a22319676667 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -224,6 +224,21 @@ void netpoll_poll_enable(struct net_device *dev)
 }
 EXPORT_SYMBOL(netpoll_poll_enable);
 
+bool netpoll_live_renaming_enabled(struct net_device *dev)
+{
+	struct netpoll_info *ni;
+	bool live_renaming_enabled = false;
+
+	rcu_read_lock();
+	ni = rcu_dereference(dev->npinfo);
+	if (ni && ni->netpoll->iface_live_renaming)
+		live_renaming_enabled = true;
+	rcu_read_unlock();
+
+	return live_renaming_enabled;
+}
+EXPORT_SYMBOL(netpoll_live_renaming_enabled);
+
 static void refill_skbs(void)
 {
 	struct sk_buff *skb;
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index e02daa74e833..bb341acfcf05 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -62,6 +62,7 @@
 #include <net/gro.h>
 #include <linux/uaccess.h>
 #include <net/pkt_sched.h>
+#include <linux/netpoll.h>
 
 /**
  * eth_header - create the Ethernet header
@@ -288,8 +289,10 @@ int eth_prepare_mac_addr_change(struct net_device *dev, void *p)
 {
 	struct sockaddr *addr = p;
 
-	if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev))
+	if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev) &&
+	    !netpoll_live_renaming_enabled(dev))
 		return -EBUSY;
+
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 	return 0;
-- 
2.38.1

Re: [PATCH net-next v2] netconsole: Enable live renaming for network interfaces used by netconsole

[Andrew Lunn]

On Tue, Nov 01, 2022 at 05:24:20PM -0700, Andy Ren wrote:
> This patch enables support for live renaming of network interfaces
> initialized by netconsole.
> 
> This resolves an issue seen when netconsole is configured to boot as a
> built-in kernel module with a kernel boot argument. As stated in the
> kernel man page - As a built-in, netconsole initializes immediately
> after NIC cards and will bring up the specified interface as soon as
> possible. Consequently, the renaming of specified interfaces will fail
> and return EBUSY. This is because by default, the kernel disallows live
> renaming unless the device explicitly sets a priv_flags bit
> (e.g: IFF_LIVE_RENAME_OK or IFF_LIVE_ADDR_CHANGE), and so renaming after
> a network interface is up returns EBUSY.
> 
> The changes to the kernel are as of following:
> 
> - Addition of a iface_live_renaming boolean flag to the netpoll struct,
> used to enable/disable interface live renaming. False by default
> - Changes to check for the aforementioned flag in network and ethernet
> driver interface renaming code
> - Adds a new optional "*" parameter to the netconsole configuration
> string that enables interface live renaming when included
> (e.g. netconsole=+*....). When this optional parameter is included,
> "iface_live_renaming" is set to true


>  /**
>   * eth_header - create the Ethernet header
> @@ -288,8 +289,10 @@ int eth_prepare_mac_addr_change(struct net_device *dev, void *p)
>  {
>  	struct sockaddr *addr = p;
>  
> -	if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev))
> +	if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev) &&
> +	    !netpoll_live_renaming_enabled(dev))
>  		return -EBUSY;
> +
>  	if (!is_valid_ether_addr(addr->sa_data))
>  		return -EADDRNOTAVAIL;
>  	return 0;

There is no mention of this in the commit message.

Changing the interface name while running is probably not an
issue. There are a few drivers which report the name to the firmware,
presumably for logging, and phoning home, but it should not otherwise
affect the hardware.

However, changing the MAC address does need changes to the hardware
configuration, and not all can do that while the interface is running.
So i think this last part needs some justification.

   Andrew

Re: [PATCH net-next v2] netconsole: Enable live renaming for network interfaces used by netconsole

[Jakub Kicinski]

On Wed, 2 Nov 2022 01:48:09 +0100 Andrew Lunn wrote:
> Changing the interface name while running is probably not an
> issue. There are a few drivers which report the name to the firmware,
> presumably for logging, and phoning home, but it should not otherwise
> affect the hardware.

Agreed. BTW I wonder if we really want to introduce a netconsole
specific uAPI for this or go ahead with something more general.
A sysctl for global "allow UP rename"?

We added the live renaming for failover a while back and there were 
no reports of user space breaking as far as I know. So perhaps nobody
actually cares and we should allow renaming all interfaces while UP?
For backwards compat we can add a sysctl as mentioned or a rtnetlink 
"I know what I'm doing" flag? 

Maybe print an info message into the logs for a few releases to aid
debug?

IOW either there is a reason we don't allow rename while up, and
netconsole being bound to an interface is immaterial. Or there is 
no reason and we should allow all.

Re: [PATCH net-next v2] netconsole: Enable live renaming for network interfaces used by netconsole

[Roman Gushchin]

On Tue, Nov 01, 2022 at 08:40:06PM -0700, Jakub Kicinski wrote:
> On Wed, 2 Nov 2022 01:48:09 +0100 Andrew Lunn wrote:
> > Changing the interface name while running is probably not an
> > issue. There are a few drivers which report the name to the firmware,
> > presumably for logging, and phoning home, but it should not otherwise
> > affect the hardware.
> 
> Agreed. BTW I wonder if we really want to introduce a netconsole
> specific uAPI for this or go ahead with something more general.

Netconsole is a bit special because it brings an interface up very early.
E.g. in our case without the netconsole the renaming is happening before
the interface is brought up.

I wonder if the netconsole-specific flag should allow renaming only once.

> A sysctl for global "allow UP rename"?

This will work for us, but I've no idea what it will break for other users
and how to check it without actually trying to break :) And likely we won't
learn about it for quite some time, asssuming they don't run net-next.

> 
> We added the live renaming for failover a while back and there were 
> no reports of user space breaking as far as I know. So perhaps nobody
> actually cares and we should allow renaming all interfaces while UP?
> For backwards compat we can add a sysctl as mentioned or a rtnetlink 
> "I know what I'm doing" flag? 
> 
> Maybe print an info message into the logs for a few releases to aid
> debug?
> 
> IOW either there is a reason we don't allow rename while up, and
> netconsole being bound to an interface is immaterial. Or there is 
> no reason and we should allow all.

My understanding is that it's not an issue for the kernel, but might be
an issue for some userspace apps which do not expect it.

If you prefer to go with the 'global sysctl' approach, how the path forward
should look like?

Thanks!

Re: [PATCH net-next v2] netconsole: Enable live renaming for network interfaces used by netconsole

[Jakub Kicinski]

On Wed, 2 Nov 2022 10:14:38 -0700 Roman Gushchin wrote:
> > Agreed. BTW I wonder if we really want to introduce a netconsole
> > specific uAPI for this or go ahead with something more general.  
> 
> Netconsole is a bit special because it brings an interface up very early.
> E.g. in our case without the netconsole the renaming is happening before
> the interface is brought up.
> 
> I wonder if the netconsole-specific flag should allow renaming only once.
>  
> > A sysctl for global "allow UP rename"?  
> 
> This will work for us, but I've no idea what it will break for other users
> and how to check it without actually trying to break :) And likely we won't
> learn about it for quite some time, asssuming they don't run net-next.

Then again IFF_LIVE_RENAME_OK was added in 5.2 so quite a while back.

> > We added the live renaming for failover a while back and there were 
> > no reports of user space breaking as far as I know. So perhaps nobody
> > actually cares and we should allow renaming all interfaces while UP?
> > For backwards compat we can add a sysctl as mentioned or a rtnetlink 
> > "I know what I'm doing" flag? 
> > 
> > Maybe print an info message into the logs for a few releases to aid
> > debug?
> > 
> > IOW either there is a reason we don't allow rename while up, and
> > netconsole being bound to an interface is immaterial. Or there is 
> > no reason and we should allow all.  
> 
> My understanding is that it's not an issue for the kernel, but might be
> an issue for some userspace apps which do not expect it.

There are in-kernel notifier users which could cache the name on up /
down. But yes, the user space is the real worry.

> If you prefer to go with the 'global sysctl' approach, how the path forward
> should look like?

That's the question. The sysctl would really just be to cover our back
sides, and be able to tell the users "you opted in by setting that
sysctl, we didn't break backward compat". But practically speaking, 
its a different entity that'd be flipping the sysctl (e.g. management
daemon) and different entity that'd be suffering (e.g. routing daemon).
So the sysctl doesn't actually help anyone :/

So maybe we should just risk it and wonder about workarounds once
complains surface, if they do. Like generate fake down/up events.
Or create some form of "don't allow live renames now" lock-like
thing a process could take.

Adding a couple more CCs and if nobody screams at us I vote we just
remove the restriction instead of special casing.

Re: [PATCH net-next v2] netconsole: Enable live renaming for network interfaces used by netconsole

[Roman Gushchin]

On Wed, Nov 02, 2022 at 12:54:18PM -0700, Jakub Kicinski wrote:
> On Wed, 2 Nov 2022 10:14:38 -0700 Roman Gushchin wrote:
> > > Agreed. BTW I wonder if we really want to introduce a netconsole
> > > specific uAPI for this or go ahead with something more general.  
> > 
> > Netconsole is a bit special because it brings an interface up very early.
> > E.g. in our case without the netconsole the renaming is happening before
> > the interface is brought up.
> > 
> > I wonder if the netconsole-specific flag should allow renaming only once.
> >  
> > > A sysctl for global "allow UP rename"?  
> > 
> > This will work for us, but I've no idea what it will break for other users
> > and how to check it without actually trying to break :) And likely we won't
> > learn about it for quite some time, asssuming they don't run net-next.
> 
> Then again IFF_LIVE_RENAME_OK was added in 5.2 so quite a while back.
> 
> > > We added the live renaming for failover a while back and there were 
> > > no reports of user space breaking as far as I know. So perhaps nobody
> > > actually cares and we should allow renaming all interfaces while UP?
> > > For backwards compat we can add a sysctl as mentioned or a rtnetlink 
> > > "I know what I'm doing" flag? 
> > > 
> > > Maybe print an info message into the logs for a few releases to aid
> > > debug?
> > > 
> > > IOW either there is a reason we don't allow rename while up, and
> > > netconsole being bound to an interface is immaterial. Or there is 
> > > no reason and we should allow all.  
> > 
> > My understanding is that it's not an issue for the kernel, but might be
> > an issue for some userspace apps which do not expect it.
> 
> There are in-kernel notifier users which could cache the name on up /
> down. But yes, the user space is the real worry.
> 
> > If you prefer to go with the 'global sysctl' approach, how the path forward
> > should look like?
> 
> That's the question. The sysctl would really just be to cover our back
> sides, and be able to tell the users "you opted in by setting that
> sysctl, we didn't break backward compat". But practically speaking, 
> its a different entity that'd be flipping the sysctl (e.g. management
> daemon) and different entity that'd be suffering (e.g. routing daemon).
> So the sysctl doesn't actually help anyone :/

Yeah, I agree, adding another sysctl for this looks like an overkill.

> 
> So maybe we should just risk it and wonder about workarounds once
> complains surface, if they do. Like generate fake down/up events.
> Or create some form of "don't allow live renames now" lock-like
> thing a process could take.
> 
> Adding a couple more CCs and if nobody screams at us I vote we just
> remove the restriction instead of special casing.

Great, thanks!

Let's do this and if there will be serious concernes raised, let's
fallback to the netconsole-specific thing (maybe with the "allow
single renaming" semantics).

Thanks,
Roman

Re: [PATCH net-next v2] netconsole: Enable live renaming for network interfaces used by netconsole

[Ido Schimmel]

On Wed, Nov 02, 2022 at 12:54:18PM -0700, Jakub Kicinski wrote:
> On Wed, 2 Nov 2022 10:14:38 -0700 Roman Gushchin wrote:
> > > Agreed. BTW I wonder if we really want to introduce a netconsole
> > > specific uAPI for this or go ahead with something more general.  
> > 
> > Netconsole is a bit special because it brings an interface up very early.
> > E.g. in our case without the netconsole the renaming is happening before
> > the interface is brought up.
> > 
> > I wonder if the netconsole-specific flag should allow renaming only once.
> >  
> > > A sysctl for global "allow UP rename"?  
> > 
> > This will work for us, but I've no idea what it will break for other users
> > and how to check it without actually trying to break :) And likely we won't
> > learn about it for quite some time, asssuming they don't run net-next.
> 
> Then again IFF_LIVE_RENAME_OK was added in 5.2 so quite a while back.
> 
> > > We added the live renaming for failover a while back and there were 
> > > no reports of user space breaking as far as I know. So perhaps nobody
> > > actually cares and we should allow renaming all interfaces while UP?
> > > For backwards compat we can add a sysctl as mentioned or a rtnetlink 
> > > "I know what I'm doing" flag? 
> > > 
> > > Maybe print an info message into the logs for a few releases to aid
> > > debug?
> > > 
> > > IOW either there is a reason we don't allow rename while up, and
> > > netconsole being bound to an interface is immaterial. Or there is 
> > > no reason and we should allow all.  
> > 
> > My understanding is that it's not an issue for the kernel, but might be
> > an issue for some userspace apps which do not expect it.
> 
> There are in-kernel notifier users which could cache the name on up /
> down. But yes, the user space is the real worry.
> 
> > If you prefer to go with the 'global sysctl' approach, how the path forward
> > should look like?
> 
> That's the question. The sysctl would really just be to cover our back
> sides, and be able to tell the users "you opted in by setting that
> sysctl, we didn't break backward compat". But practically speaking, 
> its a different entity that'd be flipping the sysctl (e.g. management
> daemon) and different entity that'd be suffering (e.g. routing daemon).
> So the sysctl doesn't actually help anyone :/
> 
> So maybe we should just risk it and wonder about workarounds once
> complains surface, if they do. Like generate fake down/up events.
> Or create some form of "don't allow live renames now" lock-like
> thing a process could take.
> 
> Adding a couple more CCs and if nobody screams at us I vote we just
> remove the restriction instead of special casing.

Tried looking at history.git to understand the reasoning behind this
restriction. I guess it's because back then it was only possible via
IOCTL and user space wouldn't be notified about such a change. Nowadays
user space gets a notification regardless of the administrative state of
the netdev (see rtnetlink_event()). At least in-kernel listeners to
NETDEV_CHANGENAME do not seem to care if the netdev is administratively
up or not. So, FWIW, the suggested approach sounds sane to me.

Re: [PATCH net-next v2] netconsole: Enable live renaming for network interfaces used by netconsole

[David Ahern]

On 11/3/22 11:52 AM, Ido Schimmel wrote:
> On Wed, Nov 02, 2022 at 12:54:18PM -0700, Jakub Kicinski wrote:
>> On Wed, 2 Nov 2022 10:14:38 -0700 Roman Gushchin wrote:
>>>> Agreed. BTW I wonder if we really want to introduce a netconsole
>>>> specific uAPI for this or go ahead with something more general.  
>>>
>>> Netconsole is a bit special because it brings an interface up very early.
>>> E.g. in our case without the netconsole the renaming is happening before
>>> the interface is brought up.
>>>
>>> I wonder if the netconsole-specific flag should allow renaming only once.
>>>  
>>>> A sysctl for global "allow UP rename"?  
>>>
>>> This will work for us, but I've no idea what it will break for other users
>>> and how to check it without actually trying to break :) And likely we won't
>>> learn about it for quite some time, asssuming they don't run net-next.
>>
>> Then again IFF_LIVE_RENAME_OK was added in 5.2 so quite a while back.
>>
>>>> We added the live renaming for failover a while back and there were 
>>>> no reports of user space breaking as far as I know. So perhaps nobody
>>>> actually cares and we should allow renaming all interfaces while UP?
>>>> For backwards compat we can add a sysctl as mentioned or a rtnetlink 
>>>> "I know what I'm doing" flag? 
>>>>
>>>> Maybe print an info message into the logs for a few releases to aid
>>>> debug?
>>>>
>>>> IOW either there is a reason we don't allow rename while up, and
>>>> netconsole being bound to an interface is immaterial. Or there is 
>>>> no reason and we should allow all.  
>>>
>>> My understanding is that it's not an issue for the kernel, but might be
>>> an issue for some userspace apps which do not expect it.
>>
>> There are in-kernel notifier users which could cache the name on up /
>> down. But yes, the user space is the real worry.
>>
>>> If you prefer to go with the 'global sysctl' approach, how the path forward
>>> should look like?
>>
>> That's the question. The sysctl would really just be to cover our back
>> sides, and be able to tell the users "you opted in by setting that
>> sysctl, we didn't break backward compat". But practically speaking, 
>> its a different entity that'd be flipping the sysctl (e.g. management
>> daemon) and different entity that'd be suffering (e.g. routing daemon).
>> So the sysctl doesn't actually help anyone :/
>>
>> So maybe we should just risk it and wonder about workarounds once
>> complains surface, if they do. Like generate fake down/up events.
>> Or create some form of "don't allow live renames now" lock-like
>> thing a process could take.
>>
>> Adding a couple more CCs and if nobody screams at us I vote we just
>> remove the restriction instead of special casing.
> 
> Tried looking at history.git to understand the reasoning behind this
> restriction. I guess it's because back then it was only possible via
> IOCTL and user space wouldn't be notified about such a change. Nowadays
> user space gets a notification regardless of the administrative state of
> the netdev (see rtnetlink_event()). At least in-kernel listeners to
> NETDEV_CHANGENAME do not seem to care if the netdev is administratively
> up or not. So, FWIW, the suggested approach sounds sane to me.

+1