| Message ID | 20200603233203.1695403-6-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove uninitialized_var() macro | expand |
On Wed, Jun 3, 2020 at 4:32 PM Kees Cook <keescook@chromium.org> wrote: > > Using uninitialized_var() is dangerous as it papers over real bugs[1] > (or can in the future), and suppresses unrelated compiler warnings (e.g. > "unused variable"). If the compiler thinks it is uninitialized, either > simply initialize the variable or make compiler changes. As a precursor > to removing[2] this[3] macro[4], just remove this variable since it was > actually unused: > > drivers/ide/ide-taskfile.c:232:34: warning: unused variable 'flags' [-Wunused-variable] > unsigned long uninitialized_var(flags); > ^ > > [1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ > [2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ > [3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ > [4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ > > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Fixes ce1e518190ea ("ide: don't disable interrupts during kmap_atomic()") > --- > drivers/ide/ide-taskfile.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c > index aab6a10435b6..a26f85ab58a9 100644 > --- a/drivers/ide/ide-taskfile.c > +++ b/drivers/ide/ide-taskfile.c > @@ -229,7 +229,6 @@ void ide_pio_bytes(ide_drive_t *drive, struct ide_cmd *cmd, > ide_hwif_t *hwif = drive->hwif; > struct scatterlist *sg = hwif->sg_table; > struct scatterlist *cursg = cmd->cursg; > - unsigned long uninitialized_var(flags); > struct page *page; > unsigned int offset; > u8 *buf; > -- > 2.25.1 > > -- > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200603233203.1695403-6-keescook%40chromium.org.
On Thu, Jun 04, 2020 at 12:29:17PM -0700, Nick Desaulniers wrote: > On Wed, Jun 3, 2020 at 4:32 PM Kees Cook <keescook@chromium.org> wrote: > > > > Using uninitialized_var() is dangerous as it papers over real bugs[1] > > (or can in the future), and suppresses unrelated compiler warnings (e.g. > > "unused variable"). If the compiler thinks it is uninitialized, either > > simply initialize the variable or make compiler changes. As a precursor > > to removing[2] this[3] macro[4], just remove this variable since it was > > actually unused: > > > > drivers/ide/ide-taskfile.c:232:34: warning: unused variable 'flags' [-Wunused-variable] > > unsigned long uninitialized_var(flags); > > ^ > > > > [1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ > > [2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ > > [3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ > > [4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Thanks for the reviews! > Fixes ce1e518190ea ("ide: don't disable interrupts during kmap_atomic()") I originally avoided adding Fixes tags because I didn't want these changes backported into a -stable without -Wmaybe-uninitialized disabled, but in these cases (variable removal), that actually does make sense. Thanks! -Kees
On Thu, Jun 4, 2020 at 1:20 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Jun 04, 2020 at 12:29:17PM -0700, Nick Desaulniers wrote: > > On Wed, Jun 3, 2020 at 4:32 PM Kees Cook <keescook@chromium.org> wrote: > > > > > > Using uninitialized_var() is dangerous as it papers over real bugs[1] > > > (or can in the future), and suppresses unrelated compiler warnings (e.g. > > > "unused variable"). If the compiler thinks it is uninitialized, either > > > simply initialize the variable or make compiler changes. As a precursor > > > to removing[2] this[3] macro[4], just remove this variable since it was > > > actually unused: > > > > > > drivers/ide/ide-taskfile.c:232:34: warning: unused variable 'flags' [-Wunused-variable] > > > unsigned long uninitialized_var(flags); > > > ^ > > > > > > [1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ > > > [2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ > > > [3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ > > > [4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ > > > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > Thanks for the reviews! > > > Fixes ce1e518190ea ("ide: don't disable interrupts during kmap_atomic()") > > I originally avoided adding Fixes tags because I didn't want these > changes backported into a -stable without -Wmaybe-uninitialized > disabled, but in these cases (variable removal), that actually does make > sense. Thanks! Saravana showed me a cool trick for quickly finding commits that removed a particular identifier that I find faster than `git blame` or vim-fugitive for the purpose of Fixes tags: $ git log -S <string> <file> I've added it to our wiki: https://github.com/ClangBuiltLinux/linux/wiki/Command-line-tips-and-tricks#for-finding-which-commit-may-have-removed-a-string-try. I should update the first tip; what was your suggestion for constraining the search to the current remote?
On Thu, Jun 4, 2020 at 10:20 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Jun 04, 2020 at 12:29:17PM -0700, Nick Desaulniers wrote: > > On Wed, Jun 3, 2020 at 4:32 PM Kees Cook <keescook@chromium.org> wrote: > > > > > > Using uninitialized_var() is dangerous as it papers over real bugs[1] > > > (or can in the future), and suppresses unrelated compiler warnings (e.g. > > > "unused variable"). If the compiler thinks it is uninitialized, either > > > simply initialize the variable or make compiler changes. As a precursor > > > to removing[2] this[3] macro[4], just remove this variable since it was > > > actually unused: > > > > > > drivers/ide/ide-taskfile.c:232:34: warning: unused variable 'flags' [-Wunused-variable] > > > unsigned long uninitialized_var(flags); > > > ^ > > > > > > [1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ > > > [2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ > > > [3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ > > > [4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ > > > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > Thanks for the reviews! > > > Fixes ce1e518190ea ("ide: don't disable interrupts during kmap_atomic()") > > I originally avoided adding Fixes tags because I didn't want these > changes backported into a -stable without -Wmaybe-uninitialized > disabled, but in these cases (variable removal), that actually does make > sense. Thanks! > Fixes tag does not automatically mean it is "for-stable". [1] says: > Patches that fix a severe bug in a released kernel should be directed > toward the stable maintainers by putting a line like this:: > > Cc: stable@vger.kernel.org - Sedat - [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n299
On Thu, Jun 04, 2020 at 01:29:44PM -0700, Nick Desaulniers wrote: > On Thu, Jun 4, 2020 at 1:20 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Thu, Jun 04, 2020 at 12:29:17PM -0700, Nick Desaulniers wrote: > > > On Wed, Jun 3, 2020 at 4:32 PM Kees Cook <keescook@chromium.org> wrote: > > > > > > > > Using uninitialized_var() is dangerous as it papers over real bugs[1] > > > > (or can in the future), and suppresses unrelated compiler warnings (e.g. > > > > "unused variable"). If the compiler thinks it is uninitialized, either > > > > simply initialize the variable or make compiler changes. As a precursor > > > > to removing[2] this[3] macro[4], just remove this variable since it was > > > > actually unused: > > > > > > > > drivers/ide/ide-taskfile.c:232:34: warning: unused variable 'flags' [-Wunused-variable] > > > > unsigned long uninitialized_var(flags); > > > > ^ > > > > > > > > [1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ > > > > [2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ > > > > [3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ > > > > [4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ > > > > > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > > Thanks for the reviews! > > > > > Fixes ce1e518190ea ("ide: don't disable interrupts during kmap_atomic()") > > > > I originally avoided adding Fixes tags because I didn't want these > > changes backported into a -stable without -Wmaybe-uninitialized > > disabled, but in these cases (variable removal), that actually does make > > sense. Thanks! > > Saravana showed me a cool trick for quickly finding commits that > removed a particular identifier that I find faster than `git blame` or > vim-fugitive for the purpose of Fixes tags: > $ git log -S <string> <file> Ah yes, I always have to look up "-S". Good reminder! > I've added it to our wiki: > https://github.com/ClangBuiltLinux/linux/wiki/Command-line-tips-and-tricks#for-finding-which-commit-may-have-removed-a-string-try. > I should update the first tip; what was your suggestion for > constraining the search to the current remote? Ah cool. I've updated it now. It was really to narrow to a "known set of tags", and Linus's tree's tags always start with "v".
diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c index aab6a10435b6..a26f85ab58a9 100644 --- a/drivers/ide/ide-taskfile.c +++ b/drivers/ide/ide-taskfile.c @@ -229,7 +229,6 @@ void ide_pio_bytes(ide_drive_t *drive, struct ide_cmd *cmd, ide_hwif_t *hwif = drive->hwif; struct scatterlist *sg = hwif->sg_table; struct scatterlist *cursg = cmd->cursg; - unsigned long uninitialized_var(flags); struct page *page; unsigned int offset; u8 *buf;
Using uninitialized_var() is dangerous as it papers over real bugs[1] (or can in the future), and suppresses unrelated compiler warnings (e.g. "unused variable"). If the compiler thinks it is uninitialized, either simply initialize the variable or make compiler changes. As a precursor to removing[2] this[3] macro[4], just remove this variable since it was actually unused: drivers/ide/ide-taskfile.c:232:34: warning: unused variable 'flags' [-Wunused-variable] unsigned long uninitialized_var(flags); ^ [1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ [2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ [3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ [4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/ide/ide-taskfile.c | 1 - 1 file changed, 1 deletion(-)