[v2] tty: Add comments for tty-ldisc module loading logic

  Current tty-ldisc module loading logic within tty_ldisc_get()
is prone to mislead beginner that the module is able to be loaded
by a user without capability CAP_SYS_MODULE, add comments to make
the logic easy to undertand.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Changes in v2:
- Remove condition checking changes

 drivers/tty/tty_ldisc.c | 4 ++++
 1 file changed, 4 insertions(+)
  

On Fri, Dec 15, 2023 at 09:41:30PM +0800, Zijun Hu wrote:
> Current tty-ldisc module loading logic within tty_ldisc_get()
> is prone to mislead beginner that the module is able to be loaded
> by a user without capability CAP_SYS_MODULE, add comments to make
> the logic easy to undertand.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
> Changes in v2:
> - Remove condition checking changes

As I asked for before, please get other, more experienced developers,
from your company, to review and sign-off on this patch before sending
it out again.  I can't take this as-is, sorry.

greg k-h
  

Hi,


On 12/15/2023 7:11 PM, Zijun Hu wrote:
> Current tty-ldisc module loading logic within tty_ldisc_get()
> is prone to mislead beginner that the module is able to be loaded
> by a user without capability CAP_SYS_MODULE, add comments to make
> the logic easy to undertand.
>
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
> Changes in v2:
> - Remove condition checking changes
>
>   drivers/tty/tty_ldisc.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
> index 3f68e213df1f..34526ffaccbc 100644
> --- a/drivers/tty/tty_ldisc.c
> +++ b/drivers/tty/tty_ldisc.c
> @@ -150,6 +150,10 @@ static struct tty_ldisc *tty_ldisc_get(struct tty_struct *tty, int disc)
>   	 */
>   	ldops = get_ldops(disc);
>   	if (IS_ERR(ldops)) {
> +		/*
> +		 * Always request tty-ldisc module regardless of user's
> +		 * CAP_SYS_MODULE if autoload is enabled.
> +		 */

Without much knowledge of this file...


What the if condition below accomplishes is evident, it probably doesn't 
require a comment.

A more useful comment would be why it does so?

Thank you...

-Vijay/


>   		if (!capable(CAP_SYS_MODULE) && !tty_ldisc_autoload)
>   			return ERR_PTR(-EPERM);
>   		request_module("tty-ldisc-%d", disc);
  

On 12/15/2023 10:12 PM, Greg KH wrote:
> On Fri, Dec 15, 2023 at 09:41:30PM +0800, Zijun Hu wrote:
>> Current tty-ldisc module loading logic within tty_ldisc_get()
>> is prone to mislead beginner that the module is able to be loaded
>> by a user without capability CAP_SYS_MODULE, add comments to make
>> the logic easy to undertand.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>> Changes in v2:
>> - Remove condition checking changes
> 
> As I asked for before, please get other, more experienced developers,
> from your company, to review and sign-off on this patch before sending
> it out again.  I can't take this as-is, sorry.
> 
okay
> greg k-h
  

On 15. 12. 23, 15:19, Vijaya Krishna Nivarthi wrote:
> Hi,
> 
> 
> On 12/15/2023 7:11 PM, Zijun Hu wrote:
>> Current tty-ldisc module loading logic within tty_ldisc_get()
>> is prone to mislead beginner that the module is able to be loaded
>> by a user without capability CAP_SYS_MODULE, add comments to make
>> the logic easy to undertand.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>> Changes in v2:
>> - Remove condition checking changes
>>
>>   drivers/tty/tty_ldisc.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
>> index 3f68e213df1f..34526ffaccbc 100644
>> --- a/drivers/tty/tty_ldisc.c
>> +++ b/drivers/tty/tty_ldisc.c
>> @@ -150,6 +150,10 @@ static struct tty_ldisc *tty_ldisc_get(struct 
>> tty_struct *tty, int disc)
>>        */
>>       ldops = get_ldops(disc);
>>       if (IS_ERR(ldops)) {
>> +        /*
>> +         * Always request tty-ldisc module regardless of user's
>> +         * CAP_SYS_MODULE if autoload is enabled.
>> +         */
> 
> Without much knowledge of this file...
> 
> 
> What the if condition below accomplishes is evident,

After a bit of thinking, sure.

> it probably doesn't require a comment.

I would not add a comment there at all. I would rewrite the code so it 
is obvious to everyone. Like:

static inline bool tty_ldisc_can_autoload(void)
{
   return capable(CAP_SYS_MODULE) || tty_ldisc_autoload;
}

And then:
if (!tty_ldisc_can_autoload())
   return ERR_PTR(-EPERM);

> A more useful comment would be why it does so?

 From an insider, the reason is obvious. But maybe not so much for 
newcomers. Well, one could document the new inline above. Like:
""
We allow loads for capable users or when autoloading is explicitly enabled.
""
or alike...

thanks,
  

On 12/15/2023 9:26 AM, Jiri Slaby wrote:
> On 15. 12. 23, 15:19, Vijaya Krishna Nivarthi wrote:
>> Hi,
>>
>>
>> On 12/15/2023 7:11 PM, Zijun Hu wrote:
>>> Current tty-ldisc module loading logic within tty_ldisc_get()
>>> is prone to mislead beginner that the module is able to be loaded
>>> by a user without capability CAP_SYS_MODULE, add comments to make
>>> the logic easy to undertand.
>>>
>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>> ---
>>> Changes in v2:
>>> - Remove condition checking changes
>>>
>>>   drivers/tty/tty_ldisc.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
>>> index 3f68e213df1f..34526ffaccbc 100644
>>> --- a/drivers/tty/tty_ldisc.c
>>> +++ b/drivers/tty/tty_ldisc.c
>>> @@ -150,6 +150,10 @@ static struct tty_ldisc *tty_ldisc_get(struct tty_struct *tty, int disc)
>>>        */
>>>       ldops = get_ldops(disc);
>>>       if (IS_ERR(ldops)) {
>>> +        /*
>>> +         * Always request tty-ldisc module regardless of user's
>>> +         * CAP_SYS_MODULE if autoload is enabled.
>>> +         */

The added comment confused me more :-)

"Request tty-ldisc if process has CAP_SYS_MODULE or autoload is enabled"

>>
>> Without much knowledge of this file...
>>
>>
>> What the if condition below accomplishes is evident,
> 
> After a bit of thinking, sure.
> 
>> it probably doesn't require a comment.
> 
> I would not add a comment there at all. I would rewrite the code so it is obvious to everyone. Like:
> 
> static inline bool tty_ldisc_can_autoload(void)
> {
>   return capable(CAP_SYS_MODULE) || tty_ldisc_autoload;
> }
> 
> And then:
> if (!tty_ldisc_can_autoload())
>   return ERR_PTR(-EPERM);
> 
>> A more useful comment would be why it does so?
> 
> From an insider, the reason is obvious. But maybe not so much for newcomers. Well, one could document the new inline above. Like:
> ""
> We allow loads for capable users or when autoloading is explicitly enabled.
> ""
> or alike...

I agree with Vijaya that it seems evident after a few moments of analysis, but we're
also maybe used to reading kernel code more. I don't think we should be opposed
to changes that make code easier to grok, even if they're trivial.

If we want to make it clearer, I like Jiri's suggestion. One other thing I'd add
is to give a reference to read config LDISC_AUTOLOAD's help text.

Zijun,

Please send future revisions of the patch to our internal pre-submit review list
before sending to kernel.org. Qualcommers can visit go/upstream.

- Elliot
  

On 12/16/2023 1:51 AM, Elliot Berman wrote:
> 
> 
> On 12/15/2023 9:26 AM, Jiri Slaby wrote:
>> On 15. 12. 23, 15:19, Vijaya Krishna Nivarthi wrote:
>>> Hi,
>>>
>>>
>>> On 12/15/2023 7:11 PM, Zijun Hu wrote:
>>>> Current tty-ldisc module loading logic within tty_ldisc_get()
>>>> is prone to mislead beginner that the module is able to be loaded
>>>> by a user without capability CAP_SYS_MODULE, add comments to make
>>>> the logic easy to undertand.
>>>>
>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>> ---
>>>> Changes in v2:
>>>> - Remove condition checking changes
>>>>
>>>>   drivers/tty/tty_ldisc.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
>>>> index 3f68e213df1f..34526ffaccbc 100644
>>>> --- a/drivers/tty/tty_ldisc.c
>>>> +++ b/drivers/tty/tty_ldisc.c
>>>> @@ -150,6 +150,10 @@ static struct tty_ldisc *tty_ldisc_get(struct tty_struct *tty, int disc)
>>>>        */
>>>>       ldops = get_ldops(disc);
>>>>       if (IS_ERR(ldops)) {
>>>> +        /*
>>>> +         * Always request tty-ldisc module regardless of user's
>>>> +         * CAP_SYS_MODULE if autoload is enabled.
>>>> +         */
> 
> The added comment confused me more :-)
> 
> "Request tty-ldisc if process has CAP_SYS_MODULE or autoload is enabled"
> 
got it, please ignore my comments and changes.
>>>
>>> Without much knowledge of this file...
>>>
>>>
>>> What the if condition below accomplishes is evident,
>>
>> After a bit of thinking, sure.
>>
>>> it probably doesn't require a comment.
>>
>> I would not add a comment there at all. I would rewrite the code so it is obvious to everyone. Like:
>>
>> static inline bool tty_ldisc_can_autoload(void)
>> {
>>   return capable(CAP_SYS_MODULE) || tty_ldisc_autoload;
>> }
>>
>> And then:
>> if (!tty_ldisc_can_autoload())
>>   return ERR_PTR(-EPERM);
>>
if you want to remain current logic, suggest think about below question:

for a user without module loading permission CAP_SYS_MODULE, kernel should not allow module to be loaded for the user,
even if kernel calls request_module() to load a module for the user, the loading operation will be refused by permission
checking triggered by request_module(). right?

i have no concern about current design if your answer is NO.

it maybe be worth double checking current logic introduced by below commit if your answer is YES
7c0cca7c847e "tty: ldisc: add sysctl to prevent autoloading of ldiscs"
i also don't understand why above commit will introduce extra capable(CAP_SYS_MODULE) checking.

>>> A more useful comment would be why it does so?
>>
>> From an insider, the reason is obvious. But maybe not so much for newcomers. Well, one could document the new inline above. Like:
>> ""
>> We allow loads for capable users or when autoloading is explicitly enabled.
>> ""
>> or alike...
> 
> I agree with Vijaya that it seems evident after a few moments of analysis, but we're
> also maybe used to reading kernel code more. I don't think we should be opposed
> to changes that make code easier to grok, even if they're trivial.
> 
> If we want to make it clearer, I like Jiri's suggestion. One other thing I'd add
> is to give a reference to read config LDISC_AUTOLOAD's help text.
> 
> Zijun,
> 
> Please send future revisions of the patch to our internal pre-submit review list
> before sending to kernel.org. Qualcommers can visit go/upstream.
> 
got it, will follow go/upstream for further patch upstream.
> - Elliot