Re: [PATCH] tap: reference to KVA of an unloaded module causes kernel panic

[David Miller]

From: Girish Moodalbail 
Date: Fri, 27 Oct 2017 00:00:16 -0700

> The commit 9a393b5d5988 ("tap: tap as an independent module") created a
> separate tap module that implements tap functionality and exports
> interfaces that will be used by macvtap and ipvtap modules to create
> create respective tap devices.
> 
> However, that patch introduced a regression wherein the modules macvtap
> and ipvtap can be removed (through modprobe -r) while there are
> applications using the respective /dev/tapX devices. These applications
> cause kernel to hold reference to /dev/tapX through 'struct cdev
> macvtap_cdev' and 'struct cdev ipvtap_dev' defined in macvtap and ipvtap
> modules respectively. So,  when the application is later closed the
> kernel panics because we are referencing KVA that is present in the
> unloaded modules.
> 
> --8<--- Example --8<--
> $ sudo ip li add name mv0 link enp7s0 type macvtap
> $ sudo ip li show mv0 |grep mv0| awk -e '{print $1 $2}'
>   14:mv0@enp7s0:
> $ cat /dev/tap14 &
> $ lsmod |egrep -i 'tap|vlan'
> macvtap16384  0
> macvlan24576  1 macvtap
> tap24576  3 macvtap
> $ sudo modprobe -r macvtap
> $ fg
> cat /dev/tap14
> ^C
> 
> <...system panics...>
> BUG: unable to handle kernel paging request at a038c500
> IP: cdev_put+0xf/0x30
> --8<-8<--
> 
> The fix is to set cdev.owner to the module that creates the tap device
> (either macvtap or ipvtap). With this set, the operations (in
> fs/char_dev.c) on char device holds and releases the module through
> cdev_get() and cdev_put() and will not allow the module to unload
> prematurely.
> 
> Fixes: 9a393b5d5988ea4e (tap: tap as an independent module)
> Signed-off-by: Girish Moodalbail 

Applied and queued up for -stable.

[PATCH] tap: reference to KVA of an unloaded module causes kernel panic

[Girish Moodalbail]

The commit 9a393b5d5988 ("tap: tap as an independent module") created a
separate tap module that implements tap functionality and exports
interfaces that will be used by macvtap and ipvtap modules to create
create respective tap devices.

However, that patch introduced a regression wherein the modules macvtap
and ipvtap can be removed (through modprobe -r) while there are
applications using the respective /dev/tapX devices. These applications
cause kernel to hold reference to /dev/tapX through 'struct cdev
macvtap_cdev' and 'struct cdev ipvtap_dev' defined in macvtap and ipvtap
modules respectively. So,  when the application is later closed the
kernel panics because we are referencing KVA that is present in the
unloaded modules.

--8<--- Example --8<--
$ sudo ip li add name mv0 link enp7s0 type macvtap
$ sudo ip li show mv0 |grep mv0| awk -e '{print $1 $2}'
  14:mv0@enp7s0:
$ cat /dev/tap14 &
$ lsmod |egrep -i 'tap|vlan'
macvtap16384  0
macvlan24576  1 macvtap
tap24576  3 macvtap
$ sudo modprobe -r macvtap
$ fg
cat /dev/tap14
^C

<...system panics...>
BUG: unable to handle kernel paging request at a038c500
IP: cdev_put+0xf/0x30
--8<-8<--

The fix is to set cdev.owner to the module that creates the tap device
(either macvtap or ipvtap). With this set, the operations (in
fs/char_dev.c) on char device holds and releases the module through
cdev_get() and cdev_put() and will not allow the module to unload
prematurely.

Fixes: 9a393b5d5988ea4e (tap: tap as an independent module)
Signed-off-by: Girish Moodalbail 
---
 drivers/net/ipvlan/ipvtap.c | 4 ++--
 drivers/net/macvtap.c   | 4 ++--
 drivers/net/tap.c   | 5 +++--
 include/linux/if_tap.h  | 4 ++--
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ipvlan/ipvtap.c b/drivers/net/ipvlan/ipvtap.c
index 5dea206..0bcc07f 100644
--- a/drivers/net/ipvlan/ipvtap.c
+++ b/drivers/net/ipvlan/ipvtap.c
@@ -197,8 +197,8 @@ static int ipvtap_init(void)
 {
int err;
 
-   err = tap_create_cdev(_cdev, _major, "ipvtap");
-
+   err = tap_create_cdev(_cdev, _major, "ipvtap",
+ THIS_MODULE);
if (err)
goto out1;
 
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index c2d0ea2..cba5cb3 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -204,8 +204,8 @@ static int macvtap_init(void)
 {
int err;
 
-   err = tap_create_cdev(_cdev, _major, "macvtap");
-
+   err = tap_create_cdev(_cdev, _major, "macvtap",
+ THIS_MODULE);
if (err)
goto out1;
 
diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index 98ee6cc..1b10fcc 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -1249,8 +1249,8 @@ static int tap_list_add(dev_t major, const char 
*device_name)
return 0;
 }
 
-int tap_create_cdev(struct cdev *tap_cdev,
-   dev_t *tap_major, const char *device_name)
+int tap_create_cdev(struct cdev *tap_cdev, dev_t *tap_major,
+   const char *device_name, struct module *module)
 {
int err;
 
@@ -1259,6 +1259,7 @@ int tap_create_cdev(struct cdev *tap_cdev,
goto out1;
 
cdev_init(tap_cdev, _fops);
+   tap_cdev->owner = module;
err = cdev_add(tap_cdev, *tap_major, TAP_NUM_DEVS);
if (err)
goto out2;
diff --git a/include/linux/if_tap.h b/include/linux/if_tap.h
index 4837157..9ae41cd 100644
--- a/include/linux/if_tap.h
+++ b/include/linux/if_tap.h
@@ -73,8 +73,8 @@ struct tap_queue {
 int tap_get_minor(dev_t major, struct tap_dev *tap);
 void tap_free_minor(dev_t major, struct tap_dev *tap);
 int tap_queue_resize(struct tap_dev *tap);
-int tap_create_cdev(struct cdev *tap_cdev,
-   dev_t *tap_major, const char *device_name);
+int tap_create_cdev(struct cdev *tap_cdev, dev_t *tap_major,
+   const char *device_name, struct module *module);
 void tap_destroy_cdev(dev_t major, struct cdev *tap_cdev);
 
 #endif /*_LINUX_IF_TAP_H_*/
-- 
1.8.3.1