lkml.org 
[lkml]   [2008]   [Sep]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] TPM: rcu locking
Quoting Paul E. McKenney (paulmck@linux.vnet.ibm.com):
> 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@linux.vnet.ibm.com>
> > Acked-by: Rajiv Andrade <srajiv@linux.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?

Yes, a good idea, because in the second repetition of this code sequence
you have the wrong set of gotos, and can end up doing
put_device(chip->dev) when dev is NULL :)

> 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

Good point. Which I missed entirely when I saw the patch before.

Taking the case of tpm_show_enabled(), it is called as a sysfs file
callback, right? So the sysfs/kobject infrastructure will pin the
device for the duration of the callback I presume?

> 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
> >


\
 
 \ /
  Last update: 2008-09-02 17:21    [from the cache]
©2003-2014 Jasper Spaans. hosted at Digital Ocean