On Sat, Dec 10, 2016 at 10:52:55PM +0100, Daniel Vetter wrote:
> Looping twice when we can do it once is silly. Also use a consistent
> style. Note that there's a good race with the connector list walking,
> since that is no longer protected by mode_config.mutex. But that's for
> a later patch to fix.
>
> v2: Actually try to not blow up, somehow I lost the hunk that checks
> we don't copy too much. Noticed by Chris.
>
> v3:
> - squash all drm_mode_getresources cleanups into one
> - use consistent style for walking objects (Chris)
>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
> drivers/gpu/drm/drm_mode_config.c | 119
> +++++++++++++++-----------------------
> 1 file changed, 47 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c
> b/drivers/gpu/drm/drm_mode_config.c
> index 2735a5847ffa..54e371dac694 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -84,17 +84,11 @@ int drm_mode_getresources(struct drm_device *dev, void
> *data,
> struct drm_file *file_priv)
> {
> struct drm_mode_card_res *card_res = data;
> - struct list_head *lh;
> struct drm_framebuffer *fb;
> struct drm_connector *connector;
> struct drm_crtc *crtc;
> struct drm_encoder *encoder;
> - int ret = 0;
> - int connector_count = 0;
> - int crtc_count = 0;
> - int fb_count = 0;
> - int encoder_count = 0;
> - int copied = 0;
> + int count, ret = 0;
> uint32_t __user *fb_id;
> uint32_t __user *crtc_id;
> uint32_t __user *connector_id;
> @@ -105,89 +99,70 @@ int drm_mode_getresources(struct drm_device *dev, void
> *data,
>
>
> mutex_lock(&file_priv->fbs_lock);
> - /*
> - * For the non-control nodes we need to limit the list of resources
> - * by IDs in the group list for this node
> - */
> - list_for_each(lh, &file_priv->fbs)
> - fb_count++;
> -
> - /* handle this in 4 parts */
> - /* FBs */
> - if (card_res->count_fbs >= fb_count) {
> - copied = 0;
> - fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
> - list_for_each_entry(fb, &file_priv->fbs, filp_head) {
> - if (put_user(fb->base.id, fb_id + copied)) {
> - mutex_unlock(&file_priv->fbs_lock);
> - return -EFAULT;
> - }
> - copied++;
> + count = 0;
> + fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
u64_to_user_ptr() please :)
> + list_for_each_entry(fb, &file_priv->fbs, filp_head) {
> + count++;
> + if (count > card_res->count_fbs)
> + continue;
> +
> + if (put_user(fb->base.id, fb_id + count)) {
In this style increment of count has to happen after the copy.
i.e.
if (count < card_res->count_fbs &&
put_user(fb->base.id, fb_id + count) {
mutex_unlock()
return -EFAULT;
}
count++;
> + mutex_unlock(&file_priv->fbs_lock);
> + return -EFAULT;
> }
> }
> - card_res->count_fbs = fb_count;
> + card_res->count_fbs = count;
> mutex_unlock(&file_priv->fbs_lock);
--
Chris Wilson, Intel Open Source Technology Centre
