linux-kernel - Re: [PATCH] Fix tsk->exit

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: <alpine.LFD.0.999.0709050544070.20571@enigma.security.iitk.ac.in>
Date:	Wed, 5 Sep 2007 05:56:47 +0530 (IST)
From:	Satyam Sharma <satyam@...radead.org>
To:	Eugene Teo <eugeneteo@...nel.sg>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Fix tsk->exit_state usage (resend)

Hi Eugene,

This already got merged into -mm, but ...


On Sun, 19 Aug 2007, Eugene Teo wrote:
> 
> tsk->exit_state can only be 0, EXIT_ZOMBIE, or EXIT_DEAD. A non-zero test
> is the same as tsk->exit_state & (EXIT_ZOMBIE | EXIT_DEAD), so just testing
> tsk->exit_state is sufficient.

... IMHO this change harms the readability of the code.


> +++ b/fs/proc/array.c
> @@ -145,8 +145,7 @@ static inline const char *get_task_state(struct task_struct *tsk)
>  					    TASK_UNINTERRUPTIBLE |
>  					    TASK_STOPPED |
>  					    TASK_TRACED)) |
> -			(tsk->exit_state & (EXIT_ZOMBIE |
> -					    EXIT_DEAD));
> +					   tsk->exit_state;

Here, for example, the code is /purposefully/ enumerating all the task
states, probably it makes sense to explicitly enumerate the exit states
as well?


> +++ b/kernel/fork.c
> @@ -115,7 +115,7 @@ EXPORT_SYMBOL(free_task);
>  
>  void __put_task_struct(struct task_struct *tsk)
>  {
> -	WARN_ON(!(tsk->exit_state & (EXIT_DEAD | EXIT_ZOMBIE)));
> +	WARN_ON(!tsk->exit_state);


> +++ b/kernel/sched.c
> @@ -5190,7 +5190,7 @@ static void migrate_dead(unsigned int dead_cpu, struct task_struct *p)
>  	struct rq *rq = cpu_rq(dead_cpu);
>  
>  	/* Must be exiting, otherwise would be on tasklist. */
> -	BUG_ON(p->exit_state != EXIT_ZOMBIE && p->exit_state != EXIT_DEAD);
> +	BUG_ON(!p->exit_state);

Regarding above two changes -- agreed, we want to catch /any/ exiting task
state, so (!p->exit_state) is /correct/, but still, enumerating those
explicitly helps readability. And although it's unlikely, in the future,
we may have an exit_state value for which we may _not_ want to complain
(WARN or BUG) in this code. So I'd still vote to keep the code explicit
like it was ...


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