| Message ID | 20240219125047.28906-1-nirmoy.das@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: check before removing mm notifier | expand |
On Mon, Feb 19, 2024 at 01:50:47PM +0100, Nirmoy Das wrote: > Error in mmu_interval_notifier_insert() can leave a NULL > notifier.mm pointer. Catch that and return early. > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Shawn Lee <shawn.c.lee@intel.com> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > index 0e21ce9d3e5a..61abfb505766 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > @@ -349,6 +349,9 @@ i915_gem_userptr_release(struct drm_i915_gem_object *obj) > { > GEM_WARN_ON(obj->userptr.page_ref); > > + if (!obj->userptr.notifier.mm) > + return; > + hmmm... right, it looks that we need this protection. But... I mean, feel free to use Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> for this patch, but I believe that if this mmu insert failed we might have other deeper problems like when checking i915_gem_object_is_userptr() ? No?! > mmu_interval_notifier_remove(&obj->userptr.notifier); > obj->userptr.notifier.mm = NULL; > } > -- > 2.42.0 >
Hi Rodrigo, On 2/19/2024 9:12 PM, Rodrigo Vivi wrote: > On Mon, Feb 19, 2024 at 01:50:47PM +0100, Nirmoy Das wrote: >> Error in mmu_interval_notifier_insert() can leave a NULL >> notifier.mm pointer. Catch that and return early. >> >> Cc: Andi Shyti<andi.shyti@linux.intel.com> >> Cc: Shawn Lee<shawn.c.lee@intel.com> >> Signed-off-by: Nirmoy Das<nirmoy.das@intel.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >> index 0e21ce9d3e5a..61abfb505766 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >> @@ -349,6 +349,9 @@ i915_gem_userptr_release(struct drm_i915_gem_object *obj) >> { >> GEM_WARN_ON(obj->userptr.page_ref); >> >> + if (!obj->userptr.notifier.mm) >> + return; >> + > hmmm... right, it looks that we need this protection. But... > > I mean, feel free to use > Reviewed-by: Rodrigo Vivi<rodrigo.vivi@intel.com> > > for this patch, > > but I believe that if this mmu insert failed we might have other > deeper problems like when checking i915_gem_object_is_userptr() ? > > No?! We are returning an error if mmu insert fails while creating a userptr object so the obj struct is only available to obj cleanup methods. As far as I see, i915_gem_object_is_userptr() should not happen on such obj struct. Thanks, Nirmoy >> mmu_interval_notifier_remove(&obj->userptr.notifier); >> obj->userptr.notifier.mm = NULL; >> } >> -- >> 2.42.0 >>
Merged it to drm-intel-gt-next with s/check/Check On 2/19/2024 1:50 PM, Nirmoy Das wrote: > Error in mmu_interval_notifier_insert() can leave a NULL > notifier.mm pointer. Catch that and return early. > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Shawn Lee <shawn.c.lee@intel.com> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > index 0e21ce9d3e5a..61abfb505766 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > @@ -349,6 +349,9 @@ i915_gem_userptr_release(struct drm_i915_gem_object *obj) > { > GEM_WARN_ON(obj->userptr.page_ref); > > + if (!obj->userptr.notifier.mm) > + return; > + > mmu_interval_notifier_remove(&obj->userptr.notifier); > obj->userptr.notifier.mm = NULL; > }
On 21/02/2024 11:52, Nirmoy Das wrote: > Merged it to drm-intel-gt-next with s/check/Check Shouldn't this have had: Fixes: ed29c2691188 ("drm/i915: Fix userptr so we do not have to worry about obj->mm.lock, v7.") Cc: <stable@vger.kernel.org> # v5.13+ ? Regards, Tvrtko > On 2/19/2024 1:50 PM, Nirmoy Das wrote: >> Error in mmu_interval_notifier_insert() can leave a NULL >> notifier.mm pointer. Catch that and return early. >> >> Cc: Andi Shyti <andi.shyti@linux.intel.com> >> Cc: Shawn Lee <shawn.c.lee@intel.com> >> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >> b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >> index 0e21ce9d3e5a..61abfb505766 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >> @@ -349,6 +349,9 @@ i915_gem_userptr_release(struct >> drm_i915_gem_object *obj) >> { >> GEM_WARN_ON(obj->userptr.page_ref); >> + if (!obj->userptr.notifier.mm) >> + return; >> + >> mmu_interval_notifier_remove(&obj->userptr.notifier); >> obj->userptr.notifier.mm = NULL; >> }
Hi Tvrtko, On 2/27/2024 10:04 AM, Tvrtko Ursulin wrote: > > On 21/02/2024 11:52, Nirmoy Das wrote: >> Merged it to drm-intel-gt-next with s/check/Check > > Shouldn't this have had: > > Fixes: ed29c2691188 ("drm/i915: Fix userptr so we do not have to worry > about obj->mm.lock, v7.") > Cc: <stable@vger.kernel.org> # v5.13+ > > ? > Yes. Sorry, I missed that. Can we still the tag ? Thanks, Nirmoy > Regards, > > Tvrtko > >> On 2/19/2024 1:50 PM, Nirmoy Das wrote: >>> Error in mmu_interval_notifier_insert() can leave a NULL >>> notifier.mm pointer. Catch that and return early. >>> >>> Cc: Andi Shyti <andi.shyti@linux.intel.com> >>> Cc: Shawn Lee <shawn.c.lee@intel.com> >>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> index 0e21ce9d3e5a..61abfb505766 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> @@ -349,6 +349,9 @@ i915_gem_userptr_release(struct >>> drm_i915_gem_object *obj) >>> { >>> GEM_WARN_ON(obj->userptr.page_ref); >>> + if (!obj->userptr.notifier.mm) >>> + return; >>> + >>> mmu_interval_notifier_remove(&obj->userptr.notifier); >>> obj->userptr.notifier.mm = NULL; >>> }
On 27/02/2024 09:26, Nirmoy Das wrote: > Hi Tvrtko, > > On 2/27/2024 10:04 AM, Tvrtko Ursulin wrote: >> >> On 21/02/2024 11:52, Nirmoy Das wrote: >>> Merged it to drm-intel-gt-next with s/check/Check >> >> Shouldn't this have had: >> >> Fixes: ed29c2691188 ("drm/i915: Fix userptr so we do not have to worry >> about obj->mm.lock, v7.") >> Cc: <stable@vger.kernel.org> # v5.13+ >> >> ? >> > Yes. Sorry, I missed that. Can we still the tag ? I've added them and force pushed the branch since commit was still at the top. FYI + Jani, Joonas and Rodrigo Regards, Tvrtko > > > Thanks, > > Nirmoy > >> Regards, >> >> Tvrtko >> >>> On 2/19/2024 1:50 PM, Nirmoy Das wrote: >>>> Error in mmu_interval_notifier_insert() can leave a NULL >>>> notifier.mm pointer. Catch that and return early. >>>> >>>> Cc: Andi Shyti <andi.shyti@linux.intel.com> >>>> Cc: Shawn Lee <shawn.c.lee@intel.com> >>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>>> b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>>> index 0e21ce9d3e5a..61abfb505766 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>>> @@ -349,6 +349,9 @@ i915_gem_userptr_release(struct >>>> drm_i915_gem_object *obj) >>>> { >>>> GEM_WARN_ON(obj->userptr.page_ref); >>>> + if (!obj->userptr.notifier.mm) >>>> + return; >>>> + >>>> mmu_interval_notifier_remove(&obj->userptr.notifier); >>>> obj->userptr.notifier.mm = NULL; >>>> }
On 2/28/2024 2:24 PM, Tvrtko Ursulin wrote: > > On 27/02/2024 09:26, Nirmoy Das wrote: >> Hi Tvrtko, >> >> On 2/27/2024 10:04 AM, Tvrtko Ursulin wrote: >>> >>> On 21/02/2024 11:52, Nirmoy Das wrote: >>>> Merged it to drm-intel-gt-next with s/check/Check >>> >>> Shouldn't this have had: >>> >>> Fixes: ed29c2691188 ("drm/i915: Fix userptr so we do not have to >>> worry about obj->mm.lock, v7.") >>> Cc: <stable@vger.kernel.org> # v5.13+ >>> >>> ? >>> >> Yes. Sorry, I missed that. Can we still the tag ? > > I've added them and force pushed the branch since commit was still at > the top. Thanks a lot, Tvrtko! > > FYI + Jani, Joonas and Rodrigo > > Regards, > > Tvrtko > >> >> >> Thanks, >> >> Nirmoy >> >>> Regards, >>> >>> Tvrtko >>> >>>> On 2/19/2024 1:50 PM, Nirmoy Das wrote: >>>>> Error in mmu_interval_notifier_insert() can leave a NULL >>>>> notifier.mm pointer. Catch that and return early. >>>>> >>>>> Cc: Andi Shyti <andi.shyti@linux.intel.com> >>>>> Cc: Shawn Lee <shawn.c.lee@intel.com> >>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>>>> b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>>>> index 0e21ce9d3e5a..61abfb505766 100644 >>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>>>> @@ -349,6 +349,9 @@ i915_gem_userptr_release(struct >>>>> drm_i915_gem_object *obj) >>>>> { >>>>> GEM_WARN_ON(obj->userptr.page_ref); >>>>> + if (!obj->userptr.notifier.mm) >>>>> + return; >>>>> + >>>>> mmu_interval_notifier_remove(&obj->userptr.notifier); >>>>> obj->userptr.notifier.mm = NULL; >>>>> }
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c index 0e21ce9d3e5a..61abfb505766 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -349,6 +349,9 @@ i915_gem_userptr_release(struct drm_i915_gem_object *obj) { GEM_WARN_ON(obj->userptr.page_ref); + if (!obj->userptr.notifier.mm) + return; + mmu_interval_notifier_remove(&obj->userptr.notifier); obj->userptr.notifier.mm = NULL; }
Error in mmu_interval_notifier_insert() can leave a NULL notifier.mm pointer. Catch that and return early. Cc: Andi Shyti <andi.shyti@linux.intel.com> Cc: Shawn Lee <shawn.c.lee@intel.com> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 +++ 1 file changed, 3 insertions(+)