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] [thread-next>] [day] [month] [year] [list]
Date:	Sun, 14 Dec 2008 17:02:05 -0800
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Jiri Slaby <jirislaby@...il.com>
Cc:	oleg@...sign.ru, kenchen@...gle.com,
	Linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: broken do_each_pid_{thread,task}

Jiri Slaby <jirislaby@...il.com> writes:

> Hi,
>
> I'm getting
> `if (type == PIDTYPE_PID)' is unreachable
> warning from kernel/exit.c. The preprocessed code looks like:
> do {
>          struct hlist_node *pos___;
>          if (pgrp != ((void *)0))
>                  for (LIST ITERATION) {
>                          {
>                           if (!((p->state & 4) != 0))
>                            continue;
>                           retval = 1;
>                           break;
>                          }
>                          if (PIDTYPE_PGID == PIDTYPE_PID)
>                                  break;
>                  }
> } while (0);
> and it's obviously wrong.

Actually the test: 
>                          if (PIDTYPE_PGID == PIDTYPE_PID)
>                                  break;
Is technically ok.  The compiler should optimize it out instead of warning.
Although seeing the unexpected corner case it gets us into I think it would
be good to reconsider this test.

The break statement is also fine because the outer loop is only executed
once so it simply functions as an enclosing block, and the break transfers
control to where it should go.

> After investigating this code usage all around, it's broken on many places
> this or similar way.
>
> For do_each_pid_thread(), even this code snippet from fs/ioprio.c is broken
> due to double do {} while expansion:
> do_each_pid_thread(pgrp, PIDTYPE_PGID, p) {
>   ret = set_task_ioprio(p, ioprio);
>   if (ret)
>     break;
> } while_each_pid_thread(pgrp, PIDTYPE_PGID, p);
>
> Any idea how to get rid of this issue?

The double loop there is certainly an issue.  I'm not quite convinced that
the error handling is correct even with the break statement.  But the
break statement was written when the code was just a single loop, so the
behavior is definitely not what we intended.

However I also agree with Ken Chen's assessment that we need to loop over
threads and not just the process group leaders in some cases such as
setting the io-priority.

With respect to error handling and IO priorities can we fix the error handling
by doing what we do when we send a signal to a process group?  That is note
that there was an error, finish processing all of the other processes and then
return the error?

Eric

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

Powered by blists - more mailing lists