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 for Android: free password hash cracker in your pocket

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

Message-ID: <CA+sbYW2iajTVnHUVhdch+y-TjexcKpwFCCS0bx+iUc=OA3U3YA@mail.gmail.com>
Date:   Fri, 10 Feb 2017 17:11:00 +0530
From:   Selvin Xavier <selvin.xavier@...adcom.com>
To:     Doug Ledford <dledford@...hat.com>
Cc:     linux-rdma@...r.kernel.org,
        Linux Netdev List <netdev@...r.kernel.org>,
        Michael Chan <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 Tue, Feb 7, 2017 at 1:56 AM, Doug Ledford <dledford@...hat.com> 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.

Thanks for your commet.
I reviewed the usage of ref_count again and it is not really required
in this patch series.
It is being incremented and decremented from the netdev notifier and
will be always 1
in bnxt_re_dev_remove. bnxt_re_dev_remove is also invoked from netdev
notifier, so
no need to wait in this function.  So, I have removed ref_count
variable and cleaned up
this function in the v5 patch set.

Thanks,
Selvin