Date:   Wed, 16 Sep 2020 17:25:16 +0200
From:   Greg KH <gregKH@linuxfoundation.org>
To:     Oliver Neukum <oneukum@suse.de>
Cc:     Alan Stern <stern@rowland.harvard.edu>, linux-usb@vger.kernel.org
Subject: Re: memory allocations dring device removal
Message-ID: <20200916152516.GC3018065@kroah.com>
References: <1600266411.2424.32.camel@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1600266411.2424.32.camel@suse.de>
Sender: linux-usb-owner@vger.kernel.org
Precedence: bulk

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