On Sun, Oct 17, 2021 at 04:02:42PM +0200, Alexandre Belloni wrote:

On 17/10/2021 15:50:11+0200, Greg KH wrote:

quoted

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:

quoted

+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?

And it would mean I can remove rtc->flags completely.

I think you can do that.

thanks,

greg k-h