[PATCH resend] VFS: simplify seq_file iteration code and interface
Just resending after 2 weeks silence, in case it fell through a crack.
Thanks,
NeilBrown
("git am -c" understands this line)
--8<-
The documentation for seq_file suggests that it is necessary to be
able to move the iterator to a given offset, however that is not the
case. If the iterator is stored in the private data and is stable
from one read() syscall to the next, it is only necessary to support
first/next interactions. Implementing this in a client is a little
clumsy.
- if ->start() is given a pos of zero, it should go to start of
sequence.
- if ->start() is given the name pos that was given to the most recent
next() or start(), it should restore the iterator to state just
before that last call
- if ->start is given another number, it should set the iterator one
beyond the start just before the last ->start or ->next call.
Also, the documentation says that the implementation can interpret the
pos however it likes (other than zero meaning start), but seq_file
increments the pos sometimes which does impose on the implementation.
This patch simplifies the interface for first/next iteration and
simplifies the code, while maintaining complete backward
compatability. Now:
- if ->start() is given a pos of zero, it should return an iterator
placed at the start of the sequence
- if ->start() is given a non-zero pos, it should return the iterator
in the same state it was after the last ->start or ->next.
This is particularly useful for interators which walk the multiple
chains in a hash table, e.g. using rhashtable_walk*. See
fs/gfs2/glock.c and drivers/staging/lustre/lustre/llite/vvp_dev.c
A large part of achieving this is to *always* call ->next after ->show
has successfully stored all of an entry in the buffer. Never just
increment the index instead.
Also:
- always pass >index to ->start() and ->next(), never a temp
variable
- don't clear ->from when ->count is zero, as ->from is dead when
->count is zero.
Some ->next functions do not increment *pos when they return NULL.
To maintain compatability with this, we still need to increment
m->index in one place, if ->next didn't increment it.
Note that such ->next functions are buggy and should be fixed.
A simple demonstration is
dd if=/proc/swaps bs=1000 skip=1
Choose any block size larger than the size of /proc/swaps.
This will always show the whole last line of /proc/swaps.
This patch doesn't work around buggy next() functions for this case.
Signed-off-by: NeilBrown <ne...@suse.com>
---
Documentation/filesystems/seq_file.txt | 63 ++
fs/seq_file.c | 53 +++-
2 files changed, 62 insertions(+), 54 deletions(-)
diff --git a/Documentation/filesystems/seq_file.txt
b/Documentation/filesystems/seq_file.txt
index 9de4303201e1..d412b236a9d6 100644
--- a/Documentation/filesystems/seq_file.txt
+++ b/Documentation/filesystems/seq_file.txt
@@ -66,23 +66,39 @@ kernel 3.10. Current versions require the following update
The iterator interface
-Modules implementing a virtual file with seq_file must implement a simple
-iterator object that allows stepping through the data of interest.
-Iterators must be able to move to a specific position - like the file they
-implement - but the interpretation of that position is up to the iterator
-itself. A seq_file implementation that is formatting firewall rules, for
-example, could interpret position N as the Nth rule in the chain.
-Positioning can thus be done in whatever way makes the most sense for the
-generator of the data, which need not be aware of how a position translates
-to an offset in the virtual file. The one obvious exception is that a
-position of zero should indicate the beginning of the file.
+Modules implementing a virtual file with seq_file must implement an
+iterator object that allows stepping through the data of interest
+during a "session" (roughly one read() system call). If the iterator
+is able to move to a specific position - like the file they implement,
+though with freedom to map the position number to a sequence location
+in whatever way is convenient - the iterator need only exist
+transiently during a session. If the iterator cannot easily find a
+numerical position but works well with a first/next interface, the
+iterator can be stored in the private data area and continue from one
+session to the next.
+
+A seq_file implementation that is formatting firewall rules from a
+table, for example, could provide a simple iterator that interprets
+position N as the Nth rule in the chain. A seq_file implementation
+that presents the content of a, potentially volatile, linked list
+might record a pointer into that list, providing that can be done
+without risk of the current location being removed.
+
+Positioning can thus be done in whatever way makes the m
[PATCH] VFS: simplify seq_file iteration code and interface
The documentation for seq_file suggests that it is necessary to be able to move the iterator to a given offset, however that is not the case. If the iterator is stored in the private data and is stable from one read() syscall to the next, it is only necessary to support first/next interactions. Implementing this in a client is a little clumsy. - if ->start() is given a pos of zero, it should go to start of sequence. - if ->start() is given the name pos that was given to the most recent next() or start(), it should restore the iterator to state just before that last call - if ->start is given another number, it should set the iterator one beyond the start just before the last ->start or ->next call. Also, the documentation says that the implementation can interpret the pos however it likes (other than zero meaning start), but seq_file increments the pos sometimes which does impose on the implementation. This patch simplifies the interface for first/next iteration and simplifies the code, while maintaining complete backward compatability. Now: - if ->start() is given a pos of zero, it should return an iterator placed at the start of the sequence - if ->start() is given a non-zero pos, it should return the iterator in the same state it was after the last ->start or ->next. This is particularly useful for interators which walk the multiple chains in a hash table, e.g. using rhashtable_walk*. See fs/gfs2/glock.c and drivers/staging/lustre/lustre/llite/vvp_dev.c A large part of achieving this is to *always* call ->next after ->show has successfully stored all of an entry in the buffer. Never just increment the index instead. Also: - always pass >index to ->start() and ->next(), never a temp variable - don't clear ->from when ->count is zero, as ->from is dead when ->count is zero. Some ->next functions do not increment *pos when they return NULL. To maintain compatability with this, we still need to increment m->index in one place, if ->next didn't increment it. Note that such ->next functions are buggy and should be fixed. A simple demonstration is dd if=/proc/swaps bs=1000 skip=1 Choose any block size larger than the size of /proc/swaps. This will always show the whole last line of /proc/swaps. This patch doesn't work around buggy next() functions for this case. Signed-off-by: NeilBrown <ne...@suse.com> --- Documentation/filesystems/seq_file.txt | 63 ++ fs/seq_file.c | 53 +++- 2 files changed, 62 insertions(+), 54 deletions(-) diff --git a/Documentation/filesystems/seq_file.txt b/Documentation/filesystems/seq_file.txt index 9de4303201e1..d412b236a9d6 100644 --- a/Documentation/filesystems/seq_file.txt +++ b/Documentation/filesystems/seq_file.txt @@ -66,23 +66,39 @@ kernel 3.10. Current versions require the following update The iterator interface -Modules implementing a virtual file with seq_file must implement a simple -iterator object that allows stepping through the data of interest. -Iterators must be able to move to a specific position - like the file they -implement - but the interpretation of that position is up to the iterator -itself. A seq_file implementation that is formatting firewall rules, for -example, could interpret position N as the Nth rule in the chain. -Positioning can thus be done in whatever way makes the most sense for the -generator of the data, which need not be aware of how a position translates -to an offset in the virtual file. The one obvious exception is that a -position of zero should indicate the beginning of the file. +Modules implementing a virtual file with seq_file must implement an +iterator object that allows stepping through the data of interest +during a "session" (roughly one read() system call). If the iterator +is able to move to a specific position - like the file they implement, +though with freedom to map the position number to a sequence location +in whatever way is convenient - the iterator need only exist +transiently during a session. If the iterator cannot easily find a +numerical position but works well with a first/next interface, the +iterator can be stored in the private data area and continue from one +session to the next. + +A seq_file implementation that is formatting firewall rules from a +table, for example, could provide a simple iterator that interprets +position N as the Nth rule in the chain. A seq_file implementation +that presents the content of a, potentially volatile, linked list +might record a pointer into that list, providing that can be done +without risk of the current location being removed. + +Positioning can thus be done in whatever way makes the most sense for +the generator of the data, which need not be aware of how a position +translates to an offset in the virtual file. The one obvious exception +is that a position
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 <ne...@suse.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 <ne...@suse.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 securit
[PATCH] Documentation: security/credentials.rst: explain need to sort group_list
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 <ne...@suse.com>
---
Documentation/security/credentials.rst | 7 +++
1 file changed, 7 insertions(+)
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::
--
2.14.0.rc0.dirty
signature.asc
Description: PGP signature
Re: [PATCH] ovl: fix reStructuredText syntax errors in documentation
On Thu, Dec 15 2016, Amir Goldstein wrote: > On Thu, Dec 8, 2016 at 9:49 AM, Amir Goldstein <amir7...@gmail.com> wrote: >> - Fix broken long line block quote >> - Fix missing newline before bullets list >> - Use correct numbered list syntax >> > > Ping. > > Neil, > I found these syntax errors when I posted your page on my wiki: > https://github.com/amir73il/overlayfs/wiki/Overlayfs-overview > > Not sure if this patch should go through Miklos's tree or John's tree, > but is surely needs the ACK from you. Does it? It is purely format changes which don't change the meaning in any way, so I felt it had little relevant to me. However I'm happy to provide an: Acked-by: NeilBrown <ne...@suse.com> if you find that useful. NeilBrown > > Thanks, > Amir. > >> Signed-off-by: Amir Goldstein <amir7...@gmail.com> >> --- >> Documentation/filesystems/overlayfs.txt | 7 --- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/filesystems/overlayfs.txt >> b/Documentation/filesystems/overlayfs.txt >> index fb6f307..634d03e 100644 >> --- a/Documentation/filesystems/overlayfs.txt >> +++ b/Documentation/filesystems/overlayfs.txt >> @@ -66,7 +66,7 @@ At mount time, the two directories given as mount options >> "lowerdir" and >> "upperdir" are combined into a merged directory: >> >>mount -t overlay overlay -olowerdir=/lower,upperdir=/upper,\ >> -workdir=/work /merged >> + workdir=/work /merged >> >> The "workdir" needs to be an empty directory on the same filesystem >> as upperdir. >> @@ -118,6 +118,7 @@ programs. >> >> seek offsets are assigned sequentially when the directories are read. >> Thus if >> + >>- read part of a directory >>- remember an offset, and close the directory >>- re-open the directory some time later >> @@ -137,12 +138,12 @@ When renaming a directory that is on the lower layer >> or merged (i.e. the >> directory was not created on the upper layer to start with) overlayfs can >> handle it in two different ways: >> >> -1) return EXDEV error: this error is returned by rename(2) when trying to >> +1. return EXDEV error: this error is returned by rename(2) when trying to >> move a file or directory across filesystem boundaries. Hence >> applications are usually prepared to hande this error (mv(1) for example >> recursively copies the directory tree). This is the default behavior. >> >> -2) If the "redirect_dir" feature is enabled, then the directory will be >> +2. If the "redirect_dir" feature is enabled, then the directory will be >> copied up (but not the contents). Then the "trusted.overlay.redirect" >> extended attribute is set to the path of the original location from the >> root of the overlay. Finally the directory is moved to the new >> -- >> 2.7.4 >> signature.asc Description: PGP signature
