cifs: fix skipping to incorrect offset in emit_cached_diretns

cifs: fix skipping to incorrect offset in emit_cached_diretns

[Ronnie Sahlberg]

Steve,

V3 of the patch.  We have to handle that there may be indicies that are
skipped in the ->pos sequence so add support to handle this.
As we now can handle this case we no longer need to modify cifs_filldir
to return -EEXIST for these cases.

This fixes generic/006/100/111/257

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

[Ronnie Sahlberg]

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.
+		 */
 		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);
 
-- 
2.35.3

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

[Tom Talpey]

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);
>   

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

[Steve French]

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);
> >



-- 
Thanks,

Steve

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

[ronnie sahlberg]

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);
> >