Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1567034

Re: [PATCH v5 4/4] sparc64: Add support for ADI (Application Data Integrity)

Show all headers | View raw

On 01/25/2017 03:20 PM, Khalid Aziz wrote:
> On 01/25/2017 03:00 PM, Rob Gardner wrote:
>> On 01/25/2017 12:57 PM, Khalid Aziz wrote:
>>>
>>> @@ -157,6 +158,24 @@ int __get_user_pages_fast(unsigned long start,
>>> int nr_pages, int write,
>>>       pgd_t *pgdp;
>>>       int nr = 0;
>>>   +#ifdef CONFIG_SPARC64
>>> +    if (adi_capable()) {
>>> +        long addr = start;
>>> +
>>> +        /* If userspace has passed a versioned address, kernel
>>> +         * will not find it in the VMAs since it does not store
>>> +         * the version tags in the list of VMAs. Storing version
>>> +         * tags in list of VMAs is impractical since they can be
>>> +         * changed any time from userspace without dropping into
>>> +         * kernel. Any address search in VMAs will be done with
>>> +         * non-versioned addresses. Ensure the ADI version bits
>>> +         * are dropped here by sign extending the last bit before
>>> +         * ADI bits. IOMMU does not implement version tags.
>>> +         */
>>> +        addr = (addr << (long)adi_nbits()) >> (long)adi_nbits();
>>
>>
>> So you are depending on the sign extension to clear the ADI bits... but
>> this only happens if there is a zero in that "last bit before ADI bits".
>> If the last bit is a 1, then the ADI bits will be set instead of
>> cleared.  That seems like an unintended consequence given the comment. I
>> am aware of the value of adi_nbits() and of the number of valid bits in
>> a virtual address on the M7 processor, but wouldn't using 'unsigned
>> long' for everything here guarantee the ADI bits get cleared regardless
>> of the state of the last non-adi bit?
>
> Sign extension is the right thing to do. MMU considers values of 0 and 
> 15 for bits 63-60 to be untagged addresses and expects bit 59 to be 
> sign-extended for untagged virtual addresses. The code I added is 
> explicitly meant to sign-extend, not zero out the top 4 bits.

OK, that wasn't perfectly clear from the comment, which said "version 
bits are dropped".

So sign extending will produce an address that the MMU can use, but will 
it produce an address that will allow a successful search in the page 
tables? ie, was this same sign extending done when first handing out 
that virtual address to the user?

Rob

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v5 0/4] Application Data Integrity feature introduced by SPARC M7 Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-25 21:00 +0100
  [PATCH v5 1/4] signals, sparc: Add signal codes for ADI violations Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-25 21:00 +0100
  [PATCH v5 3/4] sparc64: Add support for ADI register fields, ASIs and traps Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-25 21:00 +0100
  Re: [PATCH v5 4/4] sparc64: Add support for ADI (Application Data  Integrity) David Miller <davem@davemloft.net> - 2017-01-25 23:20 +0100
  Re: [PATCH v5 4/4] sparc64: Add support for ADI (Application Data  Integrity) Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-25 23:30 +0100
    Re: [PATCH v5 4/4] sparc64: Add support for ADI (Application Data  Integrity) Rob Gardner <rob.gardner@oracle.com> - 2017-01-26 00:00 +0100
      Re: [PATCH v5 4/4] sparc64: Add support for ADI (Application Data  Integrity) Khalid Aziz <khalid.aziz@oracle.com> - 2017-01-26 00:00 +0100

csiph-web