| Message ID | 5a950883-d06d-4cdc-2716-8c1ce25f394d@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/drcinfo: Fix bugs 'ibm,drc-info' property | expand |
On 05/17/2018 05:41 PM, Michael Bringmann wrote: > [Replace/withdraw previous patch submission to ensure that testing > of related patches on similar hardware progresses together.] > > This patch fixes a memory parsing bug when using of_prop_next_u32 > calls at the start of a structure. Depending upon the value of > "cur" memory pointer argument to of_prop_next_u32, it will or it > won't advance the value of the returned memory pointer by the > size of one u32. This patch corrects the code to deal with that > indexing feature when parsing the ibm,drc-info structs for CPUs. > Also, need to advance the pointer at the end of_read_drc_info_cell > for same reason. > I see that you provide an update for of_read_drc_info_cell to fix the unexpected behavior you're seeing, but I'm not sure why you're updating the code in pseries_energy.c and rpaphp_core.c? can you provide some additional information as to why these functions also need to be updated. > Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com> > Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmware feature" -- end of patch series applied to powerpc next) > --- > Changes in V3: > -- Rebased patch to 4.17-rc5 kernel > --- > arch/powerpc/platforms/pseries/of_helpers.c | 5 ++--- > arch/powerpc/platforms/pseries/pseries_energy.c | 2 ++ > drivers/pci/hotplug/rpaphp_core.c | 1 + > 3 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c > index 6df192f..20598b2 100644 > --- a/arch/powerpc/platforms/pseries/of_helpers.c > +++ b/arch/powerpc/platforms/pseries/of_helpers.c > @@ -65,9 +65,7 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval, > > /* Get drc-index-start:encode-int */ > p2 = (const __be32 *)p; > - p2 = of_prop_next_u32(*prop, p2, &data->drc_index_start); > - if (!p2) > - return -EINVAL; > + data->drc_index_start = of_read_number(p2, 1); This appears to resolve advancing the pointer for the beginning of a struct. > > /* Get drc-name-suffix-start:encode-int */ > p2 = of_prop_next_u32(*prop, p2, &data->drc_name_suffix_start); > @@ -88,6 +86,7 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval, > p2 = of_prop_next_u32(*prop, p2, &data->drc_power_domain); > if (!p2) > return -EINVAL; > + p2++; ...but why is the advancement needed here? of_prop_next_u32 should have advanced it, correct? -Nathan > > /* Should now know end of current entry */ > (*curval) = (void *)p2; > diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c b/arch/powerpc/platforms/pseries/pseries_energy.c > index 6ed2212..c7d84aa 100644 > --- a/arch/powerpc/platforms/pseries/pseries_energy.c > +++ b/arch/powerpc/platforms/pseries/pseries_energy.c > @@ -64,6 +64,7 @@ static u32 cpu_to_drc_index(int cpu) > value = of_prop_next_u32(info, NULL, &num_set_entries); > if (!value) > goto err_of_node_put; > + value++; > > for (j = 0; j < num_set_entries; j++) { > > @@ -126,6 +127,7 @@ static int drc_index_to_cpu(u32 drc_index) > value = of_prop_next_u32(info, NULL, &num_set_entries); > if (!value) > goto err_of_node_put; > + value++; > > for (j = 0; j < num_set_entries; j++) { > > diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c > index fb5e084..dccdf62 100644 > --- a/drivers/pci/hotplug/rpaphp_core.c > +++ b/drivers/pci/hotplug/rpaphp_core.c > @@ -239,6 +239,7 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name, > value = of_prop_next_u32(info, NULL, &entries); > if (!value) > return -EINVAL; > + value++; > > for (j = 0; j < entries; j++) { > of_read_drc_info_cell(&info, &value, &drc); >
See below. On 05/18/2018 02:57 PM, Nathan Fontenot wrote: > On 05/17/2018 05:41 PM, Michael Bringmann wrote: >> [Replace/withdraw previous patch submission to ensure that testing >> of related patches on similar hardware progresses together.] >> >> This patch fixes a memory parsing bug when using of_prop_next_u32 >> calls at the start of a structure. Depending upon the value of >> "cur" memory pointer argument to of_prop_next_u32, it will or it >> won't advance the value of the returned memory pointer by the >> size of one u32. This patch corrects the code to deal with that >> indexing feature when parsing the ibm,drc-info structs for CPUs. >> Also, need to advance the pointer at the end of_read_drc_info_cell >> for same reason. >> > > I see that you provide an update for of_read_drc_info_cell to fix the > unexpected behavior you're seeing, but I'm not sure why you're updating > the code in pseries_energy.c and rpaphp_core.c? can you provide some > additional information as to why these functions also need to be updated. The changes are related. of_prop_next_u32() does not read a u32 and then advance the pointer. It advances the pointer and then reads a u32. It does an error check to see whether the u32 read is within the boundary of the property value, but it returns a pointer to the u32 that was read. > >> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com> >> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmware feature" -- end of patch series applied to powerpc next) >> --- >> Changes in V3: >> -- Rebased patch to 4.17-rc5 kernel >> --- >> arch/powerpc/platforms/pseries/of_helpers.c | 5 ++--- >> arch/powerpc/platforms/pseries/pseries_energy.c | 2 ++ >> drivers/pci/hotplug/rpaphp_core.c | 1 + >> 3 files changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c >> index 6df192f..20598b2 100644 >> --- a/arch/powerpc/platforms/pseries/of_helpers.c >> +++ b/arch/powerpc/platforms/pseries/of_helpers.c >> @@ -65,9 +65,7 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval, >> >> /* Get drc-index-start:encode-int */ >> p2 = (const __be32 *)p; >> - p2 = of_prop_next_u32(*prop, p2, &data->drc_index_start); >> - if (!p2) >> - return -EINVAL; >> + data->drc_index_start = of_read_number(p2, 1); > > This appears to resolve advancing the pointer for the beginning of a struct. > >> >> /* Get drc-name-suffix-start:encode-int */ >> p2 = of_prop_next_u32(*prop, p2, &data->drc_name_suffix_start); >> @@ -88,6 +86,7 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval, >> p2 = of_prop_next_u32(*prop, p2, &data->drc_power_domain); >> if (!p2) >> return -EINVAL; >> + p2++; > > ...but why is the advancement needed here? of_prop_next_u32 should have advanced it, correct? > > -Nathan > >> >> /* Should now know end of current entry */ >> (*curval) = (void *)p2; >> diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c b/arch/powerpc/platforms/pseries/pseries_energy.c >> index 6ed2212..c7d84aa 100644 >> --- a/arch/powerpc/platforms/pseries/pseries_energy.c >> +++ b/arch/powerpc/platforms/pseries/pseries_energy.c >> @@ -64,6 +64,7 @@ static u32 cpu_to_drc_index(int cpu) >> value = of_prop_next_u32(info, NULL, &num_set_entries); >> if (!value) >> goto err_of_node_put; >> + value++; >> >> for (j = 0; j < num_set_entries; j++) { >> >> @@ -126,6 +127,7 @@ static int drc_index_to_cpu(u32 drc_index) >> value = of_prop_next_u32(info, NULL, &num_set_entries); >> if (!value) >> goto err_of_node_put; >> + value++; >> >> for (j = 0; j < num_set_entries; j++) { >> >> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c >> index fb5e084..dccdf62 100644 >> --- a/drivers/pci/hotplug/rpaphp_core.c >> +++ b/drivers/pci/hotplug/rpaphp_core.c >> @@ -239,6 +239,7 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name, >> value = of_prop_next_u32(info, NULL, &entries); >> if (!value) >> return -EINVAL; >> + value++; >> >> for (j = 0; j < entries; j++) { >> of_read_drc_info_cell(&info, &value, &drc); >> > >
diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c index 6df192f..20598b2 100644 --- a/arch/powerpc/platforms/pseries/of_helpers.c +++ b/arch/powerpc/platforms/pseries/of_helpers.c @@ -65,9 +65,7 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval, /* Get drc-index-start:encode-int */ p2 = (const __be32 *)p; - p2 = of_prop_next_u32(*prop, p2, &data->drc_index_start); - if (!p2) - return -EINVAL; + data->drc_index_start = of_read_number(p2, 1); /* Get drc-name-suffix-start:encode-int */ p2 = of_prop_next_u32(*prop, p2, &data->drc_name_suffix_start); @@ -88,6 +86,7 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval, p2 = of_prop_next_u32(*prop, p2, &data->drc_power_domain); if (!p2) return -EINVAL; + p2++; /* Should now know end of current entry */ (*curval) = (void *)p2; diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c b/arch/powerpc/platforms/pseries/pseries_energy.c index 6ed2212..c7d84aa 100644 --- a/arch/powerpc/platforms/pseries/pseries_energy.c +++ b/arch/powerpc/platforms/pseries/pseries_energy.c @@ -64,6 +64,7 @@ static u32 cpu_to_drc_index(int cpu) value = of_prop_next_u32(info, NULL, &num_set_entries); if (!value) goto err_of_node_put; + value++; for (j = 0; j < num_set_entries; j++) { @@ -126,6 +127,7 @@ static int drc_index_to_cpu(u32 drc_index) value = of_prop_next_u32(info, NULL, &num_set_entries); if (!value) goto err_of_node_put; + value++; for (j = 0; j < num_set_entries; j++) { diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c index fb5e084..dccdf62 100644 --- a/drivers/pci/hotplug/rpaphp_core.c +++ b/drivers/pci/hotplug/rpaphp_core.c @@ -239,6 +239,7 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name, value = of_prop_next_u32(info, NULL, &entries); if (!value) return -EINVAL; + value++; for (j = 0; j < entries; j++) { of_read_drc_info_cell(&info, &value, &drc);
[Replace/withdraw previous patch submission to ensure that testing of related patches on similar hardware progresses together.] This patch fixes a memory parsing bug when using of_prop_next_u32 calls at the start of a structure. Depending upon the value of "cur" memory pointer argument to of_prop_next_u32, it will or it won't advance the value of the returned memory pointer by the size of one u32. This patch corrects the code to deal with that indexing feature when parsing the ibm,drc-info structs for CPUs. Also, need to advance the pointer at the end of_read_drc_info_cell for same reason. Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmware feature" -- end of patch series applied to powerpc next) --- Changes in V3: -- Rebased patch to 4.17-rc5 kernel --- arch/powerpc/platforms/pseries/of_helpers.c | 5 ++--- arch/powerpc/platforms/pseries/pseries_energy.c | 2 ++ drivers/pci/hotplug/rpaphp_core.c | 1 + 3 files changed, 5 insertions(+), 3 deletions(-)