* linux-next: net->netns_ids is used after calling idr_destroy for it
@ 2016-11-15 6:23 Andrei Vagin
2016-11-15 6:39 ` Andrei Vagin
2016-11-15 18:04 ` linux-next: net->netns_ids is used after calling idr_destroy for it Cong Wang
0 siblings, 2 replies; 22+ messages in thread
From: Andrei Vagin @ 2016-11-15 6:23 UTC (permalink / raw)
To: Nicolas Dichtel, Linux Kernel Network Developers
Hi Nicolas,
cleanup_net() calls idr_destroy(net->netns_ids) for network namespaces
and then it calls unregister_netdevice_many() which calls
idr_alloc(net0>netns_ids). It looks wrong, doesn't it?
I compiled the kernel with the next patch:
diff --git a/lib/idr.c b/lib/idr.c
index 6098336..c0a3a32 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -636,6 +636,8 @@ void idr_destroy(struct idr *idp)
struct idr_layer *p = get_from_free_list(idp);
kmem_cache_free(idr_layer_cache, p);
}
+
+ idp->top = 0xdeaddead;
}
EXPORT_SYMBOL(idr_destroy);
and it crashed as expected:
[ 306.974024] BUG: unable to handle kernel paging request at 00000000deade6bd
[ 306.977724] IP: [<ffffffff8b445085>] _find_next_bit.part.0+0x15/0x70
[ 306.978490] PGD 20dfa067 [ 306.978781] PUD 0
[ 306.979043]
[ 306.979230] Oops: 0000 [#1] SMP
[ 306.979607] Modules linked in: macvlan tun bridge stp llc
nf_conntrack_netlink udp_diag tcp_diag inet_diag netlink_diag
af_packet_diag unix_diag binfmt_misc veth nf_conntrack_ipv4
nf_defrag_ipv4 nf_conntrack_ipv6 nf_defrag_ipv6 xt_conntrack
nf_conntrack nfnetlink ip6table_filter ip6_tables sunrpc ppdev
crc32c_intel joydev virtio_balloon virtio_net i2c_piix4 parport_pc
parport acpi_cpufreq tpm_tis tpm_tis_core tpm virtio_blk serio_raw
virtio_pci ata_generic virtio_ring virtio pata_acpi
[ 306.985236] CPU: 1 PID: 6 Comm: kworker/u4:0 Not tainted 4.9.0-rc5+ #91
[ 306.986005] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.9.1-1.fc24 04/01/2014
[ 306.987024] Workqueue: netns cleanup_net
[ 306.987511] task: ffff8ca63cb5a540 task.stack: ffff9e3240340000
[ 306.988207] RIP: 0010:[<ffffffff8b445085>] [<ffffffff8b445085>]
_find_next_bit.part.0+0x15/0x70
[ 306.989246] RSP: 0018:ffff9e3240343970 EFLAGS: 00010046
[ 306.989871] RAX: ffffffffffffffff RBX: 0000000000000000 RCX: 0000000000000000
[ 306.990713] RDX: 0000000000000000 RSI: 0000000000000100 RDI: 00000000deade6bd
[ 306.991548] RBP: ffff9e3240343980 R08: ffffffffffffffff R09: ffffffffffffffff
[ 306.992383] R10: 00000000f314d32d R11: 0000000000000000 R12: 00000000ffffffff
[ 306.993277] R13: 00000000fffffff8 R14: 00000000deaddead R15: 0000000000000000
[ 306.994117] FS: 0000000000000000(0000) GS:ffff8ca63fd00000(0000)
knlGS:0000000000000000
[ 306.995068] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 306.995744] CR2: 00000000deade6bd CR3: 0000000059aec000 CR4: 00000000000006e0
[ 306.996586] DR0: 00000000000100a0 DR1: 0000000000000000 DR2: 0000000000000000
[ 306.997423] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600
[ 306.998258] Stack:
[ 306.998503] ffff9e3240343980 ffffffff8b44511d ffff9e32403439e0
ffffffff8b42f819
[ 306.999434] 0000000000000000 0208002000000007 ffff8ca6289d80c0
ffff9e32403439f8
[ 307.000365] 0000000000000000 000000007fffffff ffff8ca61f09b200
ffff8ca6289d80c0
[ 307.001296] Call Trace:
[ 307.001594] [<ffffffff8b44511d>] ? find_next_zero_bit+0x1d/0x20
[ 307.002307] [<ffffffff8b42f819>] idr_get_empty_slot+0x189/0x370
[ 307.003012] [<ffffffff8b42fa5e>] idr_alloc+0x5e/0x110
[ 307.003631] [<ffffffff8b70bd88>] ? peernet2id_alloc+0x38/0xb0
[ 307.004321] [<ffffffff8b70ac3d>] __peernet2id_alloc+0x6d/0x90
[ 307.005003] [<ffffffff8b70bda5>] peernet2id_alloc+0x55/0xb0
[ 307.005673] [<ffffffff8b731216>] rtnl_fill_ifinfo+0xaa6/0x10a0
[ 307.006368] [<ffffffff8b112458>] ? rcu_read_lock_sched_held+0x58/0x60
[ 307.007136] [<ffffffff8b6ffe2b>] ? __alloc_skb+0x9b/0x1e0
[ 307.007780] [<ffffffff8b733073>] rtmsg_ifinfo_build_skb+0x73/0xd0
[ 307.008509] [<ffffffff8b7125d5>] rollback_registered_many+0x295/0x390
[ 307.009282] [<ffffffff8b712765>] unregister_netdevice_many+0x25/0x80
[ 307.010047] [<ffffffff8b7138a5>] default_device_exit_batch+0x145/0x170
[ 307.010825] [<ffffffff8b0e7b10>] ? finish_wait+0x70/0x70
[ 307.011465] [<ffffffff8b70ae52>] ops_exit_list.isra.4+0x52/0x60
[ 307.012175] [<ffffffff8b70c17f>] cleanup_net+0x1bf/0x2a0
[ 307.012811] [<ffffffff8b0b616f>] process_one_work+0x1ff/0x660
[ 307.013548] [<ffffffff8b0b60f4>] ? process_one_work+0x184/0x660
[ 307.014259] [<ffffffff8b0b661e>] worker_thread+0x4e/0x480
[ 307.014906] [<ffffffff8b0b65d0>] ? process_one_work+0x660/0x660
[ 307.015617] [<ffffffff8b0bd2a4>] kthread+0xf4/0x110
[ 307.016209] [<ffffffff8b0bd1b0>] ? kthread_park+0x60/0x60
[ 307.016857] [<ffffffff8b872efa>] ret_from_fork+0x2a/0x40
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: linux-next: net->netns_ids is used after calling idr_destroy for it
2016-11-15 6:23 linux-next: net->netns_ids is used after calling idr_destroy for it Andrei Vagin
@ 2016-11-15 6:39 ` Andrei Vagin
2016-11-16 8:41 ` [PATCH net] net: nsid cannot be allocated for a dead netns Nicolas Dichtel
2016-11-15 18:04 ` linux-next: net->netns_ids is used after calling idr_destroy for it Cong Wang
1 sibling, 1 reply; 22+ messages in thread
From: Andrei Vagin @ 2016-11-15 6:39 UTC (permalink / raw)
To: Nicolas Dichtel, Linux Kernel Network Developers
On Mon, Nov 14, 2016 at 10:23 PM, Andrei Vagin <avagin@gmail.com> wrote:
> Hi Nicolas,
>
> cleanup_net() calls idr_destroy(net->netns_ids) for network namespaces
> and then it calls unregister_netdevice_many() which calls
> idr_alloc(net0>netns_ids). It looks wrong, doesn't it?
Here is a report from kmemleak detector:
unreferenced object 0xffff91badb543950 (size 2096):
comm "kworker/u4:0", pid 6, jiffies 4295152553 (age 28.418s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 cb 5f df ba 91 ff ff .........._.....
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffffb1865bea>] kmemleak_alloc+0x4a/0xa0
[<ffffffffb1243b38>] kmem_cache_alloc+0x128/0x280
[<ffffffffb142f5ab>] idr_layer_alloc+0x2b/0x90
[<ffffffffb142f9cd>] idr_get_empty_slot+0x34d/0x370
[<ffffffffb142fa4e>] idr_alloc+0x5e/0x110
[<ffffffffb170ac3d>] __peernet2id_alloc+0x6d/0x90
[<ffffffffb170bda5>] peernet2id_alloc+0x55/0xb0
[<ffffffffb1731216>] rtnl_fill_ifinfo+0xaa6/0x10a0
[<ffffffffb1733073>] rtmsg_ifinfo_build_skb+0x73/0xd0
[<ffffffffb17125d5>] rollback_registered_many+0x295/0x390
[<ffffffffb1712765>] unregister_netdevice_many+0x25/0x80
[<ffffffffb17138a5>] default_device_exit_batch+0x145/0x170
[<ffffffffb170ae52>] ops_exit_list.isra.4+0x52/0x60
[<ffffffffb170c17f>] cleanup_net+0x1bf/0x2a0
[<ffffffffb10b616f>] process_one_work+0x1ff/0x660
[<ffffffffb10b661e>] worker_thread+0x4e/0x480
>
> I compiled the kernel with the next patch:
> diff --git a/lib/idr.c b/lib/idr.c
> index 6098336..c0a3a32 100644
> --- a/lib/idr.c
> +++ b/lib/idr.c
> @@ -636,6 +636,8 @@ void idr_destroy(struct idr *idp)
> struct idr_layer *p = get_from_free_list(idp);
> kmem_cache_free(idr_layer_cache, p);
> }
> +
> + idp->top = 0xdeaddead;
> }
> EXPORT_SYMBOL(idr_destroy);
>
> and it crashed as expected:
>
> [ 306.974024] BUG: unable to handle kernel paging request at 00000000deade6bd
> [ 306.977724] IP: [<ffffffff8b445085>] _find_next_bit.part.0+0x15/0x70
> [ 306.978490] PGD 20dfa067 [ 306.978781] PUD 0
> [ 306.979043]
> [ 306.979230] Oops: 0000 [#1] SMP
> [ 306.979607] Modules linked in: macvlan tun bridge stp llc
> nf_conntrack_netlink udp_diag tcp_diag inet_diag netlink_diag
> af_packet_diag unix_diag binfmt_misc veth nf_conntrack_ipv4
> nf_defrag_ipv4 nf_conntrack_ipv6 nf_defrag_ipv6 xt_conntrack
> nf_conntrack nfnetlink ip6table_filter ip6_tables sunrpc ppdev
> crc32c_intel joydev virtio_balloon virtio_net i2c_piix4 parport_pc
> parport acpi_cpufreq tpm_tis tpm_tis_core tpm virtio_blk serio_raw
> virtio_pci ata_generic virtio_ring virtio pata_acpi
> [ 306.985236] CPU: 1 PID: 6 Comm: kworker/u4:0 Not tainted 4.9.0-rc5+ #91
> [ 306.986005] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS 1.9.1-1.fc24 04/01/2014
> [ 306.987024] Workqueue: netns cleanup_net
> [ 306.987511] task: ffff8ca63cb5a540 task.stack: ffff9e3240340000
> [ 306.988207] RIP: 0010:[<ffffffff8b445085>] [<ffffffff8b445085>]
> _find_next_bit.part.0+0x15/0x70
> [ 306.989246] RSP: 0018:ffff9e3240343970 EFLAGS: 00010046
> [ 306.989871] RAX: ffffffffffffffff RBX: 0000000000000000 RCX: 0000000000000000
> [ 306.990713] RDX: 0000000000000000 RSI: 0000000000000100 RDI: 00000000deade6bd
> [ 306.991548] RBP: ffff9e3240343980 R08: ffffffffffffffff R09: ffffffffffffffff
> [ 306.992383] R10: 00000000f314d32d R11: 0000000000000000 R12: 00000000ffffffff
> [ 306.993277] R13: 00000000fffffff8 R14: 00000000deaddead R15: 0000000000000000
> [ 306.994117] FS: 0000000000000000(0000) GS:ffff8ca63fd00000(0000)
> knlGS:0000000000000000
> [ 306.995068] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 306.995744] CR2: 00000000deade6bd CR3: 0000000059aec000 CR4: 00000000000006e0
> [ 306.996586] DR0: 00000000000100a0 DR1: 0000000000000000 DR2: 0000000000000000
> [ 306.997423] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600
> [ 306.998258] Stack:
> [ 306.998503] ffff9e3240343980 ffffffff8b44511d ffff9e32403439e0
> ffffffff8b42f819
> [ 306.999434] 0000000000000000 0208002000000007 ffff8ca6289d80c0
> ffff9e32403439f8
> [ 307.000365] 0000000000000000 000000007fffffff ffff8ca61f09b200
> ffff8ca6289d80c0
> [ 307.001296] Call Trace:
> [ 307.001594] [<ffffffff8b44511d>] ? find_next_zero_bit+0x1d/0x20
> [ 307.002307] [<ffffffff8b42f819>] idr_get_empty_slot+0x189/0x370
> [ 307.003012] [<ffffffff8b42fa5e>] idr_alloc+0x5e/0x110
> [ 307.003631] [<ffffffff8b70bd88>] ? peernet2id_alloc+0x38/0xb0
> [ 307.004321] [<ffffffff8b70ac3d>] __peernet2id_alloc+0x6d/0x90
> [ 307.005003] [<ffffffff8b70bda5>] peernet2id_alloc+0x55/0xb0
> [ 307.005673] [<ffffffff8b731216>] rtnl_fill_ifinfo+0xaa6/0x10a0
> [ 307.006368] [<ffffffff8b112458>] ? rcu_read_lock_sched_held+0x58/0x60
> [ 307.007136] [<ffffffff8b6ffe2b>] ? __alloc_skb+0x9b/0x1e0
> [ 307.007780] [<ffffffff8b733073>] rtmsg_ifinfo_build_skb+0x73/0xd0
> [ 307.008509] [<ffffffff8b7125d5>] rollback_registered_many+0x295/0x390
> [ 307.009282] [<ffffffff8b712765>] unregister_netdevice_many+0x25/0x80
> [ 307.010047] [<ffffffff8b7138a5>] default_device_exit_batch+0x145/0x170
> [ 307.010825] [<ffffffff8b0e7b10>] ? finish_wait+0x70/0x70
> [ 307.011465] [<ffffffff8b70ae52>] ops_exit_list.isra.4+0x52/0x60
> [ 307.012175] [<ffffffff8b70c17f>] cleanup_net+0x1bf/0x2a0
> [ 307.012811] [<ffffffff8b0b616f>] process_one_work+0x1ff/0x660
> [ 307.013548] [<ffffffff8b0b60f4>] ? process_one_work+0x184/0x660
> [ 307.014259] [<ffffffff8b0b661e>] worker_thread+0x4e/0x480
> [ 307.014906] [<ffffffff8b0b65d0>] ? process_one_work+0x660/0x660
> [ 307.015617] [<ffffffff8b0bd2a4>] kthread+0xf4/0x110
> [ 307.016209] [<ffffffff8b0bd1b0>] ? kthread_park+0x60/0x60
> [ 307.016857] [<ffffffff8b872efa>] ret_from_fork+0x2a/0x40
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: linux-next: net->netns_ids is used after calling idr_destroy for it
2016-11-15 6:23 linux-next: net->netns_ids is used after calling idr_destroy for it Andrei Vagin
2016-11-15 6:39 ` Andrei Vagin
@ 2016-11-15 18:04 ` Cong Wang
2016-11-15 18:50 ` Cong Wang
1 sibling, 1 reply; 22+ messages in thread
From: Cong Wang @ 2016-11-15 18:04 UTC (permalink / raw)
To: Andrei Vagin; +Cc: Nicolas Dichtel, Linux Kernel Network Developers
On Mon, Nov 14, 2016 at 10:23 PM, Andrei Vagin <avagin@gmail.com> wrote:
> Hi Nicolas,
>
> cleanup_net() calls idr_destroy(net->netns_ids) for network namespaces
> and then it calls unregister_netdevice_many() which calls
> idr_alloc(net0>netns_ids). It looks wrong, doesn't it?
>
netns id is designed to allocate lazily, but yeah it makes no sense
to allocate id for the netns being destroyed, not to mention idr is freed.
I will send a patch.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: linux-next: net->netns_ids is used after calling idr_destroy for it
2016-11-15 18:04 ` linux-next: net->netns_ids is used after calling idr_destroy for it Cong Wang
@ 2016-11-15 18:50 ` Cong Wang
2016-11-15 20:48 ` Andrei Vagin
0 siblings, 1 reply; 22+ messages in thread
From: Cong Wang @ 2016-11-15 18:50 UTC (permalink / raw)
To: Andrei Vagin; +Cc: Nicolas Dichtel, Linux Kernel Network Developers
[-- Attachment #1: Type: text/plain, Size: 637 bytes --]
On Tue, Nov 15, 2016 at 10:04 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Nov 14, 2016 at 10:23 PM, Andrei Vagin <avagin@gmail.com> wrote:
>> Hi Nicolas,
>>
>> cleanup_net() calls idr_destroy(net->netns_ids) for network namespaces
>> and then it calls unregister_netdevice_many() which calls
>> idr_alloc(net0>netns_ids). It looks wrong, doesn't it?
>>
>
> netns id is designed to allocate lazily, but yeah it makes no sense
> to allocate id for the netns being destroyed, not to mention idr is freed.
>
> I will send a patch.
Could you try the attached patch? I just did some quick netns creation/destroy
tests.
Thanks!
[-- Attachment #2: unregister-netdevice.diff --]
[-- Type: text/plain, Size: 2296 bytes --]
diff --git a/net/core/dev.c b/net/core/dev.c
index 6deba68..f9a2969 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6685,7 +6685,7 @@ static void net_set_todo(struct net_device *dev)
dev_net(dev)->dev_unreg_count++;
}
-static void rollback_registered_many(struct list_head *head)
+static void rollback_registered_many(struct list_head *head, bool send_rtmsg)
{
struct net_device *dev, *tmp;
LIST_HEAD(close_head);
@@ -6737,8 +6737,8 @@ static void rollback_registered_many(struct list_head *head)
*/
call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
- if (!dev->rtnl_link_ops ||
- dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
+ if ((!dev->rtnl_link_ops ||
+ dev->rtnl_link_state == RTNL_LINK_INITIALIZED) && send_rtmsg)
skb = rtmsg_ifinfo_build_skb(RTM_DELLINK, dev, ~0U,
GFP_KERNEL);
@@ -6777,7 +6777,7 @@ static void rollback_registered(struct net_device *dev)
LIST_HEAD(single);
list_add(&dev->unreg_list, &single);
- rollback_registered_many(&single);
+ rollback_registered_many(&single, true);
list_del(&single);
}
@@ -7769,6 +7769,18 @@ void unregister_netdevice_queue(struct net_device *dev, struct list_head *head)
}
EXPORT_SYMBOL(unregister_netdevice_queue);
+static void __unregister_netdevice_many(struct list_head *head, bool send_rtmsg)
+{
+ struct net_device *dev;
+
+ if (!list_empty(head)) {
+ rollback_registered_many(head, send_rtmsg);
+ list_for_each_entry(dev, head, unreg_list)
+ net_set_todo(dev);
+ list_del(head);
+ }
+}
+
/**
* unregister_netdevice_many - unregister many devices
* @head: list of devices
@@ -7778,14 +7790,7 @@ EXPORT_SYMBOL(unregister_netdevice_queue);
*/
void unregister_netdevice_many(struct list_head *head)
{
- struct net_device *dev;
-
- if (!list_empty(head)) {
- rollback_registered_many(head);
- list_for_each_entry(dev, head, unreg_list)
- net_set_todo(dev);
- list_del(head);
- }
+ __unregister_netdevice_many(head, true);
}
EXPORT_SYMBOL(unregister_netdevice_many);
@@ -8239,7 +8244,7 @@ static void __net_exit default_device_exit_batch(struct list_head *net_list)
unregister_netdevice_queue(dev, &dev_kill_list);
}
}
- unregister_netdevice_many(&dev_kill_list);
+ __unregister_netdevice_many(&dev_kill_list, false);
rtnl_unlock();
}
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: linux-next: net->netns_ids is used after calling idr_destroy for it
2016-11-15 18:50 ` Cong Wang
@ 2016-11-15 20:48 ` Andrei Vagin
2016-11-15 21:07 ` Cong Wang
0 siblings, 1 reply; 22+ messages in thread
From: Andrei Vagin @ 2016-11-15 20:48 UTC (permalink / raw)
To: Cong Wang; +Cc: Nicolas Dichtel, Linux Kernel Network Developers
On Tue, Nov 15, 2016 at 10:50 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Nov 15, 2016 at 10:04 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Mon, Nov 14, 2016 at 10:23 PM, Andrei Vagin <avagin@gmail.com> wrote:
>>> Hi Nicolas,
>>>
>>> cleanup_net() calls idr_destroy(net->netns_ids) for network namespaces
>>> and then it calls unregister_netdevice_many() which calls
>>> idr_alloc(net0>netns_ids). It looks wrong, doesn't it?
>>>
>>
>> netns id is designed to allocate lazily, but yeah it makes no sense
>> to allocate id for the netns being destroyed, not to mention idr is freed.
>>
>> I will send a patch.
>
> Could you try the attached patch? I just did some quick netns creation/destroy
> tests.
Here is another fail:
unreferenced object 0xffff94153912a0c0 (size 2096):
comm "ip", pid 29175, jiffies 4294954213 (age 137.624s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 b2 3b 1d 15 94 ff ff ..........;.....
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffffac865c1a>] kmemleak_alloc+0x4a/0xa0
[<ffffffffac243b38>] kmem_cache_alloc+0x128/0x280
[<ffffffffac42f5ab>] idr_layer_alloc+0x2b/0x90
[<ffffffffac42f9cd>] idr_get_empty_slot+0x34d/0x370
[<ffffffffac42fa4e>] idr_alloc+0x5e/0x110
[<ffffffffac70ac3d>] __peernet2id_alloc+0x6d/0x90
[<ffffffffac70bda5>] peernet2id_alloc+0x55/0xb0
[<ffffffffac731246>] rtnl_fill_ifinfo+0xaa6/0x10a0
[<ffffffffac7330a3>] rtmsg_ifinfo_build_skb+0x73/0xd0
[<ffffffffac7125e1>] rollback_registered_many+0x2a1/0x3a0
[<ffffffffac712779>] __unregister_netdevice_many+0x29/0x80
[<ffffffffac7127e3>] unregister_netdevice_many+0x13/0x20
[<ffffffffc02dc4ce>] macvlan_device_event+0x13e/0x235 [macvlan]
[<ffffffffac0bef2a>] notifier_call_chain+0x4a/0x70
[<ffffffffac0bf066>] raw_notifier_call_chain+0x16/0x20
[<ffffffffac710205>] call_netdevice_notifiers_info+0x35/0x60
What do you think about calling idr_destroy() at the final step in
cleanup_net()? In this case we can avoid this sort of problems in a
future.
@@ -422,21 +425,6 @@ static void cleanup_net(struct work_struct *work)
list_for_each_entry(net, &net_kill_list, cleanup_list) {
list_del_rcu(&net->list);
list_add_tail(&net->exit_list, &net_exit_list);
- for_each_net(tmp) {
- int id;
-
- spin_lock_irq(&tmp->nsid_lock);
- id = __peernet2id(tmp, net);
- if (id >= 0)
- idr_remove(&tmp->netns_ids, id);
- spin_unlock_irq(&tmp->nsid_lock);
- if (id >= 0)
- rtnl_net_notifyid(tmp, RTM_DELNSID, id);
- }
- spin_lock_irq(&net->nsid_lock);
- idr_destroy(&net->netns_ids);
- spin_unlock_irq(&net->nsid_lock);
-
}
rtnl_unlock();
@@ -464,6 +452,22 @@ static void cleanup_net(struct work_struct *work)
/* Finally it is safe to free my network namespace structure */
list_for_each_entry_safe(net, tmp, &net_exit_list, exit_list) {
+ rtnl_lock();
+ for_each_net(tmp) {
+ int id;
+
+ spin_lock_irq(&tmp->nsid_lock);
+ id = __peernet2id(tmp, net);
+ if (id >= 0)
+ idr_remove(&tmp->netns_ids, id);
+ spin_unlock_irq(&tmp->nsid_lock);
+ if (id >= 0)
+ rtnl_net_notifyid(tmp, RTM_DELNSID, id);
+ }
+ rtnl_unlock();
+
+ idr_destroy(&net->netns_ids);
+
list_del_init(&net->exit_list);
dec_net_namespaces(net->ucounts);
put_user_ns(net->user_ns);
>
> Thanks!
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: linux-next: net->netns_ids is used after calling idr_destroy for it
2016-11-15 20:48 ` Andrei Vagin
@ 2016-11-15 21:07 ` Cong Wang
2016-11-15 22:21 ` Andrei Vagin
0 siblings, 1 reply; 22+ messages in thread
From: Cong Wang @ 2016-11-15 21:07 UTC (permalink / raw)
To: Andrei Vagin; +Cc: Nicolas Dichtel, Linux Kernel Network Developers
On Tue, Nov 15, 2016 at 12:48 PM, Andrei Vagin <avagin@gmail.com> wrote:
> On Tue, Nov 15, 2016 at 10:50 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Tue, Nov 15, 2016 at 10:04 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> On Mon, Nov 14, 2016 at 10:23 PM, Andrei Vagin <avagin@gmail.com> wrote:
>>>> Hi Nicolas,
>>>>
>>>> cleanup_net() calls idr_destroy(net->netns_ids) for network namespaces
>>>> and then it calls unregister_netdevice_many() which calls
>>>> idr_alloc(net0>netns_ids). It looks wrong, doesn't it?
>>>>
>>>
>>> netns id is designed to allocate lazily, but yeah it makes no sense
>>> to allocate id for the netns being destroyed, not to mention idr is freed.
>>>
>>> I will send a patch.
>>
>> Could you try the attached patch? I just did some quick netns creation/destroy
>> tests.
>
> Here is another fail:
>
> unreferenced object 0xffff94153912a0c0 (size 2096):
> comm "ip", pid 29175, jiffies 4294954213 (age 137.624s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 b2 3b 1d 15 94 ff ff ..........;.....
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffffac865c1a>] kmemleak_alloc+0x4a/0xa0
> [<ffffffffac243b38>] kmem_cache_alloc+0x128/0x280
> [<ffffffffac42f5ab>] idr_layer_alloc+0x2b/0x90
> [<ffffffffac42f9cd>] idr_get_empty_slot+0x34d/0x370
> [<ffffffffac42fa4e>] idr_alloc+0x5e/0x110
> [<ffffffffac70ac3d>] __peernet2id_alloc+0x6d/0x90
> [<ffffffffac70bda5>] peernet2id_alloc+0x55/0xb0
> [<ffffffffac731246>] rtnl_fill_ifinfo+0xaa6/0x10a0
> [<ffffffffac7330a3>] rtmsg_ifinfo_build_skb+0x73/0xd0
> [<ffffffffac7125e1>] rollback_registered_many+0x2a1/0x3a0
> [<ffffffffac712779>] __unregister_netdevice_many+0x29/0x80
> [<ffffffffac7127e3>] unregister_netdevice_many+0x13/0x20
> [<ffffffffc02dc4ce>] macvlan_device_event+0x13e/0x235 [macvlan]
> [<ffffffffac0bef2a>] notifier_call_chain+0x4a/0x70
> [<ffffffffac0bf066>] raw_notifier_call_chain+0x16/0x20
> [<ffffffffac710205>] call_netdevice_notifiers_info+0x35/0x60
>
Oh, drivers send rtmsg in notifiers too, hmm.
>
> What do you think about calling idr_destroy() at the final step in
> cleanup_net()? In this case we can avoid this sort of problems in a
> future.
This was my first idea too, but it looks more risky than my approach.
Also, rtmsg is really not needed because the netns is being destroyed,
no one cares about it here.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: linux-next: net->netns_ids is used after calling idr_destroy for it
2016-11-15 21:07 ` Cong Wang
@ 2016-11-15 22:21 ` Andrei Vagin
2016-11-15 22:45 ` Andrei Vagin
0 siblings, 1 reply; 22+ messages in thread
From: Andrei Vagin @ 2016-11-15 22:21 UTC (permalink / raw)
To: Cong Wang; +Cc: Nicolas Dichtel, Linux Kernel Network Developers
On Tue, Nov 15, 2016 at 1:07 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Nov 15, 2016 at 12:48 PM, Andrei Vagin <avagin@gmail.com> wrote:
>> On Tue, Nov 15, 2016 at 10:50 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> On Tue, Nov 15, 2016 at 10:04 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>> On Mon, Nov 14, 2016 at 10:23 PM, Andrei Vagin <avagin@gmail.com> wrote:
>>>>> Hi Nicolas,
>>>>>
>>>>> cleanup_net() calls idr_destroy(net->netns_ids) for network namespaces
>>>>> and then it calls unregister_netdevice_many() which calls
>>>>> idr_alloc(net0>netns_ids). It looks wrong, doesn't it?
>>>>>
>>>>
>>>> netns id is designed to allocate lazily, but yeah it makes no sense
>>>> to allocate id for the netns being destroyed, not to mention idr is freed.
>>>>
>>>> I will send a patch.
>>>
>>> Could you try the attached patch? I just did some quick netns creation/destroy
>>> tests.
>>
>> Here is another fail:
>>
>> unreferenced object 0xffff94153912a0c0 (size 2096):
>> comm "ip", pid 29175, jiffies 4294954213 (age 137.624s)
>> hex dump (first 32 bytes):
>> 00 00 00 00 00 00 00 00 00 b2 3b 1d 15 94 ff ff ..........;.....
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> backtrace:
>> [<ffffffffac865c1a>] kmemleak_alloc+0x4a/0xa0
>> [<ffffffffac243b38>] kmem_cache_alloc+0x128/0x280
>> [<ffffffffac42f5ab>] idr_layer_alloc+0x2b/0x90
>> [<ffffffffac42f9cd>] idr_get_empty_slot+0x34d/0x370
>> [<ffffffffac42fa4e>] idr_alloc+0x5e/0x110
>> [<ffffffffac70ac3d>] __peernet2id_alloc+0x6d/0x90
>> [<ffffffffac70bda5>] peernet2id_alloc+0x55/0xb0
>> [<ffffffffac731246>] rtnl_fill_ifinfo+0xaa6/0x10a0
>> [<ffffffffac7330a3>] rtmsg_ifinfo_build_skb+0x73/0xd0
>> [<ffffffffac7125e1>] rollback_registered_many+0x2a1/0x3a0
>> [<ffffffffac712779>] __unregister_netdevice_many+0x29/0x80
>> [<ffffffffac7127e3>] unregister_netdevice_many+0x13/0x20
>> [<ffffffffc02dc4ce>] macvlan_device_event+0x13e/0x235 [macvlan]
>> [<ffffffffac0bef2a>] notifier_call_chain+0x4a/0x70
>> [<ffffffffac0bf066>] raw_notifier_call_chain+0x16/0x20
>> [<ffffffffac710205>] call_netdevice_notifiers_info+0x35/0x60
>>
>
> Oh, drivers send rtmsg in notifiers too, hmm.
>
>>
>> What do you think about calling idr_destroy() at the final step in
>> cleanup_net()? In this case we can avoid this sort of problems in a
>> future.
>
> This was my first idea too, but it looks more risky than my approach.
>
> Also, rtmsg is really not needed because the netns is being destroyed,
> no one cares about it here.
I would like to agree with you here, but looks like sockets with
NETLINK_F_LISTEN_ALL_NSID are able to catch these messages.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: linux-next: net->netns_ids is used after calling idr_destroy for it
2016-11-15 22:21 ` Andrei Vagin
@ 2016-11-15 22:45 ` Andrei Vagin
2016-11-16 18:04 ` Cong Wang
0 siblings, 1 reply; 22+ messages in thread
From: Andrei Vagin @ 2016-11-15 22:45 UTC (permalink / raw)
To: Cong Wang; +Cc: Nicolas Dichtel, Linux Kernel Network Developers
On Tue, Nov 15, 2016 at 2:21 PM, Andrei Vagin <avagin@gmail.com> wrote:
> On Tue, Nov 15, 2016 at 1:07 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Tue, Nov 15, 2016 at 12:48 PM, Andrei Vagin <avagin@gmail.com> wrote:
>>> On Tue, Nov 15, 2016 at 10:50 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>> On Tue, Nov 15, 2016 at 10:04 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>>> On Mon, Nov 14, 2016 at 10:23 PM, Andrei Vagin <avagin@gmail.com> wrote:
>>>>>> Hi Nicolas,
>>>>>>
>>>>>> cleanup_net() calls idr_destroy(net->netns_ids) for network namespaces
>>>>>> and then it calls unregister_netdevice_many() which calls
>>>>>> idr_alloc(net0>netns_ids). It looks wrong, doesn't it?
>>>>>>
>>>>>
>>>>> netns id is designed to allocate lazily, but yeah it makes no sense
>>>>> to allocate id for the netns being destroyed, not to mention idr is freed.
>>>>>
>>>>> I will send a patch.
>>>>
>>>> Could you try the attached patch? I just did some quick netns creation/destroy
>>>> tests.
>>>
>>> Here is another fail:
>>>
>>> unreferenced object 0xffff94153912a0c0 (size 2096):
>>> comm "ip", pid 29175, jiffies 4294954213 (age 137.624s)
>>> hex dump (first 32 bytes):
>>> 00 00 00 00 00 00 00 00 00 b2 3b 1d 15 94 ff ff ..........;.....
>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>>> backtrace:
>>> [<ffffffffac865c1a>] kmemleak_alloc+0x4a/0xa0
>>> [<ffffffffac243b38>] kmem_cache_alloc+0x128/0x280
>>> [<ffffffffac42f5ab>] idr_layer_alloc+0x2b/0x90
>>> [<ffffffffac42f9cd>] idr_get_empty_slot+0x34d/0x370
>>> [<ffffffffac42fa4e>] idr_alloc+0x5e/0x110
>>> [<ffffffffac70ac3d>] __peernet2id_alloc+0x6d/0x90
>>> [<ffffffffac70bda5>] peernet2id_alloc+0x55/0xb0
>>> [<ffffffffac731246>] rtnl_fill_ifinfo+0xaa6/0x10a0
>>> [<ffffffffac7330a3>] rtmsg_ifinfo_build_skb+0x73/0xd0
>>> [<ffffffffac7125e1>] rollback_registered_many+0x2a1/0x3a0
>>> [<ffffffffac712779>] __unregister_netdevice_many+0x29/0x80
>>> [<ffffffffac7127e3>] unregister_netdevice_many+0x13/0x20
>>> [<ffffffffc02dc4ce>] macvlan_device_event+0x13e/0x235 [macvlan]
>>> [<ffffffffac0bef2a>] notifier_call_chain+0x4a/0x70
>>> [<ffffffffac0bf066>] raw_notifier_call_chain+0x16/0x20
>>> [<ffffffffac710205>] call_netdevice_notifiers_info+0x35/0x60
>>>
>>
>> Oh, drivers send rtmsg in notifiers too, hmm.
>>
>>>
>>> What do you think about calling idr_destroy() at the final step in
>>> cleanup_net()? In this case we can avoid this sort of problems in a
>>> future.
>>
>> This was my first idea too, but it looks more risky than my approach.
>>
>> Also, rtmsg is really not needed because the netns is being destroyed,
>> no one cares about it here.
>
> I would like to agree with you here, but looks like sockets with
> NETLINK_F_LISTEN_ALL_NSID are able to catch these messages.
Actually I found that I was wrong.
do_one_broadcast() sends a notification only if a device network
namespace has an id in a socket netns. But cleanup_net() removes all
id-s to a target namespace, so just ignore my last comment.
Thanks,
Andrei
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net] net: nsid cannot be allocated for a dead netns
2016-11-15 6:39 ` Andrei Vagin
@ 2016-11-16 8:41 ` Nicolas Dichtel
2016-11-16 8:42 ` Nicolas Dichtel
` (3 more replies)
0 siblings, 4 replies; 22+ messages in thread
From: Nicolas Dichtel @ 2016-11-16 8:41 UTC (permalink / raw)
To: avagin; +Cc: davem, netdev, xiyou.wangcong, Nicolas Dichtel
Andrei reports the following kmemleak error:
unreferenced object 0xffff91badb543950 (size 2096):
comm "kworker/u4:0", pid 6, jiffies 4295152553 (age 28.418s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 cb 5f df ba 91 ff ff .........._.....
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffffb1865bea>] kmemleak_alloc+0x4a/0xa0
[<ffffffffb1243b38>] kmem_cache_alloc+0x128/0x280
[<ffffffffb142f5ab>] idr_layer_alloc+0x2b/0x90
[<ffffffffb142f9cd>] idr_get_empty_slot+0x34d/0x370
[<ffffffffb142fa4e>] idr_alloc+0x5e/0x110
[<ffffffffb170ac3d>] __peernet2id_alloc+0x6d/0x90
[<ffffffffb170bda5>] peernet2id_alloc+0x55/0xb0
[<ffffffffb1731216>] rtnl_fill_ifinfo+0xaa6/0x10a0
[<ffffffffb1733073>] rtmsg_ifinfo_build_skb+0x73/0xd0
[<ffffffffb17125d5>] rollback_registered_many+0x295/0x390
[<ffffffffb1712765>] unregister_netdevice_many+0x25/0x80
[<ffffffffb17138a5>] default_device_exit_batch+0x145/0x170
[<ffffffffb170ae52>] ops_exit_list.isra.4+0x52/0x60
[<ffffffffb170c17f>] cleanup_net+0x1bf/0x2a0
[<ffffffffb10b616f>] process_one_work+0x1ff/0x660
[<ffffffffb10b661e>] worker_thread+0x4e/0x480
There is no reason to try to allocate an nsid for a netns which is dying.
Reported-by: Andrei Vagin <avagin@gmail.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
Thank you for the report. Can you test this patch?
Regards,
Nicolas
net/core/net_namespace.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index f61c0e02a413..f1340ed0d8df 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -159,6 +159,9 @@ static int alloc_netid(struct net *net, struct net *peer, int reqid)
max = reqid + 1;
}
+ if (!atomic_read(&net->count) || !&atomic_read(peer->count))
+ return -EINVAL;
+
return idr_alloc(&net->netns_ids, peer, min, max, GFP_ATOMIC);
}
--
2.8.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH net] net: nsid cannot be allocated for a dead netns
2016-11-16 8:41 ` [PATCH net] net: nsid cannot be allocated for a dead netns Nicolas Dichtel
@ 2016-11-16 8:42 ` Nicolas Dichtel
2016-11-16 9:10 ` kbuild test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 22+ messages in thread
From: Nicolas Dichtel @ 2016-11-16 8:42 UTC (permalink / raw)
To: avagin; +Cc: davem, netdev, xiyou.wangcong
Le 16/11/2016 à 09:41, Nicolas Dichtel a écrit :
> Andrei reports the following kmemleak error:
> unreferenced object 0xffff91badb543950 (size 2096):
> comm "kworker/u4:0", pid 6, jiffies 4295152553 (age 28.418s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 cb 5f df ba 91 ff ff .........._.....
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffffb1865bea>] kmemleak_alloc+0x4a/0xa0
> [<ffffffffb1243b38>] kmem_cache_alloc+0x128/0x280
> [<ffffffffb142f5ab>] idr_layer_alloc+0x2b/0x90
> [<ffffffffb142f9cd>] idr_get_empty_slot+0x34d/0x370
> [<ffffffffb142fa4e>] idr_alloc+0x5e/0x110
> [<ffffffffb170ac3d>] __peernet2id_alloc+0x6d/0x90
> [<ffffffffb170bda5>] peernet2id_alloc+0x55/0xb0
> [<ffffffffb1731216>] rtnl_fill_ifinfo+0xaa6/0x10a0
> [<ffffffffb1733073>] rtmsg_ifinfo_build_skb+0x73/0xd0
> [<ffffffffb17125d5>] rollback_registered_many+0x295/0x390
> [<ffffffffb1712765>] unregister_netdevice_many+0x25/0x80
> [<ffffffffb17138a5>] default_device_exit_batch+0x145/0x170
> [<ffffffffb170ae52>] ops_exit_list.isra.4+0x52/0x60
> [<ffffffffb170c17f>] cleanup_net+0x1bf/0x2a0
> [<ffffffffb10b616f>] process_one_work+0x1ff/0x660
> [<ffffffffb10b661e>] worker_thread+0x4e/0x480
>
> There is no reason to try to allocate an nsid for a netns which is dying.
>
> Reported-by: Andrei Vagin <avagin@gmail.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
'Fixes' tag is missing :/
Fixes: 0c7aecd4bde4 ("netns: add rtnl cmd to add and get peer netns ids")
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net] net: nsid cannot be allocated for a dead netns
2016-11-16 8:41 ` [PATCH net] net: nsid cannot be allocated for a dead netns Nicolas Dichtel
2016-11-16 8:42 ` Nicolas Dichtel
@ 2016-11-16 9:10 ` kbuild test robot
2016-11-16 9:45 ` Nicolas Dichtel
2016-11-16 9:49 ` [PATCH net v2] " Nicolas Dichtel
2016-11-16 10:23 ` [PATCH net] " kbuild test robot
2016-11-16 12:00 ` Sergei Shtylyov
3 siblings, 2 replies; 22+ messages in thread
From: kbuild test robot @ 2016-11-16 9:10 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: kbuild-all, avagin, davem, netdev, xiyou.wangcong, Nicolas Dichtel
[-- Attachment #1: Type: text/plain, Size: 11624 bytes --]
Hi Nicolas,
[auto build test ERROR on net/master]
url: https://github.com/0day-ci/linux/commits/Nicolas-Dichtel/net-nsid-cannot-be-allocated-for-a-dead-netns/20161116-164739
config: i386-randconfig-x006-201646 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All error/warnings (new ones prefixed by >>):
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/list.h:4,
from include/linux/timer.h:4,
from include/linux/workqueue.h:8,
from net/core/net_namespace.c:3:
net/core/net_namespace.c: In function 'alloc_netid':
>> net/core/net_namespace.c:162:49: error: incompatible type for argument 1 of 'atomic_read'
if (!atomic_read(&net->count) || !&atomic_read(peer->count))
^
include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> net/core/net_namespace.c:162:2: note: in expansion of macro 'if'
if (!atomic_read(&net->count) || !&atomic_read(peer->count))
^~
In file included from arch/x86/include/asm/msr.h:66:0,
from arch/x86/include/asm/processor.h:20,
from arch/x86/include/asm/cpufeature.h:4,
from arch/x86/include/asm/thread_info.h:52,
from include/linux/thread_info.h:58,
from arch/x86/include/asm/preempt.h:6,
from include/linux/preempt.h:59,
from include/linux/spinlock.h:50,
from include/linux/seqlock.h:35,
from include/linux/time.h:5,
from include/linux/ktime.h:24,
from include/linux/timer.h:5,
from include/linux/workqueue.h:8,
from net/core/net_namespace.c:3:
arch/x86/include/asm/atomic.h:24:28: note: expected 'const atomic_t * {aka const struct <anonymous> *}' but argument is of type 'atomic_t {aka struct <anonymous>}'
static __always_inline int atomic_read(const atomic_t *v)
^~~~~~~~~~~
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/list.h:4,
from include/linux/timer.h:4,
from include/linux/workqueue.h:8,
from net/core/net_namespace.c:3:
>> net/core/net_namespace.c:162:49: error: incompatible type for argument 1 of 'atomic_read'
if (!atomic_read(&net->count) || !&atomic_read(peer->count))
^
include/linux/compiler.h:149:42: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> net/core/net_namespace.c:162:2: note: in expansion of macro 'if'
if (!atomic_read(&net->count) || !&atomic_read(peer->count))
^~
In file included from arch/x86/include/asm/msr.h:66:0,
from arch/x86/include/asm/processor.h:20,
from arch/x86/include/asm/cpufeature.h:4,
from arch/x86/include/asm/thread_info.h:52,
from include/linux/thread_info.h:58,
from arch/x86/include/asm/preempt.h:6,
from include/linux/preempt.h:59,
from include/linux/spinlock.h:50,
from include/linux/seqlock.h:35,
from include/linux/time.h:5,
from include/linux/ktime.h:24,
from include/linux/timer.h:5,
from include/linux/workqueue.h:8,
from net/core/net_namespace.c:3:
arch/x86/include/asm/atomic.h:24:28: note: expected 'const atomic_t * {aka const struct <anonymous> *}' but argument is of type 'atomic_t {aka struct <anonymous>}'
static __always_inline int atomic_read(const atomic_t *v)
^~~~~~~~~~~
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/list.h:4,
from include/linux/timer.h:4,
from include/linux/workqueue.h:8,
from net/core/net_namespace.c:3:
>> net/core/net_namespace.c:162:49: error: incompatible type for argument 1 of 'atomic_read'
if (!atomic_read(&net->count) || !&atomic_read(peer->count))
^
include/linux/compiler.h:160:16: note: in definition of macro '__trace_if'
______r = !!(cond); \
^~~~
>> net/core/net_namespace.c:162:2: note: in expansion of macro 'if'
if (!atomic_read(&net->count) || !&atomic_read(peer->count))
^~
In file included from arch/x86/include/asm/msr.h:66:0,
from arch/x86/include/asm/processor.h:20,
from arch/x86/include/asm/cpufeature.h:4,
from arch/x86/include/asm/thread_info.h:52,
from include/linux/thread_info.h:58,
from arch/x86/include/asm/preempt.h:6,
from include/linux/preempt.h:59,
from include/linux/spinlock.h:50,
from include/linux/seqlock.h:35,
from include/linux/time.h:5,
from include/linux/ktime.h:24,
from include/linux/timer.h:5,
from include/linux/workqueue.h:8,
from net/core/net_namespace.c:3:
arch/x86/include/asm/atomic.h:24:28: note: expected 'const atomic_t * {aka const struct <anonymous> *}' but argument is of type 'atomic_t {aka struct <anonymous>}'
static __always_inline int atomic_read(const atomic_t *v)
^~~~~~~~~~~
vim +/atomic_read +162 net/core/net_namespace.c
1 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
2
> 3 #include <linux/workqueue.h>
4 #include <linux/rtnetlink.h>
5 #include <linux/cache.h>
6 #include <linux/slab.h>
7 #include <linux/list.h>
8 #include <linux/delay.h>
9 #include <linux/sched.h>
10 #include <linux/idr.h>
11 #include <linux/rculist.h>
12 #include <linux/nsproxy.h>
13 #include <linux/fs.h>
14 #include <linux/proc_ns.h>
15 #include <linux/file.h>
16 #include <linux/export.h>
17 #include <linux/user_namespace.h>
18 #include <linux/net_namespace.h>
19 #include <net/sock.h>
20 #include <net/netlink.h>
21 #include <net/net_namespace.h>
22 #include <net/netns/generic.h>
23
24 /*
25 * Our network namespace constructor/destructor lists
26 */
27
28 static LIST_HEAD(pernet_list);
29 static struct list_head *first_device = &pernet_list;
30 DEFINE_MUTEX(net_mutex);
31
32 LIST_HEAD(net_namespace_list);
33 EXPORT_SYMBOL_GPL(net_namespace_list);
34
35 struct net init_net = {
36 .dev_base_head = LIST_HEAD_INIT(init_net.dev_base_head),
37 };
38 EXPORT_SYMBOL(init_net);
39
40 static bool init_net_initialized;
41
42 #define INITIAL_NET_GEN_PTRS 13 /* +1 for len +2 for rcu_head */
43
44 static unsigned int max_gen_ptrs = INITIAL_NET_GEN_PTRS;
45
46 static struct net_generic *net_alloc_generic(void)
47 {
48 struct net_generic *ng;
49 size_t generic_size = offsetof(struct net_generic, ptr[max_gen_ptrs]);
50
51 ng = kzalloc(generic_size, GFP_KERNEL);
52 if (ng)
53 ng->len = max_gen_ptrs;
54
55 return ng;
56 }
57
58 static int net_assign_generic(struct net *net, int id, void *data)
59 {
60 struct net_generic *ng, *old_ng;
61
62 BUG_ON(!mutex_is_locked(&net_mutex));
63 BUG_ON(id == 0);
64
65 old_ng = rcu_dereference_protected(net->gen,
66 lockdep_is_held(&net_mutex));
67 ng = old_ng;
68 if (old_ng->len >= id)
69 goto assign;
70
71 ng = net_alloc_generic();
72 if (ng == NULL)
73 return -ENOMEM;
74
75 /*
76 * Some synchronisation notes:
77 *
78 * The net_generic explores the net->gen array inside rcu
79 * read section. Besides once set the net->gen->ptr[x]
80 * pointer never changes (see rules in netns/generic.h).
81 *
82 * That said, we simply duplicate this array and schedule
83 * the old copy for kfree after a grace period.
84 */
85
86 memcpy(&ng->ptr, &old_ng->ptr, old_ng->len * sizeof(void*));
87
88 rcu_assign_pointer(net->gen, ng);
89 kfree_rcu(old_ng, rcu);
90 assign:
91 ng->ptr[id - 1] = data;
92 return 0;
93 }
94
95 static int ops_init(const struct pernet_operations *ops, struct net *net)
96 {
97 int err = -ENOMEM;
98 void *data = NULL;
99
100 if (ops->id && ops->size) {
101 data = kzalloc(ops->size, GFP_KERNEL);
102 if (!data)
103 goto out;
104
105 err = net_assign_generic(net, *ops->id, data);
106 if (err)
107 goto cleanup;
108 }
109 err = 0;
110 if (ops->init)
111 err = ops->init(net);
112 if (!err)
113 return 0;
114
115 cleanup:
116 kfree(data);
117
118 out:
119 return err;
120 }
121
122 static void ops_free(const struct pernet_operations *ops, struct net *net)
123 {
124 if (ops->id && ops->size) {
125 int id = *ops->id;
126 kfree(net_generic(net, id));
127 }
128 }
129
130 static void ops_exit_list(const struct pernet_operations *ops,
131 struct list_head *net_exit_list)
132 {
133 struct net *net;
134 if (ops->exit) {
135 list_for_each_entry(net, net_exit_list, exit_list)
136 ops->exit(net);
137 }
138 if (ops->exit_batch)
139 ops->exit_batch(net_exit_list);
140 }
141
142 static void ops_free_list(const struct pernet_operations *ops,
143 struct list_head *net_exit_list)
144 {
145 struct net *net;
146 if (ops->size && ops->id) {
147 list_for_each_entry(net, net_exit_list, exit_list)
148 ops_free(ops, net);
149 }
150 }
151
152 /* should be called with nsid_lock held */
153 static int alloc_netid(struct net *net, struct net *peer, int reqid)
154 {
155 int min = 0, max = 0;
156
157 if (reqid >= 0) {
158 min = reqid;
159 max = reqid + 1;
160 }
161
> 162 if (!atomic_read(&net->count) || !&atomic_read(peer->count))
163 return -EINVAL;
164
165 return idr_alloc(&net->netns_ids, peer, min, max, GFP_ATOMIC);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27870 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net] net: nsid cannot be allocated for a dead netns
2016-11-16 9:10 ` kbuild test robot
@ 2016-11-16 9:45 ` Nicolas Dichtel
2016-11-16 9:49 ` [PATCH net v2] " Nicolas Dichtel
1 sibling, 0 replies; 22+ messages in thread
From: Nicolas Dichtel @ 2016-11-16 9:45 UTC (permalink / raw)
To: kbuild test robot; +Cc: kbuild-all, avagin, davem, netdev, xiyou.wangcong
Le 16/11/2016 à 10:10, kbuild test robot a écrit :
> Hi Nicolas,
>
> [auto build test ERROR on net/master]
And I finally send the wrong version of the patch :(
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net v2] net: nsid cannot be allocated for a dead netns
2016-11-16 9:10 ` kbuild test robot
2016-11-16 9:45 ` Nicolas Dichtel
@ 2016-11-16 9:49 ` Nicolas Dichtel
2016-11-16 17:29 ` Cong Wang
1 sibling, 1 reply; 22+ messages in thread
From: Nicolas Dichtel @ 2016-11-16 9:49 UTC (permalink / raw)
To: avagin; +Cc: davem, netdev, xiyou.wangcong, Nicolas Dichtel
Andrei reports the following kmemleak error:
unreferenced object 0xffff91badb543950 (size 2096):
comm "kworker/u4:0", pid 6, jiffies 4295152553 (age 28.418s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 cb 5f df ba 91 ff ff .........._.....
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffffb1865bea>] kmemleak_alloc+0x4a/0xa0
[<ffffffffb1243b38>] kmem_cache_alloc+0x128/0x280
[<ffffffffb142f5ab>] idr_layer_alloc+0x2b/0x90
[<ffffffffb142f9cd>] idr_get_empty_slot+0x34d/0x370
[<ffffffffb142fa4e>] idr_alloc+0x5e/0x110
[<ffffffffb170ac3d>] __peernet2id_alloc+0x6d/0x90
[<ffffffffb170bda5>] peernet2id_alloc+0x55/0xb0
[<ffffffffb1731216>] rtnl_fill_ifinfo+0xaa6/0x10a0
[<ffffffffb1733073>] rtmsg_ifinfo_build_skb+0x73/0xd0
[<ffffffffb17125d5>] rollback_registered_many+0x295/0x390
[<ffffffffb1712765>] unregister_netdevice_many+0x25/0x80
[<ffffffffb17138a5>] default_device_exit_batch+0x145/0x170
[<ffffffffb170ae52>] ops_exit_list.isra.4+0x52/0x60
[<ffffffffb170c17f>] cleanup_net+0x1bf/0x2a0
[<ffffffffb10b616f>] process_one_work+0x1ff/0x660
[<ffffffffb10b661e>] worker_thread+0x4e/0x480
There is no reason to try to allocate an nsid for a netns which is dying.
Fixes: 0c7aecd4bde4 ("netns: add rtnl cmd to add and get peer netns ids")
Reported-by: Andrei Vagin <avagin@gmail.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
v2: fix compilation
add the 'Fixes' tag.
Andrei, can you test this new version?
Regards,
Nicolas
net/core/net_namespace.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index f61c0e02a413..63f65387f4e1 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -159,6 +159,9 @@ static int alloc_netid(struct net *net, struct net *peer, int reqid)
max = reqid + 1;
}
+ if (!atomic_read(&net->count) || !atomic_read(&peer->count))
+ return -EINVAL;
+
return idr_alloc(&net->netns_ids, peer, min, max, GFP_ATOMIC);
}
--
2.8.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH net] net: nsid cannot be allocated for a dead netns
2016-11-16 8:41 ` [PATCH net] net: nsid cannot be allocated for a dead netns Nicolas Dichtel
2016-11-16 8:42 ` Nicolas Dichtel
2016-11-16 9:10 ` kbuild test robot
@ 2016-11-16 10:23 ` kbuild test robot
2016-11-16 12:00 ` Sergei Shtylyov
3 siblings, 0 replies; 22+ messages in thread
From: kbuild test robot @ 2016-11-16 10:23 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: kbuild-all, avagin, davem, netdev, xiyou.wangcong, Nicolas Dichtel
[-- Attachment #1: Type: text/plain, Size: 7593 bytes --]
Hi Nicolas,
[auto build test ERROR on net/master]
url: https://github.com/0day-ci/linux/commits/Nicolas-Dichtel/net-nsid-cannot-be-allocated-for-a-dead-netns/20161116-164739
config: sparc-defconfig (attached as .config)
compiler: sparc-linux-gcc (GCC) 6.2.0
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc
All error/warnings (new ones prefixed by >>):
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/list.h:4,
from include/linux/timer.h:4,
from include/linux/workqueue.h:8,
from net/core/net_namespace.c:3:
net/core/net_namespace.c: In function 'alloc_netid':
>> arch/sparc/include/asm/atomic_32.h:32:48: error: invalid type argument of '->' (have 'atomic_t {aka struct <anonymous>}')
#define atomic_read(v) ACCESS_ONCE((v)->counter)
^
include/linux/compiler.h:545:25: note: in definition of macro '__ACCESS_ONCE'
__maybe_unused typeof(x) __var = (__force typeof(x)) 0; \
^
>> arch/sparc/include/asm/atomic_32.h:32:33: note: in expansion of macro 'ACCESS_ONCE'
#define atomic_read(v) ACCESS_ONCE((v)->counter)
^~~~~~~~~~~
net/core/net_namespace.c:162:37: note: in expansion of macro 'atomic_read'
if (!atomic_read(&net->count) || !&atomic_read(peer->count))
^~~~~~~~~~~
>> arch/sparc/include/asm/atomic_32.h:32:48: error: invalid type argument of '->' (have 'atomic_t {aka struct <anonymous>}')
#define atomic_read(v) ACCESS_ONCE((v)->counter)
^
include/linux/compiler.h:545:52: note: in definition of macro '__ACCESS_ONCE'
__maybe_unused typeof(x) __var = (__force typeof(x)) 0; \
^
>> arch/sparc/include/asm/atomic_32.h:32:33: note: in expansion of macro 'ACCESS_ONCE'
#define atomic_read(v) ACCESS_ONCE((v)->counter)
^~~~~~~~~~~
net/core/net_namespace.c:162:37: note: in expansion of macro 'atomic_read'
if (!atomic_read(&net->count) || !&atomic_read(peer->count))
^~~~~~~~~~~
>> arch/sparc/include/asm/atomic_32.h:32:48: error: invalid type argument of '->' (have 'atomic_t {aka struct <anonymous>}')
#define atomic_read(v) ACCESS_ONCE((v)->counter)
^
include/linux/compiler.h:546:19: note: in definition of macro '__ACCESS_ONCE'
(volatile typeof(x) *)&(x); })
^
>> arch/sparc/include/asm/atomic_32.h:32:33: note: in expansion of macro 'ACCESS_ONCE'
#define atomic_read(v) ACCESS_ONCE((v)->counter)
^~~~~~~~~~~
net/core/net_namespace.c:162:37: note: in expansion of macro 'atomic_read'
if (!atomic_read(&net->count) || !&atomic_read(peer->count))
^~~~~~~~~~~
>> arch/sparc/include/asm/atomic_32.h:32:48: error: invalid type argument of '->' (have 'atomic_t {aka struct <anonymous>}')
#define atomic_read(v) ACCESS_ONCE((v)->counter)
^
include/linux/compiler.h:546:26: note: in definition of macro '__ACCESS_ONCE'
(volatile typeof(x) *)&(x); })
^
>> arch/sparc/include/asm/atomic_32.h:32:33: note: in expansion of macro 'ACCESS_ONCE'
#define atomic_read(v) ACCESS_ONCE((v)->counter)
^~~~~~~~~~~
net/core/net_namespace.c:162:37: note: in expansion of macro 'atomic_read'
if (!atomic_read(&net->count) || !&atomic_read(peer->count))
^~~~~~~~~~~
vim +32 arch/sparc/include/asm/atomic_32.h
d550bbd4 arch/sparc/include/asm/atomic_32.h David Howells 2012-03-28 16 #include <asm/cmpxchg.h>
56d36489 arch/sparc/include/asm/atomic_32.h Peter Zijlstra 2014-03-13 17 #include <asm/barrier.h>
aea1181b arch/sparc/include/asm/atomic_32.h Sam Ravnborg 2011-12-27 18 #include <asm-generic/atomic64.h>
aea1181b arch/sparc/include/asm/atomic_32.h Sam Ravnborg 2011-12-27 19
f5e706ad include/asm-sparc/atomic_32.h Sam Ravnborg 2008-07-17 20 #define ATOMIC_INIT(i) { (i) }
f5e706ad include/asm-sparc/atomic_32.h Sam Ravnborg 2008-07-17 21
4f3316c2 arch/sparc/include/asm/atomic_32.h Peter Zijlstra 2014-03-26 22 int atomic_add_return(int, atomic_t *);
3a1adb23 arch/sparc/include/asm/atomic_32.h Peter Zijlstra 2016-04-18 23 int atomic_fetch_add(int, atomic_t *);
3a1adb23 arch/sparc/include/asm/atomic_32.h Peter Zijlstra 2016-04-18 24 int atomic_fetch_and(int, atomic_t *);
3a1adb23 arch/sparc/include/asm/atomic_32.h Peter Zijlstra 2016-04-18 25 int atomic_fetch_or(int, atomic_t *);
3a1adb23 arch/sparc/include/asm/atomic_32.h Peter Zijlstra 2016-04-18 26 int atomic_fetch_xor(int, atomic_t *);
f05a6865 arch/sparc/include/asm/atomic_32.h Sam Ravnborg 2014-05-16 27 int atomic_cmpxchg(atomic_t *, int, int);
1a17fdc4 arch/sparc/include/asm/atomic_32.h Andreas Larsson 2014-11-05 28 int atomic_xchg(atomic_t *, int);
f05a6865 arch/sparc/include/asm/atomic_32.h Sam Ravnborg 2014-05-16 29 int __atomic_add_unless(atomic_t *, int, int);
f05a6865 arch/sparc/include/asm/atomic_32.h Sam Ravnborg 2014-05-16 30 void atomic_set(atomic_t *, int);
f5e706ad include/asm-sparc/atomic_32.h Sam Ravnborg 2008-07-17 31
2291059c arch/sparc/include/asm/atomic_32.h Pranith Kumar 2014-09-23 @32 #define atomic_read(v) ACCESS_ONCE((v)->counter)
f5e706ad include/asm-sparc/atomic_32.h Sam Ravnborg 2008-07-17 33
4f3316c2 arch/sparc/include/asm/atomic_32.h Peter Zijlstra 2014-03-26 34 #define atomic_add(i, v) ((void)atomic_add_return( (int)(i), (v)))
4f3316c2 arch/sparc/include/asm/atomic_32.h Peter Zijlstra 2014-03-26 35 #define atomic_sub(i, v) ((void)atomic_add_return(-(int)(i), (v)))
4f3316c2 arch/sparc/include/asm/atomic_32.h Peter Zijlstra 2014-03-26 36 #define atomic_inc(v) ((void)atomic_add_return( 1, (v)))
4f3316c2 arch/sparc/include/asm/atomic_32.h Peter Zijlstra 2014-03-26 37 #define atomic_dec(v) ((void)atomic_add_return( -1, (v)))
f5e706ad include/asm-sparc/atomic_32.h Sam Ravnborg 2008-07-17 38
3a1adb23 arch/sparc/include/asm/atomic_32.h Peter Zijlstra 2016-04-18 39 #define atomic_and(i, v) ((void)atomic_fetch_and((i), (v)))
3a1adb23 arch/sparc/include/asm/atomic_32.h Peter Zijlstra 2016-04-18 40 #define atomic_or(i, v) ((void)atomic_fetch_or((i), (v)))
:::::: The code at line 32 was first introduced by commit
:::::: 2291059c852706c6f5ffb400366042b7625066cd locking,arch: Use ACCESS_ONCE() instead of cast to volatile in atomic_read()
:::::: TO: Pranith Kumar <bobby.prani@gmail.com>
:::::: CC: Ingo Molnar <mingo@kernel.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 11551 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net] net: nsid cannot be allocated for a dead netns
2016-11-16 8:41 ` [PATCH net] net: nsid cannot be allocated for a dead netns Nicolas Dichtel
` (2 preceding siblings ...)
2016-11-16 10:23 ` [PATCH net] " kbuild test robot
@ 2016-11-16 12:00 ` Sergei Shtylyov
3 siblings, 0 replies; 22+ messages in thread
From: Sergei Shtylyov @ 2016-11-16 12:00 UTC (permalink / raw)
To: Nicolas Dichtel, avagin; +Cc: davem, netdev, xiyou.wangcong
Hello.
On 11/16/2016 11:41 AM, Nicolas Dichtel wrote:
> Andrei reports the following kmemleak error:
> unreferenced object 0xffff91badb543950 (size 2096):
> comm "kworker/u4:0", pid 6, jiffies 4295152553 (age 28.418s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 cb 5f df ba 91 ff ff .........._.....
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffffb1865bea>] kmemleak_alloc+0x4a/0xa0
> [<ffffffffb1243b38>] kmem_cache_alloc+0x128/0x280
> [<ffffffffb142f5ab>] idr_layer_alloc+0x2b/0x90
> [<ffffffffb142f9cd>] idr_get_empty_slot+0x34d/0x370
> [<ffffffffb142fa4e>] idr_alloc+0x5e/0x110
> [<ffffffffb170ac3d>] __peernet2id_alloc+0x6d/0x90
> [<ffffffffb170bda5>] peernet2id_alloc+0x55/0xb0
> [<ffffffffb1731216>] rtnl_fill_ifinfo+0xaa6/0x10a0
> [<ffffffffb1733073>] rtmsg_ifinfo_build_skb+0x73/0xd0
> [<ffffffffb17125d5>] rollback_registered_many+0x295/0x390
> [<ffffffffb1712765>] unregister_netdevice_many+0x25/0x80
> [<ffffffffb17138a5>] default_device_exit_batch+0x145/0x170
> [<ffffffffb170ae52>] ops_exit_list.isra.4+0x52/0x60
> [<ffffffffb170c17f>] cleanup_net+0x1bf/0x2a0
> [<ffffffffb10b616f>] process_one_work+0x1ff/0x660
> [<ffffffffb10b661e>] worker_thread+0x4e/0x480
>
> There is no reason to try to allocate an nsid for a netns which is dying.
>
> Reported-by: Andrei Vagin <avagin@gmail.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>
> Thank you for the report. Can you test this patch?
>
> Regards,
> Nicolas
>
> net/core/net_namespace.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index f61c0e02a413..f1340ed0d8df 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -159,6 +159,9 @@ static int alloc_netid(struct net *net, struct net *peer, int reqid)
> max = reqid + 1;
> }
>
> + if (!atomic_read(&net->count) || !&atomic_read(peer->count))
Looks like & is misplaced in the second case?
MBR, Sergei
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net v2] net: nsid cannot be allocated for a dead netns
2016-11-16 9:49 ` [PATCH net v2] " Nicolas Dichtel
@ 2016-11-16 17:29 ` Cong Wang
2016-11-17 4:17 ` David Miller
0 siblings, 1 reply; 22+ messages in thread
From: Cong Wang @ 2016-11-16 17:29 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: Andrey Wagin, David Miller, Linux Kernel Network Developers
On Wed, Nov 16, 2016 at 1:49 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index f61c0e02a413..63f65387f4e1 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -159,6 +159,9 @@ static int alloc_netid(struct net *net, struct net *peer, int reqid)
> max = reqid + 1;
> }
>
> + if (!atomic_read(&net->count) || !atomic_read(&peer->count))
> + return -EINVAL;
> +
> return idr_alloc(&net->netns_ids, peer, min, max, GFP_ATOMIC);
> }
There is already a check in peernet2id_alloc(), so why not just the following?
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index f61c0e0..7001da9 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -219,6 +219,8 @@ int peernet2id_alloc(struct net *net, struct net *peer)
bool alloc;
int id;
+ if (atomic_read(&net->count) == 0)
+ return NETNSA_NSID_NOT_ASSIGNED;
spin_lock_irqsave(&net->nsid_lock, flags);
alloc = atomic_read(&peer->count) == 0 ? false : true;
id = __peernet2id_alloc(net, peer, &alloc);
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: linux-next: net->netns_ids is used after calling idr_destroy for it
2016-11-15 22:45 ` Andrei Vagin
@ 2016-11-16 18:04 ` Cong Wang
0 siblings, 0 replies; 22+ messages in thread
From: Cong Wang @ 2016-11-16 18:04 UTC (permalink / raw)
To: Andrei Vagin; +Cc: Nicolas Dichtel, Linux Kernel Network Developers
On Tue, Nov 15, 2016 at 2:45 PM, Andrei Vagin <avagin@gmail.com> wrote:
> On Tue, Nov 15, 2016 at 2:21 PM, Andrei Vagin <avagin@gmail.com> wrote:
>> I would like to agree with you here, but looks like sockets with
>> NETLINK_F_LISTEN_ALL_NSID are able to catch these messages.
>
> Actually I found that I was wrong.
>
> do_one_broadcast() sends a notification only if a device network
> namespace has an id in a socket netns. But cleanup_net() removes all
> id-s to a target namespace, so just ignore my last comment.
The point is all sockets in that netns are already gone at that point
because of refcount.
cleanup_net() also destroys net->netns_ids, so it should not be even
accessed after that point.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net v2] net: nsid cannot be allocated for a dead netns
2016-11-16 17:29 ` Cong Wang
@ 2016-11-17 4:17 ` David Miller
2016-11-17 6:32 ` Cong Wang
2016-11-17 8:39 ` Nicolas Dichtel
0 siblings, 2 replies; 22+ messages in thread
From: David Miller @ 2016-11-17 4:17 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: nicolas.dichtel, avagin, netdev
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 16 Nov 2016 09:29:29 -0800
> On Wed, Nov 16, 2016 at 1:49 AM, Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
>> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
>> index f61c0e02a413..63f65387f4e1 100644
>> --- a/net/core/net_namespace.c
>> +++ b/net/core/net_namespace.c
>> @@ -159,6 +159,9 @@ static int alloc_netid(struct net *net, struct net *peer, int reqid)
>> max = reqid + 1;
>> }
>>
>> + if (!atomic_read(&net->count) || !atomic_read(&peer->count))
>> + return -EINVAL;
>> +
>> return idr_alloc(&net->netns_ids, peer, min, max, GFP_ATOMIC);
>> }
>
>
> There is already a check in peernet2id_alloc(), so why not just the following?
>
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index f61c0e0..7001da9 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -219,6 +219,8 @@ int peernet2id_alloc(struct net *net, struct net *peer)
> bool alloc;
> int id;
>
> + if (atomic_read(&net->count) == 0)
> + return NETNSA_NSID_NOT_ASSIGNED;
> spin_lock_irqsave(&net->nsid_lock, flags);
> alloc = atomic_read(&peer->count) == 0 ? false : true;
> id = __peernet2id_alloc(net, peer, &alloc);
Indeed, this looks cleaner, Nicolas?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net v2] net: nsid cannot be allocated for a dead netns
2016-11-17 4:17 ` David Miller
@ 2016-11-17 6:32 ` Cong Wang
2016-11-17 8:41 ` Nicolas Dichtel
2016-11-17 8:39 ` Nicolas Dichtel
1 sibling, 1 reply; 22+ messages in thread
From: Cong Wang @ 2016-11-17 6:32 UTC (permalink / raw)
To: David Miller
Cc: Nicolas Dichtel, Andrey Wagin, Linux Kernel Network Developers
On Wed, Nov 16, 2016 at 8:17 PM, David Miller <davem@davemloft.net> wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Wed, 16 Nov 2016 09:29:29 -0800
>
>> On Wed, Nov 16, 2016 at 1:49 AM, Nicolas Dichtel
>> <nicolas.dichtel@6wind.com> wrote:
>>> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
>>> index f61c0e02a413..63f65387f4e1 100644
>>> --- a/net/core/net_namespace.c
>>> +++ b/net/core/net_namespace.c
>>> @@ -159,6 +159,9 @@ static int alloc_netid(struct net *net, struct net *peer, int reqid)
>>> max = reqid + 1;
>>> }
>>>
>>> + if (!atomic_read(&net->count) || !atomic_read(&peer->count))
>>> + return -EINVAL;
>>> +
>>> return idr_alloc(&net->netns_ids, peer, min, max, GFP_ATOMIC);
>>> }
>>
>>
>> There is already a check in peernet2id_alloc(), so why not just the following?
>>
>> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
>> index f61c0e0..7001da9 100644
>> --- a/net/core/net_namespace.c
>> +++ b/net/core/net_namespace.c
>> @@ -219,6 +219,8 @@ int peernet2id_alloc(struct net *net, struct net *peer)
>> bool alloc;
>> int id;
>>
>> + if (atomic_read(&net->count) == 0)
>> + return NETNSA_NSID_NOT_ASSIGNED;
>> spin_lock_irqsave(&net->nsid_lock, flags);
>> alloc = atomic_read(&peer->count) == 0 ? false : true;
>> id = __peernet2id_alloc(net, peer, &alloc);
>
> Indeed, this looks cleaner, Nicolas?
Yeah, I already sent a formal patch:
https://patchwork.ozlabs.org/patch/695747/
since Nicolas' patch doesn't even compile...
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net v2] net: nsid cannot be allocated for a dead netns
2016-11-17 4:17 ` David Miller
2016-11-17 6:32 ` Cong Wang
@ 2016-11-17 8:39 ` Nicolas Dichtel
1 sibling, 0 replies; 22+ messages in thread
From: Nicolas Dichtel @ 2016-11-17 8:39 UTC (permalink / raw)
To: David Miller, xiyou.wangcong; +Cc: avagin, netdev
Le 17/11/2016 à 05:17, David Miller a écrit :
[snip]
> Indeed, this looks cleaner, Nicolas?
Yes, I agree. Cong already sent a v3 of my patch, let's apply it.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net v2] net: nsid cannot be allocated for a dead netns
2016-11-17 6:32 ` Cong Wang
@ 2016-11-17 8:41 ` Nicolas Dichtel
2016-11-17 16:50 ` Cong Wang
0 siblings, 1 reply; 22+ messages in thread
From: Nicolas Dichtel @ 2016-11-17 8:41 UTC (permalink / raw)
To: Cong Wang, David Miller; +Cc: Andrey Wagin, Linux Kernel Network Developers
Le 17/11/2016 à 07:32, Cong Wang a écrit :
[snip]
> since Nicolas' patch doesn't even compile...
It's always surprising how agressive you are, really :(
Can you show me the compilation error of this patch (we are talking about the v2
patch here)?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net v2] net: nsid cannot be allocated for a dead netns
2016-11-17 8:41 ` Nicolas Dichtel
@ 2016-11-17 16:50 ` Cong Wang
0 siblings, 0 replies; 22+ messages in thread
From: Cong Wang @ 2016-11-17 16:50 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: David Miller, Andrey Wagin, Linux Kernel Network Developers
On Thu, Nov 17, 2016 at 12:41 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> Le 17/11/2016 à 07:32, Cong Wang a écrit :
> [snip]
>> since Nicolas' patch doesn't even compile...
> It's always surprising how agressive you are, really :(
> Can you show me the compilation error of this patch (we are talking about the v2
> patch here)?
Sorry about that, I didn't event look at your v2, because your patch
(no matter v2 or v1) is already wrong to me, the idr_for_each() before
alloc_netid() is clearly a use-after-destroy.
Based on the same reason, my patch is not your v3, we are patching
different places.
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2016-11-17 17:16 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-15 6:23 linux-next: net->netns_ids is used after calling idr_destroy for it Andrei Vagin
2016-11-15 6:39 ` Andrei Vagin
2016-11-16 8:41 ` [PATCH net] net: nsid cannot be allocated for a dead netns Nicolas Dichtel
2016-11-16 8:42 ` Nicolas Dichtel
2016-11-16 9:10 ` kbuild test robot
2016-11-16 9:45 ` Nicolas Dichtel
2016-11-16 9:49 ` [PATCH net v2] " Nicolas Dichtel
2016-11-16 17:29 ` Cong Wang
2016-11-17 4:17 ` David Miller
2016-11-17 6:32 ` Cong Wang
2016-11-17 8:41 ` Nicolas Dichtel
2016-11-17 16:50 ` Cong Wang
2016-11-17 8:39 ` Nicolas Dichtel
2016-11-16 10:23 ` [PATCH net] " kbuild test robot
2016-11-16 12:00 ` Sergei Shtylyov
2016-11-15 18:04 ` linux-next: net->netns_ids is used after calling idr_destroy for it Cong Wang
2016-11-15 18:50 ` Cong Wang
2016-11-15 20:48 ` Andrei Vagin
2016-11-15 21:07 ` Cong Wang
2016-11-15 22:21 ` Andrei Vagin
2016-11-15 22:45 ` Andrei Vagin
2016-11-16 18:04 ` Cong Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.