linux-kernel - Re: [PATCH] usb: gadget: ffs: don't allow to open with O

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:	Fri, 03 Apr 2015 08:14:23 +0200
From:	Robert Baldyga <r.baldyga@...sung.com>
To:	Michal Nazarewicz <mina86@...a86.com>, balbi@...com
Cc:	gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: gadget: ffs: don't allow to open with O_NONBLOCK flag

Hi Michal,

On 04/01/2015 05:17 PM, Michal Nazarewicz wrote:
> On Wed, Apr 01 2015, Robert Baldyga <r.baldyga@...sung.com> wrote:
>> FunctionFS can't support O_NONBLOCK because read/write operatons are
>> directly translated into USB requests which are asynchoronous, so we
>> can't know how long we will have to wait for request completion. For
>> this reason in case of open with O_NONBLOCK flag we return
>> -EWOULDBLOCK.
> 
> ‘can’t’ is a bit strong of a word here though.  It can, but in a few
> cases it doesn’t.
> 
> It kinda saddens me that this undoes all the lines of code that were put
> into the file to support O_NONBLOCK (e.g. FFS_NO_SETUP path of
> ffs_ep0_read).
> 
> I’m also worried this may break existing applications which, for better
> or worse, open the file with O_NONBLOCK.
> 
> Most importantly though, this does not stop users from using fcntl to
> set O_NONBLOCK, so if you really want to stop O_NONBLOCK from being set,
> that path should be checked as well (if possible).

I want rather to inform users that non-blocking i/o wouldn't work for
epfiles. Indeed we can handle O_NONBLOCK for ep0 (for the same reason we
can have poll), but for other epfiles there is no way to check if
read/write operation can end up in short time. Everything is up to host.

When we can open file with O_NONBLOCK we expect that non-blocking API
will work, which isn't true in our case. Then it causes problems like
this one: https://lkml.org/lkml/2015/3/31/688

However it looks like you're right that my patch can cause ABI break, so
it shouldn't be applied.

Do you have any idea what can we do about that?

Best regards,
Robert Baldyga

> 
>> Signed-off-by: Robert Baldyga <r.baldyga@...sung.com>
>> ---
>>  drivers/usb/gadget/function/f_fs.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index 175c995..1014911 100644
>> --- a/drivers/usb/gadget/function/f_fs.c
>> +++ b/drivers/usb/gadget/function/f_fs.c
>> @@ -538,6 +538,14 @@ static int ffs_ep0_open(struct inode *inode, struct file *file)
>>  	if (unlikely(ffs->state == FFS_CLOSING))
>>  		return -EBUSY;
>>  
>> +	/*
>> +	 * We are not supporting O_NONBLOCK because read/write operatons are
>> +	 * directly translated into USB requests which are asynchoronous, so
>> +	 * we can't know how long we will have to wait for request completion.
>> +	 */
>> +	if (unlikely(file->f_flags & O_NONBLOCK))
>> +		return -EWOULDBLOCK;
>> +
>>  	file->private_data = ffs;
>>  	ffs_data_opened(ffs);
>>  
>> @@ -874,6 +882,14 @@ ffs_epfile_open(struct inode *inode, struct file *file)
>>  	if (WARN_ON(epfile->ffs->state != FFS_ACTIVE))
>>  		return -ENODEV;
>>  
>> +	/*
>> +	 * We are not supporting O_NONBLOCK because read/write operatons are
>> +	 * directly translated into USB requests which are asynchoronous, so
>> +	 * we can't know how long we will have to wait for request completion.
>> +	 */
>> +	if (unlikely(file->f_flags & O_NONBLOCK))
>> +		return -EWOULDBLOCK;
>> +
>>  	file->private_data = epfile;
>>  	ffs_data_opened(epfile->ffs);
>>  
>> -- 
>> 1.9.1
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/