Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i494840e7:Fastmail
Date:   Thu, 3 Nov 2022 19:52:31 +0200
From:   Ido Schimmel <idosch@idosch.org>
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     Roman Gushchin <roman.gushchin@linux.dev>,
        Andrew Lunn <andrew@lunn.ch>,
        Andy Ren <andy.ren@getcruise.com>, netdev@vger.kernel.org,
        richardbgobert@gmail.com, davem@davemloft.net,
        wsa+renesas@sang-engineering.com, edumazet@google.com,
        petrm@nvidia.com, pabeni@redhat.com, corbet@lwn.net,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        David Ahern <dsahern@gmail.com>,
        Stephen Hemminger <sthemmin@microsoft.com>
Subject: Re: [PATCH net-next v2] netconsole: Enable live renaming for network
 interfaces used by netconsole
Message-ID: <Y2P/33wfWmQ/xC3n@shredder>
References: <20221102002420.2613004-1-andy.ren@getcruise.com>
 <Y2G+SYXyZAB/r3X0@lunn.ch>
 <20221101204006.75b46660@kernel.org>
 <Y2KlfhfijyNl8yxT@P9FQF9L96D.corp.robot.car>
 <20221102125418.272c4381@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221102125418.272c4381@kernel.org>
Precedence: bulk

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.