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
