memory allocations dring device removal

memory allocations dring device removal

[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()?

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 <oneukum@suse.com>
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 <oneukum@suse.com>
---
 drivers/base/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index bb5806a2bd4c..509306a4ea89 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -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,
-- 
2.16.4

Re: memory allocations dring device removal

[Greg KH]

On Wed, Sep 16, 2020 at 04:26:51PM +0200, Oliver Neukum wrote:
> 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...

> 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 :(


> 
> Is this patch necessary?
> 
> 	Regards
> 		Oliver
> 
> >From 97b7e91af588b7489795e3eaf773be032bc91b70 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.com>
> 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 <oneukum@suse.com>
> ---
>  drivers/base/core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index bb5806a2bd4c..509306a4ea89 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -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,

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