Re: [PATCH v11 00/10] Application Data Integrity feature introduced by SPARC M7

[Eric W. Biederman]

Khalid Aziz <khalid.aziz@xxxxxxxxxx> writes:

> On 02/07/2018 12:38 AM, ebiederm@xxxxxxxxxxxx wrote:
>> Khalid Aziz <khalid.aziz@xxxxxxxxxx> writes:
>>
>>> On 02/01/2018 07:29 PM, ebiederm@xxxxxxxxxxxx wrote:
>>>> Khalid Aziz <khalid.aziz@xxxxxxxxxx> writes:
>>>>
>>>>> V11 changes:
>>>>> This series is same as v10 and was simply rebased on 4.15 kernel. Can
>>>>> mm maintainers please review patches 2, 7, 8 and 9 which are arch
>>>>> independent, and include/linux/mm.h and mm/ksm.c changes in patch 10
>>>>> and ack these if everything looks good?
>>>>
>>>> I am a bit puzzled how this differs from the pkey's that other
>>>> architectures are implementing to achieve a similar result.
>>>>
>>>> I am a bit mystified why you don't store the tag in a vma
>>>> instead of inventing a new way to store data on page out.
>>>
>>> Hello Eric,
>>>
>>> As Steven pointed out, sparc sets tags per cacheline unlike pkey. This results
>>> in much finer granularity for tags that pkey and hence requires larger tag
>>> storage than what we can do in a vma.
>>
>> *Nod*   I am a bit mystified where you keep the information in memory.
>> I would think the tags would need to be stored per cacheline or per
>> tlb entry, in some kind of cache that could overflow.  So I would be
>> surprised if swapping is the only time this information needs stored
>> in memory.  Which makes me wonder if you have the proper data
>> structures.
>>
>> I would think an array per vma or something in the page tables would
>> tend to make sense.
>>
>> But perhaps I am missing something.
>
> The ADI tags are stored in spare bits in the RAM. ADI tag storage is
> managed entirely by memory controller which maintains these tags per
> ADI block. An ADI block is the same size as cacheline on M7. Tags for
> each ADI block are associated with the physical ADI block, not the
> virtual address. When a physical page is reused, the physical ADI tag
> storage for that page is overwritten with new ADI tags, hence we need
> to store away the tags when we swap out a page. Kernel updates the ADI
> tags for physical page when it swaps a new page in. Each vma can cover
> variable number of pages so it is best to store a pointer to the tag
> storage in vma as opposed to actual tags in an array. Each 8K page can
> have 128 tags on it. Since each tag is 4 bits, we need 64 bytes per
> page to store the tags. That can add up for a large vma.

If the tags are already stored in RAM I can see why it does not make any
sense to store them except on page out.  Management wise this feels a
lot like the encrypted memory options I have been seeing on x86.

>>>> Can you please use force_sig_fault to send these signals instead
>>>> of force_sig_info.  Emperically I have found that it is very
>>>> error prone to generate siginfo's by hand, especially on code
>>>> paths where several different si_codes may apply.  So it helps
>>>> to go through a helper function to ensure the fiddly bits are
>>>> all correct.  AKA the unused bits all need to be set to zero before
>>>> struct siginfo is copied to userspace.
>>>>
>>>
>>> What you say makes sense. I followed the same code as other fault handlers for
>>> sparc. I could change just the fault handlers for ADI related faults. Would it
>>> make more sense to change all the fault handlers in a separate patch and keep
>>> the code in arch/sparc/kernel/traps_64.c consistent? Dave M, do you have a
>>> preference?
>>
>> It is my intention post -rc1 to start sending out patches to get the
>> rest of not just sparc but all of the architectures using the new
>> helpers.  I have the code I just ran out of time befor the merge
>> window opened to ensure everything had a good thorough review.
>>
>> So if you can handle the your new changes I expect I will handle the
>> rest.
>>
>
> I can add a patch at the end of my series to update all
> force_sig_info() in my patchset to force_sig_fault(). That will sync
> my patches up with your changes cleanly. Does that work for you? I can
> send an updated series with this change. Can you review and ack the
> patches after this change.

One additional patch would be fine.  I can certainly review and ack that
part.  You probably want to wait until post -rc1 so that you have a
clean base to work off of.

Eric