| 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: <20140710000905.GA18025@kroah.com>
Date: Wed, 9 Jul 2014 17:09:05 -0700
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Sudeep Holla <sudeep.holla@....com>
Cc: linux-kernel@...r.kernel.org,
Heiko Carstens <heiko.carstens@...ibm.com>,
Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
Rob Herring <robh@...nel.org>, linux-doc@...r.kernel.org,
linux-ia64@...r.kernel.org, linux390@...ibm.com,
linux-s390@...r.kernel.org, x86@...nel.org,
linuxppc-dev@...ts.ozlabs.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/9] drivers: base: support cpu cache information
interface to userspace via sysfs
On Wed, Jun 25, 2014 at 06:30:37PM +0100, Sudeep Holla wrote:
> +static const struct device_attribute *cache_optional_attrs[] = {
> + &dev_attr_coherency_line_size,
> + &dev_attr_ways_of_associativity,
> + &dev_attr_number_of_sets,
> + &dev_attr_size,
> + &dev_attr_attributes,
> + &dev_attr_physical_line_partition,
> + NULL
> +};
> +
> +static int device_add_attrs(struct device *dev,
> + const struct device_attribute **dev_attrs)
> +{
> + int i, error = 0;
> + struct device_attribute *dev_attr;
> + char *buf;
> +
> + if (!dev_attrs)
> + return 0;
> +
> + buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + for (i = 0; dev_attrs[i]; i++) {
> + dev_attr = (struct device_attribute *)dev_attrs[i];
> +
> + /* create attributes that provides meaningful value */
> + if (dev_attr->show(dev, dev_attr, buf) < 0)
> + continue;
> +
> + error = device_create_file(dev, dev_attrs[i]);
> + if (error) {
> + while (--i >= 0)
> + device_remove_file(dev, dev_attrs[i]);
> + break;
> + }
> + }
> +
> + kfree(buf);
> + return error;
> +}
Ick, why create your own function for this when the driver core has this
functionality built into it? Look at the is_visible() callback, and how
it is use for an attribute group please.
> +static void device_remove_attrs(struct device *dev,
> + const struct device_attribute **dev_attrs)
> +{
> + int i;
> +
> + if (!dev_attrs)
> + return;
> +
> + for (i = 0; dev_attrs[i]; dev_attrs++, i++)
> + device_remove_file(dev, dev_attrs[i]);
> +}
You should just remove a whole group at once, not individually.
> +
> +const struct device_attribute **
> +__weak cache_get_priv_attr(struct device *cache_idx_dev)
> +{
> + return NULL;
> +}
> +
> +/* Add/Remove cache interface for CPU device */
> +static void cpu_cache_sysfs_exit(unsigned int cpu)
> +{
> + int i;
> + struct device *tmp_dev;
> + const struct device_attribute **ci_priv_attr;
> +
> + if (per_cpu_index_dev(cpu)) {
> + for (i = 0; i < cache_leaves(cpu); i++) {
> + tmp_dev = per_cache_index_dev(cpu, i);
> + if (!tmp_dev)
> + continue;
> + ci_priv_attr = cache_get_priv_attr(tmp_dev);
> + device_remove_attrs(tmp_dev, ci_priv_attr);
> + device_remove_attrs(tmp_dev, cache_optional_attrs);
> + device_unregister(tmp_dev);
> + }
> + kfree(per_cpu_index_dev(cpu));
> + per_cpu_index_dev(cpu) = NULL;
> + }
> + device_unregister(per_cpu_cache_dev(cpu));
> + per_cpu_cache_dev(cpu) = NULL;
> +}
> +
> +static int cpu_cache_sysfs_init(unsigned int cpu)
> +{
> + struct device *dev = get_cpu_device(cpu);
> +
> + if (per_cpu_cacheinfo(cpu) == NULL)
> + return -ENOENT;
> +
> + per_cpu_cache_dev(cpu) = device_create(dev->class, dev, cpu,
> + NULL, "cache");
> + if (IS_ERR_OR_NULL(per_cpu_cache_dev(cpu)))
> + return PTR_ERR(per_cpu_cache_dev(cpu));
> +
> + /* Allocate all required memory */
> + per_cpu_index_dev(cpu) = kzalloc(sizeof(struct device *) *
> + cache_leaves(cpu), GFP_KERNEL);
> + if (unlikely(per_cpu_index_dev(cpu) == NULL))
> + goto err_out;
> +
> + return 0;
> +
> +err_out:
> + cpu_cache_sysfs_exit(cpu);
> + return -ENOMEM;
> +}
> +
> +static int cache_add_dev(unsigned int cpu)
> +{
> + unsigned short i;
> + int rc;
> + struct device *tmp_dev, *parent;
> + struct cacheinfo *this_leaf;
> + const struct device_attribute **ci_priv_attr;
> + struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> +
> + rc = cpu_cache_sysfs_init(cpu);
> + if (unlikely(rc < 0))
> + return rc;
> +
> + parent = per_cpu_cache_dev(cpu);
> + for (i = 0; i < cache_leaves(cpu); i++) {
> + this_leaf = this_cpu_ci->info_list + i;
> + if (this_leaf->disable_sysfs)
> + continue;
> + tmp_dev = device_create_with_groups(parent->class, parent, i,
> + this_leaf,
> + cache_default_groups,
> + "index%1u", i);
> + if (IS_ERR_OR_NULL(tmp_dev)) {
> + rc = PTR_ERR(tmp_dev);
> + goto err;
> + }
> +
> + rc = device_add_attrs(tmp_dev, cache_optional_attrs);
> + if (unlikely(rc))
> + goto err;
> +
> + ci_priv_attr = cache_get_priv_attr(tmp_dev);
> + rc = device_add_attrs(tmp_dev, ci_priv_attr);
> + if (unlikely(rc))
> + goto err;
You just raced with userspace here, creating these files _after_ the
device was announced to userspace, causing problems with anyone wanting
to read these attributes :(
I think if you fix up the is_visible() thing above, these calls will go
away, right?
--
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