All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg KH <gregkh@linuxfoundation.org>
To: William Breathitt Gray <vilhelm.gray@gmail.com>
Cc: jic23@kernel.org, linux-stm32@st-md-mailman.stormreply.com,
	kernel@pengutronix.de, a.fatoum@pengutronix.de,
	kamel.bouhara@bootlin.com, gwendal@chromium.org,
	alexandre.belloni@bootlin.com, david@lechnology.com,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, syednwaris@gmail.com,
	patrick.havelange@essensium.com, fabrice.gasnier@st.com,
	mcoquelin.stm32@gmail.com, alexandre.torgue@st.com,
	o.rempel@pengutronix.de, jarkko.nikula@linux.intel.com,
	Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: [PATCH v17 2/9] counter: Add character device interface
Date: Sun, 17 Oct 2021 15:50:11 +0200	[thread overview]
Message-ID: <YWwqE5T6h5j14M/M@kroah.com> (raw)
In-Reply-To: <b8b8c64b4065aedff43699ad1f0e2f8d1419c15b.1632884256.git.vilhelm.gray@gmail.com>

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.

thanks,

greg k-h

WARNING: multiple messages have this Message-ID (diff)
From: Greg KH <gregkh@linuxfoundation.org>
To: William Breathitt Gray <vilhelm.gray@gmail.com>
Cc: jic23@kernel.org, linux-stm32@st-md-mailman.stormreply.com,
	kernel@pengutronix.de, a.fatoum@pengutronix.de,
	kamel.bouhara@bootlin.com, gwendal@chromium.org,
	alexandre.belloni@bootlin.com, david@lechnology.com,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, syednwaris@gmail.com,
	patrick.havelange@essensium.com, fabrice.gasnier@st.com,
	mcoquelin.stm32@gmail.com, alexandre.torgue@st.com,
	o.rempel@pengutronix.de, jarkko.nikula@linux.intel.com,
	Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: [PATCH v17 2/9] counter: Add character device interface
Date: Sun, 17 Oct 2021 15:50:11 +0200	[thread overview]
Message-ID: <YWwqE5T6h5j14M/M@kroah.com> (raw)
In-Reply-To: <b8b8c64b4065aedff43699ad1f0e2f8d1419c15b.1632884256.git.vilhelm.gray@gmail.com>

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.

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-10-17 13:50 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-29  3:15 [PATCH v17 0/9] Introduce the Counter character device interface William Breathitt Gray
2021-09-29  3:15 ` William Breathitt Gray
2021-09-29  3:15 ` [PATCH v17 1/9] counter: Move counter enums to uapi header William Breathitt Gray
2021-09-29  3:15   ` William Breathitt Gray
2021-09-29  3:15 ` [PATCH v17 2/9] counter: Add character device interface William Breathitt Gray
2021-09-29  3:15   ` William Breathitt Gray
2021-10-17 13:50   ` Greg KH [this message]
2021-10-17 13:50     ` Greg KH
2021-10-17 14:02     ` Alexandre Belloni
2021-10-17 14:02       ` Alexandre Belloni
2021-10-17 14:40       ` Greg KH
2021-10-17 14:40         ` Greg KH
2021-10-17 15:35         ` Alexandre Belloni
2021-10-17 15:35           ` Alexandre Belloni
2021-09-29  3:16 ` [PATCH v17 3/9] docs: counter: Document " William Breathitt Gray
2021-09-29  3:16   ` William Breathitt Gray
2021-09-29  3:16 ` [PATCH v17 4/9] tools/counter: Create Counter tools William Breathitt Gray
2021-09-29  3:16   ` William Breathitt Gray
2021-09-29  3:16 ` [PATCH v17 5/9] counter: Implement signalZ_action_component_id sysfs attribute William Breathitt Gray
2021-09-29  3:16   ` William Breathitt Gray
2021-10-17 13:51   ` Greg KH
2021-10-17 13:51     ` Greg KH
2021-09-29  3:16 ` [PATCH v17 6/9] counter: Implement *_component_id sysfs attributes William Breathitt Gray
2021-09-29  3:16   ` William Breathitt Gray
2021-09-29  3:16 ` [PATCH v17 7/9] counter: Implement events_queue_size sysfs attribute William Breathitt Gray
2021-09-29  3:16   ` William Breathitt Gray
2021-09-29  3:16 ` [PATCH v17 8/9] counter: 104-quad-8: Replace mutex with spinlock William Breathitt Gray
2021-09-29  3:16   ` William Breathitt Gray
2021-09-29  3:16 ` [PATCH v17 9/9] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 William Breathitt Gray
2021-09-29  3:16   ` William Breathitt Gray
2021-09-30 17:21 ` [PATCH v17 0/9] Introduce the Counter character device interface Jonathan Cameron
2021-09-30 17:21   ` Jonathan Cameron

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YWwqE5T6h5j14M/M@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=a.fatoum@pengutronix.de \
    --cc=alexandre.belloni@bootlin.com \
    --cc=alexandre.torgue@st.com \
    --cc=dan.carpenter@oracle.com \
    --cc=david@lechnology.com \
    --cc=fabrice.gasnier@st.com \
    --cc=gwendal@chromium.org \
    --cc=jarkko.nikula@linux.intel.com \
    --cc=jic23@kernel.org \
    --cc=kamel.bouhara@bootlin.com \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-stm32@st-md-mailman.stormreply.com \
    --cc=mcoquelin.stm32@gmail.com \
    --cc=o.rempel@pengutronix.de \
    --cc=patrick.havelange@essensium.com \
    --cc=syednwaris@gmail.com \
    --cc=vilhelm.gray@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.