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

[Jiri Slaby <jirislaby@kernel.org>]

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,
-- 
js
suse labs