Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cloning of /dev/tpmrm0 fds on fork #134

Closed
AndreasFuchsTPM opened this issue Dec 11, 2024 · 7 comments
Closed

Fix cloning of /dev/tpmrm0 fds on fork #134

AndreasFuchsTPM opened this issue Dec 11, 2024 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@AndreasFuchsTPM
Copy link
Member

AndreasFuchsTPM commented Dec 11, 2024

Related to #130 and #118

There seems to be a problem that Apache (from #130) and NGINX (tested by me) just duplicate the whole context for their worker threads.
This leads to the case where the Esys_Context and specifically the embedded tcti and its opened fds will be accessed from several threads. This leads to confusion inside the kernel indicated by -EBUSY and subsequently some Esys_BAD_SEQUENCE errors.

The only workaround for the moment is to set thread_pool default threads=1; for NGINX (similar for apache).

One solution that comes to my mind for the moment is to introduce a global mutex (that survives the forking) to guard all Esys_* calls.

Or we need to move the Esys_Initialize until after the fork of NGINX.

I wonder and have no idea, when in the sequence of tpm2-openssl commands the fork() from NGINX happens.
Does anyone know ?

@AndreasFuchsTPM AndreasFuchsTPM changed the title Add mutex to ESYS_* calls Fix cloning of /dev/tpmrm0 fds on fork Dec 11, 2024
@AndreasFuchsTPM
Copy link
Member Author

I guess opening and closing the ESYS_CONTEXT surrounding any low-level Esys_* call will be the only way here.
@gotthardp How much insight do you have here ?

@AndreasFuchsTPM
Copy link
Member Author

Or we go ahead and memorize our pid and check it before every Esys_* call.
And if the pid has changed that we initialize a new ESYS_CONTEXT. This would prevent some of the overhead compared to making an Esys_Initialize for every Esys_Sign

@gotthardp
Copy link
Contributor

Also related to tpm2-software/tpm2-tss#2914

@gotthardp gotthardp added the bug Something isn't working label Dec 14, 2024
@gotthardp gotthardp self-assigned this Dec 14, 2024
@gotthardp
Copy link
Contributor

@AndreasFuchsTPM And how about locking access to the ESYS_CONTEXT as suggested in another thread? I implemented it in the https://github.com/tpm2-software/tpm2-openssl/tree/threads branch.

gotthardp added a commit that referenced this issue Dec 15, 2024
@AndreasFuchsTPM
Copy link
Member Author

The big question is: Does this lock span over multiple processes ?
Remember we are talking fork here, not only pthread.
I'm afraid this won't help much in this case.

gotthardp added a commit that referenced this issue Dec 23, 2024
gotthardp added a commit that referenced this issue Dec 23, 2024
gotthardp added a commit that referenced this issue Dec 23, 2024
gotthardp added a commit that referenced this issue Dec 23, 2024
@gotthardp
Copy link
Contributor

@AndreasFuchsTPM True. I reworked the patch to semaphores.

gotthardp added a commit that referenced this issue Dec 23, 2024
gotthardp added a commit that referenced this issue Dec 23, 2024
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
gotthardp added a commit that referenced this issue Jan 4, 2025
@gotthardp
Copy link
Contributor

Fixed by 775311a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants