Re: [PATCH 2/8] drm: Document use-after-free gotcha with private objects

[Daniel Vetter <daniel@ffwll.ch>]

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@cerno.tech>
> ---
>  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@ffwll.ch>



>   */
>  struct drm_private_obj {
>  	/**
> -- 
> 2.28.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel