diff mbox series

[v3] cifs: report error instead of invalid when revalidating a dentry fails

Message ID 20210202174255.4269-1-aaptel@suse.com (mailing list archive)
State New, archived
Headers show
Series [v3] cifs: report error instead of invalid when revalidating a dentry fails | expand

Commit Message

Aurélien Aptel Feb. 2, 2021, 5:42 p.m. UTC 
From: Aurelien Aptel <aaptel@suse.com>

Assuming
- //HOST/a is mounted on /mnt
- //HOST/b is mounted on /mnt/b

On a slow connection, running 'df' and killing it while it's
processing /mnt/b can make cifs_get_inode_info() returns -ERESTARTSYS.

This triggers the following chain of events:
=> the dentry revalidation fail
=> dentry is put and released
=> superblock associated with the dentry is put
=> /mnt/b is unmounted

This patch makes cifs_d_revalidate() return the error instead of
0 (invalid) when cifs_revalidate_dentry() fails, except for ENOENT
where that error means the dentry is invalid.

Signed-off-by: Aurelien Aptel <aaptel@suse.com>
Suggested-by: Shyam Prasad N <nspmangalore@gmail.com>
---
 fs/cifs/dir.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Steve French Feb. 2, 2021, 5:55 p.m. UTC | #1 
presumably cc:stable?

On Tue, Feb 2, 2021 at 11:43 AM Aurélien Aptel <aaptel@suse.com> wrote:
>
> From: Aurelien Aptel <aaptel@suse.com>
>
> Assuming
> - //HOST/a is mounted on /mnt
> - //HOST/b is mounted on /mnt/b
>
> On a slow connection, running 'df' and killing it while it's
> processing /mnt/b can make cifs_get_inode_info() returns -ERESTARTSYS.
>
> This triggers the following chain of events:
> => the dentry revalidation fail
> => dentry is put and released
> => superblock associated with the dentry is put
> => /mnt/b is unmounted
>
> This patch makes cifs_d_revalidate() return the error instead of
> 0 (invalid) when cifs_revalidate_dentry() fails, except for ENOENT
> where that error means the dentry is invalid.
>
> Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> Suggested-by: Shyam Prasad N <nspmangalore@gmail.com>
> ---
>  fs/cifs/dir.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 68900f1629bff..868c0b7263ec0 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -737,6 +737,7 @@ static int
>  cifs_d_revalidate(struct dentry *direntry, unsigned int flags)
>  {
>         struct inode *inode;
> +       int rc;
>
>         if (flags & LOOKUP_RCU)
>                 return -ECHILD;
> @@ -746,8 +747,11 @@ cifs_d_revalidate(struct dentry *direntry, unsigned int flags)
>                 if ((flags & LOOKUP_REVAL) && !CIFS_CACHE_READ(CIFS_I(inode)))
>                         CIFS_I(inode)->time = 0; /* force reval */
>
> -               if (cifs_revalidate_dentry(direntry))
> -                       return 0;
> +               rc = cifs_revalidate_dentry(direntry);
> +               if (rc) {
> +                       cifs_dbg(FYI, "cifs_revalidate_dentry failed with rc=%d", rc);
> +                       return rc == -ENOENT ? 0 : rc;
> +               }
>                 else {
>                         /*
>                          * If the inode wasn't known to be a dfs entry when
> --
> 2.29.2
>
Shyam Prasad N Feb. 2, 2021, 6:26 p.m. UTC | #2 
This looks good to me.
Let me know if you get a chance to test it out. If not, I'll test it
on my setup tomorrow.

Regards,
Shyam

On Tue, Feb 2, 2021 at 11:13 PM Aurélien Aptel <aaptel@suse.com> wrote:
>
> From: Aurelien Aptel <aaptel@suse.com>
>
> Assuming
> - //HOST/a is mounted on /mnt
> - //HOST/b is mounted on /mnt/b
>
> On a slow connection, running 'df' and killing it while it's
> processing /mnt/b can make cifs_get_inode_info() returns -ERESTARTSYS.
>
> This triggers the following chain of events:
> => the dentry revalidation fail
> => dentry is put and released
> => superblock associated with the dentry is put
> => /mnt/b is unmounted
>
> This patch makes cifs_d_revalidate() return the error instead of
> 0 (invalid) when cifs_revalidate_dentry() fails, except for ENOENT
> where that error means the dentry is invalid.
>
> Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> Suggested-by: Shyam Prasad N <nspmangalore@gmail.com>
> ---
>  fs/cifs/dir.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 68900f1629bff..868c0b7263ec0 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -737,6 +737,7 @@ static int
>  cifs_d_revalidate(struct dentry *direntry, unsigned int flags)
>  {
>         struct inode *inode;
> +       int rc;
>
>         if (flags & LOOKUP_RCU)
>                 return -ECHILD;
> @@ -746,8 +747,11 @@ cifs_d_revalidate(struct dentry *direntry, unsigned int flags)
>                 if ((flags & LOOKUP_REVAL) && !CIFS_CACHE_READ(CIFS_I(inode)))
>                         CIFS_I(inode)->time = 0; /* force reval */
>
> -               if (cifs_revalidate_dentry(direntry))
> -                       return 0;
> +               rc = cifs_revalidate_dentry(direntry);
> +               if (rc) {
> +                       cifs_dbg(FYI, "cifs_revalidate_dentry failed with rc=%d", rc);
> +                       return rc == -ENOENT ? 0 : rc;
> +               }
>                 else {
>                         /*
>                          * If the inode wasn't known to be a dfs entry when
> --
> 2.29.2
>
Shyam Prasad N Feb. 2, 2021, 6:27 p.m. UTC | #3 
I agree.

On Tue, Feb 2, 2021 at 11:25 PM Steve French <smfrench@gmail.com> wrote:
>
> presumably cc:stable?
>
> On Tue, Feb 2, 2021 at 11:43 AM Aurélien Aptel <aaptel@suse.com> wrote:
> >
> > From: Aurelien Aptel <aaptel@suse.com>
> >
> > Assuming
> > - //HOST/a is mounted on /mnt
> > - //HOST/b is mounted on /mnt/b
> >
> > On a slow connection, running 'df' and killing it while it's
> > processing /mnt/b can make cifs_get_inode_info() returns -ERESTARTSYS.
> >
> > This triggers the following chain of events:
> > => the dentry revalidation fail
> > => dentry is put and released
> > => superblock associated with the dentry is put
> > => /mnt/b is unmounted
> >
> > This patch makes cifs_d_revalidate() return the error instead of
> > 0 (invalid) when cifs_revalidate_dentry() fails, except for ENOENT
> > where that error means the dentry is invalid.
> >
> > Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> > Suggested-by: Shyam Prasad N <nspmangalore@gmail.com>
> > ---
> >  fs/cifs/dir.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > index 68900f1629bff..868c0b7263ec0 100644
> > --- a/fs/cifs/dir.c
> > +++ b/fs/cifs/dir.c
> > @@ -737,6 +737,7 @@ static int
> >  cifs_d_revalidate(struct dentry *direntry, unsigned int flags)
> >  {
> >         struct inode *inode;
> > +       int rc;
> >
> >         if (flags & LOOKUP_RCU)
> >                 return -ECHILD;
> > @@ -746,8 +747,11 @@ cifs_d_revalidate(struct dentry *direntry, unsigned int flags)
> >                 if ((flags & LOOKUP_REVAL) && !CIFS_CACHE_READ(CIFS_I(inode)))
> >                         CIFS_I(inode)->time = 0; /* force reval */
> >
> > -               if (cifs_revalidate_dentry(direntry))
> > -                       return 0;
> > +               rc = cifs_revalidate_dentry(direntry);
> > +               if (rc) {
> > +                       cifs_dbg(FYI, "cifs_revalidate_dentry failed with rc=%d", rc);
> > +                       return rc == -ENOENT ? 0 : rc;
> > +               }
> >                 else {
> >                         /*
> >                          * If the inode wasn't known to be a dfs entry when
> > --
> > 2.29.2
> >
>
>
> --
> Thanks,
>
> Steve
Aurélien Aptel Feb. 2, 2021, 6:34 p.m. UTC | #4 
Shyam Prasad N <nspmangalore@gmail.com> writes:
> This looks good to me.
> Let me know if you get a chance to test it out. If not, I'll test it
> on my setup tomorrow.

I've done some tests: the reproducer cannot trigger the bug, accessing a
deleted file invalidates, accessing an existing file revalidates. It looks
ok.

Cheers,
Steve French Feb. 3, 2021, 4:11 a.m. UTC | #5 
tentatively merged into cifs-2.6.git for-next pending a little more
testing, and cc:stable

as a separate patch - would like to know if worth trying the test case
references in commit ecf3d1f1aa7  and adding the d_weak_revalidate
routine that three filesystems added in that patch of Jeff.


On Tue, Feb 2, 2021 at 11:43 AM Aurélien Aptel <aaptel@suse.com> wrote:
>
> From: Aurelien Aptel <aaptel@suse.com>
>
> Assuming
> - //HOST/a is mounted on /mnt
> - //HOST/b is mounted on /mnt/b
>
> On a slow connection, running 'df' and killing it while it's
> processing /mnt/b can make cifs_get_inode_info() returns -ERESTARTSYS.
>
> This triggers the following chain of events:
> => the dentry revalidation fail
> => dentry is put and released
> => superblock associated with the dentry is put
> => /mnt/b is unmounted
>
> This patch makes cifs_d_revalidate() return the error instead of
> 0 (invalid) when cifs_revalidate_dentry() fails, except for ENOENT
> where that error means the dentry is invalid.
>
> Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> Suggested-by: Shyam Prasad N <nspmangalore@gmail.com>
> ---
>  fs/cifs/dir.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 68900f1629bff..868c0b7263ec0 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -737,6 +737,7 @@ static int
>  cifs_d_revalidate(struct dentry *direntry, unsigned int flags)
>  {
>         struct inode *inode;
> +       int rc;
>
>         if (flags & LOOKUP_RCU)
>                 return -ECHILD;
> @@ -746,8 +747,11 @@ cifs_d_revalidate(struct dentry *direntry, unsigned int flags)
>                 if ((flags & LOOKUP_REVAL) && !CIFS_CACHE_READ(CIFS_I(inode)))
>                         CIFS_I(inode)->time = 0; /* force reval */
>
> -               if (cifs_revalidate_dentry(direntry))
> -                       return 0;
> +               rc = cifs_revalidate_dentry(direntry);
> +               if (rc) {
> +                       cifs_dbg(FYI, "cifs_revalidate_dentry failed with rc=%d", rc);
> +                       return rc == -ENOENT ? 0 : rc;
> +               }
>                 else {
>                         /*
>                          * If the inode wasn't known to be a dfs entry when
> --
> 2.29.2
>


--
Thanks,

Steve
Shyam Prasad N Feb. 3, 2021, 4:24 a.m. UTC | #6 
Sounds good.

Regards,
Shyam

On Wed, Feb 3, 2021 at 12:04 AM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Shyam Prasad N <nspmangalore@gmail.com> writes:
> > This looks good to me.
> > Let me know if you get a chance to test it out. If not, I'll test it
> > on my setup tomorrow.
>
> I've done some tests: the reproducer cannot trigger the bug, accessing a
> deleted file invalidates, accessing an existing file revalidates. It looks
> ok.
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
>
Shyam Prasad N Feb. 5, 2021, 1:32 p.m. UTC | #7 
Hi Aurélien,

xfstest 070 was failing today with this patch.
It looks like we need to handle ESTALE here, in addition to ENOENT.
ESTALE happens when the file type or inode number has changed on the
server, which indicates that it's a new entry.

Regards,
Shyam

On Tue, Feb 2, 2021 at 10:34 AM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Shyam Prasad N <nspmangalore@gmail.com> writes:
> > This looks good to me.
> > Let me know if you get a chance to test it out. If not, I'll test it
> > on my setup tomorrow.
>
> I've done some tests: the reproducer cannot trigger the bug, accessing a
> deleted file invalidates, accessing an existing file revalidates. It looks
> ok.
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
>
diff mbox series

Patch

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 68900f1629bff..868c0b7263ec0 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -737,6 +737,7 @@  static int
 cifs_d_revalidate(struct dentry *direntry, unsigned int flags)
 {
 	struct inode *inode;
+	int rc;
 
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
@@ -746,8 +747,11 @@  cifs_d_revalidate(struct dentry *direntry, unsigned int flags)
 		if ((flags & LOOKUP_REVAL) && !CIFS_CACHE_READ(CIFS_I(inode)))
 			CIFS_I(inode)->time = 0; /* force reval */
 
-		if (cifs_revalidate_dentry(direntry))
-			return 0;
+		rc = cifs_revalidate_dentry(direntry);
+		if (rc) {
+			cifs_dbg(FYI, "cifs_revalidate_dentry failed with rc=%d", rc);
+			return rc == -ENOENT ? 0 : rc;
+		}
 		else {
 			/*
 			 * If the inode wasn't known to be a dfs entry when