netdev - Re: [PATCH net-next v8 1/8] bnxt_en: Add auxiliary driver support

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]

Date:   Mon, 23 Jan 2023 23:07:07 -0800
From:   Ajit Khaparde <ajit.khaparde@...adcom.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Jakub Kicinski <kuba@...nel.org>, andrew.gospodarek@...adcom.com,
        davem@...emloft.net, edumazet@...gle.com, jgg@...pe.ca,
        leon@...nel.org, linux-kernel@...r.kernel.org,
        linux-rdma@...r.kernel.org, michael.chan@...adcom.com,
        netdev@...r.kernel.org, pabeni@...hat.com,
        selvin.xavier@...adcom.com, Leon Romanovsky <leonro@...dia.com>
Subject: Re: [PATCH net-next v8 1/8] bnxt_en: Add auxiliary driver support

On Mon, Jan 23, 2023 at 10:42 PM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> On Mon, Jan 23, 2023 at 10:33:05PM -0800, Jakub Kicinski wrote:
> > On Thu, 19 Jan 2023 22:05:28 -0800 Ajit Khaparde wrote:
> > > @@ -13212,6 +13214,7 @@ static void bnxt_remove_one(struct pci_dev *pdev)
> > >     kfree(bp->rss_indir_tbl);
> > >     bp->rss_indir_tbl = NULL;
> > >     bnxt_free_port_stats(bp);
> > > +   bnxt_aux_priv_free(bp);
> > >     free_netdev(dev);
> >
> > You're still freeing the memory in which struct device sits regardless
> > of its reference count.
> >
> > Greg, is it legal to call:
> >
> >       auxiliary_device_delete(adev);  // AKA device_del(&auxdev->dev);
> >       auxiliary_device_uninit(adev);  // AKA put_device(&auxdev->dev);
> >       free(adev);                     // frees struct device
>
> Ick, the aux device release callback should be doing the freeing of the
> memory, you shouldn't ever have to free it "manually" like this.  To do
> so would be a problem (i.e. the release callback would then free it
> again, right?)
Yikes!
Thanks for the refresher.

>
> > ? I tried to explain this three times, maybe there's some wait during
> > device_del() I'm not seeing which makes this safe :S
Apologies.
I thought since the driver was allocating the memory, it had to free it.
I stand corrected.

Thanks
Ajit

>
> Nope, no intentional wait normally.  You can add one by enabling a
> debugging option to find all of the places where people are doing bad
> things like this by delaying the freeing of the device memory until a
> few seconds later, but that's generally not something you should run in
> a production kernel as it finds all sorts of nasty bugs...
>
> thanks,
>
> greg k-h

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4218 bytes)