Hi,
Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
>> 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?
>
> You could do some silly name munging, like changing an empty name to
> " " whenever you encounter it. Or adding an '_' to the end of any name
> that consists of nothing but '_' characters.
Hmm, that could be done. So everytime userspace gives us an empty name,
we would convert to '_'. That still doesn't solve the problems of
mounting functionfs, though. But I guess there's nothing that can be
done in that case.
--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
