Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 19 Oct 2021 15:53:08 +0900
From:   William Breathitt Gray <vilhelm.gray@gmail.com>
To:     David Lechner <david@lechnology.com>
Cc:     linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
        Greg KH <gregkh@linuxfoundation.org>
Subject: Re: [PATCH] counter: drop chrdev_lock
Message-ID: <YW5rVLrbrVVJ75SY@shinobu>
References: <20211017185521.3468640-1-david@lechnology.com>
 <YW0673OckeCY6Qs/@shinobu>
 <e8158cd7-fbde-5a9a-f4d9-a863745e3d58@lechnology.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="C+K/4FCKEYwZGzrL"
Content-Disposition: inline
In-Reply-To: <e8158cd7-fbde-5a9a-f4d9-a863745e3d58@lechnology.com>
Precedence: bulk


--C+K/4FCKEYwZGzrL
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Oct 18, 2021 at 11:03:49AM -0500, David Lechner wrote:
> On 10/18/21 4:14 AM, William Breathitt Gray wrote:
> > On Sun, Oct 17, 2021 at 01:55:21PM -0500, David Lechner wrote:
> >> diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter=
-sysfs.c
> >> index 1ccd771da25f..7bf8882ff54d 100644
> >> --- a/drivers/counter/counter-sysfs.c
> >> +++ b/drivers/counter/counter-sysfs.c
> >> @@ -796,25 +796,18 @@ static int counter_events_queue_size_write(struc=
t counter_device *counter,
> >>   					   u64 val)
> >>   {
> >>   	DECLARE_KFIFO_PTR(events, struct counter_event);
> >> -	int err =3D 0;
> >> -
> >> -	/* Ensure chrdev is not opened more than 1 at a time */
> >> -	if (!atomic_add_unless(&counter->chrdev_lock, 1, 1))
> >> -		return -EBUSY;
> >> +	int err;
> >>  =20
> >>   	/* Allocate new events queue */
> >>   	err =3D kfifo_alloc(&events, val, GFP_KERNEL);
> >>   	if (err)
> >> -		goto exit_early;
> >> +		return err;
> >>  =20
> >>   	/* Swap in new events queue */
> >>   	kfifo_free(&counter->events);
> >>   	counter->events.kfifo =3D events.kfifo;
> >=20
> > Do we need to hold the events_lock mutex here for this swap in case
> > counter_chrdev_read() is in the middle of reading the kfifo to
> > userspace, or do the kfifo macros already protect us from a race
> > condition here?
> >=20
> Another possibility might be to disallow changing the size while
> events are enabled. Otherwise, we also need to protect against
> write after free.
>=20
> I considered this:
>=20
> 	swap(counter->events.kfifo, events.kfifo);
> 	kfifo_free(&events);
>=20
> But I'm not sure that would be safe enough.

I think it depends on whether it's safe to call kfifo_free() while other
kfifo_*() calls are executing. I suspect it is not safe because I don't
think kfifo_free() waits until all kfifo read/write operations are
finished before freeing -- but if I'm wrong here please let me know.

Because of that, will need to hold the counter->events_lock afterall so
that we don't modify the events fifo while a kfifo read/write is going
on, lest we suffer an address fault. This can happen regardless of
whether you swap before or after the kfifo_free() because the old fifo
address could still be in use within those uncompleted kfifo_*() calls
if they were called before the swap but don't complete before the
kfifo_free().

So we have a problem now that I think you have already noticed: the
kfifo_in() call in counter_push_events() also needs protection, but it's
executing within an interrupt context so we can't try to lock a mutex
lest we end up sleeping.

One option we have is as you suggested: we disallow changing size while
events are enabled. However, that will require us to keep track of when
events are disabled and implement a spinlock to ensure that we don't
disable events in the middle of a kfifo_in().

Alternatively, we could change events_lock to a spinlock and use it to
protect all these operations on the counter->events fifo. Would this
alternative be a better option so that we avoid creating another
separate lock?

As a side note, you can also remove the atomic.h includes from these
files now as well because we won't be working with an atomic_t anymore.

William Breathitt Gray

--C+K/4FCKEYwZGzrL
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEk5I4PDJ2w1cDf/bghvpINdm7VJIFAmFua0kACgkQhvpINdm7
VJL86w//SlZer095FG9pRoX+CBucmT51djupq6qVoMVeyP5z2RQN55oo1VTD3//F
8+ZgnwbyS4UBDa2U9Py6hzkA30fjtQdi2avzHR6xGtyaWFeYIFehmjtCw5OrVu2G
AREfHSui1wlW5wcGSl7uXnSJkWo+ttPf1z77aWyZVgl/cDBgifUwddcdEbbpHasv
Thac8fLe5OaeW9Ic/I+/3mycOHfG1bc3q7SrLlB1+OOgxcr6QtruPHghVppXTWEm
NBJDND/lzJaglO12pmi6/bG2uYhSB8AI6iS87cIwcVRo7wknUE4qzByfBVA77Fck
sMBlX3eqKn0EAizAmkpcoG4XYMfCUNQ4Loe9OM6Gtp4izeMmIWwbR2o45NW0tHyt
YUE+armrB7tl+RLaRbmQzglIrgDRQT1lg8YBBI+OLW9w4YUKktyAszH0YbF1vgFR
TWK9sUQpIJVQ7bW2agh5TKPBZX67VYIAMF1LauyaOcYzG9ysiulwSZQQzp90Jg92
XdZOctkjIEP7PeFeTPeTLJcceiyZaw50Dq4OY5hrlvAKXEdk2vgdXtEzr1h8rfVs
GhJ1ndYV4hos7yjs+RVlOfATZLkXMXKvogZ2iyqeY7Aej0TWyK5s0PiuDLLzIMco
MNlxoqdEEaVUbJE6ZZF7zr8zXAIW3QbwhLonhAgJOOcRqIGQ5OA=
=gcuP
-----END PGP SIGNATURE-----

--C+K/4FCKEYwZGzrL--