Re: [PATCH 1/3] drm/scheduler: Add more documentation

[Asahi Lina]

On 14/07/2023 18.47, Christian König wrote:
> Am 14.07.23 um 11:39 schrieb Asahi Lina:
> > On 14/07/2023 17.40, Christian König wrote:
> > > Am 14.07.23 um 10:21 schrieb Asahi Lina:
> > > > Document the implied lifetime rules of the scheduler (or at least the
> > > > intended ones), as well as the expectations of how resource acquisition
> > > > should be handled.
> > > >
> > > > Signed-off-by: Asahi Lina <lina@xxxxxxxxxxxxx>
> > > > ---
> > > >     drivers/gpu/drm/scheduler/sched_main.c | 58
> > > > ++++++++++++++++++++++++++++++++--
> > > >     1 file changed, 55 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > index 7b2bfc10c1a5..1f3bc3606239 100644
> > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > @@ -43,9 +43,61 @@
> > > >      *
> > > >      * The jobs in a entity are always scheduled in the order that
> > > > they were pushed.
> > > >      *
> > > > - * Note that once a job was taken from the entities queue and
> > > > pushed to the
> > > > - * hardware, i.e. the pending queue, the entity must not be
> > > > referenced anymore
> > > > - * through the jobs entity pointer.
> > > > + * Lifetime rules
> > > > + * --------------
> > > > + *
> > > > + * Getting object lifetimes right across the stack is critical to
> > > > avoid UAF
> > > > + * issues. The DRM scheduler has the following lifetime rules:
> > > > + *
> > > > + * - The scheduler must outlive all of its entities.
> > > > + * - Jobs pushed to the scheduler are owned by it, and must only be
> > > > freed
> > > > + *   after the free_job() callback is called.
> > > > + * - Scheduler fences are reference-counted and may outlive the
> > > > scheduler.
> > >
> > > > + * - The scheduler *may* be destroyed while jobs are still in flight.
> > >
> > > That's not correct. The scheduler can only be destroyed after all the
> > > entities serving it have been destroyed as well as all the jobs already
> > > pushed to the hw finished.
> >
> > The point of this series is to change this behavior so I can actually
> > use the scheduler in my use case, and that begins with formally
> > documenting it as Daniel suggested. That is, I need it to be safe for
> > jobs to not be yet complete before the scheduler is destroyed (the
> > entities do get destroyed first, that's the first bullet point).
>
> Yeah, but you need to document the current situation not how you like it
> to be.

Daniel told me to document how I think it should be, then fix the bugs 
that make it not so. That's what this series does.

> > We already had this discussion. Without this guarantee, I cannot build
> > a reasonable safe Rust abstraction. Unless you have another
> > suggestion, as far as I can tell it's either this or I give up on
> > using the DRM scheduler entirely and reimplement something else on my
> > own.
> >
> > > What might be possible to add is that the hw is still working on the
> > > already pushed jobs, but so far that was rejected as undesirable.
> >
> > Where was this rejected?
>
> Years ago. Our initial driver suspend/resume design relied on that.
> Turned out not to be a good idea

Times change, maybe it's time to revisit that decision?

~~ Lina