Re: [PATCH] proc: nommu: /proc/<pid>/maps: release mmap read lock

[Ben Wolsieffer <ben.wolsieffer@hefring.com>]

On Fri, Sep 15, 2023 at 02:15:15PM +0200, Oleg Nesterov wrote:
> Sorry for the offtopic question. I know NOTHING about nommu and when I tried to
> review this patch I was puzzled by
> 
> 	/* See m_next(). Zero at the start or after lseek. */
> 	if (addr == -1UL)
> 		return NULL;
> 
> at the start of m_start(). OK, lets look at
> 
> 	static void *m_next(struct seq_file *m, void *_p, loff_t *pos)
> 	{
> 		struct vm_area_struct *vma = _p;
> 
> 		*pos = vma->vm_end;
> 		return find_vma(vma->vm_mm, vma->vm_end);
> 	}
> 
> where does this -1UL come from? Does this mean that on nommu
> 
> 	last_vma->vm_end == -1UL
> 
> or what?
> 
> fs/proc/task_mmu.c has the same check at the start, but in this case
> the "See m_next()" comment actually helps.

Yes, this is another copying mistake from the MMU implementation. In
fact, it turns out that no-MMU /proc/<pid>/maps is completely broken
after 0c563f148043 ("proc: remove VMA rbtree use from nommu"). It just
returns an empty file.

This happens because find_vma() doesn't do what we want here. It "look[s]
up the first VMA in which addr resides, NULL if none", and the address
will be zero in in m_start(), which makes find_vma() return NULL (unless
presumably the zero address is actually part of the process's address
space).

I didn't run into this because I developed my patch against an older
kernel, and didn't test the latest version until today.

I'm preparing a second patch to fix this bug.

> 
> Just curious, thanks.
> 
> Oleg.
> 

Thanks, Ben