Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[Jonathan Corbet]

On Mon, 08 Jan 2018 10:39:14 +1100
NeilBrown  wrote:

> > 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.  
> 
> Is this the sort of thing that might result in that happy day?
> I didn't spend tooo much time on it, in case including the
> kernel-doc in credentials.rst like this was the wrong direction.

>From a very quick look, yes. I'll take a closer look soon.  Life has been
a bit ... busy ... around here...

Thanks,

jon
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[NeilBrown]

On Sat, Jan 06 2018, Jonathan Corbet wrote:

> 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.

Is this the sort of thing that might result in that happy day?
I didn't spend tooo much time on it, in case including the
kernel-doc in credentials.rst like this was the wrong direction.

Thanks,
NeilBrown


--8<-
From: NeilBrown 
Subject: [PATCH] Documentation: include kernel-doc in credentials.rst

- kernel-doc from include/linux/cred.h, kernel/cred.c, and
  kernel/group.c is included in credentials.rst.
- Various function references in credentials.rst are changed
  to link to this documentation.
- Various minor improvements to the documentation.

Signed-off-by: NeilBrown 
---
 Documentation/security/credentials.rst | 55 ++
 kernel/groups.c| 19 +++-
 2 files changed, 47 insertions(+), 27 deletions(-)

diff --git a/Documentation/security/credentials.rst 
b/Documentation/security/credentials.rst
index 66a2e24939d8..fd1b7d3b2a78 100644
--- a/Documentation/security/credentials.rst
+++ b/Documentation/security/credentials.rst
@@ -296,7 +296,7 @@ for example), it must be considered immutable, barring two 
exceptions:
 
 To catch accidental credential alteration at compile time, struct task_struct
 has _const_ pointers to its credential sets, as does struct file.  Furthermore,
-certain functions such as ``get_cred()`` and ``put_cred()`` operate on const
+certain functions such as :c:func:`get_cred` and :c:func:`put_cred` operate on 
const
 pointers, thus rendering casts unnecessary, but require to temporarily ditch
 the const qualification to be able to alter the reference count.
 
@@ -391,8 +391,8 @@ This does all the RCU magic inside of it.  The caller must 
call put_cred() on
 the credentials so obtained when they're finished with.
 
 .. note::
-   The result of ``__task_cred()`` should not be passed directly to
-   ``get_cred()`` as this may race with ``commit_cred()``.
+   The result of :c:func:`__task_cred()` should not be passed directly to
+   :c:func:`get_cred` as this may race with :c:func:`commit_cred`.
 
 There are a couple of convenience functions to access bits of another task's
 credentials, hiding the RCU magic from the caller::
@@ -406,7 +406,7 @@ If the caller is holding the RCU read lock at the time 
anyway, then::
__task_cred(task)->euid
 
 should be used instead.  Similarly, if multiple aspects of a task's credentials
-need to be accessed, RCU read lock should be used, ``__task_cred()`` called,
+need to be accessed, RCU read lock should be used, :c:func:`__task_cred` 
called,
 the result stored in a temporary pointer and then the credential aspects called
 from that before dropping the lock.  This prevents the potentially expensive
 RCU magic from being invoked multiple times.
@@ -433,11 +433,8 @@ alter those of another task.  This means that it doesn't 
need to use any
 locking to alter its own credentials.
 
 To alter the current process's credentials, a function should first prepare a
-new set of credentials by calling::
-
-   struct cred *prepare_creds(void);
-
-this locks current->cred_replace_mutex and then allocates and constructs a
+new set of credentials by calling :c:func:`prepare_creds`.
+This locks ``current->cred_replace_mutex`` and then allocates and constructs a
 duplicate of the current process's credentials, returning with the mutex still
 held if successful.  It returns NULL if not successful (out of memory).
 
@@ -453,10 +450,7 @@ still at this point.
 
 
 When the credential set is ready, it should be committed to the current process
-by calling::
-
-   int commit_creds(struct cred *new);
-
+by calling :c:func:`commit_creds`.
 This will alter various aspects of the credentials and the process, giving the
 LSM a chance to do likewise, then it will use ``rcu_assign_pointer()`` to
 actually commit the new credentials to ``current->cred``, it will release
@@ -467,20 +461,17 @@ This function is guaranteed to return 0, so that it can 
be tail-called at the
 end of such functions as ``sys_setresuid()``.
 
 Note that this function consumes the caller's reference to the new credentials.
-The caller should _not_ call ``put_cred()`` on the new credentials afterwards.
+The caller should _not_ call :c:func:`put_cred` on the new credentials 
afterwards.
 
 Furthermore, once this function has been called on a new set of credentials,
 those credentials may _not_ be changed further.
 
 
 Should the security checks fail or some other error occur after

Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[Randy Dunlap]

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.


-- 
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[Matthew Wilcox]

On Sat, Jan 06, 2018 at 11:09:08AM -0700, Jonathan Corbet wrote:
> On Tue, 2 Jan 2018 13:04:31 -0800
> Matthew Wilcox  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 :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[Jonathan Corbet]

On Tue, 2 Jan 2018 13:04:31 -0800
Matthew Wilcox  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
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list

[Matthew Wilcox]

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
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html