diff mbox series

net/smc: Fix possible leaked pernet namespace in smc_init()

Message ID 20221101093722.127223-1-chenzhongjin@huawei.com
State New
Headers
Series net/smc: Fix possible leaked pernet namespace in smc_init() |

Commit Message

Chen Zhongjin Nov. 1, 2022, 9:37 a.m. UTC 
  In smc_init(), register_pernet_subsys(&smc_net_stat_ops) is called
without any error handling.
If it fails, registering of &smc_net_ops won't be reverted.
And if smc_nl_init() fails, &smc_net_stat_ops itself won't be reverted.

This leaves wild ops in subsystem linkedlist and when another module
tries to call register_pernet_operations() it triggers page fault:

BUG: unable to handle page fault for address: fffffbfff81b964c
RIP: 0010:register_pernet_operations+0x1b9/0x5f0
Call Trace:
  <TASK>
  register_pernet_subsys+0x29/0x40
  ebtables_init+0x58/0x1000 [ebtables]
  ...

Fixes: 194730a9beb5 ("net/smc: Make SMC statistics network namespace aware")
Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 net/smc/af_smc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Tony Lu Nov. 1, 2022, 11 a.m. UTC | #1 
On Tue, Nov 01, 2022 at 05:37:22PM +0800, Chen Zhongjin wrote:
> In smc_init(), register_pernet_subsys(&smc_net_stat_ops) is called
> without any error handling.
> If it fails, registering of &smc_net_ops won't be reverted.
> And if smc_nl_init() fails, &smc_net_stat_ops itself won't be reverted.
> 
> This leaves wild ops in subsystem linkedlist and when another module
> tries to call register_pernet_operations() it triggers page fault:
> 
> BUG: unable to handle page fault for address: fffffbfff81b964c
> RIP: 0010:register_pernet_operations+0x1b9/0x5f0
> Call Trace:
>   <TASK>
>   register_pernet_subsys+0x29/0x40
>   ebtables_init+0x58/0x1000 [ebtables]
>   ...
> 
> Fixes: 194730a9beb5 ("net/smc: Make SMC statistics network namespace aware")
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>

This patch looks good to me. 

The subject of this patch should be in net, the prefix tag is missed.

Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>

> ---
>  net/smc/af_smc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 3ccbf3c201cd..e12d4fa5aece 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -3380,14 +3380,14 @@ static int __init smc_init(void)
>  
>  	rc = register_pernet_subsys(&smc_net_stat_ops);
>  	if (rc)
> -		return rc;
> +		goto out_pernet_subsys;
>  
>  	smc_ism_init();
>  	smc_clc_init();
>  
>  	rc = smc_nl_init();
>  	if (rc)
> -		goto out_pernet_subsys;
> +		goto out_pernet_subsys_stat;
>  
>  	rc = smc_pnet_init();
>  	if (rc)
> @@ -3480,6 +3480,8 @@ static int __init smc_init(void)
>  	smc_pnet_exit();
>  out_nl:
>  	smc_nl_exit();
> +out_pernet_subsys_stat:
> +	unregister_pernet_subsys(&smc_net_stat_ops);
>  out_pernet_subsys:
>  	unregister_pernet_subsys(&smc_net_ops);
>  
> -- 
> 2.17.1
Chen Zhongjin Nov. 1, 2022, 11:07 a.m. UTC | #2 
Hi,

On 2022/11/1 19:00, Tony Lu wrote:
> On Tue, Nov 01, 2022 at 05:37:22PM +0800, Chen Zhongjin wrote:
>> In smc_init(), register_pernet_subsys(&smc_net_stat_ops) is called
>> without any error handling.
>> If it fails, registering of &smc_net_ops won't be reverted.
>> And if smc_nl_init() fails, &smc_net_stat_ops itself won't be reverted.
>>
>> This leaves wild ops in subsystem linkedlist and when another module
>> tries to call register_pernet_operations() it triggers page fault:
>>
>> BUG: unable to handle page fault for address: fffffbfff81b964c
>> RIP: 0010:register_pernet_operations+0x1b9/0x5f0
>> Call Trace:
>>    <TASK>
>>    register_pernet_subsys+0x29/0x40
>>    ebtables_init+0x58/0x1000 [ebtables]
>>    ...
>>
>> Fixes: 194730a9beb5 ("net/smc: Make SMC statistics network namespace aware")
>> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> This patch looks good to me.
>
> The subject of this patch should be in net, the prefix tag is missed.

Thanks for review and remind! I will add net tag for the next time I 
send patch to netdev.


Best,

Chen

>
> Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
>
>> ---
>>   net/smc/af_smc.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index 3ccbf3c201cd..e12d4fa5aece 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -3380,14 +3380,14 @@ static int __init smc_init(void)
>>   
>>   	rc = register_pernet_subsys(&smc_net_stat_ops);
>>   	if (rc)
>> -		return rc;
>> +		goto out_pernet_subsys;
>>   
>>   	smc_ism_init();
>>   	smc_clc_init();
>>   
>>   	rc = smc_nl_init();
>>   	if (rc)
>> -		goto out_pernet_subsys;
>> +		goto out_pernet_subsys_stat;
>>   
>>   	rc = smc_pnet_init();
>>   	if (rc)
>> @@ -3480,6 +3480,8 @@ static int __init smc_init(void)
>>   	smc_pnet_exit();
>>   out_nl:
>>   	smc_nl_exit();
>> +out_pernet_subsys_stat:
>> +	unregister_pernet_subsys(&smc_net_stat_ops);
>>   out_pernet_subsys:
>>   	unregister_pernet_subsys(&smc_net_ops);
>>   
>> -- 
>> 2.17.1
Wenjia Zhang Nov. 1, 2022, 12:54 p.m. UTC | #3 
On 01.11.22 10:37, Chen Zhongjin wrote:
> In smc_init(), register_pernet_subsys(&smc_net_stat_ops) is called
> without any error handling.
> If it fails, registering of &smc_net_ops won't be reverted.
> And if smc_nl_init() fails, &smc_net_stat_ops itself won't be reverted.
> 
> This leaves wild ops in subsystem linkedlist and when another module
> tries to call register_pernet_operations() it triggers page fault:
> 
> BUG: unable to handle page fault for address: fffffbfff81b964c
> RIP: 0010:register_pernet_operations+0x1b9/0x5f0
> Call Trace:
>    <TASK>
>    register_pernet_subsys+0x29/0x40
>    ebtables_init+0x58/0x1000 [ebtables]
>    ...
> 
> Fixes: 194730a9beb5 ("net/smc: Make SMC statistics network namespace aware")
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>

It looks good, thank you for the effort!

Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
> ---
>   net/smc/af_smc.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 3ccbf3c201cd..e12d4fa5aece 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -3380,14 +3380,14 @@ static int __init smc_init(void)
>   
>   	rc = register_pernet_subsys(&smc_net_stat_ops);
>   	if (rc)
> -		return rc;
> +		goto out_pernet_subsys;
>   
>   	smc_ism_init();
>   	smc_clc_init();
>   
>   	rc = smc_nl_init();
>   	if (rc)
> -		goto out_pernet_subsys;
> +		goto out_pernet_subsys_stat;
>   
>   	rc = smc_pnet_init();
>   	if (rc)
> @@ -3480,6 +3480,8 @@ static int __init smc_init(void)
>   	smc_pnet_exit();
>   out_nl:
>   	smc_nl_exit();
> +out_pernet_subsys_stat:
> +	unregister_pernet_subsys(&smc_net_stat_ops);
>   out_pernet_subsys:
>   	unregister_pernet_subsys(&smc_net_ops);
>
patchwork-bot+netdevbpf@kernel.org Nov. 3, 2022, 3:50 a.m. UTC | #4 
Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 1 Nov 2022 17:37:22 +0800 you wrote:
> In smc_init(), register_pernet_subsys(&smc_net_stat_ops) is called
> without any error handling.
> If it fails, registering of &smc_net_ops won't be reverted.
> And if smc_nl_init() fails, &smc_net_stat_ops itself won't be reverted.
> 
> This leaves wild ops in subsystem linkedlist and when another module
> tries to call register_pernet_operations() it triggers page fault:
> 
> [...]

Here is the summary with links:
  - net/smc: Fix possible leaked pernet namespace in smc_init()
    https://git.kernel.org/netdev/net/c/62ff373da253

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 3ccbf3c201cd..e12d4fa5aece 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -3380,14 +3380,14 @@  static int __init smc_init(void)
 
 	rc = register_pernet_subsys(&smc_net_stat_ops);
 	if (rc)
-		return rc;
+		goto out_pernet_subsys;
 
 	smc_ism_init();
 	smc_clc_init();
 
 	rc = smc_nl_init();
 	if (rc)
-		goto out_pernet_subsys;
+		goto out_pernet_subsys_stat;
 
 	rc = smc_pnet_init();
 	if (rc)
@@ -3480,6 +3480,8 @@  static int __init smc_init(void)
 	smc_pnet_exit();
 out_nl:
 	smc_nl_exit();
+out_pernet_subsys_stat:
+	unregister_pernet_subsys(&smc_net_stat_ops);
 out_pernet_subsys:
 	unregister_pernet_subsys(&smc_net_ops);