| Message ID | 20161209141944.22121-5-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 09, 2016 at 03:19:42PM +0100, Daniel Vetter wrote: > Looping when we keep track of this is silly. Only thing we have to > be careful is with sampling the connector count. To avoid inconsisten > results due to gcc re-computing this, use READ_ONCE. Later on in the function we take the mutex that should prevent the values from changing. If each block was like mutex_lock(&mode_config->lockc); count = dev->mode_config.num_crtc; if (card_res->count_crtcs >= count) { u32 __user id = u64_to_user_ptr(card_res->crtc_id_ptr); unsigned int copied = 0; drm_for_each_crtc(crtc, dev) { if (copied >= count) break; if (put_user(crtc->base.id, id + copied)) { ret = -EFAULT; goto out; } copied++; } count = copied; } card_res->count_crtcs = count; ... mutex_unlock(&mode_config->lockc); it would look a bit neater. -Chris
On Fri, Dec 09, 2016 at 09:47:56PM +0000, Chris Wilson wrote: > On Fri, Dec 09, 2016 at 03:19:42PM +0100, Daniel Vetter wrote: > > Looping when we keep track of this is silly. Only thing we have to > > be careful is with sampling the connector count. To avoid inconsisten > > results due to gcc re-computing this, use READ_ONCE. > > Later on in the function we take the mutex that should prevent the > values from changing. > > If each block was like > > mutex_lock(&mode_config->lockc); Well that mutex_lock is fiction too. It's not needed for any of the lists, and it definitely doesn't protect connector_list. Step-by-step I'd say ... -Daniel > > count = dev->mode_config.num_crtc; > if (card_res->count_crtcs >= count) { > u32 __user id = u64_to_user_ptr(card_res->crtc_id_ptr); > unsigned int copied = 0; > > drm_for_each_crtc(crtc, dev) { > if (copied >= count) > break; > > if (put_user(crtc->base.id, id + copied)) { > ret = -EFAULT; > goto out; > } > > copied++; > } > > count = copied; > } > card_res->count_crtcs = count; > > ... > > mutex_unlock(&mode_config->lockc); > > it would look a bit neater. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 2735a5847ffa..64c2971478db 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -90,10 +90,10 @@ int drm_mode_getresources(struct drm_device *dev, void *data, struct drm_crtc *crtc; struct drm_encoder *encoder; int ret = 0; - int connector_count = 0; - int crtc_count = 0; + int connector_count = READ_ONCE(dev->mode_config.num_connector); + int crtc_count = dev->mode_config.num_crtc; int fb_count = 0; - int encoder_count = 0; + int encoder_count = dev->mode_config.num_encoder; int copied = 0; uint32_t __user *fb_id; uint32_t __user *crtc_id; @@ -131,15 +131,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data, /* mode_config.mutex protects the connector list against e.g. DP MST * connector hot-adding. CRTC/Plane lists are invariant. */ mutex_lock(&dev->mode_config.mutex); - drm_for_each_crtc(crtc, dev) - crtc_count++; - - drm_for_each_connector(connector, dev) - connector_count++; - - drm_for_each_encoder(encoder, dev) - encoder_count++; - card_res->max_height = dev->mode_config.max_height; card_res->min_height = dev->mode_config.min_height; card_res->max_width = dev->mode_config.max_width; @@ -179,6 +170,9 @@ int drm_mode_getresources(struct drm_device *dev, void *data, copied = 0; connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr; drm_for_each_connector(connector, dev) { + if (copied >= connector_count) + break; + if (put_user(connector->base.id, connector_id + copied)) { ret = -EFAULT; @@ -186,6 +180,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, } copied++; } + connector_count = copied; } card_res->count_connectors = connector_count;
Looping when we keep track of this is silly. Only thing we have to be careful is with sampling the connector count. To avoid inconsisten results due to gcc re-computing this, use READ_ONCE. And to avoid surprising userspace, make sure we don't copy more connectors than planned, and report the actual number of connectors copied. That way any racing hot-add/remove will be handled. v2: Actually try to not blow up, somehow I lost the hunk that checks we don't copy too much. Noticed by Chris. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_mode_config.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)