linux-kernel - Re: [PATCH v8 20/22] counter: Implement events_queue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 27 Feb 2021 09:20:48 +0900
From:   William Breathitt Gray <vilhelm.gray@...il.com>
To:     David Lechner <david@...hnology.com>
Cc:     Jonathan Cameron <jic23@...nel.org>, kernel@...gutronix.de,
        linux-stm32@...md-mailman.stormreply.com, a.fatoum@...gutronix.de,
        kamel.bouhara@...tlin.com, gwendal@...omium.org,
        alexandre.belloni@...tlin.com, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        syednwaris@...il.com, patrick.havelange@...ensium.com,
        fabrice.gasnier@...com, mcoquelin.stm32@...il.com,
        alexandre.torgue@...com, o.rempel@...gutronix.de
Subject: Re: [PATCH v8 20/22] counter: Implement events_queue_size sysfs
 attribute

On Fri, Feb 26, 2021 at 06:14:12PM -0600, David Lechner wrote:
> On 2/25/21 6:03 PM, William Breathitt Gray wrote:
> > On Sun, Feb 21, 2021 at 03:51:40PM +0000, Jonathan Cameron wrote:
> >> On Thu, 18 Feb 2021 19:32:16 +0900
> >> William Breathitt Gray <vilhelm.gray@...il.com> wrote:
> >>
> >>> On Sun, Feb 14, 2021 at 06:11:46PM +0000, Jonathan Cameron wrote:
> >>>> On Fri, 12 Feb 2021 21:13:44 +0900
> >>>> William Breathitt Gray <vilhelm.gray@...il.com> wrote:
> >>>>    
> >>>>> The events_queue_size sysfs attribute provides a way for users to
> >>>>> dynamically configure the Counter events queue size for the Counter
> >>>>> character device interface. The size is in number of struct
> >>>>> counter_event data structures. The number of elements will be rounded-up
> >>>>> to a power of 2 due to a requirement of the kfifo_alloc function called
> >>>>> during reallocation of the queue.
> >>>>>
> >>>>> Cc: Oleksij Rempel <o.rempel@...gutronix.de>
> >>>>> Signed-off-by: William Breathitt Gray <vilhelm.gray@...il.com>
> >>>>> ---
> >>>>>   Documentation/ABI/testing/sysfs-bus-counter |  8 +++++++
> >>>>>   drivers/counter/counter-chrdev.c            | 23 +++++++++++++++++++
> >>>>>   drivers/counter/counter-chrdev.h            |  2 ++
> >>>>>   drivers/counter/counter-sysfs.c             | 25 +++++++++++++++++++++
> >>>>>   4 files changed, 58 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> >>>>> index 847e96f19d19..f6cb2a8b08a7 100644
> >>>>> --- a/Documentation/ABI/testing/sysfs-bus-counter
> >>>>> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> >>>>> @@ -212,6 +212,14 @@ Description:
> >>>>>   		both edges:
> >>>>>   			Any state transition.
> >>>>>   
> >>>>> +What:		/sys/bus/counter/devices/counterX/events_queue_size
> >>>>> +KernelVersion:	5.13
> >>>>> +Contact:	linux-iio@...r.kernel.org
> >>>>> +Description:
> >>>>> +		Size of the Counter events queue in number of struct
> >>>>> +		counter_event data structures. The number of elements will be
> >>>>> +		rounded-up to a power of 2.
> >>>>> +
> >>>>>   What:		/sys/bus/counter/devices/counterX/name
> >>>>>   KernelVersion:	5.2
> >>>>>   Contact:	linux-iio@...r.kernel.org
> >>>>> diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
> >>>>> index 16f02df7f73d..53eea894e13f 100644
> >>>>> --- a/drivers/counter/counter-chrdev.c
> >>>>> +++ b/drivers/counter/counter-chrdev.c
> >>>>> @@ -375,6 +375,29 @@ void counter_chrdev_remove(struct counter_device *const counter)
> >>>>>   	cdev_del(&counter->chrdev);
> >>>>>   }
> >>>>>   
> >>>>> +int counter_chrdev_realloc_queue(struct counter_device *const counter,
> >>>>> +				 size_t queue_size)
> >>>>> +{
> >>>>> +	int err;
> >>>>> +	DECLARE_KFIFO_PTR(events, struct counter_event);
> >>>>> +	unsigned long flags;
> >>>>> +
> >>>>> +	/* Allocate new events queue */
> >>>>> +	err = kfifo_alloc(&events, queue_size, GFP_ATOMIC);
> >>>>
> >>>> Is there any potential for losing events?
> >>>
> >>> We take the events_list_lock down below so we're safe against missing an
> >>> event, but past events currently unread in the queue will be lost.
> >>>
> >>> Shortening the size of the queue is inherently a destructive process if
> >>> we have more events in the current queue than can fit in the new queue.
> >>> Because we a liable to lose some events in such a case, I think it's
> >>> best to keep the behavior of this reallocation consistent and have it
> >>> provide a fresh empty queue every time, as opposed to sometimes dropping
> >>> events and sometimes not.
> >>>
> >>> I also suspect an actual user would be setting the size of their queue
> >>> to the required amount before they begin watching events, rather than
> >>> adjusting it sporadically during a live operation.
> >>>
> >>
> >> Absolutely agree.   As such I wonder if you are better off enforcing this
> >> behaviour?  If the cdev is open for reading, don't allow the fifo to be
> >> resized.
> >>
> >> Jonathan
> > 
> > I can't really think of a good reason not to, so let's enforce it: if
> > the cdev is open, then we'll return an EINVAL if the user attempts to
> > resize the queue.
> > 
> > What is a good way to check for this condition? Should I just call
> > kref_read() and see if it's greater than 1? For example, in
> > counter_chrdev_realloc_queue():
> > 
> > 	if (kref_read(&counter->dev.kobj.kref) > 1)
> > 		return -EINVAL;
> > 
> > William Breathitt Gray
> > 
> 
> Wouldn't EBUSY make more sense?

Yes, EBUSY would be better here because the operation isn't necessarily
invalid, just unavailable because someone else has the cdev open.

William Breathitt Gray

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)