| Message ID | 20241009062455.1796615-1-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/hdcp: Move dig_port assignment lower in the sequence | expand |
On Wed, 09 Oct 2024, Suraj Kandpal <suraj.kandpal@intel.com> wrote: > Move dig_port assignment much lower in the sequence to avoid NULL > pointer deference in case encoder is not present. Please describe the case exactly. Is this real or a static analyzer warning? I see there's commit 6c63e6e14da7 ("drm/i915/hdcp: No HDCP when encoder is't initialized") adding the !connector->encoder check. But how was that supposed to fix anything when it leaves struct intel_digital_port *dig_port = intel_attached_dig_port(connector); above it in place, the line you're fixing now? If we haven't seen an oops here, it makes me think the reasoning in 6c63e6e14da7 is bogus, the connector->encoder check is bogus, and this fix on top is also bogus. OTOH if we have seen a real issue, we need the backtrace and the conditions triggering it, and we need to backport this. While it may seem benign and defensive to add extra NULL checks, one of the dangers is the cargo culting of those checks. Static analyzers see a check somewhere, so they complain about unchecked dereferences everywhere. Checks start cropping up everywhere, and people lose track of when it's actually possible for the pointers to be NULL, and when not. BR, Jani. > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > --- > drivers/gpu/drm/i915/display/intel_hdcp.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c > index ed6aa87403e2..ea8d56b25f6e 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c > @@ -2404,7 +2404,7 @@ static int _intel_hdcp_enable(struct intel_atomic_state *state, > struct drm_i915_private *i915 = to_i915(display->drm); > struct intel_connector *connector = > to_intel_connector(conn_state->connector); > - struct intel_digital_port *dig_port = intel_attached_dig_port(connector); > + struct intel_digital_port *dig_port; > struct intel_hdcp *hdcp = &connector->hdcp; > unsigned long check_link_interval = DRM_HDCP_CHECK_PERIOD_MS; > int ret = -EINVAL; > @@ -2418,6 +2418,8 @@ static int _intel_hdcp_enable(struct intel_atomic_state *state, > return -ENODEV; > } > > + dig_port = intel_attached_dig_port(connector); > + > mutex_lock(&hdcp->mutex); > mutex_lock(&dig_port->hdcp_mutex); > drm_WARN_ON(display->drm,
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Wednesday, October 9, 2024 3:20 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel- > xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > Cc: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Kandpal, Suraj > <suraj.kandpal@intel.com> > Subject: Re: [PATCH] drm/i915/hdcp: Move dig_port assignment lower in the > sequence > > On Wed, 09 Oct 2024, Suraj Kandpal <suraj.kandpal@intel.com> wrote: > > Move dig_port assignment much lower in the sequence to avoid NULL > > pointer deference in case encoder is not present. > > Please describe the case exactly. Is this real or a static analyzer warning? Yes this was a static analyzer warning > > I see there's commit 6c63e6e14da7 ("drm/i915/hdcp: No HDCP when > encoder is't initialized") adding the !connector->encoder check. But how > was that supposed to fix anything when it leaves > > struct intel_digital_port *dig_port = > intel_attached_dig_port(connector); > > above it in place, the line you're fixing now? > > If we haven't seen an oops here, it makes me think the reasoning in > 6c63e6e14da7 is bogus, the connector->encoder check is bogus, and this fix > on top is also bogus. > > OTOH if we have seen a real issue, we need the backtrace and the > conditions triggering it, and we need to backport this. > > While it may seem benign and defensive to add extra NULL checks, one of > the dangers is the cargo culting of those checks. Static analyzers see a check > somewhere, so they complain about unchecked dereferences everywhere. > Checks start cropping up everywhere, and people lose track of when it's > actually possible for the pointers to be NULL, and when not. That change was brought about because I had seen a oops when hdcp_capable was being called from debugfs And I thought it would be better to fail safe other places too but I see you point how the code may end up getting Cluttering up with useless checks Regards, Suraj Kandpal > > BR, > Jani. > > > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_hdcp.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c > > b/drivers/gpu/drm/i915/display/intel_hdcp.c > > index ed6aa87403e2..ea8d56b25f6e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c > > @@ -2404,7 +2404,7 @@ static int _intel_hdcp_enable(struct > intel_atomic_state *state, > > struct drm_i915_private *i915 = to_i915(display->drm); > > struct intel_connector *connector = > > to_intel_connector(conn_state->connector); > > - struct intel_digital_port *dig_port = > intel_attached_dig_port(connector); > > + struct intel_digital_port *dig_port; > > struct intel_hdcp *hdcp = &connector->hdcp; > > unsigned long check_link_interval = DRM_HDCP_CHECK_PERIOD_MS; > > int ret = -EINVAL; > > @@ -2418,6 +2418,8 @@ static int _intel_hdcp_enable(struct > intel_atomic_state *state, > > return -ENODEV; > > } > > > > + dig_port = intel_attached_dig_port(connector); > > + > > mutex_lock(&hdcp->mutex); > > mutex_lock(&dig_port->hdcp_mutex); > > drm_WARN_ON(display->drm, > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index ed6aa87403e2..ea8d56b25f6e 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -2404,7 +2404,7 @@ static int _intel_hdcp_enable(struct intel_atomic_state *state, struct drm_i915_private *i915 = to_i915(display->drm); struct intel_connector *connector = to_intel_connector(conn_state->connector); - struct intel_digital_port *dig_port = intel_attached_dig_port(connector); + struct intel_digital_port *dig_port; struct intel_hdcp *hdcp = &connector->hdcp; unsigned long check_link_interval = DRM_HDCP_CHECK_PERIOD_MS; int ret = -EINVAL; @@ -2418,6 +2418,8 @@ static int _intel_hdcp_enable(struct intel_atomic_state *state, return -ENODEV; } + dig_port = intel_attached_dig_port(connector); + mutex_lock(&hdcp->mutex); mutex_lock(&dig_port->hdcp_mutex); drm_WARN_ON(display->drm,
Move dig_port assignment much lower in the sequence to avoid NULL pointer deference in case encoder is not present. Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- drivers/gpu/drm/i915/display/intel_hdcp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)