From: Peter Rosin <peda@axentia.se> To: Nicolas Ferre <nicolas.ferre@microchip.com>, Tudor.Ambarus@microchip.com, Alexandre Belloni <alexandre.belloni@bootlin.com> Cc: Daniels Umanovskis <du@axentia.se>, Patrice Vilchez <patrice.vilchez@microchip.com>, Cristian Birsan <Cristian.Birsan@microchip.com>, Ludovic Desroches <ludovic.desroches@microchip.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Saravana Kannan <saravanak@google.com> Subject: Re: Regression: memory corruption on Atmel SAMA5D31 Date: Thu, 10 Mar 2022 10:58:56 +0100 [thread overview] Message-ID: <220ddbef-5592-47b7-5150-4291f9532c6d@axentia.se> (raw) In-Reply-To: <e5a715c5-ad9f-6fd4-071e-084ab950603e@microchip.com> [bringing this threadlet back to the lists, hope that's ok] On 2022-03-10 09:27, Nicolas Ferre wrote: > On 09/03/2022 at 12:42, Peter Rosin wrote: >> On 2022-03-09 11:38, Nicolas Ferre wrote: >>> Hi Peter, >>> *snip* >>> One of my colleagues had an idea about this issue and in particular with >>> the fact that removing some of the entries in the structure triggered >>> the problem: "isn't it some kind of misalignment between structures that >>> are supposed to be treated in 64 bits machines and our 32 bits core that >>> we use?" >>> This misalignment or "wrong assumption" of using 64 bits machine might >>> be present in the USB stack as it seems to be related to this sub-system >>> somehow. >> >> Yes, something like that has been creeping around in the back of my >> head too. And it could be something much later in struct device that >> is no longer sufficiently aligned when struct dev_links_info changes. >> But what? I verified the alignment of various things. With the old working struct dev_links_info, i.e. struct dev_links_info { struct list_head suppliers; struct list_head consumers; struct list_head needs_suppliers; struct list_head defer_sync; bool need_for_probe; enum dl_dev_state status; }; I get sizeof(struct device) 440 sizeof(struct dev_links_info) 40 offsetof(struct device, links) 80 offsetof(struct device, power) 120 "power" is the next member after "struct dev_links_info links" in struct device, and I find no other uses of struct dev_links_info. With the new problematic layout, i.e. struct dev_links_info { struct list_head suppliers; struct list_head consumers; struct list_head defer_sync; enum dl_dev_state status; }; I get: sizeof(struct device) 432 sizeof(struct dev_links_info) 28 offsetof(struct device, links) 80 offsetof(struct device, power) 112 Which means that everything around and within dev_links_info is 8-byte aligned in the same way in either case. The exception being that "status" no longer shares 8-byte space with "need_for_probe" (which is gone). But that should only make things better, no? That combined with the test with this permuted version (swapped two list_heads in the middle): struct dev_links_info { struct list_head suppliers; struct list_head consumers; struct list_head defer_sync; struct list_head needs_suppliers; bool need_for_probe; enum dl_dev_state status; }; which displayed a new failure mode (BUG instead of corruption, see upthread) indicates that this is not an alignment issue. Famous last words... > From that article: > https://lwn.net/Articles/885941/ > > I read: > "Koschel included a patch fixing a bug in the USB subsystem where the > iterator passed to this macro was used after the exit from the macro, > which is a dangerous thing to do. Depending on what happens within the > list, the contents of that iterator could be something surprising, even > in the absence of speculative execution. Koschel fixed the problem by > reworking the code in question to stop using the iterator after the loop. " > > USB subsystem, "struct list_head *next, *prev;"... Some keywords present > there... worth a try? > > Regards, > Nicolas gr_udc.c is not built with the config that is in use, which is sad because it looked like a good candidate. Cheers, Peter
WARNING: multiple messages have this Message-ID (diff)
From: Peter Rosin <peda@axentia.se> To: Nicolas Ferre <nicolas.ferre@microchip.com>, Tudor.Ambarus@microchip.com, Alexandre Belloni <alexandre.belloni@bootlin.com> Cc: Daniels Umanovskis <du@axentia.se>, Patrice Vilchez <patrice.vilchez@microchip.com>, Cristian Birsan <Cristian.Birsan@microchip.com>, Ludovic Desroches <ludovic.desroches@microchip.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Saravana Kannan <saravanak@google.com> Subject: Re: Regression: memory corruption on Atmel SAMA5D31 Date: Thu, 10 Mar 2022 10:58:56 +0100 [thread overview] Message-ID: <220ddbef-5592-47b7-5150-4291f9532c6d@axentia.se> (raw) In-Reply-To: <e5a715c5-ad9f-6fd4-071e-084ab950603e@microchip.com> [bringing this threadlet back to the lists, hope that's ok] On 2022-03-10 09:27, Nicolas Ferre wrote: > On 09/03/2022 at 12:42, Peter Rosin wrote: >> On 2022-03-09 11:38, Nicolas Ferre wrote: >>> Hi Peter, >>> *snip* >>> One of my colleagues had an idea about this issue and in particular with >>> the fact that removing some of the entries in the structure triggered >>> the problem: "isn't it some kind of misalignment between structures that >>> are supposed to be treated in 64 bits machines and our 32 bits core that >>> we use?" >>> This misalignment or "wrong assumption" of using 64 bits machine might >>> be present in the USB stack as it seems to be related to this sub-system >>> somehow. >> >> Yes, something like that has been creeping around in the back of my >> head too. And it could be something much later in struct device that >> is no longer sufficiently aligned when struct dev_links_info changes. >> But what? I verified the alignment of various things. With the old working struct dev_links_info, i.e. struct dev_links_info { struct list_head suppliers; struct list_head consumers; struct list_head needs_suppliers; struct list_head defer_sync; bool need_for_probe; enum dl_dev_state status; }; I get sizeof(struct device) 440 sizeof(struct dev_links_info) 40 offsetof(struct device, links) 80 offsetof(struct device, power) 120 "power" is the next member after "struct dev_links_info links" in struct device, and I find no other uses of struct dev_links_info. With the new problematic layout, i.e. struct dev_links_info { struct list_head suppliers; struct list_head consumers; struct list_head defer_sync; enum dl_dev_state status; }; I get: sizeof(struct device) 432 sizeof(struct dev_links_info) 28 offsetof(struct device, links) 80 offsetof(struct device, power) 112 Which means that everything around and within dev_links_info is 8-byte aligned in the same way in either case. The exception being that "status" no longer shares 8-byte space with "need_for_probe" (which is gone). But that should only make things better, no? That combined with the test with this permuted version (swapped two list_heads in the middle): struct dev_links_info { struct list_head suppliers; struct list_head consumers; struct list_head defer_sync; struct list_head needs_suppliers; bool need_for_probe; enum dl_dev_state status; }; which displayed a new failure mode (BUG instead of corruption, see upthread) indicates that this is not an alignment issue. Famous last words... > From that article: > https://lwn.net/Articles/885941/ > > I read: > "Koschel included a patch fixing a bug in the USB subsystem where the > iterator passed to this macro was used after the exit from the macro, > which is a dangerous thing to do. Depending on what happens within the > list, the contents of that iterator could be something surprising, even > in the absence of speculative execution. Koschel fixed the problem by > reworking the code in question to stop using the iterator after the loop. " > > USB subsystem, "struct list_head *next, *prev;"... Some keywords present > there... worth a try? > > Regards, > Nicolas gr_udc.c is not built with the config that is in use, which is sad because it looked like a good candidate. Cheers, Peter _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-03-10 9:59 UTC|newest] Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-03 0:29 Regression: memory corruption on Atmel SAMA5D31 Peter Rosin 2022-03-03 3:02 ` Saravana Kannan 2022-03-03 3:02 ` Saravana Kannan 2022-03-03 9:17 ` Peter Rosin 2022-03-03 9:17 ` Peter Rosin 2022-03-04 3:55 ` Saravana Kannan 2022-03-04 3:55 ` Saravana Kannan 2022-03-04 6:57 ` Peter Rosin 2022-03-04 6:57 ` Peter Rosin 2022-03-04 10:57 ` Peter Rosin 2022-03-04 10:57 ` Peter Rosin 2022-03-04 11:12 ` Tudor.Ambarus 2022-03-04 11:12 ` Tudor.Ambarus 2022-03-04 12:38 ` Peter Rosin 2022-03-04 12:38 ` Peter Rosin 2022-03-04 16:48 ` Tudor.Ambarus 2022-03-04 16:48 ` Tudor.Ambarus 2022-03-07 9:45 ` Tudor.Ambarus 2022-03-07 9:45 ` Tudor.Ambarus 2022-03-07 11:32 ` Peter Rosin 2022-03-07 11:32 ` Peter Rosin 2022-03-07 20:32 ` Peter Rosin 2022-03-07 20:32 ` Peter Rosin 2022-03-08 7:55 ` Nicolas Ferre 2022-03-08 7:55 ` Nicolas Ferre 2022-03-09 8:30 ` Peter Rosin 2022-03-09 8:30 ` Peter Rosin [not found] ` <6d9561a4-39e4-3dbe-5fe2-c6f88ee2a4c6@axentia.se> [not found] ` <ed24a281-1790-8e24-5f5a-25b66527044b@microchip.com> [not found] ` <d563c7ba-6431-2639-9f2a-2e2c6788e625@axentia.se> [not found] ` <e5a715c5-ad9f-6fd4-071e-084ab950603e@microchip.com> 2022-03-10 9:58 ` Peter Rosin [this message] 2022-03-10 9:58 ` Peter Rosin 2022-03-10 10:40 ` Peter Rosin 2022-03-10 10:40 ` Peter Rosin 2022-04-09 13:02 ` Thorsten Leemhuis 2022-04-09 13:02 ` Thorsten Leemhuis 2022-04-11 6:21 ` Tudor.Ambarus 2022-04-11 6:21 ` Tudor.Ambarus 2022-05-17 14:50 ` Peter Rosin 2022-05-17 14:50 ` Peter Rosin 2022-05-18 6:21 ` Tudor.Ambarus 2022-05-18 6:21 ` Tudor.Ambarus 2022-05-18 7:51 ` Peter Rosin 2022-05-18 7:51 ` Peter Rosin 2022-06-20 7:04 ` Thorsten Leemhuis 2022-06-20 7:04 ` Thorsten Leemhuis 2022-06-20 8:43 ` Tudor.Ambarus 2022-06-20 8:43 ` Tudor.Ambarus 2022-06-20 14:22 ` Tudor.Ambarus 2022-06-20 14:22 ` Tudor.Ambarus 2022-06-21 7:00 ` Peter Rosin 2022-06-21 7:00 ` Peter Rosin 2022-06-21 10:46 ` Peter Rosin 2022-06-21 10:46 ` Peter Rosin 2022-06-27 12:26 ` Tudor.Ambarus 2022-06-27 12:26 ` Tudor.Ambarus 2022-06-27 16:53 ` Tudor.Ambarus 2022-06-27 16:53 ` Tudor.Ambarus 2022-06-30 5:20 ` Peter Rosin 2022-06-30 5:20 ` Peter Rosin 2022-06-30 9:23 ` Tudor.Ambarus 2022-06-30 9:23 ` Tudor.Ambarus 2022-06-30 10:20 ` Tudor.Ambarus 2022-06-30 10:20 ` Tudor.Ambarus 2022-07-13 16:01 ` Tudor.Ambarus 2022-07-13 16:01 ` Tudor.Ambarus 2022-07-28 7:45 ` Tudor.Ambarus 2022-07-28 7:45 ` Tudor.Ambarus 2022-07-28 8:39 ` Tudor.Ambarus 2022-07-28 8:39 ` Tudor.Ambarus 2022-07-29 20:09 ` Peter Rosin 2022-07-29 20:09 ` Peter Rosin 2022-07-30 11:37 ` Peter Rosin 2022-07-30 11:37 ` Peter Rosin 2022-07-31 3:44 ` Tudor.Ambarus 2022-07-31 3:44 ` Tudor.Ambarus 2022-03-04 20:06 ` Saravana Kannan 2022-03-04 20:06 ` Saravana Kannan 2022-03-04 8:00 ` Thorsten Leemhuis 2022-03-04 8:00 ` Thorsten Leemhuis
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=220ddbef-5592-47b7-5150-4291f9532c6d@axentia.se \ --to=peda@axentia.se \ --cc=Cristian.Birsan@microchip.com \ --cc=Tudor.Ambarus@microchip.com \ --cc=alexandre.belloni@bootlin.com \ --cc=du@axentia.se \ --cc=gregkh@linuxfoundation.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=ludovic.desroches@microchip.com \ --cc=nicolas.ferre@microchip.com \ --cc=patrice.vilchez@microchip.com \ --cc=saravanak@google.com \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.