Change it so that we call the deinit functions at one place at the end
of the function (by gotos). And while at it use some sane label names.
This is a preparation for the deinitialization of tty in the next
patch.
Signed-off-by: Jiri Slaby <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: Julian Anastasov <[email protected]>
---
This is not tested yet, I'm building a kernel to test it a bit, but it
will take some time. If you could too to check if your problem
dissapears, it would be great.
P.S. the drivers/char/tty_<TAB><TAB> habit sucks :).
^^^^
drivers/tty/tty_io.c | 23 ++++++++++++-----------
1 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 936a4ea..6312cc3 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1391,16 +1391,15 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
return ERR_PTR(-ENODEV);
tty = alloc_tty_struct();
- if (!tty)
- goto fail_no_mem;
+ if (!tty) {
+ retval = -ENOMEM;
+ goto err_module_put;
+ }
initialize_tty_struct(tty, driver, idx);
retval = tty_driver_install_tty(driver, tty);
- if (retval < 0) {
- free_tty_struct(tty);
- module_put(driver->owner);
- return ERR_PTR(retval);
- }
+ if (retval < 0)
+ goto err_free_tty;
/*
* Structures all installed ... call the ldisc open routines.
@@ -1409,15 +1408,17 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
*/
retval = tty_ldisc_setup(tty, tty->link);
if (retval)
- goto release_mem_out;
+ goto err_release_tty;
return tty;
-fail_no_mem:
+err_free_tty:
+ free_tty_struct(tty);
+err_module_put:
module_put(driver->owner);
- return ERR_PTR(-ENOMEM);
+ return ERR_PTR(retval);
/* call the tty release_tty routine to clean out this slot */
-release_mem_out:
+err_release_tty:
if (printk_ratelimit())
printk(KERN_INFO "tty_init_dev: ldisc open failed, "
"clearing slot %d\n", idx);
--
1.7.4.1
Change it so that we call the deinit functions at one place at the end
of the function (by gotos). And while at it use some sane label names.
This is a preparation for the deinitialization of tty in the next
patch.
Signed-off-by: Jiri Slaby <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: Julian Anastasov <[email protected]>
---
drivers/tty/pty.c | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 2107747..f511918 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -295,8 +295,8 @@ static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
return -ENOMEM;
if (!try_module_get(driver->other->owner)) {
/* This cannot in fact currently happen */
- free_tty_struct(o_tty);
- return -ENOMEM;
+ retval = -ENOMEM;
+ goto err_free_tty;
}
initialize_tty_struct(o_tty, driver->other, idx);
@@ -304,13 +304,11 @@ static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
the easy way .. */
retval = tty_init_termios(tty);
if (retval)
- goto free_mem_out;
+ goto err_module_put;
retval = tty_init_termios(o_tty);
- if (retval) {
- tty_free_termios(tty);
- goto free_mem_out;
- }
+ if (retval)
+ goto err_free_termios;
/*
* Everything allocated ... set up the o_tty structure.
@@ -327,10 +325,13 @@ static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
tty->count++;
driver->ttys[idx] = tty;
return 0;
-free_mem_out:
+err_free_termios:
+ tty_free_termios(tty);
+err_module_put:
module_put(o_tty->driver->owner);
+err_free_tty:
free_tty_struct(o_tty);
- return -ENOMEM;
+ return retval;
}
static int pty_bsd_ioctl(struct tty_struct *tty,
--
1.7.4.1
Introduce deinitialize_tty_struct which should be called after
initialize_tty_struct and before successfull tty_ldisc_setup.
It calls tty_ldisc_deinit which is opposite of tty_ldisc_init. It only
puts a reference to ldisc and assigns NULL to tty->ldisc.
It will be used to shut down ldisc when tty_release cannot be called
yet.
Signed-off-by: Jiri Slaby <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: Julian Anastasov <[email protected]>
---
drivers/tty/tty_io.c | 14 ++++++++++++++
drivers/tty/tty_ldisc.c | 13 +++++++++++++
include/linux/tty.h | 2 ++
3 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 6312cc3..a4d47fa 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2889,6 +2889,20 @@ void initialize_tty_struct(struct tty_struct *tty,
}
/**
+ * deinitialize_tty_struct
+ * @tty: tty to deinitialize
+ *
+ * This subroutine deinitializes a tty structure that has been newly
+ * allocated but tty_release cannot be called on that yet.
+ *
+ * Locking: none - tty in question must not be exposed at this point
+ */
+void deinitialize_tty_struct(struct tty_struct *tty)
+{
+ tty_ldisc_deinit(tty);
+}
+
+/**
* tty_put_char - write one character to a tty
* @tty: tty
* @ch: character
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 0fc564a..525a901 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -956,6 +956,19 @@ void tty_ldisc_init(struct tty_struct *tty)
tty_ldisc_assign(tty, ld);
}
+/**
+ * tty_ldisc_init - ldisc cleanup for new tty
+ * @tty: tty that was allocated recently
+ *
+ * The tty structure must not becompletely set up (tty_ldisc_setup) when
+ * this call is made.
+ */
+void tty_ldisc_deinit(struct tty_struct *tty)
+{
+ put_ldisc(tty->ldisc);
+ tty_ldisc_assign(tty, NULL);
+}
+
void tty_ldisc_begin(void)
{
/* Setup the default TTY line discipline. */
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 4e53d46..a39e2b1 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -472,6 +472,7 @@ extern int tty_add_file(struct tty_struct *tty, struct file *file);
extern void free_tty_struct(struct tty_struct *tty);
extern void initialize_tty_struct(struct tty_struct *tty,
struct tty_driver *driver, int idx);
+extern void deinitialize_tty_struct(struct tty_struct *tty);
extern struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
int first_ok);
extern int tty_release(struct inode *inode, struct file *filp);
@@ -525,6 +526,7 @@ extern int tty_set_ldisc(struct tty_struct *tty, int ldisc);
extern int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty);
extern void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty);
extern void tty_ldisc_init(struct tty_struct *tty);
+extern void tty_ldisc_deinit(struct tty_struct *tty);
extern void tty_ldisc_begin(void);
/* This last one is just for the tty layer internals and shouldn't be used elsewhere */
extern void tty_ldisc_enable(struct tty_struct *tty);
--
1.7.4.1
Change it so that we call the deinit functions at one place at the end
of the function (by gotos). And while at it use some sane label names.
This is a preparation for the deinitialization of tty in the next
patch.
Signed-off-by: Jiri Slaby <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: Julian Anastasov <[email protected]>
---
drivers/tty/pty.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index f511918..b25d6c4 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -560,20 +560,19 @@ static int pty_unix98_install(struct tty_driver *driver, struct tty_struct *tty)
return -ENOMEM;
if (!try_module_get(driver->other->owner)) {
/* This cannot in fact currently happen */
- free_tty_struct(o_tty);
- return -ENOMEM;
+ goto err_free_tty;
}
initialize_tty_struct(o_tty, driver->other, idx);
tty->termios = kzalloc(sizeof(struct ktermios[2]), GFP_KERNEL);
if (tty->termios == NULL)
- goto free_mem_out;
+ goto err_free_mem;
*tty->termios = driver->init_termios;
tty->termios_locked = tty->termios + 1;
o_tty->termios = kzalloc(sizeof(struct ktermios[2]), GFP_KERNEL);
if (o_tty->termios == NULL)
- goto free_mem_out;
+ goto err_free_mem;
*o_tty->termios = driver->other->init_termios;
o_tty->termios_locked = o_tty->termios + 1;
@@ -592,11 +591,12 @@ static int pty_unix98_install(struct tty_driver *driver, struct tty_struct *tty)
tty->count++;
pty_count++;
return 0;
-free_mem_out:
+err_free_mem:
kfree(o_tty->termios);
+ kfree(tty->termios);
module_put(o_tty->driver->owner);
+err_free_tty:
free_tty_struct(o_tty);
- kfree(tty->termios);
return -ENOMEM;
}
--
1.7.4.1
Used the newly introduced deinitialize_tty_struct to properly shut
down ldisc.
It is intended to fix the Julian's reported problem. He reports that
kmemleak checker warns about memory leak:
unreferenced object 0xc0e19860 (size 8):
comm cat, pid 1226, jiffies 4294919464 (age 287.476s)
hex dump (first 8 bytes):
44 de 2d c1 01 00 00 00 D.-.....
backtrace:
[<c1065a74>] create_object+0x109/0x1ad
[<c1063d2b>] kmem_cache_alloc+0x60/0x68
[<c113505c>] tty_ldisc_get+0x54/0x76
[<c11358c9>] tty_ldisc_init+0xa/0x20
[<c1130ab4>] initialize_tty_struct+0x2d/0x1ac
[<c1130c8c>] tty_init_dev+0x59/0x10d
[<c113136d>] tty_open+0x24a/0x3a2
...
Signed-off-by: Jiri Slaby <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Alan Cox <[email protected]>
Reported-by: Julian Anastasov <[email protected]>
---
drivers/tty/pty.c | 6 ++++--
drivers/tty/tty_io.c | 5 +++--
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index b25d6c4..21bddf3 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -304,7 +304,7 @@ static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
the easy way .. */
retval = tty_init_termios(tty);
if (retval)
- goto err_module_put;
+ goto err_deinit_tty;
retval = tty_init_termios(o_tty);
if (retval)
@@ -327,7 +327,8 @@ static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
return 0;
err_free_termios:
tty_free_termios(tty);
-err_module_put:
+err_deinit_tty:
+ deinitialize_tty_struct(o_tty);
module_put(o_tty->driver->owner);
err_free_tty:
free_tty_struct(o_tty);
@@ -592,6 +593,7 @@ static int pty_unix98_install(struct tty_driver *driver, struct tty_struct *tty)
pty_count++;
return 0;
err_free_mem:
+ deinitialize_tty_struct(o_tty);
kfree(o_tty->termios);
kfree(tty->termios);
module_put(o_tty->driver->owner);
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index a4d47fa..48eb1aa 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1399,7 +1399,7 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
retval = tty_driver_install_tty(driver, tty);
if (retval < 0)
- goto err_free_tty;
+ goto err_deinit_tty;
/*
* Structures all installed ... call the ldisc open routines.
@@ -1411,7 +1411,8 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
goto err_release_tty;
return tty;
-err_free_tty:
+err_deinit_tty:
+ deinitialize_tty_struct(tty);
free_tty_struct(tty);
err_module_put:
module_put(driver->owner);
--
1.7.4.1
When tty_add_file fails we omit to clean up. Fix that by calling
tty_release appropriatelly.
Signed-off-by: Jiri Slaby <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Alan Cox <[email protected]>
---
drivers/tty/tty_io.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 48eb1aa..2ec3df9 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1894,6 +1894,7 @@ got_driver:
retval = tty_add_file(tty, filp);
if (retval) {
tty_unlock();
+ tty_release(inode, filp);
return retval;
}
--
1.7.4.1
Hello,
On Wed, 23 Mar 2011, Jiri Slaby wrote:
> Change it so that we call the deinit functions at one place at the end
> of the function (by gotos). And while at it use some sane label names.
>
> This is a preparation for the deinitialization of tty in the next
> patch.
>
> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Alan Cox <[email protected]>
> Cc: Julian Anastasov <[email protected]>
> ---
>
> This is not tested yet, I'm building a kernel to test it a bit, but it
> will take some time. If you could too to check if your problem
> dissapears, it would be great.
Thanks! Patches 1-5 work for me, I backported them
to 2.6.33.8 for the test.
>
> P.S. the drivers/char/tty_<TAB><TAB> habit sucks :).
> ^^^^
>
> drivers/tty/tty_io.c | 23 ++++++++++++-----------
> 1 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 936a4ea..6312cc3 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1391,16 +1391,15 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
> return ERR_PTR(-ENODEV);
>
> tty = alloc_tty_struct();
> - if (!tty)
> - goto fail_no_mem;
> + if (!tty) {
> + retval = -ENOMEM;
> + goto err_module_put;
> + }
> initialize_tty_struct(tty, driver, idx);
>
> retval = tty_driver_install_tty(driver, tty);
> - if (retval < 0) {
> - free_tty_struct(tty);
> - module_put(driver->owner);
> - return ERR_PTR(retval);
> - }
> + if (retval < 0)
> + goto err_free_tty;
>
> /*
> * Structures all installed ... call the ldisc open routines.
> @@ -1409,15 +1408,17 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx,
> */
> retval = tty_ldisc_setup(tty, tty->link);
> if (retval)
> - goto release_mem_out;
> + goto err_release_tty;
> return tty;
>
> -fail_no_mem:
> +err_free_tty:
> + free_tty_struct(tty);
> +err_module_put:
> module_put(driver->owner);
> - return ERR_PTR(-ENOMEM);
> + return ERR_PTR(retval);
>
> /* call the tty release_tty routine to clean out this slot */
> -release_mem_out:
> +err_release_tty:
> if (printk_ratelimit())
> printk(KERN_INFO "tty_init_dev: ldisc open failed, "
> "clearing slot %d\n", idx);
> --
> 1.7.4.1
Regards
--
Julian Anastasov <[email protected]>
On 03/23/2011 09:53 PM, Julian Anastasov wrote:
>
> Hello,
>
> On Wed, 23 Mar 2011, Jiri Slaby wrote:
>
>> Change it so that we call the deinit functions at one place at the end
>> of the function (by gotos). And while at it use some sane label names.
>>
>> This is a preparation for the deinitialization of tty in the next
>> patch.
>>
>> Signed-off-by: Jiri Slaby <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Alan Cox <[email protected]>
>> Cc: Julian Anastasov <[email protected]>
>> ---
>>
>> This is not tested yet, I'm building a kernel to test it a bit, but it
>> will take some time. If you could too to check if your problem
>> dissapears, it would be great.
>
> Thanks! Patches 1-5 work for me, I backported them
> to 2.6.33.8 for the test.
Thanks for testing. I see no regressions either.
Greg, will you take it?
regards,
--
js
suse labs
On Tue, Mar 29, 2011 at 02:17:42PM +0200, Jiri Slaby wrote:
> On 03/23/2011 09:53 PM, Julian Anastasov wrote:
> >
> > Hello,
> >
> > On Wed, 23 Mar 2011, Jiri Slaby wrote:
> >
> >> Change it so that we call the deinit functions at one place at the end
> >> of the function (by gotos). And while at it use some sane label names.
> >>
> >> This is a preparation for the deinitialization of tty in the next
> >> patch.
> >>
> >> Signed-off-by: Jiri Slaby <[email protected]>
> >> Cc: Greg Kroah-Hartman <[email protected]>
> >> Cc: Alan Cox <[email protected]>
> >> Cc: Julian Anastasov <[email protected]>
> >> ---
> >>
> >> This is not tested yet, I'm building a kernel to test it a bit, but it
> >> will take some time. If you could too to check if your problem
> >> dissapears, it would be great.
> >
> > Thanks! Patches 1-5 work for me, I backported them
> > to 2.6.33.8 for the test.
>
> Thanks for testing. I see no regressions either.
>
> Greg, will you take it?
Yes I will queue it up in a week or so, thanks.
greg k-h