| Message ID | 20180109182116.olzb5rroeazq4ufv@xylophone.i.decadent.org.uk |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] ipv6: Fix cleanup ordering on inet6_init() error path | expand |
On Tue, Jan 9, 2018 at 10:21 AM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > Commit 15e668070a64 reordered the initialisation in inet6_init() to > fix a crash on an error path further down the call stack. It also > reordered cleanup on the error path in inet6_init(), but the result > is not the reverse of the initialisation order. This presumably > can result in a resource leak or crash in some error cases. Reorder > cleanup again to fix this. Can you be specific on what resource we leak here? Also, it looks like you not just revert the order changed in commit 15e668070a64, but also you move icmpv6_cleanup() even earlier.
On Wed, 2018-01-10 at 14:25 -0800, Cong Wang wrote: > On Tue, Jan 9, 2018 at 10:21 AM, Ben Hutchings > <ben.hutchings@codethink.co.uk> wrote: > > Commit 15e668070a64 reordered the initialisation in inet6_init() to > > fix a crash on an error path further down the call stack. It also > > reordered cleanup on the error path in inet6_init(), but the result > > is not the reverse of the initialisation order. This presumably > > can result in a resource leak or crash in some error > > cases. Reorder > > cleanup again to fix this. > > Can you be specific on what resource we leak here? If icmpv6_init() fails, after ip6_mr_init(), then ip6_mr_cleanup() is not called. Also, if ip6_mr_init() fails, we don't unregister inet6_net_ops. I think that will result in a crash - immediately if ipv6 is a module, otherwise when the next net namespace is created. > Also, it looks like you not just revert the order changed in commit > 15e668070a64, but also you move icmpv6_cleanup() even earlier. So should I add another Fixes: there? Ben.
On Thu, Jan 11, 2018 at 8:48 AM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > On Wed, 2018-01-10 at 14:25 -0800, Cong Wang wrote: >> On Tue, Jan 9, 2018 at 10:21 AM, Ben Hutchings >> <ben.hutchings@codethink.co.uk> wrote: >> > Commit 15e668070a64 reordered the initialisation in inet6_init() to >> > fix a crash on an error path further down the call stack. It also >> > reordered cleanup on the error path in inet6_init(), but the result >> > is not the reverse of the initialisation order. This presumably >> > can result in a resource leak or crash in some error >> > cases. Reorder >> > cleanup again to fix this. >> >> Can you be specific on what resource we leak here? > > If icmpv6_init() fails, after ip6_mr_init(), then ip6_mr_cleanup() is > not called. > > Also, if ip6_mr_init() fails, we don't unregister inet6_net_ops. I > think that will result in a crash - immediately if ipv6 is a module, > otherwise when the next net namespace is created. Ah, I somehow misread the patch. It looks good. > >> Also, it looks like you not just revert the order changed in commit >> 15e668070a64, but also you move icmpv6_cleanup() even earlier. > > So should I add another Fixes: there? No, I think it is okay.
On Tue, Jan 9, 2018 at 10:21 AM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > Commit 15e668070a64 reordered the initialisation in inet6_init() to > fix a crash on an error path further down the call stack. It also > reordered cleanup on the error path in inet6_init(), but the result > is not the reverse of the initialisation order. This presumably > can result in a resource leak or crash in some error cases. Reorder > cleanup again to fix this. > > Fixes: 15e668070a64 ("ipv6: reorder icmpv6_init() and ip6_mr_init()") > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c index c9441ca45399..fbaa70d95d7f 100644 --- a/net/ipv6/af_inet6.c +++ b/net/ipv6/af_inet6.c @@ -1074,11 +1074,11 @@ static int __init inet6_init(void) igmp_fail: ndisc_cleanup(); ndisc_fail: - ip6_mr_cleanup(); + icmpv6_cleanup(); icmp_fail: - unregister_pernet_subsys(&inet6_net_ops); + ip6_mr_cleanup(); ipmr_fail: - icmpv6_cleanup(); + unregister_pernet_subsys(&inet6_net_ops); register_pernet_fail: sock_unregister(PF_INET6); rtnl_unregister_all(PF_INET6);
Commit 15e668070a64 reordered the initialisation in inet6_init() to fix a crash on an error path further down the call stack. It also reordered cleanup on the error path in inet6_init(), but the result is not the reverse of the initialisation order. This presumably can result in a resource leak or crash in some error cases. Reorder cleanup again to fix this. Fixes: 15e668070a64 ("ipv6: reorder icmpv6_init() and ip6_mr_init()") Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> --- This fix is untested and based only on my review of the earlier commit. Ben. net/ipv6/af_inet6.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)