Russell King wrote:

>Hi,
>
>The existing serial.c contains a nice module use count bug which is easily
>triggerable.  Without anything connected to ttyS0, do:
>
> stty -clocal -F /dev/ttyS0
> stty -aF /dev/ttyS0
>
>Hit ^c, lsmod shows use count of -1.  Repeat to decrement further.
>
>Here's a patch that fixes this bogosity - please see the comment within
>the patch for the reason.
>
>Marcelo, please apply to both 2.4.
>Linus, please apply to 2.5 as a stop-gap until my new serial drivers are
>ready to be merged.
>
>Thanks.
>
>--- linux-orig/drivers/char/serial.c	Tue Nov 13 12:37:12 2001
>+++ linux/drivers/char/serial.c	Thu Nov 29 13:07:52 2001
>@@ -3133,6 +3133,10 @@
>  * enables interrupts for a serial port, linking in its async structure into
>  * the IRQ chain.   It also performs the serial-specific
>  * initialization for the tty structure.
>+ *
>+ * Note that on failure, we don't decrement the module use count - the tty
>+ * later will call rs_close, which will decrement it for us as long as
>+ * tty->driver_data is set non-NULL. --rmk
>  */
> static int rs_open(struct tty_struct *tty, struct file * filp)
> {
>@@ -3153,10 +3157,8 @@
> 	}
> 	tty->driver_data = info;
> 	info->tty = tty;
>-	if (serial_paranoia_check(info, tty->device, "rs_open")) {
>-		MOD_DEC_USE_COUNT;		
>+	if (serial_paranoia_check(info, tty->device, "rs_open"))
> 		return -ENODEV;
>-	}
> 
> #ifdef SERIAL_DEBUG_OPEN
> 	printk("rs_open %s%d, count = %d\n", tty->driver.name, info->line,
>@@ -3171,10 +3173,8 @@
> 	 */
> 	if (!tmp_buf) {
> 		page = get_zeroed_page(GFP_KERNEL);
>-		if (!page) {
>-			MOD_DEC_USE_COUNT;
>+		if (!page)
> 			return -ENOMEM;
>-		}
>

The current code on my system 2.5.0 looks like

                if (!page) {
                        MOD_DEC_USE_COUNT;
                        return -ENOMEM;
                }


I was wondering that if the open failed with something like this

    open(/dev/ttyS0, args) = -ENOMEM;

why would somebody call close on /dev/ttyS0? If you call close on
a descriptor that failed to open, it is *BAD* code. I am assuming
that the tty layer that talks to the serial driver calls rs_close().

The same thing applies to the code below. I think that the open routine
should instead set tty->driver_data to NULL upon failure.


Comments,
Balbir Singh.




>
> 		if (tmp_buf)
> 			free_page(page);
> 		else
>@@ -3188,7 +3188,6 @@
> 	    (info->flags & ASYNC_CLOSING)) {
> 		if (info->flags & ASYNC_CLOSING)
> 			interruptible_sleep_on(&info->close_wait);
>-		MOD_DEC_USE_COUNT;
> #ifdef SERIAL_DO_RESTART
> 		return ((info->flags & ASYNC_HUP_NOTIFY) ?
> 			-EAGAIN : -ERESTARTSYS);
>@@ -3201,10 +3200,8 @@
> 	 * Start up serial port
> 	 */
> 	retval = startup(info);
>-	if (retval) {
>-		MOD_DEC_USE_COUNT;
>+	if (retval)
> 		return retval;
>-	}
> 
> 	retval = block_til_ready(tty, filp, info);
> 	if (retval) {
>@@ -3212,7 +3209,6 @@
> 		printk("rs_open returning after block_til_ready with %d\n",
> 		       retval);
> #endif
>-		MOD_DEC_USE_COUNT;
> 		return retval;
> 	}
> 
>
>
>--
>Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
>             http://www.arm.linux.org.uk/personal/aboutme.html
>
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>