Re: [PATCH] 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

Re: [PATCH] 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

Re: [PATCH] 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

Re: [PATCH] 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

Re: [PATCH] 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 
>> ---
>>  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 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #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_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

Re: [PATCH] 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 
>> ---
>>  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 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #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_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

Re: [PATCH] usbip: vhc_hcd: prevent module being removed while device are attached

[Greg KH]

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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  
>  #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_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

Re: [PATCH] usbip: vhc_hcd: prevent module being removed while device are attached

[Greg KH]

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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  
>  #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_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