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;
Date:   Tue, 8 Feb 2022 09:55:12 -0800
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>,
        gregkh@linuxfoundation.org
Subject: Re: [PATCH v5] modules: add heuristic when stripping unneeded symbols
Message-ID: <YgKugFcF1kkWPsNa@bombadil.infradead.org>
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>
 <CALkUMdSVV=Vju3TtryObKney7Q1TJLgGd7G7OB4X3FW9eJf4-w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CALkUMdSVV=Vju3TtryObKney7Q1TJLgGd7G7OB4X3FW9eJf4-w@mail.gmail.com>
Sender: Luis Chamberlain <mcgrof@infradead.org>
Precedence: bulk

On Tue, Feb 08, 2022 at 10:22:06AM +0530, Vimal Agrawal wrote:
> > > 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)

You are saying that sometimes having "best" evaluated to non zero
yields incorrect results, where the symbol found is actualy not a .text
symbol for a text address? If so, is this really true for cases where
no stripping is used? If so this is bigger news and I'd like to address
this separately in another commit but we need proof, not just
speculation.

And you seem to be suggesting that this seems to hold more true when
"--strip-unneeded" is used given there are fewer symbols left after
striping?

Did I comprehend what you are trying to say correctly?

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

OK.

> without stripping, it will definitely
> resolve to some .text address closely matching in case of no stripping

OK so there is no issue when stripping is used.

> but it can go wrong with stripping. I have seen it a few times post
> the first patch during testing.

OK then we need to take care your added heuristics do not affect
non-stripping.

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

You should be explicit about this in your commit log.

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

When you submit a v2 patch and you change something like that you must
clarify changes which are not clear either in the commit log or below
the --- lines after the diffstat and before the actual patch. Each new
patch iteration should have a set of bullets with all the changes you
have made so that the maintainer can track what you have done
differently on each iteration.

Right now you are not making any of this easy on me so I ask that you
stop submitting new patches willy nilly until we have actualy discussed
each item, and we decide what to do. I also ask that you keep track of
each change you are making on each new patch iteration on the patch
after the --- lines and before the patch, so I can easily tell all the
changes you have made on each new iteration.

  Luis