[AMD Official Use Only - General] Good point, as if (1 || 0) should short-circuit at the if (1) part. Thus we should go down to NOTIFY_DONE if m is NULL. Yazen can confirm here, as he was the one who with me when we found the original issue. It's possible that one of the 3 message fields was NULL instead of the actual message in our repro situation, but I'll double-check the short-circuit side of C to confirm. I know it short circuits for &&, I don't know if it does for ||. Kent -----Original Message----- From: Lazar, Lijo <Lijo.Lazar@xxxxxxx> Sent: Thursday, August 25, 2022 8:34 AM To: Russell, Kent <Kent.Russell@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx Cc: Ghannam, Yazen <Yazen.Ghannam@xxxxxxx> Subject: Re: [PATCH] drm/amdgpu: Handle potential NULL pointer dereference On 8/25/2022 5:16 PM, Russell, Kent wrote: > [AMD Official Use Only - General] > > Friendly ping > Wonder how it goes any further when m is NULL. It should do shortcut evaluation and return NOTIFY_DONE, right? Or is this for better readability? Thanks, Lijo > Kent > > -----Original Message----- > From: Russell, Kent <Kent.Russell@xxxxxxx> > Sent: Monday, August 15, 2022 11:31 AM > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx > Cc: Ghannam, Yazen <Yazen.Ghannam@xxxxxxx>; Russell, Kent > <Kent.Russell@xxxxxxx> > Subject: [PATCH] drm/amdgpu: Handle potential NULL pointer dereference > > If m is NULL, we will end up referencing a NULL pointer in the subsequent m elements like extcpu, bank and status. Pull the NULL check out and do it first before referencing m's elements. > > Signed-off-by: Kent Russell <kent.russell@xxxxxxx> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c > index ab9ba5a9c33d..028495fdfa62 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c > @@ -2838,12 +2838,15 @@ static int amdgpu_bad_page_notifier(struct notifier_block *nb, > struct eeprom_table_record err_rec; > uint64_t retired_page; > > + if (!m) > + return NOTIFY_DONE; > + > /* > * If the error was generated in UMC_V2, which belongs to GPU UMCs, > * and error occurred in DramECC (Extended error code = 0) then only > * process the error, else bail out. > */ > - if (!m || !((smca_get_bank_type(m->extcpu, m->bank) == SMCA_UMC_V2) && > + if (!((smca_get_bank_type(m->extcpu, m->bank) == SMCA_UMC_V2) && > (XEC(m->status, 0x3f) == 0x0))) > return NOTIFY_DONE; > > -- > 2.25.1 >
