Message-ID: <49fbb357-ea22-2647-25d0-a2993cd5ad32@gmail.com>
Date:   Thu, 3 Nov 2022 16:38:00 -0600
From:   David Ahern <dsahern@...il.com>
To:     Ido Schimmel <idosch@...sch.org>, Jakub Kicinski <kuba@...nel.org>
Cc:     Roman Gushchin <roman.gushchin@...ux.dev>,
        Andrew Lunn <andrew@...n.ch>,
        Andy Ren <andy.ren@...cruise.com>, netdev@...r.kernel.org,
        richardbgobert@...il.com, davem@...emloft.net,
        wsa+renesas@...g-engineering.com, edumazet@...gle.com,
        petrm@...dia.com, pabeni@...hat.com, corbet@....net,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        Stephen Hemminger <sthemmin@...rosoft.com>
Subject: Re: [PATCH net-next v2] netconsole: Enable live renaming for network
 interfaces used by netconsole

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

Powered by blists - more mailing lists