linux-kernel - Re: [BUG] mm: bdi: export BDI attributes in sysfs

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 for Android: free password hash cracker in your pocket

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

Date:	Thu, 15 May 2008 12:18:57 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Miklos Szeredi <miklos@...redi.hu>
cc:	ajones@...erbed.com, a.p.zijlstra@...llo.nl, mszeredi@...e.cz,
	akpm@...ux-foundation.org, kay.sievers@...y.org, greg@...ah.com,
	trond.myklebust@....uio.no, linux-kernel@...r.kernel.org
Subject: Re: [BUG] mm: bdi: export BDI attributes in sysfs

On Thu, 15 May 2008, Miklos Szeredi wrote:
> 
> This is not meant as a final solution, I'm sure Greg or Kay can help
> find a better solution.

Yeah, don't do this:

> +static struct backing_dev_info *dev_get_bdi(struct device *dev)
> +{
> +	mutex_lock(&bdi_dev_mutex);
> +	mutex_unlock(&bdi_dev_mutex);
> +
> +	return dev_get_drvdata(dev);
> +}

This kind of serialization can often hide bugs, and in some cases even 
make them go away (if the caller of the function means that the device is 
pinned and the tear-down cannot happen, for example), but it's really 
really bad form.

In order to use locking in a repeatable manner that is easy to think 
about, you really need to *keep* the lock until you've stopped using the 
data (or have dereferenced it into a stable form - eg maybe accessing the 
pointer itself needs locking, but some individual data read _off_ the 
pointer does not).

So the above kind of "get and release the lock" does obviously serialize 
wrt others that hold the lock, but it's still wrong.

>  static ssize_t read_ahead_kb_store(struct device *dev,
>  				  struct device_attribute *attr,
>  				  const char *buf, size_t count)
>  {
> -	struct backing_dev_info *bdi = dev_get_drvdata(dev);
> +	struct backing_dev_info *bdi = dev_get_bdi(dev);
>  	char *end;
>  	unsigned long read_ahead_kb;
>  	ssize_t ret = -EINVAL;

You should just get the lock in the routines that acually use this thing.

Or, if the "struct backing_dev_info *" pointer itself is stable, and it 
really is just the access from "struct device" that needs protection, then 
at the very least it should have been

	static struct backing_dev_info *dev_get_bdi(struct device *dev)
	{
		struct backing_dev_info *bdi;

		mutex_lock(&bdi_dev_mutex);
		bdi = dev_get_drvdata(dev);
		mutex_unlock(&bdi_dev_mutex);
		return bdi;
	}

which makes it clear that it's the "dev_get_drvdata()" that needs the 
locking, not the bdi pointer itself.

		Linus
--
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/