Re: [PATCH 02/10] audit: Fix possible spurious -ENOSPC error
On Fri 27-07-18 00:47:10, Paul Moore wrote:
On Tue, Jul 10, 2018 at 6:02 AM Jan Kara <jack(a)suse.cz> wrote:
> When an inode is tagged with a tree, tag_chunk() checks whether there is
> audit_tree_group mark attached to the inode and adds one if not. However
> nothing protects another tag_chunk() to add the mark between we've
> checked and try to add the fsnotify mark thus resulting in an error from
> fsnotify_add_mark() and consequently an ENOSPC error from tag_chunk().
>
> Fix the problem by holding mark_mutex over the whole check-insert code
> sequence.
>
> Signed-off-by: Jan Kara <jack(a)suse.cz>
> ---
> kernel/audit_tree.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
...
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 1c82eb6674c4..de8d344d91b1 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -342,25 +342,29 @@ static void untag_chunk(struct node *p)
> spin_lock(&hash_lock);
> }
>
> +/* Call with group->mark_mutex held, releases it */
Stuff like that always makes me nervous.
Yes, I also prefer to avoid stuff like this.
Could we defer releasing the mutex to the caller, after
create_chunk()
returns? It looks like fsnotify_destroy_mark() allows a single level of
nesting so it should be okay, yes?
This won't work. fsnotify_destroy_mark() would try to acquire the same
mutex and block indefinitely (the nesting depth is there just for lockdep
so that you can possibly nest mark_mutexes of two different group's). And
even if we do more work and use separate fsnotify_detach_mark() and
fsnotify_free_mark() calls instead of fsnotify_destroy_mark(), the problem
is still there as fsnotify_free_mark() must not be called under mark_mutex
(as it can acquire it in some cases).
So as much as I don't like functions that release locks they didn't take I
don't see how to avoid that here without creating even bigger mess.
Honza
--
Jan Kara <jack(a)suse.com>
SUSE Labs, CR