[PATCH][SRU][Disco] UBUNTU: SAUCE: shiftfs: fix buggy unlink logic

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH][SRU][Disco] UBUNTU: SAUCE: shiftfs: fix buggy unlink logic

Christian Brauner-3
BugLink: https://bugs.launchpad.net/bugs/1841977

The way we messed with setting i_nlink was brittle and wrong. We used to
set the i_nlink of the shiftfs dentry to be deleted to the i_nlink count
of the underlay dentry of the directory it resided in which makes no
sense whatsoever. We also missed drop_nlink() which is crucial since
i_nlink affects whether a dentry is cleaned up on dput().
With this I cannot reproduce the bug anymore where shiftfs misleads zfs
into believing that a deleted file can not be removed from disk because
it is still referenced.

Fixes: commit 87011da41961 ("shiftfs: rework and extend")
Signed-off-by: Christian Brauner <[hidden email]>
---
 fs/shiftfs.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index 57e04a02d74c..084aa7a25566 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -585,6 +585,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
 {
  struct dentry *lowerd = dentry->d_fsdata;
  struct inode *loweri = dir->i_private;
+ struct inode *inode = d_inode(dentry);
  int err;
  const struct cred *oldcred;
 
@@ -594,15 +595,19 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
  err = vfs_rmdir(loweri, lowerd);
  else
  err = vfs_unlink(loweri, lowerd, NULL);
- inode_unlock(loweri);
  revert_creds(oldcred);
 
- shiftfs_copyattr(loweri, dir);
- set_nlink(d_inode(dentry), loweri->i_nlink);
- if (!err)
+ if (!err) {
  d_drop(dentry);
 
- set_nlink(dir, loweri->i_nlink);
+ if (rmdir)
+ clear_nlink(inode);
+ else
+ drop_nlink(inode);
+ }
+ inode_unlock(loweri);
+
+ shiftfs_copyattr(loweri, dir);
 
  return err;
 }
--
2.23.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK / APPLIED[E/Unstable]: [PATCH][SRU][Disco] UBUNTU: SAUCE: shiftfs: fix buggy unlink logic

Seth Forshee
On Thu, Aug 29, 2019 at 08:45:07PM +0200, Christian Brauner wrote:

> BugLink: https://bugs.launchpad.net/bugs/1841977
>
> The way we messed with setting i_nlink was brittle and wrong. We used to
> set the i_nlink of the shiftfs dentry to be deleted to the i_nlink count
> of the underlay dentry of the directory it resided in which makes no
> sense whatsoever. We also missed drop_nlink() which is crucial since
> i_nlink affects whether a dentry is cleaned up on dput().
> With this I cannot reproduce the bug anymore where shiftfs misleads zfs
> into believing that a deleted file can not be removed from disk because
> it is still referenced.

Obviously the intent was not to copy i_nlink from the corresponding
inode in the lower filesystem, not the directory inode. Seems likely
that some confusing/inconsistent variable naming contributed to the
mistake.

As I mentioned on irc, I'm conflicted on whether we should just
decrement the reference count like this or copy from the lower fs inode.
It should only matter if the lower fs is being modified underneath
shiftfs, and shiftfs cannot promise correct behavior if that happens, so
this should be ok.

Acked-by: Seth Forshee <[hidden email]>

Please remember to submit these for the development release as well.
Applied to eoan/master-next and unstable/master, thanks!

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH][SRU][Disco] UBUNTU: SAUCE: shiftfs: fix buggy unlink logic

Stefan Bader-2
In reply to this post by Christian Brauner-3
On 29.08.19 20:45, Christian Brauner wrote:

> BugLink: https://bugs.launchpad.net/bugs/1841977
>
> The way we messed with setting i_nlink was brittle and wrong. We used to
> set the i_nlink of the shiftfs dentry to be deleted to the i_nlink count
> of the underlay dentry of the directory it resided in which makes no
> sense whatsoever. We also missed drop_nlink() which is crucial since
> i_nlink affects whether a dentry is cleaned up on dput().
> With this I cannot reproduce the bug anymore where shiftfs misleads zfs
> into believing that a deleted file can not be removed from disk because
> it is still referenced.
>
> Fixes: commit 87011da41961 ("shiftfs: rework and extend")
> Signed-off-by: Christian Brauner <[hidden email]>
Acked-by: Stefan Bader <[hidden email]>

> ---
>  fs/shiftfs.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> index 57e04a02d74c..084aa7a25566 100644
> --- a/fs/shiftfs.c
> +++ b/fs/shiftfs.c
> @@ -585,6 +585,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
>  {
>   struct dentry *lowerd = dentry->d_fsdata;
>   struct inode *loweri = dir->i_private;
> + struct inode *inode = d_inode(dentry);
>   int err;
>   const struct cred *oldcred;
>  
> @@ -594,15 +595,19 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
>   err = vfs_rmdir(loweri, lowerd);
>   else
>   err = vfs_unlink(loweri, lowerd, NULL);
> - inode_unlock(loweri);
>   revert_creds(oldcred);
>  
> - shiftfs_copyattr(loweri, dir);
> - set_nlink(d_inode(dentry), loweri->i_nlink);
> - if (!err)
> + if (!err) {
>   d_drop(dentry);
>  
> - set_nlink(dir, loweri->i_nlink);
> + if (rmdir)
> + clear_nlink(inode);
> + else
> + drop_nlink(inode);
> + }
> + inode_unlock(loweri);
> +
> + shiftfs_copyattr(loweri, dir);
>  
>   return err;
>  }
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH][SRU][Disco] UBUNTU: SAUCE: shiftfs: fix buggy unlink logic

Connor Kuehl
In reply to this post by Christian Brauner-3
On 8/29/19 11:45 AM, Christian Brauner wrote:

> BugLink: https://bugs.launchpad.net/bugs/1841977
>
> The way we messed with setting i_nlink was brittle and wrong. We used to
> set the i_nlink of the shiftfs dentry to be deleted to the i_nlink count
> of the underlay dentry of the directory it resided in which makes no
> sense whatsoever. We also missed drop_nlink() which is crucial since
> i_nlink affects whether a dentry is cleaned up on dput().
> With this I cannot reproduce the bug anymore where shiftfs misleads zfs
> into believing that a deleted file can not be removed from disk because
> it is still referenced.
>
> Fixes: commit 87011da41961 ("shiftfs: rework and extend")
> Signed-off-by: Christian Brauner <[hidden email]>

Acked-by: Connor Kuehl <[hidden email]>

> ---
>   fs/shiftfs.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> index 57e04a02d74c..084aa7a25566 100644
> --- a/fs/shiftfs.c
> +++ b/fs/shiftfs.c
> @@ -585,6 +585,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
>   {
>   struct dentry *lowerd = dentry->d_fsdata;
>   struct inode *loweri = dir->i_private;
> + struct inode *inode = d_inode(dentry);
>   int err;
>   const struct cred *oldcred;
>  
> @@ -594,15 +595,19 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
>   err = vfs_rmdir(loweri, lowerd);
>   else
>   err = vfs_unlink(loweri, lowerd, NULL);
> - inode_unlock(loweri);
>   revert_creds(oldcred);
>  
> - shiftfs_copyattr(loweri, dir);
> - set_nlink(d_inode(dentry), loweri->i_nlink);
> - if (!err)
> + if (!err) {
>   d_drop(dentry);
>  
> - set_nlink(dir, loweri->i_nlink);
> + if (rmdir)
> + clear_nlink(inode);
> + else
> + drop_nlink(inode);
> + }
> + inode_unlock(loweri);
> +
> + shiftfs_copyattr(loweri, dir);
>  
>   return err;
>   }
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [PATCH][SRU][Disco] UBUNTU: SAUCE: shiftfs: fix buggy unlink logic

Kleber Souza
In reply to this post by Christian Brauner-3
On 29.08.19 20:45, Christian Brauner wrote:

> BugLink: https://bugs.launchpad.net/bugs/1841977
>
> The way we messed with setting i_nlink was brittle and wrong. We used to
> set the i_nlink of the shiftfs dentry to be deleted to the i_nlink count
> of the underlay dentry of the directory it resided in which makes no
> sense whatsoever. We also missed drop_nlink() which is crucial since
> i_nlink affects whether a dentry is cleaned up on dput().
> With this I cannot reproduce the bug anymore where shiftfs misleads zfs
> into believing that a deleted file can not be removed from disk because
> it is still referenced.
>
> Fixes: commit 87011da41961 ("shiftfs: rework and extend")
> Signed-off-by: Christian Brauner <[hidden email]>
> ---
>  fs/shiftfs.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> index 57e04a02d74c..084aa7a25566 100644
> --- a/fs/shiftfs.c
> +++ b/fs/shiftfs.c
> @@ -585,6 +585,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
>  {
>   struct dentry *lowerd = dentry->d_fsdata;
>   struct inode *loweri = dir->i_private;
> + struct inode *inode = d_inode(dentry);
>   int err;
>   const struct cred *oldcred;
>  
> @@ -594,15 +595,19 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
>   err = vfs_rmdir(loweri, lowerd);
>   else
>   err = vfs_unlink(loweri, lowerd, NULL);
> - inode_unlock(loweri);
>   revert_creds(oldcred);
>  
> - shiftfs_copyattr(loweri, dir);
> - set_nlink(d_inode(dentry), loweri->i_nlink);
> - if (!err)
> + if (!err) {
>   d_drop(dentry);
>  
> - set_nlink(dir, loweri->i_nlink);
> + if (rmdir)
> + clear_nlink(inode);
> + else
> + drop_nlink(inode);
> + }
> + inode_unlock(loweri);
> +
> + shiftfs_copyattr(loweri, dir);
>  
>   return err;
>  }
>


Applied to disco/master-next branch.

Thanks,
Kleber


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team