linux-kernel - Re: [PATCH, v4] hwmon: coretemp: use list instead of fixed size array for temp data

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: <20120509072339.GB21556@otc-wbsnb-06>
Date:	Wed, 9 May 2012 10:23:39 +0300
From:	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To:	Guenter Roeck <guenter.roeck@...csson.com>
Cc:	Fenghua Yu <fenghua.yu@...el.com>,
	Durgadoss R <durgadoss.r@...el.com>,
	Andi Kleen <ak@...ux.intel.com>,
	Jean Delvare <khali@...ux-fr.org>,
	"lm-sensors@...sensors.org" <lm-sensors@...sensors.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH, v4] hwmon: coretemp: use list instead of fixed size
 array for temp data

On Wed, May 09, 2012 at 10:09:06AM +0300, Kirill A. Shutemov wrote:
> On Tue, May 08, 2012 at 09:39:40AM -0700, Guenter Roeck wrote:
> > On Tue, 2012-05-08 at 06:49 -0400, Kirill A. Shutemov wrote:
> > > From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
> > > 
> > > Let's rework code to allow arbitrary number of cores on a CPU, not
> > > limited by hardcoded array size.
> > > 
> > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> > > ---
> > >  v4:
> > >    - address issues pointed by Guenter Roeck;
> > >  v3:
> > >    - drop redundant refcounting and checks;
> > >  v2:
> > >    - fix NULL pointer dereference. Thanks to R, Durgadoss;
> > >    - use mutex instead of spinlock for list locking.
> > > ---
> > 
> > Hi Kirill,
> > 
> > unfortunately now we have another race condition :(. See below ...
> 
> Ughh..
> 
> > > @@ -557,11 +579,22 @@ exit_free:
> > >  static int __devexit coretemp_remove(struct platform_device *pdev)
> > >  {
> > >  	struct platform_data *pdata = platform_get_drvdata(pdev);
> > > -	int i;
> > > +	struct temp_data *tdata;
> > >  
> > > -	for (i = MAX_CORE_DATA - 1; i >= 0; --i)
> > > -		if (pdata->core_data[i])
> > > -			coretemp_remove_core(pdata, &pdev->dev, i);
> > > +	for (;;) {
> > > +		mutex_lock(&pdata->temp_data_lock);
> > > +		if (!list_empty(&pdata->temp_data_list)) {
> > > +			tdata = list_first_entry(&pdata->temp_data_list,
> > > +					struct temp_data, list);
> > > +			list_del(&tdata->list);
> > > +		} else
> > > +			tdata = NULL;
> > > +		mutex_unlock(&pdata->temp_data_lock);
> > > +
> > > +		if (!tdata)
> > > +			break;
> > > +		coretemp_remove_core(tdata, &pdev->dev);
> > > +	}
> > >  
> > Unfortunately, that results in a race condition, since the tdata list
> > entry is gone before the attribute file is deleted.
> > 
> > I think you can still use list_for_each_entry_safe, only outside the
> > mutex, and remove the list entry at the end of coretemp_remove_core()

I haven't got how list_for_each_entry_safe() will be really safe without
the lock.

> > after deleting the attribute file. Just keep the code as it was, and
> > remove the list entry (mutex-protected) where core_data[] was set to
> > NULL.
> 
> I think
> 
> if (tdata)
> 	return -ENODEV;
> 
> in show methods will fix the issue. Right?

It won't. Stupid me.

But the check + kref seems will work...

-- 
 Kirill A. Shutemov

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)