[PATCH net] ipv6: Fix cleanup ordering on inet6_init() error path

[PATCH net] ipv6: Fix cleanup ordering on inet6_init() error path

[Ben Hutchings]

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(-)

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);
-- 
2.15.0.rc0

Re: [PATCH net] ipv6: Fix cleanup ordering on inet6_init() error path

[Cong Wang]

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.

Re: [PATCH net] ipv6: Fix cleanup ordering on inet6_init() error path

[Ben Hutchings]

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.

-- 
Ben Hutchings
Software Developer, Codethink Ltd.

Re: [PATCH net] ipv6: Fix cleanup ordering on inet6_init() error path

[Cong Wang]

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.

Re: [PATCH net] ipv6: Fix cleanup ordering on inet6_init() error path

[Cong Wang]

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>