overlayfs: During copy up, first copy up data and then xattrs

If a file with capability set (and hence security.capability xattr) is written kernel clears security.capability xattr. For overlay, during file copy up if xattrs are copied up first and then data is, copied up. This means data copy up will result in clearing of security.capability xattr file on lower has. And this can result into surprises. If
a lower file has CAP_SETUID, then it should not be cleared over copy up (if nothing was actually written to file).

This also creates problems with chown logic where it first copies up file and then tries to clear setuid bit. But by that time security.capability xattr is already gone (due to data copy up), and caller gets -ENODATA. This has been reported by Giuseppe here.

https://github.com/containers/libpod/issues/2015#issuecomment-447824842

Fix this by copying up data first and then metadta. This is a regression which has been introduced by my commit as part of metadata only copy up patches.

TODO: There will be some corner cases where a file is copied up metadata
only and later data copy up happens and that will clear security.capability xattr. Something needs to be done about that too.

Fixes bd64e57586d3 ("ovl: During copy up, first copy up metadata and then data")

Reported-by Giuseppe Scrivano <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/copy_up.c |   30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

Index: rhvgoyal-linux/fs/overlayfs/copy_up.c
===================================================================

fs/overlayfs/copy_up.c 2019-01-10 13:29:05.997079686 -0500 renamed from fs/overlayfs/copy_up.c 2018-11-28 08:45:58.122478207 -0500 +7 -0

@@ -443,6 +443,23 @@ static int ovl_copy_up_inode(struct ovl_
 {
        int err;
 
+       /*
+        * Copy up data first and then xattrs. Writing data after
+        * xattrs will remove security.capability xattr automatically.
+        */
+       if (S_ISREG(c->stat.mode) && !c->metacopy) {
+               struct path upperpath, datapath;
+

Quoted 51 lines · Vivek Goyal

If a file with capability set (and hence security.capability xattr) is written kernel clears security.capability xattr. For overlay, during file copy up if xattrs are copied up first and then data is, copied up. This means data copy up will result in clearing of security.capability xattr file on lower has. And this can result into surprises. If
a lower file has CAP_SETUID, then it should not be cleared over copy up (if nothing was actually written to file).

This also creates problems with chown logic where it first copies up file and then tries to clear setuid bit. But by that time security.capability xattr is already gone (due to data copy up), and caller gets -ENODATA. This has been reported by Giuseppe here.

https://github.com/containers/libpod/issues/2015#issuecomment-447824842

Fix this by copying up data first and then metadta. This is a regression which has been introduced by my commit as part of metadata only copy up patches.

TODO: There will be some corner cases where a file is copied up metadata
only and later data copy up happens and that will clear security.capability xattr. Something needs to be done about that too.

Fixes bd64e57586d3 ("ovl: During copy up, first copy up metadata and then data")

Reported-by Giuseppe Scrivano <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/copy_up.c |   30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

Index: rhvgoyal-linux/fs/overlayfs/copy_up.c
===================================================================

fs/overlayfs/copy_up.c 2019-01-10 13:29:05.997079686 -0500 renamed from fs/overlayfs/copy_up.c 2018-11-28 08:45:58.122478207 -0500 +12 -0

@@ -443,6 +443,23 @@ static int ovl_copy_up_inode(struct ovl_
 {
        int err;
 
+       /*
+        * Copy up data first and then xattrs. Writing data after
+        * xattrs will remove security.capability xattr automatically.
+        */
+       if (S_ISREG(c->stat.mode) && !c->metacopy) {
+               struct path upperpath, datapath;
+
+               ovl_path_upper(c->dentry, &upperpath);
+               BUG_ON(upperpath.dentry != NULL);
+               upperpath.dentry = temp;
+
+               ovl_path_lowerdata(c->dentry, &datapath);

fs/overlayfs/copy_up.c 2019-01-10 13:29:05.997079686 -0500 static int ovl_copy_up_inode(struct ovl_ · line 462

Quoted 5 lines

fs/overlayfs/copy_up.c 2019-01-10 13:29:05.997079686 -0500 renamed from fs/overlayfs/copy_up.c 2018-11-28 08:45:58.122478207 -0500 +5 -0

+               err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
+               if (err)
+                       return err;
+       }
+

I know you just moved this code around, but please change it to if (WARN_ON(...)) return -EIO;

If it was up to me, I would change the call arguments to:

              err = ovl_copy_up_data(c, temp);

and compose the paths inside ovl_copy_up_data().
There are more similarities to both call sites than there are differences.