Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()

[Thiago Jung Bauermann]

Dave Hansen  writes:

> On 03/28/2018 01:47 PM, Thiago Jung Bauermann wrote:
if (flags)
 -  assert(rdpkey_reg() > orig_pkey_reg);
 +  assert(rdpkey_reg() < orig_pkey_reg);
  }

  void pkey_write_allow(int pkey)
>>> This seems so horribly wrong that I wonder how it worked in the first
>>> place.  Any idea?
>> The code simply wasn't used. pkey_disable_clear() is called by
>> pkey_write_allow() and pkey_access_allow(), but before this patch series
>> nothing called either of these functions.
>> 
>
> Ahh, that explains it.  Can that get stuck in the changelog, please?

Yes, will be in the next version.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()

[Dave Hansen]

On 03/28/2018 01:47 PM, Thiago Jung Bauermann wrote:
>>> if (flags)
>>> -   assert(rdpkey_reg() > orig_pkey_reg);
>>> +   assert(rdpkey_reg() < orig_pkey_reg);
>>>  }
>>>
>>>  void pkey_write_allow(int pkey)
>> This seems so horribly wrong that I wonder how it worked in the first
>> place.  Any idea?
> The code simply wasn't used. pkey_disable_clear() is called by
> pkey_write_allow() and pkey_access_allow(), but before this patch series
> nothing called either of these functions.
> 

Ahh, that explains it.  Can that get stuck in the changelog, please?

Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()

[Thiago Jung Bauermann]

Dave Hansen  writes:

> On 02/21/2018 05:55 PM, Ram Pai wrote:
>> --- a/tools/testing/selftests/vm/protection_keys.c
>> +++ b/tools/testing/selftests/vm/protection_keys.c
>> @@ -461,7 +461,7 @@ void pkey_disable_clear(int pkey, int flags)
>>  pkey, pkey, pkey_rights);
>>  pkey_assert(pkey_rights >= 0);
>>
>> -pkey_rights |= flags;
>> +pkey_rights &= ~flags;
>>
>>  ret = pkey_set(pkey, pkey_rights, 0);
>>  /* pkey_reg and flags have the same format */
>> @@ -475,7 +475,7 @@ void pkey_disable_clear(int pkey, int flags)
>>  dprintf1("%s(%d) pkey_reg: 0x%016lx\n", __func__,
>>  pkey, rdpkey_reg());
>>  if (flags)
>> -assert(rdpkey_reg() > orig_pkey_reg);
>> +assert(rdpkey_reg() < orig_pkey_reg);
>>  }
>>
>>  void pkey_write_allow(int pkey)
>
> This seems so horribly wrong that I wonder how it worked in the first
> place.  Any idea?

The code simply wasn't used. pkey_disable_clear() is called by
pkey_write_allow() and pkey_access_allow(), but before this patch series
nothing called either of these functions.


--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()

[Dave Hansen]

On 02/21/2018 05:55 PM, Ram Pai wrote:
> --- a/tools/testing/selftests/vm/protection_keys.c
> +++ b/tools/testing/selftests/vm/protection_keys.c
> @@ -461,7 +461,7 @@ void pkey_disable_clear(int pkey, int flags)
>   pkey, pkey, pkey_rights);
>   pkey_assert(pkey_rights >= 0);
>  
> - pkey_rights |= flags;
> + pkey_rights &= ~flags;
>  
>   ret = pkey_set(pkey, pkey_rights, 0);
>   /* pkey_reg and flags have the same format */
> @@ -475,7 +475,7 @@ void pkey_disable_clear(int pkey, int flags)
>   dprintf1("%s(%d) pkey_reg: 0x%016lx\n", __func__,
>   pkey, rdpkey_reg());
>   if (flags)
> - assert(rdpkey_reg() > orig_pkey_reg);
> + assert(rdpkey_reg() < orig_pkey_reg);
>  }
>  
>  void pkey_write_allow(int pkey)

This seems so horribly wrong that I wonder how it worked in the first
place.  Any idea?

[PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()

[Ram Pai]

instead of clearing the bits, pkey_disable_clear() was setting
the bits. Fixed it.

Also fixed a wrong assertion in that function. When bits are
cleared, the resulting bit value will be less than the original.

cc: Dave Hansen 
cc: Florian Weimer 
Signed-off-by: Ram Pai 
---
 tools/testing/selftests/vm/protection_keys.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/vm/protection_keys.c 
b/tools/testing/selftests/vm/protection_keys.c
index 0109388..ca54a95 100644
--- a/tools/testing/selftests/vm/protection_keys.c
+++ b/tools/testing/selftests/vm/protection_keys.c
@@ -461,7 +461,7 @@ void pkey_disable_clear(int pkey, int flags)
pkey, pkey, pkey_rights);
pkey_assert(pkey_rights >= 0);
 
-   pkey_rights |= flags;
+   pkey_rights &= ~flags;
 
ret = pkey_set(pkey, pkey_rights, 0);
/* pkey_reg and flags have the same format */
@@ -475,7 +475,7 @@ void pkey_disable_clear(int pkey, int flags)
dprintf1("%s(%d) pkey_reg: 0x%016lx\n", __func__,
pkey, rdpkey_reg());
if (flags)
-   assert(rdpkey_reg() > orig_pkey_reg);
+   assert(rdpkey_reg() < orig_pkey_reg);
 }
 
 void pkey_write_allow(int pkey)
-- 
1.7.1