Re: [PATCH v3 3/3] kernel/events/uprobes: uprobe_write_opcode() rewrite

[David Hildenbrand <david@redhat.com>]

On 21.03.25 14:05, Jiri Olsa wrote:
> On Fri, Mar 21, 2025 at 12:37:13PM +0100, David Hildenbrand wrote:
> 
> SNIP
> 
>> +static int __uprobe_write_opcode(struct vm_area_struct *vma,
>> +		struct folio_walk *fw, struct folio *folio,
>> +		unsigned long opcode_vaddr, uprobe_opcode_t opcode)
>> +{
>> +	const unsigned long vaddr = opcode_vaddr & PAGE_MASK;
>> +	const bool is_register = !!is_swbp_insn(&opcode);
>> +	bool pmd_mappable;
>> +
>> +	/* For now, we'll only handle PTE-mapped folios. */
>> +	if (fw->level != FW_LEVEL_PTE)
>> +		return -EFAULT;
>> +
>> +	/*
>> +	 * See can_follow_write_pte(): we'd actually prefer a writable PTE here,
>> +	 * but the VMA might not be writable.
>> +	 */
>> +	if (!pte_write(fw->pte)) {
>> +		if (!PageAnonExclusive(fw->page))
>> +			return -EFAULT;
>> +		if (unlikely(userfaultfd_pte_wp(vma, fw->pte)))
>> +			return -EFAULT;
>> +		/* SOFTDIRTY is handled via pte_mkdirty() below. */
>> +	}
>> +
>> +	/*
>> +	 * We'll temporarily unmap the page and flush the TLB, such that we can
>> +	 * modify the page atomically.
>> +	 */
>> +	flush_cache_page(vma, vaddr, pte_pfn(fw->pte));
>> +	fw->pte = ptep_clear_flush(vma, vaddr, fw->ptep);
>> +	copy_to_page(fw->page, opcode_vaddr, &opcode, UPROBE_SWBP_INSN_SIZE);
>> +
>> +	/*
>> +	 * When unregistering, we may only zap a PTE if uffd is disabled and
>> +	 * there are no unexpected folio references ...
>> +	 */
>> +	if (is_register || userfaultfd_missing(vma) ||
>> +	    (folio_ref_count(folio) != folio_mapcount(folio) + 1 +
>> +	     folio_test_swapcache(folio) * folio_nr_pages(folio)))
>> +		goto remap;
>> +
>> +	/*
>> +	 * ... and the mapped page is identical to the original page that
>> +	 * would get faulted in on next access.
>> +	 */
>> +	if (!orig_page_is_identical(vma, vaddr, fw->page, &pmd_mappable))
>> +		goto remap;
>> +
>> +	dec_mm_counter(vma->vm_mm, MM_ANONPAGES);
>> +	folio_remove_rmap_pte(folio, fw->page, vma);
>> +	if (!folio_mapped(folio) && folio_test_swapcache(folio) &&
>> +	     folio_trylock(folio)) {
>> +		folio_free_swap(folio);
>> +		folio_unlock(folio);
>> +	}
>> +	folio_put(folio);
> 
> hi,
> it's probably ok and I'm missing something, but why do we call folio_put
> in here, I'd think it's done by folio_put call in uprobe_write_opcode

Hi!

We're zapping a page table mapping mapping (folio_remove_rmap_pte()), so 
we have to drop that reference.

That's the folio_put(old_folio) in the old __replace_page().

Thanks!

-- 
Cheers,

David / dhildenb