[PATCH] tpm: tpm_crb: relinquish locality on error path.

In crb_map_io() function, __crb_request_locality() is called prior
to crb_cmd_ready(), but if one of the consecutive function fails
the flow bails out instead of trying to relinquish locality.
This patch adds goto jump to __crb_relinquish_locality() on the error path.

Fixes: 888d867df441 (tpm: cmd_ready command can be issued only after granting locality)
Signed-off-by: Tomas Winkler <redacted>
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 7f78482cd157..34fbc6cb097b 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf->control_address,
 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}
 
 	/*
 	 * PTT HW bug w/a: wake up the device to access

@@ -520,7 +522,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;
 
 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	crb_go_idle(dev, priv);
 
+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);
 
 	return ret;

-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

Thanks, please just call it before returning in the error path.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function, __crb_request_locality() is called prior to
crb_cmd_ready(), but if one of the consecutive function fails the flow
bails out instead of trying to relinquish locality.
This patch adds goto jump to __crb_relinquish_locality() on the error path.

Fixes: 888d867df441 (tpm: cmd_ready command can be issued only after
granting
locality)
Signed-off-by: Tomas Winkler <redacted>
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 7f78482cd157..34fbc6cb097b 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct acpi_device *device,

struct crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf->control_address,
 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to access @@ -520,7 +522,7

@@

quoted

static int crb_map_io(struct acpi_device *device, struct crb_priv
*priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct acpi_device *device,

struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error path.

Can you please elaborate why, isn't the centralized exiting of functions preferred kernel coding style? 
https://www.kernel.org/doc/html/v4.11/process/coding-style.html#centralized-exiting-of-functions 

Thanks
Tomas

/Jarkko

????{.n?+???????+%?????????w??{.n?+????{??????????v?^?)????w*jg???
???????j????G??????
???j:+v???w?j?m?????
??
?w?????f???h?????????

On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:

quoted

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function, __crb_request_locality() is called prior to
crb_cmd_ready(), but if one of the consecutive function fails the flow
bails out instead of trying to relinquish locality.
This patch adds goto jump to __crb_relinquish_locality() on the error
path.

Fixes: 888d867df441 (tpm: cmd_ready command can be issued only after
granting
locality)
Signed-off-by: Tomas Winkler <redacted>
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 7f78482cd157..34fbc6cb097b 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct acpi_device *device,

struct crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf-

quoted

control_address,

 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to access @@ -520,7 +522,7

@@

quoted

static int crb_map_io(struct acpi_device *device, struct crb_priv
*priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct acpi_device *device,

struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error path.

Can you please elaborate why, isn't the centralized exiting of functions
preferred kernel coding style? 
https://www.kernel.org/doc/html/v4.11/process/coding-style.html#centralized-ex
iting-of-functions 

You exit only from one location (not multiple) and not from a
nested context. Here you just add more complexity by doing
this.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:

quoted

quoted

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function, __crb_request_locality() is called prior
to crb_cmd_ready(), but if one of the consecutive function fails
the flow bails out instead of trying to relinquish locality.
This patch adds goto jump to __crb_relinquish_locality() on the
error path.

Fixes: 888d867df441 (tpm: cmd_ready command can be issued only
after granting
locality)
Signed-off-by: Tomas Winkler <redacted>
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c

b/drivers/char/tpm/tpm_crb.c index 7f78482cd157..34fbc6cb097b
100644

--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct acpi_device

*device, struct crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf-

quoted

control_address,

 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to access @@ -520,7 +522,7

@@

quoted

static int crb_map_io(struct acpi_device *device, struct crb_priv
*priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct acpi_device

*device, struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error path.

Can you please elaborate why, isn't the centralized exiting of
functions preferred kernel coding style?
https://www.kernel.org/doc/html/v4.11/process/coding-style.html#centra
lized-ex
iting-of-functions

You exit only from one location (not multiple) and not from a nested
context. Here you just add more complexity by doing this.

Where is the complexitity ? I see it as a standard way of undoing on exit.
Tomas 

????{.n?+???????+%?????????w??{.n?+????{??????????v?^?)????w*jg???
???????j????G??????
???j:+v???w?j?m?????
??
?w?????f???h?????????

quoted

On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:

quoted

quoted

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function, __crb_request_locality() is called
prior to crb_cmd_ready(), but if one of the consecutive function
fails the flow bails out instead of trying to relinquish locality.
This patch adds goto jump to __crb_relinquish_locality() on the
error path.

Fixes: 888d867df441 (tpm: cmd_ready command can be issued only
after granting
locality)
Signed-off-by: Tomas Winkler <redacted>
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c

b/drivers/char/tpm/tpm_crb.c index 7f78482cd157..34fbc6cb097b
100644

--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct acpi_device

*device, struct crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf-

quoted

control_address,

 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to access @@ -520,7
+522,7

@@

quoted

static int crb_map_io(struct acpi_device *device, struct
crb_priv *priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct acpi_device

*device, struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error path.

Can you please elaborate why, isn't the centralized exiting of
functions preferred kernel coding style?
https://www.kernel.org/doc/html/v4.11/process/coding-style.html#cent
ra
lized-ex
iting-of-functions

You exit only from one location (not multiple) and not from a nested
context. Here you just add more complexity by doing this.

Where is the complexity ? I see it as a standard way of undoing on exit.
Tomas

Jarkko, can you please respond.
Thanks
Tomas

????{.n?+???????+%?????????w??{.n?+????{??????????v?^?)????w*jg???
???????j????G??????
???j:+v???w?j?m?????
??
?w?????f???h?????????

On Fri, Apr 20, 2018 at 01:19:12PM +0000, Winkler, Tomas wrote:

quoted

quoted

On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:

quoted

quoted

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function, __crb_request_locality() is called
prior to crb_cmd_ready(), but if one of the consecutive function
fails the flow bails out instead of trying to relinquish locality.
This patch adds goto jump to __crb_relinquish_locality() on the
error path.

Fixes: 888d867df441 (tpm: cmd_ready command can be issued only
after granting
locality)
Signed-off-by: Tomas Winkler <redacted>
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c

b/drivers/char/tpm/tpm_crb.c index 7f78482cd157..34fbc6cb097b
100644

--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct acpi_device

*device, struct crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf-

quoted

control_address,

 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to access @@ -520,7
+522,7

@@

quoted

static int crb_map_io(struct acpi_device *device, struct
crb_priv *priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct acpi_device

*device, struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error path.

Can you please elaborate why, isn't the centralized exiting of
functions preferred kernel coding style?
https://www.kernel.org/doc/html/v4.11/process/coding-style.html#cent
ra
lized-ex
iting-of-functions

You exit only from one location (not multiple) and not from a nested
context. Here you just add more complexity by doing this.

Where is the complexity ? I see it as a standard way of undoing on exit.
Tomas

Jarkko, can you please respond.
Thanks
Tomas

I was away for Mon-Wed last week and did not work on TPM for Thu-Fri.

My earlier comment was incorrect as there are two locations to exit (not
sure how I managed to overlook the patch that way).

Thus, I have only two very  minor requets:

* Remove the extra newline (the last line addition in the patch).
* Use just label named out as we have only one exception handler.

I'll move on to testing, and if it it passes, I can do those updates
myself.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Apr 24, 2018 at 07:03:28PM +0300, Jarkko Sakkinen wrote:

On Fri, Apr 20, 2018 at 01:19:12PM +0000, Winkler, Tomas wrote:

quoted

quoted

quoted

On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:

quoted

quoted

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function, __crb_request_locality() is called
prior to crb_cmd_ready(), but if one of the consecutive function
fails the flow bails out instead of trying to relinquish locality.
This patch adds goto jump to __crb_relinquish_locality() on the
error path.

Fixes: 888d867df441 (tpm: cmd_ready command can be issued only
after granting
locality)
Signed-off-by: Tomas Winkler <redacted>
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c

b/drivers/char/tpm/tpm_crb.c index 7f78482cd157..34fbc6cb097b
100644

--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct acpi_device

*device, struct crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf-

quoted

control_address,

 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to access @@ -520,7
+522,7

@@

quoted

static int crb_map_io(struct acpi_device *device, struct
crb_priv *priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct acpi_device

*device, struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error path.

Can you please elaborate why, isn't the centralized exiting of
functions preferred kernel coding style?
https://www.kernel.org/doc/html/v4.11/process/coding-style.html#cent
ra
lized-ex
iting-of-functions

You exit only from one location (not multiple) and not from a nested
context. Here you just add more complexity by doing this.

Where is the complexity ? I see it as a standard way of undoing on exit.
Tomas

Jarkko, can you please respond.
Thanks
Tomas

I was away for Mon-Wed last week and did not work on TPM for Thu-Fri.

My earlier comment was incorrect as there are two locations to exit (not
sure how I managed to overlook the patch that way).

Thus, I have only two very  minor requets:

* Remove the extra newline (the last line addition in the patch).
* Use just label named out as we have only one exception handler.

I'll move on to testing, and if it it passes, I can do those updates
myself.

/Jarkko

Tested-by: Jarkko Sakkinen <redacted>

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.

On Fri, Apr 20, 2018 at 01:19:12PM +0000, Winkler, Tomas wrote:

quoted

quoted

quoted

On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:

quoted

quoted

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function, __crb_request_locality() is called
prior to crb_cmd_ready(), but if one of the consecutive
function fails the flow bails out instead of trying to relinquish

locality.

quoted

quoted

quoted

quoted

quoted

quoted

This patch adds goto jump to __crb_relinquish_locality() on
the error path.

Fixes: 888d867df441 (tpm: cmd_ready command can be issued
only after granting
locality)
Signed-off-by: Tomas Winkler <redacted>
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c

b/drivers/char/tpm/tpm_crb.c index
7f78482cd157..34fbc6cb097b
100644

--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct

acpi_device *device, struct crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf-

quoted

control_address,

 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to access @@ -520,7
+522,7

@@

quoted

static int crb_map_io(struct acpi_device *device, struct
crb_priv *priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct acpi_device

*device, struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error path.

Can you please elaborate why, isn't the centralized exiting of
functions preferred kernel coding style?
https://www.kernel.org/doc/html/v4.11/process/coding-style.html#
cent
ra
lized-ex
iting-of-functions

You exit only from one location (not multiple) and not from a
nested context. Here you just add more complexity by doing this.

Where is the complexity ? I see it as a standard way of undoing on exit.
Tomas

Jarkko, can you please respond.
Thanks
Tomas

I was away for Mon-Wed last week and did not work on TPM for Thu-Fri.

My earlier comment was incorrect as there are two locations to exit (not
sure how I managed to overlook the patch that way).

Thus, I have only two very  minor requets:

* Remove the extra newline (the last line addition in the patch).

Okay

* Use just label named out as we have only one exception handler.

Cannot do that, as the bail out is prior to cmd_ready request so there is no need for go_idle which is under out label.

I'll move on to testing, and if it it passes, I can do those updates myself.

Thanks,  I prefer to resend myself. 

Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Apr 24, 2018 at 07:03:28PM +0300, Jarkko Sakkinen wrote:

quoted

On Fri, Apr 20, 2018 at 01:19:12PM +0000, Winkler, Tomas wrote:

quoted

quoted

quoted

On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:

quoted

quoted

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function, __crb_request_locality() is
called prior to crb_cmd_ready(), but if one of the
consecutive function fails the flow bails out instead of trying to

relinquish locality.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

This patch adds goto jump to __crb_relinquish_locality()
on the error path.

Fixes: 888d867df441 (tpm: cmd_ready command can be issued
only after granting
locality)
Signed-off-by: Tomas Winkler <redacted>
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c

b/drivers/char/tpm/tpm_crb.c index
7f78482cd157..34fbc6cb097b
100644

--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct

acpi_device *device, struct crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf-

quoted

control_address,

 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to access @@
-520,7
+522,7

@@

quoted

static int crb_map_io(struct acpi_device *device, struct
crb_priv *priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct

acpi_device *device, struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error path.

Can you please elaborate why, isn't the centralized exiting of
functions preferred kernel coding style?
https://www.kernel.org/doc/html/v4.11/process/coding-style.htm
l#cent
ra
lized-ex
iting-of-functions

You exit only from one location (not multiple) and not from a
nested context. Here you just add more complexity by doing this.

Where is the complexity ? I see it as a standard way of undoing on exit.
Tomas

Jarkko, can you please respond.
Thanks
Tomas

I was away for Mon-Wed last week and did not work on TPM for Thu-Fri.

My earlier comment was incorrect as there are two locations to exit
(not sure how I managed to overlook the patch that way).

Thus, I have only two very  minor requets:

* Remove the extra newline (the last line addition in the patch).
* Use just label named out as we have only one exception handler.

I'll move on to testing, and if it it passes, I can do those updates
myself.

/Jarkko

Tested-by: Jarkko Sakkinen <redacted>

Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Apr 24, 2018 at 08:04:01PM +0000, Winkler, Tomas wrote:

quoted

Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.

On Fri, Apr 20, 2018 at 01:19:12PM +0000, Winkler, Tomas wrote:

quoted

quoted

quoted

On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:

quoted

quoted

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function, __crb_request_locality() is called
prior to crb_cmd_ready(), but if one of the consecutive
function fails the flow bails out instead of trying to relinquish

locality.

quoted

quoted

quoted

quoted

quoted

quoted

This patch adds goto jump to __crb_relinquish_locality() on
the error path.

Fixes: 888d867df441 (tpm: cmd_ready command can be issued
only after granting
locality)
Signed-off-by: Tomas Winkler <redacted>
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c

b/drivers/char/tpm/tpm_crb.c index
7f78482cd157..34fbc6cb097b
100644

--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct

acpi_device *device, struct crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf-

quoted

control_address,

 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to access @@ -520,7
+522,7

@@

quoted

static int crb_map_io(struct acpi_device *device, struct
crb_priv *priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct acpi_device

*device, struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error path.

Can you please elaborate why, isn't the centralized exiting of
functions preferred kernel coding style?
https://www.kernel.org/doc/html/v4.11/process/coding-style.html#
cent
ra
lized-ex
iting-of-functions

You exit only from one location (not multiple) and not from a
nested context. Here you just add more complexity by doing this.

Where is the complexity ? I see it as a standard way of undoing on exit.
Tomas

Jarkko, can you please respond.
Thanks
Tomas

I was away for Mon-Wed last week and did not work on TPM for Thu-Fri.

My earlier comment was incorrect as there are two locations to exit (not
sure how I managed to overlook the patch that way).

Thus, I have only two very  minor requets:

* Remove the extra newline (the last line addition in the patch).

Okay

quoted

* Use just label named out as we have only one exception handler.

Cannot do that, as the bail out is prior to cmd_ready request so there is no need for go_idle which is under out label.

quoted

I'll move on to testing, and if it it passes, I can do those updates myself.

Thanks,  I prefer to resend myself. 

Tomas

Add my tested-by as it is cosmectic change, thanks.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Apr 24, 2018 at 08:04:01PM +0000, Winkler, Tomas wrote:

quoted

quoted

Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.

On Fri, Apr 20, 2018 at 01:19:12PM +0000, Winkler, Tomas wrote:

quoted

quoted

quoted

On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:

quoted

quoted

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function, __crb_request_locality() is
called prior to crb_cmd_ready(), but if one of the
consecutive function fails the flow bails out instead of
trying to relinquish

locality.

quoted

quoted

quoted

quoted

quoted

quoted

This patch adds goto jump to __crb_relinquish_locality()
on the error path.

Fixes: 888d867df441 (tpm: cmd_ready command can be
issued only after granting
locality)
Signed-off-by: Tomas Winkler <redacted>
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c

b/drivers/char/tpm/tpm_crb.c index
7f78482cd157..34fbc6cb097b
100644

--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct

acpi_device *device, struct crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf-

quoted

control_address,

 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to access @@
-520,7
+522,7

@@

quoted

static int crb_map_io(struct acpi_device *device, struct
crb_priv *priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct

acpi_device *device, struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error path.

Can you please elaborate why, isn't the centralized exiting
of functions preferred kernel coding style?
https://www.kernel.org/doc/html/v4.11/process/coding-style.h
tml#
cent
ra
lized-ex
iting-of-functions

You exit only from one location (not multiple) and not from a
nested context. Here you just add more complexity by doing this.

Where is the complexity ? I see it as a standard way of undoing on

exit.

quoted

quoted

quoted

quoted

Tomas

Jarkko, can you please respond.
Thanks
Tomas

I was away for Mon-Wed last week and did not work on TPM for Thu-Fri.

My earlier comment was incorrect as there are two locations to exit
(not sure how I managed to overlook the patch that way).

Thus, I have only two very  minor requets:

* Remove the extra newline (the last line addition in the patch).

Okay

quoted

* Use just label named out as we have only one exception handler.

Cannot do that, as the bail out is prior to cmd_ready request so there is no

need for go_idle which is under out label.

quoted

quoted

I'll move on to testing, and if it it passes, I can do those updates myself.

Thanks,  I prefer to resend myself.

Tomas

Add my tested-by as it is cosmectic change, thanks.

What change exactly? I had impression you've accepted the patch as is?
Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, May 02, 2018 at 01:35:47PM +0000, Winkler, Tomas wrote:

quoted

On Tue, Apr 24, 2018 at 08:04:01PM +0000, Winkler, Tomas wrote:

quoted

quoted

Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.

On Fri, Apr 20, 2018 at 01:19:12PM +0000, Winkler, Tomas wrote:

quoted

quoted

quoted

On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:

quoted

quoted

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function, __crb_request_locality() is
called prior to crb_cmd_ready(), but if one of the
consecutive function fails the flow bails out instead of
trying to relinquish

locality.

quoted

quoted

quoted

quoted

quoted

quoted

This patch adds goto jump to __crb_relinquish_locality()
on the error path.

Fixes: 888d867df441 (tpm: cmd_ready command can be
issued only after granting
locality)
Signed-off-by: Tomas Winkler <redacted>
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c

b/drivers/char/tpm/tpm_crb.c index
7f78482cd157..34fbc6cb097b
100644

--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct

acpi_device *device, struct crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res, buf-

quoted

control_address,

 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to access @@
-520,7
+522,7

@@

quoted

static int crb_map_io(struct acpi_device *device, struct
crb_priv *priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct

acpi_device *device, struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error path.

Can you please elaborate why, isn't the centralized exiting
of functions preferred kernel coding style?
https://www.kernel.org/doc/html/v4.11/process/coding-style.h
tml#
cent
ra
lized-ex
iting-of-functions

You exit only from one location (not multiple) and not from a
nested context. Here you just add more complexity by doing this.

Where is the complexity ? I see it as a standard way of undoing on

exit.

quoted

quoted

quoted

quoted

Tomas

Jarkko, can you please respond.
Thanks
Tomas

I was away for Mon-Wed last week and did not work on TPM for Thu-Fri.

My earlier comment was incorrect as there are two locations to exit
(not sure how I managed to overlook the patch that way).

Thus, I have only two very  minor requets:

* Remove the extra newline (the last line addition in the patch).

Okay

quoted

* Use just label named out as we have only one exception handler.

Cannot do that, as the bail out is prior to cmd_ready request so there is no

need for go_idle which is under out label.

quoted

quoted

I'll move on to testing, and if it it passes, I can do those updates myself.

Thanks,  I prefer to resend myself.

Tomas

Add my tested-by as it is cosmectic change, thanks.

What change exactly? I had impression you've accepted the patch as is?
Thanks
Tomas

Hmm... maybe there is some misunderstandig but I thought you were
going to make the updates above yourself and send a revised patch.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

quoted

quoted

On Tue, Apr 24, 2018 at 08:04:01PM +0000, Winkler, Tomas wrote:

quoted

quoted

Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.

On Fri, Apr 20, 2018 at 01:19:12PM +0000, Winkler, Tomas wrote:

quoted

quoted

quoted

On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:

quoted

quoted

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function, __crb_request_locality()
is called prior to crb_cmd_ready(), but if one of
the consecutive function fails the flow bails out
instead of trying to relinquish

locality.

quoted

quoted

quoted

quoted

quoted

quoted

This patch adds goto jump to
__crb_relinquish_locality() on the error path.

Fixes: 888d867df441 (tpm: cmd_ready command can be
issued only after granting
locality)
Signed-off-by: Tomas Winkler
[off-list ref]
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c

b/drivers/char/tpm/tpm_crb.c index
7f78482cd157..34fbc6cb097b
100644

--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct

acpi_device *device, struct crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res,
buf-

quoted

control_address,

 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to access

@@

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

-520,7
+522,7

@@

quoted

static int crb_map_io(struct acpi_device *device,
struct crb_priv *priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t-

ctrl_cmd_pa_high);

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

 	pa_low  =
ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct

acpi_device *device, struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error path.

Can you please elaborate why, isn't the centralized
exiting of functions preferred kernel coding style?
https://www.kernel.org/doc/html/v4.11/process/coding-sty
le.h
tml#
cent
ra
lized-ex
iting-of-functions

You exit only from one location (not multiple) and not
from a nested context. Here you just add more complexity by

doing this.

quoted

quoted

quoted

quoted

quoted

quoted

Where is the complexity ? I see it as a standard way of
undoing on

exit.

quoted

quoted

quoted

quoted

Tomas

Jarkko, can you please respond.
Thanks
Tomas

I was away for Mon-Wed last week and did not work on TPM for Thu-

Fri.

quoted

quoted

quoted

quoted

My earlier comment was incorrect as there are two locations to
exit (not sure how I managed to overlook the patch that way).

Thus, I have only two very  minor requets:

* Remove the extra newline (the last line addition in the patch).

Okay

quoted

* Use just label named out as we have only one exception handler.

Cannot do that, as the bail out is prior to cmd_ready request so
there is no

need for go_idle which is under out label.

quoted

quoted

I'll move on to testing, and if it it passes, I can do those updates

myself.

quoted

quoted

quoted

Thanks,  I prefer to resend myself.

Tomas

Add my tested-by as it is cosmectic change, thanks.

What change exactly? I had impression you've accepted the patch as is?
Thanks
Tomas

Hmm... maybe there is some misunderstandig but I thought you were going
to make the updates above yourself and send a revised patch.

Probably,  if I remember there were to mails going in asynchronous, I've received your tested-by, the second I've sent this answer,
so I thought you came to the conclusion that there is nothing to change in the patch yourself.
Frankly I've checked the patch and there are no even extra new lines in my version unless it has scrambled on the way.

Thanks
Tomas



--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, May 03, 2018 at 06:42:26AM +0000, Winkler, Tomas wrote:

quoted

quoted

quoted

On Tue, Apr 24, 2018 at 08:04:01PM +0000, Winkler, Tomas wrote:

quoted

quoted

Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.

On Fri, Apr 20, 2018 at 01:19:12PM +0000, Winkler, Tomas wrote:

quoted

quoted

quoted

On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:

quoted

quoted

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function, __crb_request_locality()
is called prior to crb_cmd_ready(), but if one of
the consecutive function fails the flow bails out
instead of trying to relinquish

locality.

quoted

quoted

quoted

quoted

quoted

quoted

This patch adds goto jump to
__crb_relinquish_locality() on the error path.

Fixes: 888d867df441 (tpm: cmd_ready command can be
issued only after granting
locality)
Signed-off-by: Tomas Winkler
[off-list ref]
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c

b/drivers/char/tpm/tpm_crb.c index
7f78482cd157..34fbc6cb097b
100644

--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int crb_map_io(struct

acpi_device *device, struct crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res,
buf-

quoted

control_address,

 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to access

@@

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

-520,7
+522,7

@@

quoted

static int crb_map_io(struct acpi_device *device,
struct crb_priv *priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t-

ctrl_cmd_pa_high);

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

 	pa_low  =
ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct

acpi_device *device, struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error path.

Can you please elaborate why, isn't the centralized
exiting of functions preferred kernel coding style?
https://www.kernel.org/doc/html/v4.11/process/coding-sty
le.h
tml#
cent
ra
lized-ex
iting-of-functions

You exit only from one location (not multiple) and not
from a nested context. Here you just add more complexity by

doing this.

quoted

quoted

quoted

quoted

quoted

quoted

Where is the complexity ? I see it as a standard way of
undoing on

exit.

quoted

quoted

quoted

quoted

Tomas

Jarkko, can you please respond.
Thanks
Tomas

I was away for Mon-Wed last week and did not work on TPM for Thu-

Fri.

quoted

quoted

quoted

quoted

My earlier comment was incorrect as there are two locations to
exit (not sure how I managed to overlook the patch that way).

Thus, I have only two very  minor requets:

* Remove the extra newline (the last line addition in the patch).

Okay

quoted

* Use just label named out as we have only one exception handler.

Cannot do that, as the bail out is prior to cmd_ready request so
there is no

need for go_idle which is under out label.

quoted

quoted

I'll move on to testing, and if it it passes, I can do those updates

myself.

quoted

quoted

quoted

Thanks,  I prefer to resend myself.

Tomas

Add my tested-by as it is cosmectic change, thanks.

What change exactly? I had impression you've accepted the patch as is?
Thanks
Tomas

Hmm... maybe there is some misunderstandig but I thought you were going
to make the updates above yourself and send a revised patch.

Probably,  if I remember there were to mails going in asynchronous, I've received your tested-by, the second I've sent this answer,
so I thought you came to the conclusion that there is nothing to change in the patch yourself.
Frankly I've checked the patch and there are no even extra new lines in my version unless it has scrambled on the way.

Thanks
Tomas

The only thing was the label name. That is why I asked if I can
change it by myself instead of you having to send a follow up
patch. Did you send a new one? Did not find it from patchwork.

/Jarkko

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, May 03, 2018 at 06:42:26AM +0000, Winkler, Tomas wrote:

quoted

quoted

quoted

quoted

On Tue, Apr 24, 2018 at 08:04:01PM +0000, Winkler, Tomas wrote:

quoted

quoted

Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error

path.

quoted

quoted

quoted

quoted

quoted

quoted

On Fri, Apr 20, 2018 at 01:19:12PM +0000, Winkler, Tomas wrote:

quoted

quoted

quoted

On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:

quoted

quoted

On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:

quoted

In crb_map_io() function,
__crb_request_locality() is called prior to
crb_cmd_ready(), but if one of the consecutive
function fails the flow bails out instead of
trying to relinquish

locality.

quoted

quoted

quoted

quoted

quoted

quoted

This patch adds goto jump to
__crb_relinquish_locality() on the error path.

Fixes: 888d867df441 (tpm: cmd_ready command can
be issued only after granting
locality)
Signed-off-by: Tomas Winkler
[off-list ref]
---
 drivers/char/tpm/tpm_crb.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c

b/drivers/char/tpm/tpm_crb.c index
7f78482cd157..34fbc6cb097b
100644

--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c

@@ -511,8 +511,10 @@ static int

crb_map_io(struct acpi_device *device, struct
crb_priv *priv,

 	priv->regs_t = crb_map_res(dev, priv, &io_res,
buf-

quoted

control_address,

 				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t))
-		return PTR_ERR(priv->regs_t);
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_relinquish_locality;
+	}

 	/*
 	 * PTT HW bug w/a: wake up the device to
access

@@

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

-520,7
+522,7

@@

quoted

static int crb_map_io(struct acpi_device
*device, struct crb_priv *priv,
 	 */
 	ret = crb_cmd_ready(dev, priv);
 	if (ret)
-		return ret;
+		goto out_relinquish_locality;

 	pa_high = ioread32(&priv->regs_t-

ctrl_cmd_pa_high);

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

 	pa_low  =
ioread32(&priv->regs_t->ctrl_cmd_pa_low);

@@ -565,6 +567,8 @@ static int crb_map_io(struct

acpi_device *device, struct crb_priv *priv,

 	crb_go_idle(dev, priv);

+out_relinquish_locality:
+
 	__crb_relinquish_locality(dev, priv, 0);

 	return ret;

Thanks, please just call it before returning in the error

path.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Can you please elaborate why, isn't the centralized
exiting of functions preferred kernel coding style?
https://www.kernel.org/doc/html/v4.11/process/coding
-sty
le.h
tml#
cent
ra
lized-ex
iting-of-functions

You exit only from one location (not multiple) and not
from a nested context. Here you just add more
complexity by

doing this.

quoted

quoted

quoted

quoted

quoted

quoted

Where is the complexity ? I see it as a standard way of
undoing on

exit.

quoted

quoted

quoted

quoted

Tomas

Jarkko, can you please respond.
Thanks
Tomas

I was away for Mon-Wed last week and did not work on TPM for
Thu-

Fri.

quoted

quoted

quoted

quoted

My earlier comment was incorrect as there are two locations
to exit (not sure how I managed to overlook the patch that way).

Thus, I have only two very  minor requets:

* Remove the extra newline (the last line addition in the patch).

Okay

quoted

* Use just label named out as we have only one exception

handler.

quoted

quoted

quoted

quoted

quoted

Cannot do that, as the bail out is prior to cmd_ready request
so there is no

need for go_idle which is under out label.

quoted

quoted

I'll move on to testing, and if it it passes, I can do those
updates

myself.

quoted

quoted

quoted

Thanks,  I prefer to resend myself.

Tomas

Add my tested-by as it is cosmectic change, thanks.

What change exactly? I had impression you've accepted the patch as is?
Thanks
Tomas

Hmm... maybe there is some misunderstandig but I thought you were
going to make the updates above yourself and send a revised patch.

Probably,  if I remember there were to mails going in asynchronous,
I've received your tested-by, the second I've sent this answer, so I thought

you came to the conclusion that there is nothing to change in the patch
yourself.

quoted

Frankly I've checked the patch and there are no even extra new lines in my

version unless it has scrambled on the way.

quoted

Thanks
Tomas

The only thing was the label name. That is why I asked if I can change it by
myself instead of you having to send a follow up patch. Did you send a new
one? Did not find it from patchwork.

There is already one 'out' label in that function, what would you prefer for this new label name?
Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, May 05, 2018 at 09:07:21PM +0000, Winkler, Tomas wrote:

There is already one 'out' label in that function, what would you prefer for this new label name?
Thanks
Tomas

Aah, of course. My bad that I did not notice it but please just point
me if I miss something obvious like that :-) I'll apply the patch
today..

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, May 05, 2018 at 09:07:21PM +0000, Winkler, Tomas wrote:

quoted

There is already one 'out' label in that function, what would you prefer for

this new label name?

quoted

Thanks
Tomas

Aah, of course. My bad that I did not notice it but please just point me if I
miss something obvious like that :-) I'll apply the patch today..

Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, May 09, 2018 at 09:49:49PM +0000, Winkler, Tomas wrote:

quoted

On Sat, May 05, 2018 at 09:07:21PM +0000, Winkler, Tomas wrote:

quoted

There is already one 'out' label in that function, what would you prefer for

this new label name?

quoted

Thanks
Tomas

Aah, of course. My bad that I did not notice it but please just point me if I
miss something obvious like that :-) I'll apply the patch today..

Thanks
Tomas

It's also in next now!

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html