From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760799AbdACTNt (ORCPT ); Tue, 3 Jan 2017 14:13:49 -0500 Received: from quartz.orcorp.ca ([184.70.90.242]:58778 "EHLO quartz.orcorp.ca" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757444AbdACTNm (ORCPT ); Tue, 3 Jan 2017 14:13:42 -0500 Date: Tue, 3 Jan 2017 12:13:28 -0700 From: Jason Gunthorpe To: Jarkko Sakkinen Cc: tpmdd-devel@lists.sourceforge.net, linux-security-module@vger.kernel.org, Peter Huewe , Marcel Selhorst , open list Subject: Re: [PATCH RFC 1/4] tpm: migrate struct tpm_buf to struct tpm_chip Message-ID: <20170103191328.GB26706@obsidianresearch.com> References: <20170102132213.22880-1-jarkko.sakkinen@linux.intel.com> <20170102132213.22880-2-jarkko.sakkinen@linux.intel.com> <20170102210101.GA5544@obsidianresearch.com> <20170103005737.t2qrc32xzdnvqy4b@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170103005737.t2qrc32xzdnvqy4b@intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Broken-Reverse-DNS: no host name found for IP address 10.0.0.156 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 03, 2017 at 02:57:37AM +0200, Jarkko Sakkinen wrote: > On Mon, Jan 02, 2017 at 02:01:01PM -0700, Jason Gunthorpe wrote: > > On Mon, Jan 02, 2017 at 03:22:07PM +0200, Jarkko Sakkinen wrote: > > > Since there is only one thread using TPM chip at a time to transmit data > > > we can migrate struct tpm_buf to struct tpm_chip. This makes the use of > > > it more fail safe as the buffer is allocated from heap when the device > > > is created and not for every transaction. > > > > Eh? What? I don't think that is the case.. > > > > We don't serialize until we hit tramsit_cmd at which point the buffer > > is already being used and cannot be shared between threads. > > There is a regression in the patch. All functions that use 'tr_buf' > should take tpm_mutex first and use TPM_TRANSMIT_UNLOCKED. There's > also a similar regression in TPM space patch that I have to correct. No, you can't steal TPM_TRANSMIT_UNLOCKED and tpm_mutex for this, that is to allow a chain of commands to execute atomicly, so a new lock is needed just for the tr_buf. > > Why would the resource manager need a single global tpm buffer? That > > seems like a big regression from where we have been going. I don't > > think this is a good idea to go down this road. > > What? 'tr_buf' is not specifically for resource manager. This commit > makes creating TPM commands more fail-safe because there is no need > to allocate page for every transmit. That doesn't seem all that important, honestly. There kernel does not fail single page allocations without a lot of duress. > For RM decorations this is really important because I rather would have > them fail as rarely as possible. If this would become a scalability > issue then the granularity could be reconsidered. Why? The RM design already seems to have the prepare/commit/abort kind of model so it can already fail. What does it matter if the caller can fail before getting that far? It seems like alot of dangerous churn to introduce a new locking model without a really good reason... Jason From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Gunthorpe Subject: Re: [PATCH RFC 1/4] tpm: migrate struct tpm_buf to struct tpm_chip Date: Tue, 3 Jan 2017 12:13:28 -0700 Message-ID: <20170103191328.GB26706@obsidianresearch.com> References: <20170102132213.22880-1-jarkko.sakkinen@linux.intel.com> <20170102132213.22880-2-jarkko.sakkinen@linux.intel.com> <20170102210101.GA5544@obsidianresearch.com> <20170103005737.t2qrc32xzdnvqy4b@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20170103005737.t2qrc32xzdnvqy4b-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: tpmdd-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Jarkko Sakkinen Cc: linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, open list List-Id: tpmdd-devel@lists.sourceforge.net On Tue, Jan 03, 2017 at 02:57:37AM +0200, Jarkko Sakkinen wrote: > On Mon, Jan 02, 2017 at 02:01:01PM -0700, Jason Gunthorpe wrote: > > On Mon, Jan 02, 2017 at 03:22:07PM +0200, Jarkko Sakkinen wrote: > > > Since there is only one thread using TPM chip at a time to transmit data > > > we can migrate struct tpm_buf to struct tpm_chip. This makes the use of > > > it more fail safe as the buffer is allocated from heap when the device > > > is created and not for every transaction. > > > > Eh? What? I don't think that is the case.. > > > > We don't serialize until we hit tramsit_cmd at which point the buffer > > is already being used and cannot be shared between threads. > > There is a regression in the patch. All functions that use 'tr_buf' > should take tpm_mutex first and use TPM_TRANSMIT_UNLOCKED. There's > also a similar regression in TPM space patch that I have to correct. No, you can't steal TPM_TRANSMIT_UNLOCKED and tpm_mutex for this, that is to allow a chain of commands to execute atomicly, so a new lock is needed just for the tr_buf. > > Why would the resource manager need a single global tpm buffer? That > > seems like a big regression from where we have been going. I don't > > think this is a good idea to go down this road. > > What? 'tr_buf' is not specifically for resource manager. This commit > makes creating TPM commands more fail-safe because there is no need > to allocate page for every transmit. That doesn't seem all that important, honestly. There kernel does not fail single page allocations without a lot of duress. > For RM decorations this is really important because I rather would have > them fail as rarely as possible. If this would become a scalability > issue then the granularity could be reconsidered. Why? The RM design already seems to have the prepare/commit/abort kind of model so it can already fail. What does it matter if the caller can fail before getting that far? It seems like alot of dangerous churn to introduce a new locking model without a really good reason... Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot