On Fri, Nov 13, 2020 at 04:29:50PM +0100, Maxime Ripard wrote:
> The private objects have a gotcha that could result in a use-after-free,
> make sure it's properly documented.
>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> ---
> include/drm/drm_atomic.h | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 413fd0ca56a8..24b52b3a459f 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -248,6 +248,24 @@ struct drm_private_state_funcs {
> * drm_dev_register()
> * 2/ all calls to drm_atomic_private_obj_fini() must be done after calling
> * drm_dev_unregister()
> + *
> + * If that private object is used to store a state shared my multiple
s/my/by/
> + * CRTCs, proper care must be taken to ensure that non-blocking commits are
> + * properly ordered to avoid a use-after-free issue.
> + *
> + * Indeed, assuming a sequence of two non-blocking commits on two different
> + * CRTCs using different planes and connectors, so with no resources shared,
> + * there's no guarantee on which commit is going to happen first. However, the
> + * second commit will consider the first private state its old state, and will
> + * be in charge of freeing it whenever the second commit is done.
> + *
> + * If the first commit happens after it, it will consider its private state the
> + * new state and will be likely to access it, resulting in an access to a freed
> + * memory region. A way to circumvent this is to store (and get a reference to)
s/A way to circumvent/Driver should/
And maybe make the paragraph break here and remove the previous one in the
middle of your example.
> + * the crtc commit in our private state in
&struct drm_crtc_commit so it's linked properly
> + * &drm_mode_config_helper_funcs.atomic_commit_setup, and then wait for that
> + * commit to complete as part of
s/as part of/as the first step of/
> + * &drm_mode_config_helper_funcs.atomic_commit_tail.
And maybe add "... similar to drm_atomic_helper_wait_for_dependencies()"
With the nits addressed:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> */
> struct drm_private_obj {
> /**
> --
> 2.28.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
