| Message ID | 20210430060049.FrKVS3ZER%akpm@linux-foundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [001/178] arch/ia64/kernel/head.S: remove duplicate include | expand |
On Thu, Apr 29, 2021 at 11:00 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > From: Andrey Konovalov <andreyknvl@google.com> > Subject: kasan: detect false-positives in tests > > Currently, KASAN-KUnit tests can check that a particular annotated part of > code causes a KASAN report. However, they do not check that no unwanted > reports happen between the annotated parts. > > This patch implements these checks. No. This patch does no such thing. This patch is untested garbage that doesn't even compile. > + if (IS_ENABLED(CONFIG_KASAN_HW_TAGS)) { \ > + if (READ_ONCE(fail_data.report_found)) \ > + kasan_enable_tagging(); \ > + migrate_enable(); \ kasan_enable_tagging() was renamed to kasan_enable_tagging_sync() in commit 2603f8a78dfb ("kasan: Add KASAN mode kernel parameter") and this patch was clearly rebased onto current -git without any testing at all. You can even see the effects of the rename in the patch itself, in that the code it removes has the new name: - if (IS_ENABLED(CONFIG_KASAN_HW_TAGS) && \ - !kasan_async_mode_enabled()) { \ - if (READ_ONCE(fail_data.report_found)) \ - kasan_enable_tagging_sync(); \ but then the patch re-introduces the old name in the new version of that code. Andrew, you really need to stop using your garbage workflow of randomly rebasing patches at the last minute. In the cover letter, you clearly say: "178 patches, based on 8ca5297e7e38f2dc8c753d33a5092e7be181fff0." where that commit was basically my top commit as of yesterday afternoon - and that base very much has that rename in place. So you rebased your patch series - apparently without any testing at all - since that afternoon commit. This *HAS* to stop. If you use your old legacy "patch series" model, then dammit, stop sending those patches on top of random commits. Pick a base, and *STICK* to it. Make sure your patch series is actually *tested*. Because I'd much rather handle conflicts and blame myself if I screw something up during the merge, than get something from you that is untested and may be subtly - or as in this case, very much not-subtly - broken. Right now, the way you work, all the work that Stephen Rothwell has done to merge your patch series and have it tested in linux-next is basically made almost entirely worthless, because you end up changing your patches and rebasing it all the last minute, and all the testing it got earlier is now thrown right out the window. What used to be a perfectly cromulent patch is now garbage that doesn't even compile. I'm going to fix this one patch up by hand, but I really need you to stop doing this. Use the previous release as a base (ie in this case 5.12), and don't do any last-minute rebasing. If you have patches in your patch-series that depend on stuff that comes in this merge window, just drop them - or at least don't send them to me. Or better yet, base it on something even older than the release that opens the merge window, so that you don't have to rebase anything *during* the merge window, and so that your patch series actually gets tested as-is in the real format in linux-next even before the merge window opens. If you had created this series two weeks ago, marked it as "ready for the 5.13 merge window", and it had been tested as such in linux-next, we would have known about the conflict from linux-next, and I'd have seen it as something that was trivial to fix when I did the merge. And that would catch all the cases that I can't catch, because linux-next actually does a whole lot more build-testing than I do. Different architectures, lots of different configs etc. Instead, I applied a series of 178 patches (ok, 175 after I had dropped a couple), saw nothing wrong, and did my (very basic and minimal) build testing, and saw that the patches you sent to me didn't even compile. I bet a fixed workflow will make it easier for Stephen too, to not continually have to rebase your series, and have a separate queue for patch-series that get randomly updated. I'm perfectly happy with you not using git - I apply patches from others too. But I don't want to see this broken "rebase with no testing" model. A patch queue that has its base changed has all the same issues that a blind "git rebase" has, and is wrong for all the same reasons - all whether you use git or not. Linus
On Fri, 30 Apr 2021 11:32:43 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > This patch is untested garbage that doesn't even compile. Sorry, I fat-fingered a thing and used gcc-9, which disables CONFIG_KASAN. > Use the previous release as a base (ie in this case 5.12) Not a problem for the first batch of patches, but what base do I use for the second and succeeding batches? > If you have patches in > your patch-series that depend on stuff that comes in this merge > window, just drop them - or at least don't send them to me. Maybe 10% of the patches I carry are based on changes which are in linux-next. Things like - tree-wide renames which need to catch other developers adding instances of the old name. They'll pop up in third-party testing and I do a last-minute grep to find these. - changes which I've worked with other developers to stage things in that order. - removal of duplicated includes for which I need to do a last-minute check that nobody has gone and removed the other include. - new syscalls which would generate a ton of conflicts with new syscalls in other trees. - right now I'm carrying a pile of little spelling fixes. I stage these after linux-next to find out whether other tree maintainers have independently merged the same patches, or partially similar ones from others. - etc. I send this sort of material to you very late in the merge window, after the prerequisites have been merged up.
On Sun, May 2, 2021 at 11:04 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > Sorry, I fat-fingered a thing and used gcc-9, which disables CONFIG_KASAN. My point is that linux-next would have caught it. And that commit _was_ in linux-next. But it was rebased literally just hours before being sent to me, and thus all the testing that it got was thrown away. > > Use the previous release as a base (ie in this case 5.12) > > Not a problem for the first batch of patches, but what base do I use for > the second and succeeding batches? Well, the first batch is usually the biggest and most core one, and in many ways the most important that it would have been a branch of its own, and be something that has actually been tested as-is in linux-next. Ad to succeeding batches.. Optimally if they don't have any dependencies on other trees or the first batch, they'd actually entirely independent of the first batch, and just a separate patch queue entirely, and tested as such in linux-next (and again on top of some previous base). But that kind of workflow would require you literally have multiple separate patch-queues that you test independently. That does sound like a good idea, but it also sounds very much like a "git topic branch" model, and I think quilt basically is a "single series" tool, not able to handle multiple independent series? I don't know quilt, and a quick google of "multiple independent quilt patch series" ended up at a web page by Andreas Grünbacher that was so old that it was in Latin1 and doesn't even display properly on any modern browser. Which is a statement in itself, but whatever. I'd actually be perfectly ok with being told that subsequent patches be based on top of your previous patch series: I already create a branch called "akpm" for applying all your patches, and while right now it's a temporary branch that gets removed after I merge it, I could easily just keep it around - and then apply your next series on top of it. So the only issues would be the things that actually end up being dependent on other branches in linux-next: > Maybe 10% of the patches I carry are based on changes which are in > linux-next. These are the ones that you'd have to keep separate, in order to not rebase the patches that _aren't_ based on linux-next changes.. Again, I don't know how to do that with quilt (and iirc, you aren't actually using quilt, you're using your own extended version?) The quilt man-page does have some signs that there can be multiple series of patches (wording like "current series" vs "another series", and "Different series files can be used.."), but the actual quilt commands don't really show much sign of switching between different patch series. So I get the feeling that it's more of a "theoretically possible" thing rather than something that is actually supported by the tooling natively. Linus
On Sun, 2 May 2021 12:08:17 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > Use the previous release as a base (ie in this case 5.12) > > > > Not a problem for the first batch of patches, but what base do I use for > > the second and succeeding batches? > > Well, the first batch is usually the biggest and most core one, and in > many ways the most important that it would have been a branch of its > own, and be something that has actually been tested as-is in > linux-next. > > Ad to succeeding batches.. Optimally if they don't have any > dependencies on other trees or the first batch, they'd actually > entirely independent of the first batch, and just a separate patch > queue entirely, and tested as such in linux-next (and again on top of > some previous base). The mm/ patches tend to be significantly interdependent. I try to separate things into mm/whatever subparts, but things are often quite smeary, both textually and conceptually. I don't send the whole mm/ queue in a single hit because it tends to be rather large. I could do so. > But that kind of workflow would require you literally have multiple > separate patch-queues that you test independently. That does sound > like a good idea, but it also sounds very much like a "git topic > branch" model, and I think quilt basically is a "single series" tool, > not able to handle multiple independent series? > > I don't know quilt, and a quick google of "multiple independent quilt > patch series" ended up at a web page by Andreas Grünbacher that was so > old that it was in Latin1 and doesn't even display properly on any > modern browser. I haven't found a need to do this. I presently identify 353 subsystems in the -mm patch queue. Most have zero patches in them most of the time. But apart from mm/ being all tangled up internally, I basically never see any overlap between these subsystems. So I stack 'em all up in the same series. > I'd actually be perfectly ok with being told that subsequent patches > be based on top of your previous patch series: I already create a > branch called "akpm" for applying all your patches, and while right > now it's a temporary branch that gets removed after I merge it, I > could easily just keep it around - and then apply your next series on > top of it. OK, I'll do that this week and shall have a bit of a think. > So the only issues would be the things that actually end up being > dependent on other branches in linux-next: > > > Maybe 10% of the patches I carry are based on changes which are in > > linux-next. > > These are the ones that you'd have to keep separate, in order to not > rebase the patches that _aren't_ based on linux-next changes.. > > Again, I don't know how to do that with quilt (and iirc, you aren't > actually using quilt, you're using your own extended version?) Yes, my patch-scripts is quilt's grandfather and I continue to use and evolve it. > The quilt man-page does have some signs that there can be multiple > series of patches (wording like "current series" vs "another series", > and "Different series files can be used.."), but the actual quilt > commands don't really show much sign of switching between different > patch series. So I get the feeling that it's more of a "theoretically > possible" thing rather than something that is actually supported by > the tooling natively. Easy. You'd have a bunch of different series files and then do ln -s series-ocfs2 series ln -s series-procfs series etc to work on a particular series. Then for assembling an entire tree, have a master series-of-series file which contains the names of the various sub-series files, to define the stacking order: series-ocfs2 series-procfs ... Then cat $(cat series-of-series) > series and go for it. But as said, I haven't seen the need because they're almost always all orthogonal. A fairly recent series file is at https://ozlabs.org/~akpm/mmotm/series. It defines the applying order, the various subsystems, tags for which bits are to be included in linux-next, lots of notes-to-self, etc.
--- a/lib/test_kasan.c~kasan-detect-false-positives-in-tests +++ a/lib/test_kasan.c @@ -54,6 +54,10 @@ static int kasan_test_init(struct kunit multishot = kasan_save_enable_multi_shot(); kasan_set_tagging_report_once(false); + fail_data.report_found = false; + fail_data.report_expected = false; + kunit_add_named_resource(test, NULL, NULL, &resource, + "kasan_data", &fail_data); return 0; } @@ -61,6 +65,7 @@ static void kasan_test_exit(struct kunit { kasan_set_tagging_report_once(true); kasan_restore_multi_shot(multishot); + KUNIT_EXPECT_FALSE(test, fail_data.report_found); } /** @@ -78,33 +83,31 @@ static void kasan_test_exit(struct kunit * fields, it can reorder or optimize away the accesses to those fields. * Use READ/WRITE_ONCE() for the accesses and compiler barriers around the * expression to prevent that. + * + * In between KUNIT_EXPECT_KASAN_FAIL checks, fail_data.report_found is kept as + * false. This allows detecting KASAN reports that happen outside of the checks + * by asserting !fail_data.report_found at the start of KUNIT_EXPECT_KASAN_FAIL + * and in kasan_test_exit. */ -#define KUNIT_EXPECT_KASAN_FAIL(test, expression) do { \ - if (IS_ENABLED(CONFIG_KASAN_HW_TAGS) && \ - !kasan_async_mode_enabled()) \ - migrate_disable(); \ - WRITE_ONCE(fail_data.report_expected, true); \ - WRITE_ONCE(fail_data.report_found, false); \ - kunit_add_named_resource(test, \ - NULL, \ - NULL, \ - &resource, \ - "kasan_data", &fail_data); \ - barrier(); \ - expression; \ - barrier(); \ - if (kasan_async_mode_enabled()) \ - kasan_force_async_fault(); \ - barrier(); \ - KUNIT_EXPECT_EQ(test, \ - READ_ONCE(fail_data.report_expected), \ - READ_ONCE(fail_data.report_found)); \ - if (IS_ENABLED(CONFIG_KASAN_HW_TAGS) && \ - !kasan_async_mode_enabled()) { \ - if (READ_ONCE(fail_data.report_found)) \ - kasan_enable_tagging_sync(); \ - migrate_enable(); \ - } \ +#define KUNIT_EXPECT_KASAN_FAIL(test, expression) do { \ + if (IS_ENABLED(CONFIG_KASAN_HW_TAGS) && \ + !kasan_async_mode_enabled()) \ + migrate_disable(); \ + KUNIT_EXPECT_FALSE(test, READ_ONCE(fail_data.report_found)); \ + WRITE_ONCE(fail_data.report_expected, true); \ + barrier(); \ + expression; \ + barrier(); \ + KUNIT_EXPECT_EQ(test, \ + READ_ONCE(fail_data.report_expected), \ + READ_ONCE(fail_data.report_found)); \ + if (IS_ENABLED(CONFIG_KASAN_HW_TAGS)) { \ + if (READ_ONCE(fail_data.report_found)) \ + kasan_enable_tagging(); \ + migrate_enable(); \ + } \ + WRITE_ONCE(fail_data.report_found, false); \ + WRITE_ONCE(fail_data.report_expected, false); \ } while (0) #define KASAN_TEST_NEEDS_CONFIG_ON(test, config) do { \