[PATCH 1/1] x86: Fix mcheck_init_device() to handle misc_register() correctly

[PATCH 1/1] x86: Fix mcheck_init_device() to handle misc_register() correctly

[J.P. Lacerda]

The return value for misc_register() was not being taken into account.
Furthermore, if misc_register() fails, we must rollback any changes made by
mcheck_init_device()

Signed-off-by: J.P. Lacerda <jp.lacerda@codethink.co.uk>
---
 arch/x86/kernel/cpu/mcheck/mce.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index d916183..20c2c44 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -2140,7 +2140,18 @@ static __init int mcheck_init_device(void)
 	}
 
 	register_hotcpu_notifier(&mce_cpu_notifier);
-	misc_register(&mce_log_device);
+	err = misc_register(&mce_log_device);
+
+	if (err) {
+		unregister_hotcpu_notifier(&mce_cpu_notifier);
+
+		for_each_online_cpu(i) {
+			mce_remove_device(i);
+		}
+
+		sysdev_class_unregister(&mce_sysclass);
+		free_cpumask_var(mce_dev_initialized);
+	}
 
 	return err;
 }
-- 
1.7.1

Re: [PATCH 1/1] x86: Fix mcheck_init_device() to handle misc_register() correctly

[Borislav Petkov]

On Fri, Mar 04, 2011 at 01:29:27PM +0000, J.P. Lacerda wrote:
> The return value for misc_register() was not being taken into account.
> Furthermore, if misc_register() fails, we must rollback any changes made by
> mcheck_init_device()

If you're going to fix all error paths here, you still need to handle
unrolling the setup done by mce_create_device() if we fail somewhere in
between.

> Signed-off-by: J.P. Lacerda <jp.lacerda@codethink.co.uk>
> ---
>  arch/x86/kernel/cpu/mcheck/mce.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index d916183..20c2c44 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -2140,7 +2140,18 @@ static __init int mcheck_init_device(void)
>  	}
>  
>  	register_hotcpu_notifier(&mce_cpu_notifier);
> -	misc_register(&mce_log_device);
> +	err = misc_register(&mce_log_device);
> +

no newline here.

> +	if (err) {
> +		unregister_hotcpu_notifier(&mce_cpu_notifier);
> +
> +		for_each_online_cpu(i) {
> +			mce_remove_device(i);
> +		}

no need for braces around a single loop body statement.

> +
> +		sysdev_class_unregister(&mce_sysclass);
> +		free_cpumask_var(mce_dev_initialized);
> +	}
>  
>  	return err;

Anyway, while this is makes sense from correctness POV, if we hit an
error path here this early then something else is going terribly wrong
which would've screamed very loudly already. Are you hitting this on a
real workload or you caught this by code staring?

Because if it is the second case, the merit of fixing those error
paths vs adding code which is almost never executed is significantly
diminished.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH 1/1] x86: Fix mcheck_init_device() to handle misc_register() correctly

[Johan Wessfeldt]

On Fri, Mar 4, 2011 at 6:16 PM, Borislav Petkov <bp@amd64.org> wrote:
> On Fri, Mar 04, 2011 at 01:29:27PM +0000, J.P. Lacerda wrote:
>> The return value for misc_register() was not being taken into account.
>> Furthermore, if misc_register() fails, we must rollback any changes made by
>> mcheck_init_device()
>
> If you're going to fix all error paths here, you still need to handle
> unrolling the setup done by mce_create_device() if we fail somewhere in
> between.
>
>> Signed-off-by: J.P. Lacerda <jp.lacerda@codethink.co.uk>
>> ---
>>  arch/x86/kernel/cpu/mcheck/mce.c |   13 ++++++++++++-
>>  1 files changed, 12 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
>> index d916183..20c2c44 100644
>> --- a/arch/x86/kernel/cpu/mcheck/mce.c
>> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
>> @@ -2140,7 +2140,18 @@ static __init int mcheck_init_device(void)
>>       }
>>
>>       register_hotcpu_notifier(&mce_cpu_notifier);
>> -     misc_register(&mce_log_device);
>> +     err = misc_register(&mce_log_device);
>> +
>
> no newline here.
>
>> +     if (err) {
>> +             unregister_hotcpu_notifier(&mce_cpu_notifier);
>> +
>> +             for_each_online_cpu(i) {
>> +                     mce_remove_device(i);
>> +             }
>
> no need for braces around a single loop body statement.
>
>> +
>> +             sysdev_class_unregister(&mce_sysclass);
>> +             free_cpumask_var(mce_dev_initialized);
>> +     }
>>
>>       return err;
>
> Anyway, while this is makes sense from correctness POV, if we hit an
> error path here this early then something else is going terribly wrong
> which would've screamed very loudly already. Are you hitting this on a
> real workload or you caught this by code staring?
The orginal patch was made with the intention of auditing the code
according to the kernel-janitors TODO list:
http://kernelnewbies.org/KernelJanitors/Todo/ReturnCodes .

To clear things up. I originally posted a minor patch, which basically passed
the return value from misc_register up the stack. See
http://marc.info/?l=linux-kernel&m=129889198732342&w=2

The patch was rejected with comments about how to clean everything up
if misc_register() fails.

Here's my second attempt:
http://marc.info/?l=linux-kernel&m=129926353413345&w=2

(Dan Carpenter just explained to me how to go about resubmitting the
patches thanks)

Sorry for the confusion.
>
> Because if it is the second case, the merit of fixing those error
> paths vs adding code which is almost never executed is significantly
> diminished.
>
> Thanks.
>
> --
> Regards/Gruss,
> Boris.
>
> Advanced Micro Devices GmbH
> Einsteinring 24, 85609 Dornach
> General Managers: Alberto Bozzo, Andrew Bowd
> Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
> Registergericht Muenchen, HRB Nr. 43632
>

Re: [PATCH 1/1] x86: Fix mcheck_init_device() to handle misc_register() correctly

[Borislav Petkov]

On Fri, Mar 04, 2011 at 04:07:21PM -0500, Johan Wessfeldt wrote:
> On Fri, Mar 4, 2011 at 6:16 PM, Borislav Petkov <bp@amd64.org> wrote:

..

> > Anyway, while this is makes sense from correctness POV, if we hit an
> > error path here this early then something else is going terribly wrong
> > which would've screamed very loudly already. Are you hitting this on a
> > real workload or you caught this by code staring?
> The orginal patch was made with the intention of auditing the code
> according to the kernel-janitors TODO list:
> http://kernelnewbies.org/KernelJanitors/Todo/ReturnCodes .
> 
> To clear things up. I originally posted a minor patch, which basically passed
> the return value from misc_register up the stack. See
> http://marc.info/?l=linux-kernel&m=129889198732342&w=2

I see what you're doing and I think it is great that you're trying to
audit the code - this is actually very commendable but I'm questioning
whether such a "fix" makes sense in this case. As I said above, if we
hit those error paths, then we definitely botched up something else big
time and recovering here is pretty moot at this point. Therefore, adding
a bunch of code which is almost never executed is simply unneeded.

HTH.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH 1/1] x86: Fix mcheck_init_device() to handle misc_register() correctly

[Dan Carpenter]

On Fri, Mar 04, 2011 at 01:29:27PM +0000, J.P. Lacerda wrote:
> The return value for misc_register() was not being taken into account.
> Furthermore, if misc_register() fails, we must rollback any changes made by
> mcheck_init_device()
> 
> Signed-off-by: J.P. Lacerda <jp.lacerda@codethink.co.uk>

Ah...  You found a bug that Johan missed in his original patch.  But you
still left out the case where mce_create_device() can fail which his 
patch addressed.  Also his patch was nicer; using gotos to unwind is 
the standard way for kernel error handling.

Also why did you poach his patch without giving any kind of credit,
instead of just telling him about the bug???  WTF?

Johan, please fix add a free_cpumask_var(mce_dev_initialized); to your
patch.  Call it [PATCH v2].  Add in the changelog that J.P. Lacerda
told you about the missing free_cpumask_var(mce_dev_initialized).

regards,
dan carpenter

Re: [PATCH 1/1] x86: Fix mcheck_init_device() to handle misc_register() correctly

[Johan Wessfeldt]

On Fri, Mar 4, 2011 at 2:29 PM, J.P. Lacerda <jp.lacerda@codethink.co.uk> wrote:
> The return value for misc_register() was not being taken into account.
> Furthermore, if misc_register() fails, we must rollback any changes made by
> mcheck_init_device()
>
> Signed-off-by: J.P. Lacerda <jp.lacerda@codethink.co.uk>
> ---
>  arch/x86/kernel/cpu/mcheck/mce.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index d916183..20c2c44 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -2140,7 +2140,18 @@ static __init int mcheck_init_device(void)
>        }
>
>        register_hotcpu_notifier(&mce_cpu_notifier);
> -       misc_register(&mce_log_device);
> +       err = misc_register(&mce_log_device);
> +
> +       if (err) {
> +               unregister_hotcpu_notifier(&mce_cpu_notifier);
> +
> +               for_each_online_cpu(i) {
> +                       mce_remove_device(i);
> +               }
> +
> +               sysdev_class_unregister(&mce_sysclass);
> +               free_cpumask_var(mce_dev_initialized);
> +       }
>
>        return err;
>  }
> --
> 1.7.1
>
>
I want to thank J.P. Lacerda for pointing out an additional leak that I
missed in my first version. ie. calling free_cpumask_var(mce_dev_initialized).

Regards.
// Johan W.