linux-kernel - Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 11 Jul 2022 23:03:05 +0200
From:   Lino Sanfilippo <LinoSanfilippo@....de>
To:     Jarkko Sakkinen <jarkko@...nel.org>
Cc:     peterhuewe@....de, jgg@...pe.ca, stefanb@...ux.vnet.ibm.com,
        linux@...ewoehner.de, linux-integrity@...r.kernel.org,
        linux-kernel@...r.kernel.org, l.sanfilippo@...bus.com,
        lukas@...ner.de, p.rosenberger@...bus.com
Subject: Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for
 locality

On 11.07.22 04:50, Jarkko Sakkinen wrote:
> On Mon, Jul 04, 2022 at 07:45:12PM +0200, Lino Sanfilippo wrote:
>>
>>
>> On 01.07.22 01:29, Jarkko Sakkinen wrote:
>>
>>>
>>> I'm kind of thinking that should tpm_tis_data have a lock for its
>>> contents?
>>
>> Most of the tpm_tis_data structure elements are set once during init and
>> then never changed but only read. So no need for locking for these. The
>> exceptions I see are
>>
>> - flags
>> - locality_count
>> - locality
>
> I'd still go for single data struct lock, since this lock would
> be taken in every transmit flow.

Well in both cases, transmit and receive, we end up in wait_for_tmp_stat().
Whatever lock we hold at this time cannot be taken in the interrupt
handler, since this would deadlock (wait_for_tmp_stat() waits for the interrupt
handler to complete but holds the lock that the interrupt handler needs to proceed).

So in the interrupt handler we need something that is not held during the whole
transmit/receive flow.

This is the reason why the locality_count_mutex only protects the one thing we
have to take care of in the interrupt handler, namely the locality counter.

> It makes the whole thing easier
> to maintain over time, and does not really affect scalability>
> This brings me to another question: what does this lock protect
> against given that tpm_try_get_ops() already takes tpm_mutex?
> It's not clear and that should be somehow reasoned in the commit
> message.

See above, we cannot take the tpm mutex in the interrupt handler for the same
reason.

> Anyway, *if* a lock is needed the granularity should be the whole
> struct.
>
> BR, Jarkko

Regards,
Lino