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

[David Rientjes <rientjes@google.com>]

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().

If we remove the pfn, feel free to add

	Acked-by: David Rientjes <rientjes@google.com>

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