[PATCH] overlay: create directory over deleted whiteout

[PATCH] overlay: create directory over deleted whiteout

[Miklos Szeredi]

There's a bug in the overlayfs implementation starting from the very first
merged version that may cause an Oops of various forms if a directory is created
over a whiteout dentry, but the actual whiteout on the upper layer was removed
to the directory creation.

Reported by: kaixuxia <xiakaixu1987@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 tests/overlay/062     | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/062.out |  2 ++
 tests/overlay/group   |  1 +
 3 files changed, 63 insertions(+)
 create mode 100644 tests/overlay/062
 create mode 100644 tests/overlay/062.out

diff --git a/tests/overlay/062 b/tests/overlay/062
new file mode 100644
index 000000000000..b2cc350afefb
--- /dev/null
+++ b/tests/overlay/062
@@ -0,0 +1,60 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2018 Red Hat Inc.  All Rights Reserved.
+#
+# FS QA Test 062
+#
+# Create dir over cached negative dentry, but whiteout removed from upper
+#
+# The following kernel commit fixed the kernel crash: ???
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+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
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+_supported_fs overlay
+_supported_os Linux
+_require_scratch
+
+# Remove all files from previous tests
+_scratch_mkfs
+
+# Create test file
+lowerdir=${OVL_BASE_SCRATCH_MNT}/${OVL_LOWER}
+upperdir=${OVL_BASE_SCRATCH_MNT}/${OVL_UPPER}
+mkdir -p $lowerdir
+touch ${lowerdir}/file
+
+_scratch_mount
+
+rm ${SCRATCH_MNT}/file
+ls -l ${SCRATCH_MNT}/file > /dev/null 2>&1
+rm ${upperdir}/file
+mkdir ${SCRATCH_MNT}/file > /dev/null 2>&1
+
+# unmount overlayfs
+$UMOUNT_PROG $SCRATCH_MNT
+
+echo "Silence is golden"
+# success, all done
+status=0
+exit
diff --git a/tests/overlay/062.out b/tests/overlay/062.out
new file mode 100644
index 000000000000..a1578f4842ef
--- /dev/null
+++ b/tests/overlay/062.out
@@ -0,0 +1,2 @@
+QA output created by 062
+Silence is golden
diff --git a/tests/overlay/group b/tests/overlay/group
index ccc71f3bf846..0d3ceed3602d 100644
--- a/tests/overlay/group
+++ b/tests/overlay/group
@@ -64,3 +64,4 @@
 059 auto quick copyup
 060 auto quick metacopy
 061 auto quick copyup
+062 auto quick whiteout
-- 
2.14.3

Re: [PATCH] overlay: create directory over deleted whiteout

[Amir Goldstein]

On Wed, Oct 31, 2018 at 11:55 AM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> There's a bug in the overlayfs implementation starting from the very first
> merged version that may cause an Oops of various forms if a directory is created
> over a whiteout dentry, but the actual whiteout on the upper layer was removed
> to the directory creation.
>
> Reported by: kaixuxia <xiakaixu1987@gmail.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---

Looks good. A bit of commentary could be useful...

>  tests/overlay/062     | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/062.out |  2 ++
>  tests/overlay/group   |  1 +
>  3 files changed, 63 insertions(+)
>  create mode 100644 tests/overlay/062
>  create mode 100644 tests/overlay/062.out
>
> diff --git a/tests/overlay/062 b/tests/overlay/062
> new file mode 100644
> index 000000000000..b2cc350afefb
> --- /dev/null
> +++ b/tests/overlay/062
> @@ -0,0 +1,60 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2018 Red Hat Inc.  All Rights Reserved.
> +#
> +# FS QA Test 062
> +#
> +# Create dir over cached negative dentry, but whiteout removed from upper
> +#
> +# The following kernel commit fixed the kernel crash: ???
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +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
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +_supported_fs overlay
> +_supported_os Linux
> +_require_scratch
> +
> +# Remove all files from previous tests
> +_scratch_mkfs
> +
> +# Create test file
> +lowerdir=${OVL_BASE_SCRATCH_MNT}/${OVL_LOWER}
> +upperdir=${OVL_BASE_SCRATCH_MNT}/${OVL_UPPER}
> +mkdir -p $lowerdir
> +touch ${lowerdir}/file
> +
> +_scratch_mount
> +

# Create whiteout and populate dcache with negative dentry

> +rm ${SCRATCH_MNT}/file
> +ls -l ${SCRATCH_MNT}/file > /dev/null 2>&1

# Remove whiteout and try to create dir over negative dentry

> +rm ${upperdir}/file
> +mkdir ${SCRATCH_MNT}/file > /dev/null 2>&1
> +
> +# unmount overlayfs
> +$UMOUNT_PROG $SCRATCH_MNT

Umount of scratch is not needed at the end of the test.

Thanks,
Amir.

Re: [PATCH] overlay: create directory over deleted whiteout

[Miklos Szeredi]

On Wed, Oct 31, 2018 at 11:17 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Oct 31, 2018 at 11:55 AM Miklos Szeredi <mszeredi@redhat.com> wrote:
>>
>> There's a bug in the overlayfs implementation starting from the very first
>> merged version that may cause an Oops of various forms if a directory is created
>> over a whiteout dentry, but the actual whiteout on the upper layer was removed
>> to the directory creation.
>>
>> Reported by: kaixuxia <xiakaixu1987@gmail.com>
>> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>> ---
>
> Looks good. A bit of commentary could be useful...

Okay.

>> +
>> +# unmount overlayfs
>> +$UMOUNT_PROG $SCRATCH_MNT
>
> Umount of scratch is not needed at the end of the test.

It's not strictly needed, but gives additional assurance that things
hadn't gone bad (after the oops umount returns with EBUSY).

Thanks,
Miklos

Re: [PATCH] overlay: create directory over deleted whiteout

[Amir Goldstein]

On Wed, Oct 31, 2018 at 12:26 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, Oct 31, 2018 at 11:17 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Wed, Oct 31, 2018 at 11:55 AM Miklos Szeredi <mszeredi@redhat.com> wrote:
> >>
> >> There's a bug in the overlayfs implementation starting from the very first
> >> merged version that may cause an Oops of various forms if a directory is created
> >> over a whiteout dentry, but the actual whiteout on the upper layer was removed
> >> to the directory creation.
> >>
> >> Reported by: kaixuxia <xiakaixu1987@gmail.com>
> >> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> >> ---
> >
> > Looks good. A bit of commentary could be useful...
>
> Okay.
>
> >> +
> >> +# unmount overlayfs
> >> +$UMOUNT_PROG $SCRATCH_MNT
> >
> > Umount of scratch is not needed at the end of the test.
>
> It's not strictly needed, but gives additional assurance that things
> hadn't gone bad (after the oops umount returns with EBUSY).
>

But the test harness unmount the scratch mount anyway
and if you have fsck.overlay installed it also runs fsck.overay on the layers
after each test.

The question is why do you need the umount command in the test itself?
Is the output/result different without the explicit umount command in the test?

Thanks,
Amir.

Re: [PATCH] overlay: create directory over deleted whiteout

[Miklos Szeredi]

On Wed, Oct 31, 2018 at 11:33 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Oct 31, 2018 at 12:26 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>> On Wed, Oct 31, 2018 at 11:17 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> > On Wed, Oct 31, 2018 at 11:55 AM Miklos Szeredi <mszeredi@redhat.com> wrote:
>> >>
>> >> There's a bug in the overlayfs implementation starting from the very first
>> >> merged version that may cause an Oops of various forms if a directory is created
>> >> over a whiteout dentry, but the actual whiteout on the upper layer was removed
>> >> to the directory creation.
>> >>
>> >> Reported by: kaixuxia <xiakaixu1987@gmail.com>
>> >> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>> >> ---
>> >
>> > Looks good. A bit of commentary could be useful...
>>
>> Okay.
>>
>> >> +
>> >> +# unmount overlayfs
>> >> +$UMOUNT_PROG $SCRATCH_MNT
>> >
>> > Umount of scratch is not needed at the end of the test.
>>
>> It's not strictly needed, but gives additional assurance that things
>> hadn't gone bad (after the oops umount returns with EBUSY).
>>
>
> But the test harness unmount the scratch mount anyway
> and if you have fsck.overlay installed it also runs fsck.overay on the layers
> after each test.
>
> The question is why do you need the umount command in the test itself?
> Is the output/result different without the explicit umount command in the test?

The output is different, because the umount command fails if the mkdir crashed.

Thanks,
Miklos

Re: [PATCH] overlay: create directory over deleted whiteout

[Amir Goldstein]

On Wed, Oct 31, 2018 at 12:17 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Oct 31, 2018 at 11:55 AM Miklos Szeredi <mszeredi@redhat.com> wrote:
> >
> > There's a bug in the overlayfs implementation starting from the very first
> > merged version that may cause an Oops of various forms if a directory is created
> > over a whiteout dentry, but the actual whiteout on the upper layer was removed
> > to the directory creation.
> >
> > Reported by: kaixuxia <xiakaixu1987@gmail.com>
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
>
> Looks good. A bit of commentary could be useful...

Eryu,

I see you have not included this test in today's update.
Maybe you though that this discussion did not conclude.
On my part, besides adding the 2 comment lines, there
are no further concerns.

FYI, the fix for this bug is now in master:
5e1275808630 ovl: check whiteout in ovl_create_over_whiteout()

Thanks,
Amir.

>
> >  tests/overlay/062     | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/overlay/062.out |  2 ++
> >  tests/overlay/group   |  1 +
> >  3 files changed, 63 insertions(+)
> >  create mode 100644 tests/overlay/062
> >  create mode 100644 tests/overlay/062.out
> >
> > diff --git a/tests/overlay/062 b/tests/overlay/062
> > new file mode 100644
> > index 000000000000..b2cc350afefb
> > --- /dev/null
> > +++ b/tests/overlay/062
> > @@ -0,0 +1,60 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2018 Red Hat Inc.  All Rights Reserved.
> > +#
> > +# FS QA Test 062
> > +#
> > +# Create dir over cached negative dentry, but whiteout removed from upper
> > +#
> > +# The following kernel commit fixed the kernel crash: ???
> > +#
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +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
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +# real QA test starts here
> > +_supported_fs overlay
> > +_supported_os Linux
> > +_require_scratch
> > +
> > +# Remove all files from previous tests
> > +_scratch_mkfs
> > +
> > +# Create test file
> > +lowerdir=${OVL_BASE_SCRATCH_MNT}/${OVL_LOWER}
> > +upperdir=${OVL_BASE_SCRATCH_MNT}/${OVL_UPPER}
> > +mkdir -p $lowerdir
> > +touch ${lowerdir}/file
> > +
> > +_scratch_mount
> > +
>
> # Create whiteout and populate dcache with negative dentry
>
> > +rm ${SCRATCH_MNT}/file
> > +ls -l ${SCRATCH_MNT}/file > /dev/null 2>&1
>
> # Remove whiteout and try to create dir over negative dentry
>
> > +rm ${upperdir}/file
> > +mkdir ${SCRATCH_MNT}/file > /dev/null 2>&1
> > +
> > +# unmount overlayfs
> > +$UMOUNT_PROG $SCRATCH_MNT
>
> Umount of scratch is not needed at the end of the test.
>
> Thanks,
> Amir.

Re: [PATCH] overlay: create directory over deleted whiteout

[Eryu Guan]

On Sun, Nov 11, 2018 at 06:45:25PM +0200, Amir Goldstein wrote:
> On Wed, Oct 31, 2018 at 12:17 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, Oct 31, 2018 at 11:55 AM Miklos Szeredi <mszeredi@redhat.com> wrote:
> > >
> > > There's a bug in the overlayfs implementation starting from the very first
> > > merged version that may cause an Oops of various forms if a directory is created
> > > over a whiteout dentry, but the actual whiteout on the upper layer was removed
> > > to the directory creation.
> > >
> > > Reported by: kaixuxia <xiakaixu1987@gmail.com>
> > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > > ---
> >
> > Looks good. A bit of commentary could be useful...
> 
> Eryu,
> 
> I see you have not included this test in today's update.
> Maybe you though that this discussion did not conclude.
> On my part, besides adding the 2 comment lines, there
> are no further concerns.

Yeah, I saw the discussion was on going so I didn't save the thread to
my "to-review" queue and it fell out of my radar.. I'll queue it now.

> 
> FYI, the fix for this bug is now in master:
> 5e1275808630 ovl: check whiteout in ovl_create_over_whiteout()

Thanks for the reminder!

Eryu