On 12/13/2017 10:29 AM, Felipe Balbi wrote:
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.
How is it different from disallowing empty name?
It may also cause some "broken" scripts stop working.
Isn't it going to introduce some weird problems like:
mkdir g1/function/ffs._
mkdir g2/function/ffs.
-EBUSY
Best regards,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
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
