Message-ID: <20241007234850.83600-1-kuniyu@amazon.com>
Date: Mon, 7 Oct 2024 16:48:50 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <ebiederm@...ssion.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<kuni1840@...il.com>, <kuniyu@...zon.com>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>
Subject: Re: [PATCH v3 net 5/6] mpls: Handle error of rtnl_register_module().

From: "Eric W. Biederman" <ebiederm@...ssion.com>
Date: Mon, 07 Oct 2024 17:18:59 -0500
> Kuniyuki Iwashima <kuniyu@...zon.com> writes:
> 
> > From: "Eric W. Biederman" <ebiederm@...ssion.com>
> > Date: Mon, 07 Oct 2024 11:28:11 -0500
> >> Kuniyuki Iwashima <kuniyu@...zon.com> writes:
> >> 
> >> > From: "Eric W. Biederman" <ebiederm@...ssion.com>
> >> > Date: Mon, 07 Oct 2024 09:56:44 -0500
> >> >> Kuniyuki Iwashima <kuniyu@...zon.com> writes:
> >> >> 
> >> >> > Since introduced, mpls_init() has been ignoring the returned
> >> >> > value of rtnl_register_module(), which could fail.
> >> >> 
> >> >> As I recall that was deliberate.  The module continues to work if the
> >> >> rtnetlink handlers don't operate, just some functionality is lost.
> >> >
> >> > It's ok if it wasn't a module.  rtnl_register() logs an error message
> >> > in syslog, but rtnl_register_module() doesn't.  That's why this series
> >> > only changes some rtnl_register_module() calls.
> >> 
> >> You talk about the series.  Is there an introductory letter I should
> >> lookup on netdev that explains things in more detail?
> >> 
> >> I have only seen the patch that is sent directly to me.
> >
> > Some context here.
> > https://lore.kernel.org/netdev/20241007124459.5727-1-kuniyu@amazon.com/
> >
> > Before addf9b90de22, rtnl_register_module() didn't actually need
> > error handling for some callers, but even after the commit, some
> > modules copy-and-pasted the wrong code.
> 
> That is wrong. As far back as commit e284986385b6 ("[RTNL]: Message
> handler registration interface") it was possible for rtnl_register to
> error.  Of course that dates back to the time when small allocations
> were guaranteed to succeed or panic the kernel.  So I expect it was
> the change to small allocations that actually made it possible to
> see failures from rtnl_register.

I mean as of addf9b90de22~1, once rtnl_msg_handlers[protocol] was
allocated by rtnl_register() or rtnl_register_module(), the following
calls for the same protocol never failed.

For example, after rtnl_register() was called for PF_UNSPEC/PF_BRIDGE
from the core code, rtnl_register_module() for the protocols didn't
fail, thus some callers didn't need error handling.

Another example is PHONET, where only the first call of
rtnl_register_module() handled the error, which was correct before
addf9b90de22 but not after that commit.


> 
> That said there is a difference between code that generates an error,
> and callers that need to handle the error.  Even today I do not
> see that MPLS needs to handle the error.
> 
> Other than for policy objectives such as making it harder for
> syzkaller to get confused, and making the code more like other
> callers I don't see a need for handling such an error in MPLS.
> 
> >> > What if the memory pressure happend to be relaxed soon after the module
> >> > was loaded incompletely ?
> >> 
> >> Huh?  The module will load completely.  It will just won't have full
> >> functionality.  The rtnetlink functionality simply won't work.
> >> 
> >> > Silent failure is much worse to me.
> >> 
> >> My point is from the point of view of the MPLS functionality it isn't
> >> a __failure__.
> >
> > My point is it _is_ failure for those who use MPLS rtnetlink
> > functionality, it's uAPI.  Not everyone uses the plain MPLS
> > without rtnetlink.
> 
> No matter what the code does userspace attempting to use rtnetlink to
> configure mpls will fail.  Either the MPLS module will fail to load, and
> nothing works, or the module will load and do the best it can with what
> it has.  Allowing the folks you don't need the rtnetlink uAPI to
> succeed.
> 
> It isn't a uAPI failure.  It is a lack of uAPI availability.  There
> is nothing else it can be.

The former is much easier to understand what happened and what's
the next action, just retrying.  The latter lets lsmod show the
module is actually loaded, but there's no hint indicating the module
lacks the rtnetlink availability at the load time.

It's even worse if only a part of rtnetlink handlers is loaded, like
RTM_NEWROUTE works but RTM_GET/DELROUTE returns -ENOTSUPP, it's really
confusing.


> 
> In general the kernel tries to limp along the best it can without
> completely failing.
> 
> > Also, I don't want to waste resource due to such an issue on QEMU w/ 2GB
> > RAM where I'm running syzkaller and often see OOM.  syzkaller searches
> > and loads modules and exercises various uAPIs randomly, and it handles
> > module-load-failure properly.
> 
> Then please mention that use case in your change description.  That is
> the only real motivation I see to for this change in behavior. Perhaps
> something like:
> 
>     Handler errors from rtnetlink registration allowing syzkaller to view a
>     module as an all or nothing thing in terms of the functionality it
>     provides.  This prevents syzkaller from reporting spurious errors
>     from it's tests.
> 
> That seems like a fine motivation and a fine reason.  But if you don't
> say that, people will make changes that don't honor that, and people
> won't be able to look into the history and figure out why such a change
> was made.

I see, I'll add that sentences in v4.

pw-bot: cr

Powered by blists - more mailing lists