Add proper locking to the fpu_ctx allocated by aesni..
ClosedPublic
Actions

Authored by jmg on Jul 7 2015, 4:25 PM.

Details

Reviewers

kib
eri
gnn

Commits

rS285289: address an issue where consumers, like IPsec, can reuse the same

Summary

Previously the use of fpu_ctx relied on the callers to only have one
outstanding crypto operation per session, this was violated by IPsec..
This means that the same fpu_ctx could be in use by multiple threads
at the same time... We now have a fpu context per CPU and aquire a
mutex for the current cpu... As we lock the mutex, this can safely
handle thread migration to another cpu after the lock is aquired..

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jmg updated this revision to Diff 6756.Jul 7 2015, 4:25 PM

jmg retitled this revision from to Add proper locking to the fpu_ctx allocated by aesni...

jmg updated this object.

jmg edited the test plan for this revision. (Show Details)

jmg added reviewers: gnn, eri, kib.

Herald added a subscriber: imp. · View Herald TranscriptJul 7 2015, 4:25 PM

Except the pause() thing, the patch looks good.

sys/crypto/aesni/aesni.c
142 ↗	(On Diff #6756)	This line should not be committed.
154 ↗	(On Diff #6756)	sizeof(*ctx_mtx) is both style-required and easier to read.
161 ↗	(On Diff #6756)	Note that top (and ddb etc) only has 5 symbols for the wchan.
201 ↗	(On Diff #6756)	What is this ?
499 ↗	(On Diff #6756)	Spaces should be put around '\|'.

gnn added inline comments.Jul 8 2015, 1:14 PM

sys/crypto/aesni/aesni.c
509 ↗	(On Diff #6756)	You jump into an if block? Do you have to do this this way?

Other than the jump into if block I'm fine with this.

This revision is now accepted and ready to land.Jul 8 2015, 1:15 PM

Apart the pause its OK.

sys/crypto/aesni/aesni.c
201 ↗	(On Diff #6756)	Yeah the pause is really wierd here i did the same comment on offline review.

jmg marked an inline comment as done.Jul 8 2015, 6:31 PM

jmg added inline comments.

sys/crypto/aesni/aesni.c
154 ↗	(On Diff #6756)	I prefer to do without, and I have good style reasons to do so. You cannot do sizeof type, it will generate a compile error... The lack of parens shows to me that I am using a variable which, IMO, is the correct usage, and denotes in my head if I see parens to examine the statement more closely that the size may not match the variable, and I have to go upto the variable declaration to validate it...
161 ↗	(On Diff #6756)	well, 6 it looks like.. should I shorted aesni to ani?
201 ↗	(On Diff #6756)	The issue laid out: Thread 1 run up through just before 197 Thread 2 tries to create new session, and gets into _newsession and blocks on line 259 Thread 1 runs through, and executes line 203, destroying rw lock Thread 2 continues and attemptes to aquire destroyed rw lock. This should be fixed in the crypto framework, the ability to lock out new sessions with a _drain routine... The routine simply aquires the CRYPTO_DRIVER lock, and releases it, ensuring that all code is out of _newsession. I'll work on that next.
509 ↗	(On Diff #6756)	I could do the ugly: if (kt) return error; 508 509 510 512 But then the pattern of if(!kt) would be broken to match with the above... also, I'm jumping from one if (!kt) block to another if (!kt) block, so IMO less bad, and keeps the code more logical/uniform..

address a couple of kib's comments...

This revision now requires review to proceed.Jul 8 2015, 7:03 PM

Closed by commit rS285289: address an issue where consumers, like IPsec, can reuse the same (authored by jmg). · Explain WhyJul 8 2015, 7:15 PM

This revision was automatically updated to reflect the committed changes.

jmg mentioned this in rS285290: Now that aesni won't reuse fpu contexts (D3016), add seatbelts to the.Jul 8 2015, 7:26 PM

the pause has been removed in rS285297, it was unneeded because another lock by the crypto driver prevented it. I have also documented this in the man page.