linux-kernel - Re: [PATCH] chardev: call tty_init() in real chrdev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e4eb9000-e246-c01b-abde-de1535ff0374@linux.alibaba.com>
Date:   Mon, 7 Feb 2022 14:37:46 +0800
From:   Yu Xu <xuyu@...ux.alibaba.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        arnd@...db.de, viro@...iv.linux.org.uk, dhowells@...hat.com
Subject: Re: [PATCH] chardev: call tty_init() in real chrdev_init()

On 2/7/22 1:03 AM, Greg KH wrote:
> On Mon, Feb 07, 2022 at 12:27:31AM +0800, Xu Yu wrote:
>> It is confusing that tty_init() in called in the initialization of
>> memdev, i.e., static chr_dev_init().
>>
>> Through blame, it is introduced by commit 31d1d48e199e ("Fix init
>> ordering of /dev/console vs callers of modprobe"), which fixes the
>> initialization order of /dev/console driver. However, there seems
>> to be a typo in the patch, i.e., chrdev_init, instead of chr_dev_init.
>>
>> This fixes the typo, IIUC.
>>
>> Note that the return value of tty_init() is always 0, and thus no error
>> handling is provided in chrdev_init().
>>
>> Fixes: 31d1d48e199e ("Fix init ordering of /dev/console vs callers of modprobe")
>> Signed-off-by: Xu Yu <xuyu@...ux.alibaba.com>
>> ---
>>   drivers/char/mem.c | 2 +-
>>   fs/char_dev.c      | 1 +
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
>> index cc296f0823bd..8c90881f8115 100644
>> --- a/drivers/char/mem.c
>> +++ b/drivers/char/mem.c
>> @@ -775,7 +775,7 @@ static int __init chr_dev_init(void)
>>   			      NULL, devlist[minor].name);
>>   	}
>>   
>> -	return tty_init();
>> +	return 0;
>>   }
>>   
>>   fs_initcall(chr_dev_init);
>> diff --git a/fs/char_dev.c b/fs/char_dev.c
>> index ba0ded7842a7..fc042a0a098f 100644
>> --- a/fs/char_dev.c
>> +++ b/fs/char_dev.c
>> @@ -667,6 +667,7 @@ static struct kobject *base_probe(dev_t dev, int *part, void *data)
>>   void __init chrdev_init(void)
>>   {
>>   	cdev_map = kobj_map_init(base_probe, &chrdevs_lock);
>> +	tty_init();
>>   }
>>   
> 
> You just changed the ordering sequence here, are you SURE this is
> correct?

To be honest, not 100% sure.

> 
> How was this tested?  Did you verify that the problem that the original
> commit here was fixing is now not happening again?

I tried to reproduce the issue described in the original commit, and
failed. The issue does not appear, or my reproduction is wrong.
   1. revert 31d1d48e199e manually;
   2. request_module("xxx") anywhere before do_initcalls(), since
      tty_init() now is initialized by module_init();
   3. no warning on request_module is shown.

> 
> And what real problem is this solving?  How did you hit the issue that
> this solves?

No real problem actually. As described in the log, it is confusing that
tty_init() in called in the initialization of memdev. They don't have
strong dependencies. I found the issue when I read through codes of
drivers/char/mem.c.

> 
> And finally, yes, it is not good to throw away the return value of
> tty_init().  If it really can not return anything but 0, then let us
> make it a void function first.

Got it. But I will first try to figure out whether this patch is a real
issue.

> 
> thanks,
> 
> greg k-h

-- 
Thanks,
Yu