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

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

[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.
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 <xinhuiX.pan@intel.com>
---
 drivers/tty/tty_io.c       |   17 +++++++++++++++--
 include/linux/tty_driver.h |    1 +
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 3411071..7e79281 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -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);
diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
index 756a609..7294f1b 100644
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -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;
-- 
1.7.9.5

Re: [PATCH] tty/tty_io.c: make a check before reuse cdev

[Greg KH]

On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote:
> 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?

> 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

Re: [PATCH] tty/tty_io.c: make a check before reuse cdev

[xinhui.pan]

于 2014年07月21日 23:38, Greg KH 写道:
> On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote:
>> 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?

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. :)

> 
>> 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

> thanks,
> 
> greg k-h
> 

Re: [PATCH] tty/tty_io.c: make a check before reuse cdev

[Peter Hurley]

On 07/22/2014 07:52 AM, xinhui.pan wrote:
> 
> 于 2014年07月21日 23:38, Greg KH 写道:
>> On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote:
>>> 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?
> 
> 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. :)

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

>>> 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

Re: [PATCH] tty/tty_io.c: make a check before reuse cdev

[Greg KH]

On Tue, Jul 22, 2014 at 12:40:59PM -0400, Peter Hurley wrote:
> On 07/22/2014 07:52 AM, xinhui.pan wrote:
> > 
> > 于 2014年07月21日 23:38, Greg KH 写道:
> >> On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote:
> >>> 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?
> > 
> > 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. :)
> 
> Or a process could simply ignore SIGHUP, in which case /dev/gsmttyXX
> will not be closed until process termination.
> 
> >>> 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.

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

greg k-h

Re: [PATCH] tty/tty_io.c: make a check before reuse cdev

[xinhui.pan]

于 2014年07月23日 00:40, Peter Hurley 写道:
> On 07/22/2014 07:52 AM, xinhui.pan wrote:
>>
>> 于 2014年07月21日 23:38, Greg KH 写道:
>>> On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote:
>>>> 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?
>>
>> 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. :)
> 
> 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.


>>>> 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.
> 

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

> Regards,
> Peter Hurley
> 

Re: [PATCH] tty/tty_io.c: make a check before reuse cdev

[Peter Hurley]

Hi Xinhui,

On 07/23/2014 05:21 AM, xinhui.pan wrote:
> 于 2014年07月23日 00:40, Peter Hurley 写道:
>> On 07/22/2014 07:52 AM, xinhui.pan wrote:
>>> 于 2014年07月21日 23:38, Greg KH 写道:
>>>> On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote:

>>>>> 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.
>>
> 
> 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

Re: [PATCH] tty/tty_io.c: make a check before reuse cdev

[One Thousand Gnomes]

> 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

Re: [PATCH] tty/tty_io.c: make a check before reuse cdev

[xinhui.pan]

hi, Peter

于 2014年07月24日 00:04, Peter Hurley 写道:
> Hi Xinhui,
> 
> On 07/23/2014 05:21 AM, xinhui.pan wrote:
>> 于 2014年07月23日 00:40, Peter Hurley 写道:
>>> On 07/22/2014 07:52 AM, xinhui.pan wrote:
>>>> 于 2014年07月21日 23:38, Greg KH 写道:
>>>>> On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote:
> 
>>>>>> 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.
>>>
>>
>> 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.
>

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. :)

> 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

> Regards,
> Peter Hurley
> 

Re: [PATCH] tty/tty_io.c: make a check before reuse cdev

[xinhui.pan]

于 2014年07月24日 00:07, One Thousand Gnomes 写道:
>> 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.

> Alan
> 
> 

Re: [PATCH] tty/tty_io.c: make a check before reuse cdev

[xinhui.pan]

于 2014年07月24日 00:07, One Thousand Gnomes 写道:
>> 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 left from kernel.
Welcome back to kernel, I didn't know that you are back. So sorry.

thanks,

xinhui

> Alan
> 
> 

Re: [PATCH] tty/tty_io.c: make a check before reuse cdev

[xinhui]

hi, Peter

On 2014年07月24日 00:04, Peter Hurley wrote:
> Hi Xinhui,
> 
> On 07/23/2014 05:21 AM, xinhui.pan wrote:
>> 于 2014年07月23日 00:40, Peter Hurley 写道:
>>> On 07/22/2014 07:52 AM, xinhui.pan wrote:
>>>> 于 2014年07月21日 23:38, Greg KH 写道:
>>>>> On Mon, Jul 21, 2014 at 08:47:16PM +0800, pp wrote:
> 
>>>>>> 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.
>>>
>>
>> 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.
> 

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

> Regards,
> Peter Hurley
>