| Message ID | 20181031095502.12504-1-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | overlay: create directory over deleted whiteout | expand |
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.
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
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.
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
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.
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
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
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