linux-kernel - Re: [PATCH] kthread: airo.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20060725231839.6a69a85e.akpm@osdl.org>
Date:	Tue, 25 Jul 2006 23:18:39 -0700
From:	Andrew Morton <akpm@...l.org>
To:	Sukadev Bhattiprolu <sukadev@...ibm.com>
Cc:	achirica@...il.com, hch@...radead.org, linville@...driver.com,
	haveblue@...ibm.com, serue@...ibm.com, clr@...ibm.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kthread: airo.c

On Mon, 24 Jul 2006 11:13:09 -0700
Sukadev Bhattiprolu <sukadev@...ibm.com> wrote:

> Sukadev Bhattiprolu [sukadev@...ibm.com] wrote:
> 
> | Andrew,
> | 
> | Javier Achirica, one of the major contributors to drivers/net/wireless/airo.c
> | took a look at this patch, and doesn't have any problems with it. It doesn't
> | fix any bugs and is just a cleanup, so it certainly isn't a candidate
> | for this mainline cycle
> 
> Here is the same patch, merged up to 2.6.18-rc2. Christoph's patch (see
> http://lkml.org/lkml/2006/7/13/332) still applies cleanly on top of this.
> 
> -----
> The airo driver is currently caching a pid for later use, but with the
> implementation of containers, pids themselves do not uniquely identify
> a task. The driver is also using kernel_thread() which is deprecated in
> drivers.
> 
> This patch essentially replaces the kernel_thread() with kthread_create().
> It also stores the task_struct of the airo_thread rather than its pid.
> Since this introduces a second task_struct in struct airo_info, the patch
> renames airo_info.task to airo_info.list_bss_task.
> 
> As an extension of these changes, the patch further:
> 
>          - replaces kill_proc() with kthread_stop()
>          - replaces signal_pending() with kthread_should_stop()
> 	 - removes thread completion synchronisation which is handled by
> 	   kthread_stop().
> 
> ..
>
> @@ -1736,9 +1736,9 @@ static int readBSSListRid(struct airo_in
>  		issuecommand(ai, &cmd, &rsp);
>  		up(&ai->sem);
>  		/* Let the command take effect */
> -		ai->task = current;
> +		ai->list_bss_task = current;
>  		ssleep(3);
> -		ai->task = NULL;
> +		ai->list_bss_task = NULL;

This looks a little racy to me.  It's relatively benign - a race will cause
us to sleep for too long.  But it's easy to fix:


--- a/drivers/net/wireless/airo.c~kthread-airoc-race-fix
+++ a/drivers/net/wireless/airo.c
@@ -1733,10 +1733,10 @@ static int readBSSListRid(struct airo_in
 		cmd.cmd=CMD_LISTBSS;
 		if (down_interruptible(&ai->sem))
 			return -ERESTARTSYS;
+		ai->list_bss_task = current;
 		issuecommand(ai, &cmd, &rsp);
 		up(&ai->sem);
 		/* Let the command take effect */
-		ai->list_bss_task = current;
 		ssleep(3);
 		ai->list_bss_task = NULL;
 	}
_

<looks more closely>

Actually, ssleep() ends up doing

        while (timeout)
                timeout = schedule_timeout_uninterruptible(timeout);

so if the intent of this code is to terminate the sleep early, when the
interrupt has happened then it isn't working right.  A fix would be to
convert the ssleep(3) into schedule_timeout_uninterruptible(3 * HZ).
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/