linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Matthias Maennich <maennich@google.com>
To: Quentin Perret <qperret@google.com>
Cc: stern@rowland.harvard.edu, gregkh@linuxfoundation.org,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	gprocida@google.com, kernel-team@android.com
Subject: Re: [PATCH] ehci-hcd: Move include to keep CRC stable
Date: Thu, 17 Sep 2020 12:44:11 +0100	[thread overview]
Message-ID: <20200917114411.GB3897889@google.com> (raw)
In-Reply-To: <20200916171825.3228122-1-qperret@google.com>

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
>

      parent reply	other threads:[~2020-09-17 11:46 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-16 17:18 [PATCH] ehci-hcd: Move include to keep CRC stable Quentin Perret
2020-09-16 17:31 ` Greg KH
2020-09-16 17:32   ` Greg KH
2020-09-16 20:16     ` Alan Stern
2020-09-17 11:44 ` Matthias Maennich [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200917114411.GB3897889@google.com \
    --to=maennich@google.com \
    --cc=gprocida@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=qperret@google.com \
    --cc=stern@rowland.harvard.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).