usb: gadget: configfs: Disallow empty function instance name

[Felipe Balbi <balbi@kernel.org>]

Hi,

Krzysztof Opasiak <k.opasiak@samsung.com> writes:
> On 12/12/2017 01:31 PM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Krzysztof Opasiak <k.opasiak@samsung.com> writes:
>>> Every function should have a type and instance name.
>>> Unfortunately in most cases instance name was left unused and unchecked.
>>> This may lead to situations like FunctionFS device name identified by ""
>>> or some misleading debug messages from TCM like:
>>>
>>> tcm: Activating
>>>
>>> To avoid this let's add a check that instance name should have at least
>>> one character.
>>>
>>> Reported-by: Stefan Agner <stefan@agner.ch>
>>> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
>>> ---
>>>   drivers/usb/gadget/configfs.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>>> index aeb9f3c40521..bdc9ec597d6a 100644
>>> --- a/drivers/usb/gadget/configfs.c
>>> +++ b/drivers/usb/gadget/configfs.c
>>> @@ -548,6 +548,11 @@ static struct config_group *function_make(
>>>   	*instance_name = '\0';
>>>   	instance_name++;
>>>   
>>> +	if (*instance_name == '\0') {
>>> +		pr_err("Instance name (after .) should not be empty\n");
>>> +		return ERR_PTR(-EINVAL);
>>> +	}
>> 
>> aaaaaand just like that you break potentially existing scripts :-)
>> 
>> We need to find a better way of enforcing a name which doesn't break
>> existing users.
>
> I'm really open for suggestions how to enforce this without breaking 
> those scripts ;)
>
> The origin of this commit is github issue for libusbgx[1].
> So the problem is that library allows to create a function with empty 
> name (because I mistakenly assumed that kernel rejects this) but then it 
> is unable to reinitialize the ConfigFS state because there is a check 
> that disallows this. From my point of view I'd be happy to disallow 
> empty names because it causes some problems (f_fs) or weird debug 
> messages (f_tcm) so is generally inconvenient and seems to be 
> unintentional. But I would like to keep this consistent with kernel policy.

I think we need to first fix libusbgx to prevent empty names.

I don't want to be the one hearing from Linus that "we don't break
userspace". It's clear that empty names shouldn't be allowed, but they
_are_ allowed as of today, so how can we cause a regression all of a
sudden?

Alan, Greg, any suggestions?