diff mbox

Documentation: security/credentials.rst: explain need to sort group_list

Message ID 87wp10dlgk.fsf@notabene.neil.brown.name
State New, archived
Headers show

Commit Message

NeilBrown Jan. 2, 2018, 9:01 p.m. UTC
This patch updates the documentation with the observations that led
to commit bdcf0a423ea1 ("kernel: make groups_sort calling a
responsibility group_info allocators") and the new behaviour required.
Specifically that groups_sort() should be called on a new group_list
before set_groups() or set_current_groups() is called.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 Documentation/security/credentials.rst | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Matthew Wilcox Jan. 2, 2018, 9:04 p.m. UTC | #1
On Wed, Jan 03, 2018 at 08:01:15AM +1100, NeilBrown wrote:
>  
> +When replacing the group list, the new list must be sorted before it
> +is added to the credential, as a binary search is used to test for
> +membership.  In practice, this means ``groups_sort()`` should be

For a .rst file, shouldn't we be using :c:func:`groups_sort` instead of
``groups_sort()``?

> +called before ``set_groups()`` or ``set_current_groups()``.
> +``groups_sort()`` must not be called on a ``struct group_list`` which
> +is shared as it may permute elements as part of the sorting process
> +even if the array is already sorted.
>  
>  When the credential set is ready, it should be committed to the current process
>  by calling::
> -- 
> 2.14.0.rc0.dirty
>
Jonathan Corbet Jan. 6, 2018, 6:09 p.m. UTC | #2
On Tue, 2 Jan 2018 13:04:31 -0800
Matthew Wilcox <willy@infradead.org> wrote:

> > +When replacing the group list, the new list must be sorted before it
> > +is added to the credential, as a binary search is used to test for
> > +membership.  In practice, this means ``groups_sort()`` should be  
> 
> For a .rst file, shouldn't we be using :c:func:`groups_sort` instead of
> ``groups_sort()``?

There is value in using the c:func syntax, as it will generate
cross-references to the kerneldoc comments for those functions.  In this
case, it would appear that these comments exist, but nobody has pulled
them into the docs yet.  I took the liberty of adding :c:func: references
to this patch on its way into docs-next so that things will Just Work on
that happy day when somebody adds the function documentation.

Thanks,

jon
Matthew Wilcox Jan. 6, 2018, 8:20 p.m. UTC | #3
On Sat, Jan 06, 2018 at 11:09:08AM -0700, Jonathan Corbet wrote:
> On Tue, 2 Jan 2018 13:04:31 -0800
> Matthew Wilcox <willy@infradead.org> wrote:
> 
> > > +When replacing the group list, the new list must be sorted before it
> > > +is added to the credential, as a binary search is used to test for
> > > +membership.  In practice, this means ``groups_sort()`` should be  
> > 
> > For a .rst file, shouldn't we be using :c:func:`groups_sort` instead of
> > ``groups_sort()``?
> 
> There is value in using the c:func syntax, as it will generate
> cross-references to the kerneldoc comments for those functions.  In this
> case, it would appear that these comments exist, but nobody has pulled
> them into the docs yet.  I took the liberty of adding :c:func: references
> to this patch on its way into docs-next so that things will Just Work on
> that happy day when somebody adds the function documentation.

Thanks for making that substitution.

I've been thinking about all the kernel-doc we have that's completely
unincorporated.  I've also been thinking about core-api/kernel-api.rst
which to my mind is completely unreadable in its current form -- look at
https://www.kernel.org/doc/html/latest/core-api/kernel-api.html and you
wouldn't really know there's anything in it beyond the List Management
Functions.

I think the right path forward is to have kernel-api.rst be the dumping
ground for all the files with kernel-doc but nothing more.  That gives
us somewhere to link to.

Then we need little stories about how all the functions in a subsystem
fit together.  For example, we can create a list.rst which explains how
this is a doubly-linked list that you use by embedding a list_head into
your data structure, and has O(1) insertion/deletion, etc, etc.  Then we
would move all the list.h kernel-doc from kernel-api.rst into list.rst.

Is this a reasonable strategy to follow?  Does anyone have a better
strategy?  I mean ... you've written a book, you presumably have some
idea about how to present the vast amount of information we've accumulated
over the years :-)
Randy Dunlap Jan. 6, 2018, 10:36 p.m. UTC | #4
On 01/06/18 12:20, Matthew Wilcox wrote:
> 
> I've been thinking about all the kernel-doc we have that's completely
> unincorporated.  I've also been thinking about core-api/kernel-api.rst
> which to my mind is completely unreadable in its current form -- look at
> https://www.kernel.org/doc/html/latest/core-api/kernel-api.html and you
> wouldn't really know there's anything in it beyond the List Management
> Functions.

The index is on the left side, but would be better (duplicated?) at the beginning
of the chapter.  The left side is still useful for navigation, but then it
scrolls away too quickly when the right side text is scrolled.

> I think the right path forward is to have kernel-api.rst be the dumping
> ground for all the files with kernel-doc but nothing more.  That gives
> us somewhere to link to.

FWFW, I have recently done firewire.rst, infiniband.rst, and some additions
to scsi.rst.  But the new firewire.rst and infiniband.rst could use some
introductory material before just jumping into the API.


> Then we need little stories about how all the functions in a subsystem
> fit together.  For example, we can create a list.rst which explains how
> this is a doubly-linked list that you use by embedding a list_head into
> your data structure, and has O(1) insertion/deletion, etc, etc.  Then we
> would move all the list.h kernel-doc from kernel-api.rst into list.rst.
Jonathan Corbet Jan. 8, 2018, 4:36 p.m. UTC | #5
On Sat, 6 Jan 2018 12:20:13 -0800
Matthew Wilcox <willy@infradead.org> wrote:

> I've been thinking about all the kernel-doc we have that's completely
> unincorporated.  I've also been thinking about core-api/kernel-api.rst
> which to my mind is completely unreadable in its current form -- look at
> https://www.kernel.org/doc/html/latest/core-api/kernel-api.html and you
> wouldn't really know there's anything in it beyond the List Management
> Functions.

Yes, it's a mess.

> I think the right path forward is to have kernel-api.rst be the dumping
> ground for all the files with kernel-doc but nothing more.  That gives
> us somewhere to link to.

That's kind of what it is now :)

Note that we have a script now (scripts/find-unused-docs.sh) that can
seek out kerneldoc comments that aren't yet pulled in to the RST document
tree.  

> Then we need little stories about how all the functions in a subsystem
> fit together.  For example, we can create a list.rst which explains how
> this is a doubly-linked list that you use by embedding a list_head into
> your data structure, and has O(1) insertion/deletion, etc, etc.  Then we
> would move all the list.h kernel-doc from kernel-api.rst into list.rst.
> 
> Is this a reasonable strategy to follow?  Does anyone have a better
> strategy? 

That more-or-less corresponds to what I've been aiming at.  All of
Documentation/ currently is a bit of a dumping ground; I'd really like to
knit it together into something coherent and useful.  Progress on that
front has been slower than I would have liked - I've been distracted by a
number of other things.  Funny, I've never heard of that happening before.

The "little stories", incidentally, can also go into DOC: sections in the
source itself.  There's a bit of tension, though, between doing that and
putting the stories in the RST files.  The former approach puts the docs
where they might be most useful and most likely to be updated, but it can
also leave the RST files as a collection of kerneldoc directives that is
rather unhelpful to read in unprocessed form.  Since the whole idea is to
not require any sort of processing to make the docs useful, I don't
entirely like that outcome.

Thanks,

jon
diff mbox

Patch

diff --git a/Documentation/security/credentials.rst b/Documentation/security/credentials.rst
index 66a2e24939d8..5d659e3e52ad 100644
--- a/Documentation/security/credentials.rst
+++ b/Documentation/security/credentials.rst
@@ -451,6 +451,13 @@  checks and hooks done.  Both the current and the proposed sets of credentials
 are available for this purpose as current_cred() will return the current set
 still at this point.
 
+When replacing the group list, the new list must be sorted before it
+is added to the credential, as a binary search is used to test for
+membership.  In practice, this means ``groups_sort()`` should be
+called before ``set_groups()`` or ``set_current_groups()``.
+``groups_sort()`` must not be called on a ``struct group_list`` which
+is shared as it may permute elements as part of the sorting process
+even if the array is already sorted.
 
 When the credential set is ready, it should be committed to the current process
 by calling::