Re: [PATCH] counter: drop chrdev_lock

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Oct 19, 2021 at 03:53:08PM +0900, William Breathitt Gray wrote:
> 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(struct counter_device *counter,
> > >>   					   u64 val)
> > >>   {
> > >>   	DECLARE_KFIFO_PTR(events, struct counter_event);
> > >> -	int err = 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;
> > >>   
> > >>   	/* Allocate new events queue */
> > >>   	err = kfifo_alloc(&events, val, GFP_KERNEL);
> > >>   	if (err)
> > >> -		goto exit_early;
> > >> +		return err;
> > >>   
> > >>   	/* Swap in new events queue */
> > >>   	kfifo_free(&counter->events);
> > >>   	counter->events.kfifo = events.kfifo;
> > > 
> > > 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?
> > > 
> > Another possibility might be to disallow changing the size while
> > events are enabled. Otherwise, we also need to protect against
> > write after free.
> > 
> > I considered this:
> > 
> > 	swap(counter->events.kfifo, events.kfifo);
> > 	kfifo_free(&events);
> > 
> > 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?

I would recommend just having a single lock here if at all possible,
until you determine that there a performance problem that can be
measured that would require it to be split up.

thanks,

greg k-h