[PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs

[PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs

[Jeff Mahoney]

On NFS or Overlayfs, "mkfs" turns into rm -rf $SCRATCH_MNT/*

There is no warning/error in check if SCRATCH_MNT is unset.

Also add the checks to _scratch_cleanup_files.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 check     | 12 ++++++++++++
 common/rc |  8 ++++++++
 2 files changed, 20 insertions(+)

diff --git a/check b/check
index 69341d8..b22d2df 100755
--- a/check
+++ b/check
@@ -512,6 +512,18 @@ for section in $HOST_OPTIONS_SECTIONS; do
 	needwrap=true
 
 	if [ ! -z "$SCRATCH_DEV" ]; then
+	   if [ -z "$SCRATCH_MNT" ]
+	    then
+		echo "\$SCRATCH_MNT is unset"
+		status=1
+		exit
+	    fi
+	   if [ ! -d "$SCRATCH_MNT" ]
+	    then
+		echo "\$SCRATCH_MNT is not a dir"
+		status=1
+		exit
+	    fi
 	  _scratch_unmount 2> /dev/null
 	  # call the overridden mkfs - make sure the FS is built
 	  # the same as we'll create it later.
diff --git a/common/rc b/common/rc
index 13afc6a..4266c18 100644
--- a/common/rc
+++ b/common/rc
@@ -764,10 +764,18 @@ _scratch_cleanup_files()
 {
 	case $FSTYP in
 	overlay)
+		if [ -z "$SCRATCH_DEV" ]; then
+			echo "\$SCRATCH_DEV is unset."
+			return 1
+		fi
 		# $SCRATCH_DEV is a valid directory in overlay case
 		rm -rf $SCRATCH_DEV/*
 		;;
 	*)
+		if [ -z "$SCRATCH_MNT" ]; then
+			echo "\$SCRATCH_MNT is unset."
+			return 1
+		fi
 		_scratch_mount
 		rm -rf $SCRATCH_MNT/*
 		_scratch_unmount
-- 
1.7.12.4

Re: [PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs

[Dave Chinner]

On Sun, Sep 25, 2016 at 09:44:13PM -0400, Jeff Mahoney wrote:
> On NFS or Overlayfs, "mkfs" turns into rm -rf $SCRATCH_MNT/*
> 
> There is no warning/error in check if SCRATCH_MNT is unset.
> 
> Also add the checks to _scratch_cleanup_files.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  check     | 12 ++++++++++++
>  common/rc |  8 ++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/check b/check
> index 69341d8..b22d2df 100755
> --- a/check
> +++ b/check
> @@ -512,6 +512,18 @@ for section in $HOST_OPTIONS_SECTIONS; do
>  	needwrap=true
>  
>  	if [ ! -z "$SCRATCH_DEV" ]; then
> +	   if [ -z "$SCRATCH_MNT" ]
> +	    then
> +		echo "\$SCRATCH_MNT is unset"
> +		status=1
> +		exit
> +	    fi
> +	   if [ ! -d "$SCRATCH_MNT" ]
> +	    then
> +		echo "\$SCRATCH_MNT is not a dir"
> +		status=1
> +		exit
> +	    fi

That is supposed to be checked in get_next_config() at the start of
the loop. It runs these checks on the scratch config:

	_check_device SCRATCH_DEV optional $SCRATCH_DEV
	if [ ! -z "$SCRATCH_MNT" -a ! -d "$SCRATCH_MNT" ]; then
		echo "common/config: Error: \$SCRATCH_MNT ($SCRATCH_MNT) is not a directory"
		exit 1
	fi

If SCRATCH_DEV/SCRATCH_MNT is not set - which is a valid config -
the all that is supposed to happen is that tests which call
_require_scratch() should not run. This, in turn should prevent
the mkfs->rm problem you mention. The above code in
get_next_config() is what needs fixing, not the check code...

How did you actually trip over this?  I'm guessing you have a config
problem where you are defining SCRATCH_DEV but not SCRATCH_MNT?
Or you didn't set SCRATCH_DEV_NOT_USED?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 2971 bytes --]

On 9/26/16 12:44 AM, Dave Chinner wrote:
> On Sun, Sep 25, 2016 at 09:44:13PM -0400, Jeff Mahoney wrote:
>> On NFS or Overlayfs, "mkfs" turns into rm -rf $SCRATCH_MNT/*
>>
>> There is no warning/error in check if SCRATCH_MNT is unset.
>>
>> Also add the checks to _scratch_cleanup_files.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> ---
>>  check     | 12 ++++++++++++
>>  common/rc |  8 ++++++++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/check b/check
>> index 69341d8..b22d2df 100755
>> --- a/check
>> +++ b/check
>> @@ -512,6 +512,18 @@ for section in $HOST_OPTIONS_SECTIONS; do
>>  	needwrap=true
>>  
>>  	if [ ! -z "$SCRATCH_DEV" ]; then
>> +	   if [ -z "$SCRATCH_MNT" ]
>> +	    then
>> +		echo "\$SCRATCH_MNT is unset"
>> +		status=1
>> +		exit
>> +	    fi
>> +	   if [ ! -d "$SCRATCH_MNT" ]
>> +	    then
>> +		echo "\$SCRATCH_MNT is not a dir"
>> +		status=1
>> +		exit
>> +	    fi
> 
> That is supposed to be checked in get_next_config() at the start of
> the loop. It runs these checks on the scratch config:
> 
> 	_check_device SCRATCH_DEV optional $SCRATCH_DEV
> 	if [ ! -z "$SCRATCH_MNT" -a ! -d "$SCRATCH_MNT" ]; then
> 		echo "common/config: Error: \$SCRATCH_MNT ($SCRATCH_MNT) is not a directory"
> 		exit 1
> 	fi

But that only presents an error if SCRATCH_MNT isn't empty.  If it is,
we skip happily past.

> If SCRATCH_DEV/SCRATCH_MNT is not set - which is a valid config -

SCRATCH_DEV without SCRATCH_MNT isn't a valid config, though.  That
./check assumes that SCRATCH_DEV being valid means SCRATCH_MNT is too is
the source of the problem.  The test in get_next_config would prevent
the problem if we replaced the ! -z "$SCRATCH_MNT" with ! -z "$SCRATCH_DEV"

> the all that is supposed to happen is that tests which call
> _require_scratch() should not run. This, in turn should prevent

... but _require_scratch doesn't run in ./check.  The individual test
cases are safe because _require_scratch runs there and does check.
./check just checks to see if $SCRATCH_DEV is set and then calls
_scratch_mkfs without checking $SCRATCH_MNT.  Since _scratch_mkfs
doesn't check either, boom.

> the mkfs->rm problem you mention. The above code in
> get_next_config() is what needs fixing, not the check code...

In ./check, sure.  Fixing get_next_config does work as well.  I think
I'd like to keep the checks in _scratch_cleanup_files though so we don't
accidentally trip over that elsewhere.

> How did you actually trip over this?  I'm guessing you have a config
> problem where you are defining SCRATCH_DEV but not SCRATCH_MNT?
> Or you didn't set SCRATCH_DEV_NOT_USED?

Yeah, it's definitely a config problem.  SCRATCH_DEV was defined but I'd
defined SCRATCH_DIR instead of SCRATCH_MNT. Unfortunately, it's one I
discovered after "rm -rf /*" made it through to my autofs mount and
started working there.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]

Re: [PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs

[Dave Chinner]

On Mon, Sep 26, 2016 at 09:13:37AM -0400, Jeff Mahoney wrote:
> On 9/26/16 12:44 AM, Dave Chinner wrote:
> > On Sun, Sep 25, 2016 at 09:44:13PM -0400, Jeff Mahoney wrote:
> >> On NFS or Overlayfs, "mkfs" turns into rm -rf $SCRATCH_MNT/*
> >>
> >> There is no warning/error in check if SCRATCH_MNT is unset.
> >>
> >> Also add the checks to _scratch_cleanup_files.
> >>
> >> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> >> ---
> >>  check     | 12 ++++++++++++
> >>  common/rc |  8 ++++++++
> >>  2 files changed, 20 insertions(+)
> >>
> >> diff --git a/check b/check
> >> index 69341d8..b22d2df 100755
> >> --- a/check
> >> +++ b/check
> >> @@ -512,6 +512,18 @@ for section in $HOST_OPTIONS_SECTIONS; do
> >>  	needwrap=true
> >>  
> >>  	if [ ! -z "$SCRATCH_DEV" ]; then
> >> +	   if [ -z "$SCRATCH_MNT" ]
> >> +	    then
> >> +		echo "\$SCRATCH_MNT is unset"
> >> +		status=1
> >> +		exit
> >> +	    fi
> >> +	   if [ ! -d "$SCRATCH_MNT" ]
> >> +	    then
> >> +		echo "\$SCRATCH_MNT is not a dir"
> >> +		status=1
> >> +		exit
> >> +	    fi
> > 
> > That is supposed to be checked in get_next_config() at the start of
> > the loop. It runs these checks on the scratch config:
> > 
> > 	_check_device SCRATCH_DEV optional $SCRATCH_DEV
> > 	if [ ! -z "$SCRATCH_MNT" -a ! -d "$SCRATCH_MNT" ]; then
> > 		echo "common/config: Error: \$SCRATCH_MNT ($SCRATCH_MNT) is not a directory"
> > 		exit 1
> > 	fi
> 
> But that only presents an error if SCRATCH_MNT isn't empty.  If it is,
> we skip happily past.
> 
> > If SCRATCH_DEV/SCRATCH_MNT is not set - which is a valid config -
> 
> SCRATCH_DEV without SCRATCH_MNT isn't a valid config, though.  That
> ./check assumes that SCRATCH_DEV being valid means SCRATCH_MNT is too is
> the source of the problem.  The test in get_next_config would prevent
> the problem if we replaced the ! -z "$SCRATCH_MNT" with ! -z "$SCRATCH_DEV"

Because SCRATCH_DEV is optional, the check for SCRATCH_MNT in
get_next_config() needs to take that into account. i.e. if
SCRATCH_DEV is set, then SCRATCH_MNT must be set, too.

> > the all that is supposed to happen is that tests which call
> > _require_scratch() should not run. This, in turn should prevent
> 
> ... but _require_scratch doesn't run in ./check.  The individual test
> cases are safe because _require_scratch runs there and does check.
> ./check just checks to see if $SCRATCH_DEV is set and then calls
> _scratch_mkfs without checking $SCRATCH_MNT.  Since _scratch_mkfs
> doesn't check either, boom.

Yes, I know. That's why it should be checked in get_next_config() -
it is supposed to catch any config errors before they get used
anywhere.

It makes no sense to sprinkle random "is the config valid" checks
throughout the code. We should validate the config once - and once
only - before we run anything. Then we can assume (correctly) the
config is valid everywhere else.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 3307 bytes --]

On 9/26/16 6:10 PM, Dave Chinner wrote:
> On Mon, Sep 26, 2016 at 09:13:37AM -0400, Jeff Mahoney wrote:
>> On 9/26/16 12:44 AM, Dave Chinner wrote:
>>> On Sun, Sep 25, 2016 at 09:44:13PM -0400, Jeff Mahoney wrote:
>>>> On NFS or Overlayfs, "mkfs" turns into rm -rf $SCRATCH_MNT/*
>>>>
>>>> There is no warning/error in check if SCRATCH_MNT is unset.
>>>>
>>>> Also add the checks to _scratch_cleanup_files.
>>>>
>>>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>>>> ---
>>>>  check     | 12 ++++++++++++
>>>>  common/rc |  8 ++++++++
>>>>  2 files changed, 20 insertions(+)
>>>>
>>>> diff --git a/check b/check
>>>> index 69341d8..b22d2df 100755
>>>> --- a/check
>>>> +++ b/check
>>>> @@ -512,6 +512,18 @@ for section in $HOST_OPTIONS_SECTIONS; do
>>>>  	needwrap=true
>>>>  
>>>>  	if [ ! -z "$SCRATCH_DEV" ]; then
>>>> +	   if [ -z "$SCRATCH_MNT" ]
>>>> +	    then
>>>> +		echo "\$SCRATCH_MNT is unset"
>>>> +		status=1
>>>> +		exit
>>>> +	    fi
>>>> +	   if [ ! -d "$SCRATCH_MNT" ]
>>>> +	    then
>>>> +		echo "\$SCRATCH_MNT is not a dir"
>>>> +		status=1
>>>> +		exit
>>>> +	    fi
>>>
>>> That is supposed to be checked in get_next_config() at the start of
>>> the loop. It runs these checks on the scratch config:
>>>
>>> 	_check_device SCRATCH_DEV optional $SCRATCH_DEV
>>> 	if [ ! -z "$SCRATCH_MNT" -a ! -d "$SCRATCH_MNT" ]; then
>>> 		echo "common/config: Error: \$SCRATCH_MNT ($SCRATCH_MNT) is not a directory"
>>> 		exit 1
>>> 	fi
>>
>> But that only presents an error if SCRATCH_MNT isn't empty.  If it is,
>> we skip happily past.
>>
>>> If SCRATCH_DEV/SCRATCH_MNT is not set - which is a valid config -
>>
>> SCRATCH_DEV without SCRATCH_MNT isn't a valid config, though.  That
>> ./check assumes that SCRATCH_DEV being valid means SCRATCH_MNT is too is
>> the source of the problem.  The test in get_next_config would prevent
>> the problem if we replaced the ! -z "$SCRATCH_MNT" with ! -z "$SCRATCH_DEV"
> 
> Because SCRATCH_DEV is optional, the check for SCRATCH_MNT in
> get_next_config() needs to take that into account. i.e. if
> SCRATCH_DEV is set, then SCRATCH_MNT must be set, too.

Yep.  I'll post an updated patch that does that.

>>> the all that is supposed to happen is that tests which call
>>> _require_scratch() should not run. This, in turn should prevent
>>
>> ... but _require_scratch doesn't run in ./check.  The individual test
>> cases are safe because _require_scratch runs there and does check.
>> ./check just checks to see if $SCRATCH_DEV is set and then calls
>> _scratch_mkfs without checking $SCRATCH_MNT.  Since _scratch_mkfs
>> doesn't check either, boom.
> 
> Yes, I know. That's why it should be checked in get_next_config() -
> it is supposed to catch any config errors before they get used
> anywhere.
> 
> It makes no sense to sprinkle random "is the config valid" checks
> throughout the code. We should validate the config once - and once
> only - before we run anything. Then we can assume (correctly) the
> config is valid everywhere else.

In general, I agree.  But when the end result for a missing value is rm
-rf /*, I don't think it's overkill.  A mkfs with a missing value isn't
going to accidentally mkfs everything.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]