linux-kernel - Re: [PATCH v4 2/2] staging: Add driver for XillyUSB (Xillybus variant for USB)

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:   Mon, 22 Mar 2021 12:13:51 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Eli Billauer <eli.billauer@...il.com>
Cc:     devel@...verdev.osuosl.org, dan.carpenter@...cle.com,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        arnd@...db.de
Subject: Re: [PATCH v4 2/2] staging: Add driver for XillyUSB (Xillybus
 variant for USB)

On Mon, Mar 22, 2021 at 01:01:54PM +0200, Eli Billauer wrote:
> Hello, Greg.
> 
> Thanks for your comments. I'd like to address a couple of them.
> 
> First, there's the lockless FIFO that is implemented in the driver:
> 
> On 21/03/21 14:23, Greg KH wrote:
> > 
> > > +
> > > +static unsigned int fifo_read(struct xillyfifo *fifo,
> > > +			      void *data, unsigned int len,
> > > +			      int (*copier)(void *, const void *, int))
> > > +{
> > > +	unsigned int done = 0;
> > > +	unsigned int todo = len;
> > > +	unsigned int fill;
> > > +	unsigned int readpos = fifo->readpos;
> > > +	unsigned int readbuf = fifo->readbuf;
> > > +
> > > +	fill = atomic_read(&fifo->fill);
> > And the number changed right after reading it :(
> > 
> > Again, no atomics, use a lock please.
> > 
> > This is a USB device, you are NOT doing high-speed data transfers at
> > all.
> > 
> The current XillyUSB hardware is USB 3.0 based, running at ~400 MB/s, and
> this is just the beginning. For comparison, when the PCIe-based Xillybus
> started at 200 MB/s, I didn't believe it would reach 6.6 GB/s.
> 
> So that's why I made the effort to implement a lockless FIFO, with all the
> extra synchronization fuss. And yes, it works perfectly, and has been
> heavily fuzz tested on an x86_64 machine. The memory barriers are carefully
> placed to make this work on less favorable platforms as well, but even if I
> got it wrong -- odds are that the fix will be a line or two.
> 
> Replacing atomics with spinlocks is a piece of cake, of course. But given
> this extra information, do you still think it's a good idea?

Trying to review this code is hard, if not impossible because of the
structure.  Again, USB interfaces are slow, a "custom lockless FIFO" is
something for the core kernel to implement, not for a random individual
driver, to ensure it is working properly.

And it seems like an overkill, are you sure those locks are a slowdown?

thanks,

greg k-h