lkml.org 
[lkml]   [2018]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] kernel: audit_tree: Fix a sleep-in-atomic-context bug
    On Fri, Jun 22, 2018 at 5:23 AM Jan Kara <jack@suse.cz> wrote:
    > On Wed 20-06-18 21:29:12, Matthew Wilcox wrote:
    > > On Thu, Jun 21, 2018 at 11:32:45AM +0800, Jia-Ju Bai wrote:
    > > > The kernel may sleep with holding a spinlock.
    > > > The function call paths (from bottom to top) in Linux-4.16.7 are:
    > > >
    > > > [FUNC] kmem_cache_alloc(GFP_KERNEL)
    > > > fs/notify/mark.c, 439:
    > > > kmem_cache_alloc in fsnotify_attach_connector_to_object
    > > > fs/notify/mark.c, 520:
    > > > fsnotify_attach_connector_to_object in fsnotify_add_mark_list
    > > > fs/notify/mark.c, 590:
    > > > fsnotify_add_mark_list in fsnotify_add_mark_locked
    > > > kernel/audit_tree.c, 437:
    > > > fsnotify_add_mark_locked in tag_chunk
    > > > kernel/audit_tree.c, 423:
    > > > spin_lock in tag_chunk
    > >
    > > There are several locks here; your report would be improved by saying
    > > which one is the problem. I'm assuming it's old_entry->lock.
    > >
    > > spin_lock(&old_entry->lock);
    > > ...
    > > if (fsnotify_add_inode_mark_locked(chunk_entry,
    > > old_entry->connector->inode, 1)) {
    > > ...
    > > return fsnotify_add_mark_locked(mark, inode, NULL, allow_dups);
    > > ...
    > > ret = fsnotify_add_mark_list(mark, inode, mnt, allow_dups);
    > > ...
    > > if (inode)
    > > connp = &inode->i_fsnotify_marks;
    > > conn = fsnotify_grab_connector(connp);
    > > if (!conn) {
    > > err = fsnotify_attach_connector_to_object(connp, inode, mnt);
    > >
    > > It seems to me that this is safe because old_entry is looked up from
    > > fsnotify_find_mark, and it can't be removed while its lock is held.
    > > Therefore there's always a 'conn' returned from fsnotify_grab_connector(),
    > > and so this path will never be taken.
    > >
    > > But this code path is confusing to me, and I could be wrong. Jan, please
    > > confirm my analysis is correct?
    >
    > Yes, you are correct. The presence of another mark in the list (and the
    > fact we pin it there using refcount & mark_mutex) guarantees we won't need
    > to allocate the connector. I agree the audit code's use of fsnotify would
    > deserve some cleanup.

    I'm always open to suggestions and patches (hint, hint) from the
    fsnotify experts ;)

    --
    paul moore
    www.paul-moore.com

    \
     
     \ /
      Last update: 2018-06-22 20:57    [W:5.722 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site