Re: [PATCH] usb: gadget: configfs: Disallow empty function instance name

2017-12-13 Thread Krzysztof Opasiak 



On 12/13/2017 10:29 AM, Felipe Balbi wrote:


Hi,

Alan Stern  writes:

Krzysztof Opasiak  writes:

On 12/12/2017 01:31 PM, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  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 
Signed-off-by: Krzysztof Opasiak 
---
   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);
+   }


aand 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 Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: configfs: Disallow empty function instance name

2017-12-13 Thread Felipe Balbi 

Hi,

Alan Stern  writes:
>> Krzysztof Opasiak  writes:
>> > On 12/12/2017 01:31 PM, Felipe Balbi wrote:
>> >> 
>> >> Hi,
>> >> 
>> >> Krzysztof Opasiak  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 
>> >>> Signed-off-by: Krzysztof Opasiak 
>> >>> ---
>> >>>   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);
>> >>> +}
>> >> 
>> >> aand 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: configfs: Disallow empty function instance name

2017-12-12 Thread Alan Stern 
On Tue, 12 Dec 2017, Felipe Balbi wrote:

> Hi,
> 
> Krzysztof Opasiak  writes:
> > On 12/12/2017 01:31 PM, Felipe Balbi wrote:
> >> 
> >> Hi,
> >> 
> >> Krzysztof Opasiak  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 
> >>> Signed-off-by: Krzysztof Opasiak 
> >>> ---
> >>>   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);
> >>> + }
> >> 
> >> aand 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.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: configfs: Disallow empty function instance name

2017-12-12 Thread Krzysztof Opasiak 



On 12/12/2017 02:16 PM, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  writes:

On 12/12/2017 01:31 PM, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  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 
Signed-off-by: Krzysztof Opasiak 
---
   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);
+   }


aand 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 created PR for this[1]. If anyone here has any objections to this 
please let me now as soon as possible.


If there is no veto or other solution, I merge this in the end of week.

Footnotes:
1 - https://github.com/libusbgx/libusbgx/pull/20

Best regards,
--
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: configfs: Disallow empty function instance name

2017-12-12 Thread Felipe Balbi 

Hi,

Krzysztof Opasiak  writes:
> On 12/12/2017 01:31 PM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Krzysztof Opasiak  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 
>>> Signed-off-by: Krzysztof Opasiak 
>>> ---
>>>   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);
>>> +   }
>> 
>> aand 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


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: configfs: Disallow empty function instance name

2017-12-12 Thread Krzysztof Opasiak 



On 12/12/2017 01:31 PM, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  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 
Signed-off-by: Krzysztof Opasiak 
---
  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);
+   }


aand 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.


Footnotes:
1 - https://github.com/libusbgx/libusbgx/issues/19
--
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: configfs: Disallow empty function instance name

2017-12-12 Thread Felipe Balbi 

Hi,

Krzysztof Opasiak  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 
> Signed-off-by: Krzysztof Opasiak 
> ---
>  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);
> + }

aand 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.

-- 
balbi


signature.asc
Description: PGP signature