Re: [PATCH v2 1/2] mm/page_reporting: Add checks for page_reporting_order param

[Wei Liu <wei.liu@kernel.org>]

On Thu, Sep 29, 2022 at 11:01:38PM -0700, Shradha Gupta wrote:
> Current code allows the page_reporting_order parameter to be changed
> via sysfs to any integer value.  The new value is used immediately
> in page reporting code with no validation, which could cause incorrect
> behavior.  Fix this by adding validation of the new value.
> Export this parameter for use in the driver that is calling the
> page_reporting_register().
> This is needed by drivers like hv_balloon to know the order of the
> pages reported. Traditionally the values provided in the kernel boot
> line or subsequently changed via sysfs take priority therefore, if
> page_reporting_order parameter's value is set, it takes precedence
> over the value passed while registering with the driver.
> 
> Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com>

Andrew and other MM reviewers, can I get an ack / nack for this patch?

> ---
>  mm/page_reporting.c | 50 ++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/page_reporting.c b/mm/page_reporting.c
> index 382958eef8a9..29d67c824fd2 100644
> --- a/mm/page_reporting.c
> +++ b/mm/page_reporting.c
> @@ -11,10 +11,42 @@
>  #include "page_reporting.h"
>  #include "internal.h"
>  
> -unsigned int page_reporting_order = MAX_ORDER;
> -module_param(page_reporting_order, uint, 0644);
> +/* Initialize to an unsupported value */
> +unsigned int page_reporting_order = -1;
> +
> +int page_order_update_notify(const char *val, const struct kernel_param *kp)
> +{
> +	/*
> +	 * If param is set beyond this limit, order is set to default
> +	 * pageblock_order value
> +	 */
> +	return  param_set_uint_minmax(val, kp, 0, MAX_ORDER-1);
> +}
> +
> +const struct kernel_param_ops page_reporting_param_ops = {
> +	.set = &page_order_update_notify,
> +	/*
> +	 * For the get op, use param_get_int instead of param_get_uint.
> +	 * This is to make sure that when unset the initialized value of
> +	 * -1 is shown correctly
> +	 */
> +	.get = &param_get_int,
> +};
> +
> +module_param_cb(page_reporting_order, &page_reporting_param_ops,
> +			&page_reporting_order, 0644);
>  MODULE_PARM_DESC(page_reporting_order, "Set page reporting order");
>  
> +/*
> + * This symbol is also a kernel parameter. Export the page_reporting_order
> + * symbol so that other drivers can access it to control order values without
> + * having to introduce another configurable parameter. Only one driver can
> + * register with the page_reporting driver for the service, so we have just
> + * one control parameter for the use case(which can be accessed in both
> + * drivers)
> + */
> +EXPORT_SYMBOL_GPL(page_reporting_order);
> +
>  #define PAGE_REPORTING_DELAY	(2 * HZ)
>  static struct page_reporting_dev_info __rcu *pr_dev_info __read_mostly;
>  
> @@ -330,10 +362,18 @@ int page_reporting_register(struct page_reporting_dev_info *prdev)
>  	}
>  
>  	/*
> -	 * Update the page reporting order if it's specified by driver.
> -	 * Otherwise, it falls back to @pageblock_order.
> +	 * If the page_reporting_order value is not set, we check if
> +	 * an order is provided from the driver that is performing the
> +	 * registration. If that is not provided either, we default to
> +	 * pageblock_order.
>  	 */
> -	page_reporting_order = prdev->order ? : pageblock_order;
> +
> +	if (page_reporting_order == -1) {
> +		if (prdev->order > 0 && prdev->order <= MAX_ORDER)
> +			page_reporting_order = prdev->order;
> +		else
> +			page_reporting_order = pageblock_order;
> +	}
>  
>  	/* initialize state and work structures */
>  	atomic_set(&prdev->state, PAGE_REPORTING_IDLE);
> -- 
> 2.37.2
>