Re: [PATCH v2 1/3] drm/mm: Ensure that the entry is not NULL before extracting rb_node
On 23/02/2022 04:35, Kasireddy, Vivek wrote: 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. ">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? Please send without the first patch so we get clean set of CI results. You can use "--subject-prefix=CI" with git format-patchs and --suppress-cc=all with git send-email to avoid spamming people and let readers know the re-send is just for the purpose of getting CI results. Regards, Tvrtko
RE: [PATCH v2 1/3] drm/mm: Ensure that the entry is not NULL before extracting rb_node
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.
> ">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
> >>> Cc: Christian König
> >>> Signed-off-by: Vivek Kasireddy
> >>> ---
> >>>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 = >rb_hole_addr; \
> >>> + struct rb_node *parent, *node; \
> >>>
> >>> \
> >>> - if (!entry || RB_EMPTY_NODE(node)) \
> >>> + if (!entry || RB_EMPTY_NODE(>rb_hole_addr)) \
> >>> return NULL;
> >>> \
> >>>
> >>> \
> >>> + node = >rb_hole_addr;\
> >>> if (usable_hole_addr(node->first, size)) {
> >>> \
> >>> node = node->first;
> >>> \
> >>> while (usable_hole_addr(node->last, size))
> >>> \
Re: [PATCH v2 1/3] drm/mm: Ensure that the entry is not NULL before extracting rb_node
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.
">rb_hole_addr" is taking an address of, which works "fine" is
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.
Regards,
Tvrtko
Thanks,
Vivek
Regards,
Tvrtko
Cc: Tvrtko Ursulin
Cc: Christian König
Signed-off-by: Vivek Kasireddy
---
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 = >rb_hole_addr;\
+ struct rb_node *parent, *node; \
\
- if (!entry || RB_EMPTY_NODE(node)) \
+ if (!entry || RB_EMPTY_NODE(>rb_hole_addr)) \
return NULL;\
\
+ node = >rb_hole_addr; \
if (usable_hole_addr(node->first, size)) { \
node = node->first; \
while (usable_hole_addr(node->last, size)) \
RE: [PATCH v2 1/3] drm/mm: Ensure that the entry is not NULL before extracting rb_node
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.
Thanks,
Vivek
>
> Regards,
>
> Tvrtko
>
> >
> > Cc: Tvrtko Ursulin
> > Cc: Christian König
> > Signed-off-by: Vivek Kasireddy
> > ---
> > 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 = >rb_hole_addr; \
> > + struct rb_node *parent, *node; \
> > \
> > - if (!entry || RB_EMPTY_NODE(node)) \
> > + if (!entry || RB_EMPTY_NODE(>rb_hole_addr)) \
> > return NULL;\
> > \
> > + node = >rb_hole_addr;\
> > if (usable_hole_addr(node->first, size)) { \
> > node = node->first; \
> > while (usable_hole_addr(node->last, size)) \
Re: [PATCH v2 1/3] drm/mm: Ensure that the entry is not NULL before extracting rb_node
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?
Regards,
Tvrtko
Cc: Tvrtko Ursulin
Cc: Christian König
Signed-off-by: Vivek Kasireddy
---
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 = >rb_hole_addr;\
+ struct rb_node *parent, *node; \
\
- if (!entry || RB_EMPTY_NODE(node)) \
+ if (!entry || RB_EMPTY_NODE(>rb_hole_addr)) \
return NULL;\
\
+ node = >rb_hole_addr; \
if (usable_hole_addr(node->first, size)) { \
node = node->first; \
while (usable_hole_addr(node->last, size)) \
