Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Fri, 15 Sep 2023 11:44:40 -0400
From:   Ben Wolsieffer <ben.wolsieffer@hefring.com>
To:     Oleg Nesterov <oleg@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        Greg Ungerer <gerg@uclinux.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Giulio Benetti <giulio.benetti@benettiengineering.com>
Subject: Re: [PATCH] proc: nommu: /proc/<pid>/maps: release mmap read lock
Message-ID: <ZQR76GSNEmG6w2oe@dell-precision-5540>
References: <20230914163019.4050530-2-ben.wolsieffer@hefring.com>
 <20230915121514.GA2768@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230915121514.GA2768@redhat.com>
Precedence: bulk

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