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
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.
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!
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.
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
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.
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