Message-ID: <87608dck7x.fsf@notabene.neil.brown.name>
Date:   Mon, 08 Jan 2018 10:39:14 +1100
From:   NeilBrown <neilb@...e.com>
To:     Jonathan Corbet <corbet@....net>,
        Matthew Wilcox <willy@...radead.org>
Cc:     linux-doc@...r.kernel.org, dhowells@...hat.com,
        Thiago Rafael Becker <thiago.becker@...il.com>,
        viro@...iv.linux.org.uk, schwidefsky@...ibm.com,
        bfields@...ldses.org, linux-nfs@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: 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: NeilBrown <neilb@...e.com>
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 <neilb@...e.com>
---
 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
-``prepare_creds()`` has been called, then the following function should be
-invoked::
-
-	void abort_creds(struct cred *new);
-
+:c:func:`prepare_creds` has been called, then the function
+:c:func:`abort_creds` should be invoked.
 This releases the lock on ``current->cred_replace_mutex`` that
-``prepare_creds()`` got and then releases the new credentials.
+:c:func:`prepare_creds` got and then releases the new credentials.
 
 
 A typical credentials alteration function would look something like this::
@@ -512,19 +503,20 @@ There are some functions to help manage credentials:
 
  - ``void put_cred(const struct cred *cred);``
 
-     This releases a reference to the given set of credentials.  If the
+     :c:func:`put_cred` releases a reference to the given set of credentials.  If the
      reference count reaches zero, the credentials will be scheduled for
      destruction by the RCU system.
 
  - ``const struct cred *get_cred(const struct cred *cred);``
 
-     This gets a reference on a live set of credentials, returning a pointer to
-     that set of credentials.
+     :c:func:`get_cred` gets a reference on a live set of credentials,
+     returning a pointer to that set of credentials.
 
  - ``struct cred *get_new_cred(struct cred *cred);``
 
-     This gets a reference on a set of credentials that is under construction
-     and is thus still mutable, returning a pointer to that set of credentials.
+     :c:func:`get_new_cred` gets a reference on a set of credentials
+     that is under construction and is thus still mutable, returning a
+     pointer to that set of credentials. 
 
 
 Open File Credentials
@@ -546,9 +538,20 @@ Overriding the VFS's Use of Credentials
 =======================================
 
 Under some circumstances it is desirable to override the credentials used by
-the VFS, and that can be done by calling into such as ``vfs_mkdir()`` with a
+the VFS, and that can be done by calling into such as :c:func:`vfs_mkdir` with a
 different set of credentials.  This is done in the following places:
 
  * ``sys_faccessat()``.
  * ``do_coredump()``.
  * nfs4recover.c.
+
+List of functions for managing credentials
+==========================================
+
+.. kernel-doc:: include/linux/cred.h
+
+.. kernel-doc:: kernel/cred.c
+   :export:
+
+.. kernel-doc:: kernel/groups.c
+   :export:
diff --git a/kernel/groups.c b/kernel/groups.c
index daae2f2dc6d4..dddbb52f9035 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -86,6 +86,19 @@ static int gid_cmp(const void *_a, const void *_b)
 	return gid_gt(a, b) - gid_lt(a, b);
 }
 
+/**
+ * groups_sort - sort supplementary group list numerically
+ * @group_info: The group list
+ *
+ * groups_search() uses a binary search to see if a
+ * given gid is a member of a group list, so the list must be
+ * sorted.  This can be achieved by calling groups_sort().
+ * As a &struct group_info is often shared, and as this function
+ * can temporary permute elements even of a sorted list,
+ * groups_sort() must be called *before* a @struct group_info
+ * is added to a credential, typically using set_groups() or
+ * set_current_groups().
+ */
 void groups_sort(struct group_info *group_info)
 {
 	sort(group_info->gid, group_info->ngroups, sizeof(*group_info->gid),
@@ -119,6 +132,10 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
  * set_groups - Change a group subscription in a set of credentials
  * @new: The newly prepared set of credentials to alter
  * @group_info: The group list to install
+ *
+ * The group list must already be sorted (by groups_sort())
+ * and the credential must not be in active use.  To change the
+ * groups list of an active credential, use set_current_groups().
  */
 void set_groups(struct cred *new, struct group_info *group_info)
 {
@@ -134,7 +151,7 @@ EXPORT_SYMBOL(set_groups);
  * @group_info: The group list to impose
  *
  * Validate a group subscription and, if valid, impose it upon current's task
- * security record.
+ * security record.  The group list must already be sorted.
  */
 int set_current_groups(struct group_info *group_info)
 {
-- 
2.14.0.rc0.dirty


Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)

Powered by blists - more mailing lists