* [PATCH v0] kernfs: Skip kernfs_put of parent from child node @ 2019-04-05 11:21 Gaurav Kohli 2019-04-05 11:21 ` Gaurav Kohli 2019-04-05 11:33 ` Greg KH 0 siblings, 2 replies; 12+ messages in thread From: Gaurav Kohli @ 2019-04-05 11:21 UTC (permalink / raw) To: gregkh, tj, linux-kernel; +Cc: linux-arm-msm, Gaurav Kohli, Mukesh Ojha While adding kernfs node for child to the parent kernfs node and when child node founds that parent kn count is zero, then below comes like: WARNING: fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 This indicates that parent is in kernfs_put path/ or already freed, and if the child node keeps continue to make new kernfs node, then there is chance of below race for parent node: CPU0 CPU1 //Parent node //child node kernfs_put atomic_dec_and_test(&kn->count) //count is 0, so continue kernfs_new_node(child) kernfs_get(parent); //increment parent count to 1 //warning come as parent count is 0 /* link in */ kernfs_add_one(kn); // this should fail as parent is //in free path. kernfs_put(child) kmem_cache_free(parent) kmem_cache_free(child) kn = parent atomic_dec_and_test(&kn->count)) //this is 0 now, so release will continue for parent. kmem_cache_free(parent) To prevent this race, child simply has to decrement count of parent kernfs node and keep continue the free path for itself. Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c index b84d635..d5a36e8 100644 --- a/fs/kernfs/dir.c +++ b/fs/kernfs/dir.c @@ -515,7 +515,6 @@ void kernfs_put(struct kernfs_node *kn) if (!kn || !atomic_dec_and_test(&kn->count)) return; root = kernfs_root(kn); - repeat: /* * Moving/renaming is always done while holding reference. * kn->parent won't change beneath us. @@ -545,8 +544,8 @@ void kernfs_put(struct kernfs_node *kn) kn = parent; if (kn) { - if (atomic_dec_and_test(&kn->count)) - goto repeat; + /* Parent may be on free path, so simply decrement the count */ + atomic_dec_and_test(&kn->count); } else { /* just released the root kn, free @root too */ idr_destroy(&root->ino_idr); -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v0] kernfs: Skip kernfs_put of parent from child node 2019-04-05 11:21 [PATCH v0] kernfs: Skip kernfs_put of parent from child node Gaurav Kohli @ 2019-04-05 11:21 ` Gaurav Kohli 2019-04-05 11:33 ` Greg KH 1 sibling, 0 replies; 12+ messages in thread From: Gaurav Kohli @ 2019-04-05 11:21 UTC (permalink / raw) To: gregkh, tj, linux-kernel; +Cc: linux-arm-msm, Gaurav Kohli, Mukesh Ojha While adding kernfs node for child to the parent kernfs node and when child node founds that parent kn count is zero, then below comes like: WARNING: fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 This indicates that parent is in kernfs_put path/ or already freed, and if the child node keeps continue to make new kernfs node, then there is chance of below race for parent node: CPU0 CPU1 //Parent node //child node kernfs_put atomic_dec_and_test(&kn->count) //count is 0, so continue kernfs_new_node(child) kernfs_get(parent); //increment parent count to 1 //warning come as parent count is 0 /* link in */ kernfs_add_one(kn); // this should fail as parent is //in free path. kernfs_put(child) kmem_cache_free(parent) kmem_cache_free(child) kn = parent atomic_dec_and_test(&kn->count)) //this is 0 now, so release will continue for parent. kmem_cache_free(parent) To prevent this race, child simply has to decrement count of parent kernfs node and keep continue the free path for itself. Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c index b84d635..d5a36e8 100644 --- a/fs/kernfs/dir.c +++ b/fs/kernfs/dir.c @@ -515,7 +515,6 @@ void kernfs_put(struct kernfs_node *kn) if (!kn || !atomic_dec_and_test(&kn->count)) return; root = kernfs_root(kn); - repeat: /* * Moving/renaming is always done while holding reference. * kn->parent won't change beneath us. @@ -545,8 +544,8 @@ void kernfs_put(struct kernfs_node *kn) kn = parent; if (kn) { - if (atomic_dec_and_test(&kn->count)) - goto repeat; + /* Parent may be on free path, so simply decrement the count */ + atomic_dec_and_test(&kn->count); } else { /* just released the root kn, free @root too */ idr_destroy(&root->ino_idr); -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v0] kernfs: Skip kernfs_put of parent from child node 2019-04-05 11:21 [PATCH v0] kernfs: Skip kernfs_put of parent from child node Gaurav Kohli 2019-04-05 11:21 ` Gaurav Kohli @ 2019-04-05 11:33 ` Greg KH 2019-04-05 11:33 ` Greg KH 2019-04-05 11:43 ` Gaurav Kohli 1 sibling, 2 replies; 12+ messages in thread From: Greg KH @ 2019-04-05 11:33 UTC (permalink / raw) To: Gaurav Kohli; +Cc: tj, linux-kernel, linux-arm-msm, Mukesh Ojha On Fri, Apr 05, 2019 at 04:51:07PM +0530, Gaurav Kohli wrote: > While adding kernfs node for child to the parent kernfs > node and when child node founds that parent kn count is > zero, then below comes like: > > WARNING: fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 > > This indicates that parent is in kernfs_put path/ or already > freed, and if the child node keeps continue to > make new kernfs node, then there is chance of > below race for parent node: > > CPU0 CPU1 > //Parent node //child node > kernfs_put > atomic_dec_and_test(&kn->count) > //count is 0, so continue > kernfs_new_node(child) > kernfs_get(parent); > //increment parent count to 1 > //warning come as parent count is 0 > /* link in */ > kernfs_add_one(kn); > // this should fail as parent is > //in free path. > kernfs_put(child) > kmem_cache_free(parent) > kmem_cache_free(child) > kn = parent > atomic_dec_and_test(&kn->count)) > //this is 0 now, so release will > continue for parent. > kmem_cache_free(parent) > > To prevent this race, child simply has to decrement count of parent > kernfs node and keep continue the free path for itself. > > Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org> > Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > index b84d635..d5a36e8 100644 > --- a/fs/kernfs/dir.c > +++ b/fs/kernfs/dir.c > @@ -515,7 +515,6 @@ void kernfs_put(struct kernfs_node *kn) > if (!kn || !atomic_dec_and_test(&kn->count)) > return; > root = kernfs_root(kn); > - repeat: > /* > * Moving/renaming is always done while holding reference. > * kn->parent won't change beneath us. > @@ -545,8 +544,8 @@ void kernfs_put(struct kernfs_node *kn) > > kn = parent; > if (kn) { > - if (atomic_dec_and_test(&kn->count)) > - goto repeat; > + /* Parent may be on free path, so simply decrement the count */ That's the wrong indentation :( And how are you hitting this issue? What user of kernfs is causing this? thanks, greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v0] kernfs: Skip kernfs_put of parent from child node 2019-04-05 11:33 ` Greg KH @ 2019-04-05 11:33 ` Greg KH 2019-04-05 11:43 ` Gaurav Kohli 1 sibling, 0 replies; 12+ messages in thread From: Greg KH @ 2019-04-05 11:33 UTC (permalink / raw) To: Gaurav Kohli; +Cc: tj, linux-kernel, linux-arm-msm, Mukesh Ojha On Fri, Apr 05, 2019 at 04:51:07PM +0530, Gaurav Kohli wrote: > While adding kernfs node for child to the parent kernfs > node and when child node founds that parent kn count is > zero, then below comes like: > > WARNING: fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 > > This indicates that parent is in kernfs_put path/ or already > freed, and if the child node keeps continue to > make new kernfs node, then there is chance of > below race for parent node: > > CPU0 CPU1 > //Parent node //child node > kernfs_put > atomic_dec_and_test(&kn->count) > //count is 0, so continue > kernfs_new_node(child) > kernfs_get(parent); > //increment parent count to 1 > //warning come as parent count is 0 > /* link in */ > kernfs_add_one(kn); > // this should fail as parent is > //in free path. > kernfs_put(child) > kmem_cache_free(parent) > kmem_cache_free(child) > kn = parent > atomic_dec_and_test(&kn->count)) > //this is 0 now, so release will > continue for parent. > kmem_cache_free(parent) > > To prevent this race, child simply has to decrement count of parent > kernfs node and keep continue the free path for itself. > > Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org> > Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > index b84d635..d5a36e8 100644 > --- a/fs/kernfs/dir.c > +++ b/fs/kernfs/dir.c > @@ -515,7 +515,6 @@ void kernfs_put(struct kernfs_node *kn) > if (!kn || !atomic_dec_and_test(&kn->count)) > return; > root = kernfs_root(kn); > - repeat: > /* > * Moving/renaming is always done while holding reference. > * kn->parent won't change beneath us. > @@ -545,8 +544,8 @@ void kernfs_put(struct kernfs_node *kn) > > kn = parent; > if (kn) { > - if (atomic_dec_and_test(&kn->count)) > - goto repeat; > + /* Parent may be on free path, so simply decrement the count */ That's the wrong indentation :( And how are you hitting this issue? What user of kernfs is causing this? thanks, greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v0] kernfs: Skip kernfs_put of parent from child node 2019-04-05 11:33 ` Greg KH 2019-04-05 11:33 ` Greg KH @ 2019-04-05 11:43 ` Gaurav Kohli 2019-04-05 11:43 ` Gaurav Kohli 2019-04-05 12:10 ` Greg KH 1 sibling, 2 replies; 12+ messages in thread From: Gaurav Kohli @ 2019-04-05 11:43 UTC (permalink / raw) To: Greg KH; +Cc: tj, linux-kernel, linux-arm-msm, Mukesh Ojha On 4/5/2019 5:03 PM, Greg KH wrote: > On Fri, Apr 05, 2019 at 04:51:07PM +0530, Gaurav Kohli wrote: >> While adding kernfs node for child to the parent kernfs >> node and when child node founds that parent kn count is >> zero, then below comes like: >> >> WARNING: fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 >> >> This indicates that parent is in kernfs_put path/ or already >> freed, and if the child node keeps continue to >> make new kernfs node, then there is chance of >> below race for parent node: >> >> CPU0 CPU1 >> //Parent node //child node >> kernfs_put >> atomic_dec_and_test(&kn->count) >> //count is 0, so continue >> kernfs_new_node(child) >> kernfs_get(parent); >> //increment parent count to 1 >> //warning come as parent count is 0 >> /* link in */ >> kernfs_add_one(kn); >> // this should fail as parent is >> //in free path. >> kernfs_put(child) >> kmem_cache_free(parent) >> kmem_cache_free(child) >> kn = parent >> atomic_dec_and_test(&kn->count)) >> //this is 0 now, so release will >> continue for parent. >> kmem_cache_free(parent) >> >> To prevent this race, child simply has to decrement count of parent >> kernfs node and keep continue the free path for itself. >> >> Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org> >> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> >> >> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c >> index b84d635..d5a36e8 100644 >> --- a/fs/kernfs/dir.c >> +++ b/fs/kernfs/dir.c >> @@ -515,7 +515,6 @@ void kernfs_put(struct kernfs_node *kn) >> if (!kn || !atomic_dec_and_test(&kn->count)) >> return; >> root = kernfs_root(kn); >> - repeat: >> /* >> * Moving/renaming is always done while holding reference. >> * kn->parent won't change beneath us. >> @@ -545,8 +544,8 @@ void kernfs_put(struct kernfs_node *kn) >> >> kn = parent; >> if (kn) { >> - if (atomic_dec_and_test(&kn->count)) >> - goto repeat; >> + /* Parent may be on free path, so simply decrement the count */ > That's the wrong indentation :( > > And how are you hitting this issue? What user of kernfs is causing > this? Hi Greg, Thanks, will fix comment indentation, seen during sys-executor running: We have only one instance , In logs below warning also came: WARNING: CPU: 4 kernel/msm-4.14/fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 which indicated parent is in put path. [ 160.125151] Disabling lock debugging due to kernel taint [ 160.130626] INFO: Allocated in __kernfs_new_node+0x8c/0x3c0 age=11 cpu=2 pid=7098 [ 160.138314] kmem_cache_alloc+0x358/0x388 [ 160.142445] __kernfs_new_node+0x8c/0x3c0 [ 160.146590] kernfs_new_node+0x80/0xc8 [ 160.150462] kernfs_create_dir_ns+0x44/0xfc [ 160.154777] sysfs_create_dir_ns+0xa8/0x130 [ 160.158416] CPU5: update max cpu_capacity 1024 [ 160.159085] kobject_add_internal+0x278/0x650 [ 160.163567] kobject_add_varg+0xe0/0x130 [ 160.167606] kobject_add+0x15c/0x1d0 [ 160.168452] CPU5: update max cpu_capacity 780 [ 160.171287] get_device_parent+0x2d0/0x34c [ 160.175510] device_add+0x240/0xde0 [ 160.178371] CPU6: update max cpu_capacity 916 [ 160.179108] input_register_device+0x5f4/0xa0c [ 160.183686] uinput_ioctl_handler+0x1184/0x2198 [ 160.202436] INFO: Freed in kernfs_put+0x2c8/0x434 age=14 cpu=0 pid=7096 [ 160.209230] kernfs_put+0x2c8/0x434 [ 160.212825] kobject_del+0x50/0xcc [ 160.216332] cleanup_glue_dir+0x124/0x16c [ 160.220456] device_del+0x55c/0x5c8 [ 160.224047] __input_unregister_device+0x274/0x2a8 [ 160.228974] input_unregister_device+0x90/0xd0 [ 160.233553] uinput_destroy_device+0x15c/0x1dc [ 160.238131] uinput_release+0x44/0x5c [ 160.241898] __fput+0x1f4/0x4e4 [ 160.245127] ____fput+0x20/0x2c during code review, I have found race between kernfs parent put call and child get call. Regards Gaurav > > thanks, > > greg k-h -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v0] kernfs: Skip kernfs_put of parent from child node 2019-04-05 11:43 ` Gaurav Kohli @ 2019-04-05 11:43 ` Gaurav Kohli 2019-04-05 12:10 ` Greg KH 1 sibling, 0 replies; 12+ messages in thread From: Gaurav Kohli @ 2019-04-05 11:43 UTC (permalink / raw) To: Greg KH; +Cc: tj, linux-kernel, linux-arm-msm, Mukesh Ojha On 4/5/2019 5:03 PM, Greg KH wrote: > On Fri, Apr 05, 2019 at 04:51:07PM +0530, Gaurav Kohli wrote: >> While adding kernfs node for child to the parent kernfs >> node and when child node founds that parent kn count is >> zero, then below comes like: >> >> WARNING: fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 >> >> This indicates that parent is in kernfs_put path/ or already >> freed, and if the child node keeps continue to >> make new kernfs node, then there is chance of >> below race for parent node: >> >> CPU0 CPU1 >> //Parent node //child node >> kernfs_put >> atomic_dec_and_test(&kn->count) >> //count is 0, so continue >> kernfs_new_node(child) >> kernfs_get(parent); >> //increment parent count to 1 >> //warning come as parent count is 0 >> /* link in */ >> kernfs_add_one(kn); >> // this should fail as parent is >> //in free path. >> kernfs_put(child) >> kmem_cache_free(parent) >> kmem_cache_free(child) >> kn = parent >> atomic_dec_and_test(&kn->count)) >> //this is 0 now, so release will >> continue for parent. >> kmem_cache_free(parent) >> >> To prevent this race, child simply has to decrement count of parent >> kernfs node and keep continue the free path for itself. >> >> Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org> >> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> >> >> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c >> index b84d635..d5a36e8 100644 >> --- a/fs/kernfs/dir.c >> +++ b/fs/kernfs/dir.c >> @@ -515,7 +515,6 @@ void kernfs_put(struct kernfs_node *kn) >> if (!kn || !atomic_dec_and_test(&kn->count)) >> return; >> root = kernfs_root(kn); >> - repeat: >> /* >> * Moving/renaming is always done while holding reference. >> * kn->parent won't change beneath us. >> @@ -545,8 +544,8 @@ void kernfs_put(struct kernfs_node *kn) >> >> kn = parent; >> if (kn) { >> - if (atomic_dec_and_test(&kn->count)) >> - goto repeat; >> + /* Parent may be on free path, so simply decrement the count */ > That's the wrong indentation :( > > And how are you hitting this issue? What user of kernfs is causing > this? Hi Greg, Thanks, will fix comment indentation, seen during sys-executor running: We have only one instance , In logs below warning also came: WARNING: CPU: 4 kernel/msm-4.14/fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 which indicated parent is in put path. [ 160.125151] Disabling lock debugging due to kernel taint [ 160.130626] INFO: Allocated in __kernfs_new_node+0x8c/0x3c0 age=11 cpu=2 pid=7098 [ 160.138314] kmem_cache_alloc+0x358/0x388 [ 160.142445] __kernfs_new_node+0x8c/0x3c0 [ 160.146590] kernfs_new_node+0x80/0xc8 [ 160.150462] kernfs_create_dir_ns+0x44/0xfc [ 160.154777] sysfs_create_dir_ns+0xa8/0x130 [ 160.158416] CPU5: update max cpu_capacity 1024 [ 160.159085] kobject_add_internal+0x278/0x650 [ 160.163567] kobject_add_varg+0xe0/0x130 [ 160.167606] kobject_add+0x15c/0x1d0 [ 160.168452] CPU5: update max cpu_capacity 780 [ 160.171287] get_device_parent+0x2d0/0x34c [ 160.175510] device_add+0x240/0xde0 [ 160.178371] CPU6: update max cpu_capacity 916 [ 160.179108] input_register_device+0x5f4/0xa0c [ 160.183686] uinput_ioctl_handler+0x1184/0x2198 [ 160.202436] INFO: Freed in kernfs_put+0x2c8/0x434 age=14 cpu=0 pid=7096 [ 160.209230] kernfs_put+0x2c8/0x434 [ 160.212825] kobject_del+0x50/0xcc [ 160.216332] cleanup_glue_dir+0x124/0x16c [ 160.220456] device_del+0x55c/0x5c8 [ 160.224047] __input_unregister_device+0x274/0x2a8 [ 160.228974] input_unregister_device+0x90/0xd0 [ 160.233553] uinput_destroy_device+0x15c/0x1dc [ 160.238131] uinput_release+0x44/0x5c [ 160.241898] __fput+0x1f4/0x4e4 [ 160.245127] ____fput+0x20/0x2c during code review, I have found race between kernfs parent put call and child get call. Regards Gaurav > > thanks, > > greg k-h -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v0] kernfs: Skip kernfs_put of parent from child node 2019-04-05 11:43 ` Gaurav Kohli 2019-04-05 11:43 ` Gaurav Kohli @ 2019-04-05 12:10 ` Greg KH 2019-04-05 12:10 ` Greg KH 2019-04-05 12:31 ` Mukesh Ojha 1 sibling, 2 replies; 12+ messages in thread From: Greg KH @ 2019-04-05 12:10 UTC (permalink / raw) To: Gaurav Kohli; +Cc: tj, linux-kernel, linux-arm-msm, Mukesh Ojha On Fri, Apr 05, 2019 at 05:13:00PM +0530, Gaurav Kohli wrote: > > On 4/5/2019 5:03 PM, Greg KH wrote: > > On Fri, Apr 05, 2019 at 04:51:07PM +0530, Gaurav Kohli wrote: > > > While adding kernfs node for child to the parent kernfs > > > node and when child node founds that parent kn count is > > > zero, then below comes like: > > > > > > WARNING: fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 > > > > > > This indicates that parent is in kernfs_put path/ or already > > > freed, and if the child node keeps continue to > > > make new kernfs node, then there is chance of > > > below race for parent node: > > > > > > CPU0 CPU1 > > > //Parent node //child node > > > kernfs_put > > > atomic_dec_and_test(&kn->count) > > > //count is 0, so continue > > > kernfs_new_node(child) > > > kernfs_get(parent); > > > //increment parent count to 1 > > > //warning come as parent count is 0 > > > /* link in */ > > > kernfs_add_one(kn); > > > // this should fail as parent is > > > //in free path. > > > kernfs_put(child) > > > kmem_cache_free(parent) > > > kmem_cache_free(child) > > > kn = parent > > > atomic_dec_and_test(&kn->count)) > > > //this is 0 now, so release will > > > continue for parent. > > > kmem_cache_free(parent) > > > > > > To prevent this race, child simply has to decrement count of parent > > > kernfs node and keep continue the free path for itself. > > > > > > Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org> > > > Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> > > > > > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > > > index b84d635..d5a36e8 100644 > > > --- a/fs/kernfs/dir.c > > > +++ b/fs/kernfs/dir.c > > > @@ -515,7 +515,6 @@ void kernfs_put(struct kernfs_node *kn) > > > if (!kn || !atomic_dec_and_test(&kn->count)) > > > return; > > > root = kernfs_root(kn); > > > - repeat: > > > /* > > > * Moving/renaming is always done while holding reference. > > > * kn->parent won't change beneath us. > > > @@ -545,8 +544,8 @@ void kernfs_put(struct kernfs_node *kn) > > > kn = parent; > > > if (kn) { > > > - if (atomic_dec_and_test(&kn->count)) > > > - goto repeat; > > > + /* Parent may be on free path, so simply decrement the count */ > > That's the wrong indentation :( > > > > And how are you hitting this issue? What user of kernfs is causing > > this? > > Hi Greg, > > Thanks, will fix comment indentation, seen during sys-executor running: > > We have only one instance , In logs below warning also came: > > WARNING: CPU: 4 kernel/msm-4.14/fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 > > which indicated parent is in put path. > > [ 160.125151] Disabling lock debugging due to kernel taint > [ 160.130626] INFO: Allocated in __kernfs_new_node+0x8c/0x3c0 age=11 cpu=2 > pid=7098 > [ 160.138314] kmem_cache_alloc+0x358/0x388 > [ 160.142445] __kernfs_new_node+0x8c/0x3c0 > [ 160.146590] kernfs_new_node+0x80/0xc8 > [ 160.150462] kernfs_create_dir_ns+0x44/0xfc > [ 160.154777] sysfs_create_dir_ns+0xa8/0x130 > [ 160.158416] CPU5: update max cpu_capacity 1024 > [ 160.159085] kobject_add_internal+0x278/0x650 > [ 160.163567] kobject_add_varg+0xe0/0x130 > [ 160.167606] kobject_add+0x15c/0x1d0 > [ 160.168452] CPU5: update max cpu_capacity 780 > [ 160.171287] get_device_parent+0x2d0/0x34c > [ 160.175510] device_add+0x240/0xde0 > [ 160.178371] CPU6: update max cpu_capacity 916 > [ 160.179108] input_register_device+0x5f4/0xa0c > [ 160.183686] uinput_ioctl_handler+0x1184/0x2198 > [ 160.202436] INFO: Freed in kernfs_put+0x2c8/0x434 age=14 cpu=0 pid=7096 > [ 160.209230] kernfs_put+0x2c8/0x434 > [ 160.212825] kobject_del+0x50/0xcc > [ 160.216332] cleanup_glue_dir+0x124/0x16c > [ 160.220456] device_del+0x55c/0x5c8 > [ 160.224047] __input_unregister_device+0x274/0x2a8 > [ 160.228974] input_unregister_device+0x90/0xd0 > [ 160.233553] uinput_destroy_device+0x15c/0x1dc > [ 160.238131] uinput_release+0x44/0x5c > [ 160.241898] __fput+0x1f4/0x4e4 > [ 160.245127] ____fput+0x20/0x2c > > > during code review, I have found race between kernfs parent put call and > child get call. So this is a sysfs usage of this? Using input devices or cpu devices for the stress test? thanks, greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v0] kernfs: Skip kernfs_put of parent from child node 2019-04-05 12:10 ` Greg KH @ 2019-04-05 12:10 ` Greg KH 2019-04-05 12:31 ` Mukesh Ojha 1 sibling, 0 replies; 12+ messages in thread From: Greg KH @ 2019-04-05 12:10 UTC (permalink / raw) To: Gaurav Kohli; +Cc: tj, linux-kernel, linux-arm-msm, Mukesh Ojha [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 4409 bytes --] On Fri, Apr 05, 2019 at 05:13:00PM +0530, Gaurav Kohli wrote: > > On 4/5/2019 5:03 PM, Greg KH wrote: > > On Fri, Apr 05, 2019 at 04:51:07PM +0530, Gaurav Kohli wrote: > > > While adding kernfs node for child to the parent kernfs > > > node and when child node founds that parent kn count is > > > zero, then below comes like: > > > > > > WARNING: fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 > > > > > > This indicates that parent is in kernfs_put path/ or already > > > freed, and if the child node keeps continue to > > > make new kernfs node, then there is chance of > > > below race for parent node: > > > > > > CPU0 CPU1 > > > //Parent node //child node > > > kernfs_put > > > atomic_dec_and_test(&kn->count) > > > //count is 0, so continue > > > kernfs_new_node(child) > > > kernfs_get(parent); > > > //increment parent count to 1 > > > //warning come as parent count is 0 > > > /* link in */ > > > kernfs_add_one(kn); > > > // this should fail as parent is > > > //in free path. > > > kernfs_put(child) > > > kmem_cache_free(parent) > > > kmem_cache_free(child) > > > kn = parent > > > atomic_dec_and_test(&kn->count)) > > > //this is 0 now, so release will > > > continue for parent. > > > kmem_cache_free(parent) > > > > > > To prevent this race, child simply has to decrement count of parent > > > kernfs node and keep continue the free path for itself. > > > > > > Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org> > > > Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> > > > > > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > > > index b84d635..d5a36e8 100644 > > > --- a/fs/kernfs/dir.c > > > +++ b/fs/kernfs/dir.c > > > @@ -515,7 +515,6 @@ void kernfs_put(struct kernfs_node *kn) > > > if (!kn || !atomic_dec_and_test(&kn->count)) > > > return; > > > root = kernfs_root(kn); > > > - repeat: > > > /* > > > * Moving/renaming is always done while holding reference. > > > * kn->parent won't change beneath us. > > > @@ -545,8 +544,8 @@ void kernfs_put(struct kernfs_node *kn) > > > kn = parent; > > > if (kn) { > > > - if (atomic_dec_and_test(&kn->count)) > > > - goto repeat; > > > + /* Parent may be on free path, so simply decrement the count */ > > That's the wrong indentation :( > > > > And how are you hitting this issue? What user of kernfs is causing > > this? > > Hi Greg, > > Thanks, will fix comment indentation, seen during sys-executor running: > > We have only one instance , In logs below warning also came: > > WARNING: CPU: 4 kernel/msm-4.14/fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 > > which indicated parent is in put path. > > [ 160.125151] Disabling lock debugging due to kernel taint > [ 160.130626] INFO: Allocated in __kernfs_new_node+0x8c/0x3c0 age=11 cpu=2 > pid=7098 > [ 160.138314] kmem_cache_alloc+0x358/0x388 > [ 160.142445] __kernfs_new_node+0x8c/0x3c0 > [ 160.146590] kernfs_new_node+0x80/0xc8 > [ 160.150462] kernfs_create_dir_ns+0x44/0xfc > [ 160.154777] sysfs_create_dir_ns+0xa8/0x130 > [ 160.158416] CPU5: update max cpu_capacity 1024 > [ 160.159085] kobject_add_internal+0x278/0x650 > [ 160.163567] kobject_add_varg+0xe0/0x130 > [ 160.167606] kobject_add+0x15c/0x1d0 > [ 160.168452] CPU5: update max cpu_capacity 780 > [ 160.171287] get_device_parent+0x2d0/0x34c > [ 160.175510] device_add+0x240/0xde0 > [ 160.178371] CPU6: update max cpu_capacity 916 > [ 160.179108] input_register_device+0x5f4/0xa0c > [ 160.183686] uinput_ioctl_handler+0x1184/0x2198 > [ 160.202436] INFO: Freed in kernfs_put+0x2c8/0x434 age=14 cpu=0 pid=7096 > [ 160.209230] kernfs_put+0x2c8/0x434 > [ 160.212825] kobject_del+0x50/0xcc > [ 160.216332] cleanup_glue_dir+0x124/0x16c > [ 160.220456] device_del+0x55c/0x5c8 > [ 160.224047] __input_unregister_device+0x274/0x2a8 > [ 160.228974] input_unregister_device+0x90/0xd0 > [ 160.233553] uinput_destroy_device+0x15c/0x1dc > [ 160.238131] uinput_release+0x44/0x5c > [ 160.241898] __fput+0x1f4/0x4e4 > [ 160.245127] ____fput+0x20/0x2c > > > during code review, I have found race between kernfs parent put call and > child get call. So this is a sysfs usage of this? Using input devices or cpu devices for the stress test? thanks, greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v0] kernfs: Skip kernfs_put of parent from child node 2019-04-05 12:10 ` Greg KH 2019-04-05 12:10 ` Greg KH @ 2019-04-05 12:31 ` Mukesh Ojha 2019-04-05 12:31 ` Mukesh Ojha 2019-04-08 10:16 ` Gaurav Kohli 1 sibling, 2 replies; 12+ messages in thread From: Mukesh Ojha @ 2019-04-05 12:31 UTC (permalink / raw) To: Greg KH, Gaurav Kohli; +Cc: tj, linux-kernel, linux-arm-msm On 4/5/2019 5:40 PM, Greg KH wrote: > On Fri, Apr 05, 2019 at 05:13:00PM +0530, Gaurav Kohli wrote: >> On 4/5/2019 5:03 PM, Greg KH wrote: >>> On Fri, Apr 05, 2019 at 04:51:07PM +0530, Gaurav Kohli wrote: >>>> While adding kernfs node for child to the parent kernfs >>>> node and when child node founds that parent kn count is >>>> zero, then below comes like: >>>> >>>> WARNING: fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 >>>> >>>> This indicates that parent is in kernfs_put path/ or already >>>> freed, and if the child node keeps continue to >>>> make new kernfs node, then there is chance of >>>> below race for parent node: >>>> >>>> CPU0 CPU1 >>>> //Parent node //child node >>>> kernfs_put >>>> atomic_dec_and_test(&kn->count) >>>> //count is 0, so continue >>>> kernfs_new_node(child) >>>> kernfs_get(parent); >>>> //increment parent count to 1 >>>> //warning come as parent count is 0 >>>> /* link in */ >>>> kernfs_add_one(kn); >>>> // this should fail as parent is >>>> //in free path. >>>> kernfs_put(child) >>>> kmem_cache_free(parent) >>>> kmem_cache_free(child) >>>> kn = parent >>>> atomic_dec_and_test(&kn->count)) >>>> //this is 0 now, so release will >>>> continue for parent. >>>> kmem_cache_free(parent) >>>> >>>> To prevent this race, child simply has to decrement count of parent >>>> kernfs node and keep continue the free path for itself. >>>> >>>> Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org> >>>> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> >>>> >>>> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c >>>> index b84d635..d5a36e8 100644 >>>> --- a/fs/kernfs/dir.c >>>> +++ b/fs/kernfs/dir.c >>>> @@ -515,7 +515,6 @@ void kernfs_put(struct kernfs_node *kn) >>>> if (!kn || !atomic_dec_and_test(&kn->count)) >>>> return; >>>> root = kernfs_root(kn); >>>> - repeat: >>>> /* >>>> * Moving/renaming is always done while holding reference. >>>> * kn->parent won't change beneath us. >>>> @@ -545,8 +544,8 @@ void kernfs_put(struct kernfs_node *kn) >>>> kn = parent; >>>> if (kn) { >>>> - if (atomic_dec_and_test(&kn->count)) >>>> - goto repeat; >>>> + /* Parent may be on free path, so simply decrement the count */ >>> That's the wrong indentation :( >>> >>> And how are you hitting this issue? What user of kernfs is causing >>> this? >> Hi Greg, >> >> Thanks, will fix comment indentation, seen during sys-executor running: >> >> We have only one instance , In logs below warning also came: >> >> WARNING: CPU: 4 kernel/msm-4.14/fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 >> >> which indicated parent is in put path. >> >> [ 160.125151] Disabling lock debugging due to kernel taint >> [ 160.130626] INFO: Allocated in __kernfs_new_node+0x8c/0x3c0 age=11 cpu=2 >> pid=7098 >> [ 160.138314] kmem_cache_alloc+0x358/0x388 >> [ 160.142445] __kernfs_new_node+0x8c/0x3c0 >> [ 160.146590] kernfs_new_node+0x80/0xc8 >> [ 160.150462] kernfs_create_dir_ns+0x44/0xfc >> [ 160.154777] sysfs_create_dir_ns+0xa8/0x130 >> [ 160.158416] CPU5: update max cpu_capacity 1024 >> [ 160.159085] kobject_add_internal+0x278/0x650 >> [ 160.163567] kobject_add_varg+0xe0/0x130 >> [ 160.167606] kobject_add+0x15c/0x1d0 >> [ 160.168452] CPU5: update max cpu_capacity 780 >> [ 160.171287] get_device_parent+0x2d0/0x34c >> [ 160.175510] device_add+0x240/0xde0 >> [ 160.178371] CPU6: update max cpu_capacity 916 >> [ 160.179108] input_register_device+0x5f4/0xa0c >> [ 160.183686] uinput_ioctl_handler+0x1184/0x2198 >> [ 160.202436] INFO: Freed in kernfs_put+0x2c8/0x434 age=14 cpu=0 pid=7096 >> [ 160.209230] kernfs_put+0x2c8/0x434 >> [ 160.212825] kobject_del+0x50/0xcc >> [ 160.216332] cleanup_glue_dir+0x124/0x16c >> [ 160.220456] device_del+0x55c/0x5c8 >> [ 160.224047] __input_unregister_device+0x274/0x2a8 >> [ 160.228974] input_unregister_device+0x90/0xd0 >> [ 160.233553] uinput_destroy_device+0x15c/0x1dc >> [ 160.238131] uinput_release+0x44/0x5c >> [ 160.241898] __fput+0x1f4/0x4e4 >> [ 160.245127] ____fput+0x20/0x2c >> >> >> during code review, I have found race between kernfs parent put call and >> child get call. > So this is a sysfs usage of this? > yes > Using input devices or cpu devices > for the stress test? input devices .. [ 1714.090310] input: syz1 as /devices/virtual/input/input191 [ 1714.223037] input: syz1 as /devices/virtual/input/input192 .. [ 1714.428228] input: syz1 as /devices/virtual/input/input193 .. [ 1714.528256] input: syz1 as /devices/virtual/input/input194 .. [ 1714.756481] input: syz1 as /devices/virtual/input/input195 .. [ 1714.831920] input: syz1 as /devices/virtual/input/input196 .. Cheers, Mukesh > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v0] kernfs: Skip kernfs_put of parent from child node 2019-04-05 12:31 ` Mukesh Ojha @ 2019-04-05 12:31 ` Mukesh Ojha 2019-04-08 10:16 ` Gaurav Kohli 1 sibling, 0 replies; 12+ messages in thread From: Mukesh Ojha @ 2019-04-05 12:31 UTC (permalink / raw) To: Greg KH, Gaurav Kohli; +Cc: tj, linux-kernel, linux-arm-msm On 4/5/2019 5:40 PM, Greg KH wrote: > On Fri, Apr 05, 2019 at 05:13:00PM +0530, Gaurav Kohli wrote: >> On 4/5/2019 5:03 PM, Greg KH wrote: >>> On Fri, Apr 05, 2019 at 04:51:07PM +0530, Gaurav Kohli wrote: >>>> While adding kernfs node for child to the parent kernfs >>>> node and when child node founds that parent kn count is >>>> zero, then below comes like: >>>> >>>> WARNING: fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 >>>> >>>> This indicates that parent is in kernfs_put path/ or already >>>> freed, and if the child node keeps continue to >>>> make new kernfs node, then there is chance of >>>> below race for parent node: >>>> >>>> CPU0 CPU1 >>>> //Parent node //child node >>>> kernfs_put >>>> atomic_dec_and_test(&kn->count) >>>> //count is 0, so continue >>>> kernfs_new_node(child) >>>> kernfs_get(parent); >>>> //increment parent count to 1 >>>> //warning come as parent count is 0 >>>> /* link in */ >>>> kernfs_add_one(kn); >>>> // this should fail as parent is >>>> //in free path. >>>> kernfs_put(child) >>>> kmem_cache_free(parent) >>>> kmem_cache_free(child) >>>> kn = parent >>>> atomic_dec_and_test(&kn->count)) >>>> //this is 0 now, so release will >>>> continue for parent. >>>> kmem_cache_free(parent) >>>> >>>> To prevent this race, child simply has to decrement count of parent >>>> kernfs node and keep continue the free path for itself. >>>> >>>> Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org> >>>> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> >>>> >>>> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c >>>> index b84d635..d5a36e8 100644 >>>> --- a/fs/kernfs/dir.c >>>> +++ b/fs/kernfs/dir.c >>>> @@ -515,7 +515,6 @@ void kernfs_put(struct kernfs_node *kn) >>>> if (!kn || !atomic_dec_and_test(&kn->count)) >>>> return; >>>> root = kernfs_root(kn); >>>> - repeat: >>>> /* >>>> * Moving/renaming is always done while holding reference. >>>> * kn->parent won't change beneath us. >>>> @@ -545,8 +544,8 @@ void kernfs_put(struct kernfs_node *kn) >>>> kn = parent; >>>> if (kn) { >>>> - if (atomic_dec_and_test(&kn->count)) >>>> - goto repeat; >>>> + /* Parent may be on free path, so simply decrement the count */ >>> That's the wrong indentation :( >>> >>> And how are you hitting this issue? What user of kernfs is causing >>> this? >> Hi Greg, >> >> Thanks, will fix comment indentation, seen during sys-executor running: >> >> We have only one instance , In logs below warning also came: >> >> WARNING: CPU: 4 kernel/msm-4.14/fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 >> >> which indicated parent is in put path. >> >> [ 160.125151] Disabling lock debugging due to kernel taint >> [ 160.130626] INFO: Allocated in __kernfs_new_node+0x8c/0x3c0 age=11 cpu=2 >> pid=7098 >> [ 160.138314] kmem_cache_alloc+0x358/0x388 >> [ 160.142445] __kernfs_new_node+0x8c/0x3c0 >> [ 160.146590] kernfs_new_node+0x80/0xc8 >> [ 160.150462] kernfs_create_dir_ns+0x44/0xfc >> [ 160.154777] sysfs_create_dir_ns+0xa8/0x130 >> [ 160.158416] CPU5: update max cpu_capacity 1024 >> [ 160.159085] kobject_add_internal+0x278/0x650 >> [ 160.163567] kobject_add_varg+0xe0/0x130 >> [ 160.167606] kobject_add+0x15c/0x1d0 >> [ 160.168452] CPU5: update max cpu_capacity 780 >> [ 160.171287] get_device_parent+0x2d0/0x34c >> [ 160.175510] device_add+0x240/0xde0 >> [ 160.178371] CPU6: update max cpu_capacity 916 >> [ 160.179108] input_register_device+0x5f4/0xa0c >> [ 160.183686] uinput_ioctl_handler+0x1184/0x2198 >> [ 160.202436] INFO: Freed in kernfs_put+0x2c8/0x434 age=14 cpu=0 pid=7096 >> [ 160.209230] kernfs_put+0x2c8/0x434 >> [ 160.212825] kobject_del+0x50/0xcc >> [ 160.216332] cleanup_glue_dir+0x124/0x16c >> [ 160.220456] device_del+0x55c/0x5c8 >> [ 160.224047] __input_unregister_device+0x274/0x2a8 >> [ 160.228974] input_unregister_device+0x90/0xd0 >> [ 160.233553] uinput_destroy_device+0x15c/0x1dc >> [ 160.238131] uinput_release+0x44/0x5c >> [ 160.241898] __fput+0x1f4/0x4e4 >> [ 160.245127] ____fput+0x20/0x2c >> >> >> during code review, I have found race between kernfs parent put call and >> child get call. > So this is a sysfs usage of this? > yes > Using input devices or cpu devices > for the stress test? input devices .. [ 1714.090310] input: syz1 as /devices/virtual/input/input191 [ 1714.223037] input: syz1 as /devices/virtual/input/input192 .. [ 1714.428228] input: syz1 as /devices/virtual/input/input193 .. [ 1714.528256] input: syz1 as /devices/virtual/input/input194 .. [ 1714.756481] input: syz1 as /devices/virtual/input/input195 .. [ 1714.831920] input: syz1 as /devices/virtual/input/input196 .. Cheers, Mukesh > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v0] kernfs: Skip kernfs_put of parent from child node 2019-04-05 12:31 ` Mukesh Ojha 2019-04-05 12:31 ` Mukesh Ojha @ 2019-04-08 10:16 ` Gaurav Kohli 2019-04-08 10:16 ` Gaurav Kohli 1 sibling, 1 reply; 12+ messages in thread From: Gaurav Kohli @ 2019-04-08 10:16 UTC (permalink / raw) To: Mukesh Ojha, Greg KH; +Cc: tj, linux-kernel, linux-arm-msm On 4/5/2019 6:01 PM, Mukesh Ojha wrote: > > On 4/5/2019 5:40 PM, Greg KH wrote: >> On Fri, Apr 05, 2019 at 05:13:00PM +0530, Gaurav Kohli wrote: >>> On 4/5/2019 5:03 PM, Greg KH wrote: >>>> On Fri, Apr 05, 2019 at 04:51:07PM +0530, Gaurav Kohli wrote: >>>>> While adding kernfs node for child to the parent kernfs >>>>> node and when child node founds that parent kn count is >>>>> zero, then below comes like: >>>>> >>>>> WARNING: fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 >>>>> >>>>> This indicates that parent is in kernfs_put path/ or already >>>>> freed, and if the child node keeps continue to >>>>> make new kernfs node, then there is chance of >>>>> below race for parent node: >>>>> >>>>> CPU0 CPU1 >>>>> //Parent node //child node >>>>> kernfs_put >>>>> atomic_dec_and_test(&kn->count) >>>>> //count is 0, so continue >>>>> kernfs_new_node(child) >>>>> kernfs_get(parent); >>>>> //increment parent count to 1 >>>>> //warning come as parent count is 0 >>>>> /* link in */ >>>>> kernfs_add_one(kn); >>>>> // this should fail as parent is >>>>> //in free path. >>>>> kernfs_put(child) >>>>> kmem_cache_free(parent) >>>>> kmem_cache_free(child) >>>>> kn = parent >>>>> atomic_dec_and_test(&kn->count)) >>>>> //this is 0 now, so release will >>>>> continue for parent. >>>>> kmem_cache_free(parent) >>>>> >>>>> To prevent this race, child simply has to decrement count of parent >>>>> kernfs node and keep continue the free path for itself. >>>>> >>>>> Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org> >>>>> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> >>>>> >>>>> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c >>>>> index b84d635..d5a36e8 100644 >>>>> --- a/fs/kernfs/dir.c >>>>> +++ b/fs/kernfs/dir.c >>>>> @@ -515,7 +515,6 @@ void kernfs_put(struct kernfs_node *kn) >>>>> if (!kn || !atomic_dec_and_test(&kn->count)) >>>>> return; >>>>> root = kernfs_root(kn); >>>>> - repeat: >>>>> /* >>>>> * Moving/renaming is always done while holding reference. >>>>> * kn->parent won't change beneath us. >>>>> @@ -545,8 +544,8 @@ void kernfs_put(struct kernfs_node *kn) >>>>> kn = parent; >>>>> if (kn) { >>>>> - if (atomic_dec_and_test(&kn->count)) >>>>> - goto repeat; >>>>> + /* Parent may be on free path, so simply decrement the count */ >>>> That's the wrong indentation :( >>>> >>>> And how are you hitting this issue? What user of kernfs is causing >>>> this? >>> Hi Greg, >>> >>> Thanks, will fix comment indentation, seen during sys-executor >>> running: >>> >>> We have only one instance , In logs below warning also came: >>> >>> WARNING: CPU: 4 kernel/msm-4.14/fs/kernfs/dir.c:494 >>> kernfs_get+0x64/0x88 >>> >>> which indicated parent is in put path. >>> >>> [ 160.125151] Disabling lock debugging due to kernel taint >>> [ 160.130626] INFO: Allocated in __kernfs_new_node+0x8c/0x3c0 >>> age=11 cpu=2 >>> pid=7098 >>> [ 160.138314] kmem_cache_alloc+0x358/0x388 >>> [ 160.142445] __kernfs_new_node+0x8c/0x3c0 >>> [ 160.146590] kernfs_new_node+0x80/0xc8 >>> [ 160.150462] kernfs_create_dir_ns+0x44/0xfc >>> [ 160.154777] sysfs_create_dir_ns+0xa8/0x130 >>> [ 160.158416] CPU5: update max cpu_capacity 1024 >>> [ 160.159085] kobject_add_internal+0x278/0x650 >>> [ 160.163567] kobject_add_varg+0xe0/0x130 >>> [ 160.167606] kobject_add+0x15c/0x1d0 >>> [ 160.168452] CPU5: update max cpu_capacity 780 >>> [ 160.171287] get_device_parent+0x2d0/0x34c >>> [ 160.175510] device_add+0x240/0xde0 >>> [ 160.178371] CPU6: update max cpu_capacity 916 >>> [ 160.179108] input_register_device+0x5f4/0xa0c >>> [ 160.183686] uinput_ioctl_handler+0x1184/0x2198 >>> [ 160.202436] INFO: Freed in kernfs_put+0x2c8/0x434 age=14 cpu=0 >>> pid=7096 >>> [ 160.209230] kernfs_put+0x2c8/0x434 >>> [ 160.212825] kobject_del+0x50/0xcc >>> [ 160.216332] cleanup_glue_dir+0x124/0x16c >>> [ 160.220456] device_del+0x55c/0x5c8 >>> [ 160.224047] __input_unregister_device+0x274/0x2a8 >>> [ 160.228974] input_unregister_device+0x90/0xd0 >>> [ 160.233553] uinput_destroy_device+0x15c/0x1dc >>> [ 160.238131] uinput_release+0x44/0x5c >>> [ 160.241898] __fput+0x1f4/0x4e4 >>> [ 160.245127] ____fput+0x20/0x2c >>> >>> >>> during code review, I have found race between kernfs parent put call >>> and >>> child get call. >> So this is a sysfs usage of this? >> > yes > >> Using input devices or cpu devices >> for the stress test? > > input devices .. > > [ 1714.090310] input: syz1 as /devices/virtual/input/input191 > [ 1714.223037] input: syz1 as /devices/virtual/input/input192 > > .. > > [ 1714.428228] input: syz1 as /devices/virtual/input/input193 > > .. > [ 1714.528256] input: syz1 as /devices/virtual/input/input194 > .. > > [ 1714.756481] input: syz1 as /devices/virtual/input/input195 > .. > [ 1714.831920] input: syz1 as /devices/virtual/input/input196 > > .. > > Cheers, > Mukesh Hi Greg, Tejun, With earlier patch that i have posted, there is chance of memory leak for parent, if below case occurs: Add parent Add child put parent put child -> this will skip the freeing of parent node with above patch. So to avoid race mentioned in earlier mail, creation of kernfs should not proceed, if count of parent is zero, Instead of warning, we should return from that place. Can you please check below patch for the same, or please let us know some other way to fix the race. diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c index 89d1dc1..c41085a 100644 --- a/fs/kernfs/dir.c +++ b/fs/kernfs/dir.c @@ -676,6 +676,11 @@ struct kernfs_node *kernfs_new_node(struct kernfs_node *parent, { struct kernfs_node *kn; + if (!atomic_read(&parent->count)) { + WARN_ON(1); + return NULL; + } + kn = __kernfs_new_node(kernfs_root(parent), name, mode, flags); if (kn) { kernfs_get(parent); Regards Gaurav > >> >> thanks, >> >> greg k-h -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v0] kernfs: Skip kernfs_put of parent from child node 2019-04-08 10:16 ` Gaurav Kohli @ 2019-04-08 10:16 ` Gaurav Kohli 0 siblings, 0 replies; 12+ messages in thread From: Gaurav Kohli @ 2019-04-08 10:16 UTC (permalink / raw) To: Mukesh Ojha, Greg KH; +Cc: tj, linux-kernel, linux-arm-msm On 4/5/2019 6:01 PM, Mukesh Ojha wrote: > > On 4/5/2019 5:40 PM, Greg KH wrote: >> On Fri, Apr 05, 2019 at 05:13:00PM +0530, Gaurav Kohli wrote: >>> On 4/5/2019 5:03 PM, Greg KH wrote: >>>> On Fri, Apr 05, 2019 at 04:51:07PM +0530, Gaurav Kohli wrote: >>>>> While adding kernfs node for child to the parent kernfs >>>>> node and when child node founds that parent kn count is >>>>> zero, then below comes like: >>>>> >>>>> WARNING: fs/kernfs/dir.c:494 kernfs_get+0x64/0x88 >>>>> >>>>> This indicates that parent is in kernfs_put path/ or already >>>>> freed, and if the child node keeps continue to >>>>> make new kernfs node, then there is chance of >>>>> below race for parent node: >>>>> >>>>> CPU0 CPU1 >>>>> //Parent node //child node >>>>> kernfs_put >>>>> atomic_dec_and_test(&kn->count) >>>>> //count is 0, so continue >>>>> kernfs_new_node(child) >>>>> kernfs_get(parent); >>>>> //increment parent count to 1 >>>>> //warning come as parent count is 0 >>>>> /* link in */ >>>>> kernfs_add_one(kn); >>>>> // this should fail as parent is >>>>> //in free path. >>>>> kernfs_put(child) >>>>> kmem_cache_free(parent) >>>>> kmem_cache_free(child) >>>>> kn = parent >>>>> atomic_dec_and_test(&kn->count)) >>>>> //this is 0 now, so release will >>>>> continue for parent. >>>>> kmem_cache_free(parent) >>>>> >>>>> To prevent this race, child simply has to decrement count of parent >>>>> kernfs node and keep continue the free path for itself. >>>>> >>>>> Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org> >>>>> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> >>>>> >>>>> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c >>>>> index b84d635..d5a36e8 100644 >>>>> --- a/fs/kernfs/dir.c >>>>> +++ b/fs/kernfs/dir.c >>>>> @@ -515,7 +515,6 @@ void kernfs_put(struct kernfs_node *kn) >>>>> if (!kn || !atomic_dec_and_test(&kn->count)) >>>>> return; >>>>> root = kernfs_root(kn); >>>>> - repeat: >>>>> /* >>>>> * Moving/renaming is always done while holding reference. >>>>> * kn->parent won't change beneath us. >>>>> @@ -545,8 +544,8 @@ void kernfs_put(struct kernfs_node *kn) >>>>> kn = parent; >>>>> if (kn) { >>>>> - if (atomic_dec_and_test(&kn->count)) >>>>> - goto repeat; >>>>> + /* Parent may be on free path, so simply decrement the count */ >>>> That's the wrong indentation :( >>>> >>>> And how are you hitting this issue? What user of kernfs is causing >>>> this? >>> Hi Greg, >>> >>> Thanks, will fix comment indentation, seen during sys-executor >>> running: >>> >>> We have only one instance , In logs below warning also came: >>> >>> WARNING: CPU: 4 kernel/msm-4.14/fs/kernfs/dir.c:494 >>> kernfs_get+0x64/0x88 >>> >>> which indicated parent is in put path. >>> >>> [ 160.125151] Disabling lock debugging due to kernel taint >>> [ 160.130626] INFO: Allocated in __kernfs_new_node+0x8c/0x3c0 >>> age=11 cpu=2 >>> pid=7098 >>> [ 160.138314] kmem_cache_alloc+0x358/0x388 >>> [ 160.142445] __kernfs_new_node+0x8c/0x3c0 >>> [ 160.146590] kernfs_new_node+0x80/0xc8 >>> [ 160.150462] kernfs_create_dir_ns+0x44/0xfc >>> [ 160.154777] sysfs_create_dir_ns+0xa8/0x130 >>> [ 160.158416] CPU5: update max cpu_capacity 1024 >>> [ 160.159085] kobject_add_internal+0x278/0x650 >>> [ 160.163567] kobject_add_varg+0xe0/0x130 >>> [ 160.167606] kobject_add+0x15c/0x1d0 >>> [ 160.168452] CPU5: update max cpu_capacity 780 >>> [ 160.171287] get_device_parent+0x2d0/0x34c >>> [ 160.175510] device_add+0x240/0xde0 >>> [ 160.178371] CPU6: update max cpu_capacity 916 >>> [ 160.179108] input_register_device+0x5f4/0xa0c >>> [ 160.183686] uinput_ioctl_handler+0x1184/0x2198 >>> [ 160.202436] INFO: Freed in kernfs_put+0x2c8/0x434 age=14 cpu=0 >>> pid=7096 >>> [ 160.209230] kernfs_put+0x2c8/0x434 >>> [ 160.212825] kobject_del+0x50/0xcc >>> [ 160.216332] cleanup_glue_dir+0x124/0x16c >>> [ 160.220456] device_del+0x55c/0x5c8 >>> [ 160.224047] __input_unregister_device+0x274/0x2a8 >>> [ 160.228974] input_unregister_device+0x90/0xd0 >>> [ 160.233553] uinput_destroy_device+0x15c/0x1dc >>> [ 160.238131] uinput_release+0x44/0x5c >>> [ 160.241898] __fput+0x1f4/0x4e4 >>> [ 160.245127] ____fput+0x20/0x2c >>> >>> >>> during code review, I have found race between kernfs parent put call >>> and >>> child get call. >> So this is a sysfs usage of this? >> > yes > >> Using input devices or cpu devices >> for the stress test? > > input devices .. > > [ 1714.090310] input: syz1 as /devices/virtual/input/input191 > [ 1714.223037] input: syz1 as /devices/virtual/input/input192 > > .. > > [ 1714.428228] input: syz1 as /devices/virtual/input/input193 > > .. > [ 1714.528256] input: syz1 as /devices/virtual/input/input194 > .. > > [ 1714.756481] input: syz1 as /devices/virtual/input/input195 > .. > [ 1714.831920] input: syz1 as /devices/virtual/input/input196 > > .. > > Cheers, > Mukesh Hi Greg, Tejun, With earlier patch that i have posted, there is chance of memory leak for parent, if below case occurs: Add parent Add child put parent put child -> this will skip the freeing of parent node with above patch. So to avoid race mentioned in earlier mail, creation of kernfs should not proceed, if count of parent is zero, Instead of warning, we should return from that place. Can you please check below patch for the same, or please let us know some other way to fix the race. diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c index 89d1dc1..c41085a 100644 --- a/fs/kernfs/dir.c +++ b/fs/kernfs/dir.c @@ -676,6 +676,11 @@ struct kernfs_node *kernfs_new_node(struct kernfs_node *parent, { struct kernfs_node *kn; + if (!atomic_read(&parent->count)) { + WARN_ON(1); + return NULL; + } + kn = __kernfs_new_node(kernfs_root(parent), name, mode, flags); if (kn) { kernfs_get(parent); Regards Gaurav > >> >> thanks, >> >> greg k-h -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-04-08 10:16 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-05 11:21 [PATCH v0] kernfs: Skip kernfs_put of parent from child node Gaurav Kohli 2019-04-05 11:21 ` Gaurav Kohli 2019-04-05 11:33 ` Greg KH 2019-04-05 11:33 ` Greg KH 2019-04-05 11:43 ` Gaurav Kohli 2019-04-05 11:43 ` Gaurav Kohli 2019-04-05 12:10 ` Greg KH 2019-04-05 12:10 ` Greg KH 2019-04-05 12:31 ` Mukesh Ojha 2019-04-05 12:31 ` Mukesh Ojha 2019-04-08 10:16 ` Gaurav Kohli 2019-04-08 10:16 ` Gaurav Kohli
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).