linux-kernel - Re: [PATCH v5 4/4] bus: mhi: clients: Add userspace client interface 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>] [day] [month] [year] [list]

Date:   Fri, 11 Sep 2020 09:47:52 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Hemant Kumar <hemantk@...eaurora.org>
Cc:     manivannan.sadhasivam@...aro.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, jhugo@...eaurora.org,
        bbhatt@...eaurora.org
Subject: Re: [PATCH v5 4/4] bus: mhi: clients: Add userspace client interface
 driver

On Fri, Sep 11, 2020 at 06:28:02AM +0000, Hemant Kumar wrote:
> > > +struct uci_dev {
> > > +	unsigned int minor;
> > > +	struct mhi_device *mhi_dev;
> > > +	const char *chan;
> > > +
> > > +	/* protects uci_dev struct members */
> > > +	struct mutex lock;
> > > +
> > > +	struct uci_chan ul_chan;
> > > +	struct uci_chan dl_chan;
> > > +	size_t mtu;
> > > +	size_t actual_mtu;
> > > +	struct kref ref_count;
> > > +	struct kref open_count;
> > 
> > I'm stopping right here.  A structure can only have ONE reference count
> > to control its lifespan.  You have 2 here, which guarantees that either
> > you are using a kref incorrectly, or your code is totally confused and
> > will break easily.
> > 
> > Please fix this as this is not how to do this.
> > 
> > Also, why does anyone need to care about the number of times that open()
> > is called?  The vfs layer should handle all of that for you, right?
> Reason for using open_count was to allow start MHI channel only when first
> open() was called and stop the MHI channel when last release() is called.
> Since uci driver just need to handle one open() from user space
> other calls to open can simply return -EBUSY. i will get rid of open_count
> and does not let multiple threads to open same file node.

You will fail in trying to attempt only one open on your device node,
sorry.  You can properly trigger off of the first/last things, but
having two different reference counts is NOT how to do this, those are
to control the lifetime of a structure/object.

greg k-h