27 Jul, 2016

2 commits

  • We are going to need to call shmem_charge() under tree_lock to get
    accoutning right on collapse of small tmpfs pages into a huge one.

    The problem is that tree_lock is irq-safe and lockdep is not happy, that
    we take irq-unsafe lock under irq-safe[1].

    Let's convert the lock to irq-safe.

    [1] https://gist.github.com/kiryl/80c0149e03ed35dfaf26628b8e03cdbc

    Link: http://lkml.kernel.org/r/1466021202-61880-34-git-send-email-kirill.shutemov@linux.intel.com
    Signed-off-by: Kirill A. Shutemov
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Kirill A. Shutemov
     
  • Provide a shmem_get_unmapped_area method in file_operations, called at
    mmap time to decide the mapping address. It could be conditional on
    CONFIG_TRANSPARENT_HUGEPAGE, but save #ifdefs in other places by making
    it unconditional.

    shmem_get_unmapped_area() first calls the usual mm->get_unmapped_area
    (which we treat as a black box, highly dependent on architecture and
    config and executable layout). Lots of conditions, and in most cases it
    just goes with the address that chose; but when our huge stars are
    rightly aligned, yet that did not provide a suitable address, go back to
    ask for a larger arena, within which to align the mapping suitably.

    There have to be some direct calls to shmem_get_unmapped_area(), not via
    the file_operations: because of the way shmem_zero_setup() is called to
    create a shmem object late in the mmap sequence, when MAP_SHARED is
    requested with MAP_ANONYMOUS or /dev/zero. Though this only matters
    when /proc/sys/vm/shmem_huge has been set.

    Link: http://lkml.kernel.org/r/1466021202-61880-29-git-send-email-kirill.shutemov@linux.intel.com
    Signed-off-by: Hugh Dickins
    Signed-off-by: Kirill A. Shutemov

    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Hugh Dickins
     

24 May, 2016

1 commit

  • shmat and shmdt rely on mmap_sem for write. If the waiting task gets
    killed by the oom killer it would block oom_reaper from asynchronous
    address space reclaim and reduce the chances of timely OOM resolving.
    Wait for the lock in the killable mode and return with EINTR if the task
    got killed while waiting.

    Signed-off-by: Michal Hocko
    Acked-by: Davidlohr Bueso
    Acked-by: Vlastimil Babka
    Cc: Hugh Dickins
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Michal Hocko
     

19 Feb, 2016

1 commit

  • remap_file_pages(2) emulation can reach file which represents removed
    IPC ID as long as a memory segment is mapped. It breaks expectations of
    IPC subsystem.

    Test case (rewritten to be more human readable, originally autogenerated
    by syzkaller[1]):

    #define _GNU_SOURCE
    #include
    #include
    #include
    #include

    #define PAGE_SIZE 4096

    int main()
    {
    int id;
    void *p;

    id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
    p = shmat(id, NULL, 0);
    shmctl(id, IPC_RMID, NULL);
    remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);

    return 0;
    }

    The patch changes shm_mmap() and code around shm_lock() to propagate
    locking error back to caller of shm_mmap().

    [1] http://github.com/google/syzkaller

    Signed-off-by: Kirill A. Shutemov
    Reported-by: Dmitry Vyukov
    Cc: Davidlohr Bueso
    Cc: Manfred Spraul
    Cc:
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Kirill A. Shutemov
     

21 Jan, 2016

1 commit

  • Make is_file_shm_hugepages() return bool to improve readability due to
    this particular function only using either one or zero as its return
    value.

    No functional change.

    Signed-off-by: Yaowei Bai
    Acked-by: Michal Hocko
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Yaowei Bai
     

01 Oct, 2015

1 commit

  • As reported by Dmitry Vyukov, we really shouldn't do ipc_addid() before
    having initialized the IPC object state. Yes, we initialize the IPC
    object in a locked state, but with all the lockless RCU lookup work,
    that IPC object lock no longer means that the state cannot be seen.

    We already did this for the IPC semaphore code (see commit e8577d1f0329:
    "ipc/sem.c: fully initialize sem_array before making it visible") but we
    clearly forgot about msg and shm.

    Reported-by: Dmitry Vyukov
    Cc: Manfred Spraul
    Cc: Davidlohr Bueso
    Cc: stable@vger.kernel.org
    Signed-off-by: Linus Torvalds

    Linus Torvalds
     

11 Sep, 2015

1 commit

  • Considering Linus' past rants about the (ab)use of BUG in the kernel, I
    took a look at how we deal with such calls in ipc. Given that any errors
    or corruption in ipc code are most likely contained within the set of
    processes participating in the broken mechanisms, there aren't really many
    strong fatal system failure scenarios that would require a BUG call.
    Also, if something is seriously wrong, ipc might not be the place for such
    a BUG either.

    1. For example, recently, a customer hit one of these BUG_ONs in shm
    after failing shm_lock(). A busted ID imho does not merit a BUG_ON,
    and WARN would have been better.

    2. MSG_COPY functionality of posix msgrcv(2) for checkpoint/restore.
    I don't see how we can hit this anyway -- at least it should be IS_ERR.
    The 'copy' arg from do_msgrcv is always set by calling prepare_copy()
    first and foremost. We could also probably drop this check altogether.
    Either way, it does not merit a BUG_ON.

    3. No ->fault() callback for the fs getting the corresponding page --
    seems selfish to make the system unusable.

    Signed-off-by: Davidlohr Bueso
    Cc: Manfred Spraul
    Cc: Linus Torvalds
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     

07 Aug, 2015

1 commit

  • The shm implementation internally uses shmem or hugetlbfs inodes for shm
    segments. As these inodes are never directly exposed to userspace and
    only accessed through the shm operations which are already hooked by
    security modules, mark the inodes with the S_PRIVATE flag so that inode
    security initialization and permission checking is skipped.

    This was motivated by the following lockdep warning:

    ======================================================
    [ INFO: possible circular locking dependency detected ]
    4.2.0-0.rc3.git0.1.fc24.x86_64+debug #1 Tainted: G W
    -------------------------------------------------------
    httpd/1597 is trying to acquire lock:
    (&ids->rwsem){+++++.}, at: shm_close+0x34/0x130
    but task is already holding lock:
    (&mm->mmap_sem){++++++}, at: SyS_shmdt+0x4b/0x180
    which lock already depends on the new lock.
    the existing dependency chain (in reverse order) is:
    -> #3 (&mm->mmap_sem){++++++}:
    lock_acquire+0xc7/0x270
    __might_fault+0x7a/0xa0
    filldir+0x9e/0x130
    xfs_dir2_block_getdents.isra.12+0x198/0x1c0 [xfs]
    xfs_readdir+0x1b4/0x330 [xfs]
    xfs_file_readdir+0x2b/0x30 [xfs]
    iterate_dir+0x97/0x130
    SyS_getdents+0x91/0x120
    entry_SYSCALL_64_fastpath+0x12/0x76
    -> #2 (&xfs_dir_ilock_class){++++.+}:
    lock_acquire+0xc7/0x270
    down_read_nested+0x57/0xa0
    xfs_ilock+0x167/0x350 [xfs]
    xfs_ilock_attr_map_shared+0x38/0x50 [xfs]
    xfs_attr_get+0xbd/0x190 [xfs]
    xfs_xattr_get+0x3d/0x70 [xfs]
    generic_getxattr+0x4f/0x70
    inode_doinit_with_dentry+0x162/0x670
    sb_finish_set_opts+0xd9/0x230
    selinux_set_mnt_opts+0x35c/0x660
    superblock_doinit+0x77/0xf0
    delayed_superblock_init+0x10/0x20
    iterate_supers+0xb3/0x110
    selinux_complete_init+0x2f/0x40
    security_load_policy+0x103/0x600
    sel_write_load+0xc1/0x750
    __vfs_write+0x37/0x100
    vfs_write+0xa9/0x1a0
    SyS_write+0x58/0xd0
    entry_SYSCALL_64_fastpath+0x12/0x76
    ...

    Signed-off-by: Stephen Smalley
    Reported-by: Morten Stevens
    Acked-by: Hugh Dickins
    Acked-by: Paul Moore
    Cc: Manfred Spraul
    Cc: Davidlohr Bueso
    Cc: Prarit Bhargava
    Cc: Eric Paris
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Stephen Smalley
     

01 Jul, 2015

2 commits

  • ... to ipc_obtain_object_idr, which is more meaningful and makes the code
    slightly easier to follow.

    Signed-off-by: Davidlohr Bueso
    Cc: Manfred Spraul
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     
  • Upon every shm_lock call, we BUG_ON if an error was returned, indicating
    racing either in idr or in shm_destroy. Move this logic into the locking.

    [akpm@linux-foundation.org: simplify code]
    Signed-off-by: Davidlohr Bueso
    Cc: Manfred Spraul
    Cc: Davidlohr Bueso
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     

27 Apr, 2015

1 commit

  • Pull fourth vfs update from Al Viro:
    "d_inode() annotations from David Howells (sat in for-next since before
    the beginning of merge window) + four assorted fixes"

    * 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs:
    RCU pathwalk breakage when running into a symlink overmounting something
    fix I_DIO_WAKEUP definition
    direct-io: only inc/dec inode->i_dio_count for file systems
    fs/9p: fix readdir()
    VFS: assorted d_backing_inode() annotations
    VFS: fs/inode.c helpers: d_inode() annotations
    VFS: fs/cachefiles: d_backing_inode() annotations
    VFS: fs library helpers: d_inode() annotations
    VFS: assorted weird filesystems: d_inode() annotations
    VFS: normal filesystems (and lustre): d_inode() annotations
    VFS: security/: d_inode() annotations
    VFS: security/: d_backing_inode() annotations
    VFS: net/: d_inode() annotations
    VFS: net/unix: d_backing_inode() annotations
    VFS: kernel/: d_inode() annotations
    VFS: audit: d_backing_inode() annotations
    VFS: Fix up some ->d_inode accesses in the chelsio driver
    VFS: Cachefiles should perform fs modifications on the top layer only
    VFS: AF_UNIX sockets should call mknod on the top layer only

    Linus Torvalds
     

16 Apr, 2015

2 commits


14 Dec, 2014

2 commits

  • Andrew Morton noted

    http://lkml.kernel.org/r/20141104142027.a7a0d010772d84560b445f59@linux-foundation.org

    that the shmdt uses inode->i_size outside of i_mutex being held.
    There is one more case in shm.c in shm_destroy(). This converts
    both users over to use i_size_read().

    Signed-off-by: Dave Hansen
    Cc: Manfred Spraul
    Cc: Davidlohr Bueso
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Dave Hansen
     
  • This is a highly-contrived scenario. But, a single shmdt() call can be
    induced in to unmapping memory from mulitple shm segments. Example code
    is here:

    http://www.sr71.net/~dave/intel/shmfun.c

    The fix is pretty simple: Record the 'struct file' for the first VMA we
    encounter and then stick to it. Decline to unmap anything not from the
    same file and thus the same segment.

    I found this by inspection and the odds of anyone hitting this in practice
    are pretty darn small.

    Lightly tested, but it's a pretty small patch.

    Signed-off-by: Dave Hansen
    Cc: Manfred Spraul
    Reviewed-by: Davidlohr Bueso
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Dave Hansen
     

14 Oct, 2014

1 commit

  • do_shmat() is the only user of ->start_stack (proc just reports its
    value), and this check looks ugly and wrong.

    The reason for this check is not clear at all, and it wrongly assumes that
    the stack can only grow down.

    But the main problem is that in general mm->start_stack has nothing to do
    with stack_vma->vm_start. Not only the application can switch to another
    stack and even unmap this area, setup_arg_pages() expands the stack
    without updating mm->start_stack during exec(). This means that in the
    likely case "addr > start_stack - size - PAGE_SIZE * 5" is simply
    impossible after find_vma_intersection() == F, or the stack can't grow
    anyway because of RLIMIT_STACK.

    Many thanks to Hugh for his explanations.

    Signed-off-by: Oleg Nesterov
    Acked-by: Hugh Dickins
    Cc: Cyrill Gorcunov
    Cc: Davidlohr Bueso
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Oleg Nesterov
     

09 Aug, 2014

2 commits

  • If shm_rmid_force (the default state) is not set then the shmids are only
    marked as orphaned and does not require any add, delete, or locking of the
    tree structure.

    Seperate the sysctl on and off case, and only obtain the read lock. The
    newly added list head can be deleted under the read lock because we are
    only called with current and will only change the semids allocated by this
    task and not manipulate the list.

    This commit assumes that up_read includes a sufficient memory barrier for
    the writes to be seen my others that later obtain a write lock.

    Signed-off-by: Milton Miller
    Signed-off-by: Jack Miller
    Cc: Davidlohr Bueso
    Cc: Manfred Spraul
    Cc: Anton Blanchard
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Jack Miller
     
  • This is small set of patches our team has had kicking around for a few
    versions internally that fixes tasks getting hung on shm_exit when there
    are many threads hammering it at once.

    Anton wrote a simple test to cause the issue:

    http://ozlabs.org/~anton/junkcode/bust_shm_exit.c

    Before applying this patchset, this test code will cause either hanging
    tracebacks or pthread out of memory errors.

    After this patchset, it will still produce output like:

    root@somehost:~# ./bust_shm_exit 1024 160
    ...
    INFO: rcu_sched detected stalls on CPUs/tasks: {} (detected by 116, t=2111 jiffies, g=241, c=240, q=7113)
    INFO: Stall ended before state dump start
    ...

    But the task will continue to run along happily, so we consider this an
    improvement over hanging, even if it's a bit noisy.

    This patch (of 3):

    exit_shm obtains the ipc_ns shm rwsem for write and holds it while it
    walks every shared memory segment in the namespace. Thus the amount of
    work is related to the number of shm segments in the namespace not the
    number of segments that might need to be cleaned.

    In addition, this occurs after the task has been notified the thread has
    exited, so the number of tasks waiting for the ns shm rwsem can grow
    without bound until memory is exausted.

    Add a list to the task struct of all shmids allocated by this task. Init
    the list head in copy_process. Use the ns->rwsem for locking. Add
    segments after id is added, remove before removing from id.

    On unshare of NEW_IPCNS orphan any ids as if the task had exited, similar
    to handling of semaphore undo.

    I chose a define for the init sequence since its a simple list init,
    otherwise it would require a function call to avoid include loops between
    the semaphore code and the task struct. Converting the list_del to
    list_del_init for the unshare cases would remove the exit followed by
    init, but I left it blow up if not inited.

    Signed-off-by: Milton Miller
    Signed-off-by: Jack Miller
    Cc: Davidlohr Bueso
    Cc: Manfred Spraul
    Cc: Anton Blanchard
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Jack Miller
     

07 Jun, 2014

6 commits

  • SHMMAX is the upper limit for the size of a shared memory segment, counted
    in bytes. The actual allocation is that size, rounded up to the next full
    page.

    Add a check that prevents the creation of segments where the rounded up
    size causes an integer overflow.

    Signed-off-by: Manfred Spraul
    Acked-by: Davidlohr Bueso
    Acked-by: KOSAKI Motohiro
    Acked-by: Michael Kerrisk
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Manfred Spraul
     
  • shm_tot counts the total number of pages used by shm segments.

    If SHMALL is ULONG_MAX (or nearly ULONG_MAX), then the number can
    overflow. Subsequent calls to shmctl(,SHM_INFO,) would return wrong
    values for shm_tot.

    The patch adds a detection for overflows.

    Signed-off-by: Manfred Spraul
    Acked-by: Davidlohr Bueso
    Acked-by: KOSAKI Motohiro
    Acked-by: Michael Kerrisk
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Manfred Spraul
     
  • The increase of SHMMAX/SHMALL is a 4 patch series.

    The change itself is trivial, the only problem are interger overflows.
    The overflows are not new, but if we make huge values the default, then
    the code should be free from overflows.

    SHMMAX:

    - shmmem_file_setup places a hard limit on the segment size:
    MAX_LFS_FILESIZE.

    On 32-bit, the limit is > 1 TB, i.e. 4 GB-1 byte segments are
    possible. Rounded up to full pages the actual allocated size
    is 0. --> must be fixed, patch 3

    - shmat:
    - find_vma_intersection does not handle overflows properly.
    --> must be fixed, patch 1

    - the rest is fine, do_mmap_pgoff limits mappings to TASK_SIZE
    and checks for overflows (i.e.: map 2 GB, starting from
    addr=2.5GB fails).

    SHMALL:
    - after creating 8192 segments size (1L< must be fixed, patch 2.

    Userspace:
    - Obviously, there could be overflows in userspace. There is nothing
    we can do, only use values smaller than ULONG_MAX.
    I ended with "ULONG_MAX - 1L<
    Acked-by: Davidlohr Bueso
    Acked-by: KOSAKI Motohiro
    Acked-by: Michael Kerrisk
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Manfred Spraul
     
  • trailing whitespace

    Signed-off-by: Paul McQuade
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Paul McQuade
     
  • Use #include instead of
    Use #include instead of

    Signed-off-by: Paul McQuade
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Paul McQuade
     
  • There is no need to recreate the very same ipc_ops structure on every
    kernel entry for msgget/semget/shmget. Just declare it static and be
    done with it. While at it, constify it as we don't modify the structure
    at runtime.

    Found in the PaX patch, written by the PaX Team.

    Signed-off-by: Mathias Krause
    Cc: PaX Team
    Cc: Davidlohr Bueso
    Cc: Manfred Spraul
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Mathias Krause
     

28 Jan, 2014

3 commits

  • IPC commenting style is all over the place, *specially* in util.c. This
    patch orders things a bit.

    Signed-off-by: Davidlohr Bueso
    Cc: Aswin Chandramouleeswaran
    Cc: Rik van Riel
    Acked-by: Manfred Spraul
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     
  • The ipc code does not adhere the typical linux coding style.
    This patch fixes lots of simple whitespace errors.

    - mostly autogenerated by
    scripts/checkpatch.pl -f --fix \
    --types=pointer_location,spacing,space_before_tab
    - one manual fixup (keep structure members tab-aligned)
    - removal of additional space_before_tab that were not found by --fix

    Tested with some of my msg and sem test apps.

    Andrew: Could you include it in -mm and move it towards Linus' tree?

    Signed-off-by: Manfred Spraul
    Suggested-by: Li Bin
    Cc: Joe Perches
    Acked-by: Rafael Aquini
    Cc: Davidlohr Bueso
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Manfred Spraul
     
  • After the locking semantics for the SysV IPC API got improved, a couple
    of IPC_RMID race windows were opened because we ended up dropping the
    'kern_ipc_perm.deleted' check performed way down in ipc_lock(). The
    spotted races got sorted out by re-introducing the old test within the
    racy critical sections.

    This patch introduces ipc_valid_object() to consolidate the way we cope
    with IPC_RMID races by using the same abstraction across the API
    implementation.

    Signed-off-by: Rafael Aquini
    Acked-by: Rik van Riel
    Acked-by: Greg Thelen
    Reviewed-by: Davidlohr Bueso
    Cc: Manfred Spraul
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Rafael Aquini
     

22 Nov, 2013

2 commits

  • Commit 2caacaa82a51 ("ipc,shm: shorten critical region for shmctl")
    restructured the ipc shm to shorten critical region, but introduced a
    path where the return value could be -EPERM, even if the operation
    actually was performed.

    Before the commit, the err return value was reset by the return value
    from security_shm_shmctl() after the if (!ns_capable(...)) statement.

    Now, we still exit the if statement with err set to -EPERM, and in the
    case of SHM_UNLOCK, it is not reset at all, and used as the return value
    from shmctl.

    To fix this, we only set err when errors occur, leaving the fallthrough
    case alone.

    Signed-off-by: Jesper Nilsson
    Cc: Davidlohr Bueso
    Cc: Rik van Riel
    Cc: Michel Lespinasse
    Cc: Al Viro
    Cc: [3.12.x]
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Jesper Nilsson
     
  • When IPC_RMID races with other shm operations there's potential for
    use-after-free of the shm object's associated file (shm_file).

    Here's the race before this patch:

    TASK 1 TASK 2
    ------ ------
    shm_rmid()
    ipc_lock_object()
    shmctl()
    shp = shm_obtain_object_check()

    shm_destroy()
    shum_unlock()
    fput(shp->shm_file)
    ipc_lock_object()
    shmem_lock(shp->shm_file)

    The oops is caused because shm_destroy() calls fput() after dropping the
    ipc_lock. fput() clears the file's f_inode, f_path.dentry, and
    f_path.mnt, which causes various NULL pointer references in task 2. I
    reliably see the oops in task 2 if with shmlock, shmu

    This patch fixes the races by:
    1) set shm_file=NULL in shm_destroy() while holding ipc_object_lock().
    2) modify at risk operations to check shm_file while holding
    ipc_object_lock().

    Example workloads, which each trigger oops...

    Workload 1:
    while true; do
    id=$(shmget 1 4096)
    shm_rmid $id &
    shmlock $id &
    wait
    done

    The oops stack shows accessing NULL f_inode due to racing fput:
    _raw_spin_lock
    shmem_lock
    SyS_shmctl

    Workload 2:
    while true; do
    id=$(shmget 1 4096)
    shmat $id 4096 &
    shm_rmid $id &
    wait
    done

    The oops stack is similar to workload 1 due to NULL f_inode:
    touch_atime
    shmem_mmap
    shm_mmap
    mmap_region
    do_mmap_pgoff
    do_shmat
    SyS_shmat

    Workload 3:
    while true; do
    id=$(shmget 1 4096)
    shmlock $id
    shm_rmid $id &
    shmunlock $id &
    wait
    done

    The oops stack shows second fput tripping on an NULL f_inode. The
    first fput() completed via from shm_destroy(), but a racing thread did
    a get_file() and queued this fput():
    locks_remove_flock
    __fput
    ____fput
    task_work_run
    do_notify_resume
    int_signal

    Fixes: c2c737a0461e ("ipc,shm: shorten critical region for shmat")
    Fixes: 2caacaa82a51 ("ipc,shm: shorten critical region for shmctl")
    Signed-off-by: Greg Thelen
    Cc: Davidlohr Bueso
    Cc: Rik van Riel
    Cc: Manfred Spraul
    Cc: # 3.10.17+ 3.11.6+
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Greg Thelen
     

25 Sep, 2013

1 commit

  • Currently, IPC mechanisms do security and auditing related checks under
    RCU. However, since security modules can free the security structure,
    for example, through selinux_[sem,msg_queue,shm]_free_security(), we can
    race if the structure is freed before other tasks are done with it,
    creating a use-after-free condition. Manfred illustrates this nicely,
    for instance with shared mem and selinux:

    -> do_shmat calls rcu_read_lock()
    -> do_shmat calls shm_object_check().
    Checks that the object is still valid - but doesn't acquire any locks.
    Then it returns.
    -> do_shmat calls security_shm_shmat (e.g. selinux_shm_shmat)
    -> selinux_shm_shmat calls ipc_has_perm()
    -> ipc_has_perm accesses ipc_perms->security

    shm_close()
    -> shm_close acquires rw_mutex & shm_lock
    -> shm_close calls shm_destroy
    -> shm_destroy calls security_shm_free (e.g. selinux_shm_free_security)
    -> selinux_shm_free_security calls ipc_free_security(&shp->shm_perm)
    -> ipc_free_security calls kfree(ipc_perms->security)

    This patch delays the freeing of the security structures after all RCU
    readers are done. Furthermore it aligns the security life cycle with
    that of the rest of IPC - freeing them based on the reference counter.
    For situations where we need not free security, the current behavior is
    kept. Linus states:

    "... the old behavior was suspect for another reason too: having the
    security blob go away from under a user sounds like it could cause
    various other problems anyway, so I think the old code was at least
    _prone_ to bugs even if it didn't have catastrophic behavior."

    I have tested this patch with IPC testcases from LTP on both my
    quad-core laptop and on a 64 core NUMA server. In both cases selinux is
    enabled, and tests pass for both voluntary and forced preemption models.
    While the mentioned races are theoretical (at least no one as reported
    them), I wanted to make sure that this new logic doesn't break anything
    we weren't aware of.

    Suggested-by: Linus Torvalds
    Signed-off-by: Davidlohr Bueso
    Acked-by: Manfred Spraul
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     

12 Sep, 2013

10 commits

  • This function was replaced by a the lockless shm_obtain_object_check(),
    and no longer has any users.

    Signed-off-by: Davidlohr Bueso
    Cc: Sedat Dilek
    Cc: Rik van Riel
    Cc: Manfred Spraul
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     
  • When !CONFIG_MMU there's a chance we can derefence a NULL pointer when the
    VM area isn't found - check the return value of find_vma().

    Also, remove the redundant -EINVAL return: retval is set to the proper
    return code and *only* changed to 0, when we actually unmap the segments.

    Signed-off-by: Davidlohr Bueso
    Cc: Sedat Dilek
    Cc: Rik van Riel
    Cc: Manfred Spraul
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     
  • Since in some situations the lock can be shared for readers, we shouldn't
    be calling it a mutex, rename it to rwsem.

    Signed-off-by: Davidlohr Bueso
    Tested-by: Sedat Dilek
    Cc: Rik van Riel
    Cc: Manfred Spraul
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     
  • Similar to other system calls, acquire the kern_ipc_perm lock after doing
    the initial permission and security checks.

    [sasha.levin@oracle.com: dont leave do_shmat with rcu lock held]
    Signed-off-by: Davidlohr Bueso
    Tested-by: Sedat Dilek
    Cc: Rik van Riel
    Cc: Manfred Spraul
    Signed-off-by: Sasha Levin
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     
  • Clean up some of the messy do_shmat() spaghetti code, getting rid of
    out_free and out_put_dentry labels. This makes shortening the critical
    region of this function in the next patch a little easier to do and read.

    Signed-off-by: Davidlohr Bueso
    Tested-by: Sedat Dilek
    Cc: Rik van Riel
    Cc: Manfred Spraul
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     
  • With the *_INFO, *_STAT, IPC_RMID and IPC_SET commands already optimized,
    deal with the remaining SHM_LOCK and SHM_UNLOCK commands. Take the
    shm_perm lock after doing the initial auditing and security checks. The
    rest of the logic remains unchanged.

    Signed-off-by: Davidlohr Bueso
    Tested-by: Sedat Dilek
    Cc: Rik van Riel
    Cc: Manfred Spraul
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     
  • While the INFO cmd doesn't take the ipc lock, the STAT commands do acquire
    it unnecessarily. We can do the permissions and security checks only
    holding the rcu lock.

    [akpm@linux-foundation.org: coding-style fixes]
    Signed-off-by: Davidlohr Bueso
    Tested-by: Sedat Dilek
    Cc: Rik van Riel
    Cc: Manfred Spraul
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     
  • Similar to semctl and msgctl, when calling msgctl, the *_INFO and *_STAT
    commands can be performed without acquiring the ipc object.

    Add a shmctl_nolock() function and move the logic of *_INFO and *_STAT out
    of msgctl(). Since we are just moving functionality, this change still
    takes the lock and it will be properly lockless in the next patch.

    Signed-off-by: Davidlohr Bueso
    Tested-by: Sedat Dilek
    Cc: Rik van Riel
    Cc: Manfred Spraul
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     
  • Instead of holding the ipc lock for the entire function, use the
    ipcctl_pre_down_nolock and only acquire the lock for specific commands:
    RMID and SET.

    Signed-off-by: Davidlohr Bueso
    Tested-by: Sedat Dilek
    Cc: Rik van Riel
    Cc: Manfred Spraul
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso
     
  • This is the third and final patchset that deals with reducing the amount
    of contention we impose on the ipc lock (kern_ipc_perm.lock). These
    changes mostly deal with shared memory, previous work has already been
    done for semaphores and message queues:

    http://lkml.org/lkml/2013/3/20/546 (sems)
    http://lkml.org/lkml/2013/5/15/584 (mqueues)

    With these patches applied, a custom shm microbenchmark stressing shmctl
    doing IPC_STAT with 4 threads a million times, reduces the execution
    time by 50%. A similar run, this time with IPC_SET, reduces the
    execution time from 3 mins and 35 secs to 27 seconds.

    Patches 1-8: replaces blindly taking the ipc lock for a smarter
    combination of rcu and ipc_obtain_object, only acquiring the spinlock
    when updating.

    Patch 9: renames the ids rw_mutex to rwsem, which is what it already was.

    Patch 10: is a trivial mqueue leftover cleanup

    Patch 11: adds a brief lock scheme description, requested by Andrew.

    This patch:

    Add shm_obtain_object() and shm_obtain_object_check(), which will allow us
    to get the ipc object without acquiring the lock. Just as with other
    forms of ipc, these functions are basically wrappers around
    ipc_obtain_object*().

    Signed-off-by: Davidlohr Bueso
    Tested-by: Sedat Dilek
    Cc: Rik van Riel
    Cc: Manfred Spraul
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Davidlohr Bueso