[PATCH] firewire: fix "kobject_add failed for fw* with -EEXIST"

[PATCH] firewire: fix "kobject_add failed for fw* with -EEXIST"

[Stefan Richter]

There is a race between shutdown and creation of devices:  fw-core may
attempt to add a device with the same name of an already existing
device.  http://bugzilla.kernel.org/show_bug.cgi?id=9828

Impact of the bug:  Happens rarely, forces the user to unplug and replug
the new device to get it working.

The fix is to defer the deregistration of the minor number until after
device_unregister().  This requires an adjustment of the device lookup
function though to prevent the character device from being newly opened
during shutdown.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-device.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Index: linux/drivers/firewire/fw-device.c
===================================================================
--- linux.orig/drivers/firewire/fw-device.c
+++ linux/drivers/firewire/fw-device.c
@@ -616,6 +616,8 @@ struct fw_device *fw_device_from_devt(de
 
 	down_read(&idr_rwsem);
 	device = idr_find(&fw_device_idr, MINOR(devt));
+	if (fw_device_is_shutdown(device))
+		device = NULL;
 	up_read(&idr_rwsem);
 
 	return device;
@@ -627,13 +629,13 @@ static void fw_device_shutdown(struct wo
 		container_of(work, struct fw_device, work.work);
 	int minor = MINOR(device->device.devt);
 
-	down_write(&idr_rwsem);
-	idr_remove(&fw_device_idr, minor);
-	up_write(&idr_rwsem);
-
 	fw_device_cdev_remove(device);
 	device_for_each_child(&device->device, NULL, shutdown_unit);
+
+	down_write(&idr_rwsem);
 	device_unregister(&device->device);
+	idr_remove(&fw_device_idr, minor);
+	up_write(&idr_rwsem);
 }
 
 static struct device_type fw_device_type = {

-- 
Stefan Richter
-=====-==--- ---= ==-==
http://arcgraph.de/sr/

[PATCH update] firewire: fix "kobject_add failed for fw* with -EEXIST"

[Stefan Richter]

There is a race between shutdown and creation of devices:  fw-core may
attempt to add a device with the same name of an already existing
device.  http://bugzilla.kernel.org/show_bug.cgi?id=9828

Impact of the bug:  Happens rarely, forces the user to unplug and replug
the new device to get it working.

The fix moves deregistration of the minor number and device_unregister()
into a common rw_sem protected section.

We also move the ref count increment from fw_device_op_open into an
rw_sem protected section with the lookup of the device, so that the
device pointer can't become invalid between lookup and usage.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-cdev.c   |    6 ++++--
 drivers/firewire/fw-device.c |   10 ++++++----
 2 files changed, 10 insertions(+), 6 deletions(-)

Index: linux/drivers/firewire/fw-device.c
===================================================================
--- linux.orig/drivers/firewire/fw-device.c
+++ linux/drivers/firewire/fw-device.c
@@ -614,10 +614,12 @@ struct fw_device *fw_device_from_devt(de
 {
 	struct fw_device *device;
 
 	down_read(&idr_rwsem);
 	device = idr_find(&fw_device_idr, MINOR(devt));
+	if (device)
+		fw_device_get(device);
 	up_read(&idr_rwsem);
 
 	return device;
 }
 
@@ -625,17 +627,17 @@ static void fw_device_shutdown(struct wo
 {
 	struct fw_device *device =
 		container_of(work, struct fw_device, work.work);
 	int minor = MINOR(device->device.devt);
 
-	down_write(&idr_rwsem);
-	idr_remove(&fw_device_idr, minor);
-	up_write(&idr_rwsem);
-
 	fw_device_cdev_remove(device);
 	device_for_each_child(&device->device, NULL, shutdown_unit);
+
+	down_write(&idr_rwsem);
 	device_unregister(&device->device);
+	idr_remove(&fw_device_idr, minor);
+	up_write(&idr_rwsem);
 }
 
 static struct device_type fw_device_type = {
 	.release	= fw_device_release,
 };
Index: linux/drivers/firewire/fw-cdev.c
===================================================================
--- linux.orig/drivers/firewire/fw-cdev.c
+++ linux/drivers/firewire/fw-cdev.c
@@ -112,14 +112,16 @@ static int fw_device_op_open(struct inod
 	device = fw_device_from_devt(inode->i_rdev);
 	if (device == NULL)
 		return -ENODEV;
 
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
-	if (client == NULL)
+	if (client == NULL) {
+		fw_device_put(device);
 		return -ENOMEM;
+	}
 
-	client->device = fw_device_get(device);
+	client->device = device;
 	INIT_LIST_HEAD(&client->event_list);
 	INIT_LIST_HEAD(&client->resource_list);
 	spin_lock_init(&client->lock);
 	init_waitqueue_head(&client->wait);
 

-- 
Stefan Richter
-=====-==--- ---= ==-==
http://arcgraph.de/sr/

[PATCH] firewire: fail open() quickly if the node doesn't exist anymore

[Stefan Richter]

Scenario:  Process A keeps the character device file of node N open.
N is being unplugged.  File /dev/fwN won't be destroyed as long as A
doesn't close it.  Now, process B opens /dev/fwN as well.  Previously
it would succeed but be unable to do any IO on it of course.  With this
patch, process B's open() will fail immediately with -ENODEV.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-device.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Index: linux/drivers/firewire/fw-device.c
===================================================================
--- linux.orig/drivers/firewire/fw-device.c
+++ linux/drivers/firewire/fw-device.c
@@ -616,8 +616,12 @@ struct fw_device *fw_device_from_devt(de
 
 	down_read(&idr_rwsem);
 	device = idr_find(&fw_device_idr, MINOR(devt));
-	if (device)
-		fw_device_get(device);
+	if (device) {
+		if (fw_device_is_shutdown(device))
+			device = NULL;
+		else
+			fw_device_get(device);
+	}
 	up_read(&idr_rwsem);
 
 	return device;

-- 
Stefan Richter
-=====-==--- ---= ==-==
http://arcgraph.de/sr/

Re: [PATCH] firewire: fail open() quickly if the node doesn't exist anymore

[Jarod Wilson]

On Sunday 27 January 2008 12:21:56 pm Stefan Richter wrote:
> Scenario:  Process A keeps the character device file of node N open.
> N is being unplugged.  File /dev/fwN won't be destroyed as long as A
> doesn't close it.  Now, process B opens /dev/fwN as well.  Previously
> it would succeed but be unable to do any IO on it of course.  With this
> patch, process B's open() will fail immediately with -ENODEV.
>
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

Makes perfect sense to me, no problems with it in cursory testing.

Signed-off-by: Jarod Wilson <jwilson@redhat.com>

-- 
Jarod Wilson
jwilson@redhat.com

Re: [PATCH] firewire: fail open() quickly if the node doesn't exist anymore

[Stefan Richter]

Jarod Wilson wrote:
> On Sunday 27 January 2008 12:21:56 pm Stefan Richter wrote:
>> Scenario:  Process A keeps the character device file of node N open.
>> N is being unplugged.  File /dev/fwN won't be destroyed as long as A
>> doesn't close it.  Now, process B opens /dev/fwN as well.  Previously
>> it would succeed but be unable to do any IO on it of course.  With this
>> patch, process B's open() will fail immediately with -ENODEV.

> Makes perfect sense to me, no problems with it in cursory testing.

Actually I have 2nd thoughts about it.  Clients should have a general
way to know that a device went away.  (I.e. distinguish I/O errors from
mere stale generation from I/O errors due to the node being gone for
good.)  I will check tomorrow if the ABI does this distinction already.
If yes, we don't need the patch.  (But might still need improvements in
libraw1394 or/and clients to fail fast when appropriate.)
-- 
Stefan Richter
-=====-==--- ---= ===-=
http://arcgraph.de/sr/

Re: [PATCH update] firewire: fix "kobject_add failed for fw* with -EEXIST"

[Jarod Wilson]

On Sunday 27 January 2008 12:20:40 pm Stefan Richter wrote:
> There is a race between shutdown and creation of devices:  fw-core may
> attempt to add a device with the same name of an already existing
> device.  http://bugzilla.kernel.org/show_bug.cgi?id=9828
>
> Impact of the bug:  Happens rarely, forces the user to unplug and replug
> the new device to get it working.

If you're crazy enough to set up a software raid array on two firewire drives 
that end up contending for the fwX device, its much worse than simply having 
to unplug and replug though, since all hell breaks loose at the fs level and 
the array level.

We may have another issue there though, as when this happened to me, the md 
layer apparently never noticed (after ~6 hours) that one of the array members 
had disappeared -- not sure if that's firewire's fault or md's though... This 
will presumably avoid this situation entirely, but worth noting that there 
may still be somewhere we need to better communicate status to an upper 
layer.

> The fix moves deregistration of the minor number and device_unregister()
> into a common rw_sem protected section.
>
> We also move the ref count increment from fw_device_op_open into an
> rw_sem protected section with the lookup of the device, so that the
> device pointer can't become invalid between lookup and usage.
>
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

Looks straight-forward enough, and I'll give these a spin shortly and see if I 
can reproduce the situation I was hitting with my raid array...


-- 
Jarod Wilson
jwilson@redhat.com

Re: [PATCH update] firewire: fix "kobject_add failed for fw* with -EEXIST"

[Stefan Richter]

Jarod Wilson wrote:
> We may have another issue there though, as when this happened to me, the md 
> layer apparently never noticed (after ~6 hours) that one of the array members 
> had disappeared -- not sure if that's firewire's fault or md's though... This 
> will presumably avoid this situation entirely, but worth noting that there 
> may still be somewhere we need to better communicate status to an upper 
> layer.

I don't know how md ticks, so I have no idea what might have happened there.

Somewhat related:  What if
  - we lose connection to disk "A", represented by scsi_device "a",
  - the SCSI core sets "a" offline,
  - we gain connection to disk "A" again (i.e. it only shortly
    disappeared from the bus from firewire-core's and -sbp2's point
    of view),
  - and firewire-sbp2 adds it as scsi_device "b", even before SCSI
    core got rid of "a"?
No big problem for stand-alone volumes (unless it happens when the
volume is in use), but maybe trouble for md managed volumes.

To smooth such issues out, my longer term goal was to allow brief
periods of disconnection in (firewire-)sbp2.  I.e. the SCSI core
wouldn't notice that "A"/"a" went away, it would only notice that "a"
wasn't accessible for a short time.  I think the Fibre Channel drivers
already support this.  The ieee1394 driver even has a "limbo" for
devices which went away, in order to remember them until they come back,
but sbp2 doesn't use this feature.  (Nobody did the work to enhance sbp2
to utilize the feature.)

BTW, if you unplug and replug a FireWire disk under Mac OS X fairly
quickly, OS X will pretend that nothing happened and let the user
continue using the disk if he hadn't "ejected" it before the brief
connection loss.

Anyhow, we have a few more urgent problems to solve in firewire-sbp2's
reconnection handling before we can think about such extras.
-- 
Stefan Richter
-=====-==--- ---= ===--
http://arcgraph.de/sr/

Re: [PATCH update] firewire: fix "kobject_add failed for fw* with -EEXIST"

[Jarod Wilson]

On Monday 28 January 2008 01:54:14 pm Stefan Richter wrote:
> Jarod Wilson wrote:
> > We may have another issue there though, as when this happened to me, the
> > md layer apparently never noticed (after ~6 hours) that one of the array
> > members had disappeared -- not sure if that's firewire's fault or md's
> > though... This will presumably avoid this situation entirely, but worth
> > noting that there may still be somewhere we need to better communicate
> > status to an upper layer.
>
> I don't know how md ticks, so I have no idea what might have happened
> there.

It looks like firewire is doing the right thing, unregistering the fw* device, 
and the SCSI layer is subsequently removing the appropriate /dev/sd* nodes, 
but for whatever reason, md hasn't a clue this has happened. I can reproduce 
this particular part of the problem by bringing the array up, and then simply 
pulling the firewire cable on one of the drives in the array...

> Somewhat related:  What if
>   - we lose connection to disk "A", represented by scsi_device "a",
>   - the SCSI core sets "a" offline,
>   - we gain connection to disk "A" again (i.e. it only shortly
>     disappeared from the bus from firewire-core's and -sbp2's point
>     of view),
>   - and firewire-sbp2 adds it as scsi_device "b", even before SCSI
>     core got rid of "a"?
> No big problem for stand-alone volumes (unless it happens when the
> volume is in use), but maybe trouble for md managed volumes.

That does appear to be the case. If I reconnect the drive I disconnected, 
which was originally /dev/sdb, it comes back up as /dev/sdd now. So 
apparently, the scsi layer is at least bright enough to see that someone (md) 
is still trying to use /dev/sdb, but I'm clueless as to why md doesn't have 
any idea that /dev/sdb actually went away. :\

> To smooth such issues out, my longer term goal was to allow brief
> periods of disconnection in (firewire-)sbp2.  I.e. the SCSI core
> wouldn't notice that "A"/"a" went away, it would only notice that "a"
> wasn't accessible for a short time.  I think the Fibre Channel drivers
> already support this.  The ieee1394 driver even has a "limbo" for
> devices which went away, in order to remember them until they come back,
> but sbp2 doesn't use this feature.  (Nobody did the work to enhance sbp2
> to utilize the feature.)
>
> BTW, if you unplug and replug a FireWire disk under Mac OS X fairly
> quickly, OS X will pretend that nothing happened and let the user
> continue using the disk if he hadn't "ejected" it before the brief
> connection loss.

Certainly sounds like a feature we'd benefit from having in this particular 
case...

> Anyhow, we have a few more urgent problems to solve in firewire-sbp2's
> reconnection handling before we can think about such extras.

Very true... Perhaps I'll just file this one away a bit down the TODO list for 
now... ;)

-- 
Jarod Wilson
jwilson@redhat.com

Re: [PATCH update] firewire: fix "kobject_add failed for fw* with -EEXIST"

[Stefan Richter]

Jarod Wilson wrote:
> Looks straight-forward enough, and I'll give these a spin shortly and see if I 
> can reproduce the situation I was hitting with my raid array...

As far as the naming of devices is concerned, the bug and the necessary
fix are entirely obvious.

But the interaction with userspace processes opening /dev/fwX while the
respective node is being shut down gave me headaches.  I am still not
entirely sure if I got it right in the patch update, i.e. if it is free
from deadlocks.  fw_device_shutdown() and fw_device_op_open() can be
entered at the same time.  Would device_unregister() have to acquire a
driver core lock which open() already took?  If yes, device_unregister()
would be blocked on this lock while fw_device_op_open() is blocked on
idr_rwsem.

So why did I move device_unregister() into the idr_rwsem protected
section in the first place?  That's because I wanted to guarantee that
fw_device_op_open() wouldn't look up a fw_device which is just in the
process of being unregistered.  But maybe we don't even need this guarantee.

It would all be so easy if I knew how the driver core works.  :-/
-- 
Stefan Richter
-=====-==--- ---= ===--
http://arcgraph.de/sr/

Re: [PATCH update] firewire: fix "kobject_add failed for fw* with -EEXIST"

[Stefan Richter]

I wrote:
> But the interaction with userspace processes opening /dev/fwX while the
> respective node is being shut down gave me headaches.  I am still not
> entirely sure if I got it right in the patch update, i.e. if it is free
> from deadlocks.  fw_device_shutdown() and fw_device_op_open() can be
> entered at the same time.  Would device_unregister() have to acquire a
> driver core lock which open() already took?  If yes, device_unregister()
> would be blocked on this lock while fw_device_op_open() is blocked on
> idr_rwsem.
> 
> So why did I move device_unregister() into the idr_rwsem protected
> section in the first place?  That's because I wanted to guarantee that
> fw_device_op_open() wouldn't look up a fw_device which is just in the
> process of being unregistered.  But maybe we don't even need this guarantee.

Did some further thinking:  Yes, I have to guarantee that the pointer
which the lookup in the idr tree returned is valid.

But I don't have to shove device_unregister() into the idr_rwsem locked
section.  Instead, I can (and actually should) increment the device's
refcount when I stick the device pointer into the idr tree.  (And of
course I decrease the refcount again when I remove the pointer from the
idr tree.)

Will probably post another update tomorrow.
-- 
Stefan Richter
-=====-==--- ---= ===-=
http://arcgraph.de/sr/

[PATCH update 2] firewire: fix "kobject_add failed for fw* with -EEXIST"

[Stefan Richter]

There is a race between shutdown and creation of devices:  fw-core may
attempt to add a device with the same name of an already existing
device.  http://bugzilla.kernel.org/show_bug.cgi?id=9828

Impact of the bug:  Happens rarely (when shutdown of a device coincides
with creation of another), forces the user to unplug and replug the new
device to get it working.

The fix is obvious:  Free the minor number *after* instead of *before*
device_unregister().  This requires to take an additional reference of
the fw_device as long as the IDR tree points to it.

And while we are at it, we fix an additional race condition:
fw_device_op_open() took its reference of the fw_device a little bit too
late, hence was in danger to access an already invalid fw_device.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-cdev.c   |    8 +++++---
 drivers/firewire/fw-device.c |   20 ++++++++++++++------
 drivers/firewire/fw-device.h |    2 +-
 3 files changed, 20 insertions(+), 10 deletions(-)

Index: linux/drivers/firewire/fw-device.c
===================================================================
--- linux.orig/drivers/firewire/fw-device.c
+++ linux/drivers/firewire/fw-device.c
@@ -610,12 +610,14 @@ static DECLARE_RWSEM(idr_rwsem);
 static DEFINE_IDR(fw_device_idr);
 int fw_cdev_major;
 
-struct fw_device *fw_device_from_devt(dev_t devt)
+struct fw_device *fw_device_get_by_devt(dev_t devt)
 {
 	struct fw_device *device;
 
 	down_read(&idr_rwsem);
 	device = idr_find(&fw_device_idr, MINOR(devt));
+	if (device)
+		fw_device_get(device);
 	up_read(&idr_rwsem);
 
 	return device;
@@ -627,13 +629,14 @@ static void fw_device_shutdown(struct wo
 		container_of(work, struct fw_device, work.work);
 	int minor = MINOR(device->device.devt);
 
-	down_write(&idr_rwsem);
-	idr_remove(&fw_device_idr, minor);
-	up_write(&idr_rwsem);
-
 	fw_device_cdev_remove(device);
 	device_for_each_child(&device->device, NULL, shutdown_unit);
 	device_unregister(&device->device);
+
+	down_write(&idr_rwsem);
+	idr_remove(&fw_device_idr, minor);
+	up_write(&idr_rwsem);
+	fw_device_put(device);
 }
 
 static struct device_type fw_device_type = {
@@ -682,10 +685,13 @@ static void fw_device_init(struct work_s
 	}
 
 	err = -ENOMEM;
+
+	fw_device_get(device);
 	down_write(&idr_rwsem);
 	if (idr_pre_get(&fw_device_idr, GFP_KERNEL))
 		err = idr_get_new(&fw_device_idr, device, &minor);
 	up_write(&idr_rwsem);
+
 	if (err < 0)
 		goto error;
 
@@ -741,7 +747,9 @@ static void fw_device_init(struct work_s
 	idr_remove(&fw_device_idr, minor);
 	up_write(&idr_rwsem);
  error:
-	put_device(&device->device);
+	fw_device_put(device);		/* fw_device_idr's reference */
+
+	put_device(&device->device);	/* our reference */
 }
 
 static int update_unit(struct device *dev, void *data)
Index: linux/drivers/firewire/fw-cdev.c
===================================================================
--- linux.orig/drivers/firewire/fw-cdev.c
+++ linux/drivers/firewire/fw-cdev.c
@@ -109,15 +109,17 @@ static int fw_device_op_open(struct inod
 	struct client *client;
 	unsigned long flags;
 
-	device = fw_device_from_devt(inode->i_rdev);
+	device = fw_device_get_by_devt(inode->i_rdev);
 	if (device == NULL)
 		return -ENODEV;
 
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
-	if (client == NULL)
+	if (client == NULL) {
+		fw_device_put(device);
 		return -ENOMEM;
+	}
 
-	client->device = fw_device_get(device);
+	client->device = device;
 	INIT_LIST_HEAD(&client->event_list);
 	INIT_LIST_HEAD(&client->resource_list);
 	spin_lock_init(&client->lock);
Index: linux/drivers/firewire/fw-device.h
===================================================================
--- linux.orig/drivers/firewire/fw-device.h
+++ linux/drivers/firewire/fw-device.h
@@ -77,13 +77,13 @@ fw_device_is_shutdown(struct fw_device *
 }
 
 struct fw_device *fw_device_get(struct fw_device *device);
+struct fw_device *fw_device_get_by_devt(dev_t devt);
 void fw_device_put(struct fw_device *device);
 int fw_device_enable_phys_dma(struct fw_device *device);
 
 void fw_device_cdev_update(struct fw_device *device);
 void fw_device_cdev_remove(struct fw_device *device);
 
-struct fw_device *fw_device_from_devt(dev_t devt);
 extern int fw_cdev_major;
 
 struct fw_unit {

-- 
Stefan Richter
-=====-==--- --=- ---=-
http://arcgraph.de/sr/

Re: [PATCH update] firewire: fix "kobject_add failed for fw* with -EEXIST"

[Jarod Wilson]

On Sunday 27 January 2008 12:20:40 pm Stefan Richter wrote:
> There is a race between shutdown and creation of devices:  fw-core may
> attempt to add a device with the same name of an already existing
> device.  http://bugzilla.kernel.org/show_bug.cgi?id=9828
>
> Impact of the bug:  Happens rarely, forces the user to unplug and replug
> the new device to get it working.
>
> The fix moves deregistration of the minor number and device_unregister()
> into a common rw_sem protected section.
>
> We also move the ref count increment from fw_device_op_open into an
> rw_sem protected section with the lookup of the device, so that the
> device pointer can't become invalid between lookup and usage.
>
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

While I've got remaining issues with firewire mdraid, this does appear fix the 
problem of devices racing the grab the same fw* device node, and it seems 
obvious that should indeed be the case from the code changes.

Signed-off-by: Jarod Wilson <jwilson@redhat.com>

-- 
Jarod Wilson
jwilson@redhat.com