netdev - Re: [PATCH v3 net 5/6] mpls: Handle error of rtnl_register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87h69ohsgj.fsf@email.froward.int.ebiederm.org>
Date: Mon, 07 Oct 2024 09:56:44 -0500
From: "Eric W. Biederman" <ebiederm@...ssion.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: "David S. Miller" <davem@...emloft.net>,  Eric Dumazet
 <edumazet@...gle.com>,  Jakub Kicinski <kuba@...nel.org>,  Paolo Abeni
 <pabeni@...hat.com>,  Kuniyuki Iwashima <kuni1840@...il.com>,
  <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 net 5/6] mpls: Handle error of rtnl_register_module().

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.

I don't strongly care either way, but I want to point out that bailing
out due to a memory allocation failure actually makes the module
initialization more brittle.

> Let's handle the errors by rtnl_register_many().

Can you describe what the benefit is from completely giving up in the
face of a memory allocation failure versus having as much of the module
function as possible?

> Fixes: 03c0566542f4 ("mpls: Netlink commands to add, remove, and dump routes")

A fix?  Not really no.  You are making the code work less well in the
face of adversity.  I really don't think that is a fix.  Certainly not
without a better justification.

I get this as a code cleanup to make things more uniform and easier
to reason about.  Still you are adding more code paths that are going
to go untested so I don't see how this winds up being any better
in practice that deliberately limping along.

Eric

> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> ---
> Cc: "Eric W. Biederman" <ebiederm@...ssion.com>
> ---
>  net/mpls/af_mpls.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> index aba983531ed3..df62638b6498 100644
> --- a/net/mpls/af_mpls.c
> +++ b/net/mpls/af_mpls.c
> @@ -2728,6 +2728,15 @@ static struct rtnl_af_ops mpls_af_ops __read_mostly = {
>  	.get_stats_af_size = mpls_get_stats_af_size,
>  };
>  
> +static const struct rtnl_msg_handler mpls_rtnl_msg_handlers[] __initdata_or_module = {
> +	{THIS_MODULE, PF_MPLS, RTM_NEWROUTE, mpls_rtm_newroute, NULL, 0},
> +	{THIS_MODULE, PF_MPLS, RTM_DELROUTE, mpls_rtm_delroute, NULL, 0},
> +	{THIS_MODULE, PF_MPLS, RTM_GETROUTE, mpls_getroute, mpls_dump_routes, 0},
> +	{THIS_MODULE, PF_MPLS, RTM_GETNETCONF,
> +	 mpls_netconf_get_devconf, mpls_netconf_dump_devconf,
> +	 RTNL_FLAG_DUMP_UNLOCKED},
> +};
> +
>  static int __init mpls_init(void)
>  {
>  	int err;
> @@ -2746,24 +2755,25 @@ static int __init mpls_init(void)
>  
>  	rtnl_af_register(&mpls_af_ops);
>  
> -	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_NEWROUTE,
> -			     mpls_rtm_newroute, NULL, 0);
> -	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_DELROUTE,
> -			     mpls_rtm_delroute, NULL, 0);
> -	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_GETROUTE,
> -			     mpls_getroute, mpls_dump_routes, 0);
> -	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_GETNETCONF,
> -			     mpls_netconf_get_devconf,
> -			     mpls_netconf_dump_devconf,
> -			     RTNL_FLAG_DUMP_UNLOCKED);
> -	err = ipgre_tunnel_encap_add_mpls_ops();
> +	err = rtnl_register_many(mpls_rtnl_msg_handlers);
>  	if (err)
> +		goto out_unregister_rtnl_af;
> +
> +	err = ipgre_tunnel_encap_add_mpls_ops();
> +	if (err) {
>  		pr_err("Can't add mpls over gre tunnel ops\n");
> +		goto out_unregister_rtnl;
> +	}
>  
>  	err = 0;
>  out:
>  	return err;
>  
> +out_unregister_rtnl:
> +	rtnl_unregister_many(mpls_rtnl_msg_handlers);
> +out_unregister_rtnl_af:
> +	rtnl_af_unregister(&mpls_af_ops);
> +	dev_remove_pack(&mpls_packet_type);
>  out_unregister_pernet:
>  	unregister_pernet_subsys(&mpls_net_ops);
>  	goto out;