1. 25 Jul, 2018 8 commits
    • Jason Gunthorpe's avatar
      IB/uverbs: Always propagate errors from rdma_alloc_commit_uobject() · 2c96eb7d
      Jason Gunthorpe authored
      The ioctl framework already does this correctly, but the write path did
      not. This is trivially fixed by simply using a standard pattern to return
      uobj_alloc_commit() as the last statement in every function.
      Signed-off-by: default avatarJason Gunthorpe <jgg@mellanox.com>
      2c96eb7d
    • Jason Gunthorpe's avatar
      IB/uverbs: Rework the locking for cleaning up the ucontext · e951747a
      Jason Gunthorpe authored
      The locking here has always been a bit crazy and spread out, upon some
      careful analysis we can simplify things.
      
      Create a single function uverbs_destroy_ufile_hw() that internally handles
      all locking. This pulls together pieces of this process that were
      sprinkled all over the places into one place, and covers them with one
      lock.
      
      This eliminates several duplicate/confusing locks and makes the control
      flow in ib_uverbs_close() and ib_uverbs_free_hw_resources() extremely
      simple.
      
      Unfortunately we have to keep an extra mutex, ucontext_lock.  This lock is
      logically part of the rwsem and provides the 'down write, fail if write
      locked, wait if read locked' semantic we require.
      Signed-off-by: default avatarJason Gunthorpe <jgg@mellanox.com>
      e951747a
    • Jason Gunthorpe's avatar
      IB/uverbs: Revise and clarify the rwsem and uobjects_lock · 87064277
      Jason Gunthorpe authored
      Rename 'cleanup_rwsem' to 'hw_destroy_rwsem' which is held across any call
      to the type destroy function (aka 'hw' destroy). The main purpose of this
      lock is to prevent normal add and destroy from running concurrently with
      uverbs_cleanup_ufile()
      
      Since the uobjects list is always manipulated under the 'hw_destroy_rwsem'
      we can eliminate the uobjects_lock in the cleanup function. This allows
      converting that lock to a very simple spinlock with a narrow critical
      section.
      Signed-off-by: default avatarJason Gunthorpe <jgg@mellanox.com>
      87064277
    • Jason Gunthorpe's avatar
      IB/uverbs: Clarify and revise uverbs_close_fd · e6d5d5dd
      Jason Gunthorpe authored
      The locking requirements here have changed slightly now that we can rely
      on the ib_uverbs_file always existing and containing all the necessary
      locking infrastructure.
      
      That means we can get rid of the cleanup_mutex usage (this was protecting
      the check on !uboj->context).
      
      Otherwise, follow the same pattern that IDR uses for destroy, acquire
      exclusive write access, then call destroy and the undo the 'lookup'.
      Signed-off-by: default avatarJason Gunthorpe <jgg@mellanox.com>
      e6d5d5dd
    • Jason Gunthorpe's avatar
      IB/uverbs: Revise the placement of get/puts on uobject · 5671f79b
      Jason Gunthorpe authored
      This wasn't wrong, but the placement of two krefs didn't make any
      sense. Follow some simple rules.
      
      - A kref is held inside uobjects_list
      - A kref is held inside the IDR
      - A kref is held inside file->private
      - A stack based kref is passed bettwen alloc_begin and
        alloc_abort/alloc_commit
      
      Any place we destroy one of the above pointers, we stick a put,
      or 'move' the kref into another pointer.
      
      The key functions have sensible semantics:
      - alloc_uobj fully initializes the common members in uobj, including
        the list
      - Get rid of the uverbs_idr_remove_uobj helper since IDR remove
        does require put, but it depends on the situation. Later
        patches will re-consolidate this differently.
      - alloc_abort always consumes the passed kref, done in the type
      - alloc_commit always consumes the passed kref, done in the type
      - rdma_remove_commit_uobject always pairs with a lookup_get
      
      After it is all done the only control flow change is to:
      - move a get from alloc_commit_fd_uobject to rdma_alloc_commit_uobject
      - add a put to remove_commit_idr_uobject
      - Consistenly use rdma_lookup_put in rdma_remove_commit_uobject at
        the right place
      Signed-off-by: default avatarJason Gunthorpe <jgg@mellanox.com>
      5671f79b
    • Jason Gunthorpe's avatar
      IB/uverbs: Clarify the kref'ing ordering for alloc_commit · c561c288
      Jason Gunthorpe authored
      The alloc_commit callback makes the uobj visible to other threads,
      and it does so using a 'move' semantic of the uobj kref on the stack
      into the public storage (eg the IDR, uobject list and file_private_data)
      
      Once this is done another thread could start up and trigger deletion
      of the kref. Fortunately cleanup_rwsem happens to prevent this from
      being a bug, but that is a fantastically unclear side effect.
      
      Re-organize things so that alloc_commit is that last thing to touch
      the uobj, get rid of the sneaky implicit dependency on cleanup_rwsem,
      and add a comment reminding that uobj is no longer kref'd after
      alloc_commit.
      Signed-off-by: default avatarJason Gunthorpe <jgg@mellanox.com>
      c561c288
    • Jason Gunthorpe's avatar
      IB/uverbs: Handle IDR and FD types without truncation · 1250c304
      Jason Gunthorpe authored
      Our ABI for write() uses a s32 for FDs and a u32 for IDRs, but internally
      we ended up implicitly casting these ABI values into an 'int'. For ioctl()
      we use a s64 for FDs and a u64 for IDRs, again casting to an int.
      
      The various casts to int are all missing range checks which can cause
      userspace values that should be considered invalid to be accepted.
      
      Fix this by making the generic lookup routine accept a s64, which does not
      truncate the write API's u32/s32 or the ioctl API's s64. Then push the
      detailed range checking down to the actual type implementations to be
      shared by both interfaces.
      
      Finally, change the copy of the uobj->id to sign extend into a s64, so eg,
      if we ever wish to return a negative value for a FD it is carried
      properly.
      
      This ensures that userspace values are never weirdly interpreted due to
      the various trunctations and everything that is really out of range gets
      an EINVAL.
      Signed-off-by: default avatarJason Gunthorpe <jgg@mellanox.com>
      1250c304
    • Jason Gunthorpe's avatar
      IB/uverbs: Get rid of null_obj_type · 3df593bf
      Jason Gunthorpe authored
      If the method fails after calling rdma_explicit_destroy (eg if
      copy_to_user faults) then it will trigger a kernel oops:
      
      BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
      PGD 800000000548d067 P4D 800000000548d067 PUD 54a0067 PMD 0
      SMP PTI
      CPU: 0 PID: 359 Comm: ibv_rc_pingpong Not tainted 4.18.0-rc1+ #28
      Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
      RIP: 0010:          (null)
      Code: Bad RIP value.
      RSP: 0018:ffffc900001a3bf0 EFLAGS: 00010246
      RAX: 0000000000000000 RBX: ffff88000603bd00 RCX: 0000000000000003
      RDX: 0000000000000001 RSI: 0000000000000001 RDI: ffff88000603bd00
      RBP: 0000000000000001 R08: ffffc900001a3cf8 R09: 0000000000000000
      R10: 0000000000000000 R11: 0000000000000000 R12: ffffc900001a3cf0
      R13: 0000000000000000 R14: ffffc900001a3cf0 R15: 0000000000000000
      FS:  00007fb00dda8700(0000) GS:ffff880007c00000(0000) knlGS:0000000000000000
      CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
      CR2: ffffffffffffffd6 CR3: 000000000548e004 CR4: 00000000003606b0
      DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
      DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
      Call Trace:
       ? rdma_lookup_put_uobject+0x22/0x50 [ib_uverbs]
       ? uverbs_finalize_object+0x3b/0x60 [ib_uverbs]
       ? uverbs_finalize_attrs+0x128/0x140 [ib_uverbs]
       ? ib_uverbs_cmd_verbs+0x698/0x7c0 [ib_uverbs]
       ? find_held_lock+0x2d/0x90
       ? __might_fault+0x39/0x90
       ? ib_uverbs_ioctl+0x111/0x1f0 [ib_uverbs]
       ? do_vfs_ioctl+0xa0/0x6d0
       ? trace_hardirqs_on_caller+0xed/0x180
       ? _raw_spin_unlock_irq+0x24/0x40
       ? syscall_trace_enter+0x138/0x1d0
       ? ksys_ioctl+0x35/0x60
       ? __x64_sys_ioctl+0x11/0x20
       ? do_syscall_64+0x5b/0x1c0
       ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
      
      This is because the type was replaced with the null_type during explicit
      destroy that cannot complete the destruction.
      
      One of the side effects of replacing the type is to make the object
      handle totally unreachable - so no other command could attempt to use
      it, even though it remains on the uboject list.
      
      We can get the same end result by just fully destroying the object inside
      rdma_explicit_destroy and leaving the caller the residual kref for the
      uobj with no attached HW object, and no presence in the ubojects list.
      Signed-off-by: default avatarJason Gunthorpe <jgg@mellanox.com>
      Reviewed-by: default avatarLeon Romanovsky <leonro@mellanox.com>
      3df593bf
  2. 24 Jul, 2018 27 commits
  3. 23 Jul, 2018 5 commits