Re: [PATCH v1] mm: More ptep_get() conversion

[Ryan Roberts <ryan.roberts@arm.com>]

On 14/11/2023 16:30, Matthew Wilcox wrote:
> On Tue, Nov 14, 2023 at 03:49:45PM +0000, Ryan Roberts wrote:
>> Commit c33c794828f2 ("mm: ptep_get() conversion") converted all
>> (non-arch) call sites to use ptep_get() instead of doing a direct
>> dereference of the pte. Full rationale can be found in that commit's
>> log.
>>
>> Since then, three new call sites have snuck in, which directly
>> dereference the pte, so let's fix those up.
>>
>> Unfortunately there is no reliable automated mechanism to catch these;
>> I'm relying on a combination of Coccinelle (which throws up a lot of
>> false positives) and some compiler magic to force a compiler error on
>> dereference (While this approach finds dereferences, it also yields a
>> non-booting kernel so can't be committed).
> 
> Well ... let's see what we can come up with.
> 
> struct raw_pte {
> 	pte_t pte;
> };

pte_t is already a wrapper around the real value, at least on arm64:

typedef struct { pteval_t pte; } pte_t;

So doesn't adding extra wrapper just suggest that next year we will end up
adding a third, then a fourth...?

Fundamentally people can still just do pte->pte to dereference.


The approach I took with the compiler magic I describe above was to pass around:

typedef void* pte_handle_t;

which is just a pointer to pte_t, but you can't deref without an explcit cast.
So then I insert the explicit casts in the 5 or 6 places in the arm64 arch code
that they are required and it mostly just works. (I have the core patch which is
pretty small, then do find/replace on "pte_t *" -> "pte_handle_t" and it just
works).

But its a LOT of churn in the non-arch code, and leaves the other arches broken,
many of which are dereferencing all over the place  - it would be a huge effort
to fix them all up.

> 
> static inline pte_t ptep_get(struct raw_pte *rpte)
> {
> 	return rpte.pte;
> }
> 
> Probably quite a lot of churn to put that into place, but better than
> a never-ending treadmill of fixing the places that people overlooked?

Yes and no... agree it would be nice to automatically guard against it, but I
didn't want to spend the next 6 months of my life fixing up all the other arches...