[PATCH] missing NULL check in drivers/char/n_tty.c

[PATCH] missing NULL check in drivers/char/n_tty.c

[Dan Aloni]

Hello,

The rest of the kernel treats tty->driver->chars_in_buffer as a possible
NULL. This patch changes normal_poll() to be consistent with the rest of
the code.

Signed-off-by: Dan Aloni <da-x@colinux.org>

BTW, who is currently maintaining the tty subsystem?

--- linux-2.6.7/drivers/char/n_tty.c	2004-06-21 09:30:11.000000000 +0300
+++ linux-2.6.7/drivers/char/n_tty.c	2004-06-21 09:28:04.000000000 +0300
@@ -1272,8 +1272,8 @@
 		else
 			tty->minimum_to_wake = 1;
 	}
-	if (tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS &&
-			tty->driver->write_room(tty) > 0)
+	if ((!tty->driver->chars_in_buffer || tty->driver->chars_in_buffer(tty) < WAKEUP_CHARS)
+	   && tty->driver->write_room(tty) > 0)
 		mask |= POLLOUT | POLLWRNORM;
 	return mask;
 }

-- 
Dan Aloni
da-x@colinux.org

Re: [PATCH] missing NULL check in drivers/char/n_tty.c

[Andrew Morton]

Dan Aloni <da-x@gmx.net> wrote:
>
> The rest of the kernel treats tty->driver->chars_in_buffer as a possible
>  NULL. This patch changes normal_poll() to be consistent with the rest of
>  the code.

It would be better to change the rest of the kernel - remove the tests.

If any driver fails to implement ->chars_in_buffer() then we get a nice
oops which tells us that driver needs a stub handler.

Re: [PATCH] missing NULL check in drivers/char/n_tty.c

[Dan Aloni]

On Sun, Jun 20, 2004 at 11:58:24PM -0700, Andrew Morton wrote:
> Dan Aloni <da-x@gmx.net> wrote:
> >
> > The rest of the kernel treats tty->driver->chars_in_buffer as a possible
> >  NULL. This patch changes normal_poll() to be consistent with the rest of
> >  the code.
> 
> It would be better to change the rest of the kernel - remove the tests.
> 
> If any driver fails to implement ->chars_in_buffer() then we get a nice
> oops which tells us that driver needs a stub handler.

Are you sure that it won't affect the logic in tty_wait_until_sent() 
drastically? It acts quite differently when ->chars_in_buffer == NULL.

-- 
Dan Aloni
da-x@colinux.org

Re: [PATCH] missing NULL check in drivers/char/n_tty.c

[Andrew Morton]

Dan Aloni <da-x@gmx.net> wrote:
>
> On Sun, Jun 20, 2004 at 11:58:24PM -0700, Andrew Morton wrote:
> > Dan Aloni <da-x@gmx.net> wrote:
> > >
> > > The rest of the kernel treats tty->driver->chars_in_buffer as a possible
> > >  NULL. This patch changes normal_poll() to be consistent with the rest of
> > >  the code.
> > 
> > It would be better to change the rest of the kernel - remove the tests.
> > 
> > If any driver fails to implement ->chars_in_buffer() then we get a nice
> > oops which tells us that driver needs a stub handler.
> 
> Are you sure that it won't affect the logic in tty_wait_until_sent() 
> drastically? It acts quite differently when ->chars_in_buffer == NULL.

I did a quick grep and it appears that all drivers have set ->chars_in_buffer().

I suspect there are no drivers which fail to set chars_in_buffer. 
Otherwise normal_poll() would have been oopsing in 2.4, 2.5 and 2.6?

Re: [PATCH] missing NULL check in drivers/char/n_tty.c

[Dan Aloni]

On Mon, Jun 21, 2004 at 12:39:44AM -0700, Andrew Morton wrote:
> Dan Aloni <da-x@gmx.net> wrote:
> >
> > On Sun, Jun 20, 2004 at 11:58:24PM -0700, Andrew Morton wrote:
> > > Dan Aloni <da-x@gmx.net> wrote:
> > > >
> > > > The rest of the kernel treats tty->driver->chars_in_buffer as a possible
> > > >  NULL. This patch changes normal_poll() to be consistent with the rest of
> > > >  the code.
> > > 
> > > It would be better to change the rest of the kernel - remove the tests.
> > > 
> > > If any driver fails to implement ->chars_in_buffer() then we get a nice
> > > oops which tells us that driver needs a stub handler.
> > 
> > Are you sure that it won't affect the logic in tty_wait_until_sent() 
> > drastically? It acts quite differently when ->chars_in_buffer == NULL.
> 
> I did a quick grep and it appears that all drivers have set ->chars_in_buffer().
> 
> I suspect there are no drivers which fail to set chars_in_buffer. 
> Otherwise normal_poll() would have been oopsing in 2.4, 2.5 and 2.6?

Right. Perhaps this should be applied:

Signed-off-by: Dan Aloni <da-x@colinux.org>

--- linux-2.6.7/drivers/char/n_hdlc.c
+++ linux-2.6.7/drivers/char/n_hdlc.c
@@ -760,8 +760,7 @@
 
 	case TIOCOUTQ:
 		/* get the pending tx byte count in the driver */
-		count = tty->driver->chars_in_buffer ?
-				tty->driver->chars_in_buffer(tty) : 0;
+		count = tty->driver->chars_in_buffer(tty);
 		/* add size of next output frame in queue */
 		spin_lock_irqsave(&n_hdlc->tx_buf_list.spinlock,flags);
 		if (n_hdlc->tx_buf_list.head)
--- linux-2.6.7/drivers/char/tty_ioctl.c
+++ linux-2.6.7/drivers/char/tty_ioctl.c
@@ -45,8 +45,6 @@
 	
 	printk(KERN_DEBUG "%s wait until sent...\n", tty_name(tty, buf));
 #endif
-	if (!tty->driver->chars_in_buffer)
-		return;
 	add_wait_queue(&tty->write_wait, &wait);
 	if (!timeout)
 		timeout = MAX_SCHEDULE_TIMEOUT;
@@ -461,8 +459,7 @@
 			}
 			return 0;
 		case TIOCOUTQ:
-			return put_user(tty->driver->chars_in_buffer ?
-					tty->driver->chars_in_buffer(tty) : 0,
+			return put_user(tty->driver->chars_in_buffer(tty),
 					(int __user *) arg);
 		case TIOCINQ:
 			retval = tty->read_cnt;

-- 
Dan Aloni
da-x@colinux.org

Re: [PATCH] missing NULL check in drivers/char/n_tty.c

[Paul Fulghum]

Dan Aloni wrote:
> Andrew Morton wrote:
> I did a quick grep and it appears that all drivers have set ->chars_in_buffer().
>
> I suspect there are no drivers which fail to set chars_in_buffer. 
> Otherwise normal_poll() would have been oopsing in 2.4, 2.5 and 2.6?

An addition should be made to include/linux/tty_driver.h
to document the chars_in_buffer member of struct tty_driver
and struct tty_operations as a required function.
Currently, the documentation section of this header
does not mention chars_in_buffer.

Related issue:

In looking at this, I noticed struct tty_ldisc
(include/linux/tty_ldisc.h) defines and documents
an optional (optional == NULL) member chars_in_buffer.
N_TTY (drivers/char/n_tty.c) is the only line discipline
that implements this member.

drivers/char/pty.c is the only driver that
uses ldisc.chars_in_buffer, and it checks for
ldisc.chars_in_buffer == NULL before calling.

13 other drivers call ldisc.chars_in_buffer without checking
for ldisc.chars_in_buffer == NULL, but only inside conditional
compilation for debug output. The value is not used, only logged.
These conditional debug items look like cut and paste from
one serial driver to another, and I doubt
they have been recently used (or used at all).

Which would be better?
1. Ignore this
2. Fix conditional debug output to check
    for ldisc.chars_in_buffer==NULL
3. Remove conditional debug output

--
Paul Fulghum
paulkf@microgate.com

Re: [PATCH] missing NULL check in drivers/char/n_tty.c

[Andrew Morton]

Paul Fulghum <paulkf@microgate.com> wrote:
>
> 13 other drivers call ldisc.chars_in_buffer without checking
>  for ldisc.chars_in_buffer == NULL, but only inside conditional
>  compilation for debug output. The value is not used, only logged.
>  These conditional debug items look like cut and paste from
>  one serial driver to another, and I doubt
>  they have been recently used (or used at all).
> 
>  Which would be better?
>  1. Ignore this
>  2. Fix conditional debug output to check
>      for ldisc.chars_in_buffer==NULL
>  3. Remove conditional debug output

Option 1 is quite valid.  There are no bugs here, yes?

If someone for some reason wants to clean all this up, the best way would
be to require that ->chars_in_buffer always be valid, hence remove all
those checks.

Re: [PATCH] missing NULL check in drivers/char/n_tty.c

[Paul Fulghum]

Andrew Morton wrote:

> Paul Fulghum <paulkf@microgate.com> wrote:
>> Which would be better?
>> 1. Ignore this
>> 2. Fix conditional debug output to check
>>     for ldisc.chars_in_buffer==NULL
>> 3. Remove conditional debug output
> 
> Option 1 is quite valid.  There are no bugs here, yes?

If the debug output is enabled and
a line discipline other than N_TTY is used,
then you get an oops when the NULL method
is called.

Since the debug output is not enabled by
default, and is probably never really used,
it is not a significant bug.

I thought it might be worth eliminating or
correcting the debug outputs since they seem
to get cloned into new serial drivers.
It is certainly not a big problem.

> If someone for some reason wants to clean all this up, the best way would
> be to require that ->chars_in_buffer always be valid, hence remove all
> those checks.

OK

--
Paul Fulghum
paulkf@microgate.com

Re: [PATCH] missing NULL check in drivers/char/n_tty.c

[Dan Aloni]

On Mon, Jun 21, 2004 at 11:46:05AM -0700, Andrew Morton wrote:
> Paul Fulghum <paulkf@microgate.com> wrote:
> >
> > 13 other drivers call ldisc.chars_in_buffer without checking
> >  for ldisc.chars_in_buffer == NULL, but only inside conditional
> >  compilation for debug output. The value is not used, only logged.
> >  These conditional debug items look like cut and paste from
> >  one serial driver to another, and I doubt
> >  they have been recently used (or used at all).
> > 
> >  Which would be better?
> >  1. Ignore this
> >  2. Fix conditional debug output to check
> >      for ldisc.chars_in_buffer==NULL
> >  3. Remove conditional debug output
> 
> Option 1 is quite valid.  There are no bugs here, yes?
> 
> If someone for some reason wants to clean all this up, the best way would
> be to require that ->chars_in_buffer always be valid, hence remove all
> those checks.

Apropos cleanups in the tty subsystem, what is the purpose of all 
the *_paranoia_check() calls that almost every driver duplicates for 
itself? Perhaps this can be removed.

-- 
Dan Aloni
da-x@colinux.org

Re: [PATCH] missing NULL check in drivers/char/n_tty.c

[Paul Fulghum]

Dan Aloni wrote:

>Apropos cleanups in the tty subsystem, what is the purpose of all 
>the *_paranoia_check() calls that almost every driver duplicates for 
>itself? Perhaps this can be removed.
>
At some time in the distant past, it must have been a brute force
attempt to debug some *serious* problems. I've never had the
need to enable those checks in the last 6 years.

I agree with your thought, and will be removing them from at
least the synclinkxx drivers. The checks clutter the code
with little gain. And cruft like that keeps propogating to new drivers.

--
Paul Fulghum
paulkf@microgate.com