| Message ID | Y91g081OauhQNxMe@ubun2204.myguest.virtualbox.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gt: Avoid redundant pointer validity check | expand |
Hi, Adding Matt & Thomas as potential candidates to review. Regards, Tvrtko On 03/02/2023 19:30, Deepak R Varma wrote: > The macro definition of gen6_for_all_pdes() expands to a for loop such > that it breaks when the page table is null. Hence there is no need to > again test validity of the page table entry pointers in the pde list. > This change is identified using itnull.cocci semantic patch. > > Signed-off-by: Deepak R Varma <drv@mailo.com> > --- > Please note: Proposed change is compile tested only. > > drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > index 5aaacc53fa4c..787b9e6d9f59 100644 > --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt *ppgtt) > u32 pde; > > gen6_for_all_pdes(pt, pd, pde) > - if (pt) > - free_pt(&ppgtt->base.vm, pt); > + free_pt(&ppgtt->base.vm, pt); > } > > static void gen6_ppgtt_cleanup(struct i915_address_space *vm) > @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct i915_address_space *vm, > > /* Free all no longer used page tables */ > gen6_for_all_pdes(pt, ppgtt->base.pd, pde) { > - if (!pt || atomic_read(&pt->used)) > + if (atomic_read(&pt->used)) > continue; > > free_pt(&ppgtt->base.vm, pt);
On 06/02/2023 09:45, Tvrtko Ursulin wrote: > > Hi, > > Adding Matt & Thomas as potential candidates to review. > > Regards, > > Tvrtko > > On 03/02/2023 19:30, Deepak R Varma wrote: >> The macro definition of gen6_for_all_pdes() expands to a for loop such >> that it breaks when the page table is null. Hence there is no need to >> again test validity of the page table entry pointers in the pde list. >> This change is identified using itnull.cocci semantic patch. >> >> Signed-off-by: Deepak R Varma <drv@mailo.com> >> --- >> Please note: Proposed change is compile tested only. >> >> drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c >> b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c >> index 5aaacc53fa4c..787b9e6d9f59 100644 >> --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c >> +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c >> @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt >> *ppgtt) >> u32 pde; >> gen6_for_all_pdes(pt, pd, pde) >> - if (pt) >> - free_pt(&ppgtt->base.vm, pt); >> + free_pt(&ppgtt->base.vm, pt); >> } >> static void gen6_ppgtt_cleanup(struct i915_address_space *vm) >> @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct >> i915_address_space *vm, >> /* Free all no longer used page tables */ >> gen6_for_all_pdes(pt, ppgtt->base.pd, pde) { >> - if (!pt || atomic_read(&pt->used)) >> + if (atomic_read(&pt->used)) Wow, I was really confused trying to remember how this all works. The gen6_for_all_pdes() does: (pt = i915_pt_entry(pd, iter), true) So NULL pt is expected, and does not 'break' here, since 'true' is always the value that decides whether to terminate the loop. So this patch would lead to NULL ptr deref, AFAICT. >> continue; >> free_pt(&ppgtt->base.vm, pt);
On Mon, Feb 06, 2023 at 10:33:13AM +0000, Matthew Auld wrote: > On 06/02/2023 09:45, Tvrtko Ursulin wrote: > > > > Hi, > > > > Adding Matt & Thomas as potential candidates to review. > > > > Regards, > > > > Tvrtko > > > > On 03/02/2023 19:30, Deepak R Varma wrote: > > > The macro definition of gen6_for_all_pdes() expands to a for loop such > > > that it breaks when the page table is null. Hence there is no need to > > > again test validity of the page table entry pointers in the pde list. > > > This change is identified using itnull.cocci semantic patch. > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > --- > > > Please note: Proposed change is compile tested only. > > > > > > drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++--- > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > > > b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > > > index 5aaacc53fa4c..787b9e6d9f59 100644 > > > --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > > > +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > > > @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt > > > *ppgtt) > > > u32 pde; > > > gen6_for_all_pdes(pt, pd, pde) > > > - if (pt) > > > - free_pt(&ppgtt->base.vm, pt); > > > + free_pt(&ppgtt->base.vm, pt); > > > } > > > static void gen6_ppgtt_cleanup(struct i915_address_space *vm) > > > @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct > > > i915_address_space *vm, > > > /* Free all no longer used page tables */ > > > gen6_for_all_pdes(pt, ppgtt->base.pd, pde) { > > > - if (!pt || atomic_read(&pt->used)) > > > + if (atomic_read(&pt->used)) > > Wow, I was really confused trying to remember how this all works. > > The gen6_for_all_pdes() does: > > (pt = i915_pt_entry(pd, iter), true) > > So NULL pt is expected, and does not 'break' here, since 'true' is always > the value that decides whether to terminate the loop. So this patch would > lead to NULL ptr deref, AFAICT. Hello Matt, I understand it now. I was misreading the true as part of the function argument. Could you please also comment if the implementation of gen6_ppgtt_free_pd() in the same file is safe? It doesn't appear to have an check on pt validity here. Thank you, deepak. > > > > > > continue; > > > free_pt(&ppgtt->base.vm, pt);
On Tue, Feb 07, 2023 at 12:12:18AM +0530, Deepak R Varma wrote: > On Mon, Feb 06, 2023 at 10:33:13AM +0000, Matthew Auld wrote: > > On 06/02/2023 09:45, Tvrtko Ursulin wrote: > > > > > > Hi, > > > > > > Adding Matt & Thomas as potential candidates to review. > > > > > > Regards, > > > > > > Tvrtko > > > > > > On 03/02/2023 19:30, Deepak R Varma wrote: > > > > The macro definition of gen6_for_all_pdes() expands to a for loop such > > > > that it breaks when the page table is null. Hence there is no need to > > > > again test validity of the page table entry pointers in the pde list. > > > > This change is identified using itnull.cocci semantic patch. > > > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > > --- > > > > Please note: Proposed change is compile tested only. > > > > > > > > drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++--- > > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > > > > b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > > > > index 5aaacc53fa4c..787b9e6d9f59 100644 > > > > --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > > > > +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > > > > @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt > > > > *ppgtt) > > > > u32 pde; > > > > gen6_for_all_pdes(pt, pd, pde) > > > > - if (pt) > > > > - free_pt(&ppgtt->base.vm, pt); > > > > + free_pt(&ppgtt->base.vm, pt); > > > > } > > > > static void gen6_ppgtt_cleanup(struct i915_address_space *vm) > > > > @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct > > > > i915_address_space *vm, > > > > /* Free all no longer used page tables */ > > > > gen6_for_all_pdes(pt, ppgtt->base.pd, pde) { > > > > - if (!pt || atomic_read(&pt->used)) > > > > + if (atomic_read(&pt->used)) > > > > Wow, I was really confused trying to remember how this all works. > > > > The gen6_for_all_pdes() does: > > > > (pt = i915_pt_entry(pd, iter), true) > > > > So NULL pt is expected, and does not 'break' here, since 'true' is always > > the value that decides whether to terminate the loop. So this patch would > > lead to NULL ptr deref, AFAICT. > > Hello Matt, > I understand it now. I was misreading the true as part of the function argument. > Could you please also comment if the implementation of gen6_ppgtt_free_pd() in > the same file is safe? It doesn't appear to have an check on pt validity here. Please ignore the question. I understand it now. My apologies for inconvenience. The patch is invalid and can be dropped. deepak. > > Thank you, > deepak. > > > > > > > > > > > continue; > > > > free_pt(&ppgtt->base.vm, pt);
diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c index 5aaacc53fa4c..787b9e6d9f59 100644 --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt *ppgtt) u32 pde; gen6_for_all_pdes(pt, pd, pde) - if (pt) - free_pt(&ppgtt->base.vm, pt); + free_pt(&ppgtt->base.vm, pt); } static void gen6_ppgtt_cleanup(struct i915_address_space *vm) @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct i915_address_space *vm, /* Free all no longer used page tables */ gen6_for_all_pdes(pt, ppgtt->base.pd, pde) { - if (!pt || atomic_read(&pt->used)) + if (atomic_read(&pt->used)) continue; free_pt(&ppgtt->base.vm, pt);
The macro definition of gen6_for_all_pdes() expands to a for loop such that it breaks when the page table is null. Hence there is no need to again test validity of the page table entry pointers in the pde list. This change is identified using itnull.cocci semantic patch. Signed-off-by: Deepak R Varma <drv@mailo.com> --- Please note: Proposed change is compile tested only. drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)