Re: [PATCH V8 38/44] memremap_pages: Define pgmap_mk_{readwrite|noaccess}() calls

Dan Williams
2022-02-04T18:36:26Z

On Thu, Jan 27, 2022 at 9:55 AM <ira.weiny@intel.com> wrote:
>
> From: Ira Weiny <ira.weiny@intel.com>
>
> Users will need a way to flag valid access to pages which have been
> protected with PGMAP protections. Provide this by defining pgmap_mk_*()
> accessor functions.

I find the ambiguous use of "Users" not helpful to set the context. How about:

A thread that wants to access memory protected by PGMAP protections
must first enable access, and then disable access when it is done.

>
> pgmap_mk_{readwrite|noaccess}() take a struct page for convenience.
> They determine if the page is protected by dev_pagemap protections. If
> so, they perform the requested operation.
>
> In addition, the lower level __pgmap_* functions are exported. They
> take the dev_pagemap object directly for internal users who have
> knowledge of the of the dev_pagemap.
>
> All changes in the protections must be through the above calls. They
> abstract the protection implementation (currently the PKS api) from the
> upper layer users.
>
> Furthermore, the calls are nestable by the use of a per task reference
> count. This ensures that the first call to re-enable protection does
> not 'break' the last access of the device memory.
>
> Access to device memory during exceptions (#PF) is expected only from
> user faults. Therefore there is no need to maintain the reference count
> when entering or exiting exceptions. However, reference counting will
> occur during the exception. Recall that protection is automatically
> enabled during exceptions by the PKS core.[1]
>
> NOTE: It is not anticipated that any code paths will directly nest these
> calls. For this reason multiple reviewers, including Dan and Thomas,
> asked why this reference counting was needed at this level rather than
> in a higher level call such as kmap_{atomic,local_page}(). The reason
> is that pgmap_mk_readwrite() could nest with regards to other callers of
> pgmap_mk_*() such as kmap_{atomic,local_page}(). Therefore push this
> reference counting to the lower level and just ensure that these calls
> are nestable.

I still don't think that explains why task struct has a role to play
here, see below.

Another missing bit of clarification, maybe I missed it, is why are
the protections toggled between read-write and noaccess. For
stray-write protection toggling between read-write and read-only is
sufficient. I can imagine speculative execution and debug rationales
for noaccess, but those should be called out explicitly.

>
> [1] https://lore.kernel.org/lkml/20210401225833.566238-9-ira.weiny@intel.com/
>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>
> ---
> Changes for V8
> Split these functions into their own patch.
> This helps to clarify the commit message and usage.
> ---
> include/linux/mm.h | 34 ++++++++++++++++++++++++++++++++++
> include/linux/sched.h | 7 +++++++
> init/init_task.c | 3 +++
> mm/memremap.c | 14 ++++++++++++++
> 4 files changed, 58 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 6e4a2758e3d3..60044de77c54 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1162,10 +1162,44 @@ static inline bool devmap_protected(struct page *page)
> return false;
> }
>
> +void __pgmap_mk_readwrite(struct dev_pagemap *pgmap);
> +void __pgmap_mk_noaccess(struct dev_pagemap *pgmap);
> +
> +static inline bool pgmap_check_pgmap_prot(struct page *page)
> +{
> + if (!devmap_protected(page))
> + return false;
> +
> + /*
> + * There is no known use case to change permissions in an irq for pgmap
> + * pages
> + */
> + lockdep_assert_in_irq();
> + return true;
> +}
> +
> +static inline void pgmap_mk_readwrite(struct page *page)
> +{
> + if (!pgmap_check_pgmap_prot(page))
> + return;
> + __pgmap_mk_readwrite(page->pgmap);
> +}
> +static inline void pgmap_mk_noaccess(struct page *page)
> +{
> + if (!pgmap_check_pgmap_prot(page))
> + return;
> + __pgmap_mk_noaccess(page->pgmap);
> +}
> +
> bool pgmap_protection_available(void);
>
> #else
>
> +static inline void __pgmap_mk_readwrite(struct dev_pagemap *pgmap) { }
> +static inline void __pgmap_mk_noaccess(struct dev_pagemap *pgmap) { }
> +static inline void pgmap_mk_readwrite(struct page *page) { }
> +static inline void pgmap_mk_noaccess(struct page *page) { }
> +
> static inline bool pgmap_protection_available(void)
> {
> return false;
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index f5b2be39a78c..5020ed7e67b7 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1492,6 +1492,13 @@ struct task_struct {
> struct callback_head l1d_flush_kill;
> #endif
>
> +#ifdef CONFIG_DEVMAP_ACCESS_PROTECTION
> + /*
> + * NOTE: pgmap_prot_count is modified within a single thread of
> + * execution. So it does not need to be atomic_t.
> + */
> + u32 pgmap_prot_count;
> +#endif

It's not at all clear why the task struct needs to be burdened with
this accounting. Given that a devmap instance is needed to manage page
protections, why not move the nested protection tracking to a percpu
variable relative to an @pgmap arg? Something like:

void __pgmap_mk_readwrite(struct dev_pagemap *pgmap)
{
migrate_disable();
preempt_disable();
if (this_cpu_add_return(pgmap->pgmap_prot_count, 1) == 1)
pks_mk_readwrite(PKS_KEY_PGMAP_PROTECTION);
}
EXPORT_SYMBOL_GPL(__pgmap_mk_readwrite);

void __pgmap_mk_noaccess(struct dev_pagemap *pgmap)
{
if (!this_cpu_sub_return(pgmap->pgmap_prot_count, 1))
pks_mk_noaccess(PKS_KEY_PGMAP_PROTECTION);
preempt_enable();
migrate_enable();
}
EXPORT_SYMBOL_GPL(__pgmap_mk_noaccess);

The naming, which I had a hand in, is not aging well. When I see "mk"
I expect it to be building some value like a page table entry that
will be installed later. These helpers are directly enabling and
disabling access and are meant to be called symmetrically. So I would
expect symmetric names like:

pgmap_enable_access()
pgmap_disable_access()

> /*
> * New fields for task_struct should be added above here, so that
> * they are included in the randomized portion of task_struct.
> diff --git a/init/init_task.c b/init/init_task.c
> index 73cc8f03511a..948b32cf8139 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -209,6 +209,9 @@ struct task_struct init_task
> #ifdef CONFIG_SECCOMP_FILTER
> .seccomp = { .filter_count = ATOMIC_INIT(0) },
> #endif
> +#ifdef CONFIG_DEVMAP_ACCESS_PROTECTION
> + .pgmap_prot_count = 0,
> +#endif
> };
> EXPORT_SYMBOL(init_task);
>
> diff --git a/mm/memremap.c b/mm/memremap.c
> index d3e6f328a711..b75c4f778c59 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -96,6 +96,20 @@ static void devmap_protection_disable(void)
> static_branch_dec(&dev_pgmap_protection_static_key);
> }
>
> +void __pgmap_mk_readwrite(struct dev_pagemap *pgmap)
> +{
> + if (!current->pgmap_prot_count++)
> + pks_mk_readwrite(PKS_KEY_PGMAP_PROTECTION);
> +}
> +EXPORT_SYMBOL_GPL(__pgmap_mk_readwrite);
> +
> +void __pgmap_mk_noaccess(struct dev_pagemap *pgmap)
> +{
> + if (!--current->pgmap_prot_count)
> + pks_mk_noaccess(PKS_KEY_PGMAP_PROTECTION);
> +}
> +EXPORT_SYMBOL_GPL(__pgmap_mk_noaccess);
> +
> bool pgmap_protection_available(void)
> {
> return pks_available();
> --
> 2.31.1
>