Re: [PATCH v2 2/3] drm/nouveau: subscribe to channel-kill events on NV50 and newer

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Asymmetric teardown of `chan->kill` event and `chan->fence` leads to a Use-After-Free if a channel kill event fires during channel destruction.
- [High] Channel kill event can fire before `chan->fence` is fully initialized, leading to uninitialized spinlock and list head access.
--

--- Patch [2]: [PATCH v2 2/3] drm/nouveau: subscribe to channel-kill events on NV50 and newer ---
commit 66f8740c572f7ea5ed5181be09ddea5061fdb648
Author: Marek Czernohous <[email protected]>

drm/nouveau: subscribe to channel-kill events on NV50 and newer

nouveau_channel_init() only subscribes to the channel-killed event for
FERMI_CHANNEL_GPFIFO and newer. This patch lowers the class gate to
NV50_CHANNEL_GPFIFO, ensuring that pending fences on a killed channel are
properly signalled for NV50/Tesla hardware.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c
> index 598513f604493..1db9fbd81f5c0 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_chan.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_chan.c
> @@ -363,7 +363,7 @@ nouveau_channel_init(struct nouveau_channel *chan, u32 vram, u32 gart)
>  	if (ret)
>  		return ret;
>  
> -	if (chan->user.oclass >= FERMI_CHANNEL_GPFIFO) {
> +	if (chan->user.oclass >= NV50_CHANNEL_GPFIFO) {

[Severity: High]
This is a pre-existing issue, but does expanding this subscription to NV50
expose it to a use-after-free during channel teardown?

During teardown, nouveau_channel_del() destroys the fence context before
unregistering the channel kill event:

nouveau_channel_del() {
    ...
    if (chan->fence)
        nouveau_fence(chan->cli->drm)->context_del(chan);
    ...
    nvif_event_dtor(&chan->kill);
}

The fence context is freed in nv10_fence_context_del() where chan->fence is
set to NULL non-atomically. If a channel kill event fires concurrently, could
the asynchronous handler nouveau_channel_kill() read chan->fence locklessly
just before it is set to NULL and dereference the freed fence context inside
nouveau_fence_context_kill()?

[Severity: High]
This isn't a bug introduced by this patch, but is there also a race during
initialization where the kill event can fire before the fence context is fully
initialized?

In nouveau_channel_init(), the kill event is enabled early:

nouveau_channel_init() {
    ...
    if (ret == 0)
        ret = nvif_event_allow(&chan->kill);
    ...
}

However, the fence context is created at the very end of the function:

nouveau_channel_init() {
    ...
    return nouveau_fence(drm)->context_new(chan);
}

Inside nv50_fence_context_new(), chan->fence is assigned via kzalloc before
the underlying lists and spinlocks are initialized by
nouveau_fence_context_new():

nv50_fence_context_new() {
    ...
    fctx = chan->fence = kzalloc_obj(*fctx);
    if (!fctx)
        return -ENOMEM;

    nouveau_fence_context_new(chan, &fctx->base);
}

If a kill event fires during this window, could nouveau_fence_context_kill()
attempt to acquire an uninitialized spinlock and traverse an uninitialized
list?

>  		DEFINE_RAW_FLEX(struct nvif_event_v0, args, data,
>  				sizeof(struct nvif_chan_event_v0));
>  		struct nvif_chan_event_v0 *host =

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2