On 17.09.20 17:01, Ingo Franzki wrote:
> On 15.09.2020 11:21, Harald Freudenberger wrote:
>> When both the paes and the pkey kernel module are statically build
>> into the kernel, the paes cipher selftests run before the pkey
>> kernel module is initialized. So a static variable set in the pkey
>> init function and used in the pkey_clr2protkey function is not
>> initialized when the paes cipher's selftests request to call pckmo for
>> transforming a clear key value into a protected key.
>>
>> This patch moves the initial setup of the static variable into
>> the function pck_clr2protkey. So it's possible, to use the function
>> for transforming a clear to a protected key even before the pkey
>> init function has been called and the paes selftests may run
>> successful.
>>
>> Signed-off-by: Harald Freudenberger <freude@xxxxxxxxxxxxx>
>> Reported-by: Alexander Egorenkov <Alexander.Egorenkov@xxxxxxx>
>> Cc: Stable <stable@xxxxxxxxxxxxxxx>
>> ---
>> drivers/s390/crypto/pkey_api.c | 30 ++++++++++++++++--------------
>> 1 file changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
>> index 490917cd44d0..5f75c9dfe071 100644
>> --- a/drivers/s390/crypto/pkey_api.c
>> +++ b/drivers/s390/crypto/pkey_api.c
>> @@ -34,9 +34,6 @@ MODULE_DESCRIPTION("s390 protected key interface");
>> #define KEYBLOBBUFSIZE 8192 /* key buffer size used for internal processing */
>> #define MAXAPQNSINLIST 64 /* max 64 apqns within a apqn list */
>>
>> -/* mask of available pckmo subfunctions, fetched once at module init */
>> -static cpacf_mask_t pckmo_functions;
>> -
>> /*
>> * debug feature data and functions
>> */
>> @@ -90,6 +87,9 @@ static int pkey_clr2protkey(u32 keytype,
>> const struct pkey_clrkey *clrkey,
>> struct pkey_protkey *protkey)
>> {
>> + /* mask of available pckmo subfunctions */
>> + static cpacf_mask_t pckmo_functions;
>> +
>> long fc;
>> int keysize;
>> u8 paramblock[64];
>> @@ -113,11 +113,13 @@ static int pkey_clr2protkey(u32 keytype,
>> return -EINVAL;
>> }
>>
>> - /*
>> - * Check if the needed pckmo subfunction is available.
>> - * These subfunctions can be enabled/disabled by customers
>> - * in the LPAR profile or may even change on the fly.
>> - */
>> + /* did we already check for PCKMO ? */
>> + if (!pckmo_functions.bytes[0]) {
>> + /* no, so check now */
>> + if (!cpacf_query(CPACF_PCKMO, &pckmo_functions))
>> + return -ENODEV;
>> + }
> Does this need a lock here? What if 2 processes or threads call this concurrently?
> Certainly the cpacf_query will produce the same result, but updating static pckmo_functions concurrently might cause problems.
I'd say as long as both concurrent threads will fetch the very same value - No, even if the update is not done atomically.
For example:
u64 blubber[2] = { 0, 0 };
// thread 1 updates blubber but unfortunately not in an atomic way:
blubber[0] = 0x1122334455667788;
// now interrupted by thread 2 which updates blubber now:
blubber[1] = 0x8877665544332211;
blubber[0] = 0x1122334455667788;
// and finally thread1 comes again and does the 2. half of the update:
blubber[1] = 0x8877665544332211;
even if you reshuffle this the result is the very same as long as both threads
update blubber WITH THE SAME VALUE.
For a different value, I do agree some kind of locking is needed.
At least this is my understanding...
>> + /* check for the pckmo subfunction we need now */
>> if (!cpacf_test_func(&pckmo_functions, fc)) {
>> DEBUG_ERR("%s pckmo functions not available\n", __func__);
>> return -ENODEV;
>> @@ -1853,7 +1855,7 @@ static struct miscdevice pkey_dev = {
>> */
>> static int __init pkey_init(void)
>> {
>> - cpacf_mask_t kmc_functions;
>> + cpacf_mask_t func_mask;
>>
>> /*
>> * The pckmo instruction should be available - even if we don't
>> @@ -1861,15 +1863,15 @@ static int __init pkey_init(void)
>> * is also the minimum level for the kmc instructions which
>> * are able to work with protected keys.
>> */
>> - if (!cpacf_query(CPACF_PCKMO, &pckmo_functions))
>> + if (!cpacf_query(CPACF_PCKMO, &func_mask))
>> return -ENODEV;
>>
>> /* check for kmc instructions available */
>> - if (!cpacf_query(CPACF_KMC, &kmc_functions))
>> + if (!cpacf_query(CPACF_KMC, &func_mask))
>> return -ENODEV;
>> - if (!cpacf_test_func(&kmc_functions, CPACF_KMC_PAES_128) ||
>> - !cpacf_test_func(&kmc_functions, CPACF_KMC_PAES_192) ||
>> - !cpacf_test_func(&kmc_functions, CPACF_KMC_PAES_256))
>> + if (!cpacf_test_func(&func_mask, CPACF_KMC_PAES_128) ||
>> + !cpacf_test_func(&func_mask, CPACF_KMC_PAES_192) ||
>> + !cpacf_test_func(&func_mask, CPACF_KMC_PAES_256))
>> return -ENODEV;
>>
>> pkey_debug_init();
>>
>
