From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jarkko Sakkinen Date: Wed, 25 Oct 2017 19:34:52 +0000 Subject: Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get() Message-Id: <20171025193452.d4qa4dhacfgqejk7@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit List-Id: References: <20171025115508.5682-1-jarkko.sakkinen@linux.intel.com> In-Reply-To: To: PrasannaKumar Muralidharan Cc: linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, Matt Mackall , Herbert Xu , Peter Huewe , Marcel Selhorst , Jason Gunthorpe , Mimi Zohar , Dmitry Kasatkin , James Morris , "Serge E. Hallyn" , David Safford , David Howells , Jerry Snitselaar , "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , open list , "moderated list:TPM DEVICE DRIVER" On Wed, Oct 25, 2017 at 08:40:26PM +0530, PrasannaKumar Muralidharan wrote: > > -struct tpm_chip *tpm_chip_find_get(int chip_num) > > +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip) > > { > > - struct tpm_chip *chip, *res = NULL; > > + struct tpm_chip *res = NULL; > > + int chip_num = 0; > > int chip_prev; > > > > mutex_lock(&idr_lock); > > > > - if (chip_num = TPM_ANY_NUM) { > > - chip_num = 0; > > + if (!chip) { > > do { > > chip_prev = chip_num; > > chip = idr_get_next(&dev_nums_idr, &chip_num); > > When chip is not NULL just do tpm_try_get_ops(chip). Current code does > more things which are not required. Your observation is right that there is something wrong but conclusions are incorrect. It's actually a regression. If @chip has a value, the code does one iteration of what it is doing in the first branch of the condition. That is completely bogus semantics to say the least. To sort that out I'll introduce a new field to struct tpm_chip: u64 id; This gets a value from a global count every time a chip is created. The function will become then: struct tpm_chip *tpm_chip_find_get(u64 id) { struct tpm_chup *chip; struct tpm_chip *res = NULL; int chip_num = 0; int chip_prev; mutex_lock(&idr_lock); do { chip_prev = chip_num; chip = idr_get_next(&dev_nums_idr, &chip_num); if (chip && (!id || id = chip->id) && !tpm_try_get_ops(chip)) { res = chip; break; } } while (chip_prev != chip_num); mutex_unlock(&idr_lock); return res; } Thanks for spotting this out. I'll refine the patch. /Jarkko From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jarkko Sakkinen Subject: Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get() Date: Wed, 25 Oct 2017 21:34:52 +0200 Message-ID: <20171025193452.d4qa4dhacfgqejk7@linux.intel.com> References: <20171025115508.5682-1-jarkko.sakkinen@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, Matt Mackall , Herbert Xu , Peter Huewe , Marcel Selhorst , Jason Gunthorpe , Mimi Zohar , Dmitry Kasatkin , James Morris , "Serge E. Hallyn" , David Safford , David Howells , Jerry Snitselaar , "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , open list , "moderated list:TPM DEVICE DRIVER" , " To: PrasannaKumar Muralidharan Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-crypto.vger.kernel.org On Wed, Oct 25, 2017 at 08:40:26PM +0530, PrasannaKumar Muralidharan wrote: > > -struct tpm_chip *tpm_chip_find_get(int chip_num) > > +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip) > > { > > - struct tpm_chip *chip, *res = NULL; > > + struct tpm_chip *res = NULL; > > + int chip_num = 0; > > int chip_prev; > > > > mutex_lock(&idr_lock); > > > > - if (chip_num == TPM_ANY_NUM) { > > - chip_num = 0; > > + if (!chip) { > > do { > > chip_prev = chip_num; > > chip = idr_get_next(&dev_nums_idr, &chip_num); > > When chip is not NULL just do tpm_try_get_ops(chip). Current code does > more things which are not required. Your observation is right that there is something wrong but conclusions are incorrect. It's actually a regression. If @chip has a value, the code does one iteration of what it is doing in the first branch of the condition. That is completely bogus semantics to say the least. To sort that out I'll introduce a new field to struct tpm_chip: u64 id; This gets a value from a global count every time a chip is created. The function will become then: struct tpm_chip *tpm_chip_find_get(u64 id) { struct tpm_chup *chip; struct tpm_chip *res = NULL; int chip_num = 0; int chip_prev; mutex_lock(&idr_lock); do { chip_prev = chip_num; chip = idr_get_next(&dev_nums_idr, &chip_num); if (chip && (!id || id == chip->id) && !tpm_try_get_ops(chip)) { res = chip; break; } } while (chip_prev != chip_num); mutex_unlock(&idr_lock); return res; } Thanks for spotting this out. I'll refine the patch. /Jarkko From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com ([134.134.136.65]:37278 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751522AbdJYTfA (ORCPT ); Wed, 25 Oct 2017 15:35:00 -0400 Date: Wed, 25 Oct 2017 21:34:52 +0200 From: Jarkko Sakkinen To: PrasannaKumar Muralidharan Cc: linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, Matt Mackall , Herbert Xu , Peter Huewe , Marcel Selhorst , Jason Gunthorpe , Mimi Zohar , Dmitry Kasatkin , James Morris , "Serge E. Hallyn" , David Safford , David Howells , Jerry Snitselaar , "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , open list , "moderated list:TPM DEVICE DRIVER" , "open list:INTEGRITY MEASUREMENT ARCHITECTURE (IMA)" , "open list:INTEGRITY MEASUREMENT ARCHITECTURE (IMA)" , "open list:KEYS-TRUSTED" Subject: Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get() Message-ID: <20171025193452.d4qa4dhacfgqejk7@linux.intel.com> References: <20171025115508.5682-1-jarkko.sakkinen@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: Sender: linux-integrity-owner@vger.kernel.org List-ID: On Wed, Oct 25, 2017 at 08:40:26PM +0530, PrasannaKumar Muralidharan wrote: > > -struct tpm_chip *tpm_chip_find_get(int chip_num) > > +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip) > > { > > - struct tpm_chip *chip, *res = NULL; > > + struct tpm_chip *res = NULL; > > + int chip_num = 0; > > int chip_prev; > > > > mutex_lock(&idr_lock); > > > > - if (chip_num == TPM_ANY_NUM) { > > - chip_num = 0; > > + if (!chip) { > > do { > > chip_prev = chip_num; > > chip = idr_get_next(&dev_nums_idr, &chip_num); > > When chip is not NULL just do tpm_try_get_ops(chip). Current code does > more things which are not required. Your observation is right that there is something wrong but conclusions are incorrect. It's actually a regression. If @chip has a value, the code does one iteration of what it is doing in the first branch of the condition. That is completely bogus semantics to say the least. To sort that out I'll introduce a new field to struct tpm_chip: u64 id; This gets a value from a global count every time a chip is created. The function will become then: struct tpm_chip *tpm_chip_find_get(u64 id) { struct tpm_chup *chip; struct tpm_chip *res = NULL; int chip_num = 0; int chip_prev; mutex_lock(&idr_lock); do { chip_prev = chip_num; chip = idr_get_next(&dev_nums_idr, &chip_num); if (chip && (!id || id == chip->id) && !tpm_try_get_ops(chip)) { res = chip; break; } } while (chip_prev != chip_num); mutex_unlock(&idr_lock); return res; } Thanks for spotting this out. I'll refine the patch. /Jarkko From mboxrd@z Thu Jan 1 00:00:00 1970 From: jarkko.sakkinen@linux.intel.com (Jarkko Sakkinen) Date: Wed, 25 Oct 2017 21:34:52 +0200 Subject: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get() In-Reply-To: References: <20171025115508.5682-1-jarkko.sakkinen@linux.intel.com> Message-ID: <20171025193452.d4qa4dhacfgqejk7@linux.intel.com> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Wed, Oct 25, 2017 at 08:40:26PM +0530, PrasannaKumar Muralidharan wrote: > > -struct tpm_chip *tpm_chip_find_get(int chip_num) > > +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip) > > { > > - struct tpm_chip *chip, *res = NULL; > > + struct tpm_chip *res = NULL; > > + int chip_num = 0; > > int chip_prev; > > > > mutex_lock(&idr_lock); > > > > - if (chip_num == TPM_ANY_NUM) { > > - chip_num = 0; > > + if (!chip) { > > do { > > chip_prev = chip_num; > > chip = idr_get_next(&dev_nums_idr, &chip_num); > > When chip is not NULL just do tpm_try_get_ops(chip). Current code does > more things which are not required. Your observation is right that there is something wrong but conclusions are incorrect. It's actually a regression. If @chip has a value, the code does one iteration of what it is doing in the first branch of the condition. That is completely bogus semantics to say the least. To sort that out I'll introduce a new field to struct tpm_chip: u64 id; This gets a value from a global count every time a chip is created. The function will become then: struct tpm_chip *tpm_chip_find_get(u64 id) { struct tpm_chup *chip; struct tpm_chip *res = NULL; int chip_num = 0; int chip_prev; mutex_lock(&idr_lock); do { chip_prev = chip_num; chip = idr_get_next(&dev_nums_idr, &chip_num); if (chip && (!id || id == chip->id) && !tpm_try_get_ops(chip)) { res = chip; break; } } while (chip_prev != chip_num); mutex_unlock(&idr_lock); return res; } Thanks for spotting this out. I'll refine the 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jarkko Sakkinen Subject: Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get() Date: Wed, 25 Oct 2017 21:34:52 +0200 Message-ID: <20171025193452.d4qa4dhacfgqejk7@linux.intel.com> References: <20171025115508.5682-1-jarkko.sakkinen@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: PrasannaKumar Muralidharan Cc: linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, Matt Mackall , Herbert Xu , Peter Huewe , Marcel Selhorst , Jason Gunthorpe , Mimi Zohar , Dmitry Kasatkin , James Morris , "Serge E. Hallyn" , David Safford , David Howells , Jerry Snitselaar , "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , open list , "moderated list:TPM DEVICE DRIVER" List-Id: tpmdd-devel@lists.sourceforge.net On Wed, Oct 25, 2017 at 08:40:26PM +0530, PrasannaKumar Muralidharan wrote: > > -struct tpm_chip *tpm_chip_find_get(int chip_num) > > +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip) > > { > > - struct tpm_chip *chip, *res = NULL; > > + struct tpm_chip *res = NULL; > > + int chip_num = 0; > > int chip_prev; > > > > mutex_lock(&idr_lock); > > > > - if (chip_num == TPM_ANY_NUM) { > > - chip_num = 0; > > + if (!chip) { > > do { > > chip_prev = chip_num; > > chip = idr_get_next(&dev_nums_idr, &chip_num); > > When chip is not NULL just do tpm_try_get_ops(chip). Current code does > more things which are not required. Your observation is right that there is something wrong but conclusions are incorrect. It's actually a regression. If @chip has a value, the code does one iteration of what it is doing in the first branch of the condition. That is completely bogus semantics to say the least. To sort that out I'll introduce a new field to struct tpm_chip: u64 id; This gets a value from a global count every time a chip is created. The function will become then: struct tpm_chip *tpm_chip_find_get(u64 id) { struct tpm_chup *chip; struct tpm_chip *res = NULL; int chip_num = 0; int chip_prev; mutex_lock(&idr_lock); do { chip_prev = chip_num; chip = idr_get_next(&dev_nums_idr, &chip_num); if (chip && (!id || id == chip->id) && !tpm_try_get_ops(chip)) { res = chip; break; } } while (chip_prev != chip_num); mutex_unlock(&idr_lock); return res; } Thanks for spotting this out. I'll refine the patch. /Jarkko