Re: [PATCH] prctl: Get private anonymous memory region name

[Rong Tao]

On 11/25/23 23:56, Oleg Nesterov wrote:
> On 11/25, Rong Tao wrote:
> > +static int prctl_get_vma(unsigned long opt, unsigned long addr,
> > +			 unsigned long buf, unsigned long arg)
> > +{
> > +	struct mm_struct *mm = current->mm;
> > +	const char __user *u_buf;
> > +	int error;
> > +
> > +	switch (opt) {
> > +	case PR_GET_VMA_ANON_NAME:
> > +		const struct anon_vma_name *anon_name = NULL;
> > +
> > +		u_buf = (const char __user *)buf;
> > +		error = 0;
> > +
> > +		mmap_read_lock(mm);
> > +		anon_name = madvise_get_anon_name(mm, addr);
> > +		if (!anon_name) {
> > +			mmap_read_unlock(mm);
> > +			error = -EFAULT;
> may be another error code makes sense to distinguish this case from
> the copy_to_user() failure?
>
> > +			break;
> > +		}
> > +
> > +		if (copy_to_user((char __user *)u_buf, anon_name->name,
> > +				 strlen(anon_name->name) + 1))
> > +			error = -EFAULT;
> and I guess you can simplify this code a bit,
>
> 		anon_name = madvise_get_anon_name(...);
> 		if (!anon_name || copy_to_user(...))
> 			error = -EFAULT;
Thanks a lot, I just modify the code, please review.
> 		
> 		mmap_read_unlock(mm);
> 		anon_vma_name_put(anon_name); // safe if anon_name == NULL;
>
> > +const struct anon_vma_name *madvise_get_anon_name(struct mm_struct *mm,
> > +						  unsigned long start)
> > +{
> > +	struct vm_area_struct *vma;
> > +	struct anon_vma_name *anon_name;
> > +
> > +	vma = find_vma(mm, start);
> > +	if (vma) {
> > +		anon_name = anon_vma_name(vma);
> > +		if (anon_name) {
> > +			anon_vma_name_get(anon_name);
> > +			return anon_name;
> > +		}
> > +	}
> > +
> > +	return NULL;
> Again, afaics this can be simplified,
>
> 	struct anon_vma_name *anon_name = NULL;
>
> 	vma = find_vma(mm, start);
> 	if (vma) {
> 		anon_name = anon_vma_name(vma);
> 		anon_vma_name_get(anon_name);
> 	}
>
> 	return anon_name;
>
> Oleg.
Thanks again.

Rong Tao

>