On Thu, Aug 25, 2016 at 05:06:10PM -0400, Jarkko Sakkinen wrote:

On Thu, Aug 25, 2016 at 12:30:59PM -0600, Jason Gunthorpe wrote:

quoted

On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:

quoted

+     if (flags & TPM_TRANSMIT_LOCK)
+             mutex_lock(&chip->tpm_mutex);

I think I would invert this. UNLOCKED is the exceptional case, so I'd
make the 0 flags lock. If we see UNLOCKED in the caller then we know
to audit for locking, 0 is much less obvious.

I'm fine with either way.

quoted

quoted

@@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
 		goto out;
 	}
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
+	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);

All these points should accept a flags too and the caller should pass
in the TPM_TRASNMIT_UNLOCKED if it needs it..

For this bug fix it makes sense to implement it the way I did because it
needs to be applied to multiple releases (I think I've underlined this
in my changelog).

You shouldn't compromise the mainline kernel to ease backporting, I'm
not sure why adding a flags to tpm2_load would be a problem for the
-stable kernels?

It is generally better to make the backports move the older kernels
closer to mainline than to have them be something else, it makes it
easier to apply future backport fixes.

If you think this is high priority, I can make the next revision into
patch set of two patches. The second patch would implement the change
you suggested.

Yes, I think it is important the locking requirement be very clear
from the code.

Jason