* [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() @ 2018-12-31 18:27 James Bottomley 2019-01-03 12:59 ` Jarkko Sakkinen 0 siblings, 1 reply; 9+ messages in thread From: James Bottomley @ 2018-12-31 18:27 UTC (permalink / raw) To: Jarkko Sakkinen, Tomas Winkler; +Cc: linux-integrity Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from runtime_pm" we have been returning success from tpm_try_transmit() even if an error occurred. The reason is that the introduction of rc = tpm_go_idle() at the end of processing overwrites the value of rc if it contains an error code (mostly with success). Fix this by writing the return to a new variable rc1 instead. Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from runtime_pm" Cc: stable@vger.kernel.org Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> --- Note: the goto out looks fishy as well. The only go_idle implementor is tpm_crb and that can return a timeout as -ETIME, so it looks like it would then loop forever diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 129f640424b7..ac7ebab6140c 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, unsigned int flags) { struct tpm_output_header *header = (void *)buf; - int rc; + int rc, rc1; ssize_t len = 0; u32 count, ordinal; unsigned long stop; @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc); out: - rc = tpm_go_idle(chip, flags); - if (rc) + rc1 = tpm_go_idle(chip, flags); + if (rc1) goto out; if (need_locality) ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() 2018-12-31 18:27 [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() James Bottomley @ 2019-01-03 12:59 ` Jarkko Sakkinen 2019-01-03 13:37 ` Winkler, Tomas 2019-01-03 15:23 ` James Bottomley 0 siblings, 2 replies; 9+ messages in thread From: Jarkko Sakkinen @ 2019-01-03 12:59 UTC (permalink / raw) To: James Bottomley; +Cc: Tomas Winkler, linux-integrity On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote: > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from > runtime_pm" we have been returning success from tpm_try_transmit() > even if an error occurred. The reason is that the introduction of rc > = tpm_go_idle() at the end of processing overwrites the value of rc if > it contains an error code (mostly with success). Fix this by writing > the return to a new variable rc1 instead. > > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from runtime_pm" > Cc: stable@vger.kernel.org > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > --- > > Note: the goto out looks fishy as well. The only go_idle implementor > is tpm_crb and that can return a timeout as -ETIME, so it looks like it > would then loop forever > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index 129f640424b7..ac7ebab6140c 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, > unsigned int flags) > { > struct tpm_output_header *header = (void *)buf; > - int rc; > + int rc, rc1; > ssize_t len = 0; > u32 count, ordinal; > unsigned long stop; > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, > dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc); > > out: > - rc = tpm_go_idle(chip, flags); > - if (rc) > + rc1 = tpm_go_idle(chip, flags); > + if (rc1) > goto out; > > if (need_locality) Thanks James and sorry for latency (holiday season). Just a small suggestion. I would just: if (tpm_go_idle(chip, flags)) goto out; What do you think? /Jarkko ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() 2019-01-03 12:59 ` Jarkko Sakkinen @ 2019-01-03 13:37 ` Winkler, Tomas 2019-01-03 15:03 ` Jarkko Sakkinen 2019-01-03 15:23 ` James Bottomley 1 sibling, 1 reply; 9+ messages in thread From: Winkler, Tomas @ 2019-01-03 13:37 UTC (permalink / raw) To: Jarkko Sakkinen, James Bottomley; +Cc: linux-integrity > > On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote: > > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from > > runtime_pm" we have been returning success from tpm_try_transmit() > > even if an error occurred. The reason is that the introduction of rc > > = tpm_go_idle() at the end of processing overwrites the value of rc if > > it contains an error code (mostly with success). Fix this by writing > > the return to a new variable rc1 instead. > > > > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from runtime_pm" > > Cc: stable@vger.kernel.org > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > --- > > > > Note: the goto out looks fishy as well. The only go_idle implementor > > is tpm_crb and that can return a timeout as -ETIME, so it looks like > > it would then loop forever > > > > diff --git a/drivers/char/tpm/tpm-interface.c > > b/drivers/char/tpm/tpm-interface.c > > index 129f640424b7..ac7ebab6140c 100644 > > --- a/drivers/char/tpm/tpm-interface.c > > +++ b/drivers/char/tpm/tpm-interface.c > > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip > *chip, > > unsigned int flags) > > { > > struct tpm_output_header *header = (void *)buf; > > - int rc; > > + int rc, rc1; > > ssize_t len = 0; > > u32 count, ordinal; > > unsigned long stop; > > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip > *chip, > > dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc); > > > > out: > > - rc = tpm_go_idle(chip, flags); > > - if (rc) > > + rc1 = tpm_go_idle(chip, flags); > > + if (rc1) > > goto out; > > > > if (need_locality) > > Thanks James and sorry for latency (holiday season). Just a small suggestion. I > would just: > > if (tpm_go_idle(chip, flags)) > goto out; > > What do you think? This is wrong as well because of the jump to 'out'. I've introduced this bug in version 4 of the patch, when the wrappers were added, before the jump to 'out' was correct. If it's okay with you I will post my version of the fix. Thanks Tomas ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() 2019-01-03 13:37 ` Winkler, Tomas @ 2019-01-03 15:03 ` Jarkko Sakkinen 0 siblings, 0 replies; 9+ messages in thread From: Jarkko Sakkinen @ 2019-01-03 15:03 UTC (permalink / raw) To: Winkler, Tomas; +Cc: James Bottomley, linux-integrity On Thu, Jan 03, 2019 at 01:37:09PM +0000, Winkler, Tomas wrote: > > > > On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote: > > > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from > > > runtime_pm" we have been returning success from tpm_try_transmit() > > > even if an error occurred. The reason is that the introduction of rc > > > = tpm_go_idle() at the end of processing overwrites the value of rc if > > > it contains an error code (mostly with success). Fix this by writing > > > the return to a new variable rc1 instead. > > > > > > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from runtime_pm" > > > Cc: stable@vger.kernel.org > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > --- > > > > > > Note: the goto out looks fishy as well. The only go_idle implementor > > > is tpm_crb and that can return a timeout as -ETIME, so it looks like > > > it would then loop forever > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c > > > b/drivers/char/tpm/tpm-interface.c > > > index 129f640424b7..ac7ebab6140c 100644 > > > --- a/drivers/char/tpm/tpm-interface.c > > > +++ b/drivers/char/tpm/tpm-interface.c > > > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip > > *chip, > > > unsigned int flags) > > > { > > > struct tpm_output_header *header = (void *)buf; > > > - int rc; > > > + int rc, rc1; > > > ssize_t len = 0; > > > u32 count, ordinal; > > > unsigned long stop; > > > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip > > *chip, > > > dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc); > > > > > > out: > > > - rc = tpm_go_idle(chip, flags); > > > - if (rc) > > > + rc1 = tpm_go_idle(chip, flags); > > > + if (rc1) > > > goto out; > > > > > > if (need_locality) > > > > Thanks James and sorry for latency (holiday season). Just a small suggestion. I > > would just: > > > > if (tpm_go_idle(chip, flags)) > > goto out; > > > > What do you think? > > > This is wrong as well because of the jump to 'out'. > I've introduced this bug in version 4 of the patch, when the wrappers were added, before the jump to 'out' was correct. > If it's okay with you I will post my version of the fix. > Thanks > Tomas Works for me. /Jarkko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() 2019-01-03 12:59 ` Jarkko Sakkinen 2019-01-03 13:37 ` Winkler, Tomas @ 2019-01-03 15:23 ` James Bottomley 2019-01-03 15:34 ` Winkler, Tomas 1 sibling, 1 reply; 9+ messages in thread From: James Bottomley @ 2019-01-03 15:23 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: Tomas Winkler, linux-integrity On Thu, 2019-01-03 at 14:59 +0200, Jarkko Sakkinen wrote: > On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote: > > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from > > runtime_pm" we have been returning success from tpm_try_transmit() > > even if an error occurred. The reason is that the introduction of > > rc > > = tpm_go_idle() at the end of processing overwrites the value of rc > > if > > it contains an error code (mostly with success). Fix this by > > writing > > the return to a new variable rc1 instead. > > > > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from > > runtime_pm" > > Cc: stable@vger.kernel.org > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c > > om> > > > > --- > > > > Note: the goto out looks fishy as well. The only go_idle > > implementor > > is tpm_crb and that can return a timeout as -ETIME, so it looks > > like it > > would then loop forever > > > > diff --git a/drivers/char/tpm/tpm-interface.c > > b/drivers/char/tpm/tpm-interface.c > > index 129f640424b7..ac7ebab6140c 100644 > > --- a/drivers/char/tpm/tpm-interface.c > > +++ b/drivers/char/tpm/tpm-interface.c > > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip > > *chip, > > unsigned int flags) > > { > > struct tpm_output_header *header = (void *)buf; > > - int rc; > > + int rc, rc1; > > ssize_t len = 0; > > u32 count, ordinal; > > unsigned long stop; > > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip > > *chip, > > dev_err(&chip->dev, "tpm2_commit_space: error > > %d\n", rc); > > > > out: > > - rc = tpm_go_idle(chip, flags); > > - if (rc) > > + rc1 = tpm_go_idle(chip, flags); > > + if (rc1) > > goto out; > > > > if (need_locality) > > Thanks James and sorry for latency (holiday season). Just a small > suggestion. I would just: > > if (tpm_go_idle(chip, flags)) > goto out; > > What do you think? That it doesn't solve the loop forever with no warning problem. If anything, I think the correct thing is probably rc1 = tpm_go_idle(chip, flags); if (rc1) dev_err(&chip->dev, "go idle failed with %d\n", rc1); so we log the problem and move on. If it is a timeout, it will likely show up on the next TPM operation. Since this is the only caller of tpm_go_idle(), I think all looping should be done inside that function, but we should probably wait for Tomas to comment since he wrote it. James ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() 2019-01-03 15:23 ` James Bottomley @ 2019-01-03 15:34 ` Winkler, Tomas 2019-01-03 16:17 ` James Bottomley 2019-01-10 17:16 ` Jarkko Sakkinen 0 siblings, 2 replies; 9+ messages in thread From: Winkler, Tomas @ 2019-01-03 15:34 UTC (permalink / raw) To: James Bottomley, Jarkko Sakkinen; +Cc: linux-integrity > -----Original Message----- > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] > Sent: Thursday, January 03, 2019 17:24 > To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > Cc: Winkler, Tomas <tomas.winkler@intel.com>; linux- > integrity@vger.kernel.org > Subject: Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() > > On Thu, 2019-01-03 at 14:59 +0200, Jarkko Sakkinen wrote: > > On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote: > > > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from > > > runtime_pm" we have been returning success from tpm_try_transmit() > > > even if an error occurred. The reason is that the introduction of > > > rc = tpm_go_idle() at the end of processing overwrites the value of > > > rc if it contains an error code (mostly with success). Fix this by > > > writing the return to a new variable rc1 instead. > > > > > > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from > > > runtime_pm" > > > Cc: stable@vger.kernel.org > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c > > > om> > > > > > > --- > > > > > > Note: the goto out looks fishy as well. The only go_idle > > > implementor is tpm_crb and that can return a timeout as -ETIME, so > > > it looks like it would then loop forever > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c > > > b/drivers/char/tpm/tpm-interface.c > > > index 129f640424b7..ac7ebab6140c 100644 > > > --- a/drivers/char/tpm/tpm-interface.c > > > +++ b/drivers/char/tpm/tpm-interface.c > > > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip > > > *chip, > > > unsigned int flags) > > > { > > > struct tpm_output_header *header = (void *)buf; > > > - int rc; > > > + int rc, rc1; > > > ssize_t len = 0; > > > u32 count, ordinal; > > > unsigned long stop; > > > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip > > > *chip, > > > dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc); > > > > > > out: > > > - rc = tpm_go_idle(chip, flags); > > > - if (rc) > > > + rc1 = tpm_go_idle(chip, flags); > > > + if (rc1) > > > goto out; > > > > > > if (need_locality) > > > > Thanks James and sorry for latency (holiday season). Just a small > > suggestion. I would just: > > > > if (tpm_go_idle(chip, flags)) > > goto out; > > > > What do you think? > > That it doesn't solve the loop forever with no warning problem. If anything, I > think the correct thing is probably > > rc1 = tpm_go_idle(chip, flags); > if (rc1) > dev_err(&chip->dev, "go idle failed with %d\n", rc1); > > so we log the problem and move on. If it is a timeout, it will likely show up on > the next TPM operation. Since this is the only caller of tpm_go_idle(), I think all > looping should be done inside that function, but we should probably wait for > Tomas to comment since he wrote it. > We've already fixed it, I forgot myself , we were drinking too much :) https://patchwork.kernel.org/patch/10643565/ Not sure why it was dropped. Thanks Tomas ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() 2019-01-03 15:34 ` Winkler, Tomas @ 2019-01-03 16:17 ` James Bottomley 2019-01-10 17:16 ` Jarkko Sakkinen 2019-01-10 17:16 ` Jarkko Sakkinen 1 sibling, 1 reply; 9+ messages in thread From: James Bottomley @ 2019-01-03 16:17 UTC (permalink / raw) To: Winkler, Tomas, Jarkko Sakkinen; +Cc: linux-integrity On Thu, 2019-01-03 at 15:34 +0000, Winkler, Tomas wrote: > > -----Original Message----- > > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com > > ] > > Sent: Thursday, January 03, 2019 17:24 > > To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Cc: Winkler, Tomas <tomas.winkler@intel.com>; linux- > > integrity@vger.kernel.org > > Subject: Re: [PATCH] tpm: fix incorrect success returns from > > tpm_try_transmit() > > > > On Thu, 2019-01-03 at 14:59 +0200, Jarkko Sakkinen wrote: > > > On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote: > > > > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from > > > > runtime_pm" we have been returning success from > > > > tpm_try_transmit() even if an error occurred. The reason is > > > > that the introduction of rc = tpm_go_idle() at the end of > > > > processing overwrites the value of rc if it contains an error > > > > code (mostly with success). Fix this by writing the return to > > > > a new variable rc1 instead. > > > > > > > > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from > > > > runtime_pm" > > > > Cc: stable@vger.kernel.org > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnersh > > > > ip.c > > > > om> > > > > > > > > --- > > > > > > > > Note: the goto out looks fishy as well. The only go_idle > > > > implementor is tpm_crb and that can return a timeout as -ETIME, > > > > so it looks like it would then loop forever > > > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c > > > > b/drivers/char/tpm/tpm-interface.c > > > > index 129f640424b7..ac7ebab6140c 100644 > > > > --- a/drivers/char/tpm/tpm-interface.c > > > > +++ b/drivers/char/tpm/tpm-interface.c > > > > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct > > > > tpm_chip > > > > *chip, > > > > unsigned int flags) > > > > { > > > > struct tpm_output_header *header = (void *)buf; > > > > - int rc; > > > > + int rc, rc1; > > > > ssize_t len = 0; > > > > u32 count, ordinal; > > > > unsigned long stop; > > > > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct > > > > tpm_chip > > > > *chip, > > > > dev_err(&chip->dev, "tpm2_commit_space: error > > > > %d\n", rc); > > > > > > > > out: > > > > - rc = tpm_go_idle(chip, flags); > > > > - if (rc) > > > > + rc1 = tpm_go_idle(chip, flags); > > > > + if (rc1) > > > > goto out; > > > > > > > > if (need_locality) > > > > > > Thanks James and sorry for latency (holiday season). Just a small > > > suggestion. I would just: > > > > > > if (tpm_go_idle(chip, flags)) > > > goto out; > > > > > > What do you think? > > > > That it doesn't solve the loop forever with no warning problem. If > > anything, I think the correct thing is probably > > > > rc1 = tpm_go_idle(chip, flags); > > if (rc1) > > dev_err(&chip->dev, "go idle failed with %d\n", > > rc1); > > > > so we log the problem and move on. If it is a timeout, it will > > likely show up on the next TPM operation. Since this is the only > > caller of tpm_go_idle(), I think all looping should be done inside > > that function, but we should probably wait for Tomas to comment > > since he wrote it. > > > > We've already fixed it, I forgot myself , we were drinking too much > :) > https://patchwork.kernel.org/patch/10643565/ > Not sure why it was dropped. Taking the trouble to gather error returns and then ignoring them is not a good practice (it's actually been the bane of filesystems for a while). If you want to do it this way, tpm_go_idle() needs to be a void function that emits an error message for every problem condition. James ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() 2019-01-03 16:17 ` James Bottomley @ 2019-01-10 17:16 ` Jarkko Sakkinen 0 siblings, 0 replies; 9+ messages in thread From: Jarkko Sakkinen @ 2019-01-10 17:16 UTC (permalink / raw) To: James Bottomley; +Cc: Winkler, Tomas, linux-integrity On Thu, Jan 03, 2019 at 08:17:18AM -0800, James Bottomley wrote: > On Thu, 2019-01-03 at 15:34 +0000, Winkler, Tomas wrote: > > > -----Original Message----- > > > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com > > > ] > > > Sent: Thursday, January 03, 2019 17:24 > > > To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > Cc: Winkler, Tomas <tomas.winkler@intel.com>; linux- > > > integrity@vger.kernel.org > > > Subject: Re: [PATCH] tpm: fix incorrect success returns from > > > tpm_try_transmit() > > > > > > On Thu, 2019-01-03 at 14:59 +0200, Jarkko Sakkinen wrote: > > > > On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote: > > > > > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from > > > > > runtime_pm" we have been returning success from > > > > > tpm_try_transmit() even if an error occurred. The reason is > > > > > that the introduction of rc = tpm_go_idle() at the end of > > > > > processing overwrites the value of rc if it contains an error > > > > > code (mostly with success). Fix this by writing the return to > > > > > a new variable rc1 instead. > > > > > > > > > > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from > > > > > runtime_pm" > > > > > Cc: stable@vger.kernel.org > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnersh > > > > > ip.c > > > > > om> > > > > > > > > > > --- > > > > > > > > > > Note: the goto out looks fishy as well. The only go_idle > > > > > implementor is tpm_crb and that can return a timeout as -ETIME, > > > > > so it looks like it would then loop forever > > > > > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c > > > > > b/drivers/char/tpm/tpm-interface.c > > > > > index 129f640424b7..ac7ebab6140c 100644 > > > > > --- a/drivers/char/tpm/tpm-interface.c > > > > > +++ b/drivers/char/tpm/tpm-interface.c > > > > > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct > > > > > tpm_chip > > > > > *chip, > > > > > unsigned int flags) > > > > > { > > > > > struct tpm_output_header *header = (void *)buf; > > > > > - int rc; > > > > > + int rc, rc1; > > > > > ssize_t len = 0; > > > > > u32 count, ordinal; > > > > > unsigned long stop; > > > > > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct > > > > > tpm_chip > > > > > *chip, > > > > > dev_err(&chip->dev, "tpm2_commit_space: error > > > > > %d\n", rc); > > > > > > > > > > out: > > > > > - rc = tpm_go_idle(chip, flags); > > > > > - if (rc) > > > > > + rc1 = tpm_go_idle(chip, flags); > > > > > + if (rc1) > > > > > goto out; > > > > > > > > > > if (need_locality) > > > > > > > > Thanks James and sorry for latency (holiday season). Just a small > > > > suggestion. I would just: > > > > > > > > if (tpm_go_idle(chip, flags)) > > > > goto out; > > > > > > > > What do you think? > > > > > > That it doesn't solve the loop forever with no warning problem. If > > > anything, I think the correct thing is probably > > > > > > rc1 = tpm_go_idle(chip, flags); > > > if (rc1) > > > dev_err(&chip->dev, "go idle failed with %d\n", > > > rc1); > > > > > > so we log the problem and move on. If it is a timeout, it will > > > likely show up on the next TPM operation. Since this is the only > > > caller of tpm_go_idle(), I think all looping should be done inside > > > that function, but we should probably wait for Tomas to comment > > > since he wrote it. > > > > > > > We've already fixed it, I forgot myself , we were drinking too much > > :) > > https://patchwork.kernel.org/patch/10643565/ > > Not sure why it was dropped. > > Taking the trouble to gather error returns and then ignoring them is > not a good practice (it's actually been the bane of filesystems for a > while). If you want to do it this way, tpm_go_idle() needs to be a > void function that emits an error message for every problem condition. I'm happy to take a patch that adds logging in. /Jarkko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() 2019-01-03 15:34 ` Winkler, Tomas 2019-01-03 16:17 ` James Bottomley @ 2019-01-10 17:16 ` Jarkko Sakkinen 1 sibling, 0 replies; 9+ messages in thread From: Jarkko Sakkinen @ 2019-01-10 17:16 UTC (permalink / raw) To: Winkler, Tomas; +Cc: James Bottomley, linux-integrity On Thu, Jan 03, 2019 at 03:34:24PM +0000, Winkler, Tomas wrote: > We've already fixed it, I forgot myself , we were drinking too much :) > https://patchwork.kernel.org/patch/10643565/ > Not sure why it was dropped. > > Thanks > Tomas LOL what. Clearly I'm coming from holidays. This fix is in my tree and it was part of my last PR to James (for 5.0). And James' patch applies neither my master and next braches so I suppose he is using some old tree. /Jarkko ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-01-10 17:17 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-31 18:27 [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() James Bottomley 2019-01-03 12:59 ` Jarkko Sakkinen 2019-01-03 13:37 ` Winkler, Tomas 2019-01-03 15:03 ` Jarkko Sakkinen 2019-01-03 15:23 ` James Bottomley 2019-01-03 15:34 ` Winkler, Tomas 2019-01-03 16:17 ` James Bottomley 2019-01-10 17:16 ` Jarkko Sakkinen 2019-01-10 17:16 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).