Re: [PATCH] mm/rmap: Add NULL checks for rmap_walk_control callbacks

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Thu, Jun 19, 2025 at 03:50:40PM +0800, Ye Liu wrote:
> From: Ye Liu <liuye@kylinos.cn>
>
> Add NULL pointer checks for rmap_one callback in rmap_walk operations
> to prevent potential NULL pointer dereferences. Also clean up some
> code by removing redundant comments and caching folio_nr_pages().
>
> Signed-off-by: Ye Liu <liuye@kylinos.cn>

No sorry this patch is no good, none of these changes add any value.

> ---
>  mm/ksm.c  |  2 +-
>  mm/rmap.c | 14 +++++++-------
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 18b3690bb69a..22ad069d1860 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -3068,7 +3068,7 @@ void rmap_walk_ksm(struct folio *folio, struct rmap_walk_control *rwc)
>  			if (rwc->invalid_vma && rwc->invalid_vma(vma, rwc->arg))
>  				continue;
>
> -			if (!rwc->rmap_one(folio, vma, addr, rwc->arg)) {
> +			if (rwc->rmap_one && !rwc->rmap_one(folio, vma, addr, rwc->arg)) {

It's convention that this will be set. If it's not set, a kernel developer did
something wrong, so why the hell are we checking this every single time? And why
if we are are we just fine to do nothing?

This is not a useful change sorry.

>  				anon_vma_unlock_read(anon_vma);
>  				return;
>  			}
> diff --git a/mm/rmap.c b/mm/rmap.c
> index fb63d9256f09..17d43d104a0d 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1202,8 +1202,7 @@ int mapping_wrprotect_range(struct address_space *mapping, pgoff_t pgoff,
>  	if (!mapping)
>  		return 0;
>
> -	__rmap_walk_file(/* folio = */NULL, mapping, pgoff, nr_pages, &rwc,
> -			 /* locked = */false);
> +	__rmap_walk_file(NULL, mapping, pgoff, nr_pages, &rwc, false);

Please do not remove /* x = */ prefixes. This is not redundant in the slightest
and aids readability so you know what's being referred to.

>
>  	return state.cleaned;
>  }
> @@ -2806,6 +2805,7 @@ static void rmap_walk_anon(struct folio *folio,
>  	struct anon_vma *anon_vma;
>  	pgoff_t pgoff_start, pgoff_end;
>  	struct anon_vma_chain *avc;
> +	unsigned long nr_pages;

I don't think there's much value in adding this at this point to be honest. It's
not expensive, the compiler knows what to do and there's two invocations.

There's a trade off with noise here.

>
>  	if (locked) {
>  		anon_vma = folio_anon_vma(folio);
> @@ -2817,13 +2817,13 @@ static void rmap_walk_anon(struct folio *folio,
>  	if (!anon_vma)
>  		return;
>
> +	nr_pages = folio_nr_pages(folio);
>  	pgoff_start = folio_pgoff(folio);
> -	pgoff_end = pgoff_start + folio_nr_pages(folio) - 1;
> +	pgoff_end = pgoff_start + nr_pages - 1;
>  	anon_vma_interval_tree_foreach(avc, &anon_vma->rb_root,
>  			pgoff_start, pgoff_end) {
>  		struct vm_area_struct *vma = avc->vma;
> -		unsigned long address = vma_address(vma, pgoff_start,
> -				folio_nr_pages(folio));
> +		unsigned long address = vma_address(vma, pgoff_start, nr_pages);
>
>  		VM_BUG_ON_VMA(address == -EFAULT, vma);
>  		cond_resched();
> @@ -2831,7 +2831,7 @@ static void rmap_walk_anon(struct folio *folio,
>  		if (rwc->invalid_vma && rwc->invalid_vma(vma, rwc->arg))
>  			continue;
>
> -		if (!rwc->rmap_one(folio, vma, address, rwc->arg))
> +		if (rwc->rmap_one && !rwc->rmap_one(folio, vma, address, rwc->arg))

Same comment as with ksm.

>  			break;
>  		if (rwc->done && rwc->done(folio))
>  			break;
> @@ -2894,7 +2894,7 @@ static void __rmap_walk_file(struct folio *folio, struct address_space *mapping,
>  		if (rwc->invalid_vma && rwc->invalid_vma(vma, rwc->arg))
>  			continue;
>
> -		if (!rwc->rmap_one(folio, vma, address, rwc->arg))
> +		if (rwc->rmap_one && !rwc->rmap_one(folio, vma, address, rwc->arg))

Same comment as with ksm.

>  			goto done;
>  		if (rwc->done && rwc->done(folio))
>  			goto done;
> --
> 2.25.1
>