[PATCH 1/3] drm/display/dp_mst: Don't validate port refs in drm_dp_check_and_send_link_address()

[PATCH 1/3] drm/display/dp_mst: Don't validate port refs in drm_dp_check_and_send_link_address()

[Lyude Paul]

Drive-by cleanup, we don't need to validate the port references here as we
already previously went through the effort of refactoring things such that
we're guaranteed to be able to access ->mstb and ->port safely from
drm_dp_check_and_send_link_address(), since the only two places in the
codebase that drop an MST reference in such a way that it would remove it
from the topology are both protected under probe_lock.

Thanks for that, past Lyude!

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +++++--------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 67b3b9697da7..d84673b3294b 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -2666,24 +2666,14 @@ static int drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *mg
 	}
 
 	list_for_each_entry(port, &mstb->ports, next) {
-		struct drm_dp_mst_branch *mstb_child = NULL;
-
-		if (port->input || !port->ddps)
+		if (port->input || !port->ddps || !port->mstb)
 			continue;
 
-		if (port->mstb)
-			mstb_child = drm_dp_mst_topology_get_mstb_validated(
-			    mgr, port->mstb);
-
-		if (mstb_child) {
-			ret = drm_dp_check_and_send_link_address(mgr,
-								 mstb_child);
-			drm_dp_mst_topology_put_mstb(mstb_child);
-			if (ret == 1)
-				changed = true;
-			else if (ret < 0)
-				return ret;
-		}
+		ret = drm_dp_check_and_send_link_address(mgr, port->mstb);
+		if (ret == 1)
+			changed = true;
+		else if (ret < 0)
+			return ret;
 	}
 
 	return changed;
-- 
2.35.3

RE: [PATCH 1/3] drm/display/dp_mst: Don't validate port refs in drm_dp_check_and_send_link_address()

[Lin, Wayne]

[Public]

Thanks, Lyude.

Feel free to add
Reviewed-by: Wayne Lin <Wayne.Lin@amd.com>

> -----Original Message-----
> From: Lyude Paul <lyude@redhat.com>
> Sent: Friday, June 3, 2022 4:18 AM
> To: dri-devel@lists.freedesktop.org
> Cc: David Airlie <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>; Thomas
> Zimmermann <tzimmermann@suse.de>; Lin, Wayne
> <Wayne.Lin@amd.com>; Jani Nikula <jani.nikula@intel.com>; Lakha,
> Bhawanpreet <Bhawanpreet.Lakha@amd.com>; Rajkumar Subbiah
> <rsubbia@codeaurora.org>; open list <linux-kernel@vger.kernel.org>
> Subject: [PATCH 1/3] drm/display/dp_mst: Don't validate port refs in
> drm_dp_check_and_send_link_address()
> 
> Drive-by cleanup, we don't need to validate the port references here as we
> already previously went through the effort of refactoring things such that
> we're guaranteed to be able to access ->mstb and ->port safely from
> drm_dp_check_and_send_link_address(), since the only two places in the
> codebase that drop an MST reference in such a way that it would remove it
> from the topology are both protected under probe_lock.
> 
> Thanks for that, past Lyude!
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +++++--------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 67b3b9697da7..d84673b3294b 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -2666,24 +2666,14 @@ static int
> drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr
> *mg
>  	}
> 
>  	list_for_each_entry(port, &mstb->ports, next) {
> -		struct drm_dp_mst_branch *mstb_child = NULL;
> -
> -		if (port->input || !port->ddps)
> +		if (port->input || !port->ddps || !port->mstb)
>  			continue;
> 
> -		if (port->mstb)
> -			mstb_child =
> drm_dp_mst_topology_get_mstb_validated(
> -			    mgr, port->mstb);
> -
> -		if (mstb_child) {
> -			ret = drm_dp_check_and_send_link_address(mgr,
> -								 mstb_child);
> -			drm_dp_mst_topology_put_mstb(mstb_child);
> -			if (ret == 1)
> -				changed = true;
> -			else if (ret < 0)
> -				return ret;
> -		}
> +		ret = drm_dp_check_and_send_link_address(mgr, port-
> >mstb);
> +		if (ret == 1)
> +			changed = true;
> +		else if (ret < 0)
> +			return ret;
>  	}
> 
>  	return changed;
> --
> 2.35.3

--
Regards,
Wayne

[PATCH 2/3] drm/display/dp_mst: Fix drm_atomic_get_mst_topology_state()

[Lyude Paul]

I noticed a rather surprising issue here while working on removing all of
the non-atomic MST code: drm_atomic_get_mst_topology_state() doesn't check
the return value of drm_atomic_get_private_obj_state() and instead just
passes it directly to to_dp_mst_topology_state(). This means that if we
hit a deadlock or something else which would return an error code pointer,
we'll likely segfault the kernel.

This is definitely another one of those fixes where I'm astonished we
somehow managed never to discover this issue until now…

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: a4370c777406 ("drm/atomic: Make private objs proper objects")
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.14+
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +-
 include/drm/display/drm_dp_mst_helper.h       | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index d84673b3294b..d6e595b95f07 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -5468,7 +5468,7 @@ EXPORT_SYMBOL(drm_dp_mst_topology_state_funcs);
 struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
 								    struct drm_dp_mst_topology_mgr *mgr)
 {
-	return to_dp_mst_topology_state(drm_atomic_get_private_obj_state(state, &mgr->base));
+	return to_dp_mst_topology_state_safe(drm_atomic_get_private_obj_state(state, &mgr->base));
 }
 EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
 
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 10adec068b7f..fe7577e7f305 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -541,6 +541,8 @@ struct drm_dp_payload {
 };
 
 #define to_dp_mst_topology_state(x) container_of(x, struct drm_dp_mst_topology_state, base)
+#define to_dp_mst_topology_state_safe(x) \
+	container_of_safe(x, struct drm_dp_mst_topology_state, base)
 
 struct drm_dp_vcpi_allocation {
 	struct drm_dp_mst_port *port;
-- 
2.35.3

Re: [PATCH 2/3] drm/display/dp_mst: Fix drm_atomic_get_mst_topology_state()

[Ville Syrjälä]

On Thu, Jun 02, 2022 at 04:17:56PM -0400, Lyude Paul wrote:
> I noticed a rather surprising issue here while working on removing all of
> the non-atomic MST code: drm_atomic_get_mst_topology_state() doesn't check
> the return value of drm_atomic_get_private_obj_state() and instead just
> passes it directly to to_dp_mst_topology_state(). This means that if we
> hit a deadlock or something else which would return an error code pointer,
> we'll likely segfault the kernel.
> 
> This is definitely another one of those fixes where I'm astonished we
> somehow managed never to discover this issue until now…

It has been discussed before.

struct drm_dp_mst_topology_state {
	struct drm_private_state base;
	...
}

so offsetof(base)==0.

> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Fixes: a4370c777406 ("drm/atomic: Make private objs proper objects")
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.14+
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +-
>  include/drm/display/drm_dp_mst_helper.h       | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index d84673b3294b..d6e595b95f07 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -5468,7 +5468,7 @@ EXPORT_SYMBOL(drm_dp_mst_topology_state_funcs);
>  struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
>  								    struct drm_dp_mst_topology_mgr *mgr)
>  {
> -	return to_dp_mst_topology_state(drm_atomic_get_private_obj_state(state, &mgr->base));
> +	return to_dp_mst_topology_state_safe(drm_atomic_get_private_obj_state(state, &mgr->base));
>  }
>  EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
>  
> diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> index 10adec068b7f..fe7577e7f305 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -541,6 +541,8 @@ struct drm_dp_payload {
>  };
>  
>  #define to_dp_mst_topology_state(x) container_of(x, struct drm_dp_mst_topology_state, base)
> +#define to_dp_mst_topology_state_safe(x) \
> +	container_of_safe(x, struct drm_dp_mst_topology_state, base)

Wasn't aware of container_of_safe(). I suppose no real harm 
in using it. Not sure why we'd even keep the non-safe version
around?

Though the use of container_of_safe() everywhere won't help
when "casting" the other way (&foo->base, when foo==NULL/errptr).
In order to make that work for non-zero offsets we'd have to
introduce a casting macro for that direction as well.

>  
>  struct drm_dp_vcpi_allocation {
>  	struct drm_dp_mst_port *port;
> -- 
> 2.35.3

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/3] drm/display/dp_mst: Fix drm_atomic_get_mst_topology_state()

[Lyude Paul]

On Thu, 2022-06-02 at 23:42 +0300, Ville Syrjälä wrote:
> On Thu, Jun 02, 2022 at 04:17:56PM -0400, Lyude Paul wrote:
> > I noticed a rather surprising issue here while working on removing all of
> > the non-atomic MST code: drm_atomic_get_mst_topology_state() doesn't check
> > the return value of drm_atomic_get_private_obj_state() and instead just
> > passes it directly to to_dp_mst_topology_state(). This means that if we
> > hit a deadlock or something else which would return an error code pointer,
> > we'll likely segfault the kernel.
> > 
> > This is definitely another one of those fixes where I'm astonished we
> > somehow managed never to discover this issue until now…
> 
> It has been discussed before.
> 
> struct drm_dp_mst_topology_state {
>         struct drm_private_state base;
>         ...
> }
> 
> so offsetof(base)==0.

fhjdffds yes you're right, I guess we can just drop this patch then

> 
> > 
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Fixes: a4370c777406 ("drm/atomic: Make private objs proper objects")
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v4.14+
> > ---
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +-
> >  include/drm/display/drm_dp_mst_helper.h       | 2 ++
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index d84673b3294b..d6e595b95f07 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -5468,7 +5468,7 @@ EXPORT_SYMBOL(drm_dp_mst_topology_state_funcs);
> >  struct drm_dp_mst_topology_state
> > *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> >                                                                     struct
> > drm_dp_mst_topology_mgr *mgr)
> >  {
> > -       return
> > to_dp_mst_topology_state(drm_atomic_get_private_obj_state(state, &mgr-
> > >base));
> > +       return
> > to_dp_mst_topology_state_safe(drm_atomic_get_private_obj_state(state,
> > &mgr->base));
> >  }
> >  EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
> >  
> > diff --git a/include/drm/display/drm_dp_mst_helper.h
> > b/include/drm/display/drm_dp_mst_helper.h
> > index 10adec068b7f..fe7577e7f305 100644
> > --- a/include/drm/display/drm_dp_mst_helper.h
> > +++ b/include/drm/display/drm_dp_mst_helper.h
> > @@ -541,6 +541,8 @@ struct drm_dp_payload {
> >  };
> >  
> >  #define to_dp_mst_topology_state(x) container_of(x, struct
> > drm_dp_mst_topology_state, base)
> > +#define to_dp_mst_topology_state_safe(x) \
> > +       container_of_safe(x, struct drm_dp_mst_topology_state, base)
> 
> Wasn't aware of container_of_safe(). I suppose no real harm 
> in using it. Not sure why we'd even keep the non-safe version
> around?
> 
> Though the use of container_of_safe() everywhere won't help
> when "casting" the other way (&foo->base, when foo==NULL/errptr).
> In order to make that work for non-zero offsets we'd have to
> introduce a casting macro for that direction as well.
> 
> >  
> >  struct drm_dp_vcpi_allocation {
> >         struct drm_dp_mst_port *port;
> > -- 
> > 2.35.3
> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

[PATCH 3/3] drm/dp_mst: Get rid of old comment in drm_atomic_get_mst_topology_state docs

[Lyude Paul]

We don't actually care about connection_mutex here anymore, so let's get
rid of the comment mentioning it in this function's kdocs.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index d6e595b95f07..9f96132a5d74 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -5458,8 +5458,7 @@ EXPORT_SYMBOL(drm_dp_mst_topology_state_funcs);
  *
  * This function wraps drm_atomic_get_priv_obj_state() passing in the MST atomic
  * state vtable so that the private object state returned is that of a MST
- * topology object. Also, drm_atomic_get_private_obj_state() expects the caller
- * to care of the locking, so warn if don't hold the connection_mutex.
+ * topology object.
  *
  * RETURNS:
  *
-- 
2.35.3

RE: [PATCH 3/3] drm/dp_mst: Get rid of old comment in drm_atomic_get_mst_topology_state docs

[Lin, Wayne]

[Public]

Hi Lyude,

Feel free to add
Reviewed-by: Wayne Lin <Wayne.Lin@amd.com>

> -----Original Message-----
> From: Lyude Paul <lyude@redhat.com>
> Sent: Friday, June 3, 2022 4:18 AM
> To: dri-devel@lists.freedesktop.org
> Cc: David Airlie <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>; Thomas
> Zimmermann <tzimmermann@suse.de>; Lin, Wayne
> <Wayne.Lin@amd.com>; Jani Nikula <jani.nikula@intel.com>; Lakha,
> Bhawanpreet <Bhawanpreet.Lakha@amd.com>; Rajkumar Subbiah
> <rsubbia@codeaurora.org>; open list <linux-kernel@vger.kernel.org>
> Subject: [PATCH 3/3] drm/dp_mst: Get rid of old comment in
> drm_atomic_get_mst_topology_state docs
> 
> We don't actually care about connection_mutex here anymore, so let's get
> rid of the comment mentioning it in this function's kdocs.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index d6e595b95f07..9f96132a5d74 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -5458,8 +5458,7 @@
> EXPORT_SYMBOL(drm_dp_mst_topology_state_funcs);
>   *
>   * This function wraps drm_atomic_get_priv_obj_state() passing in the MST
> atomic
>   * state vtable so that the private object state returned is that of a MST
> - * topology object. Also, drm_atomic_get_private_obj_state() expects the
> caller
> - * to care of the locking, so warn if don't hold the connection_mutex.
> + * topology object.
>   *
>   * RETURNS:
>   *
> --
> 2.35.3

--
Regards,
Wayne Lin

Re: [PATCH 3/3] drm/dp_mst: Get rid of old comment in drm_atomic_get_mst_topology_state docs

[Lyude Paul]

Thanks! Will push these two patches upstream

On Mon, 2022-06-13 at 09:59 +0000, Lin, Wayne wrote:
> [Public]
> 
> Hi Lyude,
> 
> Feel free to add
> Reviewed-by: Wayne Lin <Wayne.Lin@amd.com>
> 
> > -----Original Message-----
> > From: Lyude Paul <lyude@redhat.com>
> > Sent: Friday, June 3, 2022 4:18 AM
> > To: dri-devel@lists.freedesktop.org
> > Cc: David Airlie <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>;
> > Thomas
> > Zimmermann <tzimmermann@suse.de>; Lin, Wayne
> > <Wayne.Lin@amd.com>; Jani Nikula <jani.nikula@intel.com>; Lakha,
> > Bhawanpreet <Bhawanpreet.Lakha@amd.com>; Rajkumar Subbiah
> > <rsubbia@codeaurora.org>; open list <linux-kernel@vger.kernel.org>
> > Subject: [PATCH 3/3] drm/dp_mst: Get rid of old comment in
> > drm_atomic_get_mst_topology_state docs
> > 
> > We don't actually care about connection_mutex here anymore, so let's get
> > rid of the comment mentioning it in this function's kdocs.
> > 
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > ---
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index d6e595b95f07..9f96132a5d74 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -5458,8 +5458,7 @@
> > EXPORT_SYMBOL(drm_dp_mst_topology_state_funcs);
> >   *
> >   * This function wraps drm_atomic_get_priv_obj_state() passing in the MST
> > atomic
> >   * state vtable so that the private object state returned is that of a
> > MST
> > - * topology object. Also, drm_atomic_get_private_obj_state() expects the
> > caller
> > - * to care of the locking, so warn if don't hold the connection_mutex.
> > + * topology object.
> >   *
> >   * RETURNS:
> >   *
> > --
> > 2.35.3
> 
> --
> Regards,
> Wayne Lin
> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat