Re: [PATCH for bnxt_re V4 03/21] RDMA/bnxt_re: register with the NIC driver

[Doug Ledford <dledford@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]

On Wed, 2016-12-21 at 03:41 -0800, Selvin Xavier wrote:
> +static void bnxt_re_dev_remove(struct bnxt_re_dev *rdev)
> +{
> +       int i = BNXT_RE_REF_WAIT_COUNT;
> +
> +       /* Wait for rdev refcount to come down */
> +       while ((atomic_read(&rdev->ref_count) > 1) && i--)
> +               msleep(100);
> +
> +       if (atomic_read(&rdev->ref_count) > 1)
> +               dev_err(rdev_to_dev(rdev),
> +                       "Failed waiting for ref count to deplete %d",
> +                       atomic_read(&rdev->ref_count));
> +
> +       atomic_set(&rdev->ref_count, 0);
> +       dev_put(rdev->netdev);
> +       rdev->netdev = NULL;
> +
> +       mutex_lock(&bnxt_re_dev_lock);
> +       list_del_rcu(&rdev->list);
> +       mutex_unlock(&bnxt_re_dev_lock);
> +
> +       synchronize_rcu();
> +       flush_workqueue(bnxt_re_wq);
> +
> +       ib_dealloc_device(&rdev->ibdev);
> +       /* rdev is gone */
> +}

This looks bad.  Either your ref counting is right, and your ref count
should go to 1, or you have an issue that won't be helped by forcibly
removing the device.  In its current form, this looks like an oopser
waiting to happen.

If you know your ref counting is right, then simply wait until it goes
to 1, don't have this bailout logic.  If you truly need to bailout for
some reason, then you need to not free things.  Better to shutdown then
leak and live than release and have a use after free data corrupter.


-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]