Hi,
Krzysztof Opasiak <k.opasiak@xxxxxxxxxxx> writes:
> On 12/12/2017 01:31 PM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Krzysztof Opasiak <k.opasiak@xxxxxxxxxxx> 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@xxxxxxxx>
>>> Signed-off-by: Krzysztof Opasiak <k.opasiak@xxxxxxxxxxx>
>>> ---
>>> 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?
--
balbi
Attachment:
signature.asc
Description: PGP signature
