Re: [PATCH] Documentation: security/credentials.rst: explain need to sort group_list
On Mon, 08 Jan 2018 10:39:14 +1100 NeilBrownwrote: > > 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
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: NeilBrownSubject: [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
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
On Sat, Jan 06, 2018 at 11:09:08AM -0700, Jonathan Corbet wrote: > On Tue, 2 Jan 2018 13:04:31 -0800 > Matthew Wilcoxwrote: > > > > +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
On Tue, 2 Jan 2018 13:04:31 -0800 Matthew Wilcoxwrote: > > +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
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