Re: [PATCH v17 2/9] counter: Add character device interface

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 17/10/2021 16:40:14+0200, Greg KH wrote:
> On Sun, Oct 17, 2021 at 04:02:42PM +0200, Alexandre Belloni wrote:
> > On 17/10/2021 15:50:11+0200, Greg KH wrote:
> > > Note, review of this now that it has been submitted in a pull request to
> > > me, sorry I missed this previously...
> > > 
> > > On Wed, Sep 29, 2021 at 12:15:59PM +0900, William Breathitt Gray wrote:
> > > > +static int counter_chrdev_open(struct inode *inode, struct file *filp)
> > > > +{
> > > > +	struct counter_device *const counter = container_of(inode->i_cdev,
> > > > +							    typeof(*counter),
> > > > +							    chrdev);
> > > > +
> > > > +	/* Ensure chrdev is not opened more than 1 at a time */
> > > > +	if (!atomic_add_unless(&counter->chrdev_lock, 1, 1))
> > > > +		return -EBUSY;
> > > 
> > > I understand the feeling that you wish to stop userspace from doing
> > > this, but really, it does not work.  Eventhough you are doing this
> > > correctly (you should see all the other attempts at doing this), you are
> > > not preventing userspace from having multiple processes access this
> > > device node at the same time, so please, don't even attempt to stop this
> > > from happening.
> > > 
> > > So you can drop the atomic "lock" you have here, it's not needed at all.
> > > 
> > 
> > Could you elaborate a bit here because we've had a similar thing in the
> > RTC subsystem:
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/rtc/dev.c#L28
> 
> Yeah, that too will not work :(  Note, it does stop open from being
> called from different processes, but think of the following sequence of
> userspace calls:
> 	open()
> 	fork/exec()
> 	both processes access the file descriptor
> 
> or passing a fd across a socket?
> 
> Or duplicating the file descriptor and sending it to a different task
> (like across a socket or many other IPC ways)?
> 
> Once userspace has a file descriptor, all bets are off as to where it
> goes and what it does with it.  There's no need to try to save userspace
> from itself by preventing multiple opens when really, it doesn't stop
> anyone who really wants to do this.
> 
> If userspace does do multiple read/writes from different threads /
> processes / whatever on the same file descriptor, it gets to keep the
> pieces of the mess it causes.  It's not the kernel's job to try to
> "protect" userspace from itself here.
> 
> Look at serial/tty connections as one example of this always being the
> case.
> 
> Does that help?
> 

Thanks for the explanation, this is now clear to me.

> > And it would mean I can remove rtc->flags completely.
> 
> I think you can do that.
> 
> thanks,
> 
> greg k-h

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com