Message-ID: <202202251044.F509C7F3@keescook>
Date:   Fri, 25 Feb 2022 10:55:06 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Thorsten Leemhuis <linux@...mhuis.info>
Cc:     Jonathan Corbet <corbet@....net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Stefano Zacchiroli <zack@...ilon.cc>,
        Steven Rostedt <rostedt@...dmis.org>,
        Laura Abbott <labbott@...nel.org>,
        Julia Lawall <julia.lawall@...ia.fr>,
        Wenwen Wang <wenwen@...uga.edu>, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] Documentation/process: Add Researcher Guidelines

On Thu, Feb 24, 2022 at 09:19:24AM +0100, Thorsten Leemhuis wrote:
> On 24.02.22 01:14, Kees Cook wrote:
> > As a follow-up to the UMN incident[1], the TAB took the responsibility
> > to document Researcher Guidelines so there would be a common place to
> > point for describing our expectations as a developer community.
> > 
> > Document best practices researchers should follow to participate
> > successfully with the Linux developer community.
> > 
> > [1] https://lore.kernel.org/lkml/202105051005.49BFABCE@keescook/
> > 
> > Co-developed-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Co-developed-by: Jonathan Corbet <corbet@....net>
> > Signed-off-by: Jonathan Corbet <corbet@....net>
> > Co-developed-by: Stefano Zacchiroli <zack@...ilon.cc>
> > Signed-off-by: Stefano Zacchiroli <zack@...ilon.cc>
> > Co-developed-by: Steven Rostedt <rostedt@...dmis.org>
> > Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
> > Acked-by: Steve Rostedt <rostedt@...dmis.org>
> > Acked-by: Laura Abbott <labbott@...nel.org>
> > Reviewed-by: Julia Lawall <julia.lawall@...ia.fr>
> > Reviewed-by: Wenwen Wang <wenwen@...uga.edu>
> > Signed-off-by: Kees Cook <keescook@...omium.org>
> > ---
> >  Documentation/admin-guide/index.rst           |   1 +
> >  .../admin-guide/researcher-guidelines.rst     | 141 ++++++++++++++++++
> 
> Hmm, the intro for "Documentation/admin-guide/" states that "The
> following manuals are written for users of the kernel", but the added
> text afaics providing nothing regular users care about. So wouldn't it
> be better if this lived below Documentation/process/ ? It might not a
> really good fit either, but I'd say it's the better place.
> 
> But well, the best person to know is Jonathan, who is listed as a
> Co-developer above, so maybe I'm wrong my suggestion is a bad one.

I started in process/ and eventually settled on admin-guide/ since that's
basically the "front page". But I agree, there isn't an obviously correct
place for it.

> > +Researcher Guidelines
> > ++++++++++++++++++++++
> > +
> > +The Linux kernel community
> 
> Nitpicking: wondering if this maybe should be something like "The Linux
> kernel developer community" (or "Linux kernel's..."?).

The idea was to lot limit this to strictly developers. (i.e. cast a wide
net.)

> [...]
> > +Passive research that is based entirely on publicly available sources,
> > +including posts to public mailing lists and commits to public
> > +repositories, is clearly permissible. Though, as with any research,
> > +standard ethics must still be followed.
> > +
> > +Active research on developer behavior,
> 
> Nitpicking: when reading this for the first time I here wondered what is
> precisely meant by "Active research". Is this maybe an established term
> in academia I'm simply not aware of? If not, maybe simply writing
> something like "Other research on developer behavior" or "Research on
> developer behavior where you engage in development" avoid this.
> 
> > however, must be done with the
> > +explicit agreement of, and full disclosure to, the individual developers
> > +involved. Developers cannot be interacted with/experimented on without
> > +consent; this, too, is standard research ethics.
> > +
> > +To help clarify: 
> 
> Follow up to above remark: If "Active research" stays above, I'd say it
> might be worth repeating the term here to make clear what's being clarified.

Hm, yeah, it was a "passive" / "active" comparison, in the sense of
trying to describe what is examining subject's artifacts (passive)
and interacting with subjects (active).

I don't think it's an academic term, but rather an engineering term?

> > sending patches to developers *is* interacting
> > +with them, but they have already consented to receiving *good faith
> > +contributions*. Sending intentionally flawed/vulnerable patches or
> > +contributing misleading information to discussions is not consented
> > +to. Such communication can be damaging to the developer (e.g. draining
> > +time, effort, and morale) and damaging to the project by eroding
> > +the entire developer community's trust in the contributor (and the
> > +contributor's organization as a whole), undermining efforts to provide
> > +constructive feedback to contributors, and putting end users at risk of
> > +software flaws.
> 
> Nitpicking again: Quite a long sentence. Maybe split it up with
> something like a "s!, undermining !; such an approach would thus undermine"

Yeah, I can tweak this. That did bother me a little too.

> > +Participation in the development of Linux itself by researchers, as
> > +with anyone, is welcomed and encouraged. Research into Linux code is
> > +a common practice, especially when it comes to developing or running
> > +analysis tools that produce actionable results.
> > +
> > +When engaging with the developer community, sending a patch has
> > +traditionally been the best way to make an impact. Linux already has
> > +plenty of known bugs -- what's much more helpful is having vetted fixes.
> > +Before contributing, carefully read the documentation on
> > +:doc:`submitting patches <../process/submitting-patches>`,
> > +:doc:`reporting issues <reporting-issues>`, and
> > +:doc:`handling serious flaws <security-bugs>`.
> 
> Wonder if Documentation/process/{development-process.rst,5.Posting.rst}
> should be linked as well.

I wasn't sure what the balance should be between not enough and too much
information. :)

> Additionally not my area of expertise, but from what I'd noticed it
> seems it's better to avoid the :doc:`foo` tag if there is no strict need
> (and I don't think there is one in this case). For the background see here:
> 
> https://lore.kernel.org/all/cover.1623824363.git.mchehab+huawei@kernel.org/
> 
> Most of those patches got applied meanwhile afaik.

Oh! Thanks for pointing that out; I'll drop all of that.

> [...]
> > +If you are a first time contributor it is recommended that the patch
> > +itself be vetted by others privately before being posted to public lists.
> > +(This is required if you have been explicitly told your patches need
> > +more careful internal review.) These people are expected to have their
> > +"Reviewed-by" tag included in the resulting patch. Finding another
> > +developer familiar with Linux contribution, especially within your own
> > +organization, and having them help with reviews before sending them to
> > +the public mailing lists tends to significantly improve the quality of the
> > +resulting patches, and there by reduces the burden on other developers.
> 
> I like the section, but I wonder why it's needed here. Is there anything
> specific to patches produced from research in it there I missed when
> reading it? If not: Wouldn't it be better to include that section as a
> TLDR in Documentation/process/submitting-patches.rst and point there
> instead? We already have at least two places describing how to submit
> patches, creating yet another one (even if it's just in such a brief
> version) somehow feels slightly wrong to me.

Yeah, there is some redundancy here, but I wanted to have an example
specifically tuned to the scenario of really fleshing out the parts we'd
expect from a researcher to help show what context developers are
expecting.

Thanks for the review!

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists