sctp: add a checking for sctp_sysctl_net_register

PATCH In review Wang Weidong 3 participants 7 messages May 8, 2014

Subsystems (3)

Networking Network drivers SCTP protocol

lore

Share & tools

lore

Wang Weidong

author

2014-05-08 07:55 UTC #0 lore raw

PATCH 1/1 sctp: add a checking for sctp_sysctl_net_register

When register_net_sysctl failed, we should free the sysctl_table while the net_namespace is not init_net.

Signed-off-by Wang Weidong <[email protected]>

---
 net/sctp/sysctl.c | 5 +++++
 1 file changed, 5 insertions(+)

net/sctp/sysctl.c +5 -0

index 844d2b0..4c17694 100644
@@ -450,6 +450,11 @@ int sctp_sysctl_net_register(struct net *net)
 	}
 
 	net->sctp.sysctl_header = register_net_sysctl(net, "net/sctp", table);
+	if (net->sctp.sysctl_header = NULL) {
+		if (!net_eq(net, &init_net))
+			kfree(table);
+		return -ENOMEM;
+	}
 	return 0;
 }

Signature · 1 line

1.7.12

tingwei liu

2014-05-08 09:40 UTC #1 lore raw

Quoted 14 lines · Wang Weidong

When register_net_sysctl failed, we should free the sysctl_table while the net_namespace is not init_net.
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/sysctl.c | 5 +++++
 1 file changed, 5 insertions(+)
net/sctp/sysctl.c +0 -0
index 844d2b0..4c17694 100644
@@ -450,6 +450,11 @@ int sctp_sysctl_net_register(struct net *net)
        }

net/sctp/sysctl.c int sctp_sysctl_net_register(struct net *net) · line 460

Quoted 10 lines

net/sctp/sysctl.c +5 -0

 
        net->sctp.sysctl_header = register_net_sysctl(net, "net/sctp", table);
+       if (net->sctp.sysctl_header = NULL) {
+               if (!net_eq(net, &init_net))
+                       kfree(table);
+               return -ENOMEM;
+       }
        return 0;
 }

Hope to resolve the problem of sctp_sysctl_net_unregister() in one patch! kfree(table) should check net namesapce.

net/sctp/sysctl.c +2 -1

index c82fdc1..844d2b0 100644
@@ -459,7 +459,8 @@ void sctp_sysctl_net_unregister(struct net *net)
 
        table = net->sctp.sysctl_header->ctl_table_arg;
        unregister_net_sysctl_table(net->sctp.sysctl_header);
-       kfree(table);
+       if (!net_eq(net, &init_net))
+               kfree(table);
 }
 
 static struct ctl_table_header *sctp_sysctl_header;

Thanks
Tingwei Liu

Quoted 8 lines

--
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Neil Horman

2014-05-08 11:11 UTC #2 lore raw

In reply to Wang Weidong

Quoted 13 lines · Wang Weidong

When register_net_sysctl failed, we should free the sysctl_table while the net_namespace is not init_net.
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/sysctl.c | 5 +++++
 1 file changed, 5 insertions(+)
net/sctp/sysctl.c +0 -0
index 844d2b0..4c17694 100644

net/sctp/sysctl.c int sctp_sysctl_net_register(struct net *net) · line 457

Quoted 8 lines

net/sctp/sysctl.c +5 -0

@@ -450,6 +450,11 @@ int sctp_sysctl_net_register(struct net *net)
 	}
 
 	net->sctp.sysctl_header = register_net_sysctl(net, "net/sctp", table);
+	if (net->sctp.sysctl_header = NULL) {
+		if (!net_eq(net, &init_net))
+			kfree(table);
+		return -ENOMEM;
+	}

Sorry, we're not special casing this in 3 places. Why not just revert the origonal commit so that all the sysctl tables can be handled the same way Neil

Quoted 8 lines

net/sctp/sysctl.c +0 -0
 	return 0;
 }
 
Signature · 1 line
1.7.12

Wang Weidong

author

2014-05-08 11:26 UTC #3 lore raw

Quoted 16 lines · Neil Horman

Quoted 15 lines · Wang Weidong
When register_net_sysctl failed, we should free the sysctl_table while the net_namespace is not init_net.
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/sysctl.c | 5 +++++
 1 file changed, 5 insertions(+)
net/sctp/sysctl.c +0 -0
index 844d2b0..4c17694 100644
@@ -450,6 +450,11 @@ int sctp_sysctl_net_register(struct net *net)
 	}
 

Quoted 10 lines

net/sctp/sysctl.c int sctp_sysctl_net_register(struct net *net) · line 457
Quoted 6 lines
net/sctp/sysctl.c +5 -0
 	net->sctp.sysctl_header = register_net_sysctl(net, "net/sctp", table);
+	if (net->sctp.sysctl_header = NULL) {
+		if (!net_eq(net, &init_net))
+			kfree(table);
+		return -ENOMEM;
+	}
Sorry, we're not special casing this in 3 places. Why not just revert the origonal commit so that all the sysctl tables can be handled the same way Neil

Hi Neil,

The original commit avoids to kmemdup

Quoted 14 lines

Quoted 8 lines
net/sctp/sysctl.c +0 -0
 	return 0;
 }
 
Signature · 1 line
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Wang Weidong

author

2014-05-08 11:31 UTC #4 lore raw

Quoted 27 lines · Wang Weidong

Quoted 16 lines · Neil Horman
Quoted 15 lines · Wang Weidong
When register_net_sysctl failed, we should free the sysctl_table while the net_namespace is not init_net.
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/sysctl.c | 5 +++++
 1 file changed, 5 insertions(+)
net/sctp/sysctl.c +0 -0
index 844d2b0..4c17694 100644
@@ -450,6 +450,11 @@ int sctp_sysctl_net_register(struct net *net)
 	}
 
Quoted 10 lines
net/sctp/sysctl.c int sctp_sysctl_net_register(struct net *net) · line 457
Quoted 6 lines
net/sctp/sysctl.c +5 -0
 	net->sctp.sysctl_header = register_net_sysctl(net, "net/sctp", table);
+	if (net->sctp.sysctl_header = NULL) {
+		if (!net_eq(net, &init_net))
+			kfree(table);
+		return -ENOMEM;
+	}
Sorry, we're not special casing this in 3 places. Why not just revert the origonal commit so that all the sysctl tables can be handled the same way Neil

Quoted 4 lines

Hi Neil,
The original commit avoids to kmemdup

Hi Neil,

Sorry for the accomplished mail.
Although, revert the original commit, the checking for net->sctp.sysctl_header is need from coding view.

What do you think?

Regards
Wang

Quoted 15 lines

Quoted 14 lines
Quoted 8 lines
net/sctp/sysctl.c +0 -0
 	return 0;
 }
 
Signature · 1 line
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Neil Horman

2014-05-08 11:41 UTC #5 lore raw

Quoted 32 lines · Wang Weidong

Quoted 27 lines · Wang Weidong
Quoted 16 lines · Neil Horman
Quoted 15 lines · Wang Weidong
When register_net_sysctl failed, we should free the sysctl_table while the net_namespace is not init_net.
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/sysctl.c | 5 +++++
 1 file changed, 5 insertions(+)
net/sctp/sysctl.c +0 -0
index 844d2b0..4c17694 100644
@@ -450,6 +450,11 @@ int sctp_sysctl_net_register(struct net *net)
 	}
 
Quoted 10 lines
net/sctp/sysctl.c int sctp_sysctl_net_register(struct net *net) · line 457
Quoted 6 lines
net/sctp/sysctl.c +5 -0
 	net->sctp.sysctl_header = register_net_sysctl(net, "net/sctp", table);
+	if (net->sctp.sysctl_header = NULL) {
+		if (!net_eq(net, &init_net))
+			kfree(table);
+		return -ENOMEM;
+	}
Sorry, we're not special casing this in 3 places. Why not just revert the origonal commit so that all the sysctl tables can be handled the same way Neil
Quoted 4 lines
Hi Neil,
The original commit avoids to kmemdup

Quoted 8 lines

Hi Neil,
Sorry for the accomplished mail.
Although, revert the original commit, the checking for net->sctp.sysctl_header is need from coding view.
What do you think?

That was my review. I get that the origional commit avoids kmemdup for the init_net case. My point was that, in so doing you created the need to special case the handling of the sysctl table for init_net in 3 places now. It seems to me that, while its always nice to save space, it would be cleaner programming to just accept the need for the extra 1.6Kb of space and not have to worry about the special casing.

Neil

Quoted 25 lines

Regards
Wang
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
net/sctp/sysctl.c +0 -0
 	return 0;
 }
 
Signature · 1 line
1.7.12
Quoted 7 lines
Quoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Wang Weidong

author

2014-05-08 11:57 UTC #6 lore raw

Quoted 41 lines · Neil Horman

Quoted 32 lines · Wang Weidong
Quoted 27 lines · Wang Weidong
Quoted 16 lines · Neil Horman
Quoted 15 lines · Wang Weidong
When register_net_sysctl failed, we should free the sysctl_table while the net_namespace is not init_net.
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/sysctl.c | 5 +++++
 1 file changed, 5 insertions(+)
net/sctp/sysctl.c +0 -0
index 844d2b0..4c17694 100644
@@ -450,6 +450,11 @@ int sctp_sysctl_net_register(struct net *net)
 	}
 
Quoted 10 lines
net/sctp/sysctl.c int sctp_sysctl_net_register(struct net *net) · line 457
Quoted 6 lines
net/sctp/sysctl.c +5 -0
 	net->sctp.sysctl_header = register_net_sysctl(net, "net/sctp", table);
+	if (net->sctp.sysctl_header = NULL) {
+		if (!net_eq(net, &init_net))
+			kfree(table);
+		return -ENOMEM;
+	}
Sorry, we're not special casing this in 3 places. Why not just revert the origonal commit so that all the sysctl tables can be handled the same way Neil
Quoted 4 lines
Hi Neil,
The original commit avoids to kmemdup
Quoted 8 lines
Hi Neil,
Sorry for the accomplished mail.
Although, revert the original commit, the checking for net->sctp.sysctl_header is need from coding view.
What do you think?

Quoted 9 lines

That was my review. I get that the origional commit avoids kmemdup for the init_net case. My point was that, in so doing you created the need to special case the handling of the sysctl table for init_net in 3 places now. It seems to me that, while its always nice to save space, it would be cleaner programming to just accept the need for the extra 1.6Kb of space and not have to worry about the special casing.
Neil

Hm, Should I add this:

+ if (net->sctp.sysctl_header = NULL);
+ kfree(table);

to the revert patch or do a new patch?

Regards
Wang

Quoted 28 lines

Quoted 20 lines
Regards
Wang
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
net/sctp/sysctl.c +0 -0
 	return 0;
 }
 
Signature · 1 line
1.7.12
Quoted 7 lines
Quoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
.