On Fri, Nov 16, 2012 at 02:58:46PM -0500, Eric Paris wrote: > On Mon, Nov 12, 2012 at 11:57 AM, Pavel Roskin <pro...@gnu.org> wrote: > > Quoting Eric Paris <epa...@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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/