Re: [PATCH resend] ksm: add merge_only_zero_pages to merge only empty anonymous pages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mm: ksm: unsynchronized state checks in zero-page sysfs store
  handlers
--

commit 9b8576e88012c85cc90e3b5aa732dfb204eefebc
Author: Xu Xin (vivo) <xu.xin@linux.dev>

ksm: add merge_only_zero_pages to merge only empty anonymous pages

This commit adds the /sys/kernel/mm/ksm/merge_only_zero_pages sysfs attribute
so ksmd can merge only zero-filled anonymous pages with the kernel zero page
while skipping the stable and unstable trees for pages carrying data.

> diff --git a/mm/ksm.c b/mm/ksm.c
> index 340c65aa14e58..35ed40747e30e 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c

[ ... ]

> @@ -3679,12 +3695,48 @@ static ssize_t use_zero_pages_store(struct kobject *kobj,
>  	if (err)
>  		return -EINVAL;
>  
> +	/*
> +	 * merge_only_zero_pages depends on use_zero_pages: refuse to turn
> +	 * the latter off while the former is active, so the two cannot
> +	 * silently diverge.
> +	 */
> +	if (!value && ksm_merge_only_zero_pages)
> +		return -EBUSY;
> +
>  	ksm_use_zero_pages = value;
>  
>  	return count;
>  }
>  KSM_ATTR(use_zero_pages);
>  
> +static ssize_t merge_only_zero_pages_show(struct kobject *kobj,
> +					  struct kobj_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%u\n", ksm_merge_only_zero_pages);
> +}
> +static ssize_t merge_only_zero_pages_store(struct kobject *kobj,
> +					   struct kobj_attribute *attr,
> +					   const char *buf, size_t count)
> +{
> +	int err;
> +	bool value;
> +
> +	err = kstrtobool(buf, &value);
> +	if (err)
> +		return -EINVAL;
> +
> +	if (value && !ksm_use_zero_pages) {
> +		pr_warn_once("ksm: merge_only_zero_pages can only be enabled "
> +			     "when use_zero_pages is 1\n");
> +		return -EINVAL;
> +	}
> +
> +	ksm_merge_only_zero_pages = value;

[Severity: Medium]
Can concurrent writes to use_zero_pages and merge_only_zero_pages cause
ksm_use_zero_pages and ksm_merge_only_zero_pages to diverge?

Since these are separate sysfs attributes, kernfs does not serialize
use_zero_pages_store() against merge_only_zero_pages_store(), and neither
handler takes ksm_thread_mutex or another shared lock.

Starting from ksm_use_zero_pages == true and ksm_merge_only_zero_pages ==
false, if one task writes 0 to use_zero_pages while another concurrently
writes 1 to merge_only_zero_pages:

CPU 0                                   CPU 1
use_zero_pages_store()                  merge_only_zero_pages_store()
  if (!value &&                           if (value &&
      ksm_merge_only_zero_pages)              !ksm_use_zero_pages)
    /* sees false */                        /* sees true */
  ksm_use_zero_pages = false;             ksm_merge_only_zero_pages = true;

With ksm_use_zero_pages set to false and ksm_merge_only_zero_pages set to
true, try_to_merge_with_zero_page() always returns -EFAULT and the
subsequent ksm_merge_only_zero_pages check in cmp_and_merge_page() returns
early:

mm/ksm.c:cmp_and_merge_page() {
    ...
		if (!try_to_merge_with_zero_page(rmap_item, page))
			return;

		if (ksm_merge_only_zero_pages)
			return;
    ...
}

Wouldn't this leave ksmd scanning and checksumming pages without ever
merging either zero pages or non-zero pages?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106034841.233085-1-xu.xin@linux.dev?part=1