tty: cleanup release_mem
PATCH 1/1 tty: cleanup release_mem
release_mem contains two copies of exactly the same code. Refactor these into a new helper, release_tty. The only change in behaviour is that the driver reference count is now decremented after the master tty has been freed instead of before.
drivers/char/tty_io.c +15 -28
@@ -2105,7 +2105,7 @@ }/** -* release_mem - release tty structure memory +* release_tty - release tty structure memory * * Releases memory associated with a tty structure, and clears out the * driver table slots. This function is called when a device is no longer
@@ -2117,37 +2117,14 @@* of ttys that the driver keeps. * FIXME: should we require tty_mutex is held here ?? */ - -static void release_mem (struct tty_struct *tty,int idx) +static void release_tty (struct tty_struct *tty,int idx) { -struct tty_struct *o_tty; -struct ktermios *tp;int devpts = tty->driver->flags &TTY_DRIVER_DEVPTS_MEM ; - -if ((o_tty = tty->link) !=NULL ) { -if (!devpts) - o_tty->driver->ttys[idx] =NULL ; -if (o_tty->driver->flags &TTY_DRIVER_RESET_TERMIOS ) { - tp = o_tty->termios; -if (!devpts) - o_tty->driver->termios[idx] =NULL ; -kfree (tp); - - tp = o_tty->termios_locked; -if (!devpts) - o_tty->driver->termios_locked[idx] =NULL ; -kfree (tp); - } - o_tty->magic =0 ; - o_tty->driver->refcount--; -file_list_lock (); -list_del_init (&o_tty->tty_files); -file_list_unlock (); -free_tty_struct (o_tty); - } +struct ktermios *tp;if (!devpts) tty->driver->ttys[idx] =NULL ; +if (tty->driver->flags &TTY_DRIVER_RESET_TERMIOS ) { tp = tty->termios;if (!devpts)
@@ -2160,15 +2137,25 @@kfree (tp); } + tty->magic =0 ; tty->driver->refcount--; +file_list_lock ();list_del_init (&tty->tty_files);file_list_unlock (); -module_put (tty->driver->owner); +free_tty_struct (tty); } +static void release_mem (struct tty_struct *tty,int idx) +{ +if (tty->link) +release_tty (tty->link, idx); +release_tty (tty, idx); +module_put (tty->driver->owner); +} +/* * Even releasing the tty structures is a tricky business.. We have * to be very careful that the structures are all released at the
Quoted 7 lines · Christoph Hellwig
release_memcontains two copies of exactly the same code. Refactor these into a new helper,release_tty. The only change in behaviour is that the driver reference count is now decremented after the master tty has been freed instead of before.Signed-off-by Christoph Hellwig <[email protected]>
Please propogate the mutex_ FIXME comment to both functions as it may well need to cover tty->link
Quoted 7 lines · Alan Cox
Quoted 6 lines · Christoph Hellwig
release_memcontains two copies of exactly the same code. Refactor these into a new helper,release_tty. The only change in behaviour is that the driver reference count is now decremented after the master tty has been freed instead of before.
Quoted 6 lines
Quoted 1 line
Signed-off-by Christoph Hellwig <[email protected]>Acked-by Alan Cox <[email protected]>Please propogate the
mutex_FIXME comment to both functions as it may well need to covertty->link
Okay. Now that we get into the details I've also added some renaming, release_mem becomes release_tty and the new factored out function is release_one_tty. The difference is documented in the kdoc comments.
drivers/char/tty_io.c +35 -36
@@ -154,7 +154,7 @@int tty_ioctl (struct inode * inode,struct file * file,unsigned int cmd,unsigned long arg);static int tty_fasync (int fd,struct file * filp,int on); -static void release_mem (struct tty_struct *tty,int idx); +static void release_tty (struct tty_struct *tty,int idx);/** * alloc_tty_struct - allocate a tty object
@@ -2003,7 +2003,7 @@/* * All structures have been allocated, so now we install them. -* Failures after this point use release_mem to clean up, so +* Failures after this point use release_tty to clean up, so * there's no need to null out the local pointers. */ if (!(driver->flags &TTY_DRIVER_DEVPTS_MEM )) {
@@ -2024,8 +2024,8 @@/* * Structures all installed ... call the ldisc open routines. -* If we fail here just call release_mem to clean up. No need -* to decrement the use counts, as release_mem doesn't care. +* If we fail here just call release_tty to clean up. No need +* to decrement the use counts, as release_tty doesn't care. */ if (tty->ldisc.open) {
@@ -2095,17 +2095,17 @@ retval = -ENOMEM ; goto end_init; -/* call the tty release_mem routine to clean out this slot */ +/* call the tty release_tty routine to clean out this slot */ release_mem_out:if (printk_ratelimit ())printk (KERN_INFO "init_dev: ldisc open failed, " "clearing slot %d\n" , idx); -release_mem (tty, idx); +release_tty (tty, idx); goto end_init; }/** -* release_mem - release tty structure memory +* release_one_tty - release tty structure memory * * Releases memory associated with a tty structure, and clears out the * driver table slots. This function is called when a device is no longer
@@ -2117,37 +2117,14 @@* of ttys that the driver keeps. * FIXME: should we require tty_mutex is held here ?? */ - -static void release_mem (struct tty_struct *tty,int idx) +static void release_one_tty (struct tty_struct *tty,int idx) { -struct tty_struct *o_tty; -struct ktermios *tp;int devpts = tty->driver->flags &TTY_DRIVER_DEVPTS_MEM ; - -if ((o_tty = tty->link) !=NULL ) { -if (!devpts) - o_tty->driver->ttys[idx] =NULL ; -if (o_tty->driver->flags &TTY_DRIVER_RESET_TERMIOS ) { - tp = o_tty->termios; -if (!devpts) - o_tty->driver->termios[idx] =NULL ; -kfree (tp); - - tp = o_tty->termios_locked; -if (!devpts) - o_tty->driver->termios_locked[idx] =NULL ; -kfree (tp); - } - o_tty->magic =0 ; - o_tty->driver->refcount--; -file_list_lock (); -list_del_init (&o_tty->tty_files); -file_list_unlock (); -free_tty_struct (o_tty); - } +struct ktermios *tp;if (!devpts) tty->driver->ttys[idx] =NULL ; +if (tty->driver->flags &TTY_DRIVER_RESET_TERMIOS ) { tp = tty->termios;if (!devpts)
@@ -2160,15 +2137,37 @@kfree (tp); } + tty->magic =0 ; tty->driver->refcount--; +file_list_lock ();list_del_init (&tty->tty_files);file_list_unlock (); -module_put (tty->driver->owner); +free_tty_struct (tty); } +/** +* release_one_tty - release tty structure memory +* +* Release both @tty and a possible linked partner (think pty pair), +* and decrement the refcount of the backing module. +* +* Locking: +* tty_mutex - sometimes only +* takes the file list lock internally when working on the list +* of ttys that the driver keeps. +* FIXME: should we require tty_mutex is held here ?? +*/ +static void release_tty (struct tty_struct *tty,int idx) +{ +if (tty->link) +release_one_tty (tty->link, idx); +release_one_tty (tty, idx); +module_put (tty->driver->owner); +} +/* * Even releasing the tty structures is a tricky business.. We have * to be very careful that the structures are all released at the
@@ -2436,10 +2435,10 @@tty_set_termios_ldisc (o_tty,N_TTY ); }/* -* The release_mem function takes care of the details of clearing +* The release_tty function takes care of the details of clearing * the slots and preserving the termios structure. */ -release_mem (tty, idx); +release_tty (tty, idx);#ifdef CONFIG_UNIX98_PTYS /* Make this pty number available for reallocation */
Alan Cox
Acked-byQuoted 6 lines · Christoph Hellwig
Okay. Now that we get into the details I've also added some renaming,
release_membecomesrelease_ttyand the new factored out function isrelease_one_tty. The difference is documented in the kdoc comments.Signed-off-by Christoph Hellwig <[email protected]>
Quoted 8 lines · On Mon, 29 Jan 2007 14:06:04 -0500 Alan Cox <[email protected]> wrote:
Quoted 6 lines · Christoph Hellwig
Okay. Now that we get into the details I've also added some renaming,
release_membecomesrelease_ttyand the new factored out function isrelease_one_tty. The difference is documented in the kdoc comments.Signed-off-by Christoph Hellwig <[email protected]>Acked-by Alan Cox <[email protected]>
VFS: Mounted root (ext3 filesystem) readonly.
Freeing unused kernel memory: 300k freed
Write protecting the kernel read-only data: 920k
BUG: unable to handle kernel paging request at virtual address 6b6b6bdf printing eip: c02a91ec *pde = 00000000 Oops: 0000 [#1] SMP last sysfs file: /block/hdc/range Modules linked in: CPU: 0 EIP: 0060:[<c02a91ec>] Not tainted VLI EFLAGS: 00010286 (2.6.20-rc6-mm3 #1)
EIP is at release_tty+0x2c/0x40
eax: 6b6b6b6b ebx: f71c860c ecx: c0175e24 edx: 00000000
esi: 00000000 edi: 00000001 ebp: c222fe88 esp: c222fe80
ds: 007b es: 007b fs: 00d8 gs: 0033 ss: 0068
Process init (pid: 1, ti=c222e000 task=c222cae0 task.ti=c222e000)
Stack: f71c860c 00000000 c222ff3c c02aac29 00000000 c22e453c c222fea0 00000006
c222fec4 002ad15c 00000000 00000001 00000000 00000000 00000000 00000001
c06650a0 000001d8 00000000 c222ff24 c013d53f c0178d34 f7c383c4 c222ff20
Call Trace:
[<c0103f7a>] show_trace_log_lvl+0x1a/0x30
[<c0104038>] show_stack_log_lvl+0xa8/0xe0
[<c0104259>] show_registers+0x1e9/0x2f0
[<c010447a>] die+0x11a/0x250
[<c0115580>] do_page_fault+0x2f0/0x5f0
[<c03d74b4>] error_code+0x7c/0x84
[<c02aac29>] release_dev+0x519/0x750
[<c02aae72>] tty_release+0x12/0x20
[<c017b8e4>] __fput+0xb4/0x180
[<c017bb02>] fput+0x22/0x40
[<c0178db7>] filp_close+0x47/0x70
[<c0179f4d>] sys_close+0x6d/0xc0
[<c0102edc>] sysenter_past_esp+0x5d/0x99
=======================
Code: 89 e5 83 ec 08 89 1c 24 89 c3 89 74 24 04 89 d6 8b 80 fc 00 00 00 85 c0 74 05 e8 00 ff ff ff 89 f2 89 d8 e8 f7 fe ff ff 8b 43 04 <8b> 40 74 e8 8c b3 e9 ff 8b 1c 24 8b 74 24 04 89 ec 5d c3 90 55
EIP: [<c02a91ec>] release_tty+0x2c/0x40 SS:ESP 0068:c222fe80
Kernel panic - not syncing: Attempted to kill init!Quoted 1 line · Andrew Morton
BUG: unable to handle kernel paging request at virtual address 6b6b6bdf
Use after free. The new code does module_put() _after_ free_tty_struct() which is obviously wrong.