On Fri, Nov 16, 2012 at 02:58:46PM -0500, Eric Paris wrote:

On Mon, Nov 12, 2012 at 11:57 AM, Pavel Roskin [off-list ref] wrote:

quoted

Quoting Eric Paris [off-list ref]:

quoted

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?