[PATCH] ovl: create UUIDs for file systems that do not set the superblock UUID

[PATCH] ovl: create UUIDs for file systems that do not set the superblock UUID

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

Some file systems such as squashfs do not set the UUID in the
superblock resulting in a zero'd UUID.  In cases were two or more
of these file systems are overlayed on the lower layer we can hit
overlay corruption issues because identical zero'd overlayfs UUIDs
are impossible to differentiate between.  This can be fixed by
creating an overlayfs UUID based on the file system from the
superblock s_magic and s_dev fields.  (This currently seems like
enough information to be able create a UUID, but the could be
scope to use other super block fields such as the pointer s_fs_info
but may need some obfuscation).

This issue can be reproduced with the following commands:

mkdir -p /cdrom
mount -t iso9660 -o ro,noatime /dev/sr0 /cdrom
sleep 1
mkdir -p /cow
mount -t tmpfs -o 'rw,noatime,mode=755' tmpfs /cow
mkdir -p /cow/upper
mkdir -p /cow/work
modprobe -q -b overlay
modprobe -q -b loop
dev=$(losetup -f)
mkdir -p /filesystem.squashfs
losetup $dev /cdrom/casper/filesystem.squashfs
mount -t squashfs -o ro,noatime $dev /filesystem.squashfs
dev=$(losetup -f)
mkdir -p /installer.squashfs
losetup $dev /cdrom/casper/installer.squashfs
mount -t squashfs -o ro,noatime $dev /installer.squashfs
mkdir -p /root-tmp
mount -t overlay -o 'upperdir=/cow/upper,lowerdir=/installer.squashfs:/filesystem.squashfs,workdir=/cow/work' /cow /root-tmp

FILE=/root-tmp/etc/.pwd.lock

echo foo > $FILE
cat $FILE
sync
echo 3 > /proc/sys/vm/drop_caches
cat $FILE

The output from cat $FILE:
cat: /root-tmp/etc/.pwd.lock: Input/output error

dmesg reports:
[ 42.415432] overlayfs: invalid origin (etc/.pwd.lock, ftype=8000, origin ftype=4000).

BugLink: https://bugs.launchpad.net/bugs/1824407
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 fs/overlayfs/copy_up.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index b801c6353100..a578db87936b 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -231,6 +231,7 @@ struct ovl_fh *ovl_encode_real_fh(struct dentry *real, bool is_upper)
 	void *buf;
 	int buflen = MAX_HANDLE_SZ;
 	uuid_t *uuid = &real->d_sb->s_uuid;
+	static const uuid_t z_uuid;
 
 	buf = kmalloc(buflen, GFP_KERNEL);
 	if (!buf)
@@ -272,7 +273,20 @@ struct ovl_fh *ovl_encode_real_fh(struct dentry *real, bool is_upper)
 	if (is_upper)
 		fh->flags |= OVL_FH_FLAG_PATH_UPPER;
 	fh->len = fh_len;
-	fh->uuid = *uuid;
+
+	if (uuid_equal(uuid, &z_uuid)) {
+		/*
+		 * An zero'd uuid indicates the uuid in the super block was
+		 * not set by the file system, so fake one instead
+		 */
+		struct super_block *sb = real->d_sb;
+
+		memcpy(&fh->uuid.b[0], &sb->s_magic, 8);
+		memcpy(&fh->uuid.b[8], &sb->s_dev, 8);
+	} else {
+		fh->uuid = *uuid;
+	}
+
 	memcpy(fh->fid, buf, buflen);
 
 out:
-- 
2.20.1

Re: [PATCH] ovl: create UUIDs for file systems that do not set the superblock UUID

[Amir Goldstein]

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

On Thu, Nov 7, 2019 at 1:43 AM Colin King <colin.king@canonical.com> wrote:
>
> From: Colin Ian King <colin.king@canonical.com>
>
> Some file systems such as squashfs do not set the UUID in the
> superblock resulting in a zero'd UUID.  In cases were two or more
> of these file systems are overlayed on the lower layer we can hit
> overlay corruption issues because identical zero'd overlayfs UUIDs
> are impossible to differentiate between.  This can be fixed by
> creating an overlayfs UUID based on the file system from the
> superblock s_magic and s_dev fields.  (This currently seems like
> enough information to be able create a UUID, but the could be
> scope to use other super block fields such as the pointer s_fs_info
> but may need some obfuscation).
>

The fix is incorrent. uuid stored in xattr needs to have persistent properties.
In the use case that you describe, the origin file handle should simply be
ignored.

Please test attached patch.

Thanks,
Amir.

[-- Attachment #2: 0001-ovl-fix-lookup-failure-on-multi-lower-squashfs.patch --]
[-- Type: text/x-patch, Size: 4073 bytes --]

From e047b078157691ba8abd598865d7c6f08aab5310 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Thu, 7 Nov 2019 07:19:55 +0200
Subject: [PATCH] ovl: fix lookup failure on multi lower squashfs

In the past, overlayfs required that lower fs have non null uuid in
order to support nfs export and decode copy up origin file handles.

Commit 9df085f3c9a2 ("ovl: relax requirement for non null uuid of
lower fs") relaxed this requirement for nfs export support, as long
as uuid (even if null) is unique among all lower fs.

However, said commit unintentionally also relaxed the non null uuid
requirement for decoding copy up origin file handles, regardless of
the unique uuid requirement.

Amend this mistake by disabling decoding of copy up origin file handle
from lower fs with a conflicting uuid.

We still encode copy up origin file handles from those fs, because
file handles like those already exist in the wild and because they
might provide useful information in the future.

Reported-by: Colin Ian King <colin.king@canonical.com>
Link: https://lore.kernel.org/lkml/20191106234301.283006-1-colin.king@canonical.com/
Fixes: 9df085f3c9a2 ("ovl: relax requirement for non null uuid ...")
Cc: stable@vger.kernel.org # v4.20+
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c     | 8 ++++++++
 fs/overlayfs/ovl_entry.h | 2 ++
 fs/overlayfs/super.c     | 9 ++++++++-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index e9717c2f7d45..ddc5514d8c3b 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -325,6 +325,14 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
 	int i;
 
 	for (i = 0; i < ofs->numlower; i++) {
+		/*
+		 * If lower fs uuid is not unique among lower fs we cannot match
+		 * fh->uuid to layer.
+		 */
+		if (ofs->lower_layers[i].fsid &&
+		    ofs->lower_layers[i].fs->nouuid)
+			continue;
+
 		origin = ovl_decode_real_fh(fh, ofs->lower_layers[i].mnt,
 					    connected);
 		if (origin)
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index a8279280e88d..a0227c31fe17 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -22,6 +22,8 @@ struct ovl_config {
 struct ovl_sb {
 	struct super_block *sb;
 	dev_t pseudo_dev;
+	/* Unusable (conflicting) uuid */
+	bool nouuid;
 };
 
 struct ovl_layer {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index afbcb116a7f1..547f8cdbc98f 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1263,9 +1263,13 @@ static bool ovl_lower_uuid_ok(struct ovl_fs *ofs, const uuid_t *uuid)
 		 * We use uuid to associate an overlay lower file handle with a
 		 * lower layer, so we can accept lower fs with null uuid as long
 		 * as all lower layers with null uuid are on the same fs.
+		 * if we detect multiple lower fs with the same uuid, we
+		 * disable lower file handle decoding on all of them.
 		 */
-		if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid))
+		if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid)) {
+			ofs->lower_fs[i].nouuid = true;
 			return false;
+		}
 	}
 	return true;
 }
@@ -1277,6 +1281,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
 	unsigned int i;
 	dev_t dev;
 	int err;
+	bool nouuid = false;
 
 	/* fsid 0 is reserved for upper fs even with non upper overlay */
 	if (ofs->upper_mnt && ofs->upper_mnt->mnt_sb == sb)
@@ -1288,6 +1293,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
 	}
 
 	if (!ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
+		nouuid = true;
 		ofs->config.index = false;
 		ofs->config.nfs_export = false;
 		pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
@@ -1303,6 +1309,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
 
 	ofs->lower_fs[ofs->numlowerfs].sb = sb;
 	ofs->lower_fs[ofs->numlowerfs].pseudo_dev = dev;
+	ofs->lower_fs[ofs->numlowerfs].nouuid = nouuid;
 	ofs->numlowerfs++;
 
 	return ofs->numlowerfs;
-- 
2.17.1

Re: [PATCH] ovl: create UUIDs for file systems that do not set the superblock UUID

[Dan Carpenter]

On Thu, Nov 07, 2019 at 09:08:44AM +0200, Amir Goldstein wrote:
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index a8279280e88d..a0227c31fe17 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -22,6 +22,8 @@ struct ovl_config {
>  struct ovl_sb {
>  	struct super_block *sb;
>  	dev_t pseudo_dev;
> +	/* Unusable (conflicting) uuid */
> +	bool nouuid;

Could we name this ignore_uuid, skip_uuid or bad_uuid or something?
Otherwise we end up with double negatives.

regards,
dan carpenter

Re: [PATCH] ovl: create UUIDs for file systems that do not set the superblock UUID

[Colin Ian King]

On 07/11/2019 07:08, Amir Goldstein wrote:
> On Thu, Nov 7, 2019 at 1:43 AM Colin King <colin.king@canonical.com> wrote:
>>
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Some file systems such as squashfs do not set the UUID in the
>> superblock resulting in a zero'd UUID.  In cases were two or more
>> of these file systems are overlayed on the lower layer we can hit
>> overlay corruption issues because identical zero'd overlayfs UUIDs
>> are impossible to differentiate between.  This can be fixed by
>> creating an overlayfs UUID based on the file system from the
>> superblock s_magic and s_dev fields.  (This currently seems like
>> enough information to be able create a UUID, but the could be
>> scope to use other super block fields such as the pointer s_fs_info
>> but may need some obfuscation).
>>
> 
> The fix is incorrent. uuid stored in xattr needs to have persistent properties.
> In the use case that you describe, the origin file handle should simply be
> ignored.
> 
> Please test attached patch.

Thanks for the patch. Tested, and the error still occurs:

[  163.959633] overlayfs: invalid origin (etc/.pwd.lock, ftype=8000,
origin ftype=4000).

Colin

> 
> Thanks,
> Amir.
> 

Re: [PATCH] ovl: create UUIDs for file systems that do not set the superblock UUID

[Colin Ian King]

On 07/11/2019 08:45, Colin Ian King wrote:
> On 07/11/2019 07:08, Amir Goldstein wrote:
>> On Thu, Nov 7, 2019 at 1:43 AM Colin King <colin.king@canonical.com> wrote:
>>>
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> Some file systems such as squashfs do not set the UUID in the
>>> superblock resulting in a zero'd UUID.  In cases were two or more
>>> of these file systems are overlayed on the lower layer we can hit
>>> overlay corruption issues because identical zero'd overlayfs UUIDs
>>> are impossible to differentiate between.  This can be fixed by
>>> creating an overlayfs UUID based on the file system from the
>>> superblock s_magic and s_dev fields.  (This currently seems like
>>> enough information to be able create a UUID, but the could be
>>> scope to use other super block fields such as the pointer s_fs_info
>>> but may need some obfuscation).
>>>
>>
>> The fix is incorrent. uuid stored in xattr needs to have persistent properties.
>> In the use case that you describe, the origin file handle should simply be
>> ignored.
>>
>> Please test attached patch.
> 
> Thanks for the patch. Tested, and the error still occurs:
> 
> [  163.959633] overlayfs: invalid origin (etc/.pwd.lock, ftype=8000,
> origin ftype=4000).

Added debug, seems like nouuid is not being set to true, nouuid is false
on the layers 0 and 1.

> 
> Colin
> 
>>
>> Thanks,
>> Amir.
>>
> 

Re: [PATCH] ovl: create UUIDs for file systems that do not set the superblock UUID

[Colin Ian King]

On 07/11/2019 09:12, Colin Ian King wrote:
> On 07/11/2019 08:45, Colin Ian King wrote:
>> On 07/11/2019 07:08, Amir Goldstein wrote:
>>> On Thu, Nov 7, 2019 at 1:43 AM Colin King <colin.king@canonical.com> wrote:
>>>>
>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>
>>>> Some file systems such as squashfs do not set the UUID in the
>>>> superblock resulting in a zero'd UUID.  In cases were two or more
>>>> of these file systems are overlayed on the lower layer we can hit
>>>> overlay corruption issues because identical zero'd overlayfs UUIDs
>>>> are impossible to differentiate between.  This can be fixed by
>>>> creating an overlayfs UUID based on the file system from the
>>>> superblock s_magic and s_dev fields.  (This currently seems like
>>>> enough information to be able create a UUID, but the could be
>>>> scope to use other super block fields such as the pointer s_fs_info
>>>> but may need some obfuscation).
>>>>
>>>
>>> The fix is incorrent. uuid stored in xattr needs to have persistent properties.
>>> In the use case that you describe, the origin file handle should simply be
>>> ignored.
>>>
>>> Please test attached patch.
>>
>> Thanks for the patch. Tested, and the error still occurs:
>>
>> [  163.959633] overlayfs: invalid origin (etc/.pwd.lock, ftype=8000,
>> origin ftype=4000).
> 
> Added debug, seems like nouuid is not being set to true, nouuid is false
> on the layers 0 and 1.

So nouuid is not being set in ovl_lower_uuid_ok() because the code is
returning early because of the following statement:

if (!ofs->config.nfs_export && !(ofs->config.index && ofs->upper_mnt))
	return true;

..and not getting to the following for-loop.

> 
>>
>> Colin
>>
>>>
>>> Thanks,
>>> Amir.
>>>
>>
> 

Re: [PATCH] ovl: create UUIDs for file systems that do not set the superblock UUID

[Amir Goldstein]

On Thu, Nov 7, 2019 at 11:44 AM Colin Ian King <colin.king@canonical.com> wrote:
>
> On 07/11/2019 09:12, Colin Ian King wrote:
> > On 07/11/2019 08:45, Colin Ian King wrote:
> >> On 07/11/2019 07:08, Amir Goldstein wrote:
> >>> On Thu, Nov 7, 2019 at 1:43 AM Colin King <colin.king@canonical.com> wrote:
> >>>>
> >>>> From: Colin Ian King <colin.king@canonical.com>
> >>>>
> >>>> Some file systems such as squashfs do not set the UUID in the
> >>>> superblock resulting in a zero'd UUID.  In cases were two or more
> >>>> of these file systems are overlayed on the lower layer we can hit
> >>>> overlay corruption issues because identical zero'd overlayfs UUIDs
> >>>> are impossible to differentiate between.  This can be fixed by
> >>>> creating an overlayfs UUID based on the file system from the
> >>>> superblock s_magic and s_dev fields.  (This currently seems like
> >>>> enough information to be able create a UUID, but the could be
> >>>> scope to use other super block fields such as the pointer s_fs_info
> >>>> but may need some obfuscation).
> >>>>
> >>>
> >>> The fix is incorrent. uuid stored in xattr needs to have persistent properties.
> >>> In the use case that you describe, the origin file handle should simply be
> >>> ignored.
> >>>
> >>> Please test attached patch.
> >>
> >> Thanks for the patch. Tested, and the error still occurs:
> >>
> >> [  163.959633] overlayfs: invalid origin (etc/.pwd.lock, ftype=8000,
> >> origin ftype=4000).
> >
> > Added debug, seems like nouuid is not being set to true, nouuid is false
> > on the layers 0 and 1.
>
> So nouuid is not being set in ovl_lower_uuid_ok() because the code is
> returning early because of the following statement:
>
> if (!ofs->config.nfs_export && !(ofs->config.index && ofs->upper_mnt))
>         return true;
>
> ..and not getting to the following for-loop.
>

Indeed. I had this bit of information in my mind for a brief moment
and forgot about it..

Please remove this optimization and change the call to:

       if (ofs->upper_mnt && !ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
...

Maybe change the language of "falling back to index=off..." to
"enforcing index=off..."

You may then submit the patch with my Signed-off and yours.
Please also change the name nouuid to bad_uuid per Dan's review comment.

Thanks,
Amir.