[CHECKER] repetitive/contradictory comparison bugs for 2.4.7

[CHECKER] repetitive/contradictory comparison bugs for 2.4.7

[Evan Parker]

Enclosed are 11 possible bugs found by an automatic checker run over
kernel 2.4.7.  The checker is designed to look at the internal consistency
of conditionals that compare pointers against NULL, specifically for
repetitive or contradictory comparisons.  Take the following code:

if (!p) {
  ...
  if (p) ...
  ...
  if (!p) ...
}

The first conditional inside the if would be marked as a contradiction,
and the second would be marked as a repetition.  The following code would
also be marked as a contradiction, since on all paths leading to the
second if statment p would have to be non-NULL:

if (!p)
  return;

...

if (!p) ...

Now usually such repetitions and contradictions are just the result of
putting in too many checks: this is not a bad thing--in fact, it is good
coding style.  But obviously repetitve or contradictory comparisons can
often indicate bad code and/or bugs.  Out of a total of about 45 code
pieces marked by the checker, these 11 looked the most suspicious.

One of them, the last one, is pretty clearly a bug, but the
other 10 are questionable.  Those 10 are all simple variations on the
following code:

Start --->
	if (!tmp_buf) {
		page = get_free_page(GFP_KERNEL);

Error --->
		if (tmp_buf)
			free_page(page);
		else
			tmp_buf = (unsigned char *) page;
	}

One of the other variations uses a semaphore protecting the whole if
statement, and another variation uses cli() and restore_flags() to protect
just the inner if statement, but otherwise they are all basically the
same.  So my understanding is that get_free_page can block, in which case
tmp_buf might be modified by another process, making the two if statements
necessary.  But if concurrency is a problem, then shouldn't there be some
sort of protection, maybe a semaphore or a cli()/restore_flags() pair
around the inner if statement to ensure it is an atomic block?  Some of
the cases do do this, but some don't.  In any case, the variations make
it seem suspicious, so check them out.  I'm not claiming they are actually
bugs--I don't know enough to tell--but they're worth a look just to make sure.

# Summary for 2.4.7
#  Total Errors for 2.4.7	  = 11
# BUGs	|	File Name
2	|	/home/eparker/tmp/linux/2.4.7/drivers/char/generic_serial.c/
1	|	/home/eparker/tmp/linux/2.4.7/drivers/char/serial.c/
1	|	/home/eparker/tmp/linux/2.4.7/drivers/char/specialix.c/
1	|	/home/eparker/tmp/linux/2.4.7/drivers/md/md.c/
1	|	/home/eparker/tmp/linux/2.4.7/drivers/char/riscom8.c/
1	|	/home/eparker/tmp/linux/2.4.7/drivers/char/moxa.c/
1	|	/home/eparker/tmp/linux/2.4.7/drivers/video/tdfxfb.c/
1	|	/home/eparker/tmp/linux/2.4.7/drivers/char/cyclades.c/
1	|	/home/eparker/tmp/linux/2.4.7/drivers/char/rocket.c/
1	|	/home/eparker/tmp/linux/2.4.7/drivers/char/mxser.c/



############################################################
# errors
#
---------------------------------------------------------
[BUG] confusing code...there would be no need to check tmp_buf twice,
unless there is the possibility of it being modified while get_free_page
sleeps.  But then why the comment that the cli() shouldn't make a
difference?
/home/eparker/tmp/linux/2.4.7/drivers/char/generic_serial.c:953:gs_init_port: ERROR:INTERNAL_NULL3:949:953: Contradiction: "tmp_buf" is NULL so the conditional "tmp_buf != 0" will always evaluate to false! [nbytes = 1]  [distance=3]
{
	unsigned long flags;
	unsigned long page;

	save_flags (flags);
Start --->
	if (!tmp_buf) {
		page = get_free_page(GFP_KERNEL);

		cli (); /* Don't expect this to make a difference. */
Error --->
		if (tmp_buf)
			free_page(page);
		else
			tmp_buf = (unsigned char *) page;
---------------------------------------------------------
[BUG] similar to above
/home/eparker/tmp/linux/2.4.7/drivers/char/generic_serial.c:975:gs_init_port: ERROR:INTERNAL_NULL3:967:975: Contradiction: "port->xmit_buf" is NULL so the conditional "port->xmit_buf != 0" will always evaluate to false! [distance=4]
	}

	if (port->flags & ASYNC_INITIALIZED)
		return 0;

Start --->
	if (!port->xmit_buf) {
		/* We may sleep in get_free_page() */
		unsigned long tmp;

		tmp = get_free_page(GFP_KERNEL);

		/* Spinlock? */
		cli ();
Error --->
		if (port->xmit_buf)
			free_page (tmp);
		else
			port->xmit_buf = (unsigned char *) tmp;
---------------------------------------------------------
[BUG] confusing: why check twice? race condition?
/home/eparker/tmp/linux/2.4.7/drivers/md/md.c:1633:do_md_run: ERROR:INTERNAL_NULL3:1627:1633: Repetitive check: "pers[pnum]" is NULL so the conditional "pers[pnum] == 0" will always evaluate to true! [distance=4]
		 */
		printk(BAD_CHUNKSIZE);
		return -EINVAL;
	}

Start --->
	if (!pers[pnum])
	{
#ifdef CONFIG_KMOD
		char module_name[80];
		sprintf (module_name, "md-personality-%d", pnum);
		request_module (module_name);
Error --->
		if (!pers[pnum])
#endif
			return -EINVAL;
	}
---------------------------------------------------------
[BUG] similar to earlier bug in drivers/char/generic_serial.c,
moxaXmitBuff is protected by a semaphore instead of a simple cli() and
restore_flags()...but the semaphore is downed outside the first check, so
there should be no need for the second check inside the if statement...very
confusing.
/home/eparker/tmp/linux/2.4.7/drivers/char/moxa.c:576:moxa_open: ERROR:INTERNAL_NULL3:570:576: Contradiction: "moxaXmitBuff" is NULL so the conditional "moxaXmitBuff != 0" will always evaluate to false! [nbytes = 1]  [distance=13]
	if (!MoxaPortIsValid(port)) {
		tty->driver_data = NULL;
		return (-ENODEV);
	}
	down(&moxaBuffSem);
Start --->
	if (!moxaXmitBuff) {
		page = get_free_page(GFP_KERNEL);
		if (!page) {
			up(&moxaBuffSem);
			return (-ENOMEM);
		}
Error --->
		if (moxaXmitBuff)
			free_page(page);
		else
			moxaXmitBuff = (unsigned char *) page;
---------------------------------------------------------
[BUG] again, similar to the one in drivers/char/generic_serial.c, except
there is no effort to guard against race conditions: no semaphores, no
cli() and restore_flags()...so why check tmp_buf twice?  Looks like there
should be some sort of protection around the check and then assignment of
tmp_buf.
/home/eparker/tmp/linux/2.4.7/drivers/char/rocket.c:905:rp_open: ERROR:INTERNAL_NULL3:901:905: Contradiction: "tmp_buf" is NULL so the conditional "tmp_buf != 0" will always evaluate to false! [nbytes = 1]  [distance=13]
	unsigned long page;

	line = MINOR(tty->device) - tty->driver.minor_start;
	if ((line < 0) || (line >= MAX_RP_PORTS))
		return -ENODEV;
Start --->
	if (!tmp_buf) {
		page = get_free_page(GFP_KERNEL);
		if (!page)
			return -ENOMEM;
Error --->
		if (tmp_buf)
			free_page(page);
		else
			tmp_buf = (unsigned char *) page;
---------------------------------------------------------
[BUG] same
/home/eparker/tmp/linux/2.4.7/drivers/char/mxser.c:734:mxser_open: ERROR:INTERNAL_NULL3:730:734: Contradiction: "mxvar_tmp_buf" is NULL so the conditional "mxvar_tmp_buf != 0" will always evaluate to false! [nbytes = 1]  [distance=13]

	info->count++;
	tty->driver_data = info;
	info->tty = tty;

Start --->
	if (!mxvar_tmp_buf) {
		page = get_free_page(GFP_KERNEL);
		if (!page)
			return (-ENOMEM);
Error --->
		if (mxvar_tmp_buf)
			free_page(page);
		else
			mxvar_tmp_buf = (unsigned char *) page;
---------------------------------------------------------
[BUG] same
/home/eparker/tmp/linux/2.4.7/drivers/char/serial.c:3166:rs_open: ERROR:INTERNAL_NULL3:3160:3166: Contradiction: "tmp_buf" is NULL so the conditional "tmp_buf != 0" will always evaluate to false! [nbytes = 1]  [distance=13]
#endif
#if (LINUX_VERSION_CODE > 0x20100)
	info->tty->low_latency = (info->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
#endif

Start --->
	if (!tmp_buf) {
		page = get_zeroed_page(GFP_KERNEL);
		if (!page) {
			MOD_DEC_USE_COUNT;
			return -ENOMEM;
		}
Error --->
		if (tmp_buf)
			free_page(page);
		else
			tmp_buf = (unsigned char *) page;
---------------------------------------------------------
[BUG] same
/home/eparker/tmp/linux/2.4.7/drivers/char/cyclades.c:2667:cy_open: ERROR:INTERNAL_NULL3:2663:2667: Contradiction: "tmp_buf" is NULL so the conditional "tmp_buf != 0" will always evaluate to false! [nbytes = 1]  [distance=13]
    info->count++;
#ifdef CY_DEBUG_COUNT
    printk("cyc:cy_open (%d): incrementing count to %d\n",
        current->pid, info->count);
#endif
Start --->
    if (!tmp_buf) {
	page = get_free_page(GFP_KERNEL);
	if (!page)
	    return -ENOMEM;
Error --->
	if (tmp_buf)
	    free_page(page);
	else
	    tmp_buf = (unsigned char *) page;
---------------------------------------------------------
[BUG] same
/home/eparker/tmp/linux/2.4.7/drivers/char/specialix.c:1238:sx_setup_port: ERROR:INTERNAL_NULL3:1231:1238: Contradiction: "port->xmit_buf" is NULL so the conditional "port->xmit_buf != 0" will always evaluate to false! [distance=13]
	unsigned long flags;

	if (port->flags & ASYNC_INITIALIZED)
		return 0;

Start --->
	if (!port->xmit_buf) {
		/* We may sleep in get_free_page() */
		unsigned long tmp;

		if (!(tmp = get_free_page(GFP_KERNEL)))
			return -ENOMEM;

Error --->
		if (port->xmit_buf) {
			free_page(tmp);
			return -ERESTARTSYS;
		}
---------------------------------------------------------
[BUG] similar; again, looks like there might be a race condition here
/home/eparker/tmp/linux/2.4.7/drivers/char/riscom8.c:863:rc_setup_port: ERROR:INTERNAL_NULL3:856:863: Contradiction: "port->xmit_buf" is NULL so the conditional "port->xmit_buf != 0" will always evaluate to false! [distance=13]
	unsigned long flags;

	if (port->flags & ASYNC_INITIALIZED)
		return 0;

Start --->
	if (!port->xmit_buf) {
		/* We may sleep in get_free_page() */
		unsigned long tmp;

		if (!(tmp = get_free_page(GFP_KERNEL)))
			return -ENOMEM;

Error --->
		if (port->xmit_buf) {
			free_page(tmp);
			return -ERESTARTSYS;
		}
---------------------------------------------------------
[BUG] [GEM] looks like they should be checking fb_info.bufbase_virt rather than fb_info.regbase_virt
/home/eparker/tmp/linux/2.4.7/drivers/video/tdfxfb.c:1927:tdfxfb_init: ERROR:INTERNAL_NULL3:1915:1927: Contradiction: "fb_info.regbase_virt" is not NULL so the conditional "fb_info.regbase_virt == 0" will always evaluate to false! [distance=15]
	break;
      }
      fb_info.regbase_phys = pci_resource_start(pdev, 0);
      fb_info.regbase_size = 1 << 24;
      fb_info.regbase_virt = ioremap_nocache(fb_info.regbase_phys, 1 << 24);
Start --->
      if(!fb_info.regbase_virt) {
	printk("fb: Can't remap %s register area.\n", name);
	return -ENXIO;
      }

      fb_info.bufbase_phys = pci_resource_start (pdev, 1);
      if(!(fb_info.bufbase_size = do_lfb_size())) {
	iounmap(fb_info.regbase_virt);
	printk("fb: Can't count %s memory.\n", name);
	return -ENXIO;
      }
      fb_info.bufbase_virt = ioremap_nocache(fb_info.bufbase_phys, fb_info.bufbase_size);
Error --->
      if(!fb_info.regbase_virt) {
	printk("fb: Can't remap %s framebuffer.\n", name);
	iounmap(fb_info.regbase_virt);
	return -ENXIO;






--------------------------------------------------------------------
Evan Parker
nave@stanford.edu
home: (650) 497-4928
cell: (650) 207-3646
--------------------------------------------------------------------

Re: [CHECKER] repetitive/contradictory comparison bugs for 2.4.7

[Neil Brown]

On Tuesday July 24, nave@stanford.edu wrote:
> [BUG] confusing: why check twice? race condition?
> /home/eparker/tmp/linux/2.4.7/drivers/md/md.c:1633:do_md_run: ERROR:INTERNAL_NULL3:1627:1633: Repetitive check: "pers[pnum]" is NULL so the conditional "pers[pnum] == 0" will always evaluate to true! [distance=4]
> 		 */
> 		printk(BAD_CHUNKSIZE);
> 		return -EINVAL;
> 	}
> 
> Start --->
> 	if (!pers[pnum])
> 	{
> #ifdef CONFIG_KMOD
> 		char module_name[80];
> 		sprintf (module_name, "md-personality-%d", pnum);
> 		request_module (module_name);
> Error --->
> 		if (!pers[pnum])
> #endif
> 			return -EINVAL;
> 	}


This one is not a bug.  "request_module" will hopefully load a module
and run it's init routine, which should set pers[pnum] to some proper
value.
Ofcourse, it might not, so we have to check.

Note that "pers" here is a global variable, and so it should be
expected that an external function - i.e. request_module - might
change it.

Thanks anyway.

NeilBrown

Re: [CHECKER] repetitive/contradictory comparison bugs for 2.4.7

[Russell King]

On Tue, Jul 24, 2001 at 05:08:23PM -0700, Evan Parker wrote:
> One of them, the last one, is pretty clearly a bug, but the
> other 10 are questionable.  Those 10 are all simple variations on the
> following code:
> 
> Start --->
> 	if (!tmp_buf) {
> 		page = get_free_page(GFP_KERNEL);
> 
> Error --->
> 		if (tmp_buf)
> 			free_page(page);
> 		else
> 			tmp_buf = (unsigned char *) page;
> 	}

The following patch fixes this:

--- orig/drivers/char/serial.c	Sat Jul 21 10:46:42 2001
+++ linux/drivers/char/serial.c	Wed Jul 25 09:19:49 2001
@@ -3157,17 +3157,17 @@
 	info->tty->low_latency = (info->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
 #endif
 
+	down(&tmp_buf_sem);
 	if (!tmp_buf) {
 		page = get_zeroed_page(GFP_KERNEL);
 		if (!page) {
+			up(&tmp_buf_sem);
 			MOD_DEC_USE_COUNT;
 			return -ENOMEM;
 		}
-		if (tmp_buf)
-			free_page(page);
-		else
-			tmp_buf = (unsigned char *) page;
+		tmp_buf = (unsigned char *) page;
 	}
+	up(&tmp_buf_sem);
 
 	/*
 	 * If the port is the middle of closing, bail out now
@@ -5666,12 +5666,14 @@
 		if (DEACTIVATE_FUNC(brd->dev))
 			(DEACTIVATE_FUNC(brd->dev))(brd->dev);
 	}
-#endif	
+#endif
+	down(&tmp_buf_sem);
 	if (tmp_buf) {
 		unsigned long pg = (unsigned long) tmp_buf;
 		tmp_buf = NULL;
 		free_page(pg);
 	}
+	up(&tmp_buf_sem);
 	
 #ifdef ENABLE_SERIAL_PCI
 	if (serial_pci_driver.name[0])


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

Re: [CHECKER] repetitive/contradictory comparison bugs for 2.4.7

[Alan Cox]

> other 10 are questionable.  Those 10 are all simple variations on the
> following code:
> 
> Start --->
> 	if (!tmp_buf) {
> 		page = get_free_page(GFP_KERNEL);
> 
> Error --->
> 		if (tmp_buf)
> 			free_page(page);
> 		else
> 			tmp_buf = (unsigned char *) page;
> 	}

That one is not a bug. The serial drivers do this to handle a race. Really
it should be

		page = get_free_page(GFP_KERNEL)

		rmb();
		if (tmp_buf)
			..

but this will go away as and when someone switches the tty layer to new 
style locking. The precise code flow (under lock_kernel in both cases) is

	
	if (!tmp_buf)
	{
		/* tmp_buf was 0
		page = get_free_page (...)
		[SLEEPS, TASK SWITCH]

		if(tmp_buf)
		/* tmp buf was non zero - another thread allocated it */


Alan

Re: [CHECKER] repetitive/contradictory comparison bugs for 2.4.7

[Dawson Engler]

bunch of quoting for context:

> > other 10 are questionable.  Those 10 are all simple variations on the
> > following code:
> > 
> > Start --->
> > 	if (!tmp_buf) {
> > 		page = get_free_page(GFP_KERNEL);
> > 
> > Error --->
> > 		if (tmp_buf)
> > 			free_page(page);
> > 		else
> > 			tmp_buf = (unsigned char *) page;
> > 	}
> 
> That one is not a bug. The serial drivers do this to handle a race. Really
> it should be
> 
> 		page = get_free_page(GFP_KERNEL)
> 
> 		rmb();
> 		if (tmp_buf)
> 			..
> 
> but this will go away as and when someone switches the tty layer to new 
> style locking. The precise code flow (under lock_kernel in both cases) is
> 
> 	
> 	if (!tmp_buf)
> 	{
> 		/* tmp_buf was 0
> 		page = get_free_page (...)
> 		[SLEEPS, TASK SWITCH]
> 

Does this mean that the 'cli' in the following code is redundant?

	/* 2.4.7/drivers/char/generic_serial.c:953:gs_init_port: */
        if (!tmp_buf) {
                page = get_free_page(GFP_KERNEL);

                cli (); /* Don't expect this to make a difference. */
                if (tmp_buf)
                        free_page(page);
                else
                        tmp_buf = (unsigned char *) page;

Dawson

Re: [CHECKER] repetitive/contradictory comparison bugs for 2.4.7

[Alex Bligh - linux-kernel]

>> > other 10 are questionable.  Those 10 are all simple variations on the
>> > following code:
>> >
>> > Start --->
>> > 	if (!tmp_buf) {
>> > 		page = get_free_page(GFP_KERNEL);
>> >
>> > Error --->
>> > 		if (tmp_buf)
>> > 			free_page(page);
>> > 		else
>> > 			tmp_buf = (unsigned char *) page;
>> > 	}
>>
>> That one is not a bug. The serial drivers do this to handle a race.
>> Really it should be

May be I'm being dumb here, and without wishing to open the 'volatile'
can of worms elsewhere, but:

   static char * tmp_buf;

How will this be guaranteed to help handle a race, when gcc is
likely either to have tmp_buf in a register (not declared
volatile), or perhaps even optimize out the second reference.
Seems to me (and I may well be wrong), either there is a
race thread (tmp_buf being assigned between the first
test and grabbing the page), in which case as tmp_buf may
be in a register, it doesn't avoid the race (and potentially
stomps on the existing buffer), or there is not a race, in
which case the second check is unnecessary. IE the checker
found a real bug.

--
Alex Bligh

Re: [CHECKER] repetitive/contradictory comparison bugs for 2.4.7

[Russell King]

On Thu, Jul 26, 2001 at 08:54:48PM +0100, Alex Bligh - linux-kernel wrote:
> May be I'm being dumb here, and without wishing to open the 'volatile'
> can of worms elsewhere, but:
> 
>    static char * tmp_buf;

Here is the fix...  I've updated it a bit to plug another small race in
rs_write as well.

The following code uses the tmp_buf_sem to lock the creation and freeing
of the tmp_buf page.  This same lock is used to prevent concurrent accesses
to this very same page in rs_write().

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

--- orig/drivers/char/serial.c	Sat Jul 21 10:46:42 2001
+++ linux/drivers/char/serial.c	Thu Jul 26 21:14:12 2001
@@ -1848,12 +1848,17 @@
 	if (serial_paranoia_check(info, tty->device, "rs_write"))
 		return 0;
 
-	if (!tty || !info->xmit.buf || !tmp_buf)
+	if (!tty || !info->xmit.buf)
 		return 0;
 
 	save_flags(flags);
 	if (from_user) {
 		down(&tmp_buf_sem);
+		if (!tmp_buf) {
+			up(&tmp_buf_sem);
+			restore_flags(flags);
+			return 0;
+		}
 		while (1) {
 			int c1;
 			c = CIRC_SPACE_TO_END(info->xmit.head,
@@ -3129,7 +3134,6 @@
 {
 	struct async_struct	*info;
 	int 			retval, line;
-	unsigned long		page;
 
 	MOD_INC_USE_COUNT;
 	line = MINOR(tty->device) - tty->driver.minor_start;
@@ -3157,17 +3161,16 @@
 	info->tty->low_latency = (info->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
 #endif
 
+	down(&tmp_buf_sem);
 	if (!tmp_buf) {
-		page = get_zeroed_page(GFP_KERNEL);
-		if (!page) {
+		tmp_buf = (unsigned char *)get_zeroed_page(GFP_KERNEL);
+		if (!tmp_buf) {
+			up(&tmp_buf_sem);
 			MOD_DEC_USE_COUNT;
 			return -ENOMEM;
 		}
-		if (tmp_buf)
-			free_page(page);
-		else
-			tmp_buf = (unsigned char *) page;
 	}
+	up(&tmp_buf_sem);
 
 	/*
 	 * If the port is the middle of closing, bail out now
@@ -5666,12 +5669,13 @@
 		if (DEACTIVATE_FUNC(brd->dev))
 			(DEACTIVATE_FUNC(brd->dev))(brd->dev);
 	}
-#endif	
+#endif
+	down(&tmp_buf_sem);
 	if (tmp_buf) {
-		unsigned long pg = (unsigned long) tmp_buf;
+		free_page(tmp_buf);
 		tmp_buf = NULL;
-		free_page(pg);
 	}
+	up(&tmp_buf_sem);
 	
 #ifdef ENABLE_SERIAL_PCI
 	if (serial_pci_driver.name[0])

Re: [CHECKER] repetitive/contradictory comparison bugs for 2.4.7

[Alan Cox]

> How will this be guaranteed to help handle a race, when gcc is
> likely either to have tmp_buf in a register (not declared
> volatile), or perhaps even optimize out the second reference.

The function call is a synchronization point, and the function it calls
might change the value. I put the barriers into my tree to make this clear
although I cant see some future super gcc globally optimising that one
anyway

Alan

Re: [CHECKER] repetitive/contradictory comparison bugs for 2.4.7

[Alan Cox]

> >    static char * tmp_buf;
> 
> Here is the fix...  I've updated it a bit to plug another small race in
> rs_write as well.

It runs under lock_kernel so thats complete overkill

Re: [CHECKER] repetitive/contradictory comparison bugs for 2.4.7

[Alex Bligh - linux-kernel]

<alan@lxorguk.ukuu.org.uk> wrote:
>> How will this be guaranteed to help handle a race, when gcc is
>> likely either to have tmp_buf in a register (not declared
>> volatile), or perhaps even optimize out the second reference.
>
> The function call is a synchronization pointAlan,

Ooops - indeed, yes. Though the cli()/restore_flags() doesn't seem
like the right way to perform a lock. My naive interpretation is
that either it needs a (faster) real lock that doesn't disable IRQs on
multiple CPUs etc., or lock_kernel does the job in which
case cli()/restore_flags() is redundant, and, indeed, so is the
check itself. May be I have missed something (again).

I am presuming even an inline cli() is a synchronization point too,
else there would still be a race if the read of tmp_buf in
if(tmp_buf) and the cli() were reordered.

--
Alex Bligh