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;
MIME-Version: 1.0
In-Reply-To: <00dd353b-e5aa-69fb-6b52-cb59028ea90a@redhat.com>
References: <20230813123333.1705833-1-mjguzik@gmail.com> <00dd353b-e5aa-69fb-6b52-cb59028ea90a@redhat.com>
From:   Mateusz Guzik <mjguzik@gmail.com>
Date:   Mon, 14 Aug 2023 10:21:21 +0200
Message-ID: <CAGudoHHHyZJXkNfGjGyxqHDay3yAzCm97r6SOdiSLYao7q2Z7A@mail.gmail.com>
Subject: Re: [PATCH] kernel/fork: stop playing lockless games for exe_file replacement
To:     David Hildenbrand <david@redhat.com>
Cc:     linux-kernel@vger.kernel.org, torvalds@linux-foundation.org,
        brauner@kernel.org, ebiederm@xmission.com,
        akpm@linux-foundation.org, linux-mm@kvack.org, koct9i@gmail.com,
        oleg@redhat.com, dave@stgolabs.net
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On 8/14/23, David Hildenbrand <david@redhat.com> wrote:
> On 13.08.23 14:33, Mateusz Guzik wrote:
>> xchg originated in 6e399cd144d8 ("prctl: avoid using mmap_sem for
>> exe_file serialization"). While the commit message does not explain
>> *why* the change, clearly the intent was to use mmap_sem less in this
>> codepath. I found the original submission [1] which ultimately claims it
>> cleans things up.
>
> More details are apparently in v1 of that patch:
>
> https://lore.kernel.org/lkml/1424979417.10344.14.camel@stgolabs.net/
>
> Regarding your patch: adding more mmap_write_lock() where avoidable, I'm
> not so sure.
>

But exe_file access is already synchronized with the semaphore and
your own commit added a mmap_read_lock/mmap_read_unlock cycle after
the xchg in question to accomodate this requirement.

Then mmap_write_lock around the replacement is the obvious thing to do.

> Your patch doesn't look (to me) like it is removing a lot of complexity.
>

The code in the current form makes the reader ask what prompts xchg +
read-lock instead of mere write-locking.

This is not a hot path either and afaics it can only cause contention
if userspace is trying to abuse the interface to break the kernel,
messing with a processs hard at work (which would be an extra argument
to not play games on kernel side).

That said, no, it does not remove "a lot of complexity". It does
remove some though at no real downside that I can see.

But then again, it is on people who insist on xchg to justify it.

-- 
Mateusz Guzik <mjguzik gmail.com>