Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Balbir Singh]

>Err,
>	close(-ENOMEM);

>What's that going to close?  Hint: you _can't_ close
a descriptor that
>failed to open, since you don't have a descriptor to
close.  You can
>only try to close an error code, but that's not going
to make it anywhere
>near the kernel driver level.


Let me make it clearer to you,

lets say I call rs_open() on /dev/ttyS0 and if it
fails then I should not call rs_close() after a failed
rs_open().

I hope this is clear now.





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

>Here's an example why that'd be real bad:

>1. process A opens /dev/ttyS0 as a normal device. 
>This initialises
>   tty->driver_data.
>2. process B tries to open /dev/cua0
>3. process B fails with -EBUSY since the normal
>device is open and active
>   (see block_til_ready)
>4. since rs_open failed, we set tty->driver_data to
>be NULL (note that this
>   is the same tty device pointer as (1) above.
>5. process A writes to /dev/ttyS0
>6. rs_write does the following:

>        struct async_struct *info = (struct
async_struct *)tty->driver_data;

>7. Oops.

Lets see what happens with your approach

1. I call rs_open(), it fails, ref_count set to 1

2. I am sane enough not to call rs_close() on the
device which failed to open with rs_open the first
time, count is set to 1, driver never unloads.

I do not have access to block_til_ready currently.
But, I will see that function and revert with more
comments.

Comments,
Balbir Singh.


--
Russell King (rmk@arm.linux.org.uk)                The
developer of ARM Linux
            
http://www.arm.linux.org.uk/personal/aboutme.html



__________________________________________________
Do You Yahoo!?
Yahoo! GeoCities - quick and easy web site hosting, just $8.95/month.
http://geocities.yahoo.com/ps/info1

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Russell King]

On Thu, Nov 29, 2001 at 08:06:37AM -0800, Balbir Singh wrote:
> Let me make it clearer to you,
> 
> lets say I call rs_open() on /dev/ttyS0 and if it
> fails then I should not call rs_close() after a failed
> rs_open().

You don't call rs_open.  The tty layer does that for you.  The tty layer
also cleans up on close by calling the driver specific close function.

Yes I agree with you that it might not, but that is a 2.5 kernel issue,
not a 2.4 "lets do a massive change" issue.  The tty layer is complex
and messy, and we shouldn't go around randomly changing it in 2.4.

> Lets see what happens with your approach
> 
> 1. I call rs_open(), it fails, ref_count set to 1

Ok, so you're poking around in kernel code calling kernel functions that
were previously declared static and not visible to anything but the tty
layer.  That immediately makes your example invalid because you're not
following the rules that the tty layers lays down for opening tty devices.

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[BALBIR SINGH]

[-- Attachment #1: Type: text/plain, Size: 2089 bytes --]



Russell King wrote:

>On Thu, Nov 29, 2001 at 08:06:37AM -0800, Balbir Singh wrote:
>
>>Let me make it clearer to you,
>>
>>lets say I call rs_open() on /dev/ttyS0 and if it
>>fails then I should not call rs_close() after a failed
>>rs_open().
>>
>
>You don't call rs_open.  The tty layer does that for you.  The tty layer
>also cleans up on close by calling the driver specific close function.
>
>Yes I agree with you that it might not, but that is a 2.5 kernel issue,
>not a 2.4 "lets do a massive change" issue.  The tty layer is complex
>and messy, and we shouldn't go around randomly changing it in 2.4.
>
>>Lets see what happens with your approach
>>
>>1. I call rs_open(), it fails, ref_count set to 1
>>
>
>Ok, so you're poking around in kernel code calling kernel functions that
>were previously declared static and not visible to anything but the tty
>layer.  That immediately makes your example invalid because you're not
>following the rules that the tty layers lays down for opening tty devices.
>

What I mean by I call rs_open() is that the person who writes the tty layer
should not call close() after open() fails, I cannot directly invoke rs_open()
and rs_close(), I KNOW that.

If I were to define an API like rs_open(), I would expect the person who
uses these API (rs_*) in our case the tty layer, to call rs_close()
only if rs_open() is successfull. I do not want to leave a dependency
that rs_close() SHOULD BE called even if rs_open() failed.

As you say this fix works for 2.4, Since we are into 2.5, I hope
that it will be fixed in a better manner in 2.5. I have some suggestions,
ideas for the serial driver in 2.5, are u willing to discuss them?

Balbir

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



[-- Attachment #2: InterScan_Disclaimer.txt --]
[-- Type: text/plain, Size: 855 bytes --]

-------------------------------------------------------------------------------------------------------------------------
Information transmitted by this E-MAIL is proprietary to Wipro and/or its Customers and
is intended for use only by the individual or entity to which it is
addressed, and may contain information that is privileged, confidential or
exempt from disclosure under applicable law. If you are not the intended
recipient or it appears that this mail has been forwarded to you without
proper authority, you are notified that any use or dissemination of this
information in any manner is strictly prohibited. In such cases, please
notify us immediately at mailto:mailadmin@wipro.com and delete this mail
from your records.
----------------------------------------------------------------------------------------------------------------------

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Russell King]

On Fri, Nov 30, 2001 at 09:55:49AM +0530, BALBIR SINGH wrote:
> What I mean by I call rs_open() is that the person who writes the tty layer
> should not call close() after open() fails, I cannot directly invoke rs_open()
> and rs_close(), I KNOW that.

We can't change the tty layer in 2.4 at this point.  We can do it in 2.5.
It requires an audit of all drivers though.  I believe we're going over
ground covered earlier in this thread now.

> I have some suggestions, ideas for the serial driver in 2.5, are u willing
> to discuss them?

Please look at the cvs stuff at:

   :pserver:cvs@pubcvs.arm.linux.org.uk:/mnt/src/cvsroot, module serial
   (no password)

Thanks.

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Mike Fedyk]

On Fri, Nov 30, 2001 at 09:55:49AM +0530, BALBIR SINGH wrote:
> As you say this fix works for 2.4, Since we are into 2.5, I hope
> that it will be fixed in a better manner in 2.5. I have some suggestions,
> ideas for the serial driver in 2.5, are u willing to discuss them?
> 

Would it not be better if the current code in 2.5 had the fix from 2.4 until
the entire layer is audited?

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Russell King]

On Fri, Nov 30, 2001 at 02:19:20PM -0800, Mike Fedyk wrote:
> On Fri, Nov 30, 2001 at 09:55:49AM +0530, BALBIR SINGH wrote:
> > As you say this fix works for 2.4, Since we are into 2.5, I hope
> > that it will be fixed in a better manner in 2.5. I have some suggestions,
> > ideas for the serial driver in 2.5, are u willing to discuss them?
> > 
> 
> Would it not be better if the current code in 2.5 had the fix from 2.4 until
> the entire layer is audited?

That was my original intention.  IIRC, James Simmons has said he's rewriting
the tty layer anyway in 2.5.

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Mike Fedyk]

On Fri, Nov 30, 2001 at 11:06:34PM +0000, Russell King wrote:
> On Fri, Nov 30, 2001 at 02:19:20PM -0800, Mike Fedyk wrote:
> > On Fri, Nov 30, 2001 at 09:55:49AM +0530, BALBIR SINGH wrote:
> > > As you say this fix works for 2.4, Since we are into 2.5, I hope
> > > that it will be fixed in a better manner in 2.5. I have some suggestions,
> > > ideas for the serial driver in 2.5, are u willing to discuss them?
> > > 
> > 
> > Would it not be better if the current code in 2.5 had the fix from 2.4 until
> > the entire layer is audited?
> 
> That was my original intention.  IIRC, James Simmons has said he's rewriting
> the tty layer anyway in 2.5.
> 

ACK.

I'm just looping on the issue. ;)

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Jeff Randall]

Balbir Singh wrote:
> >Err,
> >	close(-ENOMEM);
> 
> >What's that going to close?  Hint: you _can't_ close
> a descriptor that
> >failed to open, since you don't have a descriptor to
> close.  You can
> >only try to close an error code, but that's not going
> to make it anywhere
> >near the kernel driver level.
> 
> Let me make it clearer to you,
> 
> lets say I call rs_open() on /dev/ttyS0 and if it
> fails then I should not call rs_close() after a failed
> rs_open().
> 
> I hope this is clear now.

All of the other UNIX variants I've dealth with behave that way.
However, you cannot just make that change without having some means
of identifying that behavior change because all of the linux
serial drivers have been written to assume that their close()
will be called even after their open() has failed.

I'm not opposed to such a change in behavior, but at least be
sure that it's somehow identifiable (kernel version, a define
set in a header, etc) so that the 3rd party drivers have a means
to identify the change.

Redhat 7.1 included that behavior change in the kernel they shipped
and it caused no end of problems for those of us that were doing
serial drivers since there was no way to easily identify that the
patch had been included.


-- 
randall+lkml@uph.com

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Russell King]

On Thu, Nov 29, 2001 at 12:03:07PM -0600, Jeff Randall wrote:
> All of the other UNIX variants I've dealth with behave that way.
> However, you cannot just make that change without having some means
> of identifying that behavior change because all of the linux
> serial drivers have been written to assume that their close()
> will be called even after their open() has failed.

It's not only serial drivers, its everything that is a tty driver.
I believe that auditing and fixing that lot in 2.4 just isn't going
to happen - it's supposed to be a stable kernel after all.

> I'm not opposed to such a change in behavior, but at least be
> sure that it's somehow identifiable (kernel version, a define
> set in a header, etc) so that the 3rd party drivers have a means
> to identify the change.

eg, #if KERNEL_VERSION >= LINUX_VERSION(2,5,0)

> Redhat 7.1 included that behavior change in the kernel they shipped
> and it caused no end of problems for those of us that were doing
> serial drivers since there was no way to easily identify that the
> patch had been included.

The change which adds the MOD_DEC_USE_COUNT stuff is bogus, and it
shouldn't have been made.  (I'm assuming this is what you're talking
about).

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Jeff Randall]

Russell King wrote:
> On Thu, Nov 29, 2001 at 12:03:07PM -0600, Jeff Randall wrote:
> > All of the other UNIX variants I've dealth with behave that way.
> > However, you cannot just make that change without having some means
> > of identifying that behavior change because all of the linux
> > serial drivers have been written to assume that their close()
> > will be called even after their open() has failed.
> 
> It's not only serial drivers, its everything that is a tty driver.
> I believe that auditing and fixing that lot in 2.4 just isn't going
> to happen - it's supposed to be a stable kernel after all.
>
> > I'm not opposed to such a change in behavior, but at least be
> > sure that it's somehow identifiable (kernel version, a define
> > set in a header, etc) so that the 3rd party drivers have a means
> > to identify the change.
> 
> eg, #if KERNEL_VERSION >= LINUX_VERSION(2,5,0)

Exactly.  This is not a minor behavior change that can be isolated to
a few drivers.  It's a fundamental change in behavior that is widespread
(I mentioned serial drivers specifically as that's what I was concerned
with but you are correct that it affects everything that uses the tty layer).

It's probably a good change to make at some point in the future, but it
needs to happen in a well defined and clearly identifiable way.  It would
probably be best for it to happen in the 2.5 tree.



> > Redhat 7.1 included that behavior change in the kernel they shipped
> > and it caused no end of problems for those of us that were doing
> > serial drivers since there was no way to easily identify that the
> > patch had been included.
> 
> The change which adds the MOD_DEC_USE_COUNT stuff is bogus, and it
> shouldn't have been made.  (I'm assuming this is what you're talking
> about).

Yes.  Redhat's default kernel in 7.1 (2.4.2-2 as it identifies itself)
will not call the close() routines if the open() routines return an
error.  The real problem isn't that the behavior changed, but that there's
no easy way to tell that they had changed it.


-- 
randall+lkml@uph.com

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[James Simmons]

> It's not only serial drivers, its everything that is a tty driver.
> I believe that auditing and fixing that lot in 2.4 just isn't going
> to happen - it's supposed to be a stable kernel after all.

Yeap. I'm working on a rewrite of the tty layer. Yesterday alone I found a
nasty flaw with the lpr console. So it will be reworked for 2.5.X.

Is their going to be a mailing for serial developement. I have lots of
ideas and a few issues to work out.

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Maciej W. Rozycki]

On Thu, 29 Nov 2001, Jeff Randall wrote:

> > lets say I call rs_open() on /dev/ttyS0 and if it
> > fails then I should not call rs_close() after a failed
> > rs_open().
> > 
> > I hope this is clear now.
> 
> All of the other UNIX variants I've dealth with behave that way.
> However, you cannot just make that change without having some means
> of identifying that behavior change because all of the linux
> serial drivers have been written to assume that their close()
> will be called even after their open() has failed.

 Nope, at least serial.c decrements its use count in rs_close() 
unconditionally (I believe other drivers do so as well), whether it was
called after a failed rs_open() or not.  As a result you may suddenly have
the use count zero while the driver is still in use.  Then modprobe
unloads it and a crash is due out soon.  That's sad the problem persists
at least since 2.2 (a temporary fix was made to 2.2.x, then it was removed
again with no better one in 2.3.x). 

 Here is a fix I'm using for 2.4.  It was submitted to Alan once, then a
discussion was performed with a conclusion like "that may be a temporary
fix for 2.4, but it needs a complete rework for 2.5".  I'm not sure if the
patch went in anywhere.

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

patch-2.4.5-tty_io-3
diff -up --recursive --new-file linux-2.4.5.macro/drivers/char/tty_io.c linux-2.4.5/drivers/char/tty_io.c
--- linux-2.4.5.macro/drivers/char/tty_io.c	Tue May  1 17:24:08 2001
+++ linux-2.4.5/drivers/char/tty_io.c	Wed Jun 27 20:46:57 2001
@@ -61,6 +61,9 @@
  * Reduced memory usage for older ARM systems
  *      -- Russell King <rmk@arm.linux.org.uk>
  *
+ * Don't call close after a failed open.
+ *	-- Maciej W. Rozycki <macro@ds2.pg.gda.pl>, 21-Jan-2001
+ *
  * Move do_SAK() into process context.  Less stack use in devfs functions.
  * alloc_tty_struct() always uses kmalloc() -- Andrew Morton <andrewm@uow.edu.eu> 17Mar01
  */
@@ -1043,7 +1046,7 @@ static void release_mem(struct tty_struc
  * WSH 09/09/97: rewritten to avoid some nasty race conditions that could
  * lead to double frees or releasing memory still in use.
  */
-static void release_dev(struct file * filp)
+static void release_dev(struct file * filp, int do_close)
 {
 	struct tty_struct *tty, *o_tty;
 	int	pty_master, tty_closing, o_tty_closing, do_sleep;
@@ -1121,7 +1124,7 @@ static void release_dev(struct file * fi
 	}
 #endif
 
-	if (tty->driver.close)
+	if (do_close && tty->driver.close)
 		tty->driver.close(tty, filp);
 
 	/*
@@ -1284,7 +1287,7 @@ static void release_dev(struct file * fi
 static int tty_open(struct inode * inode, struct file * filp)
 {
 	struct tty_struct *tty;
-	int noctty, retval;
+	int noctty, retval, open_ok;
 	kdev_t device;
 	unsigned short saved_flags;
 	char	buf[64];
@@ -1369,9 +1372,12 @@ init_dev_done:
 #ifdef TTY_DEBUG_HANGUP
 	printk("opening %s...", tty_name(tty, buf));
 #endif
-	if (tty->driver.open)
+	open_ok = 0;
+	if (tty->driver.open) {
 		retval = tty->driver.open(tty, filp);
-	else
+		if (!retval)
+			open_ok = 1;
+	} else
 		retval = -ENODEV;
 	filp->f_flags = saved_flags;
 
@@ -1384,7 +1390,7 @@ init_dev_done:
 		       tty_name(tty, buf));
 #endif
 
-		release_dev(filp);
+		release_dev(filp, open_ok);
 		if (retval != -ERESTARTSYS)
 			return retval;
 		if (signal_pending(current))
@@ -1426,7 +1432,7 @@ init_dev_done:
 static int tty_release(struct inode * inode, struct file * filp)
 {
 	lock_kernel();
-	release_dev(filp);
+	release_dev(filp, 1);
 	unlock_kernel();
 	return 0;
 }

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Russell King]

On Fri, Nov 30, 2001 at 11:44:14AM +0100, Maciej W. Rozycki wrote:
>  Here is a fix I'm using for 2.4.  It was submitted to Alan once, then a
> discussion was performed with a conclusion like "that may be a temporary
> fix for 2.4, but it needs a complete rework for 2.5".  I'm not sure if the
> patch went in anywhere.

Have you audited all the tty drivers in 2.4 to make sure that they clean
up safely?

I don't believe the serial code will clean up safely as it stands for
starters if block_til_ready in serial.c fails, leaving an interrupt
in use.  Further attempts to open the serial device will probably fail.

Try this as any user with your patch applied:

$ stty -clocal -F /dev/ttyS0
$ cat /proc/interrupts
$ cat /dev/ttyS0
^c
$ cat /proc/interrupts

I think you'll find your serial port interrupt is still claimed, despite
the module being marked as not in use.

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Maciej W. Rozycki]

On Fri, 30 Nov 2001, Russell King wrote:

> Have you audited all the tty drivers in 2.4 to make sure that they clean
> up safely?

 No, of course not -- if I had got a response like "this looks mostly OK,
but please check other drivers", then I would have certainly done.  I
think drivers/tc/zs.c is OK, too, but this was more than a year ago, so I
can't recall now, sorry. 

> I don't believe the serial code will clean up safely as it stands for
> starters if block_til_ready in serial.c fails, leaving an interrupt
> in use.  Further attempts to open the serial device will probably fail.
> 
> Try this as any user with your patch applied:
> 
> $ stty -clocal -F /dev/ttyS0
> $ cat /proc/interrupts
> $ cat /dev/ttyS0
> ^c
> $ cat /proc/interrupts
> 
> I think you'll find your serial port interrupt is still claimed, despite
> the module being marked as not in use.

 Indeed -- maybe something was changed past 2.4.5, after all.  I'll check
how things look like these days.  I nowhere use serial.c as a module
anymore, as all systems I maintain are now configured for the serial
console, so I might have missed something. 

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Russell King]

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

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[BALBIR SINGH]

[-- Attachment #1: Type: text/plain, Size: 3433 bytes --]



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



[-- Attachment #2: InterScan_Disclaimer.txt --]
[-- Type: text/plain, Size: 855 bytes --]

-------------------------------------------------------------------------------------------------------------------------
Information transmitted by this E-MAIL is proprietary to Wipro and/or its Customers and
is intended for use only by the individual or entity to which it is
addressed, and may contain information that is privileged, confidential or
exempt from disclosure under applicable law. If you are not the intended
recipient or it appears that this mail has been forwarded to you without
proper authority, you are notified that any use or dissemination of this
information in any manner is strictly prohibited. In such cases, please
notify us immediately at mailto:mailadmin@wipro.com and delete this mail
from your records.
----------------------------------------------------------------------------------------------------------------------

Re: Patch: Fix serial module use count (2.4.16 _and_ 2.5)

[Russell King]

On Thu, Nov 29, 2001 at 07:18:35PM +0530, BALBIR SINGH wrote:
> 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.

Err,
	close(-ENOMEM);

What's that going to close?  Hint: you _can't_ close a descriptor that
failed to open, since you don't have a descriptor to close.  You can
only try to close an error code, but that's not going to make it anywhere
near the kernel driver level.

> I am assuming
> that the tty layer that talks to the serial driver calls rs_close().

Correct.

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

Here's an example why that'd be real bad:

1. process A opens /dev/ttyS0 as a normal device.  This initialises
   tty->driver_data.
2. process B tries to open /dev/cua0
3. process B fails with -EBUSY since the normal device is open and active
   (see block_til_ready)
4. since rs_open failed, we set tty->driver_data to be NULL (note that this
   is the same tty device pointer as (1) above.
5. process A writes to /dev/ttyS0
6. rs_write does the following:

        struct async_struct *info = (struct async_struct *)tty->driver_data;

7. Oops.

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html