linux-kernel - Re: [Patch-next] Trival fixes in thermal_throttle_add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100831170443.GA30454@fenghua-desk.sc.intel.com>
Date:	Tue, 31 Aug 2010 10:04:43 -0700
From:	Fenghua Yu <fenghua.yu@...el.com>
To:	Jean Delvare <khali@...ux-fr.org>
Cc:	"Yu, Fenghua" <fenghua.yu@...el.com>,
	Jin Dongming <jin.dongming@...css.fujitsu.com>,
	"Brown, Len" <len.brown@...el.com>,
	Hidetoshi Seto <seto.hidetoshi@...fujitsu.com>,
	"H. Peter Anvin" <hpa@...ux.jf.intel.com>,
	LKLM <linux-kernel@...r.kernel.org>,
	"lm-sensors@...sensors.org" <lm-sensors@...sensors.org>,
	mingo Redhat <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [Patch-next] Trival fixes in thermal_throttle_add_dev().

On Tue, Aug 31, 2010 at 12:07:25AM -0700, Jean Delvare wrote:
> On Mon, 30 Aug 2010 18:02:52 -0700, Fenghua Yu wrote:
> > On Mon, Aug 30, 2010 at 05:55:48PM -0700, Jin Dongming wrote:
> > > diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > > index d9368ee..79d563a 100644
> > > --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > > +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > > @@ -216,14 +216,27 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
> > >  		err = sysfs_add_file_to_group(&sys_dev->kobj,
> > >  					      &attr_core_power_limit_count.attr,
> > >  					      thermal_attr_group.name);
> > > -	if (cpu_has(c, X86_FEATURE_PTS))
> > > +	if (err)
> > > +		goto error;
> > > +
> > > +	if (cpu_has(c, X86_FEATURE_PTS)) {
> > >  		err = sysfs_add_file_to_group(&sys_dev->kobj,
> > >  					      &attr_package_throttle_count.attr,
> > >  					      thermal_attr_group.name);
> > > +		if (err)
> > > +			goto error;
> > > +
> > >  		if (cpu_has(c, X86_FEATURE_PLN))
> > >  			err = sysfs_add_file_to_group(&sys_dev->kobj,
> > >  					&attr_package_power_limit_count.attr,
> > >  					thermal_attr_group.name);
> > > +		if (err)
> > > +			goto error;
> > > +	}
> > > +
> > > +	return 0;
> > > +error:
> > > +	sysfs_remove_group(&sys_dev->kobj, &thermal_attr_group);
> > >  
> > >  	return err;
> > >  }
> > 
> > This fix is incorrect. In this patch, a previous error prevents from adding any
> > further devices. There shouldn't be such dependency among the devices.
> 
> I don't quite follow you. Did you mean to write that a previous error
> prevents from creating further _attributes_ for the same device? This
> would be true.
> 
> Now I don't think this is a problem because 1* such errors should never
> happen anyway and 2* if they do happen then further attempts to create
> the other attributes are unlikely to succeed either.

I don't think there is dependency among the count files, i.e. failure to add a
count file to the group shouldn't impact other files. Other filles can still be
added to the group. In this case, user application only sees part of count
numbers. And kernel may just warn on the failure instead of providing nothing
to user.

In the patch, any failure to add a file will remove the whole group. This is
too strict. Kernel doesn't provide any count number to user application.

Agree with you that such errors should never happen anyway. So original code
works fine.

If to be picky to the error handling, a patch may just ignore returning errors
from sysfs_add_file_to_group. Or use err |= sysfs_add_file_to_group to collect
all errors and return err but without calling sysfs_remove_group.

Thanks.

-Fenghua
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/