[PATCH] ehci-hcd: Move include to keep CRC stable

[PATCH] ehci-hcd: Move include to keep CRC stable

[Quentin Perret]

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@google.com>
---
 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)
 
-- 
2.28.0.618.gf4bc123cb7-goog

Re: [PATCH] ehci-hcd: Move include to keep CRC stable

[Greg KH]

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@google.com>
> ---
>  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

Re: [PATCH] ehci-hcd: Move include to keep CRC stable

[Greg KH]

On Wed, Sep 16, 2020 at 07:31:27PM +0200, Greg KH wrote:
> 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@google.com>
> > ---
> >  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.

I'll queue it up after I get Alan's review first :)

thanks,

greg k-h

Re: [PATCH] ehci-hcd: Move include to keep CRC stable

[Alan Stern]

On Wed, Sep 16, 2020 at 07:32:06PM +0200, Greg KH wrote:
> On Wed, Sep 16, 2020 at 07:31:27PM +0200, Greg KH wrote:
> > 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@google.com>
> > > ---
> > >  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.
> 
> I'll queue it up after I get Alan's review first :)

It's fine with me.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

Re: [PATCH] ehci-hcd: Move include to keep CRC stable

[Matthias Maennich]

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@google.com>

Reviewed-by: Matthias Maennich <maennich@google.com>

Cheers,
Matthias

>---
> 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)
>
>-- 
>2.28.0.618.gf4bc123cb7-goog
>