On Fri, Apr 28, 2023 at 12:46?AM Lyude Paul <lyude at redhat.com> wrote:> > Hey Linus, Kees. Responses below > > On Sun, 2023-04-23 at 13:23 -0700, Kees Cook wrote: > > On April 23, 2023 10:36:24 AM PDT, Linus Torvalds <torvalds at linux-foundation.org> wrote: > > > Kees, > > > I made the mistake of upgrading my M2 Macbook Air to Fedora-38, and > > > in the process I got gcc-13 which is not WERROR-clean because we only > > > limited the 'array-bounds' warning to gcc-11 and gcc-12. But gcc-13 > > > has all the same issues. > > > > > > And I want to be able to do my arm64 builds with WERROR on still... > > > > > > I guess it never made much sense to hope it was going to go away > > > without having a confirmation, so I just changed it to be gcc-11+. > > > > Yeah, that's fine. GCC 13 released without having a fix for at least one (hopefully last) known array-bounds vs jump threading bug: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109071 > > > > > And one of them is from you. > > > > > > In particular, commit 4076ea2419cf ("drm/nouveau/disp: Fix > > > nvif_outp_acquire_dp() argument size") cannot possibly be right, It > > > changes > > > > > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[16], > > > > > > to > > > > > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[DP_RECEIVER_CAP_SIZE], > > > > > > and then does > > > > > > memcpy(args.dp.dpcd, dpcd, sizeof(args.dp.dpcd)); > > > > > > where that 'args.dp.dpcd' is a 16-byte array, and DP_RECEIVER_CAP_SIZE is 15. > > > > Yeah, it was an incomplete fix. I sent the other half here, but it fell through the cracks: > > https://lore.kernel.org/lkml/20230204184307.never.825-kees at kernel.org/ > > Thanks for bringing this to our attention, yeah this definitely just looks > like it got missed somewhere down the line. It looks like Karol responded > already so I assume the patch is in the pipeline now, but let me know if > there's anything else you need. >uhm, I didn't push anything, but I can push it through drm-misct asap, just wanted to ask if somebody wants to pick a quicker route. But I guess not?> > > > > > > > > > > > > > I think it's all entirely harmless from a code generation standpoint, > > > because the 15-byte field will be padded out to 16 bytes in the > > > structure that contains it, but it's most definitely buggy. > > > > Right; between this, that GCC 13 wasn't released yet, and I had no feedback from NV folks, I didn't chase down landing that fix. > > > > > > > > So that warning does find real cases of wrong code. But when those > > > real cases are hidden by hundreds of lines of unfixable false > > > positives, we don't have much choice. > > > > Yup, totally agreed. The false positives I've looked at all seem to be similar to the outstanding jump threading bug, so I'm hoping once that gets fixed we'll finally have a good signal with that warning enabled. :) > > > > -Kees > > > > > > -- > Cheers, > Lyude Paul (she/her) > Software Engineer at Red Hat >
On Fri, 2023-04-28 at 00:50 +0200, Karol Herbst wrote:> On Fri, Apr 28, 2023 at 12:46?AM Lyude Paul <lyude at redhat.com> wrote: > > > > Hey Linus, Kees. Responses below > > > > On Sun, 2023-04-23 at 13:23 -0700, Kees Cook wrote: > > > On April 23, 2023 10:36:24 AM PDT, Linus Torvalds <torvalds at linux-foundation.org> wrote: > > > > Kees, > > > > I made the mistake of upgrading my M2 Macbook Air to Fedora-38, and > > > > in the process I got gcc-13 which is not WERROR-clean because we only > > > > limited the 'array-bounds' warning to gcc-11 and gcc-12. But gcc-13 > > > > has all the same issues. > > > > > > > > And I want to be able to do my arm64 builds with WERROR on still... > > > > > > > > I guess it never made much sense to hope it was going to go away > > > > without having a confirmation, so I just changed it to be gcc-11+. > > > > > > Yeah, that's fine. GCC 13 released without having a fix for at least one (hopefully last) known array-bounds vs jump threading bug: > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109071 > > > > > > > And one of them is from you. > > > > > > > > In particular, commit 4076ea2419cf ("drm/nouveau/disp: Fix > > > > nvif_outp_acquire_dp() argument size") cannot possibly be right, It > > > > changes > > > > > > > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[16], > > > > > > > > to > > > > > > > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[DP_RECEIVER_CAP_SIZE], > > > > > > > > and then does > > > > > > > > memcpy(args.dp.dpcd, dpcd, sizeof(args.dp.dpcd)); > > > > > > > > where that 'args.dp.dpcd' is a 16-byte array, and DP_RECEIVER_CAP_SIZE is 15. > > > > > > Yeah, it was an incomplete fix. I sent the other half here, but it fell through the cracks: > > > https://lore.kernel.org/lkml/20230204184307.never.825-kees at kernel.org/ > > > > Thanks for bringing this to our attention, yeah this definitely just looks > > like it got missed somewhere down the line. It looks like Karol responded > > already so I assume the patch is in the pipeline now, but let me know if > > there's anything else you need. > > > > uhm, I didn't push anything, but I can push it through drm-misct asap, > just wanted to ask if somebody wants to pick a quicker route. But I > guess not?Ah whoops, I misunderstood! Yeah I would say we should just go ahead and push it since I don't see any indication here that anyone else has.> > > > > > > > > > > > > > > > > > > > > I think it's all entirely harmless from a code generation standpoint, > > > > because the 15-byte field will be padded out to 16 bytes in the > > > > structure that contains it, but it's most definitely buggy. > > > > > > Right; between this, that GCC 13 wasn't released yet, and I had no feedback from NV folks, I didn't chase down landing that fix. > > > > > > > > > > > So that warning does find real cases of wrong code. But when those > > > > real cases are hidden by hundreds of lines of unfixable false > > > > positives, we don't have much choice. > > > > > > Yup, totally agreed. The false positives I've looked at all seem to be similar to the outstanding jump threading bug, so I'm hoping once that gets fixed we'll finally have a good signal with that warning enabled. :) > > > > > > -Kees > > > > > > > > > > -- > > Cheers, > > Lyude Paul (she/her) > > Software Engineer at Red Hat > > >-- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
On April 27, 2023 3:50:06 PM PDT, Karol Herbst <kherbst at redhat.com> wrote:>On Fri, Apr 28, 2023 at 12:46?AM Lyude Paul <lyude at redhat.com> wrote: >> >> Hey Linus, Kees. Responses below >> >> On Sun, 2023-04-23 at 13:23 -0700, Kees Cook wrote: >> > On April 23, 2023 10:36:24 AM PDT, Linus Torvalds <torvalds at linux-foundation.org> wrote: >> > > Kees, >> > > I made the mistake of upgrading my M2 Macbook Air to Fedora-38, and >> > > in the process I got gcc-13 which is not WERROR-clean because we only >> > > limited the 'array-bounds' warning to gcc-11 and gcc-12. But gcc-13 >> > > has all the same issues. >> > > >> > > And I want to be able to do my arm64 builds with WERROR on still... >> > > >> > > I guess it never made much sense to hope it was going to go away >> > > without having a confirmation, so I just changed it to be gcc-11+. >> > >> > Yeah, that's fine. GCC 13 released without having a fix for at least one (hopefully last) known array-bounds vs jump threading bug: >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109071 >> > >> > > And one of them is from you. >> > > >> > > In particular, commit 4076ea2419cf ("drm/nouveau/disp: Fix >> > > nvif_outp_acquire_dp() argument size") cannot possibly be right, It >> > > changes >> > > >> > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[16], >> > > >> > > to >> > > >> > > nvif_outp_acquire_dp(struct nvif_outp *outp, u8 dpcd[DP_RECEIVER_CAP_SIZE], >> > > >> > > and then does >> > > >> > > memcpy(args.dp.dpcd, dpcd, sizeof(args.dp.dpcd)); >> > > >> > > where that 'args.dp.dpcd' is a 16-byte array, and DP_RECEIVER_CAP_SIZE is 15. >> > >> > Yeah, it was an incomplete fix. I sent the other half here, but it fell through the cracks: >> > https://lore.kernel.org/lkml/20230204184307.never.825-kees at kernel.org/ >> >> Thanks for bringing this to our attention, yeah this definitely just looks >> like it got missed somewhere down the line. It looks like Karol responded >> already so I assume the patch is in the pipeline now, but let me know if >> there's anything else you need. >> > >uhm, I didn't push anything, but I can push it through drm-misct asap, >just wanted to ask if somebody wants to pick a quicker route. But I >guess not?If you can pick it up, that would be great. There's no rush. :) -- Kees Cook