Re: [PATCH v2 2/2] mm: change vma_start_read() to drop RCU lock on failure

[Vlastimil Babka <vbabka@suse.cz>]

On 7/31/25 17:19, Suren Baghdasaryan wrote:
> vma_start_read() can drop and reacquire RCU lock in certain failure
> cases. It's not apparent that the RCU session started by the caller of
> this function might be interrupted when vma_start_read() fails to lock
> the vma. This might become a source of subtle bugs and to prevent that
> we change the locking rules for vma_start_read() to drop RCU read lock
> upon failure. This way it's more obvious that RCU-protected objects are
> unsafe after vma locking fails.
> 
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>

IIRC you considered it yourself, I just convinced you to try :)

> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

I thought we didn't need the drop rcu lock for -EAGAIN, but that would just
made it more complex for little gain, so this looks good to me.

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

Nit:

> @@ -223,11 +227,13 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
>  	MA_STATE(mas, &mm->mm_mt, address, address);
>  	struct vm_area_struct *vma;
>  
> -	rcu_read_lock();
>  retry:
> +	rcu_read_lock();
>  	vma = mas_walk(&mas);
> -	if (!vma)
> +	if (!vma) {
> +		rcu_read_unlock();
>  		goto inval;
> +	}
>  
>  	vma = vma_start_read(mm, vma);
>  	if (IS_ERR_OR_NULL(vma)) {
> @@ -241,6 +247,9 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
>  		/* Failed to lock the VMA */
>  		goto inval;
>  	}
> +
> +	rcu_read_unlock();

Would it make sense to put this under the comment below?

> +
>  	/*
>  	 * At this point, we have a stable reference to a VMA: The VMA is
>  	 * locked and we know it hasn't already been isolated.

Give it continues like this:

         * From here on, we can access the VMA without worrying about which

         * fields are accessible for RCU readers.

> @@ -249,16 +258,14 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
>  	 */
>  
>  	/* Check if the vma we locked is the right one. */
> -	if (unlikely(address < vma->vm_start || address >= vma->vm_end))
> -		goto inval_end_read;
> +	if (unlikely(address < vma->vm_start || address >= vma->vm_end)) {
> +		vma_end_read(vma);
> +		goto inval;
> +	}
>  
> -	rcu_read_unlock();
>  	return vma;
>  
> -inval_end_read:
> -	vma_end_read(vma);
>  inval:
> -	rcu_read_unlock();
>  	count_vm_vma_lock_event(VMA_LOCK_ABORT);
>  	return NULL;
>  }