No one looks at the major/minor versions except the unique/busid stuff. If we protect that with the master_mutex (since it also affects the unique of each master, oh well) we can mark these two IOCTL with DRM_UNLOCKED.
While doing this I realized that the comment for the magic_map is outdated, I've forgotten to update it in:
commit d2b34ee62b409a03c6fe43c07b779983be51d017 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Fri Jun 17 09:33:21 2016 +0200
drm: Protect authmagic with master_mutex
Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Emil Velikov emil.l.velikov@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_ioctl.c | 12 +++++++++--- include/drm/drm_auth.h | 17 +++++++++++++---- 2 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 17e941a533bd..42a17ea5d801 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -115,11 +115,15 @@ static int drm_getunique(struct drm_device *dev, void *data, struct drm_unique *u = data; struct drm_master *master = file_priv->master;
+ mutex_lock(&master->dev->master_mutex); if (u->unique_len >= master->unique_len) { - if (copy_to_user(u->unique, master->unique, master->unique_len)) + if (copy_to_user(u->unique, master->unique, master->unique_len)) { + mutex_unlock(&master->dev->master_mutex); return -EFAULT; + } } u->unique_len = master->unique_len; + mutex_unlock(&master->dev->master_mutex);
return 0; } @@ -340,6 +344,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f struct drm_set_version *sv = data; int if_version, retcode = 0;
+ mutex_lock(&dev->master_mutex); if (sv->drm_di_major != -1) { if (sv->drm_di_major != DRM_IF_MAJOR || sv->drm_di_minor < 0 || sv->drm_di_minor > DRM_IF_MINOR) { @@ -374,6 +379,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f sv->drm_di_minor = DRM_IF_MINOR; sv->drm_dd_major = dev->driver->major; sv->drm_dd_minor = dev->driver->minor; + mutex_unlock(&dev->master_mutex);
return retcode; } @@ -528,7 +534,7 @@ EXPORT_SYMBOL(drm_ioctl_permit); static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_VERSION, drm_version, DRM_UNLOCKED|DRM_RENDER_ALLOW|DRM_CONTROL_ALLOW), - DRM_IOCTL_DEF(DRM_IOCTL_GET_UNIQUE, drm_getunique, 0), + DRM_IOCTL_DEF(DRM_IOCTL_GET_UNIQUE, drm_getunique, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_GET_MAGIC, drm_getmagic, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_IRQ_BUSID, drm_irq_by_busid, DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF(DRM_IOCTL_GET_MAP, drm_legacy_getmap_ioctl, DRM_UNLOCKED), @@ -536,7 +542,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_GET_STATS, drm_getstats, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_GET_CAP, drm_getcap, DRM_UNLOCKED|DRM_RENDER_ALLOW), DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, 0), - DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_MASTER), + DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_UNLOCKED | DRM_MASTER),
DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF(DRM_IOCTL_BLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h index 610223b0481b..155588eb8ccf 100644 --- a/include/drm/drm_auth.h +++ b/include/drm/drm_auth.h @@ -33,10 +33,7 @@ * * @refcount: Refcount for this master object. * @dev: Link back to the DRM device - * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex. - * @unique_len: Length of unique field. Protected by drm_global_mutex. - * @magic_map: Map of used authentication tokens. Protected by struct_mutex. - * @lock: DRI lock information. + * @lock: DRI1 lock information. * @driver_priv: Pointer to driver-private information. * * Note that master structures are only relevant for the legacy/primary device @@ -45,8 +42,20 @@ struct drm_master { struct kref refcount; struct drm_device *dev; + /** + * @unique: Unique identifier: e.g. busid. Protected by struct + * &drm_device master_mutex. + */ char *unique; + /** + * @unique_len: Length of unique field. Protected by struct &drm_device + * master_mutex. + */ int unique_len; + /** + * @magic_map: Map of used authentication tokens. Protected by struct + * &drm_device master_mutex. + */ struct idr magic_map; struct drm_lock_data lock; void *driver_priv;
It only updates per-file feature flags. And all the ioctl which change behaviour depending upon these flags (they're all kms features) do _not_ hold the BKL. Therefor this is pure cargo-cult and can be removed.
Note that there's a risk that the ioctl will behave inconsistently, but that's ok. The only thing it's not allowed to do is oops the kernel, and from an audit all places are safe.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 42a17ea5d801..e1b2c372f021 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -541,7 +541,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_GET_CLIENT, drm_getclient, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_GET_STATS, drm_getstats, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_GET_CAP, drm_getcap, DRM_UNLOCKED|DRM_RENDER_ALLOW), - DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, 0), + DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_UNLOCKED | DRM_MASTER),
DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
On Sat, Dec 10, 2016 at 10:52:53PM +0100, Daniel Vetter wrote:
It only updates per-file feature flags. And all the ioctl which change behaviour depending upon these flags (they're all kms features) do _not_ hold the BKL. Therefor this is pure cargo-cult and can be removed.
Note that there's a risk that the ioctl will behave inconsistently,
+ if userspace is racing with itself,
but that's ok. The only thing it's not allowed to do is oops the kernel, and from an audit all places are safe.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
With the last round of changes all ioctls called by modern drivers now have their own locking. Everything else is only allowed for legacy drivers and hence the lack of locking doesn't matter.
One exception is nouveau, due to the DRIVER_KMS_LEGACY_CONTEXT flag. But that only works its magic on the context and bufs ioctls. And drm_bufs.c is protected with dev->struct_mutex, and drm_context.c by the same and dev->ctxlist_mutex. That should be all safe, and we can finally mandata drm-bkl-less ioctls for everyone!
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_ioctl.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index e1b2c372f021..e21a18ac968e 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -735,9 +735,8 @@ long drm_ioctl(struct file *filp, if (ksize > in_size) memset(kdata + in_size, 0, ksize - in_size);
- /* Enforce sane locking for modern driver ioctls. Core ioctls are - * too messy still. */ - if ((!drm_core_check_feature(dev, DRIVER_LEGACY) && is_driver_ioctl) || + /* Enforce sane locking for modern driver ioctls. */ + if (!drm_core_check_feature(dev, DRIVER_LEGACY) || (ioctl->flags & DRM_UNLOCKED)) retcode = func(dev, kdata, file_priv); else {
On Sat, Dec 10, 2016 at 10:52:54PM +0100, Daniel Vetter wrote:
With the last round of changes all ioctls called by modern drivers now have their own locking. Everything else is only allowed for legacy drivers and hence the lack of locking doesn't matter.
One exception is nouveau, due to the DRIVER_KMS_LEGACY_CONTEXT flag. But that only works its magic on the context and bufs ioctls. And drm_bufs.c is protected with dev->struct_mutex, and drm_context.c by the same and dev->ctxlist_mutex. That should be all safe, and we can finally mandata drm-bkl-less ioctls for everyone!
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Meh, I'd much rather if this was just data driven rather than overriding based on its own rules. However, it is a small simple step cleaning up after the core changes, so in that regard
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
I'd just rather see DRM_GLOBAL_LOCK instead. -Chris
On Mon, Dec 12, 2016 at 09:50:29AM +0000, Chris Wilson wrote:
On Sat, Dec 10, 2016 at 10:52:54PM +0100, Daniel Vetter wrote:
With the last round of changes all ioctls called by modern drivers now have their own locking. Everything else is only allowed for legacy drivers and hence the lack of locking doesn't matter.
One exception is nouveau, due to the DRIVER_KMS_LEGACY_CONTEXT flag. But that only works its magic on the context and bufs ioctls. And drm_bufs.c is protected with dev->struct_mutex, and drm_context.c by the same and dev->ctxlist_mutex. That should be all safe, and we can finally mandata drm-bkl-less ioctls for everyone!
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Meh, I'd much rather if this was just data driven rather than overriding based on its own rules. However, it is a small simple step cleaning up after the core changes, so in that regard
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
I'd just rather see DRM_GLOBAL_LOCK instead.
Yeah, same idea, but functional change first, then the large-scale prettification. -Daniel
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@chris-wilson.co.uk Signed-off-by: Daniel Vetter daniel.vetter@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; + 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)) { + 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);
/* 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; card_res->min_width = dev->mode_config.min_width;
- /* CRTCs */ - if (card_res->count_crtcs >= crtc_count) { - copied = 0; - crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr; - drm_for_each_crtc(crtc, dev) { - if (put_user(crtc->base.id, crtc_id + copied)) { - ret = -EFAULT; - goto out; - } - copied++; + count = 0; + crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr; + drm_for_each_crtc(crtc, dev) { + count++; + if (count > card_res->count_crtcs) + continue; + + if (put_user(crtc->base.id, crtc_id + count)) { + ret = -EFAULT; + goto out; } } - card_res->count_crtcs = crtc_count; - - /* Encoders */ - if (card_res->count_encoders >= encoder_count) { - copied = 0; - encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr; - drm_for_each_encoder(encoder, dev) { - if (put_user(encoder->base.id, encoder_id + - copied)) { - ret = -EFAULT; - goto out; - } - copied++; + card_res->count_crtcs = count; + + count = 0; + encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr; + drm_for_each_encoder(encoder, dev) { + count++; + if (count > card_res->count_encoders) + continue; + + if (put_user(encoder->base.id, encoder_id + count)) { + ret = -EFAULT; + goto out; } } - card_res->count_encoders = encoder_count; - - /* Connectors */ - if (card_res->count_connectors >= connector_count) { - copied = 0; - connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr; - drm_for_each_connector(connector, dev) { - if (put_user(connector->base.id, - connector_id + copied)) { - ret = -EFAULT; - goto out; - } - copied++; + card_res->count_encoders = count; + + count = 0; + connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr; + drm_for_each_connector(connector, dev) { + count++; + if (count >= card_res->count_connectors) + continue; + + if (put_user(connector->base.id, connector_id + count)) { + ret = -EFAULT; + goto out; } } - card_res->count_connectors = connector_count; + card_res->count_connectors = count;
out: mutex_unlock(&dev->mode_config.mutex);
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@chris-wilson.co.uk Signed-off-by: Daniel Vetter daniel.vetter@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);
On Sat, Dec 10, 2016 at 11:04 PM, Chris Wilson chris@chris-wilson.co.uk wrote:
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++;
Note I also have > instead of <, so I think it should be equivalent. Oops except for the connector lop, silly me that lost one. -Daniel
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)
v4: - Use u64_to_user_ptr (Chris) - Don't forget to copy the last connector (Chris)
Cc: Chris Wilson chris@chris-wilson.co.uk Signed-off-by: Daniel Vetter daniel.vetter@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..3e6952142974 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 = u64_to_user_ptr(card_res->fb_id_ptr); + 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)) { + 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);
/* 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; card_res->min_width = dev->mode_config.min_width;
- /* CRTCs */ - if (card_res->count_crtcs >= crtc_count) { - copied = 0; - crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr; - drm_for_each_crtc(crtc, dev) { - if (put_user(crtc->base.id, crtc_id + copied)) { - ret = -EFAULT; - goto out; - } - copied++; + count = 0; + crtc_id = u64_to_user_ptr(card_res->crtc_id_ptr); + drm_for_each_crtc(crtc, dev) { + count++; + if (count > card_res->count_crtcs) + continue; + + if (put_user(crtc->base.id, crtc_id + count)) { + ret = -EFAULT; + goto out; } } - card_res->count_crtcs = crtc_count; - - /* Encoders */ - if (card_res->count_encoders >= encoder_count) { - copied = 0; - encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr; - drm_for_each_encoder(encoder, dev) { - if (put_user(encoder->base.id, encoder_id + - copied)) { - ret = -EFAULT; - goto out; - } - copied++; + card_res->count_crtcs = count; + + count = 0; + encoder_id = u64_to_user_ptr(card_res->encoder_id_ptr); + drm_for_each_encoder(encoder, dev) { + count++; + if (count > card_res->count_encoders) + continue; + + if (put_user(encoder->base.id, encoder_id + count)) { + ret = -EFAULT; + goto out; } } - card_res->count_encoders = encoder_count; - - /* Connectors */ - if (card_res->count_connectors >= connector_count) { - copied = 0; - connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr; - drm_for_each_connector(connector, dev) { - if (put_user(connector->base.id, - connector_id + copied)) { - ret = -EFAULT; - goto out; - } - copied++; + card_res->count_encoders = count; + + count = 0; + connector_id = u64_to_user_ptr(card_res->connector_id_ptr); + drm_for_each_connector(connector, dev) { + count++; + if (count > card_res->count_connectors) + continue; + + if (put_user(connector->base.id, connector_id + count)) { + ret = -EFAULT; + goto out; } } - card_res->count_connectors = connector_count; + card_res->count_connectors = count;
out: mutex_unlock(&dev->mode_config.mutex);
On Sun, Dec 11, 2016 at 01:39:15PM +0100, Daniel Vetter wrote:
- count = 0;
- fb_id = u64_to_user_ptr(card_res->fb_id_ptr);
- 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)) {
On the first iteration what is the value of count here? What should it be? -Chris
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)
v4: - Use u64_to_user_ptr (Chris) - Don't forget to copy the last connector (Chris)
v5: Chris was right ...
Cc: Chris Wilson chris@chris-wilson.co.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_mode_config.c | 111 ++++++++++++++------------------------ 1 file changed, 39 insertions(+), 72 deletions(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 2735a5847ffa..b1e8bbceaf39 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,62 @@ 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 = u64_to_user_ptr(card_res->fb_id_ptr); + list_for_each_entry(fb, &file_priv->fbs, filp_head) { + if (count < card_res->count_fbs && + put_user(fb->base.id, fb_id + count)) { + mutex_unlock(&file_priv->fbs_lock); + return -EFAULT; } + count++; } - card_res->count_fbs = fb_count; + card_res->count_fbs = count; mutex_unlock(&file_priv->fbs_lock);
/* 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; card_res->min_width = dev->mode_config.min_width;
- /* CRTCs */ - if (card_res->count_crtcs >= crtc_count) { - copied = 0; - crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr; - drm_for_each_crtc(crtc, dev) { - if (put_user(crtc->base.id, crtc_id + copied)) { - ret = -EFAULT; - goto out; - } - copied++; + count = 0; + crtc_id = u64_to_user_ptr(card_res->crtc_id_ptr); + drm_for_each_crtc(crtc, dev) { + if (count < card_res->count_crtcs && + put_user(crtc->base.id, crtc_id + count)) { + ret = -EFAULT; + goto out; } + count++; } - card_res->count_crtcs = crtc_count; - - /* Encoders */ - if (card_res->count_encoders >= encoder_count) { - copied = 0; - encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr; - drm_for_each_encoder(encoder, dev) { - if (put_user(encoder->base.id, encoder_id + - copied)) { - ret = -EFAULT; - goto out; - } - copied++; + card_res->count_crtcs = count; + + count = 0; + encoder_id = u64_to_user_ptr(card_res->encoder_id_ptr); + drm_for_each_encoder(encoder, dev) { + if (count < card_res->count_encoders && + put_user(encoder->base.id, encoder_id + count)) { + ret = -EFAULT; + goto out; } + count++; } - card_res->count_encoders = encoder_count; - - /* Connectors */ - if (card_res->count_connectors >= connector_count) { - copied = 0; - connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr; - drm_for_each_connector(connector, dev) { - if (put_user(connector->base.id, - connector_id + copied)) { - ret = -EFAULT; - goto out; - } - copied++; + card_res->count_encoders = count; + + count = 0; + connector_id = u64_to_user_ptr(card_res->connector_id_ptr); + drm_for_each_connector(connector, dev) { + if (count < card_res->count_connectors && + put_user(connector->base.id, connector_id + count)) { + ret = -EFAULT; + goto out; } + count++; } - card_res->count_connectors = connector_count; + card_res->count_connectors = count;
out: mutex_unlock(&dev->mode_config.mutex);
On Sun, Dec 11, 2016 at 08:20:19PM +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)
v4:
- Use u64_to_user_ptr (Chris)
- Don't forget to copy the last connector (Chris)
v5: Chris was right ...
Cc: Chris Wilson chris@chris-wilson.co.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_mode_config.c | 111 ++++++++++++++------------------------ 1 file changed, 39 insertions(+), 72 deletions(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 2735a5847ffa..b1e8bbceaf39 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;
I'm down to a minor int but uABI uses u32. This being C, it all comes out in the wash. One day we may have -Wsign-compare, but not today!
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
On Sun, Dec 11, 2016 at 07:53:42PM +0000, Chris Wilson wrote:
On Sun, Dec 11, 2016 at 08:20:19PM +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)
v4:
- Use u64_to_user_ptr (Chris)
- Don't forget to copy the last connector (Chris)
v5: Chris was right ...
And CI finally concurred that v5 works ;-)
Cc: Chris Wilson chris@chris-wilson.co.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_mode_config.c | 111 ++++++++++++++------------------------ 1 file changed, 39 insertions(+), 72 deletions(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 2735a5847ffa..b1e8bbceaf39 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;
I'm down to a minor int but uABI uses u32. This being C, it all comes out in the wash. One day we may have -Wsign-compare, but not today!
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
Thanks for the review, applied to -misc. Can I volunteer you for 1-3 too please?
Thanks, Daniel
On Sat, Dec 10, 2016 at 10:52:52PM +0100, Daniel Vetter wrote:
No one looks at the major/minor versions except the unique/busid stuff. If we protect that with the master_mutex (since it also affects the unique of each master, oh well) we can mark these two IOCTL with DRM_UNLOCKED.
While doing this I realized that the comment for the magic_map is outdated, I've forgotten to update it in:
commit d2b34ee62b409a03c6fe43c07b779983be51d017 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Fri Jun 17 09:33:21 2016 +0200
drm: Protect authmagic with master_mutexCc: Chris Wilson chris@chris-wilson.co.uk Cc: Emil Velikov emil.l.velikov@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Ok, you have to jump around a bit to confirm the locking is correct.
(Mainly didn't get the connection between getunique and set_busid). Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
On Mon, Dec 12, 2016 at 09:39:55AM +0000, Chris Wilson wrote:
On Sat, Dec 10, 2016 at 10:52:52PM +0100, Daniel Vetter wrote:
No one looks at the major/minor versions except the unique/busid stuff. If we protect that with the master_mutex (since it also affects the unique of each master, oh well) we can mark these two IOCTL with DRM_UNLOCKED.
While doing this I realized that the comment for the magic_map is outdated, I've forgotten to update it in:
commit d2b34ee62b409a03c6fe43c07b779983be51d017 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Fri Jun 17 09:33:21 2016 +0200
drm: Protect authmagic with master_mutexCc: Chris Wilson chris@chris-wilson.co.uk Cc: Emil Velikov emil.l.velikov@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Ok, you have to jump around a bit to confirm the locking is correct.
(Mainly didn't get the connection between getunique and set_busid).
Yeah the naming is hilarious in this area. There's a kerneldoc comment that tries to explain it all.
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
Thanks for the review, applied the small update for patch 2 and merged them all. -Daniel
On 10 December 2016 at 21:52, Daniel Vetter daniel.vetter@ffwll.ch wrote:
No one looks at the major/minor versions except the unique/busid stuff. If we protect that with the master_mutex (since it also affects the unique of each master, oh well) we can mark these two IOCTL with DRM_UNLOCKED.
While doing this I realized that the comment for the magic_map is outdated, I've forgotten to update it in:
commit d2b34ee62b409a03c6fe43c07b779983be51d017 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Fri Jun 17 09:33:21 2016 +0200
drm: Protect authmagic with master_mutexCc: Chris Wilson chris@chris-wilson.co.uk Cc: Emil Velikov emil.l.velikov@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
Side note: I've looked at "fixing" xf86-video-amdgpu and so far I'm at ~25 patches. Admittedly it includes some related fixes/cleanups. At some point we want to address the other KMS ddx - armsoc (ahem), ati, freedreno, intel, nouveau, omap, opentegra, vmwgfx, qxl and of course modesetting ;-) Regardless if some are superseded and/or barely used.
-Emil
On Mon, Dec 12, 2016 at 01:23:46PM +0000, Emil Velikov wrote:
On 10 December 2016 at 21:52, Daniel Vetter daniel.vetter@ffwll.ch wrote:
No one looks at the major/minor versions except the unique/busid stuff. If we protect that with the master_mutex (since it also affects the unique of each master, oh well) we can mark these two IOCTL with DRM_UNLOCKED.
While doing this I realized that the comment for the magic_map is outdated, I've forgotten to update it in:
commit d2b34ee62b409a03c6fe43c07b779983be51d017 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Fri Jun 17 09:33:21 2016 +0200
drm: Protect authmagic with master_mutexCc: Chris Wilson chris@chris-wilson.co.uk Cc: Emil Velikov emil.l.velikov@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
Side note: I've looked at "fixing" xf86-video-amdgpu and so far I'm at ~25 patches. Admittedly it includes some related fixes/cleanups. At some point we want to address the other KMS ddx - armsoc (ahem), ati, freedreno, intel, nouveau, omap, opentegra, vmwgfx, qxl and of course modesetting ;-) Regardless if some are superseded and/or barely used.
Hm, I thought the grand plan is to use -modesetting almost everywhere and forget about all the others? -Daniel
On 13/12/16 05:35 PM, Daniel Vetter wrote:
On Mon, Dec 12, 2016 at 01:23:46PM +0000, Emil Velikov wrote:
On 10 December 2016 at 21:52, Daniel Vetter daniel.vetter@ffwll.ch wrote:
No one looks at the major/minor versions except the unique/busid stuff. If we protect that with the master_mutex (since it also affects the unique of each master, oh well) we can mark these two IOCTL with DRM_UNLOCKED.
While doing this I realized that the comment for the magic_map is outdated, I've forgotten to update it in:
commit d2b34ee62b409a03c6fe43c07b779983be51d017 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Fri Jun 17 09:33:21 2016 +0200
drm: Protect authmagic with master_mutexCc: Chris Wilson chris@chris-wilson.co.uk Cc: Emil Velikov emil.l.velikov@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
Side note: I've looked at "fixing" xf86-video-amdgpu and so far I'm at ~25 patches. Admittedly it includes some related fixes/cleanups. At some point we want to address the other KMS ddx - armsoc (ahem), ati, freedreno, intel, nouveau, omap, opentegra, vmwgfx, qxl and of course modesetting ;-) Regardless if some are superseded and/or barely used.
Hm, I thought the grand plan is to use -modesetting almost everywhere and forget about all the others?
Maybe if you mean s/grand plan/pipe dream/ ...
Specifically, wrt to DDX drivers for ATI/AMD GPUs:
xf86-video-ati supports old GPUs on which glamor can't work, so -modesetting can never be a complete replacement.
xf86-video-amdgpu is also used by the amdgpu-pro stack, with modifications which are probably unsuitable for -modesetting. There's also still a feature gap to -modesetting, e.g. the latter doesn't support TearFree yet, and there doesn't seem to be any interest to fix that.
So I'm afraid we can't get rid of those anytime soon, if ever.
On Tue, Dec 13, 2016 at 10:42 AM, Michel Dänzer michel@daenzer.net wrote:
Hm, I thought the grand plan is to use -modesetting almost everywhere and forget about all the others?
Maybe if you mean s/grand plan/pipe dream/ ...
I said "almost everywhere", not "everywhere". I'm fully aware that there's tons of old chips that need dedicated drivers, and that amd wants to do some fancy stuff in their -pro version. But besides those I do see a fairly clear push towards having standardized/generic kms userspace, so don't really see that much value in updating all the vendor specific drivers. And outside of X the push is even stronger.
So no pipe dream at all, it's happening. -Daniel
On 13 December 2016 at 09:46, Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Dec 13, 2016 at 10:42 AM, Michel Dänzer michel@daenzer.net wrote:
Hm, I thought the grand plan is to use -modesetting almost everywhere and forget about all the others?
Maybe if you mean s/grand plan/pipe dream/ ...
I said "almost everywhere", not "everywhere". I'm fully aware that there's tons of old chips that need dedicated drivers, and that amd wants to do some fancy stuff in their -pro version. But besides those I do see a fairly clear push towards having standardized/generic kms userspace, so don't really see that much value in updating all the vendor specific drivers. And outside of X the push is even stronger.
So no pipe dream at all, it's happening.
Precisely.
As Michel pointed out: There will [always] be compelling reasons why users/distros prefer xf86-video-foo over the modesetting one. So, barring time concerns, updating those is good idea, imho.
Pretty we don't want another DRIVER_KMS_LEGACY_CONTEXT, because user X decided to stay with the final version of xf86-video-foo which still uses nasty libdrm/ioctl Y ;-)
Thanks Emil
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Daniel Vetter -
Emil Velikov -
Michel Dänzer