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
References: <20220203060641.119522-1-vimal.agrawal@sophos.com>
 <20220204083933.136096-1-vimal.agrawal@sophos.com> <Yf2fB4mzdLiWtoki@bombadil.infradead.org>
 <CALkUMdRv0tMuLpi3Syw7MndBTa9b0xRLGdb6QvM8Q69zXnmUkw@mail.gmail.com> <YgGYL1MKEX9t/ciO@bombadil.infradead.org>
In-Reply-To: <YgGYL1MKEX9t/ciO@bombadil.infradead.org>
From:   Vimal Agrawal <avimalin@gmail.com>
Date:   Tue, 8 Feb 2022 10:22:06 +0530
Message-ID: <CALkUMdSVV=Vju3TtryObKney7Q1TJLgGd7G7OB4X3FW9eJf4-w@mail.gmail.com>
Subject: Re: [PATCH v5] modules: add heuristic when stripping unneeded symbols
To:     Luis Chamberlain <mcgrof@kernel.org>
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>,
        gregkh@linuxfoundation.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> > 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?

I hit this a few times later after the first patch. Basically there
are all symbols in symbol table and best could be none zero ( means it
matched some symbol) but it may not be match to .text symbol for text
address ( esp. when --strip-unneeded is used as there are very few
symbols left after stripping)

> OK so you're saying sometimes "best" is not true when we use
> INSTALL_MOD_STRIP="--strip-unneeded"? This is news.
>
yes, best can be non zero and may not resolve to .text address when
--strip-unneeded is used. without stripping, it will definitely
resolve to some .text address closely matching in case of no stripping
but it can go wrong with stripping. I have seen it a few times post
the first patch during testing.

>
> In particulr you seem to be suggesting that if --strip-unneeded was
> used "best" could be incorrect for !is_module_text_address().
>
best could be incorrect even for text address when --strip-unneeded is used.
e.g. in my case, it is resolving .init.text address to __this_module

> 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.
>
The Only change I did from the first patch was to move this hunk out of (!best).
Yes, I missed commenting it in the code.

> I tried to reproduce and couldn't and sent you a configuration to test.
>
Give me sometime and I will check with the config.
how does your nm test_module.ko look like after stripping?
it shows following for me:

vimal@ubuntu2:~/linux-next/linux/lib$ nm test_module.ko
0000000000000000 r .LC0
0000000000000000 D __this_module
                 U _printk
0000000000000000 T cleanup_module
0000000000000007 T init_module

> 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.
>
I wanted to avoid major change and fix only this particular back trace issue
else I would prefer a check in existing heuristic so that .text address always
resolves to .text symbol and .init to .init symbol and .data to .data symbol
always which is not the case currently that I found lately.

>   Luis