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 <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Co-developed-by: Jonathan Corbet <[email protected]>
Signed-off-by: Jonathan Corbet <[email protected]>
Co-developed-by: Stefano Zacchiroli <[email protected]>
Signed-off-by: Stefano Zacchiroli <[email protected]>
Co-developed-by: Steven Rostedt <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
Acked-by: Steve Rostedt <[email protected]>
Acked-by: Laura Abbott <[email protected]>
Reviewed-by: Julia Lawall <[email protected]>
Reviewed-by: Wenwen Wang <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
Documentation/admin-guide/index.rst | 1 +
.../admin-guide/researcher-guidelines.rst | 141 ++++++++++++++++++
2 files changed, 142 insertions(+)
create mode 100644 Documentation/admin-guide/researcher-guidelines.rst
diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
index 1bedab498104..7fff0730204d 100644
--- a/Documentation/admin-guide/index.rst
+++ b/Documentation/admin-guide/index.rst
@@ -35,6 +35,7 @@ problems and bugs in particular.
:maxdepth: 1
reporting-issues
+ researcher-guidelines
security-bugs
bug-hunting
bug-bisect
diff --git a/Documentation/admin-guide/researcher-guidelines.rst b/Documentation/admin-guide/researcher-guidelines.rst
new file mode 100644
index 000000000000..993f32d1166c
--- /dev/null
+++ b/Documentation/admin-guide/researcher-guidelines.rst
@@ -0,0 +1,141 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+.. _researcher_guidelines:
+
+Researcher Guidelines
++++++++++++++++++++++
+
+The Linux kernel community welcomes transparent research on the Linux
+kernel, the activities involved in producing it, and any other byproducts
+of its development. Linux benefits greatly from this kind of research, and
+most aspects of Linux are driven by research in one form or another.
+
+The community greatly appreciates if researchers can share preliminary
+findings before making their results public, especially if such research
+involves security. Getting involved early helps both improve the quality
+of research and ability for Linux to improve from it. In any case,
+sharing open access copies of the published research with the community
+is recommended.
+
+This document seeks to clarify what the Linux kernel community considers
+acceptable and non-acceptable practices when conducting such research. At
+the very least, such research and related activities should follow
+standard research ethics rules. For more background on research ethics
+generally, ethics in technology, and research of developer communities
+in particular, see:
+
+* `History of Research Ethics <https://www.unlv.edu/research/ORI-HSR/history-ethics>`_
+* `IEEE Ethics <https://www.ieee.org/about/ethics/index.html>`_
+* `Developer and Researcher Views on the Ethics of Experiments on
+ Open-Source Projects <https://arxiv.org/pdf/2112.13217.pdf>`_
+
+The Linux kernel community expects that everyone interacting with the
+project is participating in good faith to make Linux better. Research on
+any publicly-available artifact (including, but not limited to source
+code) produced by the Linux kernel community is welcome, though research
+on developers must be distinctly opt-in.
+
+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, 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: 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.
+
+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>`. Send a patch (including
+a commit log with all the details listed below), and follow up on any
+feedback from other developers.
+
+When sending patches produced from research, the commit logs should
+contain at least the following details, so that developers have
+appropriate context for understanding the contribution. Answer:
+
+* What is the specific problem that has been found?
+* How could the problem be reached on a running system?
+* What effect would encountering the problem have on the system?
+* How was the problem found? Specifically include details about any
+ testing, static or dynamic analysis programs, and any other tools or
+ methods used to perform the work.
+* Which version of Linux was the problem was found on? Using the most
+ recent release or a recent :doc:`linux-next branch <../process/howto>`
+ is strongly preferred.
+* What was changed to fix the problem, and why it is believed to be correct?
+* How was the change build tested and run-time tested?
+* What prior commit does this change fix? This should go in a "Fixes:"
+ tag as the documentation describes.
+* Who else has reviewed this patch? This should go in appropriate
+ "Reviewed-by:" tags; see below.
+
+For example::
+
+ From: Author <author@email>
+ Subject: [PATCH] drivers/foo_bar: Add missing kfree()
+
+ The error path in foo_bar driver does not correctly free the allocated
+ struct foo_bar_info. This can happen if the attached foo_bar device
+ rejects the initialization packets sent during foo_bar_probe(). This
+ would result in a 64 byte slab memory leak once per device attach,
+ wasting memory resources over time.
+
+ This flaw was found using an experimental static analysis tool we are
+ developing, LeakMagic[1], which reported the following warning when
+ analyzing the v5.15 kernel release:
+
+ path/to/foo_bar.c:187: missing kfree() call?
+
+ Add the missing kfree() to the error path. No other references to
+ this memory exist outside the probe function, so this is the only
+ place it can be freed.
+
+ x86_64 and arm64 defconfig builds with CONFIG_FOO_BAR=y using GCC
+ 11.2 show no new warnings, and LeakMagic no longer warns about this
+ code path. As we don't have a FooBar device to test with, no runtime
+ testing was able to be performed.
+
+ [1] https://url/to/leakmagic/details
+
+ Reported-by: Researcher <researcher@email>
+ Fixes: aaaabbbbccccdddd ("Introduce support for FooBar")
+ Signed-off-by: Author <author@email>
+ Reviewed-by: Reviewer <reviewer@email>
+
+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.
+
+If no one can be found to internally review patches and you need
+help finding such a person, or if you have any other questions
+related to this document and the developer community's expectations,
+please reach out to the private Technical Advisory Board mailing list:
+<[email protected]>.
--
2.30.2
On Wed, Feb 23, 2022 at 04:14:03PM -0800, 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 <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> Co-developed-by: Jonathan Corbet <[email protected]>
> Signed-off-by: Jonathan Corbet <[email protected]>
> Co-developed-by: Stefano Zacchiroli <[email protected]>
> Signed-off-by: Stefano Zacchiroli <[email protected]>
> Co-developed-by: Steven Rostedt <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
> Acked-by: Steve Rostedt <[email protected]>
> Acked-by: Laura Abbott <[email protected]>
> Reviewed-by: Julia Lawall <[email protected]>
> Reviewed-by: Wenwen Wang <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
Acked-by: Gustavo A. R. Silva <[email protected]>
See a comment below...
> ---
> Documentation/admin-guide/index.rst | 1 +
> .../admin-guide/researcher-guidelines.rst | 141 ++++++++++++++++++
> 2 files changed, 142 insertions(+)
> create mode 100644 Documentation/admin-guide/researcher-guidelines.rst
[..]
> +* What is the specific problem that has been found?
> +* How could the problem be reached on a running system?
> +* What effect would encountering the problem have on the system?
> +* How was the problem found? Specifically include details about any
> + testing, static or dynamic analysis programs, and any other tools or
> + methods used to perform the work.
> +* Which version of Linux was the problem was found on? Using the most
I think there is an extra "was" in the question above.
Thanks
--
Gustavo
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 <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> Co-developed-by: Jonathan Corbet <[email protected]>
> Signed-off-by: Jonathan Corbet <[email protected]>
> Co-developed-by: Stefano Zacchiroli <[email protected]>
> Signed-off-by: Stefano Zacchiroli <[email protected]>
> Co-developed-by: Steven Rostedt <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
> Acked-by: Steve Rostedt <[email protected]>
> Acked-by: Laura Abbott <[email protected]>
> Reviewed-by: Julia Lawall <[email protected]>
> Reviewed-by: Wenwen Wang <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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.
> 2 files changed, 142 insertions(+)
> create mode 100644 Documentation/admin-guide/researcher-guidelines.rst
>
> diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
> index 1bedab498104..7fff0730204d 100644
> --- a/Documentation/admin-guide/index.rst
> +++ b/Documentation/admin-guide/index.rst
> @@ -35,6 +35,7 @@ problems and bugs in particular.
> :maxdepth: 1
>
> reporting-issues
> + researcher-guidelines
> security-bugs
> bug-hunting
> bug-bisect
> diff --git a/Documentation/admin-guide/researcher-guidelines.rst b/Documentation/admin-guide/researcher-guidelines.rst
> new file mode 100644
> index 000000000000..993f32d1166c
> --- /dev/null
> +++ b/Documentation/admin-guide/researcher-guidelines.rst
> @@ -0,0 +1,141 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +.. _researcher_guidelines:
> +
> +Researcher Guidelines
> ++++++++++++++++++++++
> +
> +The Linux kernel community
Nitpicking: wondering if this maybe should be something like "The Linux
kernel developer community" (or "Linux kernel's..."?).
> welcomes transparent research on the Linux
> +kernel, the activities involved in producing it, and any other byproducts
> +of its development. Linux benefits greatly from this kind of research, and
> +most aspects of Linux are driven by research in one form or another.
> +
> +The community greatly appreciates if researchers can share preliminary
> +findings before making their results public, especially if such research
> +involves security. Getting involved early helps both improve the quality
> +of research and ability for Linux to improve from it. In any case,
> +sharing open access copies of the published research with the community
> +is recommended.
> +
> +This document seeks to clarify what the Linux kernel community considers
> +acceptable and non-acceptable practices when conducting such research. At
> +the very least, such research and related activities should follow
> +standard research ethics rules. For more background on research ethics
> +generally, ethics in technology, and research of developer communities
> +in particular, see:
> +
> +* `History of Research Ethics <https://www.unlv.edu/research/ORI-HSR/history-ethics>`_
> +* `IEEE Ethics <https://www.ieee.org/about/ethics/index.html>`_
> +* `Developer and Researcher Views on the Ethics of Experiments on
> + Open-Source Projects <https://arxiv.org/pdf/2112.13217.pdf>`_
> +
> +The Linux kernel community expects that everyone interacting with the
> +project is participating in good faith to make Linux better. Research on
> +any publicly-available artifact (including, but not limited to source
> +code) produced by the Linux kernel community is welcome, though research
> +on developers must be distinctly opt-in.
> +
> +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.
> 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"
> +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.
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/[email protected]/
Most of those patches got applied meanwhile afaik.
> Send a patch (including
> +a commit log with all the details listed below), and follow up on any
> +feedback from other developers.
> +
> +When sending patches produced from research, the commit logs should
> +contain at least the following details, so that developers have
> +appropriate context for understanding the contribution. Answer:
> +
> +* What is the specific problem that has been found?
> +* How could the problem be reached on a running system?
> +* What effect would encountering the problem have on the system?
> +* How was the problem found? Specifically include details about any
> + testing, static or dynamic analysis programs, and any other tools or
> + methods used to perform the work.
> +* Which version of Linux was the problem was found on? Using the most
> + recent release or a recent :doc:`linux-next branch <../process/howto>`
> + is strongly preferred.
> +* What was changed to fix the problem, and why it is believed to be correct?
> +* How was the change build tested and run-time tested?
> +* What prior commit does this change fix? This should go in a "Fixes:"
> + tag as the documentation describes.
> +* Who else has reviewed this patch? This should go in appropriate
> + "Reviewed-by:" tags; see below.
> +
> +For example::
> +
> + From: Author <author@email>
> + Subject: [PATCH] drivers/foo_bar: Add missing kfree()
> +
> + The error path in foo_bar driver does not correctly free the allocated
> + struct foo_bar_info. This can happen if the attached foo_bar device
> + rejects the initialization packets sent during foo_bar_probe(). This
> + would result in a 64 byte slab memory leak once per device attach,
> + wasting memory resources over time.
> +
> + This flaw was found using an experimental static analysis tool we are
> + developing, LeakMagic[1], which reported the following warning when
> + analyzing the v5.15 kernel release:
> +
> + path/to/foo_bar.c:187: missing kfree() call?
> +
> + Add the missing kfree() to the error path. No other references to
> + this memory exist outside the probe function, so this is the only
> + place it can be freed.
> +
> + x86_64 and arm64 defconfig builds with CONFIG_FOO_BAR=y using GCC
> + 11.2 show no new warnings, and LeakMagic no longer warns about this
> + code path. As we don't have a FooBar device to test with, no runtime
> + testing was able to be performed.
> +
> + [1] https://url/to/leakmagic/details
> +
> + Reported-by: Researcher <researcher@email>
> + Fixes: aaaabbbbccccdddd ("Introduce support for FooBar")
> + Signed-off-by: Author <author@email>
> + Reviewed-by: Reviewer <reviewer@email>
> +
> +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.
OTOH I fully understand that having things in one place has it's
benefits. If that's wanted, why not put that text as TLDR in
submitting-patches.rst and maintain a copy here? Sure, keeping things in
sync has downsides, but I'd say it's the lesser evil. A copy could also
be avoided by briefly mentioning some of the important bits found in
another document; that's the approach I took in my patches regarding
regressions. To quote:
```
+#. Report your issue as outlined in
Documentation/admin-guide/reporting-issues.rst,
+ it already covers all aspects important for regressions and repeated
+ below for convenience. Two of them are important: start your
report's subject
+ with "[REGRESSION]" and CC or forward it to `the regression mailing list
+ <https://lore.kernel.org/regressions/>`_ ([email protected]).
```
For full context see:
https://lore.kernel.org/regressions/[email protected]/T/#u
> +If no one can be found to internally review patches and you need
> +help finding such a person, or if you have any other questions
> +related to this document and the developer community's expectations,
> +please reach out to the private Technical Advisory Board mailing list:
> +<[email protected]>.
HTH, Ciao, Thorsten
Hello Thorsten,
Am Thu, Feb 24, 2022 at 09:19:24AM +0100 schrieb Thorsten Leemhuis:
> On 24.02.22 01:14, Kees Cook wrote:
> > +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.
>
> OTOH I fully understand that having things in one place has it's
> benefits. If that's wanted, why not put that text as TLDR in
> submitting-patches.rst and maintain a copy here? Sure, keeping things in
> sync has downsides, but I'd say it's the lesser evil. A copy could also
> be avoided by briefly mentioning some of the important bits found in
> another document; that's the approach I took in my patches regarding
> regressions. To quote:
Without further opinion on the topic or content itself:
If there's need to have "copied" parts of the documentation available
in different places, why not put that to a separate file and include
it in all places which need it?
This would solve the manual synchronization issue.
Did that in other projects using sphinx/rst already.
Only thing you have to keep an eye on is whether the surrounding
context at places of the include still matches the included piece.
Greets
Alex
On 24.02.22 10:56, Alexander Dahl wrote:
> Hello Thorsten,
>
> Am Thu, Feb 24, 2022 at 09:19:24AM +0100 schrieb Thorsten Leemhuis:
>> On 24.02.22 01:14, Kees Cook wrote:
>>> +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.
>>
>> OTOH I fully understand that having things in one place has it's
>> benefits. If that's wanted, why not put that text as TLDR in
>> submitting-patches.rst and maintain a copy here? Sure, keeping things in
>> sync has downsides, but I'd say it's the lesser evil. A copy could also
>> be avoided by briefly mentioning some of the important bits found in
>> another document; that's the approach I took in my patches regarding
>> regressions. To quote:
>
> Without further opinion on the topic or content itself:
> If there's need to have "copied" parts of the documentation available
> in different places, why not put that to a separate file and include
> it in all places which need it? [...]
Yeah, I already wondered if that's possible, but never actually
investigated, as I assume the "separate file" aspect you mentioned is a
show-stopper: it makes reading hard for everyone that looks at the rst
files directly -- and that's something we afaics want to support.
Ciao, Thorsten
On Wed, Feb 23, 2022 at 07:00:30PM -0600, Gustavo A. R. Silva wrote:
> On Wed, Feb 23, 2022 at 04:14:03PM -0800, 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 <[email protected]>
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > Co-developed-by: Jonathan Corbet <[email protected]>
> > Signed-off-by: Jonathan Corbet <[email protected]>
> > Co-developed-by: Stefano Zacchiroli <[email protected]>
> > Signed-off-by: Stefano Zacchiroli <[email protected]>
> > Co-developed-by: Steven Rostedt <[email protected]>
> > Signed-off-by: Steven Rostedt <[email protected]>
> > Acked-by: Steve Rostedt <[email protected]>
> > Acked-by: Laura Abbott <[email protected]>
> > Reviewed-by: Julia Lawall <[email protected]>
> > Reviewed-by: Wenwen Wang <[email protected]>
> > Signed-off-by: Kees Cook <[email protected]>
>
> Acked-by: Gustavo A. R. Silva <[email protected]>
>
> See a comment below...
>
> > ---
> > Documentation/admin-guide/index.rst | 1 +
> > .../admin-guide/researcher-guidelines.rst | 141 ++++++++++++++++++
> > 2 files changed, 142 insertions(+)
> > create mode 100644 Documentation/admin-guide/researcher-guidelines.rst
>
> [..]
>
> > +* What is the specific problem that has been found?
> > +* How could the problem be reached on a running system?
> > +* What effect would encountering the problem have on the system?
> > +* How was the problem found? Specifically include details about any
> > + testing, static or dynamic analysis programs, and any other tools or
> > + methods used to perform the work.
> > +* Which version of Linux was the problem was found on? Using the most
>
> I think there is an extra "was" in the question above.
Oops! Thanks; I'll fix it. :)
--
Kees Cook
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 <[email protected]>
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > Co-developed-by: Jonathan Corbet <[email protected]>
> > Signed-off-by: Jonathan Corbet <[email protected]>
> > Co-developed-by: Stefano Zacchiroli <[email protected]>
> > Signed-off-by: Stefano Zacchiroli <[email protected]>
> > Co-developed-by: Steven Rostedt <[email protected]>
> > Signed-off-by: Steven Rostedt <[email protected]>
> > Acked-by: Steve Rostedt <[email protected]>
> > Acked-by: Laura Abbott <[email protected]>
> > Reviewed-by: Julia Lawall <[email protected]>
> > Reviewed-by: Wenwen Wang <[email protected]>
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > 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/[email protected]/
>
> 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
Kees Cook <[email protected]> writes:
> On Thu, Feb 24, 2022 at 09:19:24AM +0100, Thorsten Leemhuis wrote:
>> 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.
Sorry, been a bit distracted...when we were working on this I was more
focused on the text than the location. My own feeling is that
Documentation/process is a better place for this - that's where we tell
the world how to work with the kernel community, after all. I'm not
going to dig in my heels and fight about it, but that's my sense.
Otherwise, it kind of seems like this is ready to go in. I'd like to
apply it before the merge window; lemme know where you want it in the
end and we can get it done.
Thanks,
jon
On 3/4/22 09:16, Jonathan Corbet wrote:
> Kees Cook <[email protected]> writes:
>
>> On Thu, Feb 24, 2022 at 09:19:24AM +0100, Thorsten Leemhuis wrote:
>>> 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.
>
> Sorry, been a bit distracted...when we were working on this I was more
> focused on the text than the location. My own feeling is that
> Documentation/process is a better place for this - that's where we tell
> the world how to work with the kernel community, after all. I'm not
> going to dig in my heels and fight about it, but that's my sense.
>
> Otherwise, it kind of seems like this is ready to go in. I'd like to
> apply it before the merge window; lemme know where you want it in the
> end and we can get it done.
There is a v2 of this patch, but yes, it should be in process/ IMO also.
--
~Randy
On Fri, Mar 04, 2022 at 10:16:58AM -0700, Jonathan Corbet wrote:
> Sorry, been a bit distracted...when we were working on this I was more
> focused on the text than the location. My own feeling is that
> Documentation/process is a better place for this - that's where we tell
> the world how to work with the kernel community, after all. I'm not
> going to dig in my heels and fight about it, but that's my sense.
I had no strong preference. I've moved it to /process now.
> Otherwise, it kind of seems like this is ready to go in. I'd like to
> apply it before the merge window; lemme know where you want it in the
> end and we can get it done.
Agreed. v2 had v1's feedback addressed, and v2 was not further commented
on. v3 now here:
https://lore.kernel.org/lkml/[email protected]
--
Kees Cook