linux-kernel - Re: [PATCH v2 2/2] uacce: add uacce driver

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] [day] [month] [year] [list]

Date:   Mon, 2 Sep 2019 10:52:13 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     zhangfei <zhangfei.gao@...aro.org>
Cc:     Arnd Bergmann <arnd@...db.de>, jonathan.cameron@...wei.com,
        kenneth-lee-2012@...mail.com, Wangzhou <wangzhou1@...ilicon.com>,
        linux-accelerators@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        Kenneth Lee <liguozhu@...ilicon.com>,
        Zaibo Xu <xuzaibo@...wei.com>
Subject: Re: [PATCH v2 2/2] uacce: add uacce driver

On Mon, Sep 02, 2019 at 11:44:16AM +0800, zhangfei wrote:
> 
> Hi, Greg
> 
> On 2019/8/30 下午10:54, zhangfei wrote:
> > > > On 2019/8/28 下午11:22, Greg Kroah-Hartman wrote:
> > > > > On Wed, Aug 28, 2019 at 09:27:56PM +0800, Zhangfei Gao wrote:
> > > > > > +struct uacce {
> > > > > > +    const char *drv_name;
> > > > > > +    const char *algs;
> > > > > > +    const char *api_ver;
> > > > > > +    unsigned int flags;
> > > > > > +    unsigned long qf_pg_start[UACCE_QFRT_MAX];
> > > > > > +    struct uacce_ops *ops;
> > > > > > +    struct device *pdev;
> > > > > > +    bool is_vf;
> > > > > > +    u32 dev_id;
> > > > > > +    struct cdev cdev;
> > > > > > +    struct device dev;
> > > > > > +    void *priv;
> > > > > > +    atomic_t state;
> > > > > > +    int prot;
> > > > > > +    struct mutex q_lock;
> > > > > > +    struct list_head qs;
> > > > > > +};
> > > > > At a quick glance, this problem really stood out to me.  You CAN NOT
> > > > > have two different objects within a structure that have different
> > > > > lifetime rules and reference counts.  You do that here with both a
> > > > > 'struct cdev' and a 'struct device'.  Pick one or the other, but never
> > > > > both.
> > > > > 
> > > > > I would recommend using a 'struct device' and then a 'struct cdev *'.
> > > > > That way you get the advantage of using the driver model properly, and
> > > > > then just adding your char device node pointer to "the side" which
> > > > > interacts with this device.
> > > > > 
> > > > > Then you might want to call this "struct uacce_device" :)
> I think I understand now.
> 'struct device' and then a 'struct cdev' have different refcounts.
> Using 'struct cdev *', the release is not in uacce.c, but controlled by cdev
> itself.
> So uacce is decoupled with cdev.
> 
> //Using 'struct cdev *'
> cdev_alloc->cdev_dynamic_release:kfree(p);
> uacce_destroy_chrdev:
> cdev_device_del->cdev_del(cdev)->kobject_put(&p->kobj);
> if (refcount--) == 0
> cdev_dynamic_release->kfree(p);
> 
> //Using 'struct device'
> cdev_init->cdev_default_release
> cdev is freed in uacce.c,
> So 'struct device' and then a 'struct cdev' are bind together, while cdev
> and uacce->dev have different refcount.

Yes, that is exactly the reason, glad you figured it out.

thanks,

greg k-h