On Mon, Jan 21, 2019 at 04:49:45PM +0100, Jann Horn wrote:

On Sun, Jan 20, 2019 at 11:41 PM Dave Chinner [off-list ref] wrote:

quoted

On Fri, Jan 18, 2019 at 05:14:40PM +0100, Jann Horn wrote:

quoted

As Al Viro pointed out, many filldir_t functions return error codes, but
all callers of filldir_t functions just check whether the return value is
non-zero (to determine whether to continue reading the directory); more
precise errors have to be signalled via struct dir_context.
Change all filldir_t functions to return bool instead of int.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jann Horn <jannh@google.com>
---
 arch/alpha/kernel/osf_sys.c | 12 +++----
 fs/afs/dir.c                | 30 +++++++++--------
 fs/ecryptfs/file.c          | 13 ++++----
 fs/exportfs/expfs.c         |  8 ++---
 fs/fat/dir.c                |  8 ++---
 fs/gfs2/export.c            |  6 ++--
 fs/nfsd/nfs4recover.c       |  8 ++---
 fs/nfsd/vfs.c               |  6 ++--
 fs/ocfs2/dir.c              | 10 +++---
 fs/ocfs2/journal.c          | 14 ++++----
 fs/overlayfs/readdir.c      | 24 +++++++-------
 fs/readdir.c                | 64 ++++++++++++++++++-------------------
 fs/reiserfs/xattr.c         | 20 ++++++------
 fs/xfs/scrub/dir.c          |  8 ++---
 fs/xfs/scrub/parent.c       |  4 +--
 include/linux/fs.h          | 10 +++---
 16 files changed, 125 insertions(+), 120 deletions(-)

diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index db1c2144d477..14e5ae0dac50 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c

@@ -108,7 +108,7 @@ struct osf_dirent_callback {
      int error;
 };

-static int
+static bool
 osf_filldir(struct dir_context *ctx, const char *name, int namlen,
          loff_t offset, u64 ino, unsigned int d_type)
 {

@@ -120,14 +120,14 @@ osf_filldir(struct dir_context *ctx, const char *name, int namlen,

      buf->error = check_dirent_name(name, namlen);
      if (unlikely(buf->error))
-             return -EFSCORRUPTED;
+             return false;
      buf->error = -EINVAL;   /* only used if we fail */
      if (reclen > buf->count)
-             return -EINVAL;
+             return false;

Oh, it's because the error being returned is being squashed by
dir_emit():

Yeah.

quoted

quoted

 struct dir_context {

@@ -3469,17 +3471,17 @@ static inline bool dir_emit(struct dir_context *ctx,
                          const char *name, int namelen,
                          u64 ino, unsigned type)
 {
-     return ctx->actor(ctx, name, namelen, ctx->pos, ino, type) == 0;
+     return ctx->actor(ctx, name, namelen, ctx->pos, ino, type);
 }

/me wonders if it would be cleaner to do:

static inline bool dir_emit(...)
{
        buf->error = ctx->actor(....)
        if (buf->error)
                return false;
        return true;
}

And clean up all filldir actors just to return the error state
rather than have to jump through hoops to stash the error state in
the context buffer and return the error state?

One negative thing about that, IMO, is that it mixes up the request
for termination of the loop and the presence of an error.

Doesn't the code already do that, only worse?

quoted

That then allows callers who want/need the full error info can
continue to call ctx->actor directly,

"continue to call ctx->actor directly"? I don't remember any code that
calls ctx->actor directly.

ovl_fill_real().

And the XFS directory scrubber could probably make better use of the
error return from ctx->actor when validating the directory contents
rather than just calling dir_emit() and aborting the scan at the
first error encountered. We eventually want to know exactly what
error was encountered here to determine if it is safe to continue,
not just a "stop processing" flag. e.g. a bad name length will need
to stop traversal because we can't trust the underlying structure,
but an invalid file type isn't a structural flaw that prevents us
from continuing to traverse and check the rest of the directory....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com