Re: [PATCH 0/2] drm/amd: fix VRR race condition during IRQ handling

[Tobias Jakobi]

On 9/9/24 19:18, Harry Wentland wrote:

> On 2024-09-09 13:11, Alex Deucher wrote:
> > On Sun, Sep 8, 2024 at 7:23 AM Tobias Jakobi
> > <tjakobi@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > > On 9/8/24 09:35, Christopher Snowhill wrote:
> > >
> > > > On Mon Sep 2, 2024 at 2:40 AM PDT, tjakobi wrote:
> > > > > From: Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>
> > > > >
> > > > > Hello,
> > > > >
> > > > > this fixes a nasty race condition in the set_drr() callbacks for DCN10
> > > > > and DCN35 that has existed now since quite some time, see this GitLab
> > > > > issue for reference.
> > > > >
> > > > > https://gitlab.freedesktop.org/drm/amd/-/issues/3142
> > > > >
> > > > > The report just focuses von DCN10, but the same problem also exists in
> > > > > the DCN35 code.
> > > > Does the problem not exist in the following references to funcs->set_drr?
> > > >
> > > > drivers/gpu/drm/amd/display/dc/hwss/dce110/dce110_hwseq.c:      if (pipe_ctx->stream_res.tg->funcs->set_drr)
> > > > drivers/gpu/drm/amd/display/dc/hwss/dce110/dce110_hwseq.c:              pipe_ctx->stream_res.tg->funcs->set_drr(
> > > > drivers/gpu/drm/amd/display/dc/hwss/dce110/dce110_hwseq.c:              pipe_ctx[i]->stream_res.tg->funcs->set_drr(
> > > > drivers/gpu/drm/amd/display/dc/hwss/dcn20/dcn20_hwseq.c:        if (pipe_ctx->stream_res.tg->funcs->set_drr)
> > > > drivers/gpu/drm/amd/display/dc/hwss/dcn20/dcn20_hwseq.c:                pipe_ctx->stream_res.tg->funcs->set_drr(
> > > > drivers/gpu/drm/amd/display/dc/hwss/dcn20/dcn20_hwseq.c:                if (pipe_ctx->stream_res.tg->funcs->set_drr)
> > > > drivers/gpu/drm/amd/display/dc/hwss/dcn20/dcn20_hwseq.c:                        pipe_ctx->stream_res.tg->funcs->set_drr(
> > > > drivers/gpu/drm/amd/display/dc/hwss/dcn31/dcn31_hwseq.c:        if (pipe_ctx->stream_res.tg->funcs->set_drr)
> > > > drivers/gpu/drm/amd/display/dc/hwss/dcn31/dcn31_hwseq.c:                pipe_ctx->stream_res.tg->funcs->set_drr(
> > > > drivers/gpu/drm/amd/display/dc/hwss/dcn401/dcn401_hwseq.c:      if (pipe_ctx->stream_res.tg->funcs->set_drr)
> > > > drivers/gpu/drm/amd/display/dc/hwss/dcn401/dcn401_hwseq.c:              pipe_ctx->stream_res.tg->funcs->set_drr(
> > > Maybe. But the big difference I see here, is that in this code there
> > > isn't even any kind of NULL check applied to tg. Or most of the members
> > > of *pipe_ctx. If there really is the same kind of problem here, then one
> > > would need to rewrite a bit more code to fix stuff.
> > >
> > > E.g. in the case of  dcn31_hwseq.c, the questionable code is in
> > > dcn31_reset_back_end_for_pipe(), which is static and only called from
> > > dcn31_reset_hw_ctx_wrap(). Which is assigned to the .reset_hw_ctx_wrap
> > > callback. And this specific callback, from what I can see, is only
> > > called from dce110_reset_hw_ctx_wrap(). Which is then assigned to the
> > > .apply_ctx_to_hw callback. The callback is only called from
> > > dc_commit_state_no_check(). That one is static again, and called from
> > > dc_commit_streams().
> > >
> > > I could trace this even further. My point is: I don't think this is
> > > called from any IRQ handler code. And given the depth and complexity of
> > > the callgraph, I have to admit, that, at least at this point, this is a
> > > bit over my head.
> > >
> > > Sure, I could now sprinkle a bunch of x != NULL in the code, but that
> > > would be merely voodoo. And I usually try to have a theoretical basis
> > > when I apply changes to code.
> > >
> > > Maybe if someone from the AMD display team could give some insight if
> > > there still is potentially vulnerable code in some of the instances that
> > > Christopher has posted, then I would gladly take a look.
> > @Wentland, Harry can you confirm this?
> As Tobias said, without extensive analysis and trace of the code in all
> possible use-case it's hard to say there's no possible way the other
> set_drr calls could potentially have a similar issue.
>
> I think Tobias' analysis is sound and this fixes a number of issues, hence
> my RB.
In fact one user pointed out another potentially vulnerable callback:
https://gitlab.freedesktop.org/drm/amd/-/issues/3142#note_2560109

Which is set_drr() in dce110_hwseq.c -- from which we know that it's 
called from IRQ handler code. Also the backtrace that he posted confirms 
this. That code seems to be a bit older than the DCN10/DCN25 code, as it 
lacks any kind of NULL-check. I have posted a patch that more or less 
copies over the DCN10/35 code. Still waiting for conclusive feedback if 
the patch does something.

If it does, I'm going to post it to amd-gfx as well.

With best wishes,
Tobias
> Harry
>
> > Alex
> >
> > > With best wishes,
> > > Tobias
> > >
> > > > > With best wishes,
> > > > > Tobias
> > > > >
> > > > > Tobias Jakobi (2):
> > > > >     drm/amd/display: Avoid race between dcn10_set_drr() and
> > > > >       dc_state_destruct()
> > > > >     drm/amd/display: Avoid race between dcn35_set_drr() and
> > > > >       dc_state_destruct()
> > > > >
> > > > >    .../amd/display/dc/hwss/dcn10/dcn10_hwseq.c   | 20 +++++++++++--------
> > > > >    .../amd/display/dc/hwss/dcn35/dcn35_hwseq.c   | 20 +++++++++++--------
> > > > >    2 files changed, 24 insertions(+), 16 deletions(-)