[PATCH v2 1/2] tty: add retry capability to tty_init_dev()

[PATCH v2 1/2] tty: add retry capability to tty_init_dev()

[Sudip Mukherjee]

Add a new function tty_init_dev_retry() which will have the capability
to return if 'tty->port' is not yet set and if we want to retry. There
is no change in behaviour if we do not want to retry and call
tty_init_dev().

Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
---

v1 was a combined patch of this and the next 2/2 patch. Made them
separately now.

 drivers/tty/tty_io.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e3076ea01222..95d7abeca254 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1291,9 +1291,10 @@ static int tty_reopen(struct tty_struct *tty)
 }
 
 /**
- *	tty_init_dev		-	initialise a tty device
+ *	tty_init_dev_retry	-	initialise a tty device
  *	@driver: tty driver we are opening a device on
  *	@idx: device index
+ *	@retry: retry count if driver has not set tty->port yet
  *	@ret_tty: returned tty structure
  *
  *	Prepare a tty device. This may not be a "new" clean device but
@@ -1314,7 +1315,8 @@ static int tty_reopen(struct tty_struct *tty)
  * relaxed for the (most common) case of reopening a tty.
  */
 
-struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx)
+static struct tty_struct *tty_init_dev_retry(struct tty_driver *driver,
+					     int idx, int retry)
 {
 	struct tty_struct *tty;
 	int retval;
@@ -1343,6 +1345,10 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx)
 
 	if (!tty->port)
 		tty->port = driver->ports[idx];
+	if (!tty->port && retry) {
+		retval = -EAGAIN;
+		goto err_release_driver;
+	}
 
 	WARN_RATELIMIT(!tty->port,
 			"%s: %s driver does not set tty->port. This will crash the kernel later. Fix the driver!\n",
@@ -1365,6 +1371,8 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx)
 	/* Return the tty locked so that it cannot vanish under the caller */
 	return tty;
 
+err_release_driver:
+	tty_driver_remove_tty(driver, tty);
 err_free_tty:
 	tty_unlock(tty);
 	free_tty_struct(tty);
@@ -1384,6 +1392,19 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx)
 }
 
 /**
+ *	tty_init_dev		-	initialise a tty device
+ *	@driver: tty driver we are opening a device on
+ *	@idx: device index
+ *	@ret_tty: returned tty structure
+ *
+ *	Calls tty_init_dev_retry() with retry count of 0.
+ */
+struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx)
+{
+	return tty_init_dev_retry(driver, idx, 0);
+}
+
+/**
  * tty_save_termios() - save tty termios data in driver table
  * @tty: tty whose termios data to save
  *
-- 
2.11.0

[PATCH v2 2/2] tty: use tty_init_dev_retry() to workaround a race condition

[Sudip Mukherjee]

There seems to be a race condition in tty drivers and I could see on
many boot cycles a NULL pointer dereference as tty_init_dev() tries to
do 'tty->port->itty = tty' even though tty->port is NULL.
'tty->port' will be set by the driver and if the driver has not yet done
it before we open the tty device we can get to this situation. By adding
some extra debug prints, I noticed that:

6.650130: uart_add_one_port
6.663849: register_console
6.664846: tty_open
6.674391: tty_init_dev
6.675456: tty_port_link_device

uart_add_one_port() registers the console, as soon as it registers, the
userspace tries to use it and that leads to tty_open() but
uart_add_one_port() has not yet done tty_port_link_device() and so
tty->port is not yet configured when control reaches tty_init_dev().

So, add one retry and use tty_init_dev_retry().

Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
---

v1: had some hardcoded numbers which were difficult to understand.
https://lore.kernel.org/lkml/20191120151709.14148-2-sudipm.mukherjee@gmail.com/

I know this is not a proper fix, and the proper fix should have been to
have a lock. But that will be too intrusive and adding retry was a safer
option than that.

 drivers/tty/tty_io.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 95d7abeca254..f71a11895230 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1945,6 +1945,7 @@ EXPORT_SYMBOL_GPL(tty_kopen);
 /**
  *	tty_open_by_driver	-	open a tty device
  *	@device: dev_t of device to open
+ *	@retry: number of times to retry if tty_init_dev_retry fails
  *	@filp: file pointer to tty
  *
  *	Performs the driver lookup, checks for a reopen, or otherwise
@@ -1957,7 +1958,7 @@ EXPORT_SYMBOL_GPL(tty_kopen);
  *	  - concurrent tty driver removal w/ lookup
  *	  - concurrent tty removal from driver table
  */
-static struct tty_struct *tty_open_by_driver(dev_t device,
+static struct tty_struct *tty_open_by_driver(dev_t device, int retry,
 					     struct file *filp)
 {
 	struct tty_struct *tty;
@@ -2001,7 +2002,7 @@ static struct tty_struct *tty_open_by_driver(dev_t device,
 			tty = ERR_PTR(retval);
 		}
 	} else { /* Returns with the tty_lock held for now */
-		tty = tty_init_dev(driver, index);
+		tty = tty_init_dev_retry(driver, index, retry);
 		mutex_unlock(&tty_mutex);
 	}
 out:
@@ -2036,7 +2037,7 @@ static struct tty_struct *tty_open_by_driver(dev_t device,
 static int tty_open(struct inode *inode, struct file *filp)
 {
 	struct tty_struct *tty;
-	int noctty, retval;
+	int noctty, retval, retry = 1;
 	dev_t device = inode->i_rdev;
 	unsigned saved_flags = filp->f_flags;
 
@@ -2049,7 +2050,7 @@ static int tty_open(struct inode *inode, struct file *filp)
 
 	tty = tty_open_current_tty(device, filp);
 	if (!tty)
-		tty = tty_open_by_driver(device, filp);
+		tty = tty_open_by_driver(device, retry--, filp);
 
 	if (IS_ERR(tty)) {
 		tty_free_file(filp);
-- 
2.11.0

Re: [PATCH v2 2/2] tty: use tty_init_dev_retry() to workaround a race condition

[Greg Kroah-Hartman]

On Thu, Nov 21, 2019 at 03:22:39PM +0000, Sudip Mukherjee wrote:
> There seems to be a race condition in tty drivers and I could see on
> many boot cycles a NULL pointer dereference as tty_init_dev() tries to
> do 'tty->port->itty = tty' even though tty->port is NULL.
> 'tty->port' will be set by the driver and if the driver has not yet done
> it before we open the tty device we can get to this situation. By adding
> some extra debug prints, I noticed that:
> 
> 6.650130: uart_add_one_port
> 6.663849: register_console
> 6.664846: tty_open
> 6.674391: tty_init_dev
> 6.675456: tty_port_link_device
> 
> uart_add_one_port() registers the console, as soon as it registers, the
> userspace tries to use it and that leads to tty_open() but
> uart_add_one_port() has not yet done tty_port_link_device() and so
> tty->port is not yet configured when control reaches tty_init_dev().

Shouldn't we do tty_port_link_device() before uart_add_one_port() to
remove that race?  Once you register the console, yes, tty_open() can
happen, so the driver had better be ready to go at that point in time.

This feels like it should be fixed by the caller, not in the tty core.
Any reason that can not happen?

thanks,

greg k-h

Re: [PATCH v2 2/2] tty: use tty_init_dev_retry() to workaround a race condition

[Sudip Mukherjee]

Hi Greg,

On Thu, Nov 21, 2019 at 05:41:38PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Nov 21, 2019 at 03:22:39PM +0000, Sudip Mukherjee wrote:
> > There seems to be a race condition in tty drivers and I could see on
> > many boot cycles a NULL pointer dereference as tty_init_dev() tries to
> > do 'tty->port->itty = tty' even though tty->port is NULL.
> > 'tty->port' will be set by the driver and if the driver has not yet done
> > it before we open the tty device we can get to this situation. By adding
> > some extra debug prints, I noticed that:
> > 
> > 6.650130: uart_add_one_port
> > 6.663849: register_console
> > 6.664846: tty_open
> > 6.674391: tty_init_dev
> > 6.675456: tty_port_link_device
> > 
> > uart_add_one_port() registers the console, as soon as it registers, the
> > userspace tries to use it and that leads to tty_open() but
> > uart_add_one_port() has not yet done tty_port_link_device() and so
> > tty->port is not yet configured when control reaches tty_init_dev().
> 
> Shouldn't we do tty_port_link_device() before uart_add_one_port() to
> remove that race?  Once you register the console, yes, tty_open() can
> happen, so the driver had better be ready to go at that point in time.
> 

But tty_port_link_device() is done by uart_add_one_port() itself.
After registering the console uart_add_one_port() will call
tty_port_register_device_attr_serdev() and tty_port_link_device() is
called from this. Thats still tty core.

> This feels like it should be fixed by the caller, not in the tty core.
> Any reason that can not happen?

tty_port_register_device_attr_serdev() is part of tty core. Or is my
above understanding wrong?


--
Regards
Sudip

Re: [PATCH v2 2/2] tty: use tty_init_dev_retry() to workaround a race condition

[Jiri Slaby]

On 21. 11. 19, 22:01, Sudip Mukherjee wrote:
> Hi Greg,
> 
> On Thu, Nov 21, 2019 at 05:41:38PM +0100, Greg Kroah-Hartman wrote:
>> On Thu, Nov 21, 2019 at 03:22:39PM +0000, Sudip Mukherjee wrote:
>>> There seems to be a race condition in tty drivers and I could see on
>>> many boot cycles a NULL pointer dereference as tty_init_dev() tries to
>>> do 'tty->port->itty = tty' even though tty->port is NULL.
>>> 'tty->port' will be set by the driver and if the driver has not yet done
>>> it before we open the tty device we can get to this situation. By adding
>>> some extra debug prints, I noticed that:
>>>
>>> 6.650130: uart_add_one_port
>>> 6.663849: register_console
>>> 6.664846: tty_open
>>> 6.674391: tty_init_dev
>>> 6.675456: tty_port_link_device
>>>
>>> uart_add_one_port() registers the console, as soon as it registers, the
>>> userspace tries to use it and that leads to tty_open() but
>>> uart_add_one_port() has not yet done tty_port_link_device() and so
>>> tty->port is not yet configured when control reaches tty_init_dev().
>>
>> Shouldn't we do tty_port_link_device() before uart_add_one_port() to
>> remove that race?  Once you register the console, yes, tty_open() can
>> happen, so the driver had better be ready to go at that point in time.
>>
> 
> But tty_port_link_device() is done by uart_add_one_port() itself.
> After registering the console uart_add_one_port() will call
> tty_port_register_device_attr_serdev() and tty_port_link_device() is
> called from this. Thats still tty core.

Interferences of console vs tty code are ugly. Does it help to simply
put tty_port_link_device to uart_add_one_port before uart_configure_port?

thanks,
-- 
js
suse labs

Re: [PATCH v2 2/2] tty: use tty_init_dev_retry() to workaround a race condition

[Jiri Slaby]

On 22. 11. 19, 10:05, Jiri Slaby wrote:
> On 21. 11. 19, 22:01, Sudip Mukherjee wrote:
>> Hi Greg,
>>
>> On Thu, Nov 21, 2019 at 05:41:38PM +0100, Greg Kroah-Hartman wrote:
>>> On Thu, Nov 21, 2019 at 03:22:39PM +0000, Sudip Mukherjee wrote:
>>>> There seems to be a race condition in tty drivers and I could see on
>>>> many boot cycles a NULL pointer dereference as tty_init_dev() tries to
>>>> do 'tty->port->itty = tty' even though tty->port is NULL.
>>>> 'tty->port' will be set by the driver and if the driver has not yet done
>>>> it before we open the tty device we can get to this situation. By adding
>>>> some extra debug prints, I noticed that:
>>>>
>>>> 6.650130: uart_add_one_port
>>>> 6.663849: register_console
>>>> 6.664846: tty_open
>>>> 6.674391: tty_init_dev
>>>> 6.675456: tty_port_link_device
>>>>
>>>> uart_add_one_port() registers the console, as soon as it registers, the
>>>> userspace tries to use it and that leads to tty_open() but
>>>> uart_add_one_port() has not yet done tty_port_link_device() and so
>>>> tty->port is not yet configured when control reaches tty_init_dev().
>>>
>>> Shouldn't we do tty_port_link_device() before uart_add_one_port() to
>>> remove that race?  Once you register the console, yes, tty_open() can
>>> happen, so the driver had better be ready to go at that point in time.
>>>
>>
>> But tty_port_link_device() is done by uart_add_one_port() itself.
>> After registering the console uart_add_one_port() will call
>> tty_port_register_device_attr_serdev() and tty_port_link_device() is
>> called from this. Thats still tty core.
> 
> Interferences of console vs tty code are ugly. Does it help to simply
> put tty_port_link_device to uart_add_one_port before uart_configure_port?

Alternatively, you can try setting tty_port in uart_install by:
tty->port = &state->port.

BTW do you see the warning from tty_init_dev:
"driver does not set tty->port. This will crash the kernel later. Fix
the driver!\n"
? Maybe not given console is registered already, but crashes.

> thanks,
-- 
js
suse labs

Re: [PATCH v2 2/2] tty: use tty_init_dev_retry() to workaround a race condition

[Sudip Mukherjee]

On Fri, Nov 22, 2019 at 10:11:26AM +0100, Jiri Slaby wrote:
> On 22. 11. 19, 10:05, Jiri Slaby wrote:
> > On 21. 11. 19, 22:01, Sudip Mukherjee wrote:
> >> Hi Greg,
> >>
> >> On Thu, Nov 21, 2019 at 05:41:38PM +0100, Greg Kroah-Hartman wrote:
> >>> On Thu, Nov 21, 2019 at 03:22:39PM +0000, Sudip Mukherjee wrote:
> >>>> There seems to be a race condition in tty drivers and I could see on
> >>>> many boot cycles a NULL pointer dereference as tty_init_dev() tries to

<snip>

> > 
> > Interferences of console vs tty code are ugly. Does it help to simply
> > put tty_port_link_device to uart_add_one_port before uart_configure_port?
> 
> Alternatively, you can try setting tty_port in uart_install by:
> tty->port = &state->port.

I have not tried these. will try.

> 
> BTW do you see the warning from tty_init_dev:
> "driver does not set tty->port. This will crash the kernel later. Fix
> the driver!\n"
> ? Maybe not given console is registered already, but crashes.

yes. I do see the warning but I have always assumed that the warning is
because console is openend as soon as it registers and so uart_add_one_port()
does not get the chance to link it. Is it not so?

--
Regards
Sudip

Re: [PATCH v2 2/2] tty: use tty_init_dev_retry() to workaround a race condition

[Jiri Slaby]

On 24. 11. 19, 1:02, Sudip Mukherjee wrote:
>> BTW do you see the warning from tty_init_dev:
>> "driver does not set tty->port. This will crash the kernel later. Fix
>> the driver!\n"
>> ? Maybe not given console is registered already, but crashes.
> 
> yes. I do see the warning but I have always assumed that the warning is
> because console is openend as soon as it registers and so uart_add_one_port()
> does not get the chance to link it. Is it not so?

Yes it is, I was just curious...

thanks,
-- 
js
suse labs

Re: [PATCH v2 2/2] tty: use tty_init_dev_retry() to workaround a race condition

[Sudip Mukherjee]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

Hi Jiri,

On Fri, Nov 22, 2019 at 10:05:09AM +0100, Jiri Slaby wrote:
> On 21. 11. 19, 22:01, Sudip Mukherjee wrote:
> > Hi Greg,
> > 
> > On Thu, Nov 21, 2019 at 05:41:38PM +0100, Greg Kroah-Hartman wrote:
> >> On Thu, Nov 21, 2019 at 03:22:39PM +0000, Sudip Mukherjee wrote:
> >>> There seems to be a race condition in tty drivers and I could see on
> >>> many boot cycles a NULL pointer dereference as tty_init_dev() tries to
> >>> do 'tty->port->itty = tty' even though tty->port is NULL.
<snip>
> >>>
> >>> uart_add_one_port() registers the console, as soon as it registers, the
> >>> userspace tries to use it and that leads to tty_open() but
> >>> uart_add_one_port() has not yet done tty_port_link_device() and so
> >>> tty->port is not yet configured when control reaches tty_init_dev().
> >>
> >> Shouldn't we do tty_port_link_device() before uart_add_one_port() to
> >> remove that race?  Once you register the console, yes, tty_open() can
> >> happen, so the driver had better be ready to go at that point in time.
> >>
> > 
> > But tty_port_link_device() is done by uart_add_one_port() itself.
> > After registering the console uart_add_one_port() will call
> > tty_port_register_device_attr_serdev() and tty_port_link_device() is
> > called from this. Thats still tty core.
> 
> Interferences of console vs tty code are ugly. Does it help to simply
> put tty_port_link_device to uart_add_one_port before uart_configure_port?

sorry for the late response, got busy with an out-of-tree driver.

It fixes the problem if I put tty_port_link_device() before
uart_configure_port(). Please check the attached patch and that
completely fixes the problem. Do you want me to send a proper patch for
it or do you want me to check more into it?

--
Regards
Sudip

[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 961 bytes --]

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 351843f847c0..006d478a63be 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2820,6 +2820,7 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 	if (uport->cons && uport->dev)
 		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
 
+	tty_port_link_device(port, drv->tty_driver, uport->line);
 	uart_configure_port(drv, state, uport);
 
 	port->console = uart_console(uport);
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index a9e12b3bc31d..a4f85fc75539 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -89,7 +89,8 @@ void tty_port_link_device(struct tty_port *port,
 {
 	if (WARN_ON(index >= driver->num))
 		return;
-	driver->ports[index] = port;
+	if (!driver->ports[index])
+		driver->ports[index] = port;
 }
 EXPORT_SYMBOL_GPL(tty_port_link_device);
 

Re: [PATCH v2 2/2] tty: use tty_init_dev_retry() to workaround a race condition

[Greg Kroah-Hartman]

On Tue, Dec 10, 2019 at 11:41:47AM +0000, Sudip Mukherjee wrote:
> Hi Jiri,
> 
> On Fri, Nov 22, 2019 at 10:05:09AM +0100, Jiri Slaby wrote:
> > On 21. 11. 19, 22:01, Sudip Mukherjee wrote:
> > > Hi Greg,
> > > 
> > > On Thu, Nov 21, 2019 at 05:41:38PM +0100, Greg Kroah-Hartman wrote:
> > >> On Thu, Nov 21, 2019 at 03:22:39PM +0000, Sudip Mukherjee wrote:
> > >>> There seems to be a race condition in tty drivers and I could see on
> > >>> many boot cycles a NULL pointer dereference as tty_init_dev() tries to
> > >>> do 'tty->port->itty = tty' even though tty->port is NULL.
> <snip>
> > >>>
> > >>> uart_add_one_port() registers the console, as soon as it registers, the
> > >>> userspace tries to use it and that leads to tty_open() but
> > >>> uart_add_one_port() has not yet done tty_port_link_device() and so
> > >>> tty->port is not yet configured when control reaches tty_init_dev().
> > >>
> > >> Shouldn't we do tty_port_link_device() before uart_add_one_port() to
> > >> remove that race?  Once you register the console, yes, tty_open() can
> > >> happen, so the driver had better be ready to go at that point in time.
> > >>
> > > 
> > > But tty_port_link_device() is done by uart_add_one_port() itself.
> > > After registering the console uart_add_one_port() will call
> > > tty_port_register_device_attr_serdev() and tty_port_link_device() is
> > > called from this. Thats still tty core.
> > 
> > Interferences of console vs tty code are ugly. Does it help to simply
> > put tty_port_link_device to uart_add_one_port before uart_configure_port?
> 
> sorry for the late response, got busy with an out-of-tree driver.
> 
> It fixes the problem if I put tty_port_link_device() before
> uart_configure_port(). Please check the attached patch and that
> completely fixes the problem. Do you want me to send a proper patch for
> it or do you want me to check more into it?

This looks a lot more sane to me, can you resend it in proper format so
that I can apply it?

thanks,

greg k-h

Re: [PATCH v2 2/2] tty: use tty_init_dev_retry() to workaround a race condition

[Sudip Mukherjee]

Hi Greg,

On Thu, Dec 12, 2019 at 11:15 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Dec 10, 2019 at 11:41:47AM +0000, Sudip Mukherjee wrote:
> > Hi Jiri,
> >
> > On Fri, Nov 22, 2019 at 10:05:09AM +0100, Jiri Slaby wrote:
> > > On 21. 11. 19, 22:01, Sudip Mukherjee wrote:
> > > > Hi Greg,
> > > >
> > > > On Thu, Nov 21, 2019 at 05:41:38PM +0100, Greg Kroah-Hartman wrote:
> > > >> On Thu, Nov 21, 2019 at 03:22:39PM +0000, Sudip Mukherjee wrote:
> > > >>> There seems to be a race condition in tty drivers and I could see on
> > > >>> many boot cycles a NULL pointer dereference as tty_init_dev() tries to
> > > >>> do 'tty->port->itty = tty' even though tty->port is NULL.
> > <snip>
> > > >>>
> > > >>> uart_add_one_port() registers the console, as soon as it registers, the
> > > >>> userspace tries to use it and that leads to tty_open() but
> > > >>> uart_add_one_port() has not yet done tty_port_link_device() and so
> > > >>> tty->port is not yet configured when control reaches tty_init_dev().
> > > >>
> > > >> Shouldn't we do tty_port_link_device() before uart_add_one_port() to
> > > >> remove that race?  Once you register the console, yes, tty_open() can
> > > >> happen, so the driver had better be ready to go at that point in time.
> > > >>
> > > >
> > > > But tty_port_link_device() is done by uart_add_one_port() itself.
> > > > After registering the console uart_add_one_port() will call
> > > > tty_port_register_device_attr_serdev() and tty_port_link_device() is
> > > > called from this. Thats still tty core.
> > >
> > > Interferences of console vs tty code are ugly. Does it help to simply
> > > put tty_port_link_device to uart_add_one_port before uart_configure_port?
> >
> > sorry for the late response, got busy with an out-of-tree driver.
> >
> > It fixes the problem if I put tty_port_link_device() before
> > uart_configure_port(). Please check the attached patch and that
> > completely fixes the problem. Do you want me to send a proper patch for
> > it or do you want me to check more into it?
>
> This looks a lot more sane to me, can you resend it in proper format so
> that I can apply it?

https://lore.kernel.org/lkml/20191212131602.29504-1-sudipm.mukherjee@gmail.com/


-- 
Regards
Sudip

Re: [PATCH v2 2/2] tty: use tty_init_dev_retry() to workaround a race condition

[Greg Kroah-Hartman]

On Tue, Dec 17, 2019 at 11:47:25AM +0000, Sudip Mukherjee wrote:
> Hi Greg,
> 
> On Thu, Dec 12, 2019 at 11:15 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Dec 10, 2019 at 11:41:47AM +0000, Sudip Mukherjee wrote:
> > > Hi Jiri,
> > >
> > > On Fri, Nov 22, 2019 at 10:05:09AM +0100, Jiri Slaby wrote:
> > > > On 21. 11. 19, 22:01, Sudip Mukherjee wrote:
> > > > > Hi Greg,
> > > > >
> > > > > On Thu, Nov 21, 2019 at 05:41:38PM +0100, Greg Kroah-Hartman wrote:
> > > > >> On Thu, Nov 21, 2019 at 03:22:39PM +0000, Sudip Mukherjee wrote:
> > > > >>> There seems to be a race condition in tty drivers and I could see on
> > > > >>> many boot cycles a NULL pointer dereference as tty_init_dev() tries to
> > > > >>> do 'tty->port->itty = tty' even though tty->port is NULL.
> > > <snip>
> > > > >>>
> > > > >>> uart_add_one_port() registers the console, as soon as it registers, the
> > > > >>> userspace tries to use it and that leads to tty_open() but
> > > > >>> uart_add_one_port() has not yet done tty_port_link_device() and so
> > > > >>> tty->port is not yet configured when control reaches tty_init_dev().
> > > > >>
> > > > >> Shouldn't we do tty_port_link_device() before uart_add_one_port() to
> > > > >> remove that race?  Once you register the console, yes, tty_open() can
> > > > >> happen, so the driver had better be ready to go at that point in time.
> > > > >>
> > > > >
> > > > > But tty_port_link_device() is done by uart_add_one_port() itself.
> > > > > After registering the console uart_add_one_port() will call
> > > > > tty_port_register_device_attr_serdev() and tty_port_link_device() is
> > > > > called from this. Thats still tty core.
> > > >
> > > > Interferences of console vs tty code are ugly. Does it help to simply
> > > > put tty_port_link_device to uart_add_one_port before uart_configure_port?
> > >
> > > sorry for the late response, got busy with an out-of-tree driver.
> > >
> > > It fixes the problem if I put tty_port_link_device() before
> > > uart_configure_port(). Please check the attached patch and that
> > > completely fixes the problem. Do you want me to send a proper patch for
> > > it or do you want me to check more into it?
> >
> > This looks a lot more sane to me, can you resend it in proper format so
> > that I can apply it?
> 
> https://lore.kernel.org/lkml/20191212131602.29504-1-sudipm.mukherjee@gmail.com/

It's in my "to review" queue, don't worry, not lost :)