memory allocations dring device removal

PATCH Oliver Neukum 2 participants 2 messages Sep 16, 2020

Subsystems (2)

USB Driver core

lore

Share & tools

lore

Oliver Neukum

author

2020-09-16 20:53 UTC #0 lore raw

Hi,

I am cleaning up house, electronically speaking. Reading the thread about the keyboard with the storage device reminded me about a potential issue. What happens if you allocate memory during disconnect()?

If the storage device is second, the storage driver will still be bound and the SCSI device will still exist. The kernel may use it to launder pages. This will fail, as the device is physically gone. So can we deadlock?

Is this patch necessary?

Regards
Oliver

From 97b7e91af588b7489795e3eaf773be032bc91b70 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <[email protected]>
Date: Tue, 28 May 2019 11:43:02 +0200
Subject: [PATCH] base: force NOIO allocations during unplug

There is one overlooked situation under which a driver must not do IO to allocate memory. You cannot do that while disconnecting a device. A device being disconnected is no longer functional in most cases, yet IO may fail only when the handler runs.

Signed-off-by Oliver Neukum <[email protected]>

---
 drivers/base/core.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/base/core.c +4 -0

index bb5806a2bd4c..509306a4ea89 100644
@@ -26,6 +26,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/netdevice.h>
 #include <linux/sched/signal.h>
+#include <linux/sched/mm.h>
 #include <linux/sysfs.h>
 
 #include "base.h"

@@ -3062,6 +3063,7 @@ void device_del(struct device *dev)
 	struct device *parent = dev->parent;
 	struct kobject *glue_dir = NULL;
 	struct class_interface *class_intf;
+	unsigned int noio_flag;
 
 	device_lock(dev);
 	kill_device(dev);

@@ -3073,6 +3075,7 @@ void device_del(struct device *dev)
 	/* Notify clients of device removal.  This call must come
 	 * before dpm_sysfs_remove().
 	 */
+	noio_flag = memalloc_noio_save();
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_DEL_DEVICE, dev);

@@ -3106,6 +3109,7 @@ void device_del(struct device *dev)
 	device_platform_notify(dev, KOBJ_REMOVE);
 	device_remove_properties(dev);
 	device_links_purge(dev);
+	memalloc_noio_restore(noio_flag);
 
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,

Signature · 1 line

2.16.4

Greg KH

maintainer

2020-09-16 20:35 UTC #1 lore raw

Quoted 6 lines · Oliver Neukum

Hi,
I am cleaning up house, electronically speaking. Reading the thread about the keyboard with the storage device reminded me about a potential issue. What happens if you allocate memory during disconnect()?

Does any driver do that today?

If so, I think we might have seen issues there already, but perhaps not, it's a tricky codepath...

Quoted 4 lines

If the storage device is second, the storage driver will still be bound and the SCSI device will still exist. The kernel may use it to launder pages. This will fail, as the device is physically gone. So can we deadlock?

The notifier callback worries me, who knows what can happen on that code path. So yes, I think your patch might be needed :(

Quoted 51 lines

Is this patch necessary?
Regards
Oliver
Quoted 1 line
From 97b7e91af588b7489795e3eaf773be032bc91b70 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <[email protected]>
Date: Tue, 28 May 2019 11:43:02 +0200
Subject: [PATCH] base: force NOIO allocations during unplug
There is one overlooked situation under which a driver must not do IO to allocate memory. You cannot do that while disconnecting a device. A device being disconnected is no longer functional in most cases, yet IO may fail only when the handler runs.
Signed-off-by Oliver Neukum <[email protected]>
---
 drivers/base/core.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/base/core.c +3 -0
index bb5806a2bd4c..509306a4ea89 100644
@@ -26,6 +26,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/netdevice.h>
 #include <linux/sched/signal.h>
+#include <linux/sched/mm.h>
 #include <linux/sysfs.h>
 
 #include "base.h"
@@ -3062,6 +3063,7 @@ void device_del(struct device *dev)
 	struct device *parent = dev->parent;
 	struct kobject *glue_dir = NULL;
 	struct class_interface *class_intf;
+	unsigned int noio_flag;
 
 	device_lock(dev);
 	kill_device(dev);
@@ -3073,6 +3075,7 @@ void device_del(struct device *dev)
 	/* Notify clients of device removal.  This call must come
 	 * before dpm_sysfs_remove().
 	 */
+	noio_flag = memalloc_noio_save();
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_DEL_DEVICE, dev);

drivers/base/core.c void device_del(struct device *dev) · line 3115

Quoted 7 lines

drivers/base/core.c +1 -0

@@ -3106,6 +3109,7 @@ void device_del(struct device *dev)
 	device_platform_notify(dev, KOBJ_REMOVE);
 	device_remove_properties(dev);
 	device_links_purge(dev);
+	memalloc_noio_restore(noio_flag);
 
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,

Can you move memalloc_noio_restore() down below here, odds are it should just go after the last put_device(parent) to be safe.

thanks,

greg k-h