linux-kernel - Re: [PATCH v4 2/2] tpm: add support for nonblocking operation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180812103955.GA4813@linux.intel.com>
Date:   Sun, 12 Aug 2018 13:39:55 +0300
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Tadeusz Struk <tadeusz.struk@...el.com>
Cc:     flihp@...bit.us, jgg@...pe.ca, linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/2] tpm: add support for nonblocking operation

On Fri, Aug 10, 2018 at 11:21:14AM -0700, Tadeusz Struk wrote:
> On 08/10/2018 10:43 AM, Jarkko Sakkinen wrote:
> >> +static struct workqueue_struct *tpm_dev_wq;
> > A naming contradiction with tpm_common_read() and tpm_common_write(). To
> > sort that up I would suggest adding a commit to the patch set that
> > renames these functions as tpm_dev_common_read() and
> > tpm_dev_common_write() and use the name tpm_common_dev_wq here.
> > 
> 
> Currently we have: tpm_open(), tpm_write(), tpm_release() in tpm-dev.c
> tpmrm_open(), tpmrm_read(), tpmrm_write(), tpmrm_release() in tpmrm-dev.c
> tpm_common_open(), tpm_common_read(), tpm_common_write(), tpm_common_release() in tpm-dev-common.c
> 
> I think that's pretty consistent. Do you want me to rename all of them to tpm_dev_*()?
> I don't see any value in doing this. What about if I just rename: 
> tpm_dev_wq_lock to tpm_common_wq_lock, and tpm_dev_wq to tpm_common_wq?

That is good enough. At least it is consistent.

> >> +static DEFINE_MUTEX(tpm_dev_wq_lock);
> > This is an unacceptable way to do it, Rather add:
> > 
> > int __init  tpm_dev_common_init(void)
> > {
> > 	tpm_dev_common_wq = alloc_workqueue("tpm_dev_wq", WQ_MEM_RECLAIM, 0);
> > 	if (!tpm_dev_common_wq)
> > 		return -ENOMEM;
> > 
> > 	return 0;
> > }
> > 
> > and call this in the driver initialization.
> > 
> That was the way it was implemented in v1 https://patchwork.kernel.org/patch/10442125/
> 
> See: static int __init tpm_dev_common_init(void)
> 
> and the feedback I got from Jason was:
> 
> "I wonder if it is worth creating this when the first file is
> opened.. Lots of systems have TPMs but few use the userspace.."
> 
> so I changed this to allocate the WQ on first open. I think it makes sense,
> but I leave it to you to decide.

Without a question I would go with tpm_common_init() for stability (one
less point of failure in open) and simplicity (no need for a locking
scheme).

> Tadeusz,
> -- 
> Tadeusz

/Jarkko