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

cifs: fix emitting cached direntries

[Ronnie Sahlberg]

Steve, Tom, List

I update the patch to change rc to be bool and also expanded on the comments
regarding ->pos  as from Tom's comments.

[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 | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index 8e060c00c969..1bb4624e768b 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -844,17 +844,34 @@ static bool emit_cached_dirents(struct cached_dirents *cde,
 				struct dir_context *ctx)
 {
 	struct cached_dirent *dirent;
-	int rc;
+	bool rc;
 
 	list_for_each_entry(dirent, &cde->entries, entry) {
-		if (ctx->pos >= dirent->pos)
+		/*
+		 * Skip all early entries prior to the current lseek()
+		 * position.
+		 */
+		if (ctx->pos > dirent->pos)
 			continue;
+		/*
+		 * We recorded the current ->pos value for the dirent
+		 * when we stored it in the cache.
+		 * However, this sequence of ->pos values may have holes
+		 * in it, for example dot-dirs returned from the server
+		 * are suppressed.
+		 * Handle this bu forcing ctx->pos to be the same as the
+		 * ->pos of the current dirent we emit from the cache.
+		 * This means that when we emit these entries from the cache
+		 * we now emit them with the same ->pos value as in the
+		 * initial scan.
+		 */
 		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 +1219,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 6:07 PM, 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 | 29 +++++++++++++++++++++++------
>   1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index 8e060c00c969..1bb4624e768b 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -844,17 +844,34 @@ static bool emit_cached_dirents(struct cached_dirents *cde,
>   				struct dir_context *ctx)
>   {
>   	struct cached_dirent *dirent;
> -	int rc;
> +	bool rc;
>   
>   	list_for_each_entry(dirent, &cde->entries, entry) {
> -		if (ctx->pos >= dirent->pos)
> +		/*
> +		 * Skip all early entries prior to the current lseek()
> +		 * position.
> +		 */
> +		if (ctx->pos > dirent->pos)
>   			continue;
> +		/*
> +		 * We recorded the current ->pos value for the dirent
> +		 * when we stored it in the cache.
> +		 * However, this sequence of ->pos values may have holes
> +		 * in it, for example dot-dirs returned from the server
> +		 * are suppressed.
> +		 * Handle this bu forcing ctx->pos to be the same as the
> +		 * ->pos of the current dirent we emit from the cache.
> +		 * This means that when we emit these entries from the cache
> +		 * we now emit them with the same ->pos value as in the
> +		 * initial scan.
> +		 */

It's a little wordy, but it's better, so ok.

>   		ctx->pos = dirent->pos;
>   		rc = dir_emit(ctx, dirent->name, dirent->namelen,
>   			      dirent->fattr.cf_uniqueid,
>   			      dirent->fattr.cf_dtype);
>   		if (!rc)
>   			return rc;

Well, I still think it would be clearer as "return false". And the
name "rc" really seems odd now for a bool. But again, ok I guess.

Either way, I learned a thing or two chasing down dir_emit() and the
overall approach seems good.

> +		ctx->pos++;
>   	}
>   	return true;
>   }
> @@ -1202,10 +1219,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);

This change isn't actually changing anything. Would it be
better to leave out on principle?


>   	}
>   	kfree(tmp_buf);
>  
Overall,

Reviewed-by: Tom Talpey <tom@talpey.com>

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

[Leif Sahlberg]

On Wed, Oct 12, 2022 at 10:49 AM Tom Talpey <tom@talpey.com> wrote:
>
> On 10/11/2022 6:07 PM, 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 | 29 +++++++++++++++++++++++------
> >   1 file changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> > index 8e060c00c969..1bb4624e768b 100644
> > --- a/fs/cifs/readdir.c
> > +++ b/fs/cifs/readdir.c
> > @@ -844,17 +844,34 @@ static bool emit_cached_dirents(struct cached_dirents *cde,
> >                               struct dir_context *ctx)
> >   {
> >       struct cached_dirent *dirent;
> > -     int rc;
> > +     bool rc;
> >
> >       list_for_each_entry(dirent, &cde->entries, entry) {
> > -             if (ctx->pos >= dirent->pos)
> > +             /*
> > +              * Skip all early entries prior to the current lseek()
> > +              * position.
> > +              */
> > +             if (ctx->pos > dirent->pos)
> >                       continue;
> > +             /*
> > +              * We recorded the current ->pos value for the dirent
> > +              * when we stored it in the cache.
> > +              * However, this sequence of ->pos values may have holes
> > +              * in it, for example dot-dirs returned from the server
> > +              * are suppressed.
> > +              * Handle this bu forcing ctx->pos to be the same as the
> > +              * ->pos of the current dirent we emit from the cache.
> > +              * This means that when we emit these entries from the cache
> > +              * we now emit them with the same ->pos value as in the
> > +              * initial scan.
> > +              */
>
> It's a little wordy, but it's better, so ok.
>
> >               ctx->pos = dirent->pos;
> >               rc = dir_emit(ctx, dirent->name, dirent->namelen,
> >                             dirent->fattr.cf_uniqueid,
> >                             dirent->fattr.cf_dtype);
> >               if (!rc)
> >                       return rc;
>
> Well, I still think it would be clearer as "return false". And the
> name "rc" really seems odd now for a bool. But again, ok I guess.
>
> Either way, I learned a thing or two chasing down dir_emit() and the
> overall approach seems good.
>
> > +             ctx->pos++;
> >       }
> >       return true;
> >   }
> > @@ -1202,10 +1219,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);
>
> This change isn't actually changing anything. Would it be
> better to leave out on principle?

I wanted this change since previously it was a little confusing. A
quick glance might raise questions "why only do this conditionally?"
and then when looking more carefully "oh, we actually do this
unconditionally before every new iteration".

It is as you say a no-op change.
It should arguably be done in a separate patch but it is so small I
just took the lazy path and put it in this patch instead of a separate
patch.

Thanks for the review.
>
>
> >       }
> >       kfree(tmp_buf);
> >
> Overall,
>
> Reviewed-by: Tom Talpey <tom@talpey.com>
>

cifs: fix bug when skipping one entry too many after lseek()

[Ronnie Sahlberg]

Steve, List

Please see an updated patch for the issue when skipping one entry too many
after an lseek() on a direcotry and reading out of cache.
This now makes this patch plus patch4 pass generic/257.

It also introduces a trivial change to readir to avoid creating holes in the
ctx->pos sequence.   We either need to handle holes in this sequence
or we can just avoid creating holes in the first place (which this patch does)

[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.

Also change cifs_filldir() to return -EEXIST when we encouter a dot-directory in the
reply from the server. Check for this in the caller and skip caching the entry and
skip incprementing ctx->pos or else we end up emitting a sequence of directory entries with
holes in the ctx->pos space.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/readdir.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index 8e060c00c969..0c71a771b256 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -847,14 +847,14 @@ 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;
-		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;
 }
@@ -965,7 +965,7 @@ static int cifs_filldir(char *find_entry, struct file *file,
 
 	/* skip . and .. since we added them first */
 	if (cifs_entry_is_dot(&de, file_info->srch_inf.unicode))
-		return 0;
+		return -EEXIST;
 
 	if (file_info->srch_inf.unicode) {
 		struct nls_table *nlt = cifs_sb->local_nls;
@@ -1183,6 +1183,17 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
 		*tmp_buf = 0;
 		rc = cifs_filldir(current_entry, file, ctx,
 				  tmp_buf, max_len, cfid);
+		/*
+		 * If we got a dot directory, just skip to the next entry.
+		 * We have already emitted it.
+		 */
+		if (rc == -EEXIST) {
+			current_entry =
+				nxt_dir_entry(current_entry, end_of_smb,
+					      cifsFile->srch_inf.info_level);
+			rc = 0;
+			continue;
+		}
 		if (rc) {
 			if (rc > 0)
 				rc = 0;
@@ -1202,10 +1213,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

cifs: fix failure for generic/257

[Ronnie Sahlberg]

Steve, List,

Please find attached a small patch that fixes when/where we start emitting dirents
to the application AFTER it has performed a lseek() to a different offset.
Not sure if applications should assume that the offset where a specific inode value can be
found at being stable, we need this fix anyway to address generic/257

With this patch you can also apply patch4 from the cached directory series
and generic/257 will still pass.
 

[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.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/readdir.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index 8e060c00c969..da0d1e188432 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -847,9 +847,13 @@ static bool emit_cached_dirents(struct cached_dirents *cde,
 	int rc;
 
 	list_for_each_entry(dirent, &cde->entries, entry) {
-		if (ctx->pos >= dirent->pos)
+		/*
+		 * Skip ahead until we get to the current position of the
+		 * directory.
+		 */
+		if (ctx->pos > dirent->pos)
 			continue;
-		ctx->pos = dirent->pos;
+
 		rc = dir_emit(ctx, dirent->name, dirent->namelen,
 			      dirent->fattr.cf_uniqueid,
 			      dirent->fattr.cf_dtype);
-- 
2.35.3

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

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]

here is the updated version of patch 4 which I was planning to use
(minor rebase, and trivial checkpatch cleanup).  Let me know if ok


On Wed, Oct 5, 2022 at 11:36 PM Ronnie Sahlberg <lsahlber@redhat.com> 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.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/readdir.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index 8e060c00c969..da0d1e188432 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -847,9 +847,13 @@ static bool emit_cached_dirents(struct cached_dirents *cde,
>         int rc;
>
>         list_for_each_entry(dirent, &cde->entries, entry) {
> -               if (ctx->pos >= dirent->pos)
> +               /*
> +                * Skip ahead until we get to the current position of the
> +                * directory.
> +                */
> +               if (ctx->pos > dirent->pos)
>                         continue;
> -               ctx->pos = dirent->pos;
> +
>                 rc = dir_emit(ctx, dirent->name, dirent->namelen,
>                               dirent->fattr.cf_uniqueid,
>                               dirent->fattr.cf_dtype);
> --
> 2.35.3
>


-- 
Thanks,

Steve

[-- Attachment #2: 0001-cifs-enable-caching-of-directories-for-which-a-lease.patch --]
[-- Type: text/x-patch, Size: 22454 bytes --]

From c43b60e6ae9283f66bf5d61df8509bd88e888192 Mon Sep 17 00:00:00 2001
From: Ronnie Sahlberg <lsahlber@redhat.com>
Date: Thu, 6 Oct 2022 00:14:31 -0500
Subject: [PATCH] cifs: enable caching of directories for which a lease is held

This expands the directory caching to now cache an open handle for all
directories (up to a maximum) and not just the root directory.

In this patch, locking and refcounting is intended to work as so:

The main function to get a reference to a cached handle is
find_or_create_cached_dir() called from open_cached_dir()
These functions are protected under the cfid_list_lock spin-lock
to make sure we do not race creating new references for cached dirs
with deletion of expired ones.

An successful open_cached_dir() will take out 2 references to the cfid if
this was the very first and successful call to open the directory and
it acquired a lease from the server.
One reference is for the lease  and the other is for the cfid that we
return. The is lease reference is tracked by cfid->has_lease.
If the directory already has a handle with an active lease, then we just
take out one new reference for the cfid and return it.
It can happen that we have a thread that tries to open a cached directory
where we have a cfid already but we do not, yet, have a working lease. In
this case we will just return NULL, and this the caller will fall back to
the case when no handle was available.

In this model the total number of references we have on a cfid is
1 for while the handle is open and we have a lease, and one additional
reference for each open instance of a cfid.

Once we get a lease break (cached_dir_lease_break()) we remove the
cfid from the list under the spinlock. This prevents any new threads to
use it, and we also call smb2_cached_lease_break() via the work_queue
in order to drop the reference we got for the lease (we drop it outside
of the spin-lock.)
Anytime a thread calls close_cached_dir() we also drop a reference to the
cfid.
When the last reference to the cfid is released smb2_close_cached_fid()
will be invoked which will drop the reference ot the dentry we held for
this cfid and it will also, if we the handle is open/has a lease
also call SMB2_close() to close the handle on the server.

Two events require special handling:
invalidate_all_cached_dirs() this function is called from SMB2_tdis()
and cifs_mark_open_files_invalid().
In both cases the tcon is either gone already or will be shortly so
we do not need to actually close the handles. They will be dropped
server side as part of the tcon dropping.
But we have to be careful about a potential race with a concurrent
lease break so we need to take out additional refences to avoid the
cfid from being freed while we are still referencing it.

free_cached_dirs() which is called from tconInfoFree().
This is called quite late in the umount process so there should no longer
be any open handles or files and we can just free all the remaining data.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cached_dir.c | 428 +++++++++++++++++++++++++------------------
 fs/cifs/cached_dir.h |  20 +-
 fs/cifs/inode.c      |   6 +-
 fs/cifs/smb2ops.c    |   2 +-
 4 files changed, 263 insertions(+), 193 deletions(-)

diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
index b705dac383f9..e5573d4e2d83 100644
--- a/fs/cifs/cached_dir.c
+++ b/fs/cifs/cached_dir.c
@@ -11,7 +11,53 @@
 #include "smb2proto.h"
 #include "cached_dir.h"
 
-struct cached_fid *init_cached_dir(const char *path);
+static struct cached_fid *init_cached_dir(const char *path);
+static void free_cached_dir(struct cached_fid *cfid);
+
+static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
+						    const char *path,
+						    bool lookup_only)
+{
+	struct cached_fid *cfid;
+
+	spin_lock(&cfids->cfid_list_lock);
+	list_for_each_entry(cfid, &cfids->entries, entry) {
+		if (!strcmp(cfid->path, path)) {
+			/*
+			 * If it doesn't have a lease it is either not yet
+			 * fully cached or it may be in the process of
+			 * being deleted due to a lease break.
+			 */
+			if (!cfid->has_lease) {
+				spin_unlock(&cfids->cfid_list_lock);
+				return NULL;
+			}
+			kref_get(&cfid->refcount);
+			spin_unlock(&cfids->cfid_list_lock);
+			return cfid;
+		}
+	}
+	if (lookup_only) {
+		spin_unlock(&cfids->cfid_list_lock);
+		return NULL;
+	}
+	if (cfids->num_entries >= MAX_CACHED_FIDS) {
+		spin_unlock(&cfids->cfid_list_lock);
+		return NULL;
+	}
+	cfid = init_cached_dir(path);
+	if (cfid == NULL) {
+		spin_unlock(&cfids->cfid_list_lock);
+		return NULL;
+	}
+	cfid->cfids = cfids;
+	cfids->num_entries++;
+	list_add(&cfid->entry, &cfids->entries);
+	cfid->on_list = true;
+	kref_get(&cfid->refcount);
+	spin_unlock(&cfids->cfid_list_lock);
+	return cfid;
+}
 
 /*
  * Open the and cache a directory handle.
@@ -33,61 +79,65 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
 	struct kvec qi_iov[1];
 	int rc, flags = 0;
-	__le16 utf16_path = 0; /* Null - since an open of top of share */
+	__le16 *utf16_path = NULL;
 	u8 oplock = SMB2_OPLOCK_LEVEL_II;
 	struct cifs_fid *pfid;
-	struct dentry *dentry;
+	struct dentry *dentry = NULL;
 	struct cached_fid *cfid;
+	struct cached_fids *cfids;
 
-	if (tcon == NULL || tcon->nohandlecache ||
+
+	if (tcon == NULL || tcon->cfids == NULL || tcon->nohandlecache ||
 	    is_smb1_server(tcon->ses->server))
 		return -EOPNOTSUPP;
 
 	ses = tcon->ses;
 	server = ses->server;
+	cfids = tcon->cfids;
+
+	if (!server->ops->new_lease_key)
+		return -EIO;
 
 	if (cifs_sb->root == NULL)
 		return -ENOENT;
 
+	/*
+	 * TODO: for better caching we need to find and use the dentry also
+	 * for non-root directories.
+	 */
 	if (!path[0])
 		dentry = cifs_sb->root;
-	else
-		return -ENOENT;
 
-	cfid = tcon->cfids->cfid;
-	if (cfid == NULL) {
-		cfid = init_cached_dir(path);
-		tcon->cfids->cfid = cfid;
-	}
-	if (cfid == NULL)
+	utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
+	if (!utf16_path)
 		return -ENOMEM;
 
-	mutex_lock(&cfid->fid_mutex);
-	if (cfid->is_valid) {
-		cifs_dbg(FYI, "found a cached root file handle\n");
+	cfid = find_or_create_cached_dir(cfids, path, lookup_only);
+	if (cfid == NULL) {
+		kfree(utf16_path);
+		return -ENOENT;
+	}
+	/*
+	 * At this point we either have a lease already and we can just
+	 * return it. If not we are guaranteed to be the only thread accessing
+	 * this cfid.
+	 */
+	if (cfid->has_lease) {
 		*ret_cfid = cfid;
-		kref_get(&cfid->refcount);
-		mutex_unlock(&cfid->fid_mutex);
+		kfree(utf16_path);
 		return 0;
 	}
 
 	/*
 	 * We do not hold the lock for the open because in case
-	 * SMB2_open needs to reconnect, it will end up calling
-	 * cifs_mark_open_files_invalid() which takes the lock again
-	 * thus causing a deadlock
+	 * SMB2_open needs to reconnect.
+	 * This is safe because no other thread will be able to get a ref
+	 * to the cfid until we have finished opening the file and (possibly)
+	 * acquired a lease.
 	 */
-	mutex_unlock(&cfid->fid_mutex);
-
-	if (lookup_only)
-		return -ENOENT;
-
 	if (smb3_encryption_required(tcon))
 		flags |= CIFS_TRANSFORM_REQ;
 
-	if (!server->ops->new_lease_key)
-		return -EIO;
-
 	pfid = &cfid->fid;
 	server->ops->new_lease_key(pfid);
 
@@ -108,7 +158,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	oparms.reconnect = false;
 
 	rc = SMB2_open_init(tcon, server,
-			    &rqst[0], &oplock, &oparms, &utf16_path);
+			    &rqst[0], &oplock, &oparms, utf16_path);
 	if (rc)
 		goto oshr_free;
 	smb2_set_next_command(tcon, &rqst[0]);
@@ -131,47 +181,13 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	rc = compound_send_recv(xid, ses, server,
 				flags, 2, rqst,
 				resp_buftype, rsp_iov);
-	mutex_lock(&cfid->fid_mutex);
-
-	/*
-	 * Now we need to check again as the cached root might have
-	 * been successfully re-opened from a concurrent process
-	 */
-
-	if (cfid->is_valid) {
-		/* work was already done */
-
-		/* stash fids for close() later */
-		struct cifs_fid fid = {
-			.persistent_fid = pfid->persistent_fid,
-			.volatile_fid = pfid->volatile_fid,
-		};
-
-		/*
-		 * caller expects this func to set the fid in cfid to valid
-		 * cached root, so increment the refcount.
-		 */
-		kref_get(&cfid->refcount);
-
-		mutex_unlock(&cfid->fid_mutex);
-
-		if (rc == 0) {
-			/* close extra handle outside of crit sec */
-			SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
-		}
-		rc = 0;
-		goto oshr_free;
-	}
-
-	/* Cached root is still invalid, continue normaly */
-
 	if (rc) {
 		if (rc == -EREMCHG) {
 			tcon->need_reconnect = true;
 			pr_warn_once("server share %s deleted\n",
 				     tcon->tree_name);
 		}
-		goto oshr_exit;
+		goto oshr_free;
 	}
 
 	atomic_inc(&tcon->num_remote_opens);
@@ -184,46 +200,54 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 #endif /* CIFS_DEBUG2 */
 
 	cfid->tcon = tcon;
-	cfid->is_valid = true;
-	cfid->dentry = dentry;
-	if (dentry)
+	if (dentry) {
+		cfid->dentry = dentry;
 		dget(dentry);
-	kref_init(&cfid->refcount);
-
+	}
 	/* BB TBD check to see if oplock level check can be removed below */
-	if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) {
-		/*
-		 * See commit 2f94a3125b87. Increment the refcount when we
-		 * get a lease for root, release it if lease break occurs
-		 */
-		kref_get(&cfid->refcount);
-		cfid->has_lease = true;
-		smb2_parse_contexts(server, o_rsp,
-				&oparms.fid->epoch,
-				    oparms.fid->lease_key, &oplock,
-				    NULL, NULL);
-	} else
-		goto oshr_exit;
+	if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE)
+		goto oshr_free;
+
+
+	smb2_parse_contexts(server, o_rsp,
+			    &oparms.fid->epoch,
+			    oparms.fid->lease_key, &oplock,
+			    NULL, NULL);
 
 	qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
 	if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
-		goto oshr_exit;
+		goto oshr_free;
 	if (!smb2_validate_and_copy_iov(
 				le16_to_cpu(qi_rsp->OutputBufferOffset),
 				sizeof(struct smb2_file_all_info),
 				&rsp_iov[1], sizeof(struct smb2_file_all_info),
 				(char *)&cfid->file_all_info))
 		cfid->file_all_info_is_valid = true;
-
 	cfid->time = jiffies;
+	cfid->is_open = true;
+	cfid->has_lease = true;
 
-oshr_exit:
-	mutex_unlock(&cfid->fid_mutex);
 oshr_free:
+	kfree(utf16_path);
 	SMB2_open_free(&rqst[0]);
 	SMB2_query_info_free(&rqst[1]);
 	free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
 	free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
+	spin_lock(&cfids->cfid_list_lock);
+	if (!cfid->has_lease) {
+		if (cfid->on_list) {
+			list_del(&cfid->entry);
+			cfid->on_list = false;
+			cfids->num_entries--;
+		}
+		rc = -ENOENT;
+	}
+	spin_unlock(&cfids->cfid_list_lock);
+	if (rc) {
+		free_cached_dir(cfid);
+		cfid = NULL;
+	}
+
 	if (rc == 0)
 		*ret_cfid = cfid;
 
@@ -235,20 +259,22 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
 			      struct cached_fid **ret_cfid)
 {
 	struct cached_fid *cfid;
+	struct cached_fids *cfids = tcon->cfids;
 
-	cfid = tcon->cfids->cfid;
-	if (cfid == NULL)
+	if (cfids == NULL)
 		return -ENOENT;
 
-	mutex_lock(&cfid->fid_mutex);
-	if (cfid->dentry == dentry) {
-		cifs_dbg(FYI, "found a cached root file handle by dentry\n");
-		*ret_cfid = cfid;
-		kref_get(&cfid->refcount);
-		mutex_unlock(&cfid->fid_mutex);
-		return 0;
+	spin_lock(&cfids->cfid_list_lock);
+	list_for_each_entry(cfid, &cfids->entries, entry) {
+		if (dentry && cfid->dentry == dentry) {
+			cifs_dbg(FYI, "found a cached root file handle by dentry\n");
+			kref_get(&cfid->refcount);
+			*ret_cfid = cfid;
+			spin_unlock(&cfids->cfid_list_lock);
+			return 0;
+		}
 	}
-	mutex_unlock(&cfid->fid_mutex);
+	spin_unlock(&cfids->cfid_list_lock);
 	return -ENOENT;
 }
 
@@ -257,63 +283,29 @@ smb2_close_cached_fid(struct kref *ref)
 {
 	struct cached_fid *cfid = container_of(ref, struct cached_fid,
 					       refcount);
-	struct cached_dirent *dirent, *q;
 
-	if (cfid->is_valid) {
-		cifs_dbg(FYI, "clear cached root file handle\n");
-		SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
-			   cfid->fid.volatile_fid);
+	spin_lock(&cfid->cfids->cfid_list_lock);
+	if (cfid->on_list) {
+		list_del(&cfid->entry);
+		cfid->on_list = false;
+		cfid->cfids->num_entries--;
 	}
+	spin_unlock(&cfid->cfids->cfid_list_lock);
 
-	/*
-	 * We only check validity above to send SMB2_close,
-	 * but we still need to invalidate these entries
-	 * when this function is called
-	 */
-	cfid->is_valid = false;
-	cfid->file_all_info_is_valid = false;
-	cfid->has_lease = false;
-	if (cfid->dentry) {
-		dput(cfid->dentry);
-		cfid->dentry = NULL;
-	}
-	/*
-	 * Delete all cached dirent names
-	 */
-	mutex_lock(&cfid->dirents.de_mutex);
-	list_for_each_entry_safe(dirent, q, &cfid->dirents.entries, entry) {
-		list_del(&dirent->entry);
-		kfree(dirent->name);
-		kfree(dirent);
+	dput(cfid->dentry);
+	cfid->dentry = NULL;
+
+	if (cfid->is_open) {
+		SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
+			   cfid->fid.volatile_fid);
 	}
-	cfid->dirents.is_valid = 0;
-	cfid->dirents.is_failed = 0;
-	cfid->dirents.ctx = NULL;
-	cfid->dirents.pos = 0;
-	mutex_unlock(&cfid->dirents.de_mutex);
 
+	free_cached_dir(cfid);
 }
 
 void close_cached_dir(struct cached_fid *cfid)
 {
-	mutex_lock(&cfid->fid_mutex);
 	kref_put(&cfid->refcount, smb2_close_cached_fid);
-	mutex_unlock(&cfid->fid_mutex);
-}
-
-void close_cached_dir_lease_locked(struct cached_fid *cfid)
-{
-	if (cfid->has_lease) {
-		cfid->has_lease = false;
-		kref_put(&cfid->refcount, smb2_close_cached_fid);
-	}
-}
-
-void close_cached_dir_lease(struct cached_fid *cfid)
-{
-	mutex_lock(&cfid->fid_mutex);
-	close_cached_dir_lease_locked(cfid);
-	mutex_unlock(&cfid->fid_mutex);
 }
 
 /*
@@ -326,41 +318,62 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb)
 	struct cached_fid *cfid;
 	struct cifs_tcon *tcon;
 	struct tcon_link *tlink;
+	struct cached_fids *cfids;
 
 	for (node = rb_first(root); node; node = rb_next(node)) {
 		tlink = rb_entry(node, struct tcon_link, tl_rbnode);
 		tcon = tlink_tcon(tlink);
 		if (IS_ERR(tcon))
 			continue;
-		cfid = tcon->cfids->cfid;
-		if (cfid == NULL)
+		cfids = tcon->cfids;
+		if (cfids == NULL)
 			continue;
-		mutex_lock(&cfid->fid_mutex);
-		if (cfid->dentry) {
+		list_for_each_entry(cfid, &cfids->entries, entry) {
 			dput(cfid->dentry);
 			cfid->dentry = NULL;
 		}
-		mutex_unlock(&cfid->fid_mutex);
 	}
 }
 
 /*
- * Invalidate and close all cached dirs when a TCON has been reset
+ * Invalidate all cached dirs when a TCON has been reset
  * due to a session loss.
  */
 void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
 {
-	struct cached_fid *cfid = tcon->cfids->cfid;
-
-	if (cfid == NULL)
-		return;
-
-	mutex_lock(&cfid->fid_mutex);
-	cfid->is_valid = false;
-	/* cached handle is not valid, so SMB2_CLOSE won't be sent below */
-	close_cached_dir_lease_locked(cfid);
-	memset(&cfid->fid, 0, sizeof(struct cifs_fid));
-	mutex_unlock(&cfid->fid_mutex);
+	struct cached_fids *cfids = tcon->cfids;
+	struct cached_fid *cfid, *q;
+	struct list_head entry;
+
+	INIT_LIST_HEAD(&entry);
+	spin_lock(&cfids->cfid_list_lock);
+	list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
+		list_del(&cfid->entry);
+		list_add(&cfid->entry, &entry);
+		cfids->num_entries--;
+		cfid->is_open = false;
+		/* To prevent race with smb2_cached_lease_break() */
+		kref_get(&cfid->refcount);
+	}
+	spin_unlock(&cfids->cfid_list_lock);
+
+	list_for_each_entry_safe(cfid, q, &entry, entry) {
+		cfid->on_list = false;
+		list_del(&cfid->entry);
+		cancel_work_sync(&cfid->lease_break);
+		if (cfid->has_lease) {
+			/*
+			 * We lease was never cancelled from the server so we
+			 * need to drop the reference.
+			 */
+			spin_lock(&cfids->cfid_list_lock);
+			cfid->has_lease = false;
+			spin_unlock(&cfids->cfid_list_lock);
+			kref_put(&cfid->refcount, smb2_close_cached_fid);
+		}
+		/* Drop the extra reference opened above*/
+		kref_put(&cfid->refcount, smb2_close_cached_fid);
+	}
 }
 
 static void
@@ -369,51 +382,83 @@ smb2_cached_lease_break(struct work_struct *work)
 	struct cached_fid *cfid = container_of(work,
 				struct cached_fid, lease_break);
 
-	close_cached_dir_lease(cfid);
+	spin_lock(&cfid->cfids->cfid_list_lock);
+	cfid->has_lease = false;
+	spin_unlock(&cfid->cfids->cfid_list_lock);
+	kref_put(&cfid->refcount, smb2_close_cached_fid);
 }
 
 int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
 {
-	struct cached_fid *cfid = tcon->cfids->cfid;
+	struct cached_fids *cfids = tcon->cfids;
+	struct cached_fid *cfid;
 
-	if (cfid == NULL)
+	if (cfids == NULL)
 		return false;
 
-	if (cfid->is_valid &&
-	    !memcmp(lease_key,
-		    cfid->fid.lease_key,
-		    SMB2_LEASE_KEY_SIZE)) {
-		cfid->time = 0;
-		INIT_WORK(&cfid->lease_break,
-			  smb2_cached_lease_break);
-		queue_work(cifsiod_wq,
-			   &cfid->lease_break);
-		return true;
+	spin_lock(&cfids->cfid_list_lock);
+	list_for_each_entry(cfid, &cfids->entries, entry) {
+		if (cfid->has_lease &&
+		    !memcmp(lease_key,
+			    cfid->fid.lease_key,
+			    SMB2_LEASE_KEY_SIZE)) {
+			cfid->time = 0;
+			/*
+			 * We found a lease remove it from the list
+			 * so no threads can access it.
+			 */
+			list_del(&cfid->entry);
+			cfid->on_list = false;
+			cfids->num_entries--;
+
+			queue_work(cifsiod_wq,
+				   &cfid->lease_break);
+			spin_unlock(&cfids->cfid_list_lock);
+			return true;
+		}
 	}
+	spin_unlock(&cfids->cfid_list_lock);
 	return false;
 }
 
-struct cached_fid *init_cached_dir(const char *path)
+static struct cached_fid *init_cached_dir(const char *path)
 {
 	struct cached_fid *cfid;
 
-	cfid = kzalloc(sizeof(*cfid), GFP_KERNEL);
+	cfid = kzalloc(sizeof(*cfid), GFP_ATOMIC);
 	if (!cfid)
 		return NULL;
-	cfid->path = kstrdup(path, GFP_KERNEL);
+	cfid->path = kstrdup(path, GFP_ATOMIC);
 	if (!cfid->path) {
 		kfree(cfid);
 		return NULL;
 	}
 
+	INIT_WORK(&cfid->lease_break, smb2_cached_lease_break);
+	INIT_LIST_HEAD(&cfid->entry);
 	INIT_LIST_HEAD(&cfid->dirents.entries);
 	mutex_init(&cfid->dirents.de_mutex);
-	mutex_init(&cfid->fid_mutex);
+	spin_lock_init(&cfid->fid_lock);
+	kref_init(&cfid->refcount);
 	return cfid;
 }
 
-void free_cached_dir(struct cached_fid *cfid)
+static void free_cached_dir(struct cached_fid *cfid)
 {
+	struct cached_dirent *dirent, *q;
+
+	dput(cfid->dentry);
+	cfid->dentry = NULL;
+
+	/*
+	 * Delete all cached dirent names
+	 */
+	list_for_each_entry_safe(dirent, q, &cfid->dirents.entries, entry) {
+		list_del(&dirent->entry);
+		kfree(dirent->name);
+		kfree(dirent);
+	}
+
 	kfree(cfid->path);
 	cfid->path = NULL;
 	kfree(cfid);
@@ -426,15 +471,34 @@ struct cached_fids *init_cached_dirs(void)
 	cfids = kzalloc(sizeof(*cfids), GFP_KERNEL);
 	if (!cfids)
 		return NULL;
-	mutex_init(&cfids->cfid_list_mutex);
+	spin_lock_init(&cfids->cfid_list_lock);
+	INIT_LIST_HEAD(&cfids->entries);
 	return cfids;
 }
 
+/*
+ * Called from tconInfoFree when we are tearing down the tcon.
+ * There are no active users or open files/directories at this point.
+ */
 void free_cached_dirs(struct cached_fids *cfids)
 {
-	if (cfids->cfid) {
-		free_cached_dir(cfids->cfid);
-		cfids->cfid = NULL;
+	struct cached_fid *cfid, *q;
+	struct list_head entry;
+
+	INIT_LIST_HEAD(&entry);
+	spin_lock(&cfids->cfid_list_lock);
+	list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
+		cfid->on_list = false;
+		cfid->is_open = false;
+		list_del(&cfid->entry);
+		list_add(&cfid->entry, &entry);
 	}
+	spin_unlock(&cfids->cfid_list_lock);
+
+	list_for_each_entry_safe(cfid, q, &entry, entry) {
+		list_del(&cfid->entry);
+		free_cached_dir(cfid);
+	}
+
 	kfree(cfids);
 }
diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h
index bdf6c3866653..e536304ca2ce 100644
--- a/fs/cifs/cached_dir.h
+++ b/fs/cifs/cached_dir.h
@@ -31,14 +31,17 @@ struct cached_dirents {
 };
 
 struct cached_fid {
+	struct list_head entry;
+	struct cached_fids *cfids;
 	const char *path;
-	bool is_valid:1;	/* Do we have a useable root fid */
-	bool file_all_info_is_valid:1;
 	bool has_lease:1;
+	bool is_open:1;
+	bool on_list:1;
+	bool file_all_info_is_valid:1;
 	unsigned long time; /* jiffies of when lease was taken */
 	struct kref refcount;
 	struct cifs_fid fid;
-	struct mutex fid_mutex;
+	spinlock_t fid_lock;
 	struct cifs_tcon *tcon;
 	struct dentry *dentry;
 	struct work_struct lease_break;
@@ -46,9 +49,14 @@ struct cached_fid {
 	struct cached_dirents dirents;
 };
 
+#define MAX_CACHED_FIDS 16
 struct cached_fids {
-	struct mutex cfid_list_mutex;
-	struct cached_fid *cfid;
+	/* Must be held when:
+	 * - accessing the cfids->entries list
+	 */
+	spinlock_t cfid_list_lock;
+	int num_entries;
+	struct list_head entries;
 };
 
 extern struct cached_fids *init_cached_dirs(void);
@@ -61,8 +69,6 @@ extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
 				     struct dentry *dentry,
 				     struct cached_fid **cfid);
 extern void close_cached_dir(struct cached_fid *cfid);
-extern void close_cached_dir_lease(struct cached_fid *cfid);
-extern void close_cached_dir_lease_locked(struct cached_fid *cfid);
 extern void close_all_cached_dirs(struct cifs_sb_info *cifs_sb);
 extern void invalidate_all_cached_dirs(struct cifs_tcon *tcon);
 extern int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16]);
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 3784d3a88053..5340e32e7315 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2265,13 +2265,13 @@ cifs_dentry_needs_reval(struct dentry *dentry)
 		return true;
 
 	if (!open_cached_dir_by_dentry(tcon, dentry->d_parent, &cfid)) {
-		mutex_lock(&cfid->fid_mutex);
+		spin_lock(&cfid->fid_lock);
 		if (cfid->time && cifs_i->time > cfid->time) {
-			mutex_unlock(&cfid->fid_mutex);
+			spin_unlock(&cfid->fid_lock);
 			close_cached_dir(cfid);
 			return false;
 		}
-		mutex_unlock(&cfid->fid_mutex);
+		spin_unlock(&cfid->fid_lock);
 		close_cached_dir(cfid);
 	}
 	/*
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 10f9ef68e510..537c78b30931 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -800,7 +800,7 @@ smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
 
 	rc = open_cached_dir(xid, tcon, full_path, cifs_sb, true, &cfid);
 	if (!rc) {
-		if (cfid->is_valid) {
+		if (cfid->has_lease) {
 			close_cached_dir(cfid);
 			return 0;
 		}
-- 
2.34.1

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

[Steve French]

tentatively added to cifs-2.6.git for-next pending testing

On Wed, Oct 5, 2022 at 11:36 PM Ronnie Sahlberg <lsahlber@redhat.com> 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.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/readdir.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index 8e060c00c969..da0d1e188432 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -847,9 +847,13 @@ static bool emit_cached_dirents(struct cached_dirents *cde,
>         int rc;
>
>         list_for_each_entry(dirent, &cde->entries, entry) {
> -               if (ctx->pos >= dirent->pos)
> +               /*
> +                * Skip ahead until we get to the current position of the
> +                * directory.
> +                */
> +               if (ctx->pos > dirent->pos)
>                         continue;
> -               ctx->pos = dirent->pos;
> +
>                 rc = dir_emit(ctx, dirent->name, dirent->namelen,
>                               dirent->fattr.cf_uniqueid,
>                               dirent->fattr.cf_dtype);
> --
> 2.35.3
>


-- 
Thanks,

Steve

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

[Steve French]

Adding this one patch onto for-next, it went from passing all (except
452 which is known issue with refcount and umount/mount) of buildbot
cifs-testing group to hanging on generic/010 and generic/024 to
Windows (interestingly 024 did not hang when run to Samba)

On Wed, Oct 5, 2022 at 11:36 PM Ronnie Sahlberg <lsahlber@redhat.com> 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.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/readdir.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index 8e060c00c969..da0d1e188432 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -847,9 +847,13 @@ static bool emit_cached_dirents(struct cached_dirents *cde,
>         int rc;
>
>         list_for_each_entry(dirent, &cde->entries, entry) {
> -               if (ctx->pos >= dirent->pos)
> +               /*
> +                * Skip ahead until we get to the current position of the
> +                * directory.
> +                */
> +               if (ctx->pos > dirent->pos)
>                         continue;
> -               ctx->pos = dirent->pos;
> +
>                 rc = dir_emit(ctx, dirent->name, dirent->namelen,
>                               dirent->fattr.cf_uniqueid,
>                               dirent->fattr.cf_dtype);
> --
> 2.35.3
>


-- 
Thanks,

Steve

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

[Aurélien Aptel]

Hi,

Make sure you're not re-introducing the bug where the first few files
are missing when mounting the root of a Windows drive.

See fix https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/cifs/smb2ops.c?id=0595751f267994c3c7027377058e4185b3a28e75

And bug https://bugzilla.samba.org/show_bug.cgi?id=13107

Cheers,

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

[ronnie sahlberg]

On Mon, 10 Oct 2022 at 04:04, Aurélien Aptel <aurelien.aptel@gmail.com> wrote:
>
> Hi,
>
> Make sure you're not re-introducing the bug where the first few files
> are missing when mounting the root of a Windows drive.
>
> See fix https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/cifs/smb2ops.c?id=0595751f267994c3c7027377058e4185b3a28e75
>
> And bug https://bugzilla.samba.org/show_bug.cgi?id=13107

Yeah.
What confuses things is that we do not create a contignous sequence of
index positions when we emit a directory.
For the dir_context, ctx->pos starts at 0 for the first entry and then
increments by one for each other entry.
We do not currently create a contignous space but one with one or two
holes in it.
We start by explicitely emitting '.' and '..' and these are at
position 0 and 2 respectively.
But then, for a server that for example DOES return entries for '.'
and '..' we skip emitting these entries but still increment pos,
thus we end up with a sequence for index positions of the entries of
0,1,4,5,6,7,...
I.e. there is a hole in the sequence where 2 and 3 are missing becasue
these were the dot directories that the server responded
and that we did not emit, but we incremented pos.

This should ideally be fixed because otherwise, what would lseek(3), mean ?
>
> Cheers,