Re: [PATCH v5] modules: add heuristic when stripping unneeded symbols

From: Luis Chamberlain <mcgrof@kernel.org>
To: Vimal Agrawal <avimalin@gmail.com>
Cc: Christoph Hellwig <hch@infradead.org>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Michal Marek <michal.lkml@markovi.net>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Jan Beulich <JBeulich@suse.com>, Jeff Mahoney <jeffm@suse.com>,
	Sam Ravnborg <sam@ravnborg.org>,
	linux-kbuild@vger.kernel.org, jeyu@kernel.org,
	linux-kernel@vger.kernel.org, nishit.shah@sophos.com,
	Vimal Agrawal <vimal.agrawal@sophos.com>,
	Dirk VanDerMerwe <Dirk.VanDerMerwe@sophos.com>
Subject: Re: [PATCH v5] modules: add heuristic when stripping unneeded symbols
Date: Mon, 7 Feb 2022 14:07:43 -0800	[thread overview]
Message-ID: <YgGYL1MKEX9t/ciO@bombadil.infradead.org> (raw)
In-Reply-To: <CALkUMdRv0tMuLpi3Syw7MndBTa9b0xRLGdb6QvM8Q69zXnmUkw@mail.gmail.com>

On Mon, Feb 07, 2022 at 06:51:50PM +0530, Vimal Agrawal wrote:
> >
> > You did not explain why you change your code to not use the below
> > (!best) branch. I'd much prefer it there as that is when we know
> > for sure we have no real symbol defined.
> >
> >   Luis
> 
> Actually I had it  under (!best) in my first patch. I had to change it
> because it was resolving the address to symbols like __this_module for
> init address at times which is not correct for text address.

Can you say that again?

> It was
> not entering inside if (!best) as it found some match but the match
> was not correct.

Is this a summary of what you said above and I could not understand?

OK so you're saying sometimes "best" is not true when we use
INSTALL_MOD_STRIP="--strip-unneeded"? This is news.

> It could be fine for some non text addresses but not
> for text addresses.

In particulr you seem to be suggesting that if --strip-unneeded was
used "best" could be incorrect for !is_module_text_address().

In any case, you completely changed things in your patch and did not
mention *any* of this in your follow up patch, leaving me to question
the validity of all this work.

> So I had to move this check out of (!best) and put a check explicitly
> for text address to avoid any impact on non text addresses by
> following:
> 
> +       if ((is_module_text_address(addr) &&
> +               (bestval < baseval || bestval > nextval))) {
> +               /*
> +                * return MODULE base and offset if we could not find
> +                * any best match for text address
> +                */
> 
> I tested it on next-20220116-1-gff24014a52c0 today and I am able to
> repro at least for init address easily with test_module.ko.

I tried to reproduce and couldn't and sent you a configuration to test.

> I can update the patch explaining this in comments in between the code.

The above is not clear and you need to make things crystal clear. If the
existing heuristic for best is not valid all the times it needs to be
made clear using a comment, sure.

If your heuristic is *better* than the existing heuristic *today*, that
needs to *also* be clearly spelled out. Your patch does none of this and
the commit log clearly does not reflect it.

  Luis