* [PATCH 2/3] drm/display/dp_mst: Fix drm_atomic_get_mst_topology_state() [not found] <20220602201757.30431-1-lyude@redhat.com> @ 2022-06-02 20:17 ` Lyude Paul 2022-06-02 20:42 ` Ville Syrjälä 0 siblings, 1 reply; 3+ messages in thread From: Lyude Paul @ 2022-06-02 20:17 UTC (permalink / raw) To: dri-devel Cc: Ville Syrjälä, stable, David Airlie, Daniel Vetter, Thomas Zimmermann, Jani Nikula, Wayne Lin, Bhawanpreet Lakha, Imran Khan, Javier Martinez Canillas, Fangzhi Zuo, open list 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/3] drm/display/dp_mst: Fix drm_atomic_get_mst_topology_state() 2022-06-02 20:17 ` [PATCH 2/3] drm/display/dp_mst: Fix drm_atomic_get_mst_topology_state() Lyude Paul @ 2022-06-02 20:42 ` Ville Syrjälä 2022-06-02 20:43 ` Lyude Paul 0 siblings, 1 reply; 3+ messages in thread From: Ville Syrjälä @ 2022-06-02 20:42 UTC (permalink / raw) To: Lyude Paul Cc: dri-devel, stable, David Airlie, Daniel Vetter, Thomas Zimmermann, Jani Nikula, Wayne Lin, Bhawanpreet Lakha, Imran Khan, Javier Martinez Canillas, Fangzhi Zuo, open list 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/3] drm/display/dp_mst: Fix drm_atomic_get_mst_topology_state() 2022-06-02 20:42 ` Ville Syrjälä @ 2022-06-02 20:43 ` Lyude Paul 0 siblings, 0 replies; 3+ messages in thread From: Lyude Paul @ 2022-06-02 20:43 UTC (permalink / raw) To: Ville Syrjälä Cc: dri-devel, stable, David Airlie, Daniel Vetter, Thomas Zimmermann, Jani Nikula, Wayne Lin, Bhawanpreet Lakha, Imran Khan, Javier Martinez Canillas, Fangzhi Zuo, open list 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-06-02 20:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220602201757.30431-1-lyude@redhat.com>
2022-06-02 20:17 ` [PATCH 2/3] drm/display/dp_mst: Fix drm_atomic_get_mst_topology_state() Lyude Paul
2022-06-02 20:42 ` Ville Syrjälä
2022-06-02 20:43 ` Lyude Paul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox