| Message ID | 20190110005001.106975-1-aik@ozlabs.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phb3: Deprecate reading the PHB status | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
On 10/1/19 11:50 am, Alexey Kardashevskiy wrote: > The OPAL_PCI_EEH_FREEZE_STATUS call takes a bunch of parameters, one of > them is @phb_status. It is defined as __be64* and always NULL in > the current Linux upstream but if anyone ever decides to read that status, > then the PHB3's handler will assume it is struct OpalIoPhb3ErrorData* > (which is a lot bigger than 8 bytes) and zero it causing the stack > corruption. > > This makes @phb_status deprecated by copying the error message from PHB4. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Looks like p7ioc does the same thing. If the API is obviously wrong here, should we move the deprecation warning to core/pci-opal.c and just get rid of phb_status from the internal API? > --- > hw/phb3.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/phb3.c b/hw/phb3.c > index 771ccfc..38b8f46 100644 > --- a/hw/phb3.c > +++ b/hw/phb3.c > @@ -2772,8 +2772,7 @@ static int64_t phb3_eeh_freeze_status(struct phb *phb, uint64_t pe_number, > > bail: > if (phb_status) > - phb3_read_phb_status(p, > - (struct OpalIoPhb3ErrorData *)phb_status); > + PHBERR(p, "%s: deprecated PHB status\n", __func__); > > return OPAL_SUCCESS; > } >
On 10/01/2019 12:03, Andrew Donnellan wrote: > On 10/1/19 11:50 am, Alexey Kardashevskiy wrote: >> The OPAL_PCI_EEH_FREEZE_STATUS call takes a bunch of parameters, one of >> them is @phb_status. It is defined as __be64* and always NULL in >> the current Linux upstream but if anyone ever decides to read that >> status, >> then the PHB3's handler will assume it is struct OpalIoPhb3ErrorData* >> (which is a lot bigger than 8 bytes) and zero it causing the stack >> corruption. >> >> This makes @phb_status deprecated by copying the error message from PHB4. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > Looks like p7ioc does the same thing. If the API is obviously wrong > here, should we move the deprecation warning to core/pci-opal.c and just > get rid of phb_status from the internal API? May be. I am just not so sure how Stewart deprecates things in skiboot exactly :) > >> --- >> hw/phb3.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/hw/phb3.c b/hw/phb3.c >> index 771ccfc..38b8f46 100644 >> --- a/hw/phb3.c >> +++ b/hw/phb3.c >> @@ -2772,8 +2772,7 @@ static int64_t phb3_eeh_freeze_status(struct phb >> *phb, uint64_t pe_number, >> bail: >> if (phb_status) >> - phb3_read_phb_status(p, >> - (struct OpalIoPhb3ErrorData *)phb_status); >> + PHBERR(p, "%s: deprecated PHB status\n", __func__); >> return OPAL_SUCCESS; >> } >> >
diff --git a/hw/phb3.c b/hw/phb3.c index 771ccfc..38b8f46 100644 --- a/hw/phb3.c +++ b/hw/phb3.c @@ -2772,8 +2772,7 @@ static int64_t phb3_eeh_freeze_status(struct phb *phb, uint64_t pe_number, bail: if (phb_status) - phb3_read_phb_status(p, - (struct OpalIoPhb3ErrorData *)phb_status); + PHBERR(p, "%s: deprecated PHB status\n", __func__); return OPAL_SUCCESS; }
The OPAL_PCI_EEH_FREEZE_STATUS call takes a bunch of parameters, one of them is @phb_status. It is defined as __be64* and always NULL in the current Linux upstream but if anyone ever decides to read that status, then the PHB3's handler will assume it is struct OpalIoPhb3ErrorData* (which is a lot bigger than 8 bytes) and zero it causing the stack corruption. This makes @phb_status deprecated by copying the error message from PHB4. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- hw/phb3.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)