Date:   Fri, 4 Feb 2022 10:35:59 -0800
From:   Dan Williams <dan.j.williams@...el.com>
To:     "Weiny, Ira" <ira.weiny@...el.com>
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Rick Edgecombe <rick.p.edgecombe@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V8 38/44] memremap_pages: Define pgmap_mk_{readwrite|noaccess}()
 calls

On Thu, Jan 27, 2022 at 9:55 AM <ira.weiny@...el.com> wrote:
>
> From: Ira Weiny <ira.weiny@...el.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@...el.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
>

Powered by blists - more mailing lists