| Message ID | 20241115164159.1081675-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] drm/i915/dp_mst: Fix connector initialization in intel_dp_add_mst_connector() | expand |
On Fri, Nov 15, 2024 at 06:41:56PM +0200, Imre Deak wrote: > The connector initialization in intel_dp_add_mst_connector() depends on > the device pointer in connector to be valid, at least by connector > debug printing. The device pointer is initialized by drm_connector_init(), > however that function also exposes the connector to in-kernel users, > which can't be done before the connector is fully initialized. For now > make sure the device pointer is valid before it's used, until a > follow-up change moving this to DRM core. > > This issue was revealed by the commit in the Fixes: line below, before > which the above debug printing checked and handled a NULL device pointer > gracefully in DRM core. > > Cc: Jani Nikula <jani.nikula@intel.com> > Fixes: 529798bd786a ("drm/i915/mst: convert to struct intel_display") This is awkward. This patch actually removes callers of base.dev. I don't see how that it could be causing this new null dereference. > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12799 But well, trusting more the tests then my eyes, let's move forward. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index df7edcfe885b6..f058360a26413 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -1727,6 +1727,16 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo > > intel_dp_init_modeset_retry_work(intel_connector); > > + /* > + * TODO: The following drm_connector specific initialization belongs > + * to DRM core, however it happens atm too late in > + * drm_connector_init(). That function will also expose the connector > + * to in-kernel users, so it can't be called until the connector is > + * sufficiently initialized; init the device pointer used by the > + * following DSC setup, until a fix moving this to DRM core. > + */ > + intel_connector->base.dev = mgr->dev; > + > intel_connector->dp.dsc_decompression_aux = drm_dp_mst_dsc_aux_for_port(port); > intel_dp_mst_read_decompression_port_dsc_caps(intel_dp, intel_connector); > intel_connector->dp.dsc_hblank_expansion_quirk = > -- > 2.44.2 >
On Fri, Nov 15, 2024 at 03:20:58PM -0500, Rodrigo Vivi wrote: > On Fri, Nov 15, 2024 at 06:41:56PM +0200, Imre Deak wrote: > > The connector initialization in intel_dp_add_mst_connector() depends on > > the device pointer in connector to be valid, at least by connector > > debug printing. The device pointer is initialized by drm_connector_init(), > > however that function also exposes the connector to in-kernel users, > > which can't be done before the connector is fully initialized. For now > > make sure the device pointer is valid before it's used, until a > > follow-up change moving this to DRM core. > > > > This issue was revealed by the commit in the Fixes: line below, before > > which the above debug printing checked and handled a NULL device pointer > > gracefully in DRM core. > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Fixes: 529798bd786a ("drm/i915/mst: convert to struct intel_display") > > This is awkward. This patch actually removes callers of base.dev. > I don't see how that it could be causing this new null dereference. It adds struct intel_display *display = to_intel_display(connector); which will be NULL since connector->base.dev is NULL and later display is dereferenced. > > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12799 > > But well, trusting more the tests then my eyes, let's move forward. > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > index df7edcfe885b6..f058360a26413 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > @@ -1727,6 +1727,16 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo > > > > intel_dp_init_modeset_retry_work(intel_connector); > > > > + /* > > + * TODO: The following drm_connector specific initialization belongs > > + * to DRM core, however it happens atm too late in > > + * drm_connector_init(). That function will also expose the connector > > + * to in-kernel users, so it can't be called until the connector is > > + * sufficiently initialized; init the device pointer used by the > > + * following DSC setup, until a fix moving this to DRM core. > > + */ > > + intel_connector->base.dev = mgr->dev; > > + > > intel_connector->dp.dsc_decompression_aux = drm_dp_mst_dsc_aux_for_port(port); > > intel_dp_mst_read_decompression_port_dsc_caps(intel_dp, intel_connector); > > intel_connector->dp.dsc_hblank_expansion_quirk = > > -- > > 2.44.2 > >
On Fri, Nov 15, 2024 at 10:30:12PM +0200, Imre Deak wrote: > On Fri, Nov 15, 2024 at 03:20:58PM -0500, Rodrigo Vivi wrote: > > On Fri, Nov 15, 2024 at 06:41:56PM +0200, Imre Deak wrote: > > > The connector initialization in intel_dp_add_mst_connector() depends on > > > the device pointer in connector to be valid, at least by connector > > > debug printing. The device pointer is initialized by drm_connector_init(), > > > however that function also exposes the connector to in-kernel users, > > > which can't be done before the connector is fully initialized. For now > > > make sure the device pointer is valid before it's used, until a > > > follow-up change moving this to DRM core. > > > > > > This issue was revealed by the commit in the Fixes: line below, before > > > which the above debug printing checked and handled a NULL device pointer > > > gracefully in DRM core. > > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Fixes: 529798bd786a ("drm/i915/mst: convert to struct intel_display") > > > > This is awkward. This patch actually removes callers of base.dev. > > I don't see how that it could be causing this new null dereference. > > It adds > > struct intel_display *display = to_intel_display(connector); > > which will be NULL since connector->base.dev is NULL and later display > is dereferenced. oh I see! Thanks > > > > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12799 > > > > But well, trusting more the tests then my eyes, let's move forward. > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > index df7edcfe885b6..f058360a26413 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > @@ -1727,6 +1727,16 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo > > > > > > intel_dp_init_modeset_retry_work(intel_connector); > > > > > > + /* > > > + * TODO: The following drm_connector specific initialization belongs > > > + * to DRM core, however it happens atm too late in > > > + * drm_connector_init(). That function will also expose the connector > > > + * to in-kernel users, so it can't be called until the connector is > > > + * sufficiently initialized; init the device pointer used by the > > > + * following DSC setup, until a fix moving this to DRM core. > > > + */ > > > + intel_connector->base.dev = mgr->dev; > > > + > > > intel_connector->dp.dsc_decompression_aux = drm_dp_mst_dsc_aux_for_port(port); > > > intel_dp_mst_read_decompression_port_dsc_caps(intel_dp, intel_connector); > > > intel_connector->dp.dsc_hblank_expansion_quirk = > > > -- > > > 2.44.2 > > >
On Fri, 15 Nov 2024, Imre Deak <imre.deak@intel.com> wrote: > On Fri, Nov 15, 2024 at 03:20:58PM -0500, Rodrigo Vivi wrote: >> On Fri, Nov 15, 2024 at 06:41:56PM +0200, Imre Deak wrote: >> > The connector initialization in intel_dp_add_mst_connector() depends on >> > the device pointer in connector to be valid, at least by connector >> > debug printing. The device pointer is initialized by drm_connector_init(), >> > however that function also exposes the connector to in-kernel users, >> > which can't be done before the connector is fully initialized. For now >> > make sure the device pointer is valid before it's used, until a >> > follow-up change moving this to DRM core. >> > >> > This issue was revealed by the commit in the Fixes: line below, before >> > which the above debug printing checked and handled a NULL device pointer >> > gracefully in DRM core. >> > >> > Cc: Jani Nikula <jani.nikula@intel.com> >> > Fixes: 529798bd786a ("drm/i915/mst: convert to struct intel_display") >> >> This is awkward. This patch actually removes callers of base.dev. >> I don't see how that it could be causing this new null dereference. > > It adds > > struct intel_display *display = to_intel_display(connector); > > which will be NULL since connector->base.dev is NULL and later display > is dereferenced. So this happens in detect_dsc_hblank_expansion_quirk()? The changes were: - struct drm_i915_private *i915 = to_i915(connector->base.dev); + struct intel_display *display = to_intel_display(connector); - drm_dbg_kms(&i915->drm, + drm_dbg_kms(display->drm, And apparently i915 and &i915->drm were both NULL before, but the change turned it into a NULL pointer dereference. Why do we have to do this before drm_connector_init()? What if we just moved it after the connector init? What are the in-kernel users that can get called in between? Or if it's absolutely required to do all that before init, then pass the things to it instead of assuming the connector is ready? BR, Jani. > >> > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12799 >> >> But well, trusting more the tests then my eyes, let's move forward. >> >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> >> > Signed-off-by: Imre Deak <imre.deak@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++++++ >> > 1 file changed, 10 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> > index df7edcfe885b6..f058360a26413 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c >> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> > @@ -1727,6 +1727,16 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo >> > >> > intel_dp_init_modeset_retry_work(intel_connector); >> > >> > + /* >> > + * TODO: The following drm_connector specific initialization belongs >> > + * to DRM core, however it happens atm too late in >> > + * drm_connector_init(). That function will also expose the connector >> > + * to in-kernel users, so it can't be called until the connector is >> > + * sufficiently initialized; init the device pointer used by the >> > + * following DSC setup, until a fix moving this to DRM core. >> > + */ >> > + intel_connector->base.dev = mgr->dev; >> > + >> > intel_connector->dp.dsc_decompression_aux = drm_dp_mst_dsc_aux_for_port(port); >> > intel_dp_mst_read_decompression_port_dsc_caps(intel_dp, intel_connector); >> > intel_connector->dp.dsc_hblank_expansion_quirk = >> > -- >> > 2.44.2 >> >
On Mon, Nov 18, 2024 at 11:10:18AM +0200, Jani Nikula wrote: > On Fri, 15 Nov 2024, Imre Deak <imre.deak@intel.com> wrote: > > On Fri, Nov 15, 2024 at 03:20:58PM -0500, Rodrigo Vivi wrote: > >> On Fri, Nov 15, 2024 at 06:41:56PM +0200, Imre Deak wrote: > >> > The connector initialization in intel_dp_add_mst_connector() depends on > >> > the device pointer in connector to be valid, at least by connector > >> > debug printing. The device pointer is initialized by drm_connector_init(), > >> > however that function also exposes the connector to in-kernel users, > >> > which can't be done before the connector is fully initialized. For now > >> > make sure the device pointer is valid before it's used, until a > >> > follow-up change moving this to DRM core. > >> > > >> > This issue was revealed by the commit in the Fixes: line below, before > >> > which the above debug printing checked and handled a NULL device pointer > >> > gracefully in DRM core. > >> > > >> > Cc: Jani Nikula <jani.nikula@intel.com> > >> > Fixes: 529798bd786a ("drm/i915/mst: convert to struct intel_display") > >> > >> This is awkward. This patch actually removes callers of base.dev. > >> I don't see how that it could be causing this new null dereference. > > > > It adds > > > > struct intel_display *display = to_intel_display(connector); > > > > which will be NULL since connector->base.dev is NULL and later display > > is dereferenced. > > So this happens in detect_dsc_hblank_expansion_quirk()? > > The changes were: > > - struct drm_i915_private *i915 = to_i915(connector->base.dev); > + struct intel_display *display = to_intel_display(connector); > > - drm_dbg_kms(&i915->drm, > + drm_dbg_kms(display->drm, > > And apparently i915 and &i915->drm were both NULL before, but the change > turned it into a NULL pointer dereference. > > Why do we have to do this before drm_connector_init()? drm_connector_init() adds the connector to the connector list, which makes it visible to everything else that looks up the connector through this list. Those users should see the driver specific parts of connector already inited. > What if we just moved it after the connector init? What are the > in-kernel users that can get called in between? Detection on this connector could happen in between for instance. > Or if it's absolutely required to do all that before init, then pass the > things to it instead of assuming the connector is ready? Besides DSC all the other initializing steps in intel_dp_add_mst_connector() should happen before adding it to the connector list and those need a pointer to drm_connector. > BR, > Jani. > > > > > >> > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12799 > >> > >> But well, trusting more the tests then my eyes, let's move forward. > >> > >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> > >> > Signed-off-by: Imre Deak <imre.deak@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++++++ > >> > 1 file changed, 10 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > >> > index df7edcfe885b6..f058360a26413 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > >> > @@ -1727,6 +1727,16 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo > >> > > >> > intel_dp_init_modeset_retry_work(intel_connector); > >> > > >> > + /* > >> > + * TODO: The following drm_connector specific initialization belongs > >> > + * to DRM core, however it happens atm too late in > >> > + * drm_connector_init(). That function will also expose the connector > >> > + * to in-kernel users, so it can't be called until the connector is > >> > + * sufficiently initialized; init the device pointer used by the > >> > + * following DSC setup, until a fix moving this to DRM core. > >> > + */ > >> > + intel_connector->base.dev = mgr->dev; > >> > + > >> > intel_connector->dp.dsc_decompression_aux = drm_dp_mst_dsc_aux_for_port(port); > >> > intel_dp_mst_read_decompression_port_dsc_caps(intel_dp, intel_connector); > >> > intel_connector->dp.dsc_hblank_expansion_quirk = > >> > -- > >> > 2.44.2 > >> > > > -- > Jani Nikula, Intel
On Fri, 15 Nov 2024, Imre Deak <imre.deak@intel.com> wrote: > The connector initialization in intel_dp_add_mst_connector() depends on > the device pointer in connector to be valid, at least by connector > debug printing. The device pointer is initialized by drm_connector_init(), > however that function also exposes the connector to in-kernel users, > which can't be done before the connector is fully initialized. For now > make sure the device pointer is valid before it's used, until a > follow-up change moving this to DRM core. > > This issue was revealed by the commit in the Fixes: line below, before > which the above debug printing checked and handled a NULL device pointer > gracefully in DRM core. > > Cc: Jani Nikula <jani.nikula@intel.com> > Fixes: 529798bd786a ("drm/i915/mst: convert to struct intel_display") > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12799 > Signed-off-by: Imre Deak <imre.deak@intel.com> I think you should send this patch alone to intel-gfx and intel-xe to address the regression. The others can follow later, and likely be merged via drm-misc. I have some comments about them, but I don't want to block fixing the issue. BR, Jani. > --- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index df7edcfe885b6..f058360a26413 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -1727,6 +1727,16 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo > > intel_dp_init_modeset_retry_work(intel_connector); > > + /* > + * TODO: The following drm_connector specific initialization belongs > + * to DRM core, however it happens atm too late in > + * drm_connector_init(). That function will also expose the connector > + * to in-kernel users, so it can't be called until the connector is > + * sufficiently initialized; init the device pointer used by the > + * following DSC setup, until a fix moving this to DRM core. > + */ > + intel_connector->base.dev = mgr->dev; > + > intel_connector->dp.dsc_decompression_aux = drm_dp_mst_dsc_aux_for_port(port); > intel_dp_mst_read_decompression_port_dsc_caps(intel_dp, intel_connector); > intel_connector->dp.dsc_hblank_expansion_quirk =
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index df7edcfe885b6..f058360a26413 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -1727,6 +1727,16 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo intel_dp_init_modeset_retry_work(intel_connector); + /* + * TODO: The following drm_connector specific initialization belongs + * to DRM core, however it happens atm too late in + * drm_connector_init(). That function will also expose the connector + * to in-kernel users, so it can't be called until the connector is + * sufficiently initialized; init the device pointer used by the + * following DSC setup, until a fix moving this to DRM core. + */ + intel_connector->base.dev = mgr->dev; + intel_connector->dp.dsc_decompression_aux = drm_dp_mst_dsc_aux_for_port(port); intel_dp_mst_read_decompression_port_dsc_caps(intel_dp, intel_connector); intel_connector->dp.dsc_hblank_expansion_quirk =
The connector initialization in intel_dp_add_mst_connector() depends on the device pointer in connector to be valid, at least by connector debug printing. The device pointer is initialized by drm_connector_init(), however that function also exposes the connector to in-kernel users, which can't be done before the connector is fully initialized. For now make sure the device pointer is valid before it's used, until a follow-up change moving this to DRM core. This issue was revealed by the commit in the Fixes: line below, before which the above debug printing checked and handled a NULL device pointer gracefully in DRM core. Cc: Jani Nikula <jani.nikula@intel.com> Fixes: 529798bd786a ("drm/i915/mst: convert to struct intel_display") Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12799 Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++++++ 1 file changed, 10 insertions(+)