linux-kernel - Re: [PATCH] w1: Introduce a slave mutex for serializing IO.

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]

Date:	Wed, 9 May 2012 11:43:49 +1000
From:	NeilBrown <neilb@...e.de>
To:	Evgeniy Polyakov <zbr@...emap.net>
Cc:	linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] w1: Introduce a slave mutex for serializing IO.

On Fri, 4 May 2012 01:27:06 +0400 Evgeniy Polyakov <zbr@...emap.net> wrote:

> On Fri, May 04, 2012 at 07:08:38AM +1000, NeilBrown (neilb@...e.de) wrote:
> > You can only check the owner on SMP builds, or when debugging is enabled.
> > So I don't think that approach can work.
> 
> You can store owner in master device and protect with mutex itself.
> On non-smp systems it can not be preempted, so can be checked without
> mutex.
> 

I tried that - or something a lot like it.  Patch below.

However lockdep didn't like it.  There are ordering problems between this
mutex and and sysfs's s_active.

When you access battery properies via sysfs, the sysfs lock is taken first,
then the master->mutex.
When w1_reconnect_slaves calls through to device_del and sys_addrm_finish,
the mutex is held while the sysfs lock is wanted.

So we might need to come up with something more clever.

I haven't had a chance to look really deeply into this yet.  Hopefully when I
do I'll find something clever and let you know.

Thanks,
NeilBrown



diff --git a/drivers/w1/slaves/w1_bq27000.c b/drivers/w1/slaves/w1_bq27000.c
index 52ad812..83ebaad 100644
--- a/drivers/w1/slaves/w1_bq27000.c
+++ b/drivers/w1/slaves/w1_bq27000.c
@@ -30,11 +30,14 @@ static int w1_bq27000_read(struct device *dev, unsigned int reg)
 {
 	u8 val;
 	struct w1_slave *sl = container_of(dev->parent, struct w1_slave, dev);
+	bool own_mutex = (sl->master->mutex_owner == current);
 
-	mutex_lock(&sl->master->mutex);
+	if (!own_mutex)
+		mutex_lock(&sl->master->mutex);
 	w1_write_8(sl->master, HDQ_CMD_READ | reg);
 	val = w1_read_8(sl->master);
-	mutex_unlock(&sl->master->mutex);
+	if (!own_mutex)
+		mutex_unlock(&sl->master->mutex);
 
 	return val;
 }
diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 9761950..97de03d 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -616,7 +616,13 @@ static int __w1_attach_slave_device(struct w1_slave *sl)
 	dev_dbg(&sl->dev, "%s: registering %s as %p.\n", __func__,
 		dev_name(&sl->dev), sl);
 
+	/* device_register might end up asking the slave to
+	 * access the bus, so we must let it know that it
+	 * already holds the lock.
+	 */
+	sl->master->mutex_owner = current;
 	err = device_register(&sl->dev);
+	sl->master->mutex_owner = NULL;
 	if (err < 0) {
 		dev_err(&sl->dev,
 			"Device registration [%s] failed. err=%d\n",
diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
index 4d012ca..ebb157c 100644
--- a/drivers/w1/w1.h
+++ b/drivers/w1/w1.h
@@ -180,6 +180,13 @@ struct w1_master
 
 	struct task_struct	*thread;
 	struct mutex		mutex;
+	/* The mutex_owner owns the mutex and so does not
+	 * need to take it again (and doing so would deadlock).
+	 * This is important when registering a device while holding
+	 * the mutex as the slave might need to access the bus as part
+	 * of registration.
+	 */
+	struct task_struct	*mutex_owner;
 
 	struct device_driver	*driver;
 	struct device		dev;

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