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

On Thu, Sep 14, 2023 at 01:30:08PM -0400, Ben Wolsieffer wrote:
> On Thu, Sep 14, 2023 at 10:02:03AM -0700, Andrew Morton wrote:
> > On Thu, 14 Sep 2023 12:30:20 -0400 Ben Wolsieffer <ben.wolsieffer@hefring.com> wrote:
> > 
> > > The no-MMU implementation of /proc/<pid>/map doesn't normally release
> > > the mmap read lock, because it uses !IS_ERR_OR_NULL(_vml) to determine
> > > whether to release the lock. Since _vml is NULL when the end of the
> > > mappings is reached, the lock is not released.
> > > 
> > 
> > Thanks.  Is this bug demonstrable from userspace?  If so, how?
> 
> Yes, run "cat /proc/1/maps" twice. You should observe that the
> second run hangs.
 
Hi Andrew,

I apologize because I realized I provided an incorrect reproducer for
this bug. I responded from what I remembered of this bug (I originally
wrote the patch over a year ago) and did not test it.

Reading /proc/1/maps twice doesn't reproduce the bug because it only
takes the read lock, which can be taken multiple times and therefore
doesn't show any problem if the lock isn't released. Instead, you need
to perform some operation that attempts to take the write lock after
reading /proc/<pid>/maps. To actually reproduce the bug, compile the
following code as 'proc_maps_bug':

#include <stdio.h>
#include <unistd.h>
#include <sys/mman.h>

int main(int argc, char *argv[]) {
        void *buf;
        sleep(1);
        buf = mmap(NULL, 4096, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
        puts("mmap returned");
        return 0;
}

Then, run:

  ./proc_maps_bug &; cat /proc/$!/maps; fg

Without this patch, mmap() will hang and the command will never
complete.

Additionally, it turns out you cannot reproduce this bug on recent
kernels because 0c563f148043 ("proc: remove VMA rbtree use from nommu")
introduces a second bug that completely breaks /proc/<pid>/maps and
prevents the locking bug from being triggered. I will have a second
patch for that soon.

Thanks, Ben