lkml.org 
[lkml]   [2012]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Bisected regression: iterate_fd() selinux change affects flash plugin
On Fri, Nov 16, 2012 at 02:58:46PM -0500, Eric Paris wrote:
> On Mon, Nov 12, 2012 at 11:57 AM, Pavel Roskin <proski@gnu.org> wrote:
> > Quoting Eric Paris <eparis@parisplace.org>:
> >
> >> OMG this +1 -1 stuff is nuts...
>
> Ping, Al.
>
> int iterate_fd(struct files_struct *files, unsigned n,
> [snip]
> while (!res && n < fdt->max_fds) {
> file = rcu_dereference_check_fdtable(files, fdt->fd[n++]);
> if (file)
> res = f(p, file, n);
> }
> spin_unlock(&files->file_lock);
> return res;
>
> So we increment n (the file descriptor number) in the dereference,
> then pass that (wrong) number to f().
>
> Every single f() (including SELinux, the cause of this bug) returns
> fd+1 (so now we are up by 2). Then all of the users of iterate fd
> actually use fd-1 (which is wrong)
>
> Why not have iterate_fd return -ENOENT on no entries and stop all of
> the stupid games? We fix the real bug (the above function should do
> the n++ after the f() call, and the interface is sane to design
> against...

Because we might bloody well want to have "run some test on all opened
files, return the first error". And -ENOENT is quite possible one.
Moreover, -ENOENT for "everything's OK, keep going" would be really
weird.

The bug is real, but Pavel's patch is all wrong. The problem is in the
argument; we should pass descriptor number, not descriptor + 1. And fixing
that (in iterator_fd() itself) makes all callbacks work as they ought to.

PS: Pavel, the life is painful enough as it is, no need to involve BZ into
it. Next time you need to post a patch, please do just that, especially
when it's so short, OK?


\
 
 \ /
  Last update: 2012-11-30 05:21    [W:0.068 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site