Re: [PATCH] cifs: fix skipping to incorrect offset in emit_cached_dirents

[ronnie sahlberg <ronniesahlberg@gmail.com>]

On Wed, 12 Oct 2022 at 02:40, Tom Talpey <tom@talpey.com> wrote:
>
> On 10/11/2022 2:46 AM, Ronnie Sahlberg wrote:
> > When application has done lseek() to a different offset on a directory fd
> > we skipped one entry too many before we start emitting directory entries
> > from the cache.
> >
> > We need to also make sure that when we are starting to emit directory
> > entries from the cache, the ->pos sequence might have holes and skip
> > some indices.
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >   fs/cifs/readdir.c | 15 ++++++++++-----
> >   1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> > index 8e060c00c969..7170614434a1 100644
> > --- a/fs/cifs/readdir.c
> > +++ b/fs/cifs/readdir.c
> > @@ -847,14 +847,19 @@ static bool emit_cached_dirents(struct cached_dirents *cde,
> >       int rc;
> >
> >       list_for_each_entry(dirent, &cde->entries, entry) {
> > -             if (ctx->pos >= dirent->pos)
> > +             if (ctx->pos > dirent->pos)
> >                       continue;
> > +             /*
> > +              * There may be holes in the ->pos sequence
> > +              * so always force ctx->pos to the current position.
> > +              */
>
> The comment is a bit vague by referring to "->pos", because
> it's the same name in both ctx and dirent.
>
> But I have a second question, does squeezing out the holes
> affect a later possible lseek on the dirhandle? I'm having
> trouble tracking down where that might happen.

I think if an application does an lseek() to a position we did not
previously tell
the application about, then what happens is undefined.
What happens in this patch is that we just "skip" to the next entry
with a "valid" pos
and start returning data from there.
I.e. we just skip over the "hole" but that should be fine since the
application should never
have lseek()ed to somewhere inside the hole to begin with.

>
> >               ctx->pos = dirent->pos;
> >               rc = dir_emit(ctx, dirent->name, dirent->namelen,
> >                             dirent->fattr.cf_uniqueid,
> >                             dirent->fattr.cf_dtype);
> >               if (!rc)
> >                       return rc;
>
> It's weird that "rc" is an integer, but dir_emit() returns a bool.
> It's also confusing that this isn't coded as
>
>                 if (!rc)
>                         return false;
>
> Other than those questions, it looks like a clever fix.
>
> Tom.
> > +             ctx->pos++;
> >       }
> >       return true;
> >   }
> > @@ -1202,10 +1207,10 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
> >                                ctx->pos, tmp_buf);
> >                       cifs_save_resume_key(current_entry, cifsFile);
> >                       break;
> > -             } else
> > -                     current_entry =
> > -                             nxt_dir_entry(current_entry, end_of_smb,
> > -                                     cifsFile->srch_inf.info_level);
> > +             }
> > +             current_entry =
> > +                     nxt_dir_entry(current_entry, end_of_smb,
> > +                                   cifsFile->srch_inf.info_level);
> >       }
> >       kfree(tmp_buf);
> >