Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jeff Moyer <jmoyer@redhat.com>
To:     Benjamin LaHaise <bcrl@kvack.org>
Cc:     Seth Jenkins <sethjenkins@google.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        linux-fsdevel@vger.kernel.org, linux-aio@kvack.org,
        linux-kernel@vger.kernel.org, Jann Horn <jannh@google.com>,
        Pavel Emelyanov <xemul@parallels.com>, stable@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH] aio: fix mremap after fork null-deref
References: <20221104212519.538108-1-sethjenkins@google.com>
        <x49tu0wlv0c.fsf@segfault.boston.devel.redhat.com>
        <CALxfFW5d05H-nFuDdUDS4xVDKMgkV1vvEBAmw10h3-jMVb-PZw@mail.gmail.com>
        <x49ilhbl9qd.fsf@segfault.boston.devel.redhat.com>
        <20230112220939.GO19133@kvack.org>
Date:   Fri, 13 Jan 2023 11:06:31 -0500
In-Reply-To: <20230112220939.GO19133@kvack.org> (Benjamin LaHaise's message of
        "Thu, 12 Jan 2023 17:09:39 -0500")
Message-ID: <x49edryl8qg.fsf@segfault.boston.devel.redhat.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hi, Ben,

Thanks for taking the time to chime in.

Benjamin LaHaise <bcrl@kvack.org> writes:

> On Thu, Jan 12, 2023 at 04:32:42PM -0500, Jeff Moyer wrote:
>> With the patch I proposed (flagging the ring buffer with VM_DONTCOPY),
>> the child process would still be unable to submit and reap I/Os via the
>> aio system calls.  What changes is that the child process would now be
>> unable to reap completions via the shared ring buffer.  In fact, because
>> the ring is no longer mapped in the child process, any attempt to access
>> that memory would result in a segmentation fault.  However, I would be
>> very surprised if the interface was being used in this way.
>> 
>> > If we're okay with this change though, I think it makes sense.
>> 
>> My preference is to make the interface consistent.  I think setting
>> VM_DONTCOPY on the mapping is the right way forward.  I'd welcome other
>> opinions on whether the potential risk is worth it.
>
> VM_DONTCOPY makes sense, but a SEGV is a pretty bad failure mode.  Any
> process reaping events in the child after fork() isn't going to be
> consistent in behaviour, and is able to see partial completion of an I/O
> and other inconsistencies, so they're going to be subtly broken at best.
>
> Unfortunately, we have no way of knowing if this behaviour is exercised
> anywhere without changing it and waiting for someone to holler.

OK, so it sounds like you would rather err on the side of caution.
That's fine with me.

Seth, I'll go back and review your patch.

Thanks!
Jeff