Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for retransmission.

[Enric Balletbo Serra]

2017-02-27 20:12 GMT+01:00 Wolfram Sang :
> Hi,
>
>> >> > Rather than trying small and smaller transfers, would it not be better
>> >> > to get the i2c core to expose the quirk info about transfer limits?
>> >> >
>> >>
>> >> Sounds a good idea to me, I guess the quirk info can be accessed with
>> >>
>> >>   tpm_dev.client->adapter->quirks->max_read_len
>> >>
>> >> so I think we don't need to touch the i2c core. I'll propose a second 
>> >> version of the patch.
>> >
>> > Hi Enric
>> >
>> > You should probably ask Wolfram Sang , the i2c
>> > subsystem maintainer. He may prefer adding an API call.
>
> Thanks for pointing me to this thread.
>
> I understand it looks tempting to use the quirks struct directly, but I
> don't think this is the proper solution. Quirks are complex and and to
> determine which one finally applies, you need all the logic encoded in
> i2c_check_for_quirks(). Which already gets called on every transfer.
>
> So, my suggestion would be to simply fall back to a sane minimum when
> the maximum failed. 32 (I2C_SMBUS_BLOCK_MAX) should be a good choice.
>

Sounds a good solution for me, I'll test and send a new version of the patches.

> BTW I noted that the original patch checks for -EINVAL. The core returns
> -EOPNOTSUPP, though. So, a) the patch needs to be adapted

Yes I already detected this, In this series I forget to fixup the
patch that fixed this when I did the git rebase. It's is fixed in the
second version.

> and b) it
> looks the i2c host driver returning -EINVAL could be converted to use
> the quirk infrastructure? Which driver is it?
>
> Regards,
>
>Wolfram
>

Regards,
  Enric

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 09/14] media: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe 
---
 drivers/media/cec/cec-core.c  | 3 +--
 drivers/media/media-devnode.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index aca3ab8..a475aa5 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -137,10 +137,9 @@ static int __must_check cec_devnode_register(struct 
cec_devnode *devnode,
 
/* Part 2: Initialize and register the character device */
cdev_init(>cdev, _devnode_fops);
-   devnode->cdev.kobj.parent = >dev.kobj;
devnode->cdev.owner = owner;
 
-   ret = cdev_add(>cdev, devnode->dev.devt, 1);
+   ret = device_add_cdev(>dev, >cdev);
if (ret < 0) {
pr_err("%s: cdev_add failed\n", __func__);
goto clr_bit;
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index f2772ba..8d4718c 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -255,9 +255,8 @@ int __must_check media_devnode_register(struct media_device 
*mdev,
/* Part 2: Initialize and register the character device */
cdev_init(>cdev, _devnode_fops);
devnode->cdev.owner = owner;
-   devnode->cdev.kobj.parent = >dev.kobj;
 
-   ret = cdev_add(>cdev, MKDEV(MAJOR(media_dev_t), 
devnode->minor), 1);
+   ret = device_add_cdev(>dev, >cdev);
if (ret < 0) {
pr_err("%s: cdev_add failed\n", __func__);
goto cdev_add_error;
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH 02/14] device-dax: utilize new device_add_cdev helper function

[Alexandre Belloni]

Hi,

A small comment, you must always have a commit message. Even if it is
small.

On 20/02/2017 at 22:00:41 -0700, Logan Gunthorpe wrote:
> Signed-off-by: Logan Gunthorpe 
> ---
>  drivers/dax/dax.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c
> index ed758b7..0d24822 100644
> --- a/drivers/dax/dax.c
> +++ b/drivers/dax/dax.c
> @@ -701,12 +701,12 @@ struct dax_dev *devm_create_dax_dev(struct dax_region 
> *dax_region,
>  
>   /* device_initialize() so cdev can reference kobj parent */
>   device_initialize(dev);
> + dev->devt = dev_t;
>  
>   cdev = _dev->cdev;
>   cdev_init(cdev, _fops);
>   cdev->owner = parent->driver->owner;
> - cdev->kobj.parent = >kobj;
> - rc = cdev_add(_dev->cdev, dev_t, 1);
> + rc = device_add_cdev(dev, cdev);
>   if (rc)
>   goto err_cdev;
>  
> @@ -716,7 +716,6 @@ struct dax_dev *devm_create_dax_dev(struct dax_region 
> *dax_region,
>   dax_dev->region = dax_region;
>   kref_get(_region->kref);
>  
> - dev->devt = dev_t;
>   dev->class = dax_class;
>   dev->parent = parent;
>   dev->groups = dax_attribute_groups;
> -- 
> 2.1.4
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 07/14] infiniband: utilize new device_add_cdev helper function

[Logan Gunthorpe]

This patch updates core/ucm.c which didn't originally use the
cdev.kobj.parent with it's parent device. I did not look heavily into
whether this was a bug or not, but it seems likely to me there would
be a use before free.

I also took a look at core/uverbs_main.c, core/user_mad.c and
hw/hfi1/device.c which utilize cdev.kobj.parent but because the
infiniband core seems to use kobjs internally instead of struct devices
they could not be converted to use the new helper API and still
directly manipulate the internals of the kobj.

Signed-off-by: Logan Gunthorpe 
---
 drivers/infiniband/core/ucm.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c
index e0a995b..38ea316 100644
--- a/drivers/infiniband/core/ucm.c
+++ b/drivers/infiniband/core/ucm.c
@@ -1283,18 +1283,20 @@ static void ib_ucm_add_one(struct ib_device *device)
set_bit(devnum, dev_map);
}
 
+   device_initialize(_dev->dev);
+   ucm_dev->dev.devt = base;
+
cdev_init(_dev->cdev, _fops);
ucm_dev->cdev.owner = THIS_MODULE;
kobject_set_name(_dev->cdev.kobj, "ucm%d", ucm_dev->devnum);
-   if (cdev_add(_dev->cdev, base, 1))
+   if (device_add_cdev(_dev->dev, _dev->cdev))
goto err;
 
ucm_dev->dev.class = _class;
ucm_dev->dev.parent = device->dma_device;
-   ucm_dev->dev.devt = ucm_dev->cdev.dev;
ucm_dev->dev.release = ib_ucm_release_dev;
dev_set_name(_dev->dev, "ucm%d", ucm_dev->devnum);
-   if (device_register(_dev->dev))
+   if (device_add(_dev->dev))
goto err_cdev;
 
if (device_create_file(_dev->dev, _attr_ibdev))
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 06/14] platform/chrome: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe 
---
 drivers/platform/chrome/cros_ec_dev.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_dev.c 
b/drivers/platform/chrome/cros_ec_dev.c
index 47268ec..658fb99 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -388,7 +388,6 @@ static int ec_device_probe(struct platform_device *pdev)
int retval = -ENOMEM;
struct device *dev = >dev;
struct cros_ec_platform *ec_platform = dev_get_platdata(dev);
-   dev_t devno = MKDEV(ec_major, pdev->id);
struct cros_ec_dev *ec = kzalloc(sizeof(*ec), GFP_KERNEL);
 
if (!ec)
@@ -401,6 +400,7 @@ static int ec_device_probe(struct platform_device *pdev)
ec->features[0] = -1U; /* Not cached yet */
ec->features[1] = -1U; /* Not cached yet */
device_initialize(>class_dev);
+   ec->class_dev.devt = MKDEV(ec_major, pdev->id);
cdev_init(>cdev, );
 
/*
@@ -408,8 +408,7 @@ static int ec_device_probe(struct platform_device *pdev)
 * Link cdev to the class device to be sure device is not used
 * before unbinding it.
 */
-   ec->cdev.kobj.parent = >class_dev.kobj;
-   retval = cdev_add(>cdev, devno, 1);
+   retval = device_add_cdev(>class_dev, >cdev);
if (retval) {
dev_err(dev, ": failed to add character device\n");
goto cdev_add_failed;
@@ -420,7 +419,6 @@ static int ec_device_probe(struct platform_device *pdev)
 * Link to the character device for creating the /dev entry
 * in devtmpfs.
 */
-   ec->class_dev.devt = ec->cdev.dev;
ec->class_dev.class = _class;
ec->class_dev.parent = dev;
ec->class_dev.release = __remove;
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 02/14] device-dax: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe 
---
 drivers/dax/dax.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c
index ed758b7..0d24822 100644
--- a/drivers/dax/dax.c
+++ b/drivers/dax/dax.c
@@ -701,12 +701,12 @@ struct dax_dev *devm_create_dax_dev(struct dax_region 
*dax_region,
 
/* device_initialize() so cdev can reference kobj parent */
device_initialize(dev);
+   dev->devt = dev_t;
 
cdev = _dev->cdev;
cdev_init(cdev, _fops);
cdev->owner = parent->driver->owner;
-   cdev->kobj.parent = >kobj;
-   rc = cdev_add(_dev->cdev, dev_t, 1);
+   rc = device_add_cdev(dev, cdev);
if (rc)
goto err_cdev;
 
@@ -716,7 +716,6 @@ struct dax_dev *devm_create_dax_dev(struct dax_region 
*dax_region,
dax_dev->region = dax_region;
kref_get(_region->kref);
 
-   dev->devt = dev_t;
dev->class = dax_class;
dev->parent = parent;
dev->groups = dax_attribute_groups;
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH 00/14] Cleanup chardev instances with helper function

[Richard Weinberger]

Logan,

Am 21.02.2017 um 06:00 schrieb Logan Gunthorpe:
> Hello,
> 
> Our story for this patch-set begins with a new driver I wrote and am in
> the process of submitting upstream. That driver creates a fairly
> standard char device and the code for it was copied from a similar
> instance in device-dax. However, upon review, Greg Kroah-Hartman
> noticed [1] a suspicious line that assigned to the parent field of
> the underlying kobject for the char device.
> 
> I removed that from my code and endeavoured to remove it from the
> code I copied as well. However, Dan Williams pointed out [2] that this
> code is necessary for correct reference counting of the underlying
> structures. This prompted me to do a fair bit more research and
> investigation into whats going on and I found it to be a common pattern.
> (Although misleading and though required to be correct, frequently
> forgotten.) This pattern is used in at least 15 places in the kernel
> and probably should have been used in at least 5 more.
> 
> This patch-set aims to correct this and hopefully prevent future
> developers from wasting their time on it. The first patch introduces
> a new helper API as originally proposed by Dan Williams [3]. Please
> see the commit message for that patch for a longer description of the
> problem and history.
> 
> The subsequent patches either update correct instances to use the
> new API or fix incorrect usages to ensure the cdev correctly takes
> a reference count using the new API (this is noted in those patches).
> 
> This moves all except four of the cdev.kobj.parent usages into the one
> cdev function, the remaining four are in the infiniband subsystem and
> I've left alone because that subsystem seems to make use of kobjects
> directly (instead of struct devices). These are noted in patch 7 of
> this series.
> 
> This series is based on v4.10 with the exception of the last patch
> which is for my new driver which, as yet, has not been accepted
> upstream.
> 
> @Dan the first patch in this series will need your sign-off.
> 
> Thanks,
> 
> Logan
> 
> [1] https://lkml.org/lkml/2017/2/10/370
> [2] https://lkml.org/lkml/2017/2/10/607
> [3] https://lkml.org/lkml/2017/2/13/700
> 
> Logan Gunthorpe (14):
>   chardev: add helper function to register char devs with a struct
> device
>   device-dax: utilize new device_add_cdev helper function
>   input: utilize new device_add_cdev helper function
>   gpiolib: utilize new device_add_cdev helper function
>   tpm-chip: utilize new device_add_cdev helper function
>   platform/chrome: utilize new device_add_cdev helper function
>   infiniband: utilize new device_add_cdev helper function
>   iio:core: utilize new device_add_cdev helper function
>   media: utilize new device_add_cdev helper function
>   mtd: utilize new device_add_cdev helper function
>   rapidio: utilize new device_add_cdev helper function
>   rtc: utilize new device_add_cdev helper function
>   scsi: utilize new device_add_cdev helper function
>   switchtec: utilize new device_add_cdev helper function
> 
>  drivers/char/tpm/tpm-chip.c  |  3 +--
>  drivers/dax/dax.c|  5 ++---
>  drivers/gpio/gpiolib.c   |  3 +--
>  drivers/iio/industrialio-core.c  |  3 +--
>  drivers/infiniband/core/ucm.c|  8 +---
>  drivers/input/evdev.c|  3 +--
>  drivers/input/joydev.c   |  3 +--
>  drivers/input/mousedev.c |  3 +--
>  drivers/media/cec/cec-core.c |  3 +--
>  drivers/media/media-devnode.c|  3 +--
>  drivers/mtd/ubi/build.c  |  8 +---
>  drivers/mtd/ubi/vmt.c| 10 +-
>  drivers/pci/switch/switchtec.c   |  3 +--
>  drivers/platform/chrome/cros_ec_dev.c|  6 ++
>  drivers/rapidio/devices/rio_mport_cdev.c |  9 ++---
>  drivers/rtc/rtc-dev.c|  3 +--
>  drivers/scsi/osd/osd_uld.c   |  9 +
>  fs/char_dev.c| 24 
>  include/linux/cdev.h |  1 +
>  19 files changed, 65 insertions(+), 45 deletions(-)

Do you have a git tree where I can pull from?

Thanks,
//richard

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 05/14] tpm-chip: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe 
---
 drivers/char/tpm/tpm-chip.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index a77262d..dc90b45 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -187,7 +187,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
 
cdev_init(>cdev, _fops);
chip->cdev.owner = THIS_MODULE;
-   chip->cdev.kobj.parent = >dev.kobj;
 
return chip;
 
@@ -230,7 +229,7 @@ static int tpm_add_char_device(struct tpm_chip *chip)
 {
int rc;
 
-   rc = cdev_add(>cdev, chip->dev.devt, 1);
+   rc = device_add_cdev(>dev, >cdev);
if (rc) {
dev_err(>dev,
"unable to cdev_add() %s, major %d, minor %d, err=%d\n",
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH 13/14] scsi: utilize new device_add_cdev helper function

[Boaz Harrosh]

On 02/21/2017 07:00 AM, Logan Gunthorpe wrote:
> Note: the chardev instance in osd_uld.c originally did not
> set the kobject parent. Thus, I'm reasonably confident that because
> of this, this code  would have suffered from a minor use after free
> bug if the cdev was open when the backing device was released.
> 
> Signed-off-by: Logan Gunthorpe 

Cool thanks. And even a bug fix
ACK-by: Boaz Harrosh 

> ---
>  drivers/scsi/osd/osd_uld.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> index 243eab3..519be56 100644
> --- a/drivers/scsi/osd/osd_uld.c
> +++ b/drivers/scsi/osd/osd_uld.c
> @@ -473,18 +473,19 @@ static int osd_probe(struct device *dev)
>   goto err_put_disk;
>   }
>  
> + device_initialize(>class_dev);
> + oud->class_dev.devt = MKDEV(SCSI_OSD_MAJOR, oud->minor);
> +
>   /* init the char-device for communication with user-mode */
>   cdev_init(>cdev, _fops);
>   oud->cdev.owner = THIS_MODULE;
> - error = cdev_add(>cdev,
> -  MKDEV(SCSI_OSD_MAJOR, oud->minor), 1);
> + error = device_add_cdev(>class_dev, >cdev);
>   if (error) {
>   OSD_ERR("cdev_add failed\n");
>   goto err_put_disk;
>   }
>  
>   /* class device member */
> - oud->class_dev.devt = oud->cdev.dev;
>   oud->class_dev.class = _uld_class;
>   oud->class_dev.parent = dev;
>   oud->class_dev.release = __remove;
> @@ -494,7 +495,7 @@ static int osd_probe(struct device *dev)
>   goto err_put_cdev;
>   }
>  
> - error = device_register(>class_dev);
> + error = device_add(>class_dev);
>   if (error) {
>   OSD_ERR("device_register failed => %d\n", error);
>   goto err_put_cdev;
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 01/14] chardev: add helper function to register char devs with a struct device

[Logan Gunthorpe]

Credit for this patch goes entirely to Dan Williams [1]. I've just
fleshed out the comments and created the patch, but the premise
remains exactly the same.

There's a common pattern in the kernel whereby a struct cdev is placed
in a structure along side a struct device which manages the life-cycle
of both. In the naive approach, the reference counting is broken and
the struct device can free everything before the chardev code
is entirely released.

Many developers have solved this problem by linking the internal kobjs
in this fashion:

cdev.kobj.parent = _dev.kobj;

The cdev code explicitly gets and puts a reference to it's kobj parent.
So this seems like it was intended to be used this way. Dmitrty Torokhov
first put this in place in 2012 with this commit:

2f0157f char_dev: pin parent kobject

and the first instance of the fix was then done in the input subsystem
in the following commit:

4a215aa Input: fix use-after-free introduced with dynamic minor changes

Subsequently over the years, however, this issue seems to have tripped
up multiple developers independently. For example, see these commits:

0d5b7da iio: Prevent race between IIO chardev opening and IIO device
(by Lars-Peter Clausen in 2013)

ba0ef85 tpm: Fix initialization of the cdev
(by Jason Gunthorpe in 2015)

5b28dde [media] media: fix use-after-free in cdev_put() when app exits
after driver unbind
(by Shauh Khan in 2016)

This technique is similarly done in at least 15 places within the kernel
and probably should have been done so in another, at least, 5 places.
The kobj line also looks very suspect in that one would not expect
drivers to have to mess with kobject internals in this way.
Even highly experienced kernel developers can be surprised by this
code, as seen in [2].

To help alleviate this situation, and hopefully prevent future
wasted effort on this problem, this patch introduces a helper function
to register a char device with its parent struct device. This creates
a more regular API for tying a char device to its parent without the
developer having to set members in the underlying kobject.

In [1], Dan notes he took inspiration for the form of the API
device_add_disk.

[1] https://lkml.org/lkml/2017/2/13/700
[2] https://lkml.org/lkml/2017/2/10/370

Signed-off-by: Logan Gunthorpe 
---
 fs/char_dev.c| 24 
 include/linux/cdev.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index 44a240c..1f9246c 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -471,6 +471,29 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count)
return 0;
 }
 
+/**
+ * device_add_cdev() - add a char device to the system with a parent
+ * struct device
+ * @parent: the device structure of the parent
+ * @cdev: the cdev structure for the device
+ * @count: the number of consecutive minor numbers corresponding to this
+ *
+ * device_add_cdev() adds the char device represented by @p to the system,
+ * just as cdev_add does. The dev_t for the char device will be taken from
+ * the struct device which needs to be initialized first. This helper
+ * function correctly takes a reference to the parent device so the parent
+ * will not get released until all references to the cdev are released.
+ * (Thus, cdev_del should be called before device_unregister.) This
+ * function should be used whenever the struct cdev and the struct device
+ * are members of the same structure whose lifetime is managed by the
+ * struct device.
+ */
+int device_add_cdev(struct device *parent, struct cdev *cdev)
+{
+   cdev->kobj.parent = >kobj;
+   return cdev_add(cdev, parent->devt, 1);
+}
+
 static void cdev_unmap(dev_t dev, unsigned count)
 {
kobj_unmap(cdev_map, dev, count);
@@ -570,5 +593,6 @@ EXPORT_SYMBOL(cdev_init);
 EXPORT_SYMBOL(cdev_alloc);
 EXPORT_SYMBOL(cdev_del);
 EXPORT_SYMBOL(cdev_add);
+EXPORT_SYMBOL(device_add_cdev);
 EXPORT_SYMBOL(__register_chrdev);
 EXPORT_SYMBOL(__unregister_chrdev);
diff --git a/include/linux/cdev.h b/include/linux/cdev.h
index f876361..9edbc37 100644
--- a/include/linux/cdev.h
+++ b/include/linux/cdev.h
@@ -25,6 +25,7 @@ struct cdev *cdev_alloc(void);
 void cdev_put(struct cdev *p);
 
 int cdev_add(struct cdev *, dev_t, unsigned);
+int device_add_cdev(struct device *parent, struct cdev *cdev);
 
 void cdev_del(struct cdev *);
 
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH 01/14] chardev: add helper function to register char devs with a struct device

[Logan Gunthorpe]

Hey,

Ok, I'll do a v2 later this week with some of the feedback and tags?

Logan

On 21/02/17 11:18 AM, Dan Williams wrote:
> On Mon, Feb 20, 2017 at 10:35 PM, Logan Gunthorpe  wrote:
>>
>>
>> On 20/02/17 10:35 PM, Dmitry Torokhov wrote:
>>> My only objection is to this statement. There is absolutely nothing that
>>> prevents from calling device_unregister() first and cdev_del() later.
>>> Refcounting will sort it all out.
>>
>> Yeah, I was really trying to warn people against calling cdev_del within
>> the release function of the device. If you do that, then the cdev
>> reference will block the device from ever getting released.
>>
>> Certainly, you could call device_unregister followed by cdev_del. I
>> could reword this if you feel it necessary.
> 
> I agree with Dmitry, just delete the statement in parenthesis and the
> rest is fine. If you're modifying this patch it might be good to take
> the opportunity to add a WARN_ON(!parent->kobj.state_initialized) to
> catch attempts to call device_add_cdev() with an uninitialized device.
> 
> You can also add:
> 
> Signed-off-by: Dan Williams 
> 

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 00/14] Cleanup chardev instances with helper function

[Logan Gunthorpe]

Hello,

Our story for this patch-set begins with a new driver I wrote and am in
the process of submitting upstream. That driver creates a fairly
standard char device and the code for it was copied from a similar
instance in device-dax. However, upon review, Greg Kroah-Hartman
noticed [1] a suspicious line that assigned to the parent field of
the underlying kobject for the char device.

I removed that from my code and endeavoured to remove it from the
code I copied as well. However, Dan Williams pointed out [2] that this
code is necessary for correct reference counting of the underlying
structures. This prompted me to do a fair bit more research and
investigation into whats going on and I found it to be a common pattern.
(Although misleading and though required to be correct, frequently
forgotten.) This pattern is used in at least 15 places in the kernel
and probably should have been used in at least 5 more.

This patch-set aims to correct this and hopefully prevent future
developers from wasting their time on it. The first patch introduces
a new helper API as originally proposed by Dan Williams [3]. Please
see the commit message for that patch for a longer description of the
problem and history.

The subsequent patches either update correct instances to use the
new API or fix incorrect usages to ensure the cdev correctly takes
a reference count using the new API (this is noted in those patches).

This moves all except four of the cdev.kobj.parent usages into the one
cdev function, the remaining four are in the infiniband subsystem and
I've left alone because that subsystem seems to make use of kobjects
directly (instead of struct devices). These are noted in patch 7 of
this series.

This series is based on v4.10 with the exception of the last patch
which is for my new driver which, as yet, has not been accepted
upstream.

@Dan the first patch in this series will need your sign-off.

Thanks,

Logan

[1] https://lkml.org/lkml/2017/2/10/370
[2] https://lkml.org/lkml/2017/2/10/607
[3] https://lkml.org/lkml/2017/2/13/700

Logan Gunthorpe (14):
  chardev: add helper function to register char devs with a struct
device
  device-dax: utilize new device_add_cdev helper function
  input: utilize new device_add_cdev helper function
  gpiolib: utilize new device_add_cdev helper function
  tpm-chip: utilize new device_add_cdev helper function
  platform/chrome: utilize new device_add_cdev helper function
  infiniband: utilize new device_add_cdev helper function
  iio:core: utilize new device_add_cdev helper function
  media: utilize new device_add_cdev helper function
  mtd: utilize new device_add_cdev helper function
  rapidio: utilize new device_add_cdev helper function
  rtc: utilize new device_add_cdev helper function
  scsi: utilize new device_add_cdev helper function
  switchtec: utilize new device_add_cdev helper function

 drivers/char/tpm/tpm-chip.c  |  3 +--
 drivers/dax/dax.c|  5 ++---
 drivers/gpio/gpiolib.c   |  3 +--
 drivers/iio/industrialio-core.c  |  3 +--
 drivers/infiniband/core/ucm.c|  8 +---
 drivers/input/evdev.c|  3 +--
 drivers/input/joydev.c   |  3 +--
 drivers/input/mousedev.c |  3 +--
 drivers/media/cec/cec-core.c |  3 +--
 drivers/media/media-devnode.c|  3 +--
 drivers/mtd/ubi/build.c  |  8 +---
 drivers/mtd/ubi/vmt.c| 10 +-
 drivers/pci/switch/switchtec.c   |  3 +--
 drivers/platform/chrome/cros_ec_dev.c|  6 ++
 drivers/rapidio/devices/rio_mport_cdev.c |  9 ++---
 drivers/rtc/rtc-dev.c|  3 +--
 drivers/scsi/osd/osd_uld.c   |  9 +
 fs/char_dev.c| 24 
 include/linux/cdev.h |  1 +
 19 files changed, 65 insertions(+), 45 deletions(-)

--
2.1.4

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [RFC] tpm2-space: add handling for global session exhaustion

[Dr. Greg Wettstein]

On Thu, Feb 16, 2017 at 09:04:47AM -0500, Ken Goldman wrote:

Good morning to everyone, leveraging some time between planes.

> On 2/14/2017 9:38 AM, Dr. Greg Wettstein wrote:
> >
> >I don't think there is any doubt that running cryptographic primitives
> >in userspace is going to be faster then going to hardware.  Obviously
> >that also means there is no need for a TPM resource manager which has
> >been the subject of much discussion here.

> I don't understand that comment.
>
> The resource manager schedules user space access to the TPM.  It also
> handles swapping of objects in and out of the limited number of
> TPM slots.
> 
> Without a RM, either you'd have to permit only a single TPM connection,
> blocking all other connections, or you'd have different connections
> interfering with each other.

Yes, if multiple contexts of execution require access to the TPM a
resource manager is needed to arbitrate that access.

I think, however, that we are talking past one another a bit.

We design and build systems which implement autonomous
self-regulation.  As such we need a hardware based confirmation that
the machine is in a given behavioral state.  This requires that we
reference a hardware root of trust, ie. the TPM.

Depending on the assurance granularity requirements, that may mean a
high rate of TPM verifications.  When I noticed you and James talking
about 'cloud based' levels of transactions I was assuming you were
operating at transaction rates we build for, ie. 10-100's/second.
That didn't seem feasible given our hardware measurements on Skylake
and Kabylake based systems.

James had cited the CoreOS/Tectonic white paper as an example of TPM's
working at cloud scale.  Our conversation to date seems to indicate
that the accepted modality of security appers to be to do userspace
verification of container signatures.  Given the extensive dialogue in
the paper about using TPM's for security we had inadvertently believed
that container verifications were being pinned to current platform
status which didn't correlate with expected container start time
latencies.

Our behavioral assessment code is namespaced so a supervisory system
can make statements about the behavior of a container.  We have
concluded the only way that is possible is to use userspace TPM
implementations which can meet the necessary latency requirements.

Our point in all this is that it doesn't seem to make any sense to
implement anything in the kernel more then basic resource management.
If other 'virtualization' is needed, such as session state management
and the like, the community would seem to be served better by having a
solid userspace simulation environment, with appropriate hardware
security guarantees.  That would serve needs like re-keying support
for VPNaaS applications as well as high transaction rate environments,
ie. why load the kernel with code to virtualize a resource when a
'user' can just be given its own TPM2 instance.

Just as an aside, has anyone given any thought about TPM2 resource
management in things like TXT/tboot environments?  The current tboot
code makes a rather naive assumption that it can take a handle slot to
protect its platform verification secret.  Doing resource management
correctly will require addressing extra-OS environments such as this
which may have TPM2 state requirement issues.

Our take away from all this is that it doesn't seem that we need to
worry about the fact that someone may have invented TPM2 hardware
which is faster then what we are developing on :-)

Have a good weekend.

Greg

As always,
Dr. G.W. Wettstein, Ph.D.   Enjellic Systems Development, LLC.
4206 N. 19th Ave.   Specializing in information infra-structure
Fargo, ND  58102development.
PH: 701-281-1686
FAX: 701-281-3949   EMAIL: g...@enjellic.com
--
"If you ever teach a yodeling class, probably the hardest thing is to
 keep the students from just trying to yodel right off. You see, we build
 to that."
-- Jack Handey
   Deep Thoughts

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH v2 6/7] tpm: expose spaces via a device link /dev/tpms

[Dr. Greg Wettstein]

On Sun, Feb 26, 2017 at 01:44:40PM +0200, Jarkko Sakkinen wrote:

Good day, I hope this note finds the week starting well for everyone.

> On Fri, Feb 24, 2017 at 08:02:08AM -0500, James Bottomley wrote:
> > On Thu, 2017-02-23 at 11:09 +0200, Jarkko Sakkinen wrote:
> > > On Thu, Feb 16, 2017 at 09:25:19PM +0200, Jarkko Sakkinen wrote:
> > > > From: James Bottomley 
> > > > 
> > > > Currently the tpm spaces are not exposed to userspace.  Make this
> > > > exposure via a separate device, which can now be opened multiple 
> > > > times because each read/write transaction goes separately via the
> > > > space.
> > > > 
> > > > Concurrency is protected by the chip->tpm_mutex for each read/write
> > > > transaction separately.  The TPM is cleared of all transient 
> > > > objects by the time the mutex is dropped, so there should be no
> > > > interference between the kernel and userspace.
> > > > Signed-off-by: James Bottomley <
> > > > 
> > > > james.bottom...@hansenpartnershp.com>
> > 
> > > Reviewed-by: Jarkko Sakkinen 
> > > Tested-by: Jarkko Sakkinen 
> > 
> > Thanks!
> > 
> > > Nitpicking but I've been thinking about naming. What about calling 
> > > the device as tpmrc0 as in resource context. I think that would be a
> > > better name than TPM space.
> > 
> > Well the original name was tpmrm for TPM with Resource Manager.  You
> > wanted it to be tpms for TPM with Spaces.
> > 
> > I'm not entirely sold on the Resource Context name ... I think
> > Resource Manager (because it's what the TCG calls it) or Spaces
> > (because it's what all the code comments call it) are better.
> > Resource Context sounds like what TPM2_SaveContext() creates for you
> > rather than the interface.
> > 
> > >  You do not mix it up with namespaces and/or virtualization. With
> > > resource in front it cannot be easily mixed up with TPM contexts
> > > either.
> > 
> > I'm a containers person.  What this set of patches does is precisely OS
> > level virtualization in my book, so I don't think you need to pretend
> > it is't; and OS level virtualization is what a namespace does.  The
> > only difference between this and the other kernel namespaces is that
> > you get a new namespace automatically when you open the device and you
> > can't enter an existing namespace.
> > 
> > I think therefore that tpmns for TPM Namespace would be very
> > appropriate.

> Sorry for going back and forth with this but I turn it back to your
> original tpmrm. It's in the end of the day the least confusing
> option. I think this has been anyway useful to trip a bit around the
> options because it is hard to rollback API...

Indeed, which is why we have watched this conversation with some
interest regarding exactly what the tpm{rm,ns} will represent.

In order to get a handle on how the resource manager will affect
Trusted Execution Technology (TXT) environments we downloaded your
branch and experimented with it a bit.  Our overall impression is that
it will be important to educate the user community on exactly what the
'tpm spaces' management device actually represents.

It is not a TPM device as much as it is a method of surfacing a subset
of TPM functionality over the lifespan of a file descriptor.  There is
certainly nothing wrong with this but users and system administrators
will need to understand the implications of this and not confuse the
two devices.

For example, Ken's tools which come in his TSS2 library, don't work
properly with the 'spaces' device due to the virtualization lifetime.
As an example, the getcapability call will 'lie' about the number of
transient handles which are available through the device.  Attempts to
string multiple transaction sequences together will fail as well.

So I think it will be important to stress that the 'spaces' device is
something an individual application will use to gain unimpeded access
to TPM functionality and not a representation of the device itself.
Given the quality of userspace simulators and their flexibility and
operational fidelity, I anticipate we will eventually come to the
conclusion that this problem will have been best solved by creating
and adopting a framework which provides the anchoring and spawning of
userspace TPM2 instances.

Just as an aside we still haven't been able to verify the
inter-operability of the 'spaces' driver with TXT.  There are larger
TXT/TPM2 operational issues which we are working to run down before we
can determine how the two systems will inter-operate or not.

> /Jarkko

Have a good day.

Greg

As always,
Dr. G.W. Wettstein, Ph.D.   Enjellic Systems Development, LLC.
4206 N. 19th Ave.   Specializing in information infra-structure
Fargo, ND  58102development.
PH: 701-281-1686
FAX: 701-281-3949   EMAIL: g...@enjellic.com
--
"You've got to be kidding me Nate.  

[tpmdd-devel] [PATCH 04/14] gpiolib: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe 
---
 drivers/gpio/gpiolib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a07ae9e..04dbc4a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1036,9 +1036,8 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
 
cdev_init(>chrdev, _fileops);
gdev->chrdev.owner = THIS_MODULE;
-   gdev->chrdev.kobj.parent = >dev.kobj;
gdev->dev.devt = MKDEV(MAJOR(gpio_devt), gdev->id);
-   status = cdev_add(>chrdev, gdev->dev.devt, 1);
+   status = device_add_cdev(>dev, >chrdev);
if (status < 0)
chip_warn(gdev->chip, "failed to add char device %d:%d\n",
  MAJOR(gpio_devt), gdev->id);
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [RFC] tpm2-space: add handling for global session exhaustion

[Dr. Greg Wettstein]

On Fri, Feb 17, 2017 at 02:37:12PM +0200, Jarkko Sakkinen wrote:

Hi, I hope the week is ending well for everyone.

> On Fri, Feb 17, 2017 at 03:56:26AM -0600, Dr. Greg Wettstein wrote:
> > On Thu, Feb 16, 2017 at 10:33:04PM +0200, Jarkko Sakkinen wrote:
> > 
> > Good morning to everyone.
> > 
> > > On Thu, Feb 16, 2017 at 02:06:42PM -0600, Dr. Greg Wettstein wrote:
> > > > Just as an aside, has anyone given any thought about TPM2 resource
> > > > management in things like TXT/tboot environments?  The current tboot
> > > > code makes a rather naive assumption that it can take a handle slot to
> > > > protect its platform verification secret.  Doing resource management
> > > > correctly will require addressing extra-OS environments such as this
> > > > which may have TPM2 state requirement issues.
> > 
> > > The current implementation handles stuff created from regular
> > > /dev/tpm0 so I do not think this would be an issue. You can only
> > > access objects from a TPM space that are created within that space.
> > 
> > Unless I misunderstand the number of transient objects which can be
> > managed is a characteristic of the hardware and is a limited resource,
> > hence our discussion on the notion of a resource manager to shuttle
> > context in and out of these limited slots.
> > 
> > On a Kabylake system, running the following command:
> > 
> > getcapability -cap 6 | grep trans
> > 
> > After booting into a TXT mediated measured launch environment (MLE) yields
> > the following:
> > 
> > TPM_PT 010e value 0003 TPM_PT_HR_TRANSIENT_MIN - the minimum number 
> > of transient objects that can be held in TPM RAM
> > 
> > TPM_PT 0207 value 0002 TPM_PT_HR_TRANSIENT_AVAIL - estimate of the 
> > number of additional transient objects that could be loaded into TPM RAM
> > 
> > Booting without TXT results in the getcapability call indicating that
> > three slots are available.  Based on that and reading the tboot code,
> > we are assuming the occupied slot is the ephemeral primary key
> > generated by tboot which seals the verification secret.
> > 
> > In an MLE it is possible to create and then flush a new ephemeral
> > primary key which results in the following getcapability output:
> > 
> > TPM_PT 0207 value 0003 TPM_PT_HR_TRANSIENT_AVAIL - estimate of
> > the number of additional transient objects that could be loaded into TPM RAM
> > 
> > Which is probably going to be pretty surprising to tboot in the event
> > that it tries to re-verify the system state after a suspend event.
> > 
> > So based on that it would seem there would need to be some semblance
> > of cooperation between the resource manager and an extra-OS
> > utilization of TPM2 resources such as tboot.
> > 
> > Thoughts?

> The driver swaps in and out all the objects for one send-receive
> cycle.  So unless the driver is sending a command to a TPM the
> resource manager occupies zero slots. I do not see reason for
> forseeable future to change this pattern.
>
> I discussed about some "lazier" schemes for swapping with James an
> Ken in the early Fall but came into conclusion that it would make
> the RM really complicated. There would have to be something show
> stopper work load to even to start consider it.
>
> With the capacity of current TPMs and amount of traffic and
> workloads it is really not a worth of the trouble.
>
> I guess the way we do swapping kind of indirectly sorts out the
> issue you described, doesn't it?

I'm not sure, we've pulled down your resource manager branch so we can
figure out the exact mechanics of how it works.  Based on a cursory
read of the code it appears as if it loops through all three transient
handle slots and attempts to context save each transient object it
finds.  So if it does that for each send/receive cycle it should
theoretically inter-operate with TXT/tboot.

As noted previously, with the current kernel driver, we can see that
tboot has allocated a slot for the ephemeral key which is used to seal
the memory verification secrets.  This key gets allocated to handle
8000 as one would anticipate.  However when we attempt to issue a
context save against that handle we get an error.

Interestingly, when we attempt to flush that handle manually we
receive an error as well, but the number of available transient
handles increases by one which suggests the context flush cleared the
slot.

It seems that we should be able to manually replicate what the
resource manager is doing with the standard kernel driver or is this
an incorrect assumption?

We will have to spin up a kernel with your patches and see how it
reacts to the presence of the extra-OS handle allocation.

> /Jarkko

Greg

As always,
Dr. G.W. Wettstein, Ph.D.   Enjellic Systems Development, LLC.
4206 N. 19th Ave.   Specializing in information infra-structure
Fargo, ND  58102development.
PH: 701-281-1686
FAX: 701-281-3949   EMAIL: g...@enjellic.com

[tpmdd-devel] [PATCH 12/14] rtc: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe 
---
 drivers/rtc/rtc-dev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
index a6d9434..e4012bb 100644
--- a/drivers/rtc/rtc-dev.c
+++ b/drivers/rtc/rtc-dev.c
@@ -477,12 +477,11 @@ void rtc_dev_prepare(struct rtc_device *rtc)
 
cdev_init(>char_dev, _dev_fops);
rtc->char_dev.owner = rtc->owner;
-   rtc->char_dev.kobj.parent = >dev.kobj;
 }
 
 void rtc_dev_add_device(struct rtc_device *rtc)
 {
-   if (cdev_add(>char_dev, rtc->dev.devt, 1))
+   if (device_add_cdev(>dev, >char_dev))
dev_warn(>dev, "%s: failed to add char device %d:%d\n",
rtc->name, MAJOR(rtc_devt), rtc->id);
else
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [RFC] tpm2-space: add handling for global session exhaustion

[Dr. Greg Wettstein]

On Thu, Feb 16, 2017 at 10:33:04PM +0200, Jarkko Sakkinen wrote:

Good morning to everyone.

> On Thu, Feb 16, 2017 at 02:06:42PM -0600, Dr. Greg Wettstein wrote:
> > Just as an aside, has anyone given any thought about TPM2 resource
> > management in things like TXT/tboot environments?  The current tboot
> > code makes a rather naive assumption that it can take a handle slot to
> > protect its platform verification secret.  Doing resource management
> > correctly will require addressing extra-OS environments such as this
> > which may have TPM2 state requirement issues.

> The current implementation handles stuff created from regular
> /dev/tpm0 so I do not think this would be an issue. You can only
> access objects from a TPM space that are created within that space.

Unless I misunderstand the number of transient objects which can be
managed is a characteristic of the hardware and is a limited resource,
hence our discussion on the notion of a resource manager to shuttle
context in and out of these limited slots.

On a Kabylake system, running the following command:

getcapability -cap 6 | grep trans

After booting into a TXT mediated measured launch environment (MLE) yields
the following:

TPM_PT 010e value 0003 TPM_PT_HR_TRANSIENT_MIN - the minimum number of 
transient objects that can be held in TPM RAM

TPM_PT 0207 value 0002 TPM_PT_HR_TRANSIENT_AVAIL - estimate of the 
number of additional transient objects that could be loaded into TPM RAM

Booting without TXT results in the getcapability call indicating that
three slots are available.  Based on that and reading the tboot code,
we are assuming the occupied slot is the ephemeral primary key
generated by tboot which seals the verification secret.

In an MLE it is possible to create and then flush a new ephemeral
primary key which results in the following getcapability output:

TPM_PT 0207 value 0003 TPM_PT_HR_TRANSIENT_AVAIL - estimate of
the number of additional transient objects that could be loaded into TPM RAM

Which is probably going to be pretty surprising to tboot in the event
that it tries to re-verify the system state after a suspend event.

So based on that it would seem there would need to be some semblance
of cooperation between the resource manager and an extra-OS
utilization of TPM2 resources such as tboot.

Thoughts?

> /Jarkko

Greg

As always,
Dr. G.W. Wettstein, Ph.D.   Enjellic Systems Development, LLC.
4206 N. 19th Ave.   Specializing in information infra-structure
Fargo, ND  58102development.
PH: 701-281-1686
FAX: 701-281-3949   EMAIL: g...@enjellic.com
--
"For a successful technology, reality must take precedence over public
 relations, for nature cannot be fooled."
-- Richard Feynmann

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 10/14] mtd: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Note: neither of the cdev instances in the mtd tree originally
set the kobject parent. Thus, I'm reasonably confident that
both these instances would have suffered from a minor use after
free bug if the cdevs were open when the backing device was released.

Signed-off-by: Logan Gunthorpe 
---
 drivers/mtd/ubi/build.c |  8 +---
 drivers/mtd/ubi/vmt.c   | 10 +-
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 85d54f3..a509f15 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -434,11 +434,10 @@ static int ubi_sysfs_init(struct ubi_device *ubi, int 
*ref)
int err;
 
ubi->dev.release = dev_release;
-   ubi->dev.devt = ubi->cdev.dev;
ubi->dev.class = _class;
ubi->dev.groups = ubi_dev_groups;
dev_set_name(>dev, UBI_NAME_STR"%d", ubi->ubi_num);
-   err = device_register(>dev);
+   err = device_add(>dev);
if (err)
return err;
 
@@ -508,12 +507,15 @@ static int uif_init(struct ubi_device *ubi, int *ref)
return err;
}
 
+   device_initialize(>dev);
+   ubi->dev.devt = dev;
+
ubi_assert(MINOR(dev) == 0);
cdev_init(>cdev, _cdev_operations);
dbg_gen("%s major is %u", ubi->ubi_name, MAJOR(dev));
ubi->cdev.owner = THIS_MODULE;
 
-   err = cdev_add(>cdev, dev, 1);
+   err = device_add_cdev(>dev, >cdev);
if (err) {
ubi_err(ubi, "cannot add character device");
goto out_unreg;
diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
index 7ac78c1..df84ba7 100644
--- a/drivers/mtd/ubi/vmt.c
+++ b/drivers/mtd/ubi/vmt.c
@@ -159,7 +159,6 @@ int ubi_create_volume(struct ubi_device *ubi, struct 
ubi_mkvol_req *req)
struct ubi_volume *vol;
struct ubi_vtbl_record vtbl_rec;
struct ubi_eba_table *eba_tbl = NULL;
-   dev_t dev;
 
if (ubi->ro_mode)
return -EROFS;
@@ -265,11 +264,13 @@ int ubi_create_volume(struct ubi_device *ubi, struct 
ubi_mkvol_req *req)
vol->last_eb_bytes = vol->usable_leb_size;
}
 
+   device_initialize(>dev);
+   vol->dev.devt = MKDEV(MAJOR(ubi->cdev.dev), vol_id + 1);
+
/* Register character device for the volume */
cdev_init(>cdev, _vol_cdev_operations);
vol->cdev.owner = THIS_MODULE;
-   dev = MKDEV(MAJOR(ubi->cdev.dev), vol_id + 1);
-   err = cdev_add(>cdev, dev, 1);
+   err = device_add_cdev(>dev, >cdev);
if (err) {
ubi_err(ubi, "cannot add character device");
goto out_mapping;
@@ -277,12 +278,11 @@ int ubi_create_volume(struct ubi_device *ubi, struct 
ubi_mkvol_req *req)
 
vol->dev.release = vol_release;
vol->dev.parent = >dev;
-   vol->dev.devt = dev;
vol->dev.class = _class;
vol->dev.groups = volume_dev_groups;
 
dev_set_name(>dev, "%s_%d", ubi->ubi_name, vol->vol_id);
-   err = device_register(>dev);
+   err = device_add(>dev);
if (err) {
ubi_err(ubi, "cannot register device");
goto out_cdev;
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 03/14] input: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe 
---
 drivers/input/evdev.c| 3 +--
 drivers/input/joydev.c   | 3 +--
 drivers/input/mousedev.c | 3 +--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..a3407ea 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -1416,8 +1416,7 @@ static int evdev_connect(struct input_handler *handler, 
struct input_dev *dev,
goto err_free_evdev;
 
cdev_init(>cdev, _fops);
-   evdev->cdev.kobj.parent = >dev.kobj;
-   error = cdev_add(>cdev, evdev->dev.devt, 1);
+   error = device_add_cdev(>dev, >cdev);
if (error)
goto err_unregister_handle;
 
diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index abd18f3..012e06f 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -905,8 +905,7 @@ static int joydev_connect(struct input_handler *handler, 
struct input_dev *dev,
goto err_free_joydev;
 
cdev_init(>cdev, _fops);
-   joydev->cdev.kobj.parent = >dev.kobj;
-   error = cdev_add(>cdev, joydev->dev.devt, 1);
+   error = device_add_cdev(>dev, >cdev);
if (error)
goto err_unregister_handle;
 
diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c
index b604564..efd8666 100644
--- a/drivers/input/mousedev.c
+++ b/drivers/input/mousedev.c
@@ -901,8 +901,7 @@ static struct mousedev *mousedev_create(struct input_dev 
*dev,
}
 
cdev_init(>cdev, _fops);
-   mousedev->cdev.kobj.parent = >dev.kobj;
-   error = cdev_add(>cdev, mousedev->dev.devt, 1);
+   error = device_add_cdev(>dev, >cdev);
if (error)
goto err_unregister_handle;
 
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 11/14] rapidio: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Note: the chardev instance in rio_mport_cdev originally did not
set the kobject parent. Thus, I'm reasonably confident that because
of this, this code  would have suffered from a minor use after free
bug if the cdev was open when the backing device was released.

Signed-off-by: Logan Gunthorpe 
---
 drivers/rapidio/devices/rio_mport_cdev.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/rapidio/devices/rio_mport_cdev.c 
b/drivers/rapidio/devices/rio_mport_cdev.c
index 9013a58..10a6b54 100644
--- a/drivers/rapidio/devices/rio_mport_cdev.c
+++ b/drivers/rapidio/devices/rio_mport_cdev.c
@@ -2445,23 +2445,26 @@ static struct mport_dev *mport_cdev_add(struct 
rio_mport *mport)
mutex_init(>buf_mutex);
mutex_init(>file_mutex);
INIT_LIST_HEAD(>file_list);
+
+   device_initialize(>dev);
+   md->dev.devt = MKDEV(MAJOR(dev_number), mport->id);
+
cdev_init(>cdev, _fops);
md->cdev.owner = THIS_MODULE;
-   ret = cdev_add(>cdev, MKDEV(MAJOR(dev_number), mport->id), 1);
+   ret = device_add_cdev(>dev, >cdev);
if (ret < 0) {
kfree(md);
rmcd_error("Unable to register a device, err=%d", ret);
return NULL;
}
 
-   md->dev.devt = md->cdev.dev;
md->dev.class = dev_class;
md->dev.parent = >dev;
md->dev.release = mport_device_release;
dev_set_name(>dev, DEV_NAME "%d", mport->id);
atomic_set(>active, 1);
 
-   ret = device_register(>dev);
+   ret = device_add(>dev);
if (ret) {
rmcd_error("Failed to register mport %d (err=%d)",
   mport->id, ret);
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH 02/14] device-dax: utilize new device_add_cdev helper function

[Dan Williams]

On Tue, Feb 21, 2017 at 3:37 AM, Alexandre Belloni
 wrote:
> Hi,
>
> A small comment, you must always have a commit message. Even if it is
> small.

Yes, something like: "Replace the open coded initialization of the
cdev with the new device_add_cdev() helper. The helper takes the
proper reference against the parent device while the cdev object is
alive."

With that you can also add:

Acked-by: Dan Williams 

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 08/14] iio:core: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe 
---
 drivers/iio/industrialio-core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index aaca428..b0ef245 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1718,8 +1718,7 @@ int iio_device_register(struct iio_dev *indio_dev)
 
cdev_init(_dev->chrdev, _buffer_fileops);
indio_dev->chrdev.owner = indio_dev->info->driver_module;
-   indio_dev->chrdev.kobj.parent = _dev->dev.kobj;
-   ret = cdev_add(_dev->chrdev, indio_dev->dev.devt, 1);
+   ret = device_add_cdev(_dev->dev, _dev->chrdev);
if (ret < 0)
goto error_unreg_eventset;
 
-- 
2.1.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for retransmission.

[Enric Balletbo Serra]

Bounce to Wolfram Sang

2017-02-22 15:01 GMT+01:00 Andrew Lunn :
> On Wed, Feb 22, 2017 at 12:16:08PM +0100, Enric Balletbo i Serra wrote:
>> Hi Andrew,
>>
>> Removing Bryan Freed from the loop as seems his email is not valid anymore. 
>> I already CC'ied Andrey which is doing the TPM bit in chromeos kernel.
>>
>> On 21/02/17 17:29, Andrew Lunn wrote:
>> > On Tue, Feb 21, 2017 at 03:44:59PM +0100, Enric Balletbo i Serra wrote:
>> >> From: Bryan Freed 
>> >>
>> >> When the I2C Infineon part is attached to an I2C adapter that imposes
>> >> a size limitation, large requests will fail -EINVAL.
>> >> Retry them with size backoff without re-issuing the 0x05 command
>> >> as this appears to occasionally put the TPM in a bad state.
>> >
>> > Hi Enric
>> >
>> > Rather than trying small and smaller transfers, would it not be better
>> > to get the i2c core to expose the quirk info about transfer limits?
>> >
>>
>> Sounds a good idea to me, I guess the quirk info can be accessed with
>>
>>   tpm_dev.client->adapter->quirks->max_read_len
>>
>> so I think we don't need to touch the i2c core. I'll propose a second 
>> version of the patch.
>
> Hi Enric
>
> You should probably ask Wolfram Sang , the i2c
> subsystem maintainer. He may prefer adding an API call.
>
>   Andrew

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for retransmission.

[Wolfram Sang]

Hi,

> >> > Rather than trying small and smaller transfers, would it not be better
> >> > to get the i2c core to expose the quirk info about transfer limits?
> >> >
> >>
> >> Sounds a good idea to me, I guess the quirk info can be accessed with
> >>
> >>   tpm_dev.client->adapter->quirks->max_read_len
> >>
> >> so I think we don't need to touch the i2c core. I'll propose a second 
> >> version of the patch.
> >
> > Hi Enric
> >
> > You should probably ask Wolfram Sang , the i2c
> > subsystem maintainer. He may prefer adding an API call.

Thanks for pointing me to this thread.

I understand it looks tempting to use the quirks struct directly, but I
don't think this is the proper solution. Quirks are complex and and to
determine which one finally applies, you need all the logic encoded in
i2c_check_for_quirks(). Which already gets called on every transfer.

So, my suggestion would be to simply fall back to a sane minimum when
the maximum failed. 32 (I2C_SMBUS_BLOCK_MAX) should be a good choice.

BTW I noted that the original patch checks for -EINVAL. The core returns
-EOPNOTSUPP, though. So, a) the patch needs to be adapted and b) it
looks the i2c host driver returning -EINVAL could be converted to use
the quirk infrastructure? Which driver is it?

Regards,

   Wolfram



signature.asc
Description: PGP signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH v2 6/7] tpm: expose spaces via a device link /dev/tpms

[Jason Gunthorpe]

On Sun, Feb 26, 2017 at 01:44:40PM +0200, Jarkko Sakkinen wrote:
 
> There's now tabrm-v3 branch. I had to tweak error handling in your
> device adding patch because of b4e9d7561a70. I hope I didn't break
> anything.

Just looked, the flow seemed like it works to me. Just confusing that
tpm_add_char_device isn't undone by tpm_del_char_device

Jason

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH v2 6/7] tpm: expose spaces via a device link /dev/tpms

[Jason Gunthorpe]

On Sat, Feb 25, 2017 at 12:04:49PM -0500, James Bottomley wrote:

> >  device cgroup blocks access to the cdevs of tpm0 but not to the
> > sysfs files.
> 
> What the device cgroup currently does for us and what it could do are
> two different things.  It seems if it exported
> __devcgroup_check_permission, we could use that as a check to gate the
> sysfs file access.

Make sense, maybe we should be doing that..

Stefan, are you still interested in this? This seems like a fairly
simple solution to your problem???

> > I am talking about using a situation like kernel IMA or keyring in 
> > the container with a tpm that is not tpm0, eg a vtpm.
> 
> a vtpm appears as a tpm device so it can be controlled by the device
> cgroup ... I think I'm not seeing the issue.

When an in-kernel call opens the TPM it does not go through the cdev,
it does something like this:

extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);

And hardwires 'chip_num' to TPM_ANY_NUM. Keyring does the same (see
trusted_instantiate)

Practically speaking this means in-kernel callers pretty much always
operate on tpm0.

I think we need to change TPM_ANY_NUM to something more container
friendly, but I'm not sure what that should be.

> be done at all) it's usually better to start with use cases.  So
> instead of saying we need to virtualize the PCRs we should start with X
> container has this requirement for attestation of its Y state.  Often
> the best way simply is an extension of the multi user model for the
> resource ... in this case no-one's really come up with one for PCRs, so
> that might be the place to begin.

Broadly makes sense to me.

Maybe kernel keyring is a better example, it already has a multi-user
model.

Jason

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel