Christian, Sima? Matthew? (+CC) Opinions on the below? tl;dr: I still think it's a good thing to detectably block in drm_sched_fini(), or at the very least drm_sched_flush(), because then you'll find out that the driver is broken and can repair it. P. On Fri, 2024-10-18 at 14:07 +0200, Philipp Stanner wrote: > On Mon, 2024-10-14 at 16:56 +0200, Danilo Krummrich wrote: > > On Fri, Sep 27, 2024 at 11:04:48AM +0200, Christian König wrote: > > > Am 25.09.24 um 16:53 schrieb Philipp Stanner: > > > > On Tue, 2024-09-24 at 13:18 +0200, Simona Vetter wrote: > > > > > On Mon, Sep 23, 2024 at 05:24:10PM +0200, Christian König > > > > > wrote: > > > > > > Am 20.09.24 um 15:26 schrieb Philipp Stanner: > > > > > > > On Fri, 2024-09-20 at 12:33 +0200, Christian König wrote: > > > > > > > > Am 20.09.24 um 10:57 schrieb Philipp Stanner: > > > > > > > > > On Wed, 2024-09-18 at 15:39 +0200, Christian König > > > > > > > > > wrote: > > > > > > > > > > Tearing down the scheduler with jobs still on the > > > > > > > > > > pending > > > > > > > > > > list > > > > > > > > > > can > > > > > > > > > > lead to use after free issues. Add a warning if > > > > > > > > > > drivers try > > > > > > > > > > to > > > > > > > > > > destroy a scheduler which still has work pushed to > > > > > > > > > > the HW. > > > > > > > > > Did you have time yet to look into my proposed > > > > > > > > > waitque- > > > > > > > > > solution? > > > > > > > > I don't remember seeing anything. What have I missed? > > > > > > > https://lore.kernel.org/all/20240903094446.29797-2-pstanner@xxxxxxxxxx/ > > > > > > Mhm, I didn't got that in my inbox for some reason. > > > > > > > > > > > > Interesting approach, I'm just not sure if we can or should > > > > > > wait in > > > > > > drm_sched_fini(). > > > > We do agree that jobs still pending when drm_sched_fini() > > > > starts > > > > is > > > > always a bug, right? > > > > > > Correct, the question is how to avoid that. > > > > > > > If so, what are the disadvantages of waiting in > > > > drm_sched_fini()? > > > > We > > > > could block buggy drivers as I see it. Which wouldn't be good, > > > > but > > > > could then be fixed on drivers' site. > > > > > > Sima explained that pretty well: Don't block in fops->close, do > > > that in > > > fops->flush instead. > > > > I agree that we shouldn't block in close(), but this effectively > > means that we > > need to reference count the scheduler, right? > > > > Otherwise, if we allow to just skip / interrupt the teardown, we > > can > > still leak > > memory. > > Having thought about it, I agree with Danilo. Having something that > shall wait on green light, but can be interrupted, is no guarantee > and > therefore not a feasible solution. > > To break down the solution space, these seem to be our options: > 1. We have something (either drm_sched_fini() or a helper, e.g., > drm_sched_flush()) that definitely blocks until the pending > list > has become empty. > 2. We have jobs reference-count the scheduler, so the latter can > outlive the driver and will be freed some time later. > > Can anyone think of a third solution? > > > Solution #1 has the problem of obviously blocking unconditionally if > the driver didn't make sure that all fences will be signaled within > reasonable time. In my opinion, this would actually be an advantage, > because it will be *very* noticable and force users to repair their > driver. The driver *has* to guarantee that all fences will be > signaled. > If the driver has to do fishy things, having the blocking outsourced > to > the helper drm_sched_flush() would allow them to circumvent that. > > Solution #2 has the problem of backend_ops.free_job() potentially > using > driver-data after the driver is gone, causing UAF. So with this > solutions all drivers would have to be aware of the issue and handle > it > through one of DRMs primitives dedicated to such problems. > > > Currently, all drivers either work around the problem internally or > simply ignore it, it seems. > > So I'd argue that both solutions are an improvement over the existing > situation. My preference would be #1. > > > Opinions? > > P. > > > > > > > > > One issue this solves is that when you send a SIGTERM the tear > > > down > > > handling > > > first flushes all the FDs and then closes them. > > > > > > So if flushing the FDs blocks because the process initiated > > > sending > > > a > > > terabyte of data over a 300bps line (for example) you can still > > > throw a > > > SIGKILL and abort that as well. > > > > > > If you would block in fops-close() that SIGKILL won't have any > > > effect any > > > more because by the time close() is called the process is gone > > > and > > > signals > > > are already blocked. > > > > > > And yes when I learned about that issue I was also buffed that > > > handling like > > > this in the UNIX design is nearly 50 years old and still applies > > > to > > > today. > > > > > > Probably better to make that a separate function, something > > > > > > like > > > > > > drm_sched_flush() or similar. > > > > We could do that. Such a function could then be called by > > > > drivers > > > > which > > > > are not sure whether all jobs are done before they start > > > > tearing > > > > down. > > > > > > Yes exactly that's the idea. And give that flush function a > > > return > > > code so > > > that it can return -EINTR. > > > > > > > > Yeah I don't think we should smash this into drm_sched_fini > > > > > unconditionally. I think conceptually there's about three > > > > > cases: > > > > > > > > > > - Ringbuffer schedules. Probably want everything as-is, > > > > > because > > > > > drm_sched_fini is called long after all the entities are > > > > > gone in > > > > > drm_device cleanup. > > > > > > > > > > - fw scheduler hardware with preemption support. There we > > > > > probably > > > > > want to > > > > > nuke the context by setting the tdr timeout to zero (or > > > > > maybe just > > > > > as > > > > > long as context preemption takes to be efficient), and > > > > > relying on > > > > > the > > > > > normal gpu reset flow to handle things. > > > > > drm_sched_entity_flush > > > > > kinda > > > > > does this, except not really and it's a lot more focused > > > > > on > > > > > the > > > > > ringbuffer context. So maybe we want a new > > > > > drm_sched_entity_kill. > > > > > > > > > > For this case calling drm_sched_fini() after the 1:1 > > > > > entity > > > > > is gone > > > > > should not find any linger jobs, it would actually be a > > > > > bug > > > > > somewhere if > > > > > there's a job lingering. Maybe a sanity check that there's > > > > > not just > > > > > no > > > > > jobs lingering, but also no entity left would be good > > > > > here? > > > > The check for lingering ones is in Christian's patch here IISC. > > > > At which position would you imagine the check for the entity > > > > being > > > > performed? > > > > > > > > > - fw scheduler without preemption support. There we kinda > > > > > need > > > > > the > > > > > drm_sched_flush, except blocking in fops->close is not > > > > > great. So > > > > > instead > > > > > I think the following is better: > > > > > 1. drm_sched_entity_stopped, which only stops new > > > > > submissions (for > > > > > paranoia) but doesn't tear down the entity > > > > Who would call that function? > > > > Drivers using it voluntarily could just as well stop accepting > > > > new jobs > > > > from userspace to their entities, couldn't they? > > > > > > > > > 2. drm_dev_get > > > > > 3. launch a worker which does a) drm_sched_flush (or > > > > > drm_sched_entity_flush or whatever we call it) b) > > > > > drm_sched_entity_fini > > > > > + drm_sched_fini c) drm_dev_put > > > > > > > > > > Note that semantically this implements the refcount in the > > > > > other > > > > > path > > > > > from Phillip: > > > > > > > > > > https://lore.kernel.org/all/20240903094531.29893-2-pstanner@xxxxxxxxxx/ > > > > > Except it doesn't impose refcount on everyone else who > > > > > doesn't need > > > > > it, > > > > > and it doesn't even impose refcounting on drivers that do > > > > > need it > > > > > because we use drm_sched_flush and a worker to achieve the > > > > > same. > > > > I indeed wasn't happy with the refcount approach for that > > > > reason, > > > > agreed. > > > > > > > > > Essentially helper functions for the common use-cases instead > > > > > of > > > > > trying to > > > > > solve them all by putting drm_sched_flush as a potentially > > > > > very > > > > > blocking > > > > > function into drm_sched_fini. > > > > I'm still not able to see why it blocking would be undesired – > > > > as > > > > far > > > > as I can see, it is only invoked on driver teardown, so not > > > > during > > > > active operation. Teardown doesn't happen that often, and it > > > > can > > > > (if > > > > implemented correctly) only block until the driver's code has > > > > signaled > > > > the last fences. If that doesn't happen, the block would reveal > > > > a > > > > bug. > > > > > > > > But don't get me wrong: I don't want to *push* this solution. I > > > > just > > > > want to understand when it could become a problem. > > > > > > > > > > > > Wouldn't an explicitly blocking, separate function like > > > > drm_sched_flush() or drm_sched_fini_flush() be a small, doable > > > > step > > > > towards the right direction? > > > > > > I think that this is the right thing to do, yes. > > > > > > > > > > > > > When there are still entities with jobs the > > > > > > > > > > situation > > > > > > > > > > is > > > > > > > > > > even > > > > > > > > > > worse > > > > > > > > > > since the dma_fences for those jobs can never > > > > > > > > > > signal > > > > > > > > > > we can > > > > > > > > > > just > > > > > > > > > > choose between potentially locking up core memory > > > > > > > > > > management and > > > > > > > > > > random memory corruption. When drivers really mess > > > > > > > > > > it > > > > > > > > > > up > > > > > > > > > > that > > > > > > > > > > well > > > > > > > > > > let them run into a BUG_ON(). > > > > > > > > > > > > > > > > > > > > Signed-off-by: Christian > > > > > > > > > > König<christian.koenig@xxxxxxx> > > > > > > > > > > --- > > > > > > > > > > drivers/gpu/drm/scheduler/sched_main.c | 19 > > > > > > > > > > ++++++++++++++++++- > > > > > > > > > > 1 file changed, 18 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > > > > b/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > > > > index f093616fe53c..8a46fab5cdc8 100644 > > > > > > > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > > > > @@ -1333,17 +1333,34 @@ void drm_sched_fini(struct > > > > > > > > > > drm_gpu_scheduler > > > > > > > > > > *sched) > > > > > > > > > I agree with Sima that it should first be documented > > > > > > > > > in > > > > > > > > > the > > > > > > > > > function's > > > > > > > > > docstring what the user is expected to have done > > > > > > > > > before > > > > > > > > > calling the > > > > > > > > > function. > > > > > > > > Good point, going to update the documentation as well. > > > > > > > Cool thing, thx. > > > > > > > Would be great if everything (not totally trivial) > > > > > > > necessary to > > > > > > > be done > > > > > > > before _fini() is mentioned. > > > > > > > > > > > > > > One could also think about providing a hint at how the > > > > > > > driver can > > > > > > > do > > > > > > > that. AFAICS the only way for the driver to ensure that > > > > > > > is > > > > > > > to > > > > > > > maintain > > > > > > > its own, separate list of submitted jobs. > > > > > > Even with a duplicated pending list it's actually currently > > > > > > impossible to do > > > > > > this fully cleanly. > > > > > > > > > > > > The problem is that the dma_fence object gives no guarantee > > > > > > when > > > > > > callbacks > > > > > > are processed, e.g. they can be both processed from > > > > > > interrupt > > > > > > context as > > > > > > well as from a CPU which called dma_fence_is_signaled(). > > > > > > > > > > > > So when a driver (or drm_sched_fini) waits for the last > > > > > > submitted > > > > > > fence it > > > > > > actually can be that the drm_sched object still needs to do > > > > > > some > > > > > > processing. > > > > > > See the hack in amdgpu_vm_tlb_seq() for more background on > > > > > > the > > > > > > problem. > > > > Oh dear ^^' > > > > We better work towards fixing that centrally > > > > > > > > Thanks, > > > > P. > > > > > > > > > > > > > So I thought this should be fairly easy because of the sched > > > > > hw/public > > > > > fence split: If we wait for both all jobs to finish and for > > > > > all > > > > > the > > > > > sched > > > > > work/tdr work to finish, and we make sure there's no entity > > > > > existing > > > > > that's not yet stopped we should catch them all? > > > > > > Unfortunately not. > > > > > > Even when you do a dma_fence_wait() on the last submission it can > > > still be > > > that another CPU is executing the callbacks to wake up the > > > scheduler work > > > item and cleanup the job. > > > > > > That's one of the reasons why I think the design of keeping the > > > job > > > alive is > > > so extremely awkward. The dma_fence as representation of the hw > > > submission > > > has a much better defined state machine and lifetime. > > > > > > Regards, > > > Christian. > > > > > > > > Or at least I think > > > > > it's > > > > > a bug if any other code even tries to touch the hw fence. > > > > > > > > > > If you have any other driver code which relies on the rcu > > > > > freeing > > > > > then I > > > > > think that's just a separate concern and we can ignore that > > > > > here > > > > > since the > > > > > fences themselves will till get rcu-delay freed even if > > > > > drm_sched_fini has > > > > > finished. > > > > > -Sima > > > > > > > > > > > Regards, > > > > > > Christian. > > > > > > > > > > > > > P. > > > > > > > > > > > > > > > Thanks, > > > > > > > > Christian. > > > > > > > > > > > > > > > > > P. > > > > > > > > > > > > > > > > > > > drm_sched_wqueue_stop(sched); > > > > > > > > > > + /* > > > > > > > > > > + * Tearing down the scheduler wile there are still > > > > > > > > > > unprocessed jobs can > > > > > > > > > > + * lead to use after free issues in the scheduler > > > > > > > > > > fence. > > > > > > > > > > + */ > > > > > > > > > > + WARN_ON(!list_empty(&sched->pending_list)); > > > > > > > > > > + > > > > > > > > > > for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched- > > > > > > > > > > > num_rqs; > > > > > > > > > > i++) > > > > > > > > > > { > > > > > > > > > > struct drm_sched_rq *rq = sched- > > > > > > > > > > > sched_rq[i]; > > > > > > > > > > spin_lock(&rq->lock); > > > > > > > > > > - list_for_each_entry(s_entity, &rq- > > > > > > > > > > > entities, > > > > > > > > > > list) > > > > > > > > > > + list_for_each_entry(s_entity, &rq- > > > > > > > > > > > entities, > > > > > > > > > > list) { > > > > > > > > > > + /* > > > > > > > > > > + * The justification for this > > > > > > > > > > BUG_ON() > > > > > > > > > > is > > > > > > > > > > that tearing > > > > > > > > > > + * down the scheduler while jobs > > > > > > > > > > are > > > > > > > > > > pending > > > > > > > > > > leaves > > > > > > > > > > + * dma_fences unsignaled. Since we > > > > > > > > > > have > > > > > > > > > > dependencies > > > > > > > > > > + * from the core memory management > > > > > > > > > > to > > > > > > > > > > eventually signal > > > > > > > > > > + * dma_fences this can trivially > > > > > > > > > > lead to > > > > > > > > > > a > > > > > > > > > > system wide > > > > > > > > > > + * stop because of a locked up > > > > > > > > > > memory > > > > > > > > > > management. > > > > > > > > > > + */ > > > > > > > > > > + BUG_ON(spsc_queue_count(&s_entity- > > > > > > > > > > > job_queue)); > > > > > > > > > > + > > > > > > > > > > /* > > > > > > > > > > * Prevents reinsertion and marks > > > > > > > > > > job_queue > > > > > > > > > > as idle, > > > > > > > > > > * it will removed from rq in > > > > > > > > > > drm_sched_entity_fini > > > > > > > > > > * eventually > > > > > > > > > > */ > > > > > > > > > > s_entity->stopped = true; > > > > > > > > > > + } > > > > > > > > > > spin_unlock(&rq->lock); > > > > > > > > > > kfree(sched->sched_rq[i]); > > > > > > > > > > } > > >
