[Intel-gfx] [PATCH v2 1/3] drm/mm: Ensure that the entry is not NULL before extracting rb_node

[Kasireddy, Vivek]

Hi Tvrtko,

> 
> On 18/02/2022 03:47, Kasireddy, Vivek wrote:
> > Hi Tvrtko,
> >
> >>
> >> On 17/02/2022 07:50, Vivek Kasireddy wrote:
> >>> While looking for next holes suitable for an allocation, although,
> >>> it is highly unlikely, make sure that the DECLARE_NEXT_HOLE_ADDR
> >>> macro is using a valid node before it extracts the rb_node from it.
> >>
> >> Was the need for this just a consequence of insufficient locking in the
> >> i915 patch?
> > [Kasireddy, Vivek] Partly, yes; but I figured since we are anyway doing
> > if (!entry || ..), it makes sense to dereference entry and extract the rb_node
> > after this check.
> 
> Unless I am blind I don't see that it makes a difference.
> "&entry->rb_hole_addr" is taking an address of, which works "fine" is
[Kasireddy, Vivek] Ah, didn't realize it was the same thing as offsetof(). 

> entry is NULL. And does not get past the !entry check for the actual
> de-reference via RB_EMPTY_NODE. With your patch you move that after the
> !entry check but still have it in the RB_EMPTY_NODE macro. Again, unless
> I am blind, I think just drop this patch.
[Kasireddy, Vivek] Sure; do you want me to send another version with this
patch dropped? Or, would you be able to just merge the other two from the
latest version of this series?

Thanks,
Vivek

> 
> Regards,
> 
> Tvrtko
> 
> 
> > Thanks,
> > Vivek
> >
> >>
> >> Regards,
> >>
> >> Tvrtko
> >>
> >>>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> >>> Cc: Christian König <christian.koenig at amd.com>
> >>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
> >>> ---
> >>>    drivers/gpu/drm/drm_mm.c | 5 +++--
> >>>    1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> >>> index 8257f9d4f619..499d8874e4ed 100644
> >>> --- a/drivers/gpu/drm/drm_mm.c
> >>> +++ b/drivers/gpu/drm/drm_mm.c
> >>> @@ -389,11 +389,12 @@ first_hole(struct drm_mm *mm,
> >>>    #define DECLARE_NEXT_HOLE_ADDR(name, first, last)			\
> >>>    static struct drm_mm_node *name(struct drm_mm_node *entry, u64 size)	\
> >>>    {									\
> >>> -	struct rb_node *parent, *node = &entry->rb_hole_addr;		\
> >>> +	struct rb_node *parent, *node;					\
> >>>    									\
> >>> -	if (!entry || RB_EMPTY_NODE(node))				\
> >>> +	if (!entry || RB_EMPTY_NODE(&entry->rb_hole_addr))		\
> >>>    		return NULL;						\
> >>>    									\
> >>> +	node = &entry->rb_hole_addr;					\
> >>>    	if (usable_hole_addr(node->first, size)) {			\
> >>>    		node = node->first;					\
> >>>    		while (usable_hole_addr(node->last, size))		\