Message-ID: <CACT4Y+aJ4VX5j_Lz7QDb8ynz6vAn_n8d2AM1M5=NUc0ZBUp-dA@mail.gmail.com>
Date: Mon, 16 Sep 2024 15:40:58 +0200
From: Dmitry Vyukov <dvyukov@...gle.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: syzkaller@...glegroups.com, alex.gaynor@...il.com, 
	andriy.shevchenko@...ux.intel.com, bjorn3_gh@...tonmail.com, 
	boqun.feng@...il.com, bpf@...r.kernel.org, gary@...yguo.net, 
	linux-kernel@...r.kernel.org, linux@...musvillemoes.dk, ojeda@...nel.org, 
	pmladek@...e.com, rostedt@...dmis.org, rust-for-linux@...r.kernel.org, 
	senozhatsky@...omium.org, syzkaller-bugs@...glegroups.com, wedsonaf@...il.com, 
	Joe Perches <joe@...ches.com>
Subject: Re: [syzbot] upstream boot error: BUG: unable to handle kernel NULL
 pointer dereference in __dabt_svc

On Wed, 26 Apr 2023 at 13:37, Miguel Ojeda
<miguel.ojeda.sandonis@...il.com> wrote:
> > I understand your intentions and they make sense.
>
> Thanks! I am glad you agree it may have some value -- please see below
> for details.
>
> > But adding this logic to syzbot won't help thousands of users of
> > get_maintainer.pl and dozens of other testing systems. There also will
>
> I haven't said otherwise -- as I said, I think it would be nice to
> have this be part of the kernel itself. :)
>
> > be a bit of get_maintainer.pl inside of syzbot code, so now all kernel
> > developers will need to be aware of it and also submit changes to
> > syzbot when they want to change maintainers logic.
> >
> > I think this also equally applies to all other users of K:.
> > And a number of them had similar complaints re how K; works.
>
> Yeah, I would imagine so.
>
> > I am thinking if K: should actually apply just to patches and be
> > ignored for source files?
>
> I considered that -- for things like Rust, it could make sense, but
> perhaps somebody is already using `K:` to match files they do care
> about, rather than `F:`. So we would need to ask others, but I think
> it is fine.
>
> > If there are files that belong to "rust" (or "bpf" or any other user
> > of K:), then I think these should be just listed explicitly in the
> > subsystem (that should be a limited set of files that can be
> > enumerated with wildcards).
>
> Yes, at least for Rust, modulo omissions, we match files explicitly
> with `F:`. We have a couple unimportant omissions, e.g.
> `.rustfmt.toml`, but I can send a patch.
>
> Personally, I have always seen `F:` files (and `N:`-matched ones) as
> having more weight than `K:`-matched ones, i.e. I saw `K:` as more of
> a "it depends on what it matches -- discretion needed".
>
> From a quick look, most `K:`-using subsystems seem to list `F:` and
> `N:` as I would expect.
>
> > It's also reasonable to apply K: to patches.
>
> Yes, definitely, for Rust, that is our main use case, i.e. it is
> mainly why we wanted to have the `K:` entry: to catch changes to
> things that are tagged with "Rust" in C files (early on, at least).
>
> It is particularly important for us, since we are also considering
> having more of these annotations in the future.
>
> > But if a random source file happened to mention "rust" somewhere once,
> > I am not sure you want to be CCed on all issues in that file.
> > Does it sound reasonable?
>
> For Rust, yes, that would probably work for us. Not sure for all
> subsystems using `K:`, though.
>
> Having said that, I suggested including the kernel config too in this
> decision (i.e. not for the patches case, but for testers finding
> runtime issues), because it adds information: it leaves reports out
> when something is not even enabled but matched via `K:`, but still
> allows a Cc when matched via `K:` and enabled. It is, of course, still
> potentially a false positive, but some subsystems may want to hear
> about those.
>
> For instance, for Rust, this would be fine early on, since we don't
> expect many to have `RUST=y` to begin with, and thus the odd false
> positive report via `K:` is fine. Later on, this heuristic may change,
> and we may not change those matches anymore (especially since, by
> then, the goal is that subsystems would be taking care of their own
> Rust bits).
>
> This is what I was suggesting to then put in `get_maintainer.pl`, e.g.
> a `--bot` option (or `--runtime`, or `--config-based-filtering`, or
> similar) option. Then the bots can add that option on their side.
>
> Thanks again for considering this!

Was looking at what's the status of this, and if we need to file a
feature request for syzbot.
Turns out Joe Perches fixed this a bit ago by adding
--keywords-in-file flag (off by default):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=71ca5ee18708c1f9f086e20ac0a657009bcfe43a

I think that's the right thing to do. syzbot won't be confused by
widely matched K: patterns with sending reports (since it runs
get_maintainers.pl on files).

Powered by blists - more mailing lists