[PATCH] overlay: test unstable inode number

[PATCH] overlay: test unstable inode number

[Amir Goldstein]

Introduce a new test to demonstrate a known issue with overlayfs:
- stat file A shows inode number X
- modify A to trigger copy up
- stat file A shows inode number Y != X

Also test if d_ino of readdir entries changes after copy up.

There is a POC patch by Miklos Szeredi that fixes this issue.
---
 tests/overlay/018     | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/018.out |  5 +++
 tests/overlay/group   |  1 +
 3 files changed, 102 insertions(+)
 create mode 100755 tests/overlay/018
 create mode 100644 tests/overlay/018.out

diff --git a/tests/overlay/018 b/tests/overlay/018
new file mode 100755
index 0000000..4b358e5
--- /dev/null
+++ b/tests/overlay/018
@@ -0,0 +1,96 @@
+#! /bin/bash
+# FSQA Test No. 018
+#
+# Test unstable inode number
+#
+# This simple test demonstrates a known issue with overlayfs:
+# - stat file A shows inode number X
+# - modify A to trigger copy up
+# - stat file A shows inode number Y != X
+#
+# Also test if d_ino of readdir entries changes after copy up.
+#
+#-----------------------------------------------------------------------
+#
+# Copyright (C) 2016 CTERA Networks. All Rights Reserved.
+# Author: Amir Goldstein <amir73il@gmail.com>
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# real QA test starts here
+_supported_fs overlay
+_supported_os Linux
+_require_scratch
+
+rm -f $seqres.full
+
+_scratch_mkfs >>$seqres.full 2>&1
+
+# Create our test files
+lowerdir=$SCRATCH_DEV/$OVERLAY_LOWER_DIR
+mkdir -p $lowerdir
+mkdir $lowerdir/dir
+touch $lowerdir/file
+ln -s $lowerdir/file $lowerdir/symlink
+mkfifo $lowerdir/fifo
+
+
+_scratch_mount
+
+cd $SCRATCH_MNT
+
+rm -f $tmp.before $tmp.after
+
+# Test stable stat(2) st_ino
+
+# Record inode numbers before and after copy up
+for f in dir file symlink fifo; do
+	ls -id $f >> $tmp.before
+	# chown -h modifies all those file types
+	chown -h fsgqa $f
+	ls -id $f >> $tmp.after
+done
+
+# Test stable readdir(3)/getdents(2) d_ino
+
+# find by inode number - expect to find file by inode number
+cat $tmp.before | while read ino f; do
+	find $f -inum $ino -maxdepth 0
+done
+
+# Compare before..after - expect silence
+diff $tmp.before $tmp.after
+
+status=0
+exit
diff --git a/tests/overlay/018.out b/tests/overlay/018.out
new file mode 100644
index 0000000..75e80e7
--- /dev/null
+++ b/tests/overlay/018.out
@@ -0,0 +1,5 @@
+QA output created by 018
+dir
+file
+symlink
+fifo
diff --git a/tests/overlay/group b/tests/overlay/group
index 5740d2a..683b2fc 100644
--- a/tests/overlay/group
+++ b/tests/overlay/group
@@ -19,3 +19,4 @@
 014 auto quick copyup
 015 auto quick whiteout
 016 auto quick
+018 auto quick
-- 
2.7.4

Re: [PATCH] overlay: test unstable inode number

[Eryu Guan]

On Sun, Dec 11, 2016 at 06:38:00PM +0200, Amir Goldstein wrote:
> Introduce a new test to demonstrate a known issue with overlayfs:
> - stat file A shows inode number X
> - modify A to trigger copy up
> - stat file A shows inode number Y != X
> 
> Also test if d_ino of readdir entries changes after copy up.
> 
> There is a POC patch by Miklos Szeredi that fixes this issue.
> ---
>  tests/overlay/018     | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/018.out |  5 +++
>  tests/overlay/group   |  1 +
>  3 files changed, 102 insertions(+)
>  create mode 100755 tests/overlay/018
>  create mode 100644 tests/overlay/018.out
> 
> diff --git a/tests/overlay/018 b/tests/overlay/018
> new file mode 100755
> index 0000000..4b358e5
> --- /dev/null
> +++ b/tests/overlay/018
> @@ -0,0 +1,96 @@
> +#! /bin/bash
> +# FSQA Test No. 018
> +#
> +# Test unstable inode number
> +#
> +# This simple test demonstrates a known issue with overlayfs:
> +# - stat file A shows inode number X
> +# - modify A to trigger copy up
> +# - stat file A shows inode number Y != X
> +#
> +# Also test if d_ino of readdir entries changes after copy up.
> +#
> +#-----------------------------------------------------------------------
> +#
> +# Copyright (C) 2016 CTERA Networks. All Rights Reserved.
> +# Author: Amir Goldstein <amir73il@gmail.com>
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_fs overlay
> +_supported_os Linux

Need a "_require_user" here.

> +_require_scratch
> +
> +rm -f $seqres.full
> +
> +_scratch_mkfs >>$seqres.full 2>&1
> +
> +# Create our test files
> +lowerdir=$SCRATCH_DEV/$OVERLAY_LOWER_DIR
> +mkdir -p $lowerdir
> +mkdir $lowerdir/dir
> +touch $lowerdir/file
> +ln -s $lowerdir/file $lowerdir/symlink
> +mkfifo $lowerdir/fifo

Should hardlink and device-special be tested too?

Thanks,
Eryu

Re: [PATCH] overlay: test unstable inode number

[Amir Goldstein]

On Mon, Dec 12, 2016 at 7:44 AM, Eryu Guan <eguan@redhat.com> wrote:
> On Sun, Dec 11, 2016 at 06:38:00PM +0200, Amir Goldstein wrote:
>> Introduce a new test to demonstrate a known issue with overlayfs:
>> - stat file A shows inode number X
>> - modify A to trigger copy up
>> - stat file A shows inode number Y != X
>>
>> Also test if d_ino of readdir entries changes after copy up.
>>
>> There is a POC patch by Miklos Szeredi that fixes this issue.
>> ---
>>  tests/overlay/018     | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/overlay/018.out |  5 +++
>>  tests/overlay/group   |  1 +
>>  3 files changed, 102 insertions(+)
>>  create mode 100755 tests/overlay/018
>>  create mode 100644 tests/overlay/018.out
>>
>> diff --git a/tests/overlay/018 b/tests/overlay/018
>> new file mode 100755
>> index 0000000..4b358e5
>> --- /dev/null
>> +++ b/tests/overlay/018
>> @@ -0,0 +1,96 @@
>> +#! /bin/bash
>> +# FSQA Test No. 018
>> +#
>> +# Test unstable inode number
>> +#
>> +# This simple test demonstrates a known issue with overlayfs:
>> +# - stat file A shows inode number X
>> +# - modify A to trigger copy up
>> +# - stat file A shows inode number Y != X
>> +#
>> +# Also test if d_ino of readdir entries changes after copy up.
>> +#
>> +#-----------------------------------------------------------------------
>> +#
>> +# Copyright (C) 2016 CTERA Networks. All Rights Reserved.
>> +# Author: Amir Goldstein <amir73il@gmail.com>
>> +#
>> +# This program is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU General Public License as
>> +# published by the Free Software Foundation.
>> +#
>> +# This program is distributed in the hope that it would be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write the Free Software Foundation,
>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> +#-----------------------------------------------------------------------
>> +#
>> +
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +tmp=/tmp/$$
>> +status=1     # failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +     cd /
>> +     rm -f $tmp.*
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +
>> +# real QA test starts here
>> +_supported_fs overlay
>> +_supported_os Linux
>
> Need a "_require_user" here.

Right, so I'll use an arbitrary uid instead, cause the value of uid is
not the issue.
v2 coming right up.

>
>> +_require_scratch
>> +
>> +rm -f $seqres.full
>> +
>> +_scratch_mkfs >>$seqres.full 2>&1
>> +
>> +# Create our test files
>> +lowerdir=$SCRATCH_DEV/$OVERLAY_LOWER_DIR
>> +mkdir -p $lowerdir
>> +mkdir $lowerdir/dir
>> +touch $lowerdir/file
>> +ln -s $lowerdir/file $lowerdir/symlink
>> +mkfifo $lowerdir/fifo
>
> Should hardlink and device-special be tested too?
>

Hardlink is a more complex issue, which is not solved by the POC patch
so I am leaving it to a test of its own.
Hardlinks should not only preserve inode number, but also nlink and actually
refer to same upper inode. The POC patch even breaks that brutally, by
making all broken hardlink clones apear to have the same inode number,
but actually they are completely different files/inodes.

fifo is a representative of the device special files.
Since overlayfs only ever tests for
special_file(m) := (S_ISCHR(m)||S_ISBLK(m)||S_ISFIFO(m)||S_ISSOCK(m))
and treats all special files the same,
it seemed excessive to test all of those types individually.

Amir.

Re: [PATCH] overlay: test unstable inode number

[Eryu Guan]

On Mon, Dec 12, 2016 at 09:47:47AM +0200, Amir Goldstein wrote:
> On Mon, Dec 12, 2016 at 7:44 AM, Eryu Guan <eguan@redhat.com> wrote:
> > On Sun, Dec 11, 2016 at 06:38:00PM +0200, Amir Goldstein wrote:
> >> Introduce a new test to demonstrate a known issue with overlayfs:
> >> - stat file A shows inode number X
> >> - modify A to trigger copy up
> >> - stat file A shows inode number Y != X
> >>
> >> Also test if d_ino of readdir entries changes after copy up.
> >>
> >> There is a POC patch by Miklos Szeredi that fixes this issue.
> >> ---
> >>  tests/overlay/018     | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  tests/overlay/018.out |  5 +++
> >>  tests/overlay/group   |  1 +
> >>  3 files changed, 102 insertions(+)
> >>  create mode 100755 tests/overlay/018
> >>  create mode 100644 tests/overlay/018.out
> >>
> >> diff --git a/tests/overlay/018 b/tests/overlay/018
> >> new file mode 100755
> >> index 0000000..4b358e5
> >> --- /dev/null
> >> +++ b/tests/overlay/018
> >> @@ -0,0 +1,96 @@
> >> +#! /bin/bash
> >> +# FSQA Test No. 018
> >> +#
> >> +# Test unstable inode number
> >> +#
> >> +# This simple test demonstrates a known issue with overlayfs:
> >> +# - stat file A shows inode number X
> >> +# - modify A to trigger copy up
> >> +# - stat file A shows inode number Y != X
> >> +#
> >> +# Also test if d_ino of readdir entries changes after copy up.
> >> +#
> >> +#-----------------------------------------------------------------------
> >> +#
> >> +# Copyright (C) 2016 CTERA Networks. All Rights Reserved.
> >> +# Author: Amir Goldstein <amir73il@gmail.com>
> >> +#
> >> +# This program is free software; you can redistribute it and/or
> >> +# modify it under the terms of the GNU General Public License as
> >> +# published by the Free Software Foundation.
> >> +#
> >> +# This program is distributed in the hope that it would be useful,
> >> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> +# GNU General Public License for more details.
> >> +#
> >> +# You should have received a copy of the GNU General Public License
> >> +# along with this program; if not, write the Free Software Foundation,
> >> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> >> +#-----------------------------------------------------------------------
> >> +#
> >> +
> >> +seq=`basename $0`
> >> +seqres=$RESULT_DIR/$seq
> >> +echo "QA output created by $seq"
> >> +
> >> +tmp=/tmp/$$
> >> +status=1     # failure is the default!
> >> +trap "_cleanup; exit \$status" 0 1 2 3 15
> >> +
> >> +_cleanup()
> >> +{
> >> +     cd /
> >> +     rm -f $tmp.*
> >> +}
> >> +
> >> +# get standard environment, filters and checks
> >> +. ./common/rc
> >> +. ./common/filter
> >> +
> >> +# real QA test starts here
> >> +_supported_fs overlay
> >> +_supported_os Linux
> >
> > Need a "_require_user" here.
> 
> Right, so I'll use an arbitrary uid instead, cause the value of uid is
> not the issue.
> v2 coming right up.
> 
> >
> >> +_require_scratch
> >> +
> >> +rm -f $seqres.full
> >> +
> >> +_scratch_mkfs >>$seqres.full 2>&1
> >> +
> >> +# Create our test files
> >> +lowerdir=$SCRATCH_DEV/$OVERLAY_LOWER_DIR
> >> +mkdir -p $lowerdir
> >> +mkdir $lowerdir/dir
> >> +touch $lowerdir/file
> >> +ln -s $lowerdir/file $lowerdir/symlink
> >> +mkfifo $lowerdir/fifo
> >
> > Should hardlink and device-special be tested too?
> >
> 
> Hardlink is a more complex issue, which is not solved by the POC patch
> so I am leaving it to a test of its own.
> Hardlinks should not only preserve inode number, but also nlink and actually
> refer to same upper inode. The POC patch even breaks that brutally, by

I think the hardlinks are known to be broken on copy-up,
Documentation/filesystems/overlayfs.txt documented this behavior

"
If a file with multiple hard links is copied up, then this will
"break" the link.  Changes will not be propagated to other names
referring to the same inode.
"

> making all broken hardlink clones apear to have the same inode number,
> but actually they are completely different files/inodes.

So after the POC patch, the hardlinks are still broken but having the
same inode number and making the situation more complext :)

How about adding some comments on why hardlinks are not tested?

> 
> fifo is a representative of the device special files.
> Since overlayfs only ever tests for
> special_file(m) := (S_ISCHR(m)||S_ISBLK(m)||S_ISFIFO(m)||S_ISSOCK(m))
> and treats all special files the same,
> it seemed excessive to test all of those types individually.

The handles of these file types might be changed in future, and adding
tests of these file types are easy to do, I'd prefer having a full test
coverage of them.

Thanks,
Eryu

Re: [PATCH] overlay: test unstable inode number

[Amir Goldstein]

On Mon, Dec 12, 2016 at 10:16 AM, Eryu Guan <eguan@redhat.com> wrote:
> On Mon, Dec 12, 2016 at 09:47:47AM +0200, Amir Goldstein wrote:
>> On Mon, Dec 12, 2016 at 7:44 AM, Eryu Guan <eguan@redhat.com> wrote:
>> > On Sun, Dec 11, 2016 at 06:38:00PM +0200, Amir Goldstein wrote:
>> >> Introduce a new test to demonstrate a known issue with overlayfs:
>> >> - stat file A shows inode number X
>> >> - modify A to trigger copy up
>> >> - stat file A shows inode number Y != X
>> >>
>> >> Also test if d_ino of readdir entries changes after copy up.
>> >>
>> >> There is a POC patch by Miklos Szeredi that fixes this issue.
>> >> ---
>> >>  tests/overlay/018     | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  tests/overlay/018.out |  5 +++
>> >>  tests/overlay/group   |  1 +
>> >>  3 files changed, 102 insertions(+)
>> >>  create mode 100755 tests/overlay/018
>> >>  create mode 100644 tests/overlay/018.out
>> >>
>> >> diff --git a/tests/overlay/018 b/tests/overlay/018
>> >> new file mode 100755
>> >> index 0000000..4b358e5
>> >> --- /dev/null
>> >> +++ b/tests/overlay/018
>> >> @@ -0,0 +1,96 @@
>> >> +#! /bin/bash
>> >> +# FSQA Test No. 018
>> >> +#
>> >> +# Test unstable inode number
>> >> +#
>> >> +# This simple test demonstrates a known issue with overlayfs:
>> >> +# - stat file A shows inode number X
>> >> +# - modify A to trigger copy up
>> >> +# - stat file A shows inode number Y != X
>> >> +#
>> >> +# Also test if d_ino of readdir entries changes after copy up.
>> >> +#
>> >> +#-----------------------------------------------------------------------
>> >> +#
>> >> +# Copyright (C) 2016 CTERA Networks. All Rights Reserved.
>> >> +# Author: Amir Goldstein <amir73il@gmail.com>
>> >> +#
>> >> +# This program is free software; you can redistribute it and/or
>> >> +# modify it under the terms of the GNU General Public License as
>> >> +# published by the Free Software Foundation.
>> >> +#
>> >> +# This program is distributed in the hope that it would be useful,
>> >> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> >> +# GNU General Public License for more details.
>> >> +#
>> >> +# You should have received a copy of the GNU General Public License
>> >> +# along with this program; if not, write the Free Software Foundation,
>> >> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> >> +#-----------------------------------------------------------------------
>> >> +#
>> >> +
>> >> +seq=`basename $0`
>> >> +seqres=$RESULT_DIR/$seq
>> >> +echo "QA output created by $seq"
>> >> +
>> >> +tmp=/tmp/$$
>> >> +status=1     # failure is the default!
>> >> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> >> +
>> >> +_cleanup()
>> >> +{
>> >> +     cd /
>> >> +     rm -f $tmp.*
>> >> +}
>> >> +
>> >> +# get standard environment, filters and checks
>> >> +. ./common/rc
>> >> +. ./common/filter
>> >> +
>> >> +# real QA test starts here
>> >> +_supported_fs overlay
>> >> +_supported_os Linux
>> >
>> > Need a "_require_user" here.
>>
>> Right, so I'll use an arbitrary uid instead, cause the value of uid is
>> not the issue.
>> v2 coming right up.
>>
>> >
>> >> +_require_scratch
>> >> +
>> >> +rm -f $seqres.full
>> >> +
>> >> +_scratch_mkfs >>$seqres.full 2>&1
>> >> +
>> >> +# Create our test files
>> >> +lowerdir=$SCRATCH_DEV/$OVERLAY_LOWER_DIR
>> >> +mkdir -p $lowerdir
>> >> +mkdir $lowerdir/dir
>> >> +touch $lowerdir/file
>> >> +ln -s $lowerdir/file $lowerdir/symlink
>> >> +mkfifo $lowerdir/fifo
>> >
>> > Should hardlink and device-special be tested too?
>> >
>>
>> Hardlink is a more complex issue, which is not solved by the POC patch
>> so I am leaving it to a test of its own.
>> Hardlinks should not only preserve inode number, but also nlink and actually
>> refer to same upper inode. The POC patch even breaks that brutally, by
>
> I think the hardlinks are known to be broken on copy-up,
> Documentation/filesystems/overlayfs.txt documented this behavior
>
> "
> If a file with multiple hard links is copied up, then this will
> "break" the link.  Changes will not be propagated to other names
> referring to the same inode.
> "
>
>> making all broken hardlink clones apear to have the same inode number,
>> but actually they are completely different files/inodes.
>
> So after the POC patch, the hardlinks are still broken but having the
> same inode number and making the situation more complext :)
>
> How about adding some comments on why hardlinks are not tested?
>

Sure.

>>
>> fifo is a representative of the device special files.
>> Since overlayfs only ever tests for
>> special_file(m) := (S_ISCHR(m)||S_ISBLK(m)||S_ISFIFO(m)||S_ISSOCK(m))
>> and treats all special files the same,
>> it seemed excessive to test all of those types individually.
>
> The handles of these file types might be changed in future, and adding
> tests of these file types are easy to do, I'd prefer having a full test
> coverage of them.
>

OK.