usbip: vhc_hcd: prevent module being removed while device are attached

usbip: vhc_hcd: prevent module being removed while device are attached

[Shuah Khan]

vhci_hcd module can be removed even when devices are attached. Fix to
prevent module removal when devices are still attached.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/usb/usbip/vhci_sysfs.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 48808388ec33..6a54b9aa92be 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -9,6 +9,7 @@
 #include <linux/net.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/module.h>
 
 #include "usbip_common.h"
 #include "vhci.h"
@@ -252,6 +253,8 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
 	if (ret < 0)
 		return -EINVAL;
 
+	module_put(THIS_MODULE);
+
 	usbip_dbg_vhci_sysfs("Leave\n");
 
 	return count;
@@ -302,7 +305,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
 	struct vhci_hcd *vhci_hcd;
 	struct vhci_device *vdev;
 	struct vhci *vhci;
-	int err;
+	int err, ret;
 	unsigned long flags;
 
 	/*
@@ -339,10 +342,18 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
 	else
 		vdev = &vhci->vhci_hcd_hs->vdev[rhport];
 
+	/* get module ref to avoid being removed with active attached devs */
+	if (!try_module_get(THIS_MODULE)) {
+		ret = -EAGAIN;
+		goto module_get_err;
+	}
+
 	/* Extract socket from fd. */
 	socket = sockfd_lookup(sockfd, &err);
-	if (!socket)
-		return -EINVAL;
+	if (!socket) {
+		ret = -EINVAL;
+		goto error;
+	}
 
 	/* now need lock until setting vdev status as used */
 
@@ -362,7 +373,8 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
 		 * Will be retried from userspace
 		 * if there's another free port.
 		 */
-		return -EBUSY;
+		ret = -EBUSY;
+		goto error;
 	}
 
 	dev_info(dev, "pdev(%u) rhport(%u) sockfd(%d)\n",
@@ -386,6 +398,11 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
 	rh_port_connect(vdev, speed);
 
 	return count;
+
+error:
+	module_put(THIS_MODULE);
+module_get_err:
+	return ret;
 }
 static DEVICE_ATTR_WO(attach);
 

usbip: vhc_hcd: prevent module being removed while device are attached

[Greg Kroah-Hartman]

On Mon, Apr 02, 2018 at 02:52:31PM -0600, Shuah Khan wrote:
> vhci_hcd module can be removed even when devices are attached. Fix to
> prevent module removal when devices are still attached.
> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/usb/usbip/vhci_sysfs.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
> index 48808388ec33..6a54b9aa92be 100644
> --- a/drivers/usb/usbip/vhci_sysfs.c
> +++ b/drivers/usb/usbip/vhci_sysfs.c
> @@ -9,6 +9,7 @@
>  #include <linux/net.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> +#include <linux/module.h>
>  
>  #include "usbip_common.h"
>  #include "vhci.h"
> @@ -252,6 +253,8 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
>  	if (ret < 0)
>  		return -EINVAL;
>  
> +	module_put(THIS_MODULE);
> +
>  	usbip_dbg_vhci_sysfs("Leave\n");
>  
>  	return count;
> @@ -302,7 +305,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
>  	struct vhci_hcd *vhci_hcd;
>  	struct vhci_device *vdev;
>  	struct vhci *vhci;
> -	int err;
> +	int err, ret;
>  	unsigned long flags;
>  
>  	/*
> @@ -339,10 +342,18 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
>  	else
>  		vdev = &vhci->vhci_hcd_hs->vdev[rhport];
>  
> +	/* get module ref to avoid being removed with active attached devs */
> +	if (!try_module_get(THIS_MODULE)) {
> +		ret = -EAGAIN;
> +		goto module_get_err;
> +	}

That's really not a good idea, trying to grab your own module reference
is considered racy and we stopped adding that code pattern to the kernel
a long time ago.

What's wrong if you remove the vhci module if devices are attached?  You
can do that today for any other USB host driver, this shouldn't be
"special".

Also, kernel modules are never automatically removed by the system, so
someone has to do this by hand, so they knew what they were doing :)

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usbip: vhc_hcd: prevent module being removed while device are attached

[Shuah Khan]

On 04/03/2018 12:56 AM, Greg KH wrote:
> On Mon, Apr 02, 2018 at 02:52:31PM -0600, Shuah Khan wrote:
>> vhci_hcd module can be removed even when devices are attached. Fix to
>> prevent module removal when devices are still attached.
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/usb/usbip/vhci_sysfs.c | 25 +++++++++++++++++++++----
>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
>> index 48808388ec33..6a54b9aa92be 100644
>> --- a/drivers/usb/usbip/vhci_sysfs.c
>> +++ b/drivers/usb/usbip/vhci_sysfs.c
>> @@ -9,6 +9,7 @@
>>  #include <linux/net.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>> +#include <linux/module.h>
>>  
>>  #include "usbip_common.h"
>>  #include "vhci.h"
>> @@ -252,6 +253,8 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
>>  	if (ret < 0)
>>  		return -EINVAL;
>>  
>> +	module_put(THIS_MODULE);
>> +
>>  	usbip_dbg_vhci_sysfs("Leave\n");
>>  
>>  	return count;
>> @@ -302,7 +305,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
>>  	struct vhci_hcd *vhci_hcd;
>>  	struct vhci_device *vdev;
>>  	struct vhci *vhci;
>> -	int err;
>> +	int err, ret;
>>  	unsigned long flags;
>>  
>>  	/*
>> @@ -339,10 +342,18 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
>>  	else
>>  		vdev = &vhci->vhci_hcd_hs->vdev[rhport];
>>  
>> +	/* get module ref to avoid being removed with active attached devs */
>> +	if (!try_module_get(THIS_MODULE)) {
>> +		ret = -EAGAIN;
>> +		goto module_get_err;
>> +	}
> 
> That's really not a good idea, trying to grab your own module reference
> is considered racy and we stopped adding that code pattern to the kernel
> a long time ago.

Thanks. Good to know.

> 
> What's wrong if you remove the vhci module if devices are attached?  You
> can do that today for any other USB host driver, this shouldn't be
> "special".

This is a virtual device associated to a real physical device on a different
system. My concern is that if the module gets removed accidentally then it
could disrupt access to the remote device. The remote nature of the device
with several players involved makes this scenario a bit more complex than
a regular usb case when device is physically on the system. It is probably one
of the minor problems that could happen with a remote device.

I found a few modules that hold reference to themselves in the kernel. Block,
crypto, net, md, vfio, nfs.

md for example holds reference to itself when it creates a blank device.
vfio get regerence to itself it when opens pci and mediated devices.
Some network drivers do the same for handling virtual functions.
nfs does this for rpc handling.

I am not making a case for adding one more, however I am curious if this
vhci_hcd falls into a similar special case of handling virtual connections
and devices.

> 
> Also, kernel modules are never automatically removed by the system, so
> someone has to do this by hand, so they knew what they were doing :)
> 

Yes. This will not be a usual scenario. The question is whether not kernel
should detect driver is in use and prevent it.

thanks,
-- Shuah
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usbip: vhc_hcd: prevent module being removed while device are attached

[Oliver Neukum]

Am Dienstag, den 03.04.2018, 09:56 -0600 schrieb Shuah Khan:
> This is a virtual device associated to a real physical device on a different
> system. My concern is that if the module gets removed accidentally then it
> could disrupt access to the remote device. The remote nature of the device
> with several players involved makes this scenario a bit more complex than

Hi,

you would doubtlessly lose connection to that device. Yet you would
also lose connections if you down your network. You need to be root
to unload a module. You could overwrite your root filesystems or flash
your firmware. In general we cannot and don't try to protect root
from such accidents.

	Regards
		Oliver
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usbip: vhc_hcd: prevent module being removed while device are attached

[Shuah Khan]

On 04/04/2018 02:25 AM, Oliver Neukum wrote:
> Am Dienstag, den 03.04.2018, 09:56 -0600 schrieb Shuah Khan:
>> This is a virtual device associated to a real physical device on a different
>> system. My concern is that if the module gets removed accidentally then it
>> could disrupt access to the remote device. The remote nature of the device
>> with several players involved makes this scenario a bit more complex than
> 
> Hi,
> 
> you would doubtlessly lose connection to that device. Yet you would
> also lose connections if you down your network. You need to be root
> to unload a module. You could overwrite your root filesystems or flash
> your firmware. In general we cannot and don't try to protect root
> from such accidents.
> 

Yes. There are several scenarios that could disrupt access. There are
no bad things happening other than the expected read failures when the
device is actively in use.

thanks for the review.
-- Shuah
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usbip: vhc_hcd: prevent module being removed while device are attached

[Sasha Levin]

Hi.

[This is an automated email]

This commit has been processed by the -stable helper bot and determined
to be a high probability candidate for -stable trees. (score: 13.1846)

The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, v4.4.126, 

v4.15.15: Build OK!
v4.14.32: Build OK!
v4.9.92: Failed to apply! Possible dependencies:
    3920ad4951e2 ("usbip: vhc_hcd: prevent module being removed while device are attached")
    a38711a88b7e ("usbip: auto retry for concurrent attach")

v4.4.126: Failed to apply! Possible dependencies:
    0775a9cbc694 ("usbip: vhci extension: modifications to vhci driver")
    3920ad4951e2 ("usbip: vhc_hcd: prevent module being removed while device are attached")
    a38711a88b7e ("usbip: auto retry for concurrent attach")


Please let us know if you'd like to have this patch included in a stable tree.
---
Thanks.
Sasha--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usbip: vhc_hcd: prevent module being removed while device are attached

[Shuah Khan]

On 04/05/2018 10:42 AM, Sasha Levin wrote:
> Hi.
> 
> [This is an automated email]
> 
> This commit has been processed by the -stable helper bot and determined
> to be a high probability candidate for -stable trees. (score: 13.1846)
> 
> The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, v4.4.126, 
> 
> v4.15.15: Build OK!
> v4.14.32: Build OK!
> v4.9.92: Failed to apply! Possible dependencies:
>     3920ad4951e2 ("usbip: vhc_hcd: prevent module being removed while device are attached"> 
> v4.4.126: Failed to apply! Possible dependencies:
>     0775a9cbc694 ("usbip: vhci extension: modifications to vhci driver")
>     3920ad4951e2 ("usbip: vhc_hcd: prevent module being removed while device are attached")

This patch will not go into Linus's tree. Please don't pull it into stables.

thanks,
-- Shuah
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usbip: vhc_hcd: prevent module being removed while device are attached

[Sasha Levin]

On Thu, Apr 05, 2018 at 10:54:59AM -0600, Shuah Khan wrote:
>On 04/05/2018 10:42 AM, Sasha Levin wrote:
>> Hi.
>>
>> [This is an automated email]
>>
>> This commit has been processed by the -stable helper bot and determined
>> to be a high probability candidate for -stable trees. (score: 13.1846)
>>
>> The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, v4.4.126,
>>
>> v4.15.15: Build OK!
>> v4.14.32: Build OK!
>> v4.9.92: Failed to apply! Possible dependencies:
>>     3920ad4951e2 ("usbip: vhc_hcd: prevent module being removed while device are attached">
>> v4.4.126: Failed to apply! Possible dependencies:
>>     0775a9cbc694 ("usbip: vhci extension: modifications to vhci driver")
>>     3920ad4951e2 ("usbip: vhc_hcd: prevent module being removed while device are attached")
>
>This patch will not go into Linus's tree. Please don't pull it into stables.

Commits indeed won't be picked up unless they make it to Linus' tree
(per the -stable rules). I'll clarify that in future mails, thank you!--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usbip: vhc_hcd: prevent module being removed while device are attached

[Greg KH]

On Thu, Apr 05, 2018 at 04:42:46PM +0000, Sasha Levin wrote:
> Hi.
> 
> [This is an automated email]

Why are you running this against patches that are not yet in Linus's
tree?  That feels odd.  Only about 1/3 of the patches posted end up
being merged (at best), so don't do extra work by running this on
patches that might not even be merged at all.

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usbip: vhc_hcd: prevent module being removed while device are attached

[Sasha Levin]

On Thu, Apr 05, 2018 at 09:11:02PM +0200, Greg KH wrote:
>On Thu, Apr 05, 2018 at 04:42:46PM +0000, Sasha Levin wrote:
>> Hi.
>>
>> [This is an automated email]
>
>Why are you running this against patches that are not yet in Linus's
>tree?  That feels odd.  Only about 1/3 of the patches posted end up
>being merged (at best), so don't do extra work by running this on
>patches that might not even be merged at all.

Sorry for late response, this got flagged as spam :/

I was testing out a different approach, that attempts to address the
following issues:

 - When we send a review request for a patch, it usually happens weeks
   after the patch went upstream. At that point developers have moved
   on, and we're less likely to get a helpful response.

 - Some patches are tagged for particular stable trees, but don't end up
   applying/failing build on them. Usually when you send your rejection
   mails for these patches, we see no response. This might mean that
   kernels that need a particular bugfix get missed because of things
   like trivial dependencies.

 - I was planning to integrate a larger set of testing to be done for
   these patches both on mainline, and on stable kernels. Like the XFS
   example, we could run xfstests for any given patch both on mainline
   and on any older stable trees this patch applied to, This would both
   help mainline development, and would encourage devs to address any
   issues it causes on stable kernels as well.--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html