* [PATCH] cpu-topology: Fix the potential data corruption
@ 2020-02-28 8:35 Zeng Tao
2020-02-28 10:40 ` Sudeep Holla
0 siblings, 1 reply; 6+ messages in thread
From: Zeng Tao @ 2020-02-28 8:35 UTC (permalink / raw)
To: sudeep.holla
Cc: linuxarm, Zeng Tao, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel
Currently there are only 10 bytes to store the cpu-topology info.
That is:
snprintf(buffer, 10, "cluster%d",i);
snprintf(buffer, 10, "thread%d",i);
snprintf(buffer, 10, "core%d",i);
In the boundary test, if the cluster number exceeds 100, there will be a
data corrution, and the kernel will fall into dead loop. in the cluster
parse function.
So in this patch, enlarge the buffer to fix such potential issues.
Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com>
---
drivers/base/arch_topology.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 6119e11..f489883 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -281,7 +281,7 @@ static int __init get_cpu_for_node(struct device_node *node)
static int __init parse_core(struct device_node *core, int package_id,
int core_id)
{
- char name[10];
+ char name[20];
bool leaf = true;
int i = 0;
int cpu;
@@ -327,7 +327,7 @@ static int __init parse_core(struct device_node *core, int package_id,
static int __init parse_cluster(struct device_node *cluster, int depth)
{
- char name[10];
+ char name[20];
bool leaf = true;
bool has_cores = false;
struct device_node *c;
--
2.8.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] cpu-topology: Fix the potential data corruption
2020-02-28 8:35 [PATCH] cpu-topology: Fix the potential data corruption Zeng Tao
@ 2020-02-28 10:40 ` Sudeep Holla
2020-02-29 1:41 ` Zengtao (B)
0 siblings, 1 reply; 6+ messages in thread
From: Sudeep Holla @ 2020-02-28 10:40 UTC (permalink / raw)
To: Zeng Tao
Cc: linuxarm, Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Sudeep Holla
On Fri, Feb 28, 2020 at 04:35:45PM +0800, Zeng Tao wrote:
> Currently there are only 10 bytes to store the cpu-topology info.
> That is:
> snprintf(buffer, 10, "cluster%d",i);
> snprintf(buffer, 10, "thread%d",i);
> snprintf(buffer, 10, "core%d",i);
>
> In the boundary test, if the cluster number exceeds 100, there will be a
I don't understand you mention of 100 in particular above. I can see issue
if there are cluster with more than 2-digit id. Though highly unlikely for
now, but I don't have objection to the patch.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] cpu-topology: Fix the potential data corruption
2020-02-28 10:40 ` Sudeep Holla
@ 2020-02-29 1:41 ` Zengtao (B)
2020-03-02 11:10 ` Sudeep Holla
0 siblings, 1 reply; 6+ messages in thread
From: Zengtao (B) @ 2020-02-29 1:41 UTC (permalink / raw)
To: Sudeep Holla
Cc: Linuxarm, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel@vger.kernel.org
> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> Sent: Friday, February 28, 2020 6:41 PM
> To: Zengtao (B)
> Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
> linux-kernel@vger.kernel.org; Sudeep Holla
> Subject: Re: [PATCH] cpu-topology: Fix the potential data corruption
>
> On Fri, Feb 28, 2020 at 04:35:45PM +0800, Zeng Tao wrote:
> > Currently there are only 10 bytes to store the cpu-topology info.
> > That is:
> > snprintf(buffer, 10, "cluster%d",i);
> > snprintf(buffer, 10, "thread%d",i);
> > snprintf(buffer, 10, "core%d",i);
> >
> > In the boundary test, if the cluster number exceeds 100, there will be a
>
> I don't understand you mention of 100 in particular above. I can see
> issue
> if there are cluster with more than 2-digit id. Though highly unlikely for
> now, but I don't have objection to the patch.
>
The same meaning, more than 2-digit id equals to more than 100, right?
Here 100 is for from tester/user perspective.
And we found this issue when test with QEMU.
> --
> Regards,
> Sudeep
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpu-topology: Fix the potential data corruption
2020-02-29 1:41 ` Zengtao (B)
@ 2020-03-02 11:10 ` Sudeep Holla
2020-03-03 2:58 ` Zengtao (B)
0 siblings, 1 reply; 6+ messages in thread
From: Sudeep Holla @ 2020-03-02 11:10 UTC (permalink / raw)
To: Zengtao (B)
Cc: Linuxarm, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel@vger.kernel.org, Sudeep Holla
On Sat, Feb 29, 2020 at 01:41:47AM +0000, Zengtao (B) wrote:
> > -----Original Message-----
> > From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> > Sent: Friday, February 28, 2020 6:41 PM
> > To: Zengtao (B)
> > Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
> > linux-kernel@vger.kernel.org; Sudeep Holla
> > Subject: Re: [PATCH] cpu-topology: Fix the potential data corruption
> >
> > On Fri, Feb 28, 2020 at 04:35:45PM +0800, Zeng Tao wrote:
> > > Currently there are only 10 bytes to store the cpu-topology info.
> > > That is:
> > > snprintf(buffer, 10, "cluster%d",i);
> > > snprintf(buffer, 10, "thread%d",i);
> > > snprintf(buffer, 10, "core%d",i);
> > >
> > > In the boundary test, if the cluster number exceeds 100, there will be a
> >
> > I don't understand you mention of 100 in particular above. I can see
> > issue
> > if there are cluster with more than 2-digit id. Though highly unlikely for
> > now, but I don't have objection to the patch.
> >
>
> The same meaning, more than 2-digit id equals to more than 100, right?
Yes. May be it is obvious but I prefer to word the commit message accordingly.
Mention of 100 specifically makes at-least me think something very specific
to 100 and not applicable for any more than 2-digit number.
> Here 100 is for from tester/user perspective.
> And we found this issue when test with QEMU.
OK.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] cpu-topology: Fix the potential data corruption
2020-03-02 11:10 ` Sudeep Holla
@ 2020-03-03 2:58 ` Zengtao (B)
2020-03-03 10:08 ` Sudeep Holla
0 siblings, 1 reply; 6+ messages in thread
From: Zengtao (B) @ 2020-03-03 2:58 UTC (permalink / raw)
To: Sudeep Holla
Cc: Linuxarm, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel@vger.kernel.org
> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> Sent: Monday, March 02, 2020 7:11 PM
> To: Zengtao (B)
> Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
> linux-kernel@vger.kernel.org; Sudeep Holla
> Subject: Re: [PATCH] cpu-topology: Fix the potential data corruption
>
> On Sat, Feb 29, 2020 at 01:41:47AM +0000, Zengtao (B) wrote:
> > > -----Original Message-----
> > > From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> > > Sent: Friday, February 28, 2020 6:41 PM
> > > To: Zengtao (B)
> > > Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
> > > linux-kernel@vger.kernel.org; Sudeep Holla
> > > Subject: Re: [PATCH] cpu-topology: Fix the potential data corruption
> > >
> > > On Fri, Feb 28, 2020 at 04:35:45PM +0800, Zeng Tao wrote:
> > > > Currently there are only 10 bytes to store the cpu-topology info.
> > > > That is:
> > > > snprintf(buffer, 10, "cluster%d",i);
> > > > snprintf(buffer, 10, "thread%d",i);
> > > > snprintf(buffer, 10, "core%d",i);
> > > >
> > > > In the boundary test, if the cluster number exceeds 100, there will
> be a
> > >
> > > I don't understand you mention of 100 in particular above. I can see
> > > issue
> > > if there are cluster with more than 2-digit id. Though highly unlikely
> for
> > > now, but I don't have objection to the patch.
> > >
> >
> > The same meaning, more than 2-digit id equals to more than 100,
> right?
>
> Yes. May be it is obvious but I prefer to word the commit message
> accordingly.
> Mention of 100 specifically makes at-least me think something very
> specific
> to 100 and not applicable for any more than 2-digit number.
>
Do you think I need to update the commit message and resend the patch?
And I don't mind if you can help modify the commit message since both
are fine for me, and it's a very trivial change.
> > Here 100 is for from tester/user perspective.
> > And we found this issue when test with QEMU.
>
> OK.
>
> --
> Regards,
> Sudeep
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpu-topology: Fix the potential data corruption
2020-03-03 2:58 ` Zengtao (B)
@ 2020-03-03 10:08 ` Sudeep Holla
0 siblings, 0 replies; 6+ messages in thread
From: Sudeep Holla @ 2020-03-03 10:08 UTC (permalink / raw)
To: Zengtao (B)
Cc: Linuxarm, Greg Kroah-Hartman, Rafael J. Wysocki, Sudeep Holla,
linux-kernel@vger.kernel.org
On Tue, Mar 03, 2020 at 02:58:41AM +0000, Zengtao (B) wrote:
[...]
>
> Do you think I need to update the commit message and resend the patch?
Yes
> And I don't mind if you can help modify the commit message since both
> are fine for me, and it's a very trivial change.
>
Sure, something like below:
"Currently there are only 10 bytes to store the cpu-topology 'name'
information. Only 10 bytes copied into cluster/thread/core names.
If the cluster ID exceeds 2-digit number, it will result in the data
corruption, and ending up in a dead loop in the parsing routines. The
same applies to the thread names with more that 3-digit number.
This issue was found using the boundary tests under virtualised
environment like QEMU.
Let us increase the buffer to fix such potential issues."
With the above commit log, you can add my:
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-03 10:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-28 8:35 [PATCH] cpu-topology: Fix the potential data corruption Zeng Tao
2020-02-28 10:40 ` Sudeep Holla
2020-02-29 1:41 ` Zengtao (B)
2020-03-02 11:10 ` Sudeep Holla
2020-03-03 2:58 ` Zengtao (B)
2020-03-03 10:08 ` Sudeep Holla
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox