tty/tty_io.c: make a check before reuse cdev

PATCH In review pp 5 participants 12 messages Jul 21, 2014 – Jul 25, 2014 TTY layer and serial drivers

Share & tools

pp

author

2014-07-21 12:47 UTC #0 lore raw

PATCH 1/1 tty/tty_io.c: make a check before reuse cdev

As reuse the cdev may cause panic. After we unregister the tty device, we may use tty_hangup() o other similar function to send a signal(SIGHUP) to process which has opend our device. But that not succeed if the process couldn't get the signal. for example, a process forked but his parent quited never get SIGHUP.

Here is our scence.
tty driver register its device and init the cdevs, then process "A" open one cdev.
tty driver unregister its device and cdev_del the cdevs, call tty_hangup to (S)send signal SIGHUP to process A. But that step(S) fails.
tty driver register its device and (D)init the cdevs again.
What will happen is that when process "A" close the cdev, cd_forget() get panic as there is list_del_init(&inode->i_devices); When we init the cdevs in step(D), the list head was initialized by INIT_LIST_HEAD(&cdev->list);
So check the kobj ref-count before init cdev, and use a bool array to detect if it's ok to unregister the cvdes. Because most tty driver don't check the ret code of tty_register_device(). and they may still call tty_unregister_device().

Signed-off-by xinhui.pan <[email protected]>

---
 drivers/tty/tty_io.c       |   17 +++++++++++++++--
 include/linux/tty_driver.h |    1 +
 2 files changed, 16 insertions(+), 2 deletions(-)

drivers/tty/tty_io.c +15 -2

index 3411071..7e79281 100644
@@ -3080,6 +3080,10 @@ struct class *tty_class;
 static int tty_cdev_add(struct tty_driver *driver, dev_t dev,
 		unsigned int index, unsigned int count)
 {
+	if (atomic_read(&driver->cdevs[index].kobj.kref.refcount)) {
+		WARN(1, "init a still in-used cdev!");
+		return -EBUSY;
+	}
 	/* init here, since reused cdevs cause crashes */
 	cdev_init(&driver->cdevs[index], &tty_fops);
 	driver->cdevs[index].owner = driver->owner;

@@ -3185,12 +3189,15 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
 	if (retval)
 		goto error;
 
+	driver->cdevsb[index] = 1;
 	return dev;
 
 error:
 	put_device(dev);
 	if (cdev)
 		cdev_del(&driver->cdevs[index]);
+
+	driver->cdevsb[index] = 0;
 	return ERR_PTR(retval);
 }
 EXPORT_SYMBOL_GPL(tty_register_device_attr);

@@ -3208,6 +3215,8 @@ EXPORT_SYMBOL_GPL(tty_register_device_attr);
 
 void tty_unregister_device(struct tty_driver *driver, unsigned index)
 {
+	if (driver->cdevsb[index] == 0)
+		return;
 	device_destroy(tty_class,
 		MKDEV(driver->major, driver->minor_start) + index);
 	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC))

@@ -3265,14 +3274,17 @@ struct tty_driver *__tty_alloc_driver(unsigned int lines, struct module *owner,
 		cdevs = lines;
 	}
 
-	driver->cdevs = kcalloc(cdevs, sizeof(*driver->cdevs), GFP_KERNEL);
-	if (!driver->cdevs) {
+	driver->cdevs  = kcalloc(cdevs, sizeof(*driver->cdevs), GFP_KERNEL);
+	driver->cdevsb = kcalloc(cdevs, sizeof(bool), GFP_KERNEL);
+	if (!driver->cdevs || !driver->cdevsb) {
 		err = -ENOMEM;
 		goto err_free_all;
 	}
 
 	return driver;
 err_free_all:
+	kfree(driver->cdevs);
+	kfree(driver->cdevsb);
 	kfree(driver->ports);
 	kfree(driver->ttys);
 	kfree(driver->termios);

@@ -3307,6 +3319,7 @@ static void destruct_tty_driver(struct kref *kref)
 			cdev_del(&driver->cdevs[0]);
 	}
 	kfree(driver->cdevs);
+	kfree(driver->cdevsb);
 	kfree(driver->ports);
 	kfree(driver->termios);
 	kfree(driver->ttys);

include/linux/tty_driver.h +1 -0

index 756a609..7294f1b 100644
@@ -291,6 +291,7 @@ struct tty_driver {
 	int	magic;		/* magic number for this structure */
 	struct kref kref;	/* Reference management */
 	struct cdev *cdevs;
+	bool	*cdevsb;
 	struct module	*owner;
 	const char	*driver_name;
 	const char	*name;

Signature · 1 line

1.7.9.5

Greg KH

maintainer

2014-07-21 15:39 UTC #1 lore raw

Quoted 9 lines · pp

As reuse the cdev may cause panic. After we unregister the tty device, we may use tty_hangup() o other similar function to send a signal(SIGHUP) to process which has opend our device. But that not succeed if the process couldn't get the signal. for example, a process forked but his parent quited never get SIGHUP.
Here is our scence.
tty driver register its device and init the cdevs, then process "A" open one cdev.
tty driver unregister its device and cdev_del the cdevs, call tty_hangup to (S)send signal SIGHUP to process A. But that step(S) fails.

How can that fail? What driver does this fail for?

Quoted 1 line

tty driver register its device and (D)init the cdevs again.

What driver does this with an "old" device, it should have created a new one, otherwise, as you have pointed out, it's a bug.

thanks,

greg k-h

xinhui.pan

2014-07-22 11:53 UTC #2 lore raw

Quoted 6 lines · Greg KH

Quoted 5 lines · pp
As reuse the cdev may cause panic. After we unregister the tty device, we may use tty_hangup() o other similar function to send a signal(SIGHUP) to process which has opend our device. But that not succeed if the process couldn't get the signal. for example, a process forked but his parent quited never get SIGHUP.

Quoted 6 lines

Quoted 4 lines
Here is our scence.
tty driver register its device and init the cdevs, then process "A" open one cdev.
tty driver unregister its device and cdev_del the cdevs, call tty_hangup to (S)send signal SIGHUP to process A. But that step(S) fails.
How can that fail? What driver does this fail for?

hi, Greg
Thanks for your nice comments. :)
It's gsm driver that want to unregister/register tty device. We are working on our intel mobile phone,
When the phone goes into airplane-mode, the modem will disconnect from system, then gsmld_close() -> gsmld_detach_gsm() -> tty_unregister_device(). When the phone leaves airplane-mode, the modem will connect to system, then gsmld_open() -> gsmld_attach_gsm() -> tty_register_device() In this way how gsm driver works.
It seems very normal and can work well. :)

But there is always something bad for us to deal with.
If a process(A, its name) opens the /dev/gsmttyXX, and the process(A) is, for example, running with command "A &".
The process(A) is not able to receive the signal SIGHUP from __tty_hangup() -> tty_signal_session_leader().
There are several reasons that can stop process(A) from receiving signal SIGHUP.
another example, B is running, and he makes a fork(), A is the child of B, then B quit, leave A running.
in such scenario, A is not able to receive signal SIGHUP, either.
Anyway, we cannot guarantee process(A) will close /dev/gsmttyXX in time. That means we don't know when we can reuse the tty_driver->cdevs[XX]. one second, one minute? We don't know. We just don't trust user space. :)

Quoted 6 lines

Quoted 2 lines
tty driver register its device and (D)init the cdevs again.
What driver does this with an "old" device, it should have created a new one, otherwise, as you have pointed out, it's a bug.

I can't agree more with you. we should not use "old" device.
gsm driver(gsm_tty_driver) call tty_register_device() to export gsmttyXX to userspace.
but tty core code manages cdevs by itself. cdevs is not directly used by any other tty drivers. Adding a check in n_gsm.c before call tty_register_device() seems breaking a rule and making things complex in my opinion.
I also find a fact that most tty drivers only call tty_register_device() in probe(), and call tty_unregister_device() in remove().
Q1:
Is this a rule, too? :)

I think my patch is not good enough.
Q2:
How about (1)change struct *cdevs to struct **cdevs, and (2)use cdev_alloc() instead of cdev_init()? :)
Let char_dev core code manage the cdev. when the cdev's ref-count become zero, cdev will be released by char_dev core code self.

I very appreciate for your comments. and your advices are very important and helpful for me to solve this issue.

thanks,

xinhui

Quoted 4 lines

thanks,
greg k-h

Peter Hurley

2014-07-22 16:41 UTC #3 lore raw

Quoted 23 lines · xinhui.pan

Quoted 6 lines · Greg KH
Quoted 5 lines · pp
As reuse the cdev may cause panic. After we unregister the tty device, we may use tty_hangup() o other similar function to send a signal(SIGHUP) to process which has opend our device. But that not succeed if the process couldn't get the signal. for example, a process forked but his parent quited never get SIGHUP.
Quoted 6 lines
Quoted 4 lines
Here is our scence.
tty driver register its device and init the cdevs, then process "A" open one cdev.
tty driver unregister its device and cdev_del the cdevs, call tty_hangup to (S)send signal SIGHUP to process A. But that step(S) fails.
How can that fail? What driver does this fail for?
hi, Greg
Thanks for your nice comments. :)
It's gsm driver that want to unregister/register tty device. We are working on our intel mobile phone,
When the phone goes into airplane-mode, the modem will disconnect from system, then gsmld_close() -> gsmld_detach_gsm() -> tty_unregister_device(). When the phone leaves airplane-mode, the modem will connect to system, then gsmld_open() -> gsmld_attach_gsm() -> tty_register_device() In this way how gsm driver works.
It seems very normal and can work well. :)

Quoted 8 lines

But there is always something bad for us to deal with.
If a process(A, its name) opens the /dev/gsmttyXX, and the process(A) is, for example, running with command "A &".
The process(A) is not able to receive the signal SIGHUP from __tty_hangup() -> tty_signal_session_leader().
There are several reasons that can stop process(A) from receiving signal SIGHUP.
another example, B is running, and he makes a fork(), A is the child of B, then B quit, leave A running.
in such scenario, A is not able to receive signal SIGHUP, either.
Anyway, we cannot guarantee process(A) will close /dev/gsmttyXX in time. That means we don't know when we can reuse the tty_driver->cdevs[XX]. one second, one minute? We don't know. We just don't trust user space. :)

Or a process could simply ignore SIGHUP, in which case /dev/gsmttyXX will not be closed until process termination.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
tty driver register its device and (D)init the cdevs again.
What driver does this with an "old" device, it should have created a new one, otherwise, as you have pointed out, it's a bug.
I can't agree more with you. we should not use "old" device.

This is a gsm driver problem. The GSM driver is reusing device indexes for still-open ttys.

The GSM driver uses a global table, gsm_mux[], to allocate device indexes but prematurely clears the table entry in gsm_mux_cleanup(). If instead, clearing the gsm_mux table entry were deferred to gsm_mux_free(), then device indexes would not be getting reused until after the last tty associated with the last gsm attach was closed.

Regards,
Peter Hurley

Greg KH

maintainer

2014-07-22 17:38 UTC #4 lore raw

Quoted 38 lines · Peter Hurley

Quoted 23 lines · xinhui.pan
Quoted 6 lines · Greg KH
Quoted 5 lines · pp
As reuse the cdev may cause panic. After we unregister the tty device, we may use tty_hangup() o other similar function to send a signal(SIGHUP) to process which has opend our device. But that not succeed if the process couldn't get the signal. for example, a process forked but his parent quited never get SIGHUP.
Quoted 6 lines
Quoted 4 lines
Here is our scence.
tty driver register its device and init the cdevs, then process "A" open one cdev.
tty driver unregister its device and cdev_del the cdevs, call tty_hangup to (S)send signal SIGHUP to process A. But that step(S) fails.
How can that fail? What driver does this fail for?
hi, Greg
Thanks for your nice comments. :)
It's gsm driver that want to unregister/register tty device. We are working on our intel mobile phone,
When the phone goes into airplane-mode, the modem will disconnect from system, then gsmld_close() -> gsmld_detach_gsm() -> tty_unregister_device(). When the phone leaves airplane-mode, the modem will connect to system, then gsmld_open() -> gsmld_attach_gsm() -> tty_register_device() In this way how gsm driver works.
It seems very normal and can work well. :)
Quoted 8 lines
But there is always something bad for us to deal with.
If a process(A, its name) opens the /dev/gsmttyXX, and the process(A) is, for example, running with command "A &".
The process(A) is not able to receive the signal SIGHUP from __tty_hangup() -> tty_signal_session_leader().
There are several reasons that can stop process(A) from receiving signal SIGHUP.
another example, B is running, and he makes a fork(), A is the child of B, then B quit, leave A running.
in such scenario, A is not able to receive signal SIGHUP, either.
Anyway, we cannot guarantee process(A) will close /dev/gsmttyXX in time. That means we don't know when we can reuse the tty_driver->cdevs[XX]. one second, one minute? We don't know. We just don't trust user space. :)
Or a process could simply ignore SIGHUP, in which case /dev/gsmttyXX will not be closed until process termination.
Quoted 2 lines
Quoted 2 lines
Quoted 1 line
tty driver register its device and (D)init the cdevs again.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
What driver does this with an "old" device, it should have created a new one, otherwise, as you have pointed out, it's a bug.
I can't agree more with you. we should not use "old" device.
This is a gsm driver problem. The GSM driver is reusing device indexes for still-open ttys.

I agree, it should not be doing that at all, please fix that up, and all should be fine.

greg k-h

xinhui.pan

2014-07-23 09:22 UTC #5 lore raw

In reply to Peter Hurley

Quoted 33 lines · Peter Hurley

Quoted 23 lines · xinhui.pan
Quoted 6 lines · Greg KH
Quoted 5 lines · pp
As reuse the cdev may cause panic. After we unregister the tty device, we may use tty_hangup() o other similar function to send a signal(SIGHUP) to process which has opend our device. But that not succeed if the process couldn't get the signal. for example, a process forked but his parent quited never get SIGHUP.
Quoted 6 lines
Quoted 4 lines
Here is our scence.
tty driver register its device and init the cdevs, then process "A" open one cdev.
tty driver unregister its device and cdev_del the cdevs, call tty_hangup to (S)send signal SIGHUP to process A. But that step(S) fails.
How can that fail? What driver does this fail for?
hi, Greg
Thanks for your nice comments. :)
It's gsm driver that want to unregister/register tty device. We are working on our intel mobile phone,
When the phone goes into airplane-mode, the modem will disconnect from system, then gsmld_close() -> gsmld_detach_gsm() -> tty_unregister_device(). When the phone leaves airplane-mode, the modem will connect to system, then gsmld_open() -> gsmld_attach_gsm() -> tty_register_device() In this way how gsm driver works.
It seems very normal and can work well. :)
Quoted 8 lines
But there is always something bad for us to deal with.
If a process(A, its name) opens the /dev/gsmttyXX, and the process(A) is, for example, running with command "A &".
The process(A) is not able to receive the signal SIGHUP from __tty_hangup() -> tty_signal_session_leader().
There are several reasons that can stop process(A) from receiving signal SIGHUP.
another example, B is running, and he makes a fork(), A is the child of B, then B quit, leave A running.
in such scenario, A is not able to receive signal SIGHUP, either.
Anyway, we cannot guarantee process(A) will close /dev/gsmttyXX in time. That means we don't know when we can reuse the tty_driver->cdevs[XX]. one second, one minute? We don't know. We just don't trust user space. :)

Quoted 3 lines

Or a process could simply ignore SIGHUP, in which case /dev/gsmttyXX will not be closed until process termination.

hi, Peter
Agree with you. Thanks for your nice comments.

Quoted 8 lines

Quoted 7 lines
Quoted 5 lines
Quoted 1 line
tty driver register its device and (D)init the cdevs again.
What driver does this with an "old" device, it should have created a new one, otherwise, as you have pointed out, it's a bug.
I can't agree more with you. we should not use "old" device.

Quoted 9 lines

This is a gsm driver problem. The GSM driver is reusing device indexes for still-open ttys.
The GSM driver uses a global table, gsm_mux[], to allocate device indexes but prematurely clears the table entry in gsm_mux_cleanup(). If instead, clearing the gsm_mux table entry were deferred to gsm_mux_free(), then device indexes would not be getting reused until after the last tty associated with the last gsm attach was closed.

Very nice solution. We will check if this can cause any risk, both to kernel and user space. Using a new tty base to register with new cdevs may give us more chance to wait PROCESS quit/close. when total 256 tty used up, what we should do is still in discuss.
thanks, I even want to have a cup of coffee with you :)

thanks

xinhui

Quoted 3 lines

Regards,
Peter Hurley

Peter Hurley

2014-07-23 16:04 UTC #6 lore raw

Hi Xinhui,

Quoted 23 lines · xinhui.pan

Quoted 12 lines · Peter Hurley
Quoted 10 lines · xinhui.pan
Quoted 7 lines · Greg KH
Quoted 1 line · pp
tty driver register its device and (D)init the cdevs again.
What driver does this with an "old" device, it should have created a new one, otherwise, as you have pointed out, it's a bug.
I can't agree more with you. we should not use "old" device.
Quoted 9 lines
This is a gsm driver problem. The GSM driver is reusing device indexes for still-open ttys.
The GSM driver uses a global table, gsm_mux[], to allocate device indexes but prematurely clears the table entry in gsm_mux_cleanup(). If instead, clearing the gsm_mux table entry were deferred to gsm_mux_free(), then device indexes would not be getting reused until after the last tty associated with the last gsm attach was closed.

Quoted 3 lines

Very nice solution. We will check if this can cause any risk, both to kernel and user space. Using a new tty base to register with new cdevs may give us more chance to wait PROCESS quit/close. when total 256 tty used up, what we should do is still in discuss.

I saw your patch for the use of gsm->num before gsm_activate_mux() has allocated the table entry; thanks for fixing that.

As for what to do if all the gsm_mux table entries are used: if the error is infrequent, I suggest simply returning an error which is what the driver does currently. Otherwise, a more dynamic allocation scheme may be required.

I did notice while reviewing the error handling that gsmld_open() will leak the entire composite ldisc data allocated by gsm_alloc_mux() if gsmld_attach_gsm() fails.

Regards,
Peter Hurley

One Thousand Gnomes

2014-07-23 16:08 UTC #9 lore raw

In reply to xinhui.pan

Quoted 3 lines

Very nice solution. We will check if this can cause any risk, both to kernel and user space. Using a new tty base to register with new cdevs may give us more chance to wait PROCESS quit/close. when total 256 tty used up, what we should do is still in discuss.

At that point you may want to look at how fuser works and create some kind of policy manager needs to kill problem tasks owning a device.

Or in theory there is no reason nowdays we can't go above 256 devices - in theory 8)

Alan

xinhui.pan

2014-07-24 10:24 UTC #7 lore raw

In reply to Peter Hurley

hi, Peter

Quoted 30 lines · Peter Hurley

Hi Xinhui,
Quoted 23 lines · xinhui.pan
Quoted 12 lines · Peter Hurley
Quoted 10 lines · xinhui.pan
Quoted 7 lines · Greg KH
Quoted 1 line · pp
tty driver register its device and (D)init the cdevs again.
What driver does this with an "old" device, it should have created a new one, otherwise, as you have pointed out, it's a bug.
I can't agree more with you. we should not use "old" device.
Quoted 9 lines
This is a gsm driver problem. The GSM driver is reusing device indexes for still-open ttys.
The GSM driver uses a global table, gsm_mux[], to allocate device indexes but prematurely clears the table entry in gsm_mux_cleanup(). If instead, clearing the gsm_mux table entry were deferred to gsm_mux_free(), then device indexes would not be getting reused until after the last tty associated with the last gsm attach was closed.
Quoted 3 lines
Very nice solution. We will check if this can cause any risk, both to kernel and user space. Using a new tty base to register with new cdevs may give us more chance to wait PROCESS quit/close. when total 256 tty used up, what we should do is still in discuss.

Quoted 7 lines

I saw your patch for the use of gsm->num before gsm_activate_mux() has allocated the table entry; thanks for fixing that.
As for what to do if all the gsm_mux table entries are used: if the error is infrequent, I suggest simply returning an error which is what the driver does currently. Otherwise, a more dynamic allocation scheme may be required.

Yes, agree with you. This error is infrequent. Intel software will close the gsmtty after read/write hit errors. We don't need dynamic allocation. So keep returning an error here. :)

Quoted 4 lines

I did notice while reviewing the error handling that gsmld_open() will leak the entire composite ldisc data allocated by gsm_alloc_mux() if gsmld_attach_gsm() fails.

Thanks for your reviewing. Do you mean gsm = gsm_alloc_mux() will cause leak if gsmld_attach_gsm fails? As there is no gsm_free_mux()? If so, Yes, it is. In such scenario, gsmld_close is not called. and gsm_free_mux is not called, either.
Thanks for your nice comments. You really help us fix several ugly issues.
Let me have a deep think about it.

thanks,

xinhui

Quoted 3 lines

Regards,
Peter Hurley

xinhui.pan

2014-07-24 11:02 UTC #10 lore raw

In reply to One Thousand Gnomes

Quoted 10 lines · One Thousand Gnomes

Quoted 3 lines
Very nice solution. We will check if this can cause any risk, both to kernel and user space. Using a new tty base to register with new cdevs may give us more chance to wait PROCESS quit/close. when total 256 tty used up, what we should do is still in discuss.
At that point you may want to look at how fuser works and create some kind of policy manager needs to kill problem tasks owning a device.
Or in theory there is no reason nowdays we can't go above 256 devices - in theory 8)

hi, Alan
Thanks for your nice comments.
In theory it can go over 256 devices, In fact, it will waste more memories. :) We really need a policy manager that works well.
FT guys may offer it one day. it's a little long time maybe. :(

When I was a student, I heard you leave from kernel. Welcome back to kernel, I didn't know that you are back. So sorry.

Quoted 3 lines

Alan

xinhui.pan

2014-07-24 11:03 UTC #11 lore raw

In reply to One Thousand Gnomes

Quoted 10 lines · One Thousand Gnomes

Quoted 3 lines
Very nice solution. We will check if this can cause any risk, both to kernel and user space. Using a new tty base to register with new cdevs may give us more chance to wait PROCESS quit/close. when total 256 tty used up, what we should do is still in discuss.
At that point you may want to look at how fuser works and create some kind of policy manager needs to kill problem tasks owning a device.
Or in theory there is no reason nowdays we can't go above 256 devices - in theory 8)

When I was a student, I heard you left from kernel. Welcome back to kernel, I didn't know that you are back. So sorry.

thanks,

xinhui

Quoted 3 lines

Alan

xinhui

author

2014-07-25 15:24 UTC #8 lore raw

In reply to Peter Hurley

hi, Peter

Quoted 33 lines · Peter Hurley

Hi Xinhui,
Quoted 23 lines · xinhui.pan
Quoted 12 lines · Peter Hurley
Quoted 10 lines · xinhui.pan
Quoted 7 lines · Greg KH
Quoted 1 line · pp
tty driver register its device and (D)init the cdevs again.
What driver does this with an "old" device, it should have created a new one, otherwise, as you have pointed out, it's a bug.
I can't agree more with you. we should not use "old" device.
Quoted 9 lines
This is a gsm driver problem. The GSM driver is reusing device indexes for still-open ttys.
The GSM driver uses a global table, gsm_mux[], to allocate device indexes but prematurely clears the table entry in gsm_mux_cleanup(). If instead, clearing the gsm_mux table entry were deferred to gsm_mux_free(), then device indexes would not be getting reused until after the last tty associated with the last gsm attach was closed.
Quoted 3 lines
Very nice solution. We will check if this can cause any risk, both to kernel and user space. Using a new tty base to register with new cdevs may give us more chance to wait PROCESS quit/close. when total 256 tty used up, what we should do is still in discuss.
I saw your patch for the use of gsm->num before gsm_activate_mux() has allocated the table entry; thanks for fixing that.

Quoted 8 lines

As for what to do if all the gsm_mux table entries are used: if the error is infrequent, I suggest simply returning an error which is what the driver does currently. Otherwise, a more dynamic allocation scheme may be required.
I did notice while reviewing the error handling that gsmld_open() will leak the entire composite ldisc data allocated by gsm_alloc_mux() if gsmld_attach_gsm() fails.

As for memory leak, I suggest move the codes that changing the index of gsm_mux[] into gsm_alloc_mux/gsm_free_mux. That makes things much easier to understand. And everything should be OK, including base = gsm->num << 6 :)

I think gsm_mux[] holds the gsms whcih we are able to use, not we are using. *able to use* includes *using*.

If you have any different opinions, could you point out my mistake?

thanks,

xinhui

Quoted 3 lines

Regards,
Peter Hurley