| Message ID | 20200916171825.3228122-1-qperret@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | ehci-hcd: Move include to keep CRC stable | expand |
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
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
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
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 >
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)
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(-)