Re: [PATCH 2/4] modules: Ensure 64-bit alignment on __ksymtab_* sections

[Luis Chamberlain]

On Fri, Dec 22, 2023 at 01:13:26PM +0100, Helge Deller wrote:
> Hi Luis,
> 
> On 12/22/23 06:59, Luis Chamberlain wrote:
> > On Wed, Nov 22, 2023 at 11:18:12PM +0100, deller@xxxxxxxxxx wrote:
> > > From: Helge Deller <deller@xxxxxx>
> > > 
> > > On 64-bit architectures without CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
> > > (e.g. ppc64, ppc64le, parisc, s390x,...) the __KSYM_REF() macro stores
> > > 64-bit pointers into the __ksymtab* sections.
> > > Make sure that those sections will be correctly aligned at module link time,
> > > otherwise unaligned memory accesses may happen at runtime.
> > 
> > The ramifications are not explained there.
> 
> What do you mean exactly by this?

You don't explain the impact of not applying this patch.

> > You keep sending me patches with this and we keep doing a nose dive
> > on this. It means I have to do more work.
> Sorry about that. Since you are mentioned as maintainer for modules I
> thought you are the right contact.

I am certaintly the right person to send this patch to, however, I am
saying that given our previous dialog I would like the commit log to
describe a bit more detail of a few things:

 * how you found this
 * what are the impact of not applying it

Specially if you are going to be sending patches right before the
holidays with a Cc stable fix annotation. This gets me on hight alert
and I have to put things down to see how really critical this is so to
decide to fast track this to Linus or not.

> Furthermore, this patch is pretty small and trivial.

Many patches can be but some can break things and some can be small but
also improve things critically, for example if we are aware of broken
exception handlers.

> And I had the impression that people understand that having unaligned
> structures is generally a bad idea as it usually always impacts performance
> (although the performance penalty in this case isn't critical at all,
> as we are not on hot paths).

There are two aspects to this, one is the critical nature which is
implied by your patch which pegs it as a stable fix, given you prior
patches about this it leaves me wondering if it is fixing some crashes
on some systems given faulty exception handlers.

The performance thing is also subjective, but it could not be subjective
by doing some very trivial tests as I suggested. Such a test would also
help as we lack specific selftsts for this case and we can grow it later
with other things. I figured I'd ask you to try it, since *you* keep
sending patches about misalignments on module sections so I figured
you must be seeing something others are not, and so you must care.

> > Thoughts?
> 
> I really appreciate your thoughts here!
> 
> But given the triviality of this patch, I personally think you are
> proposing a huge academic investment in time and resources with no real benefit.
> On which architecture would you suggest to test this?
> What would be the effective (really new) outcome?
> If the performance penalty is zero or close to zero, will that imply
> that such a patch isn't useful?
> Please also keep in mind that not everyone gets paid to work on the kernel,
> so I have neither the time nor the various architectures to test on.

I think you make this seem so difficult, but I understand it could seem
that way. I've attached at the end of this email a patch that does just
this then to help.

> So, honestly I don't see a real reason why it shouldn't be applied...

Like I said, you Cc'd stable as a fix, as a maintainer it is my job to
verify how critical this is and ask for more details about how you found
it and evaluate the real impact. Even if it was not a stable fix I tend
to ask this for patches, even if they are trivial.

> > > Signed-off-by: Helge Deller <deller@xxxxxx>
> > > Cc: <stable@xxxxxxxxxxxxxxx> # v6.0+
> > 
> > That's a stretch without any data, don't you think?
> 
> Yes. No need to push such a patch to stable series.

OK, can you extend the patch below with something like:

perf stat --repeat 100 --pre 'modprobe -r b a b c' -- ./tools/testing/selftests/module/find_symbol.sh

And test before and after?

I ran a simple test as-is and the data I get is within noise, and so
I think we need the --repeat 100 thing.

-----------------------------------------------------------------------------------
before:
sudo ./tools/testing/selftests/module/find_symbol.sh 

 Performance counter stats for '/sbin/modprobe test_kallsyms_b':

        81,956,206 ns   duration_time                                                         
        81,883,000 ns   system_time                                                           
               210      page-faults                                                           

       0.081956206 seconds time elapsed

       0.000000000 seconds user
       0.081883000 seconds sys



 Performance counter stats for '/sbin/modprobe test_kallsyms_b':

        85,960,863 ns   duration_time                                                         
        84,679,000 ns   system_time                                                           
               212      page-faults                                                           

       0.085960863 seconds time elapsed

       0.000000000 seconds user
       0.084679000 seconds sys



 Performance counter stats for '/sbin/modprobe test_kallsyms_b':

        86,484,868 ns   duration_time                                                         
        86,541,000 ns   system_time                                                           
               213      page-faults                                                           

       0.086484868 seconds time elapsed

       0.000000000 seconds user
       0.086541000 seconds sys

-----------------------------------------------------------------------------------
After your modules alignement fix:
sudo ./tools/testing/selftests/module/find_symbol.sh 
 Performance counter stats for '/sbin/modprobe test_kallsyms_b':

        83,579,980 ns   duration_time                                                         
        83,530,000 ns   system_time                                                           
               212      page-faults                                                           

       0.083579980 seconds time elapsed

       0.000000000 seconds user
       0.083530000 seconds sys



 Performance counter stats for '/sbin/modprobe test_kallsyms_b':

        70,721,786 ns   duration_time                                                         
        69,289,000 ns   system_time                                                           
               211      page-faults                                                           

       0.070721786 seconds time elapsed

       0.000000000 seconds user
       0.069289000 seconds sys



 Performance counter stats for '/sbin/modprobe test_kallsyms_b':

        76,513,219 ns   duration_time                                                         
        76,381,000 ns   system_time                                                           
               214      page-faults                                                           

       0.076513219 seconds time elapsed

       0.000000000 seconds user
       0.076381000 seconds sys

After your modules alignement fix:
sudo ./tools/testing/selftests/module/find_symbol.sh 
 Performance counter stats for '/sbin/modprobe test_kallsyms_b':

        83,579,980 ns   duration_time                                                         
        83,530,000 ns   system_time                                                           
               212      page-faults                                                           

       0.083579980 seconds time elapsed

       0.000000000 seconds user
       0.083530000 seconds sys



 Performance counter stats for '/sbin/modprobe test_kallsyms_b':

        70,721,786 ns   duration_time                                                         
        69,289,000 ns   system_time                                                           
               211      page-faults                                                           

       0.070721786 seconds time elapsed

       0.000000000 seconds user
       0.069289000 seconds sys



 Performance counter stats for '/sbin/modprobe test_kallsyms_b':

        76,513,219 ns   duration_time                                                         
        76,381,000 ns   system_time                                                           
               214      page-faults                                                           

       0.076513219 seconds time elapsed

       0.000000000 seconds user
       0.076381000 seconds sys
-----------------------------------------------------------------------------------