[Crash-utility] The problems when running SuSE 12 on VirtualBox

[David Mair]

On 11/19/2015 12:20 PM, Dave Anderson wrote:
> 
> 
> ----- Original Message -----
>> On 11/19/2015 08:37 AM, David Mair wrote:
>>> On 11/19/2015 07:45 AM, Dave Anderson wrote:
>>>
>>> Hi Dave,
>>>
>>>> ----- Original Message -----
>>> <snip>
>>>>
>>>>> (2) Execute "crash -d8" on physical machine will cause crash utility core
>>>>> dump.
>>>>
>>>> I can reproduce this, so I'll look into it.  It's related to the /dev/mem "test"
>>>> to determine whether the kernel was configured with CONFIG_STRICT_DEVMEM, where
>>>> it tries to read from pfn 257 (just above the CONFIG_STRICT_DEVMEM limit), but
>>>> gets into an infinite loop when used in conjunction with -d.
>>>>
>>>> Anyway, just continue to use /proc/kcore and you should be fine.
>>>
>>> This is the cause in readmem():
>>>
>>> switch(READMEM(...))
>>> {
>>> 	.
>>> 	.
>>> 	.
>>> case READ_ERROR:
>>> 	if (PRINT_ERROR_MESSAGE) ********** THIS ***********
>>> 	{
>>> 		causes a nested readmem() call before the goto gives it
>>> 		to the caller to deal with
>>> 	}
>>> 	goto readmem_error
>>> }
>>> 	.
>>> 	.
>>> 	.
>>> switch(error_handle)
>>> {
>>> 	case (RETURN_ON_ERROR):
>>> }
>>>
>>> The PRINT_ERROR_MESSAGE I assume is an escalation from -d 8 in this case.
>>>
>>
>> The whole switch_to_proc_kcore() probably shouldn't be conditional on
>> the presence of PRINT_ERROR_MESSAGE, only the actual error message
>> should be.
>>
>> Patch shortly.
>>
>> --
>> David.
> 
> Yeah, I agree.  It was done that way to catch the very first non-QUIET readmem()
> call from kernel_init(), and make the bait-and-switch right there.  Without the 
> CRASHDEBUG(x) override, it works as-is because the readmem() calls in 
> devmem_is_restricted() are purposely set to QUIET.  
> 
> So we're thinking something like this, right?:
> 
> diff --git a/memory.c b/memory.c
> index 824b3ae..2282ba9 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2207,13 +2207,12 @@ readmem(ulonglong addr, int memtype, void *buffer, long size,
>                          goto readmem_error;
>  
>                 case READ_ERROR:
> -                        if (PRINT_ERROR_MESSAGE) {
> -                               if ((pc->flags & DEVMEM) && (kt->flags & PRE_KERNEL_INIT) &&
> -                                   devmem_is_restricted() && switch_to_proc_kcore())
> -                                       return(readmem(addr, memtype, bufptr, size,
> -                                               type, error_handle));
> +                       if ((pc->flags & DEVMEM) && (kt->flags & PRE_KERNEL_INIT) && 
> +                           !(error_handle & QUIET) && devmem_is_restricted() && switch_to_proc_kcore())
> +                               return(readmem(addr, memtype, bufptr, size,
> +                                       type, error_handle));
> +                       if (PRINT_ERROR_MESSAGE)
>                                 error(INFO, READ_ERRMSG, memtype_string(memtype, 0), addr, type);
> -                       }
>                          goto readmem_error;
>  
>                 case PAGE_EXCLUDED:
> 
> Works for me, and is better than hiding a recursive-call check in devmem_is_restricted().

That protects from nesting devmem_is_restricted() as long as
devmem_is_restricted() continues to do QUIET readmem()s but fails to
switch_to_proc_kcore() for QUIET readmem() at other random failing
addresses that could be fixed via /proc/kcore. QUIET isn't a global flag
only for being in devmem_is_restricted().

Here's mine, it's the kind of clumsy hardcore you were trying to avoid,
it's not thread or signal safe. I do feel the same concern you expressed
but I believe it does work for all failing QUIET readmem() cases.

diff --git a/memory.c b/memory.c
index 72218e7..71f1e47 100644
--- a/memory.c
+++ b/memory.c
@@ -319,6 +319,7 @@ static void dump_hstates(void);
 #define ASCII_UNLIMITED ((ulong)(-1) >> 1)

 static ulong DISPLAY_DEFAULT;
+static int IN_DEVMEM_IS_RESTRICTED;

 /*
  *  Verify that the sizeof the primitive types are reasonable.
@@ -338,6 +339,7 @@ mem_init(void)
                 error(FATAL, "pointer size: %d is not sizeof(long):
%d\n", sizeof(void *), sizeof(long));

         DISPLAY_DEFAULT = (sizeof(long) == 8) ? DISPLAY_64 : DISPLAY_32;
+        IN_DEVMEM_IS_RESTRICTED = FALSE;
 }


@@ -2204,13 +2206,13 @@ readmem(ulonglong addr, int memtype, void
*buffer, long size,
                         goto readmem_error;

 		case READ_ERROR:
-                        if (PRINT_ERROR_MESSAGE) {
-				if ((pc->flags & DEVMEM) && (kt->flags & PRE_KERNEL_INIT) &&
-				    devmem_is_restricted() && switch_to_proc_kcore())
-					return(readmem(addr, memtype, bufptr, size,
-						type, error_handle));
+                        if (PRINT_ERROR_MESSAGE)
 				error(INFO, READ_ERRMSG, memtype_string(memtype, 0), addr, type);
-			}
+
+			if ((pc->flags & DEVMEM) && (kt->flags & PRE_KERNEL_INIT) &&
+			    devmem_is_restricted() && switch_to_proc_kcore())
+				return(readmem(addr, memtype, bufptr, size,
+					type, error_handle));
                         goto readmem_error;

 		case PAGE_EXCLUDED:
@@ -2393,6 +2395,11 @@ devmem_is_restricted(void)
 	long tmp;
 	int restricted;

+	if (IN_DEVMEM_IS_RESTRICTED)
+		return FALSE;
+	IN_DEVMEM_IS_RESTRICTED = TRUE;
+
+	restricted = FALSE;

 	/*
 	 *  Check for pre-CONFIG_STRICT_DEVMEM kernels.
@@ -2401,11 +2408,9 @@ devmem_is_restricted(void)
 		if (machine_type("ARM") || machine_type("ARM64") ||
 		    machine_type("X86") || machine_type("X86_64") ||
 		    machine_type("PPC") || machine_type("PPC64"))
-			return FALSE;
+			goto end_devmem_is_restricted;
 	}

-	restricted = FALSE;
-
 	if (STREQ(pc->live_memsrc, "/dev/mem")) {
 	    	if (machine_type("X86") || machine_type("X86_64")) {
 			if (readmem(255*PAGESIZE(), PHYSADDR, &tmp,
@@ -2429,6 +2434,9 @@ devmem_is_restricted(void)
 			    "source.\n");
 	}

+end_devmem_is_restricted:
+	IN_DEVMEM_IS_RESTRICTED = FALSE;
+
 	return restricted;
 }