* [PATCH] Correct the race condition in aarch64_insn_patch_text_sync() @ 2014-11-10 16:36 William Cohen 2014-11-10 17:08 ` Will Deacon 0 siblings, 1 reply; 7+ messages in thread From: William Cohen @ 2014-11-10 16:36 UTC (permalink / raw) To: linux-arm-kernel When experimenting with patches to provide kprobes support for aarch64 smp machines would hang when inserting breakpoints into kernel code. The hangs were caused by a race condition in the code called by aarch64_insn_patch_text_sync(). The first processor in the aarch64_insn_patch_text_cb() function would patch the code while other processors were still entering the function and decrementing the cpu_count field. This resulted in some processors never observing the exit condition and exiting the function. Thus, processors in the system hung. The patching function now waits for all processors to enter the patching function before changing code to ensure that none of the processors are in code that is going to be patched. Once all the processors have entered the function, the last processor to enter the patching function performs the pathing and signals that the patching is complete with one last decrement of the cpu_count field to make it -1. Signed-off-by: William Cohen <wcohen@redhat.com> --- arch/arm64/kernel/insn.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c index e007714..e6266db 100644 --- a/arch/arm64/kernel/insn.c +++ b/arch/arm64/kernel/insn.c @@ -153,8 +153,10 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg) int i, ret = 0; struct aarch64_insn_patch *pp = arg; - /* The first CPU becomes master */ - if (atomic_inc_return(&pp->cpu_count) == 1) { + /* Make sure all the processors are in this function + before patching the code. The last CPU to this function + does the update. */ + if (atomic_dec_return(&pp->cpu_count) == 0) { for (i = 0; ret == 0 && i < pp->insn_cnt; i++) ret = aarch64_insn_patch_text_nosync(pp->text_addrs[i], pp->new_insns[i]); @@ -163,7 +165,8 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg) * which ends with "dsb; isb" pair guaranteeing global * visibility. */ - atomic_set(&pp->cpu_count, -1); + /* Notifiy other processors with an additional decrement. */ + atomic_dec(&pp->cpu_count); } else { while (atomic_read(&pp->cpu_count) != -1) cpu_relax(); @@ -185,6 +188,7 @@ int __kprobes aarch64_insn_patch_text_sync(void *addrs[], u32 insns[], int cnt) if (cnt <= 0) return -EINVAL; + atomic_set(&patch.cpu_count, num_online_cpus()); return stop_machine(aarch64_insn_patch_text_cb, &patch, cpu_online_mask); } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] Correct the race condition in aarch64_insn_patch_text_sync() 2014-11-10 16:36 [PATCH] Correct the race condition in aarch64_insn_patch_text_sync() William Cohen @ 2014-11-10 17:08 ` Will Deacon 2014-11-10 19:37 ` William Cohen 0 siblings, 1 reply; 7+ messages in thread From: Will Deacon @ 2014-11-10 17:08 UTC (permalink / raw) To: linux-arm-kernel Hi Will, Thanks for the tracking this down. On Mon, Nov 10, 2014 at 04:36:02PM +0000, William Cohen wrote: > When experimenting with patches to provide kprobes support for aarch64 > smp machines would hang when inserting breakpoints into kernel code. > The hangs were caused by a race condition in the code called by > aarch64_insn_patch_text_sync(). The first processor in the > aarch64_insn_patch_text_cb() function would patch the code while other > processors were still entering the function and decrementing the s/decrementing/incrementing/ > cpu_count field. This resulted in some processors never observing the > exit condition and exiting the function. Thus, processors in the > system hung. > > The patching function now waits for all processors to enter the > patching function before changing code to ensure that none of the > processors are in code that is going to be patched. Once all the > processors have entered the function, the last processor to enter the > patching function performs the pathing and signals that the patching > is complete with one last decrement of the cpu_count field to make it > -1. > > Signed-off-by: William Cohen <wcohen@redhat.com> > --- > arch/arm64/kernel/insn.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > index e007714..e6266db 100644 > --- a/arch/arm64/kernel/insn.c > +++ b/arch/arm64/kernel/insn.c > @@ -153,8 +153,10 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg) > int i, ret = 0; > struct aarch64_insn_patch *pp = arg; > > - /* The first CPU becomes master */ > - if (atomic_inc_return(&pp->cpu_count) == 1) { > + /* Make sure all the processors are in this function > + before patching the code. The last CPU to this function > + does the update. */ > + if (atomic_dec_return(&pp->cpu_count) == 0) { > for (i = 0; ret == 0 && i < pp->insn_cnt; i++) > ret = aarch64_insn_patch_text_nosync(pp->text_addrs[i], > pp->new_insns[i]); > @@ -163,7 +165,8 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg) > * which ends with "dsb; isb" pair guaranteeing global > * visibility. > */ > - atomic_set(&pp->cpu_count, -1); > + /* Notifiy other processors with an additional decrement. */ > + atomic_dec(&pp->cpu_count); > } else { > while (atomic_read(&pp->cpu_count) != -1) > cpu_relax(); > @@ -185,6 +188,7 @@ int __kprobes aarch64_insn_patch_text_sync(void *addrs[], u32 insns[], int cnt) > if (cnt <= 0) > return -EINVAL; > > + atomic_set(&patch.cpu_count, num_online_cpus()); I think this is still racy with hotplug before stop_machine has done get_online_cpus. How about we leave the increment in the callback and change the exit condition to compare with num_online_cpus() instead? Cheers, Will ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Correct the race condition in aarch64_insn_patch_text_sync() 2014-11-10 17:08 ` Will Deacon @ 2014-11-10 19:37 ` William Cohen 2014-11-11 11:28 ` Will Deacon 0 siblings, 1 reply; 7+ messages in thread From: William Cohen @ 2014-11-10 19:37 UTC (permalink / raw) To: linux-arm-kernel On 11/10/2014 12:08 PM, Will Deacon wrote: > Hi Will, > > Thanks for the tracking this down. > > On Mon, Nov 10, 2014 at 04:36:02PM +0000, William Cohen wrote: >> When experimenting with patches to provide kprobes support for aarch64 >> smp machines would hang when inserting breakpoints into kernel code. >> The hangs were caused by a race condition in the code called by >> aarch64_insn_patch_text_sync(). The first processor in the >> aarch64_insn_patch_text_cb() function would patch the code while other >> processors were still entering the function and decrementing the > > s/decrementing/incrementing/ > >> cpu_count field. This resulted in some processors never observing the >> exit condition and exiting the function. Thus, processors in the >> system hung. >> >> The patching function now waits for all processors to enter the >> patching function before changing code to ensure that none of the >> processors are in code that is going to be patched. Once all the >> processors have entered the function, the last processor to enter the >> patching function performs the pathing and signals that the patching >> is complete with one last decrement of the cpu_count field to make it >> -1. >> >> Signed-off-by: William Cohen <wcohen@redhat.com> >> --- >> arch/arm64/kernel/insn.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c >> index e007714..e6266db 100644 >> --- a/arch/arm64/kernel/insn.c >> +++ b/arch/arm64/kernel/insn.c >> @@ -153,8 +153,10 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg) >> int i, ret = 0; >> struct aarch64_insn_patch *pp = arg; >> >> - /* The first CPU becomes master */ >> - if (atomic_inc_return(&pp->cpu_count) == 1) { >> + /* Make sure all the processors are in this functionaarch64_insn_patch_text_cb( >> + before patching the code. The last CPU to this function >> + does the update. */ >> + if (atomic_dec_return(&pp->cpu_count) == 0) { >> for (i = 0; ret == 0 && i < pp->insn_cnt; i++) >> ret = aarch64_insn_patch_text_nosync(pp->text_addrs[i], >> pp->new_insns[i]); >> @@ -163,7 +165,8 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg) >> * which ends with "dsb; isb" pair guaranteeing global >> * visibility. >> */ >> - atomic_set(&pp->cpu_count, -1); >> + /* Notifiy other processors with an additional decrement. */ >> + atomic_dec(&pp->cpu_count); >> } else { >> while (atomic_read(&pp->cpu_count) != -1) >> cpu_relax(); >> @@ -185,6 +188,7 @@ int __kprobes aarch64_insn_patch_text_sync(void *addrs[], u32 insns[], int cnt) >> if (cnt <= 0) >> return -EINVAL; >> >> + atomic_set(&patch.cpu_count, num_online_cpus()); > > I think this is still racy with hotplug before stop_machine has done > get_online_cpus. How about we leave the increment in the callback and change > the exit condition to compare with num_online_cpus() instead? > > Cheers, > > Will > Hi Will, Thanks for the feedback. I am no expert in the corner cases involved with hotplug. Dave Long suggested something similar with num_online_cpus in the arch64_insn_patch_text_cb() and using increments and checking the num_cpus_online() inside aarch64_insn_patch_text_cb(). Moving the num_cpu_online() inside the aarch64_insn_patch_text_cb() is sufficient to avoid race conditions with hotplug? If so, would the attached patch be appropriate? -Will Cohen -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Correct-the-race-condition-in-aarch64_insn_patch_tex.patch Type: text/x-patch Size: 2578 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141110/d2385862/attachment.bin> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Correct the race condition in aarch64_insn_patch_text_sync() 2014-11-10 19:37 ` William Cohen @ 2014-11-11 11:28 ` Will Deacon 2014-11-11 14:48 ` William Cohen 0 siblings, 1 reply; 7+ messages in thread From: Will Deacon @ 2014-11-11 11:28 UTC (permalink / raw) To: linux-arm-kernel On Mon, Nov 10, 2014 at 07:37:24PM +0000, William Cohen wrote: > On 11/10/2014 12:08 PM, Will Deacon wrote: > > On Mon, Nov 10, 2014 at 04:36:02PM +0000, William Cohen wrote: > >> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > >> index e007714..e6266db 100644 > >> --- a/arch/arm64/kernel/insn.c > >> +++ b/arch/arm64/kernel/insn.c > >> @@ -153,8 +153,10 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg) > >> int i, ret = 0; > >> struct aarch64_insn_patch *pp = arg; > >> > >> - /* The first CPU becomes master */ > >> - if (atomic_inc_return(&pp->cpu_count) == 1) { > >> + /* Make sure all the processors are in this functionaarch64_insn_patch_text_cb( > >> + before patching the code. The last CPU to this function > >> + does the update. */ > >> + if (atomic_dec_return(&pp->cpu_count) == 0) { > >> for (i = 0; ret == 0 && i < pp->insn_cnt; i++) > >> ret = aarch64_insn_patch_text_nosync(pp->text_addrs[i], > >> pp->new_insns[i]); > >> @@ -163,7 +165,8 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg) > >> * which ends with "dsb; isb" pair guaranteeing global > >> * visibility. > >> */ > >> - atomic_set(&pp->cpu_count, -1); > >> + /* Notifiy other processors with an additional decrement. */ > >> + atomic_dec(&pp->cpu_count); > >> } else { > >> while (atomic_read(&pp->cpu_count) != -1) > >> cpu_relax(); > >> @@ -185,6 +188,7 @@ int __kprobes aarch64_insn_patch_text_sync(void *addrs[], u32 insns[], int cnt) > >> if (cnt <= 0) > >> return -EINVAL; > >> > >> + atomic_set(&patch.cpu_count, num_online_cpus()); > > > > I think this is still racy with hotplug before stop_machine has done > > get_online_cpus. How about we leave the increment in the callback and change > > the exit condition to compare with num_online_cpus() instead? > > Thanks for the feedback. I am no expert in the corner cases involved with > hotplug. Dave Long suggested something similar with num_online_cpus in > the arch64_insn_patch_text_cb() and using increments and checking the > num_cpus_online() inside aarch64_insn_patch_text_cb(). Moving the > num_cpu_online() inside the aarch64_insn_patch_text_cb() is sufficient to > avoid race conditions with hotplug? If so, would the attached patch be > appropriate? Yes, because stop_machine() does {get,put}_online_cpus() around the invocation. > From d02e3244c436234d0d07500be6d4df64feb2052a Mon Sep 17 00:00:00 2001 > From: William Cohen <wcohen@redhat.com> > Date: Mon, 10 Nov 2014 14:26:44 -0500 > Subject: [PATCH] Correct the race condition in aarch64_insn_patch_text_sync() > > When experimenting with patches to provide kprobes support for aarch64 > smp machines would hang when inserting breakpoints into kernel code. > The hangs were caused by a race condition in the code called by > aarch64_insn_patch_text_sync(). The first processor in the > aarch64_insn_patch_text_cb() function would patch the code while other > processors were still entering the function and incrementing the > cpu_count field. This resulted in some processors never observing the > exit condition and exiting the function. Thus, processors in the > system hung. > > The patching function now waits for all processors to enter the > patching function before changing code to ensure that none of the > processors are in code that is going to be patched. Once all the > processors have entered the function, the last processor to enter the > patching function performs the patching and signals that the patching > is complete with one last increment of the cpu_count field to make it > num_cpus_online()+1. > > Signed-off-by: William Cohen <wcohen@redhat.com> > --- > arch/arm64/kernel/insn.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > index e007714..4fdddf1 100644 > --- a/arch/arm64/kernel/insn.c > +++ b/arch/arm64/kernel/insn.c > @@ -151,10 +151,13 @@ struct aarch64_insn_patch { > static int __kprobes aarch64_insn_patch_text_cb(void *arg) > { > int i, ret = 0; > + int count = num_online_cpus(); > struct aarch64_insn_patch *pp = arg; > > - /* The first CPU becomes master */ > - if (atomic_inc_return(&pp->cpu_count) == 1) { > + /* Make sure all the processors are in this function > + before patching the code. The last CPU to this function > + does the update. */ > + if (atomic_inc_return(&pp->cpu_count) == count) { Actually, you can leave this hunk alone and leave the first CPU to do the patching. > for (i = 0; ret == 0 && i < pp->insn_cnt; i++) > ret = aarch64_insn_patch_text_nosync(pp->text_addrs[i], > pp->new_insns[i]); > @@ -163,9 +166,10 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg) > * which ends with "dsb; isb" pair guaranteeing global > * visibility. > */ > - atomic_set(&pp->cpu_count, -1); > + /* Notifiy other processors with an additional increment. */ Notify > + atomic_inc(&pp->cpu_count); > } else { > - while (atomic_read(&pp->cpu_count) != -1) > + while (atomic_read(&pp->cpu_count) <= count) > cpu_relax(); Then make this 'cpu_count <= num_online_cpus()' Will ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Correct the race condition in aarch64_insn_patch_text_sync() 2014-11-11 11:28 ` Will Deacon @ 2014-11-11 14:48 ` William Cohen 2014-11-11 17:51 ` Will Deacon 0 siblings, 1 reply; 7+ messages in thread From: William Cohen @ 2014-11-11 14:48 UTC (permalink / raw) To: linux-arm-kernel On 11/11/2014 06:28 AM, Will Deacon wrote: > On Mon, Nov 10, 2014 at 07:37:24PM +0000, William Cohen wrote: >> On 11/10/2014 12:08 PM, Will Deacon wrote: >>> On Mon, Nov 10, 2014 at 04:36:02PM +0000, William Cohen wrote: >>>> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c >>>> index e007714..e6266db 100644 >>>> --- a/arch/arm64/kernel/insn.c >>>> +++ b/arch/arm64/kernel/insn.c >>>> @@ -153,8 +153,10 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg) >>>> int i, ret = 0; >>>> struct aarch64_insn_patch *pp = arg; >>>> >>>> - /* The first CPU becomes master */ >>>> - if (atomic_inc_return(&pp->cpu_count) == 1) { >>>> + /* Make sure all the processors are in this functionaarch64_insn_patch_text_cb( >>>> + before patching the code. The last CPU to this function >>>> + does the update. */ >>>> + if (atomic_dec_return(&pp->cpu_count) == 0) { >>>> for (i = 0; ret == 0 && i < pp->insn_cnt; i++) >>>> ret = aarch64_insn_patch_text_nosync(pp->text_addrs[i], >>>> pp->new_insns[i]); >>>> @@ -163,7 +165,8 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg) >>>> * which ends with "dsb; isb" pair guaranteeing global >>>> * visibility. >>>> */ >>>> - atomic_set(&pp->cpu_count, -1); >>>> + /* Notifiy other processors with an additional decrement. */ >>>> + atomic_dec(&pp->cpu_count); >>>> } else { >>>> while (atomic_read(&pp->cpu_count) != -1) >>>> cpu_relax(); >>>> @@ -185,6 +188,7 @@ int __kprobes aarch64_insn_patch_text_sync(void *addrs[], u32 insns[], int cnt) >>>> if (cnt <= 0) >>>> return -EINVAL; >>>> >>>> + atomic_set(&patch.cpu_count, num_online_cpus()); >>> >>> I think this is still racy with hotplug before stop_machine has done >>> get_online_cpus. How about we leave the increment in the callback and change >>> the exit condition to compare with num_online_cpus() instead? >> >> Thanks for the feedback. I am no expert in the corner cases involved with >> hotplug. Dave Long suggested something similar with num_online_cpus in >> the arch64_insn_patch_text_cb() and using increments and checking the >> num_cpus_online() inside aarch64_insn_patch_text_cb(). Moving the >> num_cpu_online() inside the aarch64_insn_patch_text_cb() is sufficient to >> avoid race conditions with hotplug? If so, would the attached patch be >> appropriate? > > Yes, because stop_machine() does {get,put}_online_cpus() around the > invocation. > >> From d02e3244c436234d0d07500be6d4df64feb2052a Mon Sep 17 00:00:00 2001 >> From: William Cohen <wcohen@redhat.com> >> Date: Mon, 10 Nov 2014 14:26:44 -0500 >> Subject: [PATCH] Correct the race condition in aarch64_insn_patch_text_sync() >> >> When experimenting with patches to provide kprobes support for aarch64 >> smp machines would hang when inserting breakpoints into kernel code. >> The hangs were caused by a race condition in the code called by >> aarch64_insn_patch_text_sync(). The first processor in the >> aarch64_insn_patch_text_cb() function would patch the code while other >> processors were still entering the function and incrementing the >> cpu_count field. This resulted in some processors never observing the >> exit condition and exiting the function. Thus, processors in the >> system hung. >> >> The patching function now waits for all processors to enter the >> patching function before changing code to ensure that none of the >> processors are in code that is going to be patched. Once all the >> processors have entered the function, the last processor to enter the >> patching function performs the patching and signals that the patching >> is complete with one last increment of the cpu_count field to make it >> num_cpus_online()+1. >> >> Signed-off-by: William Cohen <wcohen@redhat.com> >> --- >> arch/arm64/kernel/insn.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c >> index e007714..4fdddf1 100644 >> --- a/arch/arm64/kernel/insn.c >> +++ b/arch/arm64/kernel/insn.c >> @@ -151,10 +151,13 @@ struct aarch64_insn_patch { >> static int __kprobes aarch64_insn_patch_text_cb(void *arg) >> { >> int i, ret = 0; >> + int count = num_online_cpus(); >> struct aarch64_insn_patch *pp = arg; >> >> - /* The first CPU becomes master */ >> - if (atomic_inc_return(&pp->cpu_count) == 1) { >> + /* Make sure all the processors are in this function >> + before patching the code. The last CPU to this function >> + does the update. */ >> + if (atomic_inc_return(&pp->cpu_count) == count) { > > Actually, you can leave this hunk alone and leave the first CPU to do the > patching. Hi Will, If it doesn't matter which processor is doing the update, do the processors all need to wait for the last one to get to this function before continuing on? Or would it be acceptable to allow processors to continue once the first processor completes the patch operation? That could reduce the amount of time that processors spin waiting for other processors to enter arch64_insn_patch_text_cb. > >> for (i = 0; ret == 0 && i < pp->insn_cnt; i++) >> ret = aarch64_insn_patch_text_nosync(pp->text_addrs[i], >> pp->new_insns[i]); >> @@ -163,9 +166,10 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg) >> * which ends with "dsb; isb" pair guaranteeing global >> * visibility. >> */ >> - atomic_set(&pp->cpu_count, -1); >> + /* Notifiy other processors with an additional increment. */ > > Notify > >> + atomic_inc(&pp->cpu_count); >> } else { >> - while (atomic_read(&pp->cpu_count) != -1) >> + while (atomic_read(&pp->cpu_count) <= count) >> cpu_relax(); > > Then make this 'cpu_count <= num_online_cpus()' > > Will > Attached is a patch that addresses the current comment. -Will Cohen -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Correct-the-race-condition-in-aarch64_insn_patch_tex.patch Type: text/x-patch Size: 1831 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141111/11f7050b/attachment.bin> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Correct the race condition in aarch64_insn_patch_text_sync() 2014-11-11 14:48 ` William Cohen @ 2014-11-11 17:51 ` Will Deacon 2014-11-13 15:14 ` Catalin Marinas 0 siblings, 1 reply; 7+ messages in thread From: Will Deacon @ 2014-11-11 17:51 UTC (permalink / raw) To: linux-arm-kernel On Tue, Nov 11, 2014 at 02:48:29PM +0000, William Cohen wrote: > On 11/11/2014 06:28 AM, Will Deacon wrote: > > On Mon, Nov 10, 2014 at 07:37:24PM +0000, William Cohen wrote: > >> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > >> index e007714..4fdddf1 100644 > >> --- a/arch/arm64/kernel/insn.c > >> +++ b/arch/arm64/kernel/insn.c > >> @@ -151,10 +151,13 @@ struct aarch64_insn_patch { > >> static int __kprobes aarch64_insn_patch_text_cb(void *arg) > >> { > >> int i, ret = 0; > >> + int count = num_online_cpus(); > >> struct aarch64_insn_patch *pp = arg; > >> > >> - /* The first CPU becomes master */ > >> - if (atomic_inc_return(&pp->cpu_count) == 1) { > >> + /* Make sure all the processors are in this function > >> + before patching the code. The last CPU to this function > >> + does the update. */ > >> + if (atomic_inc_return(&pp->cpu_count) == count) { > > > > Actually, you can leave this hunk alone and leave the first CPU to do the > > patching. > > If it doesn't matter which processor is doing the update, do the > processors all need to wait for the last one to get to this function > before continuing on? Or would it be acceptable to allow processors to > continue once the first processor completes the patch operation? That > could reduce the amount of time that processors spin waiting for other > processors to enter arch64_insn_patch_text_cb. I don't think it will make a lot of difference, given the stop_machine completion. > Attached is a patch that addresses the current comment. Thanks, Will. > From 41c728aeee2185fd30ec6a8ba223a2caec875f47 Mon Sep 17 00:00:00 2001 > From: William Cohen <wcohen@redhat.com> > Date: Tue, 11 Nov 2014 09:41:27 -0500 > Subject: [PATCH] Correct the race condition in aarch64_insn_patch_text_sync() > > When experimenting with patches to provide kprobes support for aarch64 > smp machines would hang when inserting breakpoints into kernel code. > The hangs were caused by a race condition in the code called by > aarch64_insn_patch_text_sync(). The first processor in the > aarch64_insn_patch_text_cb() function would patch the code while other > processors were still entering the function and incrementing the > cpu_count field. This resulted in some processors never observing the > exit condition and exiting the function. Thus, processors in the > system hung. > > The first processor to enter the patching function performs the > patching and signals that the patching is complete with an increment > of the cpu_count field. When all the processors have incremented the > cpu_count field the cpu_count will be num_cpus_online()+1 and they > will return to normal execution. > > Signed-off-by: William Cohen <wcohen@redhat.com> > --- > arch/arm64/kernel/insn.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Acked-by: Will Deacon <will.deacon@arm.com> Catalin -- can you pick this into the fixes branch please? Will > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > index e007714..8cd27fe 100644 > --- a/arch/arm64/kernel/insn.c > +++ b/arch/arm64/kernel/insn.c > @@ -163,9 +163,10 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg) > * which ends with "dsb; isb" pair guaranteeing global > * visibility. > */ > - atomic_set(&pp->cpu_count, -1); > + /* Notify other processors with an additional increment. */ > + atomic_inc(&pp->cpu_count); > } else { > - while (atomic_read(&pp->cpu_count) != -1) > + while (atomic_read(&pp->cpu_count) <= num_online_cpus()) > cpu_relax(); > isb(); > } > -- > 1.8.3.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Correct the race condition in aarch64_insn_patch_text_sync() 2014-11-11 17:51 ` Will Deacon @ 2014-11-13 15:14 ` Catalin Marinas 0 siblings, 0 replies; 7+ messages in thread From: Catalin Marinas @ 2014-11-13 15:14 UTC (permalink / raw) To: linux-arm-kernel On Tue, Nov 11, 2014 at 05:51:33PM +0000, Will Deacon wrote: > On Tue, Nov 11, 2014 at 02:48:29PM +0000, William Cohen wrote: > > From 41c728aeee2185fd30ec6a8ba223a2caec875f47 Mon Sep 17 00:00:00 2001 > > From: William Cohen <wcohen@redhat.com> > > Date: Tue, 11 Nov 2014 09:41:27 -0500 > > Subject: [PATCH] Correct the race condition in aarch64_insn_patch_text_sync() > > > > When experimenting with patches to provide kprobes support for aarch64 > > smp machines would hang when inserting breakpoints into kernel code. > > The hangs were caused by a race condition in the code called by > > aarch64_insn_patch_text_sync(). The first processor in the > > aarch64_insn_patch_text_cb() function would patch the code while other > > processors were still entering the function and incrementing the > > cpu_count field. This resulted in some processors never observing the > > exit condition and exiting the function. Thus, processors in the > > system hung. > > > > The first processor to enter the patching function performs the > > patching and signals that the patching is complete with an increment > > of the cpu_count field. When all the processors have incremented the > > cpu_count field the cpu_count will be num_cpus_online()+1 and they > > will return to normal execution. > > > > Signed-off-by: William Cohen <wcohen@redhat.com> > > --- > > arch/arm64/kernel/insn.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > Acked-by: Will Deacon <will.deacon@arm.com> > > Catalin -- can you pick this into the fixes branch please? Applied (and added Fixes and Cc stable lines). Thanks. -- Catalin ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-11-13 15:14 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-10 16:36 [PATCH] Correct the race condition in aarch64_insn_patch_text_sync() William Cohen 2014-11-10 17:08 ` Will Deacon 2014-11-10 19:37 ` William Cohen 2014-11-11 11:28 ` Will Deacon 2014-11-11 14:48 ` William Cohen 2014-11-11 17:51 ` Will Deacon 2014-11-13 15:14 ` Catalin Marinas
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).