Re: [PATCH] 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: race between use_zero_pages_store() and
  merge_only_zero_pages_store()
--

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

ksm: add merge_only_zero_pages to merge only empty anonymous pages

This commit adds a new sysfs attribute, /sys/kernel/mm/ksm/merge_only_zero_pages,
which causes ksmd to merge only zeroed anonymous pages with the kernel zero
page and skip the stable and unstable trees for non-zero pages.

> 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;
> +
> +	return count;
> +}

[Severity: Medium]
Can concurrent writes to use_zero_pages and merge_only_zero_pages race with
each other and leave ksm_use_zero_pages set to false while
ksm_merge_only_zero_pages is set to true?

Neither use_zero_pages_store() nor merge_only_zero_pages_store() acquires
ksm_thread_mutex or another shared lock, and kernfs_fop_write_iter() only
serializes writes per open file description. Starting with
ksm_use_zero_pages == true and ksm_merge_only_zero_pages == false, two
concurrent sysfs writes can interleave:

CPU0                                  CPU1
use_zero_pages_store()                merge_only_zero_pages_store()
  if (!value &&
      ksm_merge_only_zero_pages)
    // sees false, passes check
                                        if (value && !ksm_use_zero_pages)
                                          // sees true, passes check
  ksm_use_zero_pages = false;
                                        ksm_merge_only_zero_pages = true;

If that state is reached, cmp_and_merge_page() will see
try_to_merge_with_zero_page() fail with -EFAULT because ksm_use_zero_pages is
false, and then immediately return early at:

	if (!try_to_merge_with_zero_page(rmap_item, page))
		return;

	if (ksm_merge_only_zero_pages)
		return;

causing ksmd to scan and checksum pages without ever merging either zero
pages or non-zero pages.

Should use_zero_pages_store() and merge_only_zero_pages_store() serialize
these checks and updates under ksm_thread_mutex?

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