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] [day] [month] [year] [list]

Message-ID: <20241007122824.4398-1-kuniyu@amazon.com>
Date: Mon, 7 Oct 2024 05:28:24 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <jk@...econstruct.com.au>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<kuni1840@...il.com>, <kuniyu@...zon.com>, <matt@...econstruct.com.au>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [PATCH v2 net 4/6] mctp: Handle error of rtnl_register_module().

From: Jeremy Kerr <jk@...econstruct.com.au>
Date: Mon, 07 Oct 2024 18:09:59 +0800
> 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?

Nice, will do.


> 
> >  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?

Sure, will change the labels.

Thanks!