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
