| 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
| ||
|
Date: Sun, 31 Aug 2008 13:28:08 -0700
From: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To: Rajiv Andrade <srajiv@...ux.vnet.ibm.com>
Cc: linux-kernel@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>,
James Morris <jmoriss@...ei.org>,
Peter Dolding <oiaohm@...il.com>,
Kenneth Goldman <kgoldman@...ibm.com>,
Debora Velarde <debora@...ux.vnet.ibm.com>,
David Safford <safford@...son.ibm.com>,
Serge Hallyn <serue@...ux.vnet.ibm.com>,
Mimi Zohar <zohar@...ux.vnet.ibm.com>,
Pavel Machek <pavel@...e.cz>, Greg KH <greg@...ah.com>,
Christoph Hellwig <hch@...radead.org>,
Alan Cox <alan@...rguk.ukuu.org.uk>
Subject: Re: [PATCH 2/2] TPM: rcu locking
On Sat, Aug 30, 2008 at 12:05:04AM -0300, Rajiv Andrade wrote:
> Continue to protect the tpm_chip_list using the driver_lock
> as before, and add an rcu to protect readers.
A few questions interspersed below...
Thanx, Paul
> Signed-off-by: Mimi Zohar <zohar@...ux.vnet.ibm.com>
> Acked-by: Rajiv Andrade <srajiv@...ux.vnet.ibm.com>
> ---
> drivers/char/tpm/tpm.c | 268 ++++++++++++++++++++++++++++++++----------------
> 1 files changed, 178 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index 59b31e1..704ab01 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -587,14 +587,22 @@ ssize_t tpm_show_enabled(struct device * dev, struct device_attribute * attr,
> {
> u8 *data;
> ssize_t rc;
> + struct tpm_chip *chip;
> +
The following five lines of code appear repeatedly. Why not put them
into an inline function or a macro?
Also, doesn't there need to be an rcu_dereference() somewhere either in
this sequence of code or in either dev_get_drvdata() or get_device()?
Or am I missing something subtle here?
>From what I see of the code, I actually don't understand why the
rcu_read_lock() is needed here -- only tpm_chip_list is covered by
the RCU list addition/deletion primitives in this patch. If you are
adding a multilinked data structure into an RCU-protected list, you
need RCU traversal primitives only when traversing the list, not the
data structure contained in the list. (Of course, any changes to the
data structure contained in the list must be protected somehow or other,
unless there are no such changes.)
> + rcu_read_lock();
> + chip = dev_get_drvdata(dev);
> + if (chip)
> + get_device(chip->dev); /* protect from disappearing */
> + rcu_read_unlock();
>
> - struct tpm_chip *chip = dev_get_drvdata(dev);
> if (chip == NULL)
> return -ENODEV;
>
> data = kzalloc(TPM_INTERNAL_RESULT_SIZE, GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> + if (!data) {
> + rc = -ENOMEM;
> + goto out_alloc;
> + }
>
> memcpy(data, tpm_cap, sizeof(tpm_cap));
> data[TPM_CAP_IDX] = TPM_CAP_FLAG;
> @@ -603,13 +611,15 @@ ssize_t tpm_show_enabled(struct device * dev, struct device_attribute * attr,
> rc = transmit_cmd(chip, data, TPM_INTERNAL_RESULT_SIZE,
> "attemtping to determine the permanent enabled state");
> if (rc) {
> - kfree(data);
> - return 0;
> + rc = 0;
> + goto out;
> }
>
> rc = sprintf(buf, "%d\n", !data[TPM_GET_CAP_PERM_DISABLE_IDX]);
> -
> +out:
> kfree(data);
> +out_alloc:
> + put_device(chip->dev);
> return rc;
> }
> EXPORT_SYMBOL_GPL(tpm_show_enabled);
> @@ -619,14 +629,24 @@ ssize_t tpm_show_active(struct device * dev, struct device_attribute * attr,
> {
> u8 *data;
> ssize_t rc;
> + struct tpm_chip *chip;
>
> - struct tpm_chip *chip = dev_get_drvdata(dev);
> - if (chip == NULL)
> - return -ENODEV;
> + rcu_read_lock();
> + chip = dev_get_drvdata(dev);
> + if (chip)
> + get_device(chip->dev); /* protect from disappearing */
> + rcu_read_unlock();
> +
> + if (chip == NULL) {
> + rc = -ENODEV;
> + goto out_alloc;
> + }
>
> data = kzalloc(TPM_INTERNAL_RESULT_SIZE, GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> + if (!data) {
> + rc = -ENOMEM;
> + goto out;
> + }
>
> memcpy(data, tpm_cap, sizeof(tpm_cap));
> data[TPM_CAP_IDX] = TPM_CAP_FLAG;
> @@ -635,13 +655,15 @@ ssize_t tpm_show_active(struct device * dev, struct device_attribute * attr,
> rc = transmit_cmd(chip, data, TPM_INTERNAL_RESULT_SIZE,
> "attemtping to determine the permanent active state");
> if (rc) {
> - kfree(data);
> - return 0;
> + rc = 0;
> + goto out;
> }
>
> rc = sprintf(buf, "%d\n", !data[TPM_GET_CAP_PERM_INACTIVE_IDX]);
> -
> +out:
> kfree(data);
> +out_alloc:
> + put_device(chip->dev);
> return rc;
> }
> EXPORT_SYMBOL_GPL(tpm_show_active);
> @@ -651,14 +673,22 @@ ssize_t tpm_show_owned(struct device * dev, struct device_attribute * attr,
> {
> u8 *data;
> ssize_t rc;
> + struct tpm_chip *chip;
> +
> + rcu_read_lock();
> + chip = dev_get_drvdata(dev);
> + if (chip)
> + get_device(chip->dev); /* protect from disappearing */
> + rcu_read_unlock();
>
> - struct tpm_chip *chip = dev_get_drvdata(dev);
> if (chip == NULL)
> return -ENODEV;
>
> data = kzalloc(TPM_INTERNAL_RESULT_SIZE, GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> + if (!data) {
> + rc = -ENOMEM;
> + goto out_alloc;
> + }
>
> memcpy(data, tpm_cap, sizeof(tpm_cap));
> data[TPM_CAP_IDX] = TPM_CAP_PROP;
> @@ -667,13 +697,15 @@ ssize_t tpm_show_owned(struct device * dev, struct device_attribute * attr,
> rc = transmit_cmd(chip, data, TPM_INTERNAL_RESULT_SIZE,
> "attempting to determine the owner state");
> if (rc) {
> - kfree(data);
> - return 0;
> + rc = 0;
> + goto out;
> }
>
> rc = sprintf(buf, "%d\n", data[TPM_GET_CAP_RET_BOOL_1_IDX]);
> -
> +out:
> kfree(data);
> +out_alloc:
> + put_device(chip->dev);
> return rc;
> }
> EXPORT_SYMBOL_GPL(tpm_show_owned);
> @@ -683,14 +715,22 @@ ssize_t tpm_show_temp_deactivated(struct device * dev,
> {
> u8 *data;
> ssize_t rc;
> + struct tpm_chip *chip;
> +
> + rcu_read_lock();
> + chip = dev_get_drvdata(dev);
> + if (chip)
> + get_device(chip->dev); /* protect from disappearing */
> + rcu_read_unlock();
>
> - struct tpm_chip *chip = dev_get_drvdata(dev);
> if (chip == NULL)
> return -ENODEV;
>
> data = kzalloc(TPM_INTERNAL_RESULT_SIZE, GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> + if (!data) {
> + rc = -ENOMEM;
> + goto out_alloc;
> + }
>
> memcpy(data, tpm_cap, sizeof(tpm_cap));
> data[TPM_CAP_IDX] = TPM_CAP_FLAG;
> @@ -699,13 +739,15 @@ ssize_t tpm_show_temp_deactivated(struct device * dev,
> rc = transmit_cmd(chip, data, TPM_INTERNAL_RESULT_SIZE,
> "attempting to determine the temporary state");
> if (rc) {
> - kfree(data);
> - return 0;
> + rc = 0;
> + goto out;
> }
>
> rc = sprintf(buf, "%d\n", data[TPM_GET_CAP_TEMP_INACTIVE_IDX]);
> -
> +out:
> kfree(data);
> +out_alloc:
> + put_device(chip->dev);
> return rc;
> }
> EXPORT_SYMBOL_GPL(tpm_show_temp_deactivated);
> @@ -725,14 +767,22 @@ ssize_t tpm_show_pcrs(struct device *dev, struct device_attribute *attr,
> int i, j, num_pcrs;
> __be32 index;
> char *str = buf;
> + struct tpm_chip *chip;
> +
> + rcu_read_lock();
> + chip = dev_get_drvdata(dev);
> + if (chip)
> + get_device(chip->dev); /* protect from disappearing */
> + rcu_read_unlock();
>
> - struct tpm_chip *chip = dev_get_drvdata(dev);
> if (chip == NULL)
> return -ENODEV;
>
> data = kzalloc(TPM_INTERNAL_RESULT_SIZE, GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> + if (!data) {
> + rc = -ENOMEM;
> + goto out_alloc;
> + }
>
> memcpy(data, tpm_cap, sizeof(tpm_cap));
> data[TPM_CAP_IDX] = TPM_CAP_PROP;
> @@ -741,8 +791,8 @@ ssize_t tpm_show_pcrs(struct device *dev, struct device_attribute *attr,
> rc = transmit_cmd(chip, data, TPM_INTERNAL_RESULT_SIZE,
> "attempting to determine the number of PCRS");
> if (rc) {
> - kfree(data);
> - return 0;
> + rc = 0;
> + goto out;
> }
>
> num_pcrs = be32_to_cpu(*((__be32 *) (data + 14)));
> @@ -753,15 +803,18 @@ ssize_t tpm_show_pcrs(struct device *dev, struct device_attribute *attr,
> rc = transmit_cmd(chip, data, TPM_INTERNAL_RESULT_SIZE,
> "attempting to read a PCR");
> if (rc)
> - goto out;
> + break;
> str += sprintf(str, "PCR-%02d: ", i);
> for (j = 0; j < TPM_DIGEST_SIZE; j++)
> str += sprintf(str, "%02X ", *(data + 10 + j));
> str += sprintf(str, "\n");
> }
> + rc = str - buf;
> out:
> kfree(data);
> - return str - buf;
> +out_alloc:
> + put_device(chip->dev);
> + return rc;
> }
> EXPORT_SYMBOL_GPL(tpm_show_pcrs);
>
> @@ -779,21 +832,31 @@ ssize_t tpm_show_pubek(struct device *dev, struct device_attribute *attr,
> ssize_t err;
> int i, rc;
> char *str = buf;
> + struct tpm_chip *chip;
> +
> + rcu_read_lock();
> + chip = dev_get_drvdata(dev);
> + if (chip)
> + get_device(chip->dev); /* protect from disappearing */
> + rcu_read_unlock();
>
> - struct tpm_chip *chip = dev_get_drvdata(dev);
> if (chip == NULL)
> return -ENODEV;
>
> data = kzalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> + if (!data) {
> + rc = -ENOMEM;
> + goto out_alloc;
> + }
>
> memcpy(data, readpubek, sizeof(readpubek));
>
> err = transmit_cmd(chip, data, READ_PUBEK_RESULT_SIZE,
> "attempting to read the PUBEK");
> - if (err)
> + if (err) {
> + rc = str - buf;
> goto out;
> + }
>
> /*
> ignore header 10 bytes
> @@ -823,9 +886,11 @@ ssize_t tpm_show_pubek(struct device *dev, struct device_attribute *attr,
> if ((i + 1) % 16 == 0)
> str += sprintf(str, "\n");
> }
> -out:
> rc = str - buf;
> +out:
> kfree(data);
> +out_alloc:
> + put_device(chip->dev);
> return rc;
> }
> EXPORT_SYMBOL_GPL(tpm_show_pubek);
> @@ -847,14 +912,22 @@ ssize_t tpm_show_caps(struct device *dev, struct device_attribute *attr,
> u8 *data;
> ssize_t rc;
> char *str = buf;
> + struct tpm_chip *chip;
> +
> + rcu_read_lock();
> + chip = dev_get_drvdata(dev);
> + if (chip)
> + get_device(chip->dev); /* protect from disappearing */
> + rcu_read_unlock();
>
> - struct tpm_chip *chip = dev_get_drvdata(dev);
> if (chip == NULL)
> return -ENODEV;
>
> data = kzalloc(TPM_INTERNAL_RESULT_SIZE, GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> + if (!data) {
> + rc = -ENOMEM;
> + goto out_alloc;
> + }
>
> memcpy(data, tpm_cap, sizeof(tpm_cap));
> data[TPM_CAP_IDX] = TPM_CAP_PROP;
> @@ -863,8 +936,8 @@ ssize_t tpm_show_caps(struct device *dev, struct device_attribute *attr,
> rc = transmit_cmd(chip, data, TPM_INTERNAL_RESULT_SIZE,
> "attempting to determine the manufacturer");
> if (rc) {
> - kfree(data);
> - return 0;
> + rc = 0;
> + goto out;
> }
>
> str += sprintf(str, "Manufacturer: 0x%x\n",
> @@ -874,17 +947,22 @@ ssize_t tpm_show_caps(struct device *dev, struct device_attribute *attr,
> data[CAP_VERSION_IDX] = CAP_VERSION_1_1;
> rc = transmit_cmd(chip, data, TPM_INTERNAL_RESULT_SIZE,
> "attempting to determine the 1.1 version");
> - if (rc)
> + if (rc) {
> + rc = str - buf;
> goto out;
> + }
>
> str += sprintf(str,
> "TCG version: %d.%d\nFirmware version: %d.%d\n",
> (int) data[14], (int) data[15], (int) data[16],
> (int) data[17]);
>
> + rc = str - buf;
> out:
> kfree(data);
> - return str - buf;
> +out_alloc:
> + put_device(chip->dev);
> + return rc;
> }
> EXPORT_SYMBOL_GPL(tpm_show_caps);
>
> @@ -892,16 +970,25 @@ ssize_t tpm_show_caps_1_2(struct device * dev,
> struct device_attribute * attr, char *buf)
> {
> u8 *data;
> + ssize_t rc;
> ssize_t len;
> char *str = buf;
> + struct tpm_chip *chip;
> +
> + rcu_read_lock();
> + chip = dev_get_drvdata(dev);
> + if (chip)
> + get_device(chip->dev); /* protect from disappearing */
> + rcu_read_unlock();
>
> - struct tpm_chip *chip = dev_get_drvdata(dev);
> if (chip == NULL)
> return -ENODEV;
>
> data = kzalloc(TPM_INTERNAL_RESULT_SIZE, GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> + if (!data) {
> + rc = -ENOMEM;
> + goto out_alloc;
> + }
>
> memcpy(data, tpm_cap, sizeof(tpm_cap));
> data[TPM_CAP_IDX] = TPM_CAP_PROP;
> @@ -913,7 +1000,8 @@ ssize_t tpm_show_caps_1_2(struct device * dev,
> "attempting to determine the manufacturer\n",
> be32_to_cpu(*((__be32 *) (data + TPM_RET_CODE_IDX))));
> kfree(data);
> - return 0;
> + rc = 0;
> + goto out;
> }
>
> str += sprintf(str, "Manufacturer: 0x%x\n",
> @@ -927,6 +1015,7 @@ ssize_t tpm_show_caps_1_2(struct device * dev,
> dev_err(chip->dev, "A TPM error (%d) occurred "
> "attempting to determine the 1.2 version\n",
> be32_to_cpu(*((__be32 *) (data + TPM_RET_CODE_IDX))));
> + rc = str - buf;
> goto out;
> }
> str += sprintf(str,
> @@ -934,20 +1023,29 @@ ssize_t tpm_show_caps_1_2(struct device * dev,
> (int) data[16], (int) data[17], (int) data[18],
> (int) data[19]);
>
> + rc = str - buf;
> out:
> kfree(data);
> - return str - buf;
> +out_alloc:
> + put_device(chip->dev);
> + return rc;
> }
> EXPORT_SYMBOL_GPL(tpm_show_caps_1_2);
>
> ssize_t tpm_store_cancel(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - struct tpm_chip *chip = dev_get_drvdata(dev);
> + struct tpm_chip *chip;
> +
> + rcu_read_lock();
> + chip = dev_get_drvdata(dev);
> + if (chip)
> + get_device(chip->dev); /* protect from disappearing */
> + rcu_read_unlock();
> if (chip == NULL)
> return 0;
> -
> chip->vendor.cancel(chip);
> + put_device(chip->dev);
> return count;
> }
> EXPORT_SYMBOL_GPL(tpm_store_cancel);
> @@ -956,70 +1054,59 @@ EXPORT_SYMBOL_GPL(tpm_store_cancel);
> * Device file system interface to the TPM
> *
> * It's assured that the chip will be opened just once,
> - * by the check of is_open variable, which is protected
> - * by driver_lock.
> + * by the check of is_open variable.
> */
> int tpm_open(struct inode *inode, struct file *file)
> {
> - int rc = 0, minor = iminor(inode);
> + int minor = iminor(inode);
> struct tpm_chip *chip = NULL, *pos;
>
> - spin_lock(&driver_lock);
> -
> - list_for_each_entry(pos, &tpm_chip_list, list) {
> + rcu_read_lock();
> + list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
> if (pos->vendor.miscdev.minor == minor) {
> chip = pos;
> + get_device(chip->dev); /* protect from disappearing */
> break;
> }
> }
> + rcu_read_unlock();
> + if (!chip)
> + return -ENODEV;
>
> - if (chip == NULL) {
> - rc = -ENODEV;
> - goto err_out;
> - }
> -
> - if (chip->is_open) {
> + if (!test_and_set_bit(0, &chip->is_open)) {
> dev_dbg(chip->dev, "Another process owns this TPM\n");
> - rc = -EBUSY;
> - goto err_out;
> + return -EBUSY;
> }
>
> - chip->is_open = 1;
> - get_device(chip->dev); /* protect from chip disappearing */
> -
> - spin_unlock(&driver_lock);
> -
> chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
> if (chip->data_buffer == NULL) {
> - chip->is_open = 0;
> + clear_bit(0, &chip->is_open);
> put_device(chip->dev);
> return -ENOMEM;
> }
>
> atomic_set(&chip->data_pending, 0);
> -
> file->private_data = chip;
> return 0;
> -
> -err_out:
> - spin_unlock(&driver_lock);
> - return rc;
> }
> EXPORT_SYMBOL_GPL(tpm_open);
>
> +/*
> + * Called on file close
> + */
> int tpm_release(struct inode *inode, struct file *file)
> {
> struct tpm_chip *chip = file->private_data;
>
> flush_scheduled_work();
> - spin_lock(&driver_lock);
> file->private_data = NULL;
> del_singleshot_timer_sync(&chip->user_read_timer);
> atomic_set(&chip->data_pending, 0);
> - chip->is_open = 0;
> - put_device(chip->dev);
> kfree(chip->data_buffer);
> - spin_unlock(&driver_lock);
> +
> + clear_bit(0, &chip->is_open);
> +
> + put_device(chip->dev);
> return 0;
> }
> EXPORT_SYMBOL_GPL(tpm_release);
> @@ -1082,6 +1169,7 @@ ssize_t tpm_read(struct file *file, char __user *buf,
> return ret_size;
> }
> EXPORT_SYMBOL_GPL(tpm_read);
> +
> /*
> * Called on unloading the driver.
> *
> @@ -1098,13 +1186,11 @@ void tpm_remove_hardware(struct device *dev)
> }
>
> spin_lock(&driver_lock);
> -
> - list_del(&chip->list);
> -
> + list_del_rcu(&chip->list);
> spin_unlock(&driver_lock);
> + synchronize_rcu();
>
> misc_deregister(&chip->vendor.miscdev);
> -
> sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
> tpm_bios_log_teardown(chip->bios_dir);
>
> @@ -1152,8 +1238,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_resume);
> /*
> * Once all references to platform device are down to 0,
> * release all allocated structures.
> - * In case vendor provided release function,
> - * call it too.
> + * In case vendor provided release function, call it too.
> */
> static void tpm_dev_release(struct device *dev)
> {
> @@ -1176,8 +1261,8 @@ static void tpm_dev_release(struct device *dev)
> * upon errant exit from this function specific probe function should call
> * pci_disable_device
> */
> -struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vendor_specific
> - *entry)
> +struct tpm_chip *tpm_register_hardware(struct device *dev,
> + const struct tpm_vendor_specific *entry)
> {
> #define DEVNAME_SIZE 7
>
> @@ -1243,9 +1328,12 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
> }
>
> chip->bios_dir = tpm_bios_log_setup(devname);
> +
> + /* Make chip available */
> spin_lock(&driver_lock);
> - list_add(&chip->list, &tpm_chip_list);
> + list_add_rcu(&chip->list, &tpm_chip_list);
> spin_unlock(&driver_lock);
> + synchronize_rcu();
The above synchronize_rcu() is not needed -- unless it is required that
tpm_register_hardware() not return until it can be guaranteed that all
subsequent readers will see the new entry. (Of course, the current
CPU/task is guaranteed to see it in any subsequent call, even without
the synchronize_rcu().)
> return chip;
> }
> --
> 1.5.6.3
>
--
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