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 <[email protected]>
---
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
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
> -----Original Message-----
> From: Sudeep Holla [mailto:[email protected]]
> Sent: Friday, February 28, 2020 6:41 PM
> To: Zengtao (B)
> Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
> [email protected]; 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
On Sat, Feb 29, 2020 at 01:41:47AM +0000, Zengtao (B) wrote:
> > -----Original Message-----
> > From: Sudeep Holla [mailto:[email protected]]
> > Sent: Friday, February 28, 2020 6:41 PM
> > To: Zengtao (B)
> > Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
> > [email protected]; 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
> -----Original Message-----
> From: Sudeep Holla [mailto:[email protected]]
> Sent: Monday, March 02, 2020 7:11 PM
> To: Zengtao (B)
> Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
> [email protected]; 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:[email protected]]
> > > Sent: Friday, February 28, 2020 6:41 PM
> > > To: Zengtao (B)
> > > Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
> > > [email protected]; 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
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 <[email protected]>
--
Regards,
Sudeep