Re: [PATCH] mm/madvise: preserve uprobe breakpoints across MADV_DONTNEED

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect VMA classification using vma->vm_file instead of vma_is_anonymous()
- [High] VMA addresses used as boolean flags (0 used as 'not found' sentinel)
- [High] Ignored return value in the slow path of madvise_dontneed_free() silently swallows errors
--

commit 0e33540d810d6a2eadc17b982d88ac4970eca760
Author: Darko Tominac <dtominac@cisco.com>

mm/madvise: preserve uprobe breakpoints across MADV_DONTNEED

This patch modifies MADV_DONTNEED and MADV_FREE behavior to prevent discarding
file-backed pages that contain active uprobe software breakpoints. It adds
checks to identify if a VMA contains uprobes and handles them via a slow path
that skips zapping the specific pages containing breakpoints.

Cc: xe-linux-external@cisco.com
Cc: danielwa@cisco.com
Signed-off-by: Darko Tominac <dtominac@cisco.com>

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 4084e926e284..0f8aea99b96f 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
[ ... ]
> @@ -1654,6 +1675,60 @@ vma_has_uprobes(struct vm_area_struct *vma, unsigned long start, unsigned long e
>  	return !!n;
>  }
>  
> +/**
> + * vma_first_uprobe_addr - find first uprobe in a vma range.
> + * @vma: the vma to search.
> + * @start: start address of the range (inclusive).
> + * @end: end address of the range (exclusive).
> + *
> + * Used by madvise to skip directly to uprobe pages.
> + *
> + * Return: the page-aligned virtual address of the first uprobe in
> + * [@start, @end), or 0 if none exists.
> + */
> +unsigned long vma_first_uprobe_addr(struct vm_area_struct *vma,
> +				    unsigned long start, unsigned long end)
> +{
> +	loff_t min, max, first_offset;
> +	struct inode *inode;
> +	struct rb_node *n, *t;
> +	struct uprobe *u;
> +
> +	/* No uprobes possible on anonymous mappings */
> +	if (!vma->vm_file)
> +		return 0;

Should this use !vma_is_anonymous(vma) instead of checking vma->vm_file?

Since private anonymous mappings of /dev/zero have vm_file != NULL but are
otherwise treated as anonymous, won't this incorrectly classify them as
file-backed and unnecessarily proceed with uprobe checks?

Also, since valid virtual addresses (like vm_start) can legitimately be zero,
is it safe to use 0 as a sentinel value for not found? If a uprobe is
installed at virtual address 0, it seems this would incorrectly signal that
no uprobe was found.

[ ... ]
> diff --git a/mm/madvise.c b/mm/madvise.c
> index ce238dd96f15..c9d3c26efd74 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
[ ... ]
> @@ -947,12 +972,46 @@ static long madvise_dontneed_free(struct madvise_behavior *madv_behavior)
>  		VM_WARN_ON(range->start > range->end);
>  	}
>  
> -	if (behavior == MADV_DONTNEED || behavior == MADV_DONTNEED_LOCKED)
> -		return madvise_dontneed_single_vma(madv_behavior);
> -	else if (behavior == MADV_FREE)
> -		return madvise_free_single_vma(madv_behavior);
> -	else
> -		return -EINVAL;
> +	/*
> +	 * Preserve uprobes: if any uprobes are active in this VMA range,
> +	 * avoid discarding pages that contain active breakpoints.
> +	 *
> +	 * Fast path: if no uprobes are registered system-wide, or the VMA
> +	 * is not file-backed (uprobes only instrument file-backed mappings,
> +	 * so anonymous VMAs can never contain breakpoints), or no uprobes
> +	 * are present in this VMA range, proceed with the full operation.
> +	 */
> +	if (likely(!any_uprobes_registered()) ||
> +	    !madv_behavior->vma->vm_file ||

Similar to the question above, should this use vma_is_anonymous() instead
of checking vm_file, to avoid erroneously taking the slow path for private
/dev/zero mappings?

> +	    !vma_has_uprobes(madv_behavior->vma, range->start, range->end))
> +		return madvise_dontneed_free_range(madv_behavior,
> +						   range->start, range->end);
> +
> +	/*
> +	 * Slow path: jump from uprobe to uprobe via rbtree lookup, zapping
> +	 * the clean range before each uprobe page. This is O(M * log N)
> +	 * where M is the number of uprobes in the range and N is the total
> +	 * uprobe count, versus O(pages) for a page-by-page scan. 'cur'
> +	 * tracks the beginning of the current clean range.
> +	 */
> +	cur = range->start;
> +	end = range->end;
> +	while (cur < end) {
> +		uprobe_addr = vma_first_uprobe_addr(madv_behavior->vma,
> +						    cur, end);
> +		if (!uprobe_addr) {

If uprobe_addr is 0 (a potentially valid virtual address), does this loop
terminate early and incorrectly zap the uprobe at address 0?

> +			/* No more uprobes - zap the rest */
> +			madvise_dontneed_free_range(madv_behavior, cur, end);

Does this code need to check the return value of madvise_dontneed_free_range()?

If madvise(MADV_FREE) is called on a file-backed VMA with uprobes, the fast
path would return -EINVAL. However, because the return value here is ignored,
won't the slow path unconditionally return 0 and silently swallow the error?

> +			break;
> +		}
> +		/* Zap the clean range before the uprobe page */
> +		if (cur < uprobe_addr)
> +			madvise_dontneed_free_range(madv_behavior, cur,
> +						    uprobe_addr);

Does this call also need its return value checked to prevent swallowing errors?

> +		/* Skip past the uprobe page */
> +		cur = uprobe_addr + PAGE_SIZE;
> +	}
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429131522.4049054-1-dtominac@cisco.com?part=1