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

Do not refresh object store before fetching object. #614

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

larssilven
Copy link
Contributor

@larssilven larssilven commented Mar 4, 2021

Before this commit the object store for a file token was always refreshed by reading
the file of the token every time an object of the token was fetched.
Now the HSM may be configured not to refresh when fetching an object. But the
refresh will still be done after an application gets a handle for an object.

The reason for this change is that the CPU time consumed by the reading may not
be negligible for some HW.

@rijswijk
Copy link
Contributor

rijswijk commented Mar 4, 2021

Thanks for your PR; the reason it is implemented the way it currently is is to cope with multiple independent processes concurrently manipulating the same token through the SoftHSM library. If you disable the check, then a process will fail to pick up any changes to the token made by an external process. I understand that for performance reasons this is inconvenient, but perhaps it would be better, if you want to be able to disable it, to make this a configurable feature?

I'm wondering, though, if that should then be run-time configurable or compile-time (since you might not want two processes using SoftHSM with conflicting configurations). I'd like to ask @halderen for an opinion on this too.

@halderen
Copy link
Contributor

halderen commented Mar 4, 2021

It is really needed that multiple concurrent processes access the same token at the same time for many use cases. I'd first like to know the performance impact of such a thing. SoftHSMv2 isn't really that fast anyway (for security reasons) so if the effect isn't good for me. Not having the lock (so this PR) means bringing back the old SoftHSMv1 functionality and the way SoftHSMv2 sometimes operate due to a bug. This took us with OpenDNSSEC 2 1/2 years to find, so I'd rather not burn application programmers with this. In any case, this should not be a compile time option, as this would mean package maintainers may select this, and then everyone may be stuck with this. It should be a run-time, per slot/token option (but that might not be possible) option that is not default on.

@larssilven
Copy link
Contributor Author

larssilven commented Mar 6, 2021

I'm glad that you are willing to accept my proposal with the modification that the 'refresh disabling' should be optional and disabled by default.

Could we add a key called objectstore.readrefresh to the softhsm2.conf . If this key has the value 'false' then no refreshing is done before fetching an object from the store, if something else or not define then the refresh is done (as before).

I have just tested this with the file mode. Maybe nothing should be done about the DB mode (I removed a mutex lock)?

I believe it should be possible in 'file mode' for several processes to use the same objects in a store with 'objectstore.readrefresh=false'. There will just be a problem if one of the processes use C_DestroyObject or C_SetAttribute;
then the other processes will continue to used the object as it was before.

@larssilven
Copy link
Contributor Author

New version ready for review.

@larssilven
Copy link
Contributor Author

larssilven commented Mar 10, 2021

Benchmarks

I have compared result from a Debian VM running the p11speed test with different configurations. p11speed has used 24 threads each doing 1000 signings with 2048 RSA_PKCS.
It has been done on 2 different kind of storage, ordinary VM qcow2 and 9p that access a storage on the hypervisor. Here is the result:

storage type objectstore.readrefresh objectstore.backend signes/s
9p true file 386
9p true db 1275
9p false file 2393
9p false db 1263
qcow2 true file 2534
qcow2 true db 2170
qcow2 false file 2379
qcow2 false db 2163

I will remove my DB fix since it is not doing any difference.

@larssilven larssilven deleted the branch softhsm:develop July 29, 2021 08:40
@larssilven larssilven closed this Jul 29, 2021
@larssilven larssilven deleted the develop branch July 29, 2021 08:40
@larssilven larssilven restored the develop branch July 29, 2021 08:42
@larssilven larssilven reopened this Jul 29, 2021
@larssilven larssilven deleted the branch softhsm:develop January 6, 2022 15:15
@larssilven larssilven closed this Jan 6, 2022
@larssilven larssilven deleted the develop branch January 6, 2022 15:15
@larssilven larssilven restored the develop branch January 6, 2022 15:22
@larssilven larssilven reopened this Jan 6, 2022
@jschlyter jschlyter marked this pull request as draft November 29, 2024 16:24
@jschlyter
Copy link
Contributor

Please rebase on develop and mark as ready when ready.

@larssilven larssilven marked this pull request as ready for review November 30, 2024 14:46
@larssilven larssilven requested a review from a team as a code owner November 30, 2024 14:46
@larssilven larssilven force-pushed the develop branch 2 times, most recently from d2de4f5 to 46ae6eb Compare December 3, 2024 21:02
@larssilven
Copy link
Contributor Author

Now all tests but windows are OK. But I don't think these problem has anything to do with this.

@larssilven
Copy link
Contributor Author

how stupid of me. I did just assume the the windows build was not ready yet.
Thanks @bjosv

Before this commit the object store for a file token was always refreshed by reading
the file of the token every time an object of the token was fetched.
Now the HSM may be configured not to refresh when fetching an object. But the
refresh will still be done after an application gets a handle for an object.

The reason for this change is that the CPU time consumed by the reading may not
be negligible for some HW.

It is only the objects store on file that are affected by this feature. If DB is
used refreshing will always be done since the is then no difference in CPU usage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants