Re: [PATCH v6 4/4] bus: mhi: Add userspace client interface driver

[Greg KH]

On Fri, Sep 18, 2020 at 11:14:08AM -0700, Hemant Kumar wrote:
> Hi Greg,
> 
> On 9/17/20 9:44 AM, Greg KH wrote:
> > On Wed, Sep 16, 2020 at 12:56:07PM -0700, Hemant Kumar wrote:
> ...
> ...
> > > +
> > > +static int mhi_uci_open(struct inode *inode, struct file *filp)
> > > +{
> > > +	struct uci_dev *udev = NULL;
> > > +	unsigned int minor = iminor(inode);
> > > +	int ret = -EIO;
> > > +	struct uci_buf *buf_itr, *tmp;
> > > +	struct uci_chan *dl_chan;
> > > +	struct mhi_device *mhi_dev;
> > > +	struct device *dev;
> > > +
> > > +	mutex_lock(&uci_idr_mutex);
> > > +	udev = idr_find(&uci_idr, minor);
> > > +	mutex_unlock(&uci_idr_mutex);
> > > +	if (!udev) {
> > > +		pr_err("uci dev: minor %d not found\n", minor);
> > 
> > Don't spam the kernel log for things that users can do :(
> i will change it to a pr_debug, as it helps to debug why open() is failing.
> > 
> > > +		ret = -ENODEV;
> > > +		goto error_no_dev;
> > > +	}
> > > +
> > > +	kref_get(&udev->ref_count);
> > 
> > Why grab a reference?  What does that help with?
> In case open() and driver remove() are racing, it helps to prevent use after
> free of udev in open().

Are you sure it prevents that?  Where is the lock that handles dropping
a reference count and incrementing it at the same time?

krefs are not "lock free" entirely, they need to have some type of other
control somewhere to prevent foolish things from happening :)

> > > +
> > > +	mhi_dev = udev->mhi_dev;
> > > +	dev = &mhi_dev->dev;
> > > +
> > > +	mutex_lock(&udev->lock);
> > > +	if (kref_read(&udev->ref_count) > 2) {
> > > +		dev_dbg(dev, "Node already opened\n");
> > 
> > Nope, this is NOT doing what you think it is doing.
> > 
> > I told you before, do not try to keep a device node from being opened
> > multiple times, as it will always fail (think about passing file handles
> > around between programs...)
> > 
> > If userspace wants to do this, it will do it.  If your driver can't
> > handle that, that's fine, userspace will learn not to do that.  But the
> > kernel can not prevent this from happening.
> This check is not returning error, instead just setting filp->private_data =
> udev; and return 0; It is skipping channel prepare
> and queuing of inbound buffers which was done by first open().

But don't do that by checking a kref value.  You should never care about
the value of it, that is not how you use it at all, and one reason I
hate that function is even present in the kernel...

thanks,

greg k-h