netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] sctp: fix a problem with net_namespace
@ 2014-01-27  3:49 Wang Weidong
  2014-01-27  3:49 ` [PATCH 1/2] sctp: fix a missed .data initialization Wang Weidong
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Wang Weidong @ 2014-01-27  3:49 UTC (permalink / raw)
  To: nhorman, davem, vyasevich; +Cc: dborkman, netdev

fix a problem with net_namespace, and optimize
the sctp_sysctl_net_register.

Wang Weidong (2):
  sctp: fix a missed .data initialization
  sctp: optimize the sctp_sysctl_net_register

 net/sctp/sysctl.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

-- 
1.7.12

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] sctp: fix a missed .data initialization
  2014-01-27  3:49 [PATCH 0/2] sctp: fix a problem with net_namespace Wang Weidong
@ 2014-01-27  3:49 ` Wang Weidong
  2014-01-27  3:49 ` [PATCH 2/2] sctp: optimize the sctp_sysctl_net_register Wang Weidong
  2014-01-27 11:49 ` [PATCH 0/2] sctp: fix a problem with net_namespace Neil Horman
  2 siblings, 0 replies; 10+ messages in thread
From: Wang Weidong @ 2014-01-27  3:49 UTC (permalink / raw)
  To: nhorman, davem, vyasevich; +Cc: dborkman, netdev

As commit 3c68198e75111a90("sctp: Make hmac algorithm selection for
 cookie generation dynamic"), we miss the .data initialization.
If we don't use the net_namespace, the problem that parts of the
sysctl configuration won't be isolation and won't occur.

In sctp_sysctl_net_register(), we register the sysctl for each
net, in the for(), we use the 'table[i].data' as check condition, so
when the 'i' is the index of sctp_hmac_alg, the data is NULL, then
break. So add the .data initialization.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/sysctl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index b0565af..2ddb401 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -152,6 +152,7 @@ static struct ctl_table sctp_net_table[] = {
 	},
 	{
 		.procname	= "cookie_hmac_alg",
+		.data		= &init_net.sctp.sctp_hmac_alg,
 		.maxlen		= 8,
 		.mode		= 0644,
 		.proc_handler	= proc_sctp_do_hmac_alg,
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] sctp: optimize the sctp_sysctl_net_register
  2014-01-27  3:49 [PATCH 0/2] sctp: fix a problem with net_namespace Wang Weidong
  2014-01-27  3:49 ` [PATCH 1/2] sctp: fix a missed .data initialization Wang Weidong
@ 2014-01-27  3:49 ` Wang Weidong
  2014-01-27 11:49 ` [PATCH 0/2] sctp: fix a problem with net_namespace Neil Horman
  2 siblings, 0 replies; 10+ messages in thread
From: Wang Weidong @ 2014-01-27  3:49 UTC (permalink / raw)
  To: nhorman, davem, vyasevich; +Cc: dborkman, netdev

Here, when the net is init_net, we needn't to kmemdup the ctl_table
again. So add a check for net. Also we can save some memory.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/sysctl.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index 2ddb401..b65396b 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -403,15 +403,17 @@ static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
 
 int sctp_sysctl_net_register(struct net *net)
 {
-	struct ctl_table *table;
-	int i;
+	struct ctl_table *table = sctp_net_table;
 
-	table = kmemdup(sctp_net_table, sizeof(sctp_net_table), GFP_KERNEL);
-	if (!table)
-		return -ENOMEM;
+	if (!net_eq(net, &init_net)) {
+		int i;
+		table = kmemdup(sctp_net_table, sizeof(sctp_net_table), GFP_KERNEL);
+		if (!table)
+			return -ENOMEM;
 
-	for (i = 0; table[i].data; i++)
-		table[i].data += (char *)(&net->sctp) - (char *)&init_net.sctp;
+		for (i = 0; table[i].data; i++)
+			table[i].data += (char *)(&net->sctp) - (char *)&init_net.sctp;
+	}
 
 	net->sctp.sysctl_header = register_net_sysctl(net, "net/sctp", table);
 	return 0;
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] sctp: fix a problem with net_namespace
  2014-01-27  3:49 [PATCH 0/2] sctp: fix a problem with net_namespace Wang Weidong
  2014-01-27  3:49 ` [PATCH 1/2] sctp: fix a missed .data initialization Wang Weidong
  2014-01-27  3:49 ` [PATCH 2/2] sctp: optimize the sctp_sysctl_net_register Wang Weidong
@ 2014-01-27 11:49 ` Neil Horman
  2014-01-27 13:05   ` Wang Weidong
  2014-01-28  8:13   ` Wang Weidong
  2 siblings, 2 replies; 10+ messages in thread
From: Neil Horman @ 2014-01-27 11:49 UTC (permalink / raw)
  To: Wang Weidong; +Cc: davem, vyasevich, dborkman, netdev

On Mon, Jan 27, 2014 at 11:49:01AM +0800, Wang Weidong wrote:
> fix a problem with net_namespace, and optimize
> the sctp_sysctl_net_register.
> 
> Wang Weidong (2):
>   sctp: fix a missed .data initialization
>   sctp: optimize the sctp_sysctl_net_register
> 
>  net/sctp/sysctl.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> -- 
> 1.7.12
> 
> 
> 
I don't see that either of these patches are needed.  In sctp_init_net, the
sctp_hmac_alg pointer gets initalized before calling sctp_sysctl_net_register,
and sctp_proc_do_hmac_alg is written to specifically expect NULL values, so this
code may change behavior regarding default cookie selection.

This was coded so that poniters to entires in the string table could be used,
rather than needing to allocate or maintain character buffers.  That said, it
does look like that for loop in sctp_sysctl_register_table might compute an odd
offset when cloning the table.  I think the right fix for that is likely to just
move the sysctl value initalization in sctp_init_net to below the sysctl
register function.

Neil

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] sctp: fix a problem with net_namespace
  2014-01-27 11:49 ` [PATCH 0/2] sctp: fix a problem with net_namespace Neil Horman
@ 2014-01-27 13:05   ` Wang Weidong
  2014-01-28  8:13   ` Wang Weidong
  1 sibling, 0 replies; 10+ messages in thread
From: Wang Weidong @ 2014-01-27 13:05 UTC (permalink / raw)
  To: Neil Horman, Wang Weidong; +Cc: davem, vyasevich, dborkman, netdev

From: Wang Weidong <wangweidong1@huawei.com>

On 2014/1/27 19:49, Neil Horman wrote:
> On Mon, Jan 27, 2014 at 11:49:01AM +0800, Wang Weidong wrote:
>> fix a problem with net_namespace, and optimize
>> the sctp_sysctl_net_register.
>>
>> Wang Weidong (2):
>>    sctp: fix a missed .data initialization
>>    sctp: optimize the sctp_sysctl_net_register
>>
>>   net/sctp/sysctl.c | 17 ++++++++++-------
>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> --
>> 1.7.12
>>
>>
>>
> I don't see that either of these patches are needed.  In sctp_init_net, the
> sctp_hmac_alg pointer gets initalized before calling sctp_sysctl_net_register,
> and sctp_proc_do_hmac_alg is written to specifically expect NULL values, so this
> code may change behavior regarding default cookie selection.
>
> This was coded so that poniters to entires in the string table could be used,
> rather than needing to allocate or maintain character buffers.  That said, it
> does look like that for loop in sctp_sysctl_register_table might compute an odd
> offset when cloning the table.  I think the right fix for that is likely to just
> move the sysctl value initalization in sctp_init_net to below the sysctl
> register function.
>
> Neil
>

Thanks Neil,
I will try to refix it as you said tomorrow because I am not at the office.

Regards,
Wang

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] sctp: fix a problem with net_namespace
  2014-01-27 11:49 ` [PATCH 0/2] sctp: fix a problem with net_namespace Neil Horman
  2014-01-27 13:05   ` Wang Weidong
@ 2014-01-28  8:13   ` Wang Weidong
  2014-01-28 11:57     ` Neil Horman
  1 sibling, 1 reply; 10+ messages in thread
From: Wang Weidong @ 2014-01-28  8:13 UTC (permalink / raw)
  To: Neil Horman; +Cc: davem, vyasevich, dborkman, netdev

On 2014/1/27 19:49, Neil Horman wrote:
> On Mon, Jan 27, 2014 at 11:49:01AM +0800, Wang Weidong wrote:
>> fix a problem with net_namespace, and optimize
>> the sctp_sysctl_net_register.
>>
>> Wang Weidong (2):
>>   sctp: fix a missed .data initialization
>>   sctp: optimize the sctp_sysctl_net_register
>>
>>  net/sctp/sysctl.c | 17 ++++++++++-------
>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> -- 
>> 1.7.12
>>
>>
>>
> I don't see that either of these patches are needed.  In sctp_init_net, the
> sctp_hmac_alg pointer gets initalized before calling sctp_sysctl_net_register,
> and sctp_proc_do_hmac_alg is written to specifically expect NULL values, so this
> code may change behavior regarding default cookie selection.
> 
Hi Neil,

Here, I think the sctp_proc_do_hmac_alg will be called only when we change the 
/proc/sys/net/cookie_hmac_alg. So add the .data won't effect the default value.
and the data isn't equal to the "cookie_hmac_alg"?

> This was coded so that poniters to entires in the string table could be used,
> rather than needing to allocate or maintain character buffers.  That said, it
> does look like that for loop in sctp_sysctl_register_table might compute an odd
> offset when cloning the table.  I think the right fix for that is likely to just
> move the sysctl value initalization in sctp_init_net to below the sysctl
> register function.
> 
> Neil
> 

I found the problem is that:
I use "ip netns add netns1/netns2"
In any netns(netns1 or netns2 or init_net) when I change the value of the entry
such as "addip_enable" "max_autoclose" which after the cookie_hmac_alg (contain it),
and the other netns will be effected.

In sctp_sysctl_net_register, kmemdup does cloning the table. The offset of netns1 and
init_net's clt_table.data is the same as two netns offset. So the for(){...} would do
add the offset for every clt_table.data.

The code:
	for (i = 0; table[i].data; i++)
		table[i].data += (char *)(&net->sctp) - (char *)&init_net.sctp;

And I add a pr_info into the for(){...} in sctp_sysctl_net_register and only print 7 times for each ns.
7 is the index of "cookie_preserve_enable" which before the "cookie_hmac_alg". 

As the "cookie_hmac_alg" data is NULL, so we can't add offset to the rest, and all the netns use the same
address of clt_table entry after the "cookie_hmac_alg".

So I think only "move the sysctl value initalization in sctp_init_net to below the sysctl
register function" won't solve the problem, because the problem is at the for() {...}.

Is there something wrong?

The next patch is that, the sctp_net_table is for init_net, So when load the sctp module, we needn't
to do the cloning tables for init_net again. And I found the ipv4 do it in the same way.

I have a doubt : how can I rmmod the sctp? only add '-f' ? If I do "rmmod -f sctp", I will get the
log by dmesg: "Disabling lock debugging due to kernel taint"

Regards,
Wang


> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] sctp: fix a problem with net_namespace
  2014-01-28  8:13   ` Wang Weidong
@ 2014-01-28 11:57     ` Neil Horman
  2014-01-28 14:42       ` Wang Weidong
  2014-02-10  2:56       ` Wang Weidong
  0 siblings, 2 replies; 10+ messages in thread
From: Neil Horman @ 2014-01-28 11:57 UTC (permalink / raw)
  To: Wang Weidong; +Cc: davem, vyasevich, dborkman, netdev

On Tue, Jan 28, 2014 at 04:13:44PM +0800, Wang Weidong wrote:
> On 2014/1/27 19:49, Neil Horman wrote:
> > On Mon, Jan 27, 2014 at 11:49:01AM +0800, Wang Weidong wrote:
> >> fix a problem with net_namespace, and optimize
> >> the sctp_sysctl_net_register.
> >>
> >> Wang Weidong (2):
> >>   sctp: fix a missed .data initialization
> >>   sctp: optimize the sctp_sysctl_net_register
> >>
> >>  net/sctp/sysctl.c | 17 ++++++++++-------
> >>  1 file changed, 10 insertions(+), 7 deletions(-)
> >>
> >> -- 
> >> 1.7.12
> >>
> >>
> >>
> > I don't see that either of these patches are needed.  In sctp_init_net, the
> > sctp_hmac_alg pointer gets initalized before calling sctp_sysctl_net_register,
> > and sctp_proc_do_hmac_alg is written to specifically expect NULL values, so this
> > code may change behavior regarding default cookie selection.
> > 
> Hi Neil,
> 
> Here, I think the sctp_proc_do_hmac_alg will be called only when we change the 
> /proc/sys/net/cookie_hmac_alg. So add the .data won't effect the default value.
> and the data isn't equal to the "cookie_hmac_alg"?
> 
> > This was coded so that poniters to entires in the string table could be used,
> > rather than needing to allocate or maintain character buffers.  That said, it
> > does look like that for loop in sctp_sysctl_register_table might compute an odd
> > offset when cloning the table.  I think the right fix for that is likely to just
> > move the sysctl value initalization in sctp_init_net to below the sysctl
> > register function.
> > 
> > Neil
> > 
> 
> I found the problem is that:
> I use "ip netns add netns1/netns2"
> In any netns(netns1 or netns2 or init_net) when I change the value of the entry
> such as "addip_enable" "max_autoclose" which after the cookie_hmac_alg (contain it),
> and the other netns will be effected.
> 
> In sctp_sysctl_net_register, kmemdup does cloning the table. The offset of netns1 and
> init_net's clt_table.data is the same as two netns offset. So the for(){...} would do
> add the offset for every clt_table.data.
> 
> The code:
> 	for (i = 0; table[i].data; i++)
> 		table[i].data += (char *)(&net->sctp) - (char *)&init_net.sctp;
> 
> And I add a pr_info into the for(){...} in sctp_sysctl_net_register and only print 7 times for each ns.
> 7 is the index of "cookie_preserve_enable" which before the "cookie_hmac_alg". 
> 
> As the "cookie_hmac_alg" data is NULL, so we can't add offset to the rest, and all the netns use the same
> address of clt_table entry after the "cookie_hmac_alg".
> 
> So I think only "move the sysctl value initalization in sctp_init_net to below the sysctl
> register function" won't solve the problem, because the problem is at the for() {...}.
> 
I'm sorry, you're right, the kmemdup duplicates the table, not the structure in
which the table points to with its .data pointers.  I was looking at it
backwards.

> Is there something wrong?
> 
> The next patch is that, the sctp_net_table is for init_net, So when load the sctp module, we needn't
> to do the cloning tables for init_net again. And I found the ipv4 do it in the same way.
> 
> I have a doubt : how can I rmmod the sctp? only add '-f' ? If I do "rmmod -f sctp", I will get the
> log by dmesg: "Disabling lock debugging due to kernel taint"
> 
I don't know, sounds like a bug, check the log to see what tainted the kernel
during removal.

Regards
Neil

> Regards,
> Wang
> 
> 
> > 
> > 
> 
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] sctp: fix a problem with net_namespace
  2014-01-28 11:57     ` Neil Horman
@ 2014-01-28 14:42       ` Wang Weidong
  2014-02-10  2:56       ` Wang Weidong
  1 sibling, 0 replies; 10+ messages in thread
From: Wang Weidong @ 2014-01-28 14:42 UTC (permalink / raw)
  To: Neil Horman, Wang Weidong; +Cc: davem, vyasevich, dborkman, netdev

From: Wang Weidong <wangweidong1@huawei.com>

On 2014/1/28 19:57, Neil Horman wrote:
> On Tue, Jan 28, 2014 at 04:13:44PM +0800, Wang Weidong wrote:
>> On 2014/1/27 19:49, Neil Horman wrote:
>>> On Mon, Jan 27, 2014 at 11:49:01AM +0800, Wang Weidong wrote:
>>>> fix a problem with net_namespace, and optimize
>>>> the sctp_sysctl_net_register.
>>>>
>>>> Wang Weidong (2):
>>>>    sctp: fix a missed .data initialization
>>>>    sctp: optimize the sctp_sysctl_net_register
>>>>
>>>>   net/sctp/sysctl.c | 17 ++++++++++-------
>>>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>>>
>>>> --
>>>> 1.7.12
>>>>
>>>>
>>>>
>>> I don't see that either of these patches are needed.  In sctp_init_net, the
>>> sctp_hmac_alg pointer gets initalized before calling sctp_sysctl_net_register,
>>> and sctp_proc_do_hmac_alg is written to specifically expect NULL values, so this
>>> code may change behavior regarding default cookie selection.
>>>
>> Hi Neil,
>>
>> Here, I think the sctp_proc_do_hmac_alg will be called only when we change the
>> /proc/sys/net/cookie_hmac_alg. So add the .data won't effect the default value.
>> and the data isn't equal to the "cookie_hmac_alg"?
>>
>>> This was coded so that poniters to entires in the string table could be used,
>>> rather than needing to allocate or maintain character buffers.  That said, it
>>> does look like that for loop in sctp_sysctl_register_table might compute an odd
>>> offset when cloning the table.  I think the right fix for that is likely to just
>>> move the sysctl value initalization in sctp_init_net to below the sysctl
>>> register function.
>>>
>>> Neil
>>>
>>
>> I found the problem is that:
>> I use "ip netns add netns1/netns2"
>> In any netns(netns1 or netns2 or init_net) when I change the value of the entry
>> such as "addip_enable" "max_autoclose" which after the cookie_hmac_alg (contain it),
>> and the other netns will be effected.
>>
>> In sctp_sysctl_net_register, kmemdup does cloning the table. The offset of netns1 and
>> init_net's clt_table.data is the same as two netns offset. So the for(){...} would do
>> add the offset for every clt_table.data.
>>
>> The code:
>> 	for (i = 0; table[i].data; i++)
>> 		table[i].data += (char *)(&net->sctp) - (char *)&init_net.sctp;
>>
>> And I add a pr_info into the for(){...} in sctp_sysctl_net_register and only print 7 times for each ns.
>> 7 is the index of "cookie_preserve_enable" which before the "cookie_hmac_alg".
>>
>> As the "cookie_hmac_alg" data is NULL, so we can't add offset to the rest, and all the netns use the same
>> address of clt_table entry after the "cookie_hmac_alg".
>>
>> So I think only "move the sysctl value initalization in sctp_init_net to below the sysctl
>> register function" won't solve the problem, because the problem is at the for() {...}.
>>
> I'm sorry, you're right, the kmemdup duplicates the table, not the structure in
> which the table points to with its .data pointers.  I was looking at it
> backwards.
>
>> Is there something wrong?
>>
>> The next patch is that, the sctp_net_table is for init_net, So when load the sctp module, we needn't
>> to do the cloning tables for init_net again. And I found the ipv4 do it in the same way.
>>
>> I have a doubt : how can I rmmod the sctp? only add '-f' ? If I do "rmmod -f sctp", I will get the
>> log by dmesg: "Disabling lock debugging due to kernel taint"
>>
> I don't know, sounds like a bug, check the log to see what tainted the kernel
> during removal.
>
> Regards
> Neil
>

Ok, I will have a vacation from 1.29 to 2.9.

Happy New Year!

Regards,
Wang

>> Regards,
>> Wang
>>
>>
>>>
>>>
>>
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] sctp: fix a problem with net_namespace
  2014-01-28 11:57     ` Neil Horman
  2014-01-28 14:42       ` Wang Weidong
@ 2014-02-10  2:56       ` Wang Weidong
  2014-02-10 12:08         ` Neil Horman
  1 sibling, 1 reply; 10+ messages in thread
From: Wang Weidong @ 2014-02-10  2:56 UTC (permalink / raw)
  To: Neil Horman; +Cc: davem, vyasevich, dborkman, netdev

On 2014/1/28 19:57, Neil Horman wrote:
> On Tue, Jan 28, 2014 at 04:13:44PM +0800, Wang Weidong wrote:
>> On 2014/1/27 19:49, Neil Horman wrote:
>>> On Mon, Jan 27, 2014 at 11:49:01AM +0800, Wang Weidong wrote:
>>>> fix a problem with net_namespace, and optimize
>>>> the sctp_sysctl_net_register.
>>>>
>>>> Wang Weidong (2):
>>>>   sctp: fix a missed .data initialization
>>>>   sctp: optimize the sctp_sysctl_net_register
>>>>
>>>>  net/sctp/sysctl.c | 17 ++++++++++-------
>>>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>>>
>>>> -- 
>>>> 1.7.12
>>>>
>>>>
>>>>
>>> I don't see that either of these patches are needed.  In sctp_init_net, the
>>> sctp_hmac_alg pointer gets initalized before calling sctp_sysctl_net_register,
>>> and sctp_proc_do_hmac_alg is written to specifically expect NULL values, so this
>>> code may change behavior regarding default cookie selection.
>>>
>> Hi Neil,
>>
>> Here, I think the sctp_proc_do_hmac_alg will be called only when we change the 
>> /proc/sys/net/cookie_hmac_alg. So add the .data won't effect the default value.
>> and the data isn't equal to the "cookie_hmac_alg"?
>>
>>> This was coded so that poniters to entires in the string table could be used,
>>> rather than needing to allocate or maintain character buffers.  That said, it
>>> does look like that for loop in sctp_sysctl_register_table might compute an odd
>>> offset when cloning the table.  I think the right fix for that is likely to just
>>> move the sysctl value initalization in sctp_init_net to below the sysctl
>>> register function.
>>>
>>> Neil
>>>
>>
>> I found the problem is that:
>> I use "ip netns add netns1/netns2"
>> In any netns(netns1 or netns2 or init_net) when I change the value of the entry
>> such as "addip_enable" "max_autoclose" which after the cookie_hmac_alg (contain it),
>> and the other netns will be effected.
>>
>> In sctp_sysctl_net_register, kmemdup does cloning the table. The offset of netns1 and
>> init_net's clt_table.data is the same as two netns offset. So the for(){...} would do
>> add the offset for every clt_table.data.
>>
>> The code:
>> 	for (i = 0; table[i].data; i++)
>> 		table[i].data += (char *)(&net->sctp) - (char *)&init_net.sctp;
>>
>> And I add a pr_info into the for(){...} in sctp_sysctl_net_register and only print 7 times for each ns.
>> 7 is the index of "cookie_preserve_enable" which before the "cookie_hmac_alg". 
>>
>> As the "cookie_hmac_alg" data is NULL, so we can't add offset to the rest, and all the netns use the same
>> address of clt_table entry after the "cookie_hmac_alg".
>>
>> So I think only "move the sysctl value initalization in sctp_init_net to below the sysctl
>> register function" won't solve the problem, because the problem is at the for() {...}.
>>
> I'm sorry, you're right, the kmemdup duplicates the table, not the structure in
> which the table points to with its .data pointers.  I was looking at it
> backwards.
> 

Hi Neil,

Should I resend "sctp: fix a problem with net_namespace" series again?

>> Is there something wrong?
>>
>> The next patch is that, the sctp_net_table is for init_net, So when load the sctp module, we needn't
>> to do the cloning tables for init_net again. And I found the ipv4 do it in the same way.
>>
>> I have a doubt : how can I rmmod the sctp? only add '-f' ? If I do "rmmod -f sctp", I will get the
>> log by dmesg: "Disabling lock debugging due to kernel taint"
>>
> I don't know, sounds like a bug, check the log to see what tainted the kernel
> during removal.
> 

Here, after removal, got "Disabling lock debugging due to kernel taint", it would effect the lockdep.
When load the sctp modules, Use "cat /proc/net/sctp/sctp_dbg_objcnt", we will create a socket and ep.
So the sctp is in use.I am not sure is there any way to kill the socket and ep.

Regards
Wang

> Regards
> Neil
> 
>> Regards,
>> Wang
>>
>>
>>>
>>>
>>
>>
>>
> 
> .
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] sctp: fix a problem with net_namespace
  2014-02-10  2:56       ` Wang Weidong
@ 2014-02-10 12:08         ` Neil Horman
  0 siblings, 0 replies; 10+ messages in thread
From: Neil Horman @ 2014-02-10 12:08 UTC (permalink / raw)
  To: Wang Weidong; +Cc: davem, vyasevich, dborkman, netdev

On Mon, Feb 10, 2014 at 10:56:51AM +0800, Wang Weidong wrote:
> On 2014/1/28 19:57, Neil Horman wrote:
> > On Tue, Jan 28, 2014 at 04:13:44PM +0800, Wang Weidong wrote:
> >> On 2014/1/27 19:49, Neil Horman wrote:
> >>> On Mon, Jan 27, 2014 at 11:49:01AM +0800, Wang Weidong wrote:
> >>>> fix a problem with net_namespace, and optimize
> >>>> the sctp_sysctl_net_register.
> >>>>
> >>>> Wang Weidong (2):
> >>>>   sctp: fix a missed .data initialization
> >>>>   sctp: optimize the sctp_sysctl_net_register
> >>>>
> >>>>  net/sctp/sysctl.c | 17 ++++++++++-------
> >>>>  1 file changed, 10 insertions(+), 7 deletions(-)
> >>>>
> >>>> -- 
> >>>> 1.7.12
> >>>>
> >>>>
> >>>>
> >>> I don't see that either of these patches are needed.  In sctp_init_net, the
> >>> sctp_hmac_alg pointer gets initalized before calling sctp_sysctl_net_register,
> >>> and sctp_proc_do_hmac_alg is written to specifically expect NULL values, so this
> >>> code may change behavior regarding default cookie selection.
> >>>
> >> Hi Neil,
> >>
> >> Here, I think the sctp_proc_do_hmac_alg will be called only when we change the 
> >> /proc/sys/net/cookie_hmac_alg. So add the .data won't effect the default value.
> >> and the data isn't equal to the "cookie_hmac_alg"?
> >>
> >>> This was coded so that poniters to entires in the string table could be used,
> >>> rather than needing to allocate or maintain character buffers.  That said, it
> >>> does look like that for loop in sctp_sysctl_register_table might compute an odd
> >>> offset when cloning the table.  I think the right fix for that is likely to just
> >>> move the sysctl value initalization in sctp_init_net to below the sysctl
> >>> register function.
> >>>
> >>> Neil
> >>>
> >>
> >> I found the problem is that:
> >> I use "ip netns add netns1/netns2"
> >> In any netns(netns1 or netns2 or init_net) when I change the value of the entry
> >> such as "addip_enable" "max_autoclose" which after the cookie_hmac_alg (contain it),
> >> and the other netns will be effected.
> >>
> >> In sctp_sysctl_net_register, kmemdup does cloning the table. The offset of netns1 and
> >> init_net's clt_table.data is the same as two netns offset. So the for(){...} would do
> >> add the offset for every clt_table.data.
> >>
> >> The code:
> >> 	for (i = 0; table[i].data; i++)
> >> 		table[i].data += (char *)(&net->sctp) - (char *)&init_net.sctp;
> >>
> >> And I add a pr_info into the for(){...} in sctp_sysctl_net_register and only print 7 times for each ns.
> >> 7 is the index of "cookie_preserve_enable" which before the "cookie_hmac_alg". 
> >>
> >> As the "cookie_hmac_alg" data is NULL, so we can't add offset to the rest, and all the netns use the same
> >> address of clt_table entry after the "cookie_hmac_alg".
> >>
> >> So I think only "move the sysctl value initalization in sctp_init_net to below the sysctl
> >> register function" won't solve the problem, because the problem is at the for() {...}.
> >>
> > I'm sorry, you're right, the kmemdup duplicates the table, not the structure in
> > which the table points to with its .data pointers.  I was looking at it
> > backwards.
> > 
> 
> Hi Neil,
> 
> Should I resend "sctp: fix a problem with net_namespace" series again?
> 
Not sure.  I agree with your assesment on the table adjustments, but I think the
discussion below is holding things up.  You probably want to get to the root
cause of whats causing the lockdep issue before that patch goes in

for 1/2 though
Acked-by: Neil Horman <nhorman@tuxdriver.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2014-02-10 12:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-27  3:49 [PATCH 0/2] sctp: fix a problem with net_namespace Wang Weidong
2014-01-27  3:49 ` [PATCH 1/2] sctp: fix a missed .data initialization Wang Weidong
2014-01-27  3:49 ` [PATCH 2/2] sctp: optimize the sctp_sysctl_net_register Wang Weidong
2014-01-27 11:49 ` [PATCH 0/2] sctp: fix a problem with net_namespace Neil Horman
2014-01-27 13:05   ` Wang Weidong
2014-01-28  8:13   ` Wang Weidong
2014-01-28 11:57     ` Neil Horman
2014-01-28 14:42       ` Wang Weidong
2014-02-10  2:56       ` Wang Weidong
2014-02-10 12:08         ` Neil Horman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).