On Wed, Sep 16, 2020 at 06:18:25PM +0100, Quentin Perret wrote:
> The CRC calculation done by genksyms is triggered when the parser hits
> EXPORT_SYMBOL*() macros. At this point, genksyms recursively expands the
> types of the function parameters, and uses that as the input for the CRC
> calculation. In the case of forward-declared structs, the type expands
> to 'UNKNOWN'. Following this, it appears that the result of the
> expansion of each type is cached somewhere, and seems to be re-used
> when/if the same type is seen again for another exported symbol in the
> same C file.
>
> Unfortunately, this can cause CRC 'stability' issues when a struct
> definition becomes visible in the middle of a C file. For example, let's
> assume code with the following pattern:
>
> struct foo;
>
> int bar(struct foo *arg)
> {
> /* Do work ... */
> }
> EXPORT_SYMBOL_GPL(bar);
>
> /* This contains struct foo's definition */
> #include "foo.h"
>
> int baz(struct foo *arg)
> {
> /* Do more work ... */
> }
> EXPORT_SYMBOL_GPL(baz);
>
> Here, baz's CRC will be computed using the expansion of struct foo that
> was cached after bar's CRC calculation ('UNKOWN' here). But if
> EXPORT_SYMBOL_GPL(bar) is removed from the file (because of e.g. symbol
> trimming using CONFIG_TRIM_UNUSED_KSYMS), struct foo will be expanded
> late, during baz's CRC calculation, which now has visibility over the
> full struct definition, hence resulting in a different CRC for baz.
>
> The proper fix for this certainly is in genksyms, but that will take me
> some time to get right. In the meantime, we have seen one occurrence of
> this in the ehci-hcd code which hits this problem because of the way it
> includes C files halfway through the code together with an unlucky mix
> of symbol trimming.
>
> In order to workaround this, move the include done in ehci-hub.c early
> in ehci-hcd.c, hence making sure the struct definitions are visible to
> the entire file. This improves CRC stability of the ehci-hcd exports
> even when symbol trimming is enabled.
>
> Signed-off-by: Quentin Perret <qperret@xxxxxxxxxx>
> ---
> drivers/usb/host/ehci-hcd.c | 1 +
> drivers/usb/host/ehci-hub.c | 1 -
> 2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 6257be4110ca..3575b7201881 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -22,6 +22,7 @@
> #include <linux/interrupt.h>
> #include <linux/usb.h>
> #include <linux/usb/hcd.h>
> +#include <linux/usb/otg.h>
> #include <linux/moduleparam.h>
> #include <linux/dma-mapping.h>
> #include <linux/debugfs.h>
> diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
> index ce0eaf7d7c12..087402aec5cb 100644
> --- a/drivers/usb/host/ehci-hub.c
> +++ b/drivers/usb/host/ehci-hub.c
> @@ -14,7 +14,6 @@
> */
>
> /*-------------------------------------------------------------------------*/
> -#include <linux/usb/otg.h>
>
> #define PORT_WAKE_BITS (PORT_WKOC_E|PORT_WKDISC_E|PORT_WKCONN_E)
Thanks for root-causing this issue. genksyms is a fragile beast, good
luck trying to resolve that!
I'll go queue this up and mark it for stable kernels, thanks.
greg k-h
