linux-kernel - Re: [PATCH v5 3/5] counter: Add character device interface

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] [day] [month] [year] [list]

Message-ID: <20201025175322.GA14219@shinobu>
Date:   Sun, 25 Oct 2020 13:53:22 -0400
From:   William Breathitt Gray <vilhelm.gray@...il.com>
To:     David Lechner <david@...hnology.com>
Cc:     jic23@...nel.org, kamel.bouhara@...tlin.com, gwendal@...omium.org,
        alexandre.belloni@...tlin.com, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, syednwaris@...il.com,
        patrick.havelange@...ensium.com, fabrice.gasnier@...com,
        mcoquelin.stm32@...il.com, alexandre.torgue@...com
Subject: Re: [PATCH v5 3/5] counter: Add character device interface

On Sun, Oct 25, 2020 at 11:34:43AM -0500, David Lechner wrote:
> On 10/25/20 8:18 AM, William Breathitt Gray wrote:
> > On Tue, Oct 20, 2020 at 11:06:42AM -0500, David Lechner wrote:
> >> On 10/18/20 11:58 AM, William Breathitt Gray wrote:
> >>> On Wed, Oct 14, 2020 at 05:40:44PM -0500, David Lechner wrote:
> >>>> On 9/26/20 9:18 PM, William Breathitt Gray wrote:
> >>>>> +static ssize_t counter_chrdev_read(struct file *filp, char __user *buf,
> >>>>> +				   size_t len, loff_t *f_ps)
> >>>>> +{
> >>>>> +	struct counter_device *const counter = filp->private_data;
> >>>>> +	int err;
> >>>>> +	unsigned long flags;
> >>>>> +	unsigned int copied;
> >>>>> +
> >>>>> +	if (len < sizeof(struct counter_event))
> >>>>> +		return -EINVAL;
> >>>>> +
> >>>>> +	do {
> >>>>> +		if (kfifo_is_empty(&counter->events)) {
> >>>>> +			if (filp->f_flags & O_NONBLOCK)
> >>>>> +				return -EAGAIN;
> >>>>> +
> >>>>> +			err = wait_event_interruptible(counter->events_wait,
> >>>>> +					!kfifo_is_empty(&counter->events));
> >>>>> +			if (err)
> >>>>> +				return err;
> >>>>> +		}
> >>>>> +
> >>>>> +		raw_spin_lock_irqsave(&counter->events_lock, flags);
> >>>>> +		err = kfifo_to_user(&counter->events, buf, len, &copied);
> >>>>> +		raw_spin_unlock_irqrestore(&counter->events_lock, flags);
> >>>>> +		if (err)
> >>>>> +			return err;
> >>>>> +	} while (!copied);
> >>>>> +
> >>>>> +	return copied;
> >>>>> +}
> >>>>
> >>>> All other uses of kfifo_to_user() I saw use a mutex instead of spin
> >>>> lock. I don't see a reason for disabling interrupts here.
> >>>
> >>> The Counter character device interface is special in this case because
> >>> counter->events could be accessed from an interrupt context. This is
> >>> possible if counter_push_event() is called for an interrupt (as is the
> >>> case for the 104_quad_8 driver). In this case, we can't use mutex
> >>> because we can't sleep in an interrupt context, so our only option is to
> >>> use spin lock.
> >>>
> >>
> >>
> >> The way I understand it, locking is only needed for concurrent readers
> >> and locking between reader and writer is not needed.
> > 
> > You're right, it does say in the kfifo.h comments that with only one
> > concurrent reader and one current write, we don't need extra locking to
> > use these macros. Because we only have one kfifo_to_user() operating on
> > counter->events, does that mean we don't need locking at all here for
> > the counter_chrdev_read() function?
> > 
> > William Breathitt Gray
> > 
> 
> Even if we have the policy that only one file handle to the chrdev
> can be open at a time, it is still possible that the it could be
> read from multiple threads. So it I think it makes sense to keep
> it just to be safe.

All right, I'll keep the locks in the code for now to keep it safe in
case we have multiple threads reading.

William Breathitt Gray

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