Received-SPF: pass (google.com: domain of linux-kernel+bounces-7752-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 21 Dec 2023 10:30:37 +0800
From: Changbin Du <changbin.du@huawei.com>
To: Luis Chamberlain <mcgrof@kernel.org>
CC: Changbin Du <changbin.du@huawei.com>, Andrew Morton
	<akpm@linux-foundation.org>, <linux-modules@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Hui Wang <hw.huiwang@huawei.com>, Xiaoyi Su
	<suxiaoyi@huawei.com>, Jeffrey Hugo <jhugo@codeaurora.org>
Subject: Re: [PATCH] modules: wait do_free_init correctly
Message-ID: <20231221023037.emexvbbnt6tpgcs5@M910t>
References: <20231219141231.2218215-1-changbin.du@huawei.com>
 <20231219125151.4a042a259edf3c916580ccfe@linux-foundation.org>
 <ZYIQgz+de/JQl10N@bombadil.infradead.org>
 <20231220052751.3zcnsnvjk5vf5t7j@M910t>
 <ZYL7B59dQ6XFtVP8@bombadil.infradead.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZYL7B59dQ6XFtVP8@bombadil.infradead.org>

On Wed, Dec 20, 2023 at 06:32:39AM -0800, Luis Chamberlain wrote:
> On Wed, Dec 20, 2023 at 01:27:51PM +0800, Changbin Du wrote:
> > On Tue, Dec 19, 2023 at 01:52:03PM -0800, Luis Chamberlain wrote:
> > > On Tue, Dec 19, 2023 at 12:51:51PM -0800, Andrew Morton wrote:
> > > > On Tue, 19 Dec 2023 22:12:31 +0800 Changbin Du <changbin.du@huawei.com> wrote:
> > > > 
> > > > > The commit 1a7b7d922081 ("modules: Use vmalloc special flag") moves
> > > > > do_free_init() into a global workqueue instead of call_rcu(). So now
> > > > > we should wait it via flush_work().
> > > > 
> > > > What are the runtime effects of this change?
> > > 
> > > Indeed that's needed given how old this culprit commit is:
> > > 
> > > git describe --contains 1a7b7d922081
> > > v5.2-rc1~192^2~5
> > > 
> > > Who did this work and for what reason? What triggered this itch?
> > >
> > Seems the waiting was introduced by commit ae646f0b9ca ("init: fix false positives
> > in W+X checking").
> > 
> > As what I have observed, mark_readonly() is only invoked by the first user mode
> > thread function kernel_init(), which is before userspace /init. So is it real
> > possible we have loaded modules at this point?
> 
> Are you saying we don't free any module inits at all then? I asked a lot
> of questions and your answers seem slim.
>
Yes, indeed no module loaded at all before mark_readonly(), at least on my desktop.
So I think we can just delete this synchronization. I am not sure whether there are
any historical reasons.

> How did you find this?
> What actual impact does this have without the patch?
>
This is a coincidence. We encountered a rcu problem which the barrier takes much
longger time to wait (this is an another story). So we reviewed the code and
found this issue.

There is no funcional problem without the patch. It's a unnecessary wait AFAIK,
and it does take a little cycles to wait the rcb callbacks.

> The commit must document this.
> 
>   Luis

-- 
Cheers,
Changbin Du