On Mon, Jan 21, 2019 at 04:49:45PM +0100, Jann Horn wrote:
> On Sun, Jan 20, 2019 at 11:41 PM Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> > On Fri, Jan 18, 2019 at 05:14:40PM +0100, Jann Horn wrote:
> > > 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@xxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
> > > ---
> > > 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.
>
> > > 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?
> > 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@xxxxxxxxxxxxx
