Re: [PATCH v6 2/4] mm/memory-failure: userspace controls soft-offlining pages

[Jiaqi Yan <jiaqiyan@google.com>]

On Thu, Jun 27, 2024 at 8:27 PM David Rientjes <rientjes@google.com> wrote:
>
> On Wed, 26 Jun 2024, Jiaqi Yan wrote:
>
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index 6f5ac334efba..1559e773537f 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> > @@ -68,6 +68,8 @@ static int sysctl_memory_failure_early_kill __read_mostly;
> >
> >  static int sysctl_memory_failure_recovery __read_mostly = 1;
> >
> > +static int sysctl_enable_soft_offline __read_mostly = 1;
> > +
> >  atomic_long_t num_poisoned_pages __read_mostly = ATOMIC_LONG_INIT(0);
> >
> >  static bool hw_memory_failure __read_mostly = false;
> > @@ -141,6 +143,15 @@ static struct ctl_table memory_failure_table[] = {
> >               .extra1         = SYSCTL_ZERO,
> >               .extra2         = SYSCTL_ONE,
> >       },
> > +     {
> > +             .procname       = "enable_soft_offline",
> > +             .data           = &sysctl_enable_soft_offline,
> > +             .maxlen         = sizeof(sysctl_enable_soft_offline),
> > +             .mode           = 0644,
> > +             .proc_handler   = proc_dointvec_minmax,
> > +             .extra1         = SYSCTL_ZERO,
> > +             .extra2         = SYSCTL_ONE,
> > +     }
> >  };
> >
> >  /*
> > @@ -2749,8 +2760,9 @@ static int soft_offline_in_use_page(struct page *page)
> >   * @pfn: pfn to soft-offline
> >   * @flags: flags. Same as memory_failure().
> >   *
> > - * Returns 0 on success
> > - *         -EOPNOTSUPP for hwpoison_filter() filtered the error event
> > + * Returns 0 on success,
> > + *         -EOPNOTSUPP for hwpoison_filter() filtered the error event, or
> > + *         disabled by /proc/sys/vm/enable_soft_offline,
> >   *         < 0 otherwise negated errno.
> >   *
> >   * Soft offline a page, by migration or invalidation,
> > @@ -2786,6 +2798,13 @@ int soft_offline_page(unsigned long pfn, int flags)
> >               return -EIO;
> >       }
> >
> > +     if (!sysctl_enable_soft_offline) {
> > +             pr_info_once("%#lx: disabled by /proc/sys/vm/enable_soft_offline\n",
> > +                     pfn);
>
> Any strong reason to include the pfn in the log message?
>
> I'm concerned about allowing a user to deduce the physical mapping for any
> arbitrary page since this is possible to do through MADV_SOFT_OFFLINE and
> I don't think that it adds value, especially if this is pr_info_once().

Agreed printing pfn value doesn't add value. Will get rid of it in v7.

>
> If we remove the pfn, feel free to add
>
>         Acked-by: David Rientjes <rientjes@google.com>

Thanks David!

>
> > +             put_ref_page(pfn, flags);
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> >       mutex_lock(&mf_mutex);
> >
> >       if (PageHWPoison(page)) {
> > --
> > 2.45.2.741.gdbec12cfda-goog
> >
> >