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

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?

As for the specific remark on fifo->fill changing after reading it -- 
that's OK, since it would just mean that the reader of the FIFO doesn't 
see the extra data that has just been written to it. This doesn't break 
anything.

Moving on to the second topic:

>> +static loff_t xillyusb_llseek(struct file *filp, loff_t offset, int whence)
>>      
> USB devices do not have a "seek", why is this needed?
>
>    
Xillybus' device nodes are optionally seekable, which gives the user 
application access to a RAM array on the FPGA (or logic emulating it, 
such as registers). This holds true for the existing Xillybus API as 
well as the one for XillyUSB.

Thanks and regards,
    Eli

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ