netdev - Re: [PATCH for bnxt_re V4 03/21] RDMA/bnxt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1486412780.86943.5.camel@redhat.com>
Date:   Mon, 06 Feb 2017 15:26:20 -0500
From:   Doug Ledford <dledford@...hat.com>
To:     Selvin Xavier <selvin.xavier@...adcom.com>,
        linux-rdma@...r.kernel.org
Cc:     netdev@...r.kernel.org, michael.chan@...adcom.com,
        Eddie Wai <eddie.wai@...adcom.com>,
        Devesh Sharma <devesh.sharma@...adcom.com>,
        Somnath Kotur <somnath.kotur@...adcom.com>,
        Sriharsha Basavapatna <sriharsha.basavapatna@...adcom.com>
Subject: Re: [PATCH for bnxt_re V4 03/21] RDMA/bnxt_re: register with the
 NIC driver

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@...hat.com>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)