| Message ID | 20221011064611.1428646-2-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: fix skipping to incorrect offset in emit_cached_dirents | expand |
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. > 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); >
I ran the buildbot on this and it looks like this version of the patch fixed the tests which failed with the earlier version of the patch On Tue, Oct 11, 2022 at 11:38 AM 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. > > > 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); > >
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); > >
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. + */ ctx->pos = dirent->pos; rc = dir_emit(ctx, dirent->name, dirent->namelen, dirent->fattr.cf_uniqueid, dirent->fattr.cf_dtype); if (!rc) return rc; + 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);
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(-)