| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20130311230330.GC3872@bremse>
Date: Tue, 12 Mar 2013 00:03:30 +0100
From: Daniel Vetter <daniel@...ll.ch>
To: Kees Cook <keescook@...omium.org>
Cc: linux-kernel@...r.kernel.org,
Daniel Vetter <daniel.vetter@...ll.ch>,
David Airlie <airlied@...ux.ie>,
dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm/i915: use simple attribute in debugfs routines
On Sun, Mar 10, 2013 at 02:10:06PM -0700, Kees Cook wrote:
> This replaces the manual read/write routines in debugfs with the common
> simple attribute helpers. Doing this gets rid of repeated copy/pasting
> of copy_from_user and value formatting code.
>
> Signed-off-by: Kees Cook <keescook@...omium.org>
> Cc: Daniel Vetter <daniel.vetter@...ll.ch>
Queued for -next, thanks for the patch. When sending drm/i915 patches,
please also cc intel-gfx@...ts.freedesktop.org (it's open for
non-subscribers non without nagging "you're mail is in the moderation
queue" replies).
Thanks, Daniel
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 394 +++++++++--------------------------
> 1 file changed, 98 insertions(+), 296 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index aae3148..d86c304 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -849,76 +849,42 @@ static const struct file_operations i915_error_state_fops = {
> .release = i915_error_state_release,
> };
>
> -static ssize_t
> -i915_next_seqno_read(struct file *filp,
> - char __user *ubuf,
> - size_t max,
> - loff_t *ppos)
> +static int
> +i915_next_seqno_get(void *data, u64 *val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> drm_i915_private_t *dev_priv = dev->dev_private;
> - char buf[80];
> - int len;
> int ret;
>
> ret = mutex_lock_interruptible(&dev->struct_mutex);
> if (ret)
> return ret;
>
> - len = snprintf(buf, sizeof(buf),
> - "next_seqno : 0x%x\n",
> - dev_priv->next_seqno);
> -
> + *val = dev_priv->next_seqno;
> mutex_unlock(&dev->struct_mutex);
>
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> -
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_next_seqno_write(struct file *filp,
> - const char __user *ubuf,
> - size_t cnt,
> - loff_t *ppos)
> -{
> - struct drm_device *dev = filp->private_data;
> - char buf[20];
> - u32 val = 1;
> +static int
> +i915_next_seqno_set(void *data, u64 val)
> +{
> + struct drm_device *dev = data;
> int ret;
>
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> -
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - ret = kstrtouint(buf, 0, &val);
> - if (ret < 0)
> - return ret;
> - }
> -
> ret = mutex_lock_interruptible(&dev->struct_mutex);
> if (ret)
> return ret;
>
> ret = i915_gem_set_seqno(dev, val);
> -
> mutex_unlock(&dev->struct_mutex);
>
> - return ret ?: cnt;
> + return ret;
> }
>
> -static const struct file_operations i915_next_seqno_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_next_seqno_read,
> - .write = i915_next_seqno_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_next_seqno_fops,
> + i915_next_seqno_get, i915_next_seqno_set,
> + "next_seqno : 0x%llx\n");
>
> static int i915_rstdby_delays(struct seq_file *m, void *unused)
> {
> @@ -1680,105 +1646,51 @@ static int i915_dpio_info(struct seq_file *m, void *data)
> return 0;
> }
>
> -static ssize_t
> -i915_wedged_read(struct file *filp,
> - char __user *ubuf,
> - size_t max,
> - loff_t *ppos)
> +static int
> +i915_wedged_get(void *data, u64 *val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> drm_i915_private_t *dev_priv = dev->dev_private;
> - char buf[80];
> - int len;
>
> - len = snprintf(buf, sizeof(buf),
> - "wedged : %d\n",
> - atomic_read(&dev_priv->gpu_error.reset_counter));
> + *val = atomic_read(&dev_priv->gpu_error.reset_counter);
>
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> -
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_wedged_write(struct file *filp,
> - const char __user *ubuf,
> - size_t cnt,
> - loff_t *ppos)
> +static int
> +i915_wedged_set(void *data, u64 val)
> {
> - struct drm_device *dev = filp->private_data;
> - char buf[20];
> - int val = 1;
> -
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> + struct drm_device *dev = data;
>
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - val = simple_strtoul(buf, NULL, 0);
> - }
> -
> - DRM_INFO("Manually setting wedged to %d\n", val);
> + DRM_INFO("Manually setting wedged to %llu\n", val);
> i915_handle_error(dev, val);
>
> - return cnt;
> + return 0;
> }
>
> -static const struct file_operations i915_wedged_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_wedged_read,
> - .write = i915_wedged_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_wedged_fops,
> + i915_wedged_get, i915_wedged_set,
> + "wedged : %llu\n");
>
> -static ssize_t
> -i915_ring_stop_read(struct file *filp,
> - char __user *ubuf,
> - size_t max,
> - loff_t *ppos)
> +static int
> +i915_ring_stop_get(void *data, u64 *val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> drm_i915_private_t *dev_priv = dev->dev_private;
> - char buf[20];
> - int len;
> -
> - len = snprintf(buf, sizeof(buf),
> - "0x%08x\n", dev_priv->gpu_error.stop_rings);
>
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> + *val = dev_priv->gpu_error.stop_rings;
>
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_ring_stop_write(struct file *filp,
> - const char __user *ubuf,
> - size_t cnt,
> - loff_t *ppos)
> +static int
> +i915_ring_stop_set(void *data, u64 val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - char buf[20];
> - int val = 0, ret;
> -
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> -
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - val = simple_strtoul(buf, NULL, 0);
> - }
> + int ret;
>
> - DRM_DEBUG_DRIVER("Stopping rings 0x%08x\n", val);
> + DRM_DEBUG_DRIVER("Stopping rings 0x%08llx\n", val);
>
> ret = mutex_lock_interruptible(&dev->struct_mutex);
> if (ret)
> @@ -1787,16 +1699,12 @@ i915_ring_stop_write(struct file *filp,
> dev_priv->gpu_error.stop_rings = val;
> mutex_unlock(&dev->struct_mutex);
>
> - return cnt;
> + return 0;
> }
>
> -static const struct file_operations i915_ring_stop_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_ring_stop_read,
> - .write = i915_ring_stop_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_ring_stop_fops,
> + i915_ring_stop_get, i915_ring_stop_set,
> + "0x%08llx\n");
>
> #define DROP_UNBOUND 0x1
> #define DROP_BOUND 0x2
> @@ -1806,46 +1714,23 @@ static const struct file_operations i915_ring_stop_fops = {
> DROP_BOUND | \
> DROP_RETIRE | \
> DROP_ACTIVE)
> -static ssize_t
> -i915_drop_caches_read(struct file *filp,
> - char __user *ubuf,
> - size_t max,
> - loff_t *ppos)
> +static int
> +i915_drop_caches_get(void *data, u64 *val)
> {
> - char buf[20];
> - int len;
> -
> - len = snprintf(buf, sizeof(buf), "0x%08x\n", DROP_ALL);
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> + *val = DROP_ALL;
>
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_drop_caches_write(struct file *filp,
> - const char __user *ubuf,
> - size_t cnt,
> - loff_t *ppos)
> +static int
> +i915_drop_caches_set(void *data, u64 val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_i915_gem_object *obj, *next;
> - char buf[20];
> - int val = 0, ret;
> -
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> -
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - val = simple_strtoul(buf, NULL, 0);
> - }
> + int ret;
>
> - DRM_DEBUG_DRIVER("Dropping caches: 0x%08x\n", val);
> + DRM_DEBUG_DRIVER("Dropping caches: 0x%08llx\n", val);
>
> /* No need to check and wait for gpu resets, only libdrm auto-restarts
> * on ioctls on -EAGAIN. */
> @@ -1883,27 +1768,19 @@ i915_drop_caches_write(struct file *filp,
> unlock:
> mutex_unlock(&dev->struct_mutex);
>
> - return ret ?: cnt;
> + return ret;
> }
>
> -static const struct file_operations i915_drop_caches_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_drop_caches_read,
> - .write = i915_drop_caches_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_drop_caches_fops,
> + i915_drop_caches_get, i915_drop_caches_set,
> + "0x%08llx\n");
>
> -static ssize_t
> -i915_max_freq_read(struct file *filp,
> - char __user *ubuf,
> - size_t max,
> - loff_t *ppos)
> +static int
> +i915_max_freq_get(void *data, u64 *val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> drm_i915_private_t *dev_priv = dev->dev_private;
> - char buf[80];
> - int len, ret;
> + int ret;
>
> if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> return -ENODEV;
> @@ -1912,42 +1789,23 @@ i915_max_freq_read(struct file *filp,
> if (ret)
> return ret;
>
> - len = snprintf(buf, sizeof(buf),
> - "max freq: %d\n", dev_priv->rps.max_delay * GT_FREQUENCY_MULTIPLIER);
> + *val = dev_priv->rps.max_delay * GT_FREQUENCY_MULTIPLIER;
> mutex_unlock(&dev_priv->rps.hw_lock);
>
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> -
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_max_freq_write(struct file *filp,
> - const char __user *ubuf,
> - size_t cnt,
> - loff_t *ppos)
> +static int
> +i915_max_freq_set(void *data, u64 val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - char buf[20];
> - int val = 1, ret;
> + int ret;
>
> if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> return -ENODEV;
>
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> -
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - val = simple_strtoul(buf, NULL, 0);
> - }
> -
> - DRM_DEBUG_DRIVER("Manually setting max freq to %d\n", val);
> + DRM_DEBUG_DRIVER("Manually setting max freq to %llu\n", val);
>
> ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
> if (ret)
> @@ -1961,25 +1819,19 @@ i915_max_freq_write(struct file *filp,
> gen6_set_rps(dev, val / GT_FREQUENCY_MULTIPLIER);
> mutex_unlock(&dev_priv->rps.hw_lock);
>
> - return cnt;
> + return 0;
> }
>
> -static const struct file_operations i915_max_freq_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_max_freq_read,
> - .write = i915_max_freq_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_max_freq_fops,
> + i915_max_freq_get, i915_max_freq_set,
> + "max freq: %llu\n");
>
> -static ssize_t
> -i915_min_freq_read(struct file *filp, char __user *ubuf, size_t max,
> - loff_t *ppos)
> +static int
> +i915_min_freq_get(void *data, u64 *val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> drm_i915_private_t *dev_priv = dev->dev_private;
> - char buf[80];
> - int len, ret;
> + int ret;
>
> if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> return -ENODEV;
> @@ -1988,40 +1840,23 @@ i915_min_freq_read(struct file *filp, char __user *ubuf, size_t max,
> if (ret)
> return ret;
>
> - len = snprintf(buf, sizeof(buf),
> - "min freq: %d\n", dev_priv->rps.min_delay * GT_FREQUENCY_MULTIPLIER);
> + *val = dev_priv->rps.min_delay * GT_FREQUENCY_MULTIPLIER;
> mutex_unlock(&dev_priv->rps.hw_lock);
>
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> -
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt,
> - loff_t *ppos)
> +static int
> +i915_min_freq_set(void *data, u64 val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - char buf[20];
> - int val = 1, ret;
> + int ret;
>
> if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> return -ENODEV;
>
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> -
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - val = simple_strtoul(buf, NULL, 0);
> - }
> -
> - DRM_DEBUG_DRIVER("Manually setting min freq to %d\n", val);
> + DRM_DEBUG_DRIVER("Manually setting min freq to %llu\n", val);
>
> ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
> if (ret)
> @@ -2035,28 +1870,20 @@ i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt,
> gen6_set_rps(dev, val / GT_FREQUENCY_MULTIPLIER);
> mutex_unlock(&dev_priv->rps.hw_lock);
>
> - return cnt;
> + return 0;
> }
>
> -static const struct file_operations i915_min_freq_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_min_freq_read,
> - .write = i915_min_freq_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_min_freq_fops,
> + i915_min_freq_get, i915_min_freq_set,
> + "min freq: %llu\n");
>
> -static ssize_t
> -i915_cache_sharing_read(struct file *filp,
> - char __user *ubuf,
> - size_t max,
> - loff_t *ppos)
> +static int
> +i915_cache_sharing_get(void *data, u64 *val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> drm_i915_private_t *dev_priv = dev->dev_private;
> - char buf[80];
> u32 snpcr;
> - int len, ret;
> + int ret;
>
> if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> return -ENODEV;
> @@ -2068,46 +1895,25 @@ i915_cache_sharing_read(struct file *filp,
> snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
> mutex_unlock(&dev_priv->dev->struct_mutex);
>
> - len = snprintf(buf, sizeof(buf),
> - "%d\n", (snpcr & GEN6_MBC_SNPCR_MASK) >>
> - GEN6_MBC_SNPCR_SHIFT);
> -
> - if (len > sizeof(buf))
> - len = sizeof(buf);
> + *val = (snpcr & GEN6_MBC_SNPCR_MASK) >> GEN6_MBC_SNPCR_SHIFT;
>
> - return simple_read_from_buffer(ubuf, max, ppos, buf, len);
> + return 0;
> }
>
> -static ssize_t
> -i915_cache_sharing_write(struct file *filp,
> - const char __user *ubuf,
> - size_t cnt,
> - loff_t *ppos)
> +static int
> +i915_cache_sharing_set(void *data, u64 val)
> {
> - struct drm_device *dev = filp->private_data;
> + struct drm_device *dev = data;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - char buf[20];
> u32 snpcr;
> - int val = 1;
>
> if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> return -ENODEV;
>
> - if (cnt > 0) {
> - if (cnt > sizeof(buf) - 1)
> - return -EINVAL;
> -
> - if (copy_from_user(buf, ubuf, cnt))
> - return -EFAULT;
> - buf[cnt] = 0;
> -
> - val = simple_strtoul(buf, NULL, 0);
> - }
> -
> - if (val < 0 || val > 3)
> + if (val > 3)
> return -EINVAL;
>
> - DRM_DEBUG_DRIVER("Manually setting uncore sharing to %d\n", val);
> + DRM_DEBUG_DRIVER("Manually setting uncore sharing to %llu\n", val);
>
> /* Update the cache sharing policy here as well */
> snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
> @@ -2115,16 +1921,12 @@ i915_cache_sharing_write(struct file *filp,
> snpcr |= (val << GEN6_MBC_SNPCR_SHIFT);
> I915_WRITE(GEN6_MBCUNIT_SNPCR, snpcr);
>
> - return cnt;
> + return 0;
> }
>
> -static const struct file_operations i915_cache_sharing_fops = {
> - .owner = THIS_MODULE,
> - .open = simple_open,
> - .read = i915_cache_sharing_read,
> - .write = i915_cache_sharing_write,
> - .llseek = default_llseek,
> -};
> +DEFINE_SIMPLE_ATTRIBUTE(i915_cache_sharing_fops,
> + i915_cache_sharing_get, i915_cache_sharing_set,
> + "%llu\n");
>
> /* As the drm_debugfs_init() routines are called before dev->dev_private is
> * allocated we need to hook into the minor for release. */
> --
> 1.7.9.5
>
>
> --
> Kees Cook
> Chrome OS Security
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists