diff mbox series

[v5,4/6] tee: add tee_dev_open_helper() primitive

Message ID 20210826123032.1732926-5-jens.wiklander@linaro.org
State New
Headers show
Series Asynchronous notifications from secure world | expand

Commit Message

Jens Wiklander Aug. 26, 2021, 12:30 p.m. UTC
Adds tee_dev_open_helper() and tee_dev_ctx_put() to make it easier to
create a driver internal struct tee_context without the usual
tee_device_get() on the struct tee_device as that adds a circular
reference counter dependency and would prevent the struct tee_device
from ever being released again.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_core.c  | 33 ++++++++++++++++++++++++---------
 include/linux/tee_drv.h | 27 +++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 9 deletions(-)

Comments

Sumit Garg Aug. 27, 2021, 6:23 a.m. UTC | #1
On Thu, 26 Aug 2021 at 18:00, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>

> Adds tee_dev_open_helper() and tee_dev_ctx_put() to make it easier to

> create a driver internal struct tee_context without the usual

> tee_device_get() on the struct tee_device as that adds a circular

> reference counter dependency and would prevent the struct tee_device

> from ever being released again.


Can you elaborate regarding the circular dependency issue you are
referring to? IIUC, lifespan for struct tee_device would look like
below with normal teedev_open():

optee_probe() {
  ...
  tee_device_alloc()            <- teedev->num_users = 1
  tee_device_register()
  optee_notif_init()               <- tee_device_get()
  ...
}

optee_remove() {
  ...
  optee_notif_uninit()           <- tee_device_put()
  tee_device_unregister()    <- tee_device_put()
  ...
}

-Sumit

>

> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

> ---

>  drivers/tee/tee_core.c  | 33 ++++++++++++++++++++++++---------

>  include/linux/tee_drv.h | 27 +++++++++++++++++++++++++++

>  2 files changed, 51 insertions(+), 9 deletions(-)

>

> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c

> index 85102d12d716..3beb682684a8 100644

> --- a/drivers/tee/tee_core.c

> +++ b/drivers/tee/tee_core.c

> @@ -43,14 +43,11 @@ static DEFINE_SPINLOCK(driver_lock);

>  static struct class *tee_class;

>  static dev_t tee_devt;

>

> -static struct tee_context *teedev_open(struct tee_device *teedev)

> +struct tee_context *tee_dev_open_helper(struct tee_device *teedev)

>  {

>         int rc;

>         struct tee_context *ctx;

>

> -       if (!tee_device_get(teedev))

> -               return ERR_PTR(-EINVAL);

> -

>         ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);

>         if (!ctx) {

>                 rc = -ENOMEM;

> @@ -66,10 +63,30 @@ static struct tee_context *teedev_open(struct tee_device *teedev)

>         return ctx;

>  err:

>         kfree(ctx);

> -       tee_device_put(teedev);

>         return ERR_PTR(rc);

>

>  }

> +EXPORT_SYMBOL_GPL(tee_dev_open_helper);

> +

> +void tee_dev_ctx_put(struct tee_context *ctx)

> +{

> +       teedev_ctx_put(ctx);

> +}

> +EXPORT_SYMBOL_GPL(tee_dev_ctx_put);

> +

> +static struct tee_context *teedev_open(struct tee_device *teedev)

> +{

> +       struct tee_context *ctx;

> +

> +       if (!tee_device_get(teedev))

> +               return ERR_PTR(-EINVAL);

> +

> +       ctx = tee_dev_open_helper(teedev);

> +       if (IS_ERR(ctx))

> +               tee_device_put(teedev);

> +

> +       return ctx;

> +}

>

>  void teedev_ctx_get(struct tee_context *ctx)

>  {

> @@ -90,10 +107,8 @@ static void teedev_ctx_release(struct kref *ref)

>

>  void teedev_ctx_put(struct tee_context *ctx)

>  {

> -       if (ctx->releasing)

> -               return;

> -

> -       kref_put(&ctx->refcount, teedev_ctx_release);

> +       if (ctx && !ctx->releasing)

> +               kref_put(&ctx->refcount, teedev_ctx_release);

>  }

>

>  static void teedev_close_context(struct tee_context *ctx)

> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h

> index 3ebfea0781f1..00a31ff03049 100644

> --- a/include/linux/tee_drv.h

> +++ b/include/linux/tee_drv.h

> @@ -458,6 +458,33 @@ static inline int tee_shm_get_id(struct tee_shm *shm)

>   */

>  struct tee_shm *tee_shm_get_from_id(struct tee_context *ctx, int id);

>

> +/**

> + * tee_dev_open_helper() - helper function to make a struct tee_context

> + * @teedev:    Device to open

> + *

> + * Creates the struct tee_context without increasing the reference counter

> + * on @teedev. This is needed for instance when a driver need an internal

> + * struct tee_context to operate on. By skipping the reference counter

> + * the circular dependency is broken.

> + *

> + * Note that this struct tee_context need special care when freeing in

> + * order to avoid the normal put on the struct tee_device.

> + * tee_dev_ctx_put() is the best choice for this.

> + *

> + * @returns a pointer 'struct tee_context' on success or an ERR_PTR on failure

> + */

> +struct tee_context *tee_dev_open_helper(struct tee_device *teedev);

> +

> +/**

> + * tee_dev_ctx_put() - helper function to release a struct tee_context

> + * @ctx:       The struct tee_context to release

> + *

> + * Note that this function doesn't do a tee_device_put() on the internal

> + * struct tee_device so this function should normal only be used when

> + * releasing a struct tee_context obtained with tee_dev_open_helper().

> + */

> +void tee_dev_ctx_put(struct tee_context *ctx);

> +

>  /**

>   * tee_client_open_context() - Open a TEE context

>   * @start:     if not NULL, continue search after this context

> --

> 2.31.1

>
Jens Wiklander Aug. 27, 2021, 9:26 a.m. UTC | #2
On Fri, Aug 27, 2021 at 8:23 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>

> On Thu, 26 Aug 2021 at 18:00, Jens Wiklander <jens.wiklander@linaro.org> wrote:

> >

> > Adds tee_dev_open_helper() and tee_dev_ctx_put() to make it easier to

> > create a driver internal struct tee_context without the usual

> > tee_device_get() on the struct tee_device as that adds a circular

> > reference counter dependency and would prevent the struct tee_device

> > from ever being released again.

>

> Can you elaborate regarding the circular dependency issue you are

> referring to? IIUC, lifespan for struct tee_device would look like

> below with normal teedev_open():

>

> optee_probe() {

>   ...

>   tee_device_alloc()            <- teedev->num_users = 1

>   tee_device_register()

>   optee_notif_init()               <- tee_device_get()

>   ...

> }

>

> optee_remove() {

>   ...

>   optee_notif_uninit()           <- tee_device_put()

>   tee_device_unregister()    <- tee_device_put()

>   ...

> }


You're right, it works just as well without this patch.
teedev_close_context() and teedev_open() must be exported in another
patch though. I'll replace this patch with such a patch in the next
patch set.


Thanks,
Jens

>

> -Sumit

>

> >

> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

> > ---

> >  drivers/tee/tee_core.c  | 33 ++++++++++++++++++++++++---------

> >  include/linux/tee_drv.h | 27 +++++++++++++++++++++++++++

> >  2 files changed, 51 insertions(+), 9 deletions(-)

> >

> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c

> > index 85102d12d716..3beb682684a8 100644

> > --- a/drivers/tee/tee_core.c

> > +++ b/drivers/tee/tee_core.c

> > @@ -43,14 +43,11 @@ static DEFINE_SPINLOCK(driver_lock);

> >  static struct class *tee_class;

> >  static dev_t tee_devt;

> >

> > -static struct tee_context *teedev_open(struct tee_device *teedev)

> > +struct tee_context *tee_dev_open_helper(struct tee_device *teedev)

> >  {

> >         int rc;

> >         struct tee_context *ctx;

> >

> > -       if (!tee_device_get(teedev))

> > -               return ERR_PTR(-EINVAL);

> > -

> >         ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);

> >         if (!ctx) {

> >                 rc = -ENOMEM;

> > @@ -66,10 +63,30 @@ static struct tee_context *teedev_open(struct tee_device *teedev)

> >         return ctx;

> >  err:

> >         kfree(ctx);

> > -       tee_device_put(teedev);

> >         return ERR_PTR(rc);

> >

> >  }

> > +EXPORT_SYMBOL_GPL(tee_dev_open_helper);

> > +

> > +void tee_dev_ctx_put(struct tee_context *ctx)

> > +{

> > +       teedev_ctx_put(ctx);

> > +}

> > +EXPORT_SYMBOL_GPL(tee_dev_ctx_put);

> > +

> > +static struct tee_context *teedev_open(struct tee_device *teedev)

> > +{

> > +       struct tee_context *ctx;

> > +

> > +       if (!tee_device_get(teedev))

> > +               return ERR_PTR(-EINVAL);

> > +

> > +       ctx = tee_dev_open_helper(teedev);

> > +       if (IS_ERR(ctx))

> > +               tee_device_put(teedev);

> > +

> > +       return ctx;

> > +}

> >

> >  void teedev_ctx_get(struct tee_context *ctx)

> >  {

> > @@ -90,10 +107,8 @@ static void teedev_ctx_release(struct kref *ref)

> >

> >  void teedev_ctx_put(struct tee_context *ctx)

> >  {

> > -       if (ctx->releasing)

> > -               return;

> > -

> > -       kref_put(&ctx->refcount, teedev_ctx_release);

> > +       if (ctx && !ctx->releasing)

> > +               kref_put(&ctx->refcount, teedev_ctx_release);

> >  }

> >

> >  static void teedev_close_context(struct tee_context *ctx)

> > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h

> > index 3ebfea0781f1..00a31ff03049 100644

> > --- a/include/linux/tee_drv.h

> > +++ b/include/linux/tee_drv.h

> > @@ -458,6 +458,33 @@ static inline int tee_shm_get_id(struct tee_shm *shm)

> >   */

> >  struct tee_shm *tee_shm_get_from_id(struct tee_context *ctx, int id);

> >

> > +/**

> > + * tee_dev_open_helper() - helper function to make a struct tee_context

> > + * @teedev:    Device to open

> > + *

> > + * Creates the struct tee_context without increasing the reference counter

> > + * on @teedev. This is needed for instance when a driver need an internal

> > + * struct tee_context to operate on. By skipping the reference counter

> > + * the circular dependency is broken.

> > + *

> > + * Note that this struct tee_context need special care when freeing in

> > + * order to avoid the normal put on the struct tee_device.

> > + * tee_dev_ctx_put() is the best choice for this.

> > + *

> > + * @returns a pointer 'struct tee_context' on success or an ERR_PTR on failure

> > + */

> > +struct tee_context *tee_dev_open_helper(struct tee_device *teedev);

> > +

> > +/**

> > + * tee_dev_ctx_put() - helper function to release a struct tee_context

> > + * @ctx:       The struct tee_context to release

> > + *

> > + * Note that this function doesn't do a tee_device_put() on the internal

> > + * struct tee_device so this function should normal only be used when

> > + * releasing a struct tee_context obtained with tee_dev_open_helper().

> > + */

> > +void tee_dev_ctx_put(struct tee_context *ctx);

> > +

> >  /**

> >   * tee_client_open_context() - Open a TEE context

> >   * @start:     if not NULL, continue search after this context

> > --

> > 2.31.1

> >
diff mbox series

Patch

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 85102d12d716..3beb682684a8 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -43,14 +43,11 @@  static DEFINE_SPINLOCK(driver_lock);
 static struct class *tee_class;
 static dev_t tee_devt;
 
-static struct tee_context *teedev_open(struct tee_device *teedev)
+struct tee_context *tee_dev_open_helper(struct tee_device *teedev)
 {
 	int rc;
 	struct tee_context *ctx;
 
-	if (!tee_device_get(teedev))
-		return ERR_PTR(-EINVAL);
-
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx) {
 		rc = -ENOMEM;
@@ -66,10 +63,30 @@  static struct tee_context *teedev_open(struct tee_device *teedev)
 	return ctx;
 err:
 	kfree(ctx);
-	tee_device_put(teedev);
 	return ERR_PTR(rc);
 
 }
+EXPORT_SYMBOL_GPL(tee_dev_open_helper);
+
+void tee_dev_ctx_put(struct tee_context *ctx)
+{
+	teedev_ctx_put(ctx);
+}
+EXPORT_SYMBOL_GPL(tee_dev_ctx_put);
+
+static struct tee_context *teedev_open(struct tee_device *teedev)
+{
+	struct tee_context *ctx;
+
+	if (!tee_device_get(teedev))
+		return ERR_PTR(-EINVAL);
+
+	ctx = tee_dev_open_helper(teedev);
+	if (IS_ERR(ctx))
+		tee_device_put(teedev);
+
+	return ctx;
+}
 
 void teedev_ctx_get(struct tee_context *ctx)
 {
@@ -90,10 +107,8 @@  static void teedev_ctx_release(struct kref *ref)
 
 void teedev_ctx_put(struct tee_context *ctx)
 {
-	if (ctx->releasing)
-		return;
-
-	kref_put(&ctx->refcount, teedev_ctx_release);
+	if (ctx && !ctx->releasing)
+		kref_put(&ctx->refcount, teedev_ctx_release);
 }
 
 static void teedev_close_context(struct tee_context *ctx)
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 3ebfea0781f1..00a31ff03049 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -458,6 +458,33 @@  static inline int tee_shm_get_id(struct tee_shm *shm)
  */
 struct tee_shm *tee_shm_get_from_id(struct tee_context *ctx, int id);
 
+/**
+ * tee_dev_open_helper() - helper function to make a struct tee_context
+ * @teedev:	Device to open
+ *
+ * Creates the struct tee_context without increasing the reference counter
+ * on @teedev. This is needed for instance when a driver need an internal
+ * struct tee_context to operate on. By skipping the reference counter
+ * the circular dependency is broken.
+ *
+ * Note that this struct tee_context need special care when freeing in
+ * order to avoid the normal put on the struct tee_device.
+ * tee_dev_ctx_put() is the best choice for this.
+ *
+ * @returns a pointer 'struct tee_context' on success or an ERR_PTR on failure
+ */
+struct tee_context *tee_dev_open_helper(struct tee_device *teedev);
+
+/**
+ * tee_dev_ctx_put() - helper function to release a struct tee_context
+ * @ctx:	The struct tee_context to release
+ *
+ * Note that this function doesn't do a tee_device_put() on the internal
+ * struct tee_device so this function should normal only be used when
+ * releasing a struct tee_context obtained with tee_dev_open_helper().
+ */
+void tee_dev_ctx_put(struct tee_context *ctx);
+
 /**
  * tee_client_open_context() - Open a TEE context
  * @start:	if not NULL, continue search after this context