netdev - Re: [PATCH v2 net 4/6] mctp: 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: <dcaa0489e90f7c294f6b5e4858b98210766383dc.camel@codeconstruct.com.au>
Date: Mon, 07 Oct 2024 18:09:59 +0800
From: Jeremy Kerr <jk@...econstruct.com.au>
To: Kuniyuki Iwashima <kuniyu@...zon.com>, "David S. Miller"
	 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org, Matt
	Johnston <matt@...econstruct.com.au>
Subject: Re: [PATCH v2 net 4/6] mctp: Handle error of rtnl_register_module().

Hi Kuniyuki,

> Since introduced, mctp has been ignoring the returned value
> of rtnl_register_module(), which could fail.
> 
> Let's handle the errors by rtnl_register_module_many().

Sounds good!

Just a couple of minor things inline, but regardless:

Reviewed-by: Jeremy Kerr <jk@...econstruct.com.au>

> diff --git a/net/mctp/device.c b/net/mctp/device.c
> index acb97b257428..d70e688ac886 100644
> --- a/net/mctp/device.c
> +++ b/net/mctp/device.c
> @@ -524,25 +524,31 @@ static struct notifier_block mctp_dev_nb = {
>         .priority = ADDRCONF_NOTIFY_PRIORITY,
>  };
>  
> -void __init mctp_device_init(void)
> +static struct rtnl_msg_handler mctp_device_rtnl_msg_handlers[] = {
> +       {PF_MCTP, RTM_NEWADDR, mctp_rtm_newaddr, NULL, 0},
> +       {PF_MCTP, RTM_DELADDR, mctp_rtm_deladdr, NULL, 0},
> +       {PF_MCTP, RTM_GETADDR, NULL, mctp_dump_addrinfo, 0},
> +};

Can this (and the other handler arrays) be const? And consequently, the
pointer argument that you pass to rtnl_register_module_many() from 1/6?

>  int __init mctp_routes_init(void)
>  {
> +       int err;
> +
>         dev_add_pack(&mctp_packet_type);
>  
> -       rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_GETROUTE,
> -                            NULL, mctp_dump_rtinfo, 0);
> -       rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_NEWROUTE,
> -                            mctp_newroute, NULL, 0);
> -       rtnl_register_module(THIS_MODULE, PF_MCTP, RTM_DELROUTE,
> -                            mctp_delroute, NULL, 0);
> +       err = register_pernet_subsys(&mctp_net_ops);
> +       if (err)
> +               goto fail_pernet;
> +
> +       err = rtnl_register_module_many(mctp_route_rtnl_msg_handlers);
> +       if (err)
> +               goto fail_rtnl;
>  
> -       return register_pernet_subsys(&mctp_net_ops);
> +out:
> +       return err;
> +
> +fail_rtnl:
> +       unregister_pernet_subsys(&mctp_net_ops);
> +fail_pernet:
> +       dev_remove_pack(&mctp_packet_type);
> +       goto out;
>  }

Just `return err;` here - no need for the backwards goto to the return.

And only if you end up re-rolling the patch: can these labels be err_*,
so that we're consistent with the rest of the file?

Cheers,


Jeremy