kni: check abi version between kmod and lib

kni: check abi version between kmod and lib

[Stephen Coleman]

KNI ioctl functions copy data from userspace lib, and this interface
of kmod is not compatible indeed. If the user use incompatible rte_kni.ko
bad things happen: sometimes various fields contain garbage value,
sometimes it cause a kmod soft lockup.

Some common distros ship their own rte_kni.ko, so this is likely to
happen.

This patch add abi version checking between userland lib and kmod so
that:

* if kmod ioctl got a wrong abi magic, it refuse to go on
* if userland lib, probed a wrong abi version via newly added ioctl, it
  also refuse to go on

Bugzilla ID: 998

Signed-off-by: youcai <omegacoleman@gmail.com>
---
 kernel/linux/kni/kni_misc.c | 38 +++++++++++++++++++++++++++++++++++++
 lib/kni/rte_kni.c           | 16 ++++++++++++++++
 lib/kni/rte_kni_common.h    | 11 +++++++++++
 3 files changed, 65 insertions(+)

diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
index 780187d8bf..cd9a05d8c1 100644
--- a/kernel/linux/kni/kni_misc.c
+++ b/kernel/linux/kni/kni_misc.c
@@ -236,12 +236,24 @@ kni_release(struct inode *inode, struct file *file)
     return 0;
 }

+static int kni_check_abi_version_magic(uint16_t abi_version_magic) {
+    if (abi_version_magic != RTE_KNI_ABI_VERSION_MAGIC) {
+        pr_err("KNI kmod ABI incompatible with librte_kni -- %u\n",
+               RTE_KNI_ABI_VERSION_FROM_MAGIC(abi_version_magic));
+        return -1;
+    }
+    return 0;
+}
+
 static int
 kni_check_param(struct kni_dev *kni, struct rte_kni_device_info *dev)
 {
     if (!kni || !dev)
         return -1;

+    if (kni_check_abi_version_magic(dev->abi_version_magic) < 0)
+        return -1;
+
     /* Check if network name has been used */
     if (!strncmp(kni->name, dev->name, RTE_KNI_NAMESIZE)) {
         pr_err("KNI name %s duplicated\n", dev->name);
@@ -301,12 +313,19 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
     struct rte_kni_device_info dev_info;
     struct net_device *net_dev = NULL;
     struct kni_dev *kni, *dev, *n;
+    uint16_t abi_version_magic;

     pr_info("Creating kni...\n");
     /* Check the buffer size, to avoid warning */
     if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
         return -EINVAL;

+    /* perform abi check ahead of copy, to avoid possible violation */
+    if (copy_from_user(&abi_version_magic, (void *)ioctl_param,
sizeof(uint16_t)))
+        return -EFAULT;
+    if (kni_check_abi_version_magic(abi_version_magic) < 0)
+        return -EINVAL;
+
     /* Copy kni info from user space */
     if (copy_from_user(&dev_info, (void *)ioctl_param, sizeof(dev_info)))
         return -EFAULT;
@@ -451,10 +470,17 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
     int ret = -EINVAL;
     struct kni_dev *dev, *n;
     struct rte_kni_device_info dev_info;
+    uint16_t abi_version_magic;

     if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
         return -EINVAL;

+    /* perform abi check ahead of copy, to avoid possible violation */
+    if (copy_from_user(&abi_version_magic, (void *)ioctl_param,
sizeof(uint16_t)))
+        return -EFAULT;
+    if (kni_check_abi_version_magic(abi_version_magic) < 0)
+        return -EINVAL;
+
     if (copy_from_user(&dev_info, (void *)ioctl_param, sizeof(dev_info)))
         return -EFAULT;

@@ -484,6 +510,15 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
     return ret;
 }

+static int
+kni_ioctl_abi_version(struct net *net, uint32_t ioctl_num,
+        unsigned long ioctl_param)
+{
+    uint16_t abi_version = RTE_KNI_ABI_VERSION;
+    copy_to_user((void *)ioctl_param, &abi_version, sizeof(uint16_t));
+    return 0;
+}
+
 static long
 kni_ioctl(struct file *file, unsigned int ioctl_num, unsigned long ioctl_param)
 {
@@ -505,6 +540,9 @@ kni_ioctl(struct file *file, unsigned int
ioctl_num, unsigned long ioctl_param)
     case _IOC_NR(RTE_KNI_IOCTL_RELEASE):
         ret = kni_ioctl_release(net, ioctl_num, ioctl_param);
         break;
+    case _IOC_NR(RTE_KNI_IOCTL_ABI_VERSION):
+        ret = kni_ioctl_abi_version(net, ioctl_num, ioctl_param);
+        break;
     default:
         pr_debug("IOCTL default\n");
         break;
diff --git a/lib/kni/rte_kni.c b/lib/kni/rte_kni.c
index 7971c56bb4..d7116e582c 100644
--- a/lib/kni/rte_kni.c
+++ b/lib/kni/rte_kni.c
@@ -113,6 +113,19 @@ rte_kni_init(unsigned int max_kni_ifaces __rte_unused)
         }
     }

+    uint16_t abi_version;
+    int ret = ioctl(kni_fd, RTE_KNI_IOCTL_ABI_VERSION, &abi_version);
+    if (ret < 0) {
+        RTE_LOG(ERR, KNI, "Cannot verify rte_kni kmod ABI version:
ioctl failed\n");
+        return -1;
+    }
+    if (abi_version != RTE_KNI_ABI_VERSION) {
+        RTE_LOG(ERR, KNI,
+          "rte_kni kmod ABI version mismatch: "
+          "need %" PRIu16 " got %" PRIu16 "\n", RTE_KNI_ABI_VERSION,
abi_version);
+        return -1;
+    }
+
     return 0;
 }

@@ -255,6 +268,7 @@ rte_kni_alloc(struct rte_mempool *pktmbuf_pool,
         kni->ops.port_id = UINT16_MAX;

     memset(&dev_info, 0, sizeof(dev_info));
+    dev_info.abi_version_magic = RTE_KNI_ABI_VERSION_MAGIC;
     dev_info.core_id = conf->core_id;
     dev_info.force_bind = conf->force_bind;
     dev_info.group_id = conf->group_id;
@@ -409,6 +423,8 @@ rte_kni_release(struct rte_kni *kni)
     if (!kni)
         return -1;

+    dev_info.abi_version_magic = RTE_KNI_ABI_VERSION_MAGIC;
+
     kni_list = RTE_TAILQ_CAST(rte_kni_tailq.head, rte_kni_list);

     rte_mcfg_tailq_write_lock();
diff --git a/lib/kni/rte_kni_common.h b/lib/kni/rte_kni_common.h
index 8d3ee0fa4f..c353043cb6 100644
--- a/lib/kni/rte_kni_common.h
+++ b/lib/kni/rte_kni_common.h
@@ -26,6 +26,14 @@ extern "C" {

 #define RTE_CACHE_LINE_MIN_SIZE 64

+/*
+ * Ascend this number if ABI is altered between kmod and userland lib
+ */
+#define RTE_KNI_ABI_VERSION 1
+#define RTE_KNI_ABI_VERSION_MAGIC_MASK 0xAAAA
+#define RTE_KNI_ABI_VERSION_MAGIC (((RTE_KNI_ABI_VERSION) ^
RTE_KNI_ABI_VERSION_MAGIC_MASK))
+#define RTE_KNI_ABI_VERSION_FROM_MAGIC(__magic) (((__magic) ^
RTE_KNI_ABI_VERSION_MAGIC_MASK))
+
 /*
  * Request id.
  */
@@ -102,6 +110,8 @@ struct rte_kni_mbuf {
  */

 struct rte_kni_device_info {
+    uint16_t abi_version_magic;
+
     char name[RTE_KNI_NAMESIZE];  /**< Network device name for KNI */

     phys_addr_t tx_phys;
@@ -139,6 +149,7 @@ struct rte_kni_device_info {
 #define RTE_KNI_IOCTL_TEST    _IOWR(0, 1, int)
 #define RTE_KNI_IOCTL_CREATE  _IOWR(0, 2, struct rte_kni_device_info)
 #define RTE_KNI_IOCTL_RELEASE _IOWR(0, 3, struct rte_kni_device_info)
+#define RTE_KNI_IOCTL_ABI_VERSION _IOWR(0, 4, uint16_t)

 #ifdef __cplusplus
 }
-- 
2.30.1

Re: kni: check abi version between kmod and lib

[Ray Kinsella]

Stephen Coleman <omegacoleman@gmail.com> writes:

> KNI ioctl functions copy data from userspace lib, and this interface
> of kmod is not compatible indeed. If the user use incompatible rte_kni.ko
> bad things happen: sometimes various fields contain garbage value,
> sometimes it cause a kmod soft lockup.
>
> Some common distros ship their own rte_kni.ko, so this is likely to
> happen.
>
> This patch add abi version checking between userland lib and kmod so
> that:
>
> * if kmod ioctl got a wrong abi magic, it refuse to go on
> * if userland lib, probed a wrong abi version via newly added ioctl, it
>   also refuse to go on
>
> Bugzilla ID: 998
>
> Signed-off-by: youcai <omegacoleman@gmail.com>
> ---
>  kernel/linux/kni/kni_misc.c | 38 +++++++++++++++++++++++++++++++++++++
>  lib/kni/rte_kni.c           | 16 ++++++++++++++++
>  lib/kni/rte_kni_common.h    | 11 +++++++++++
>  3 files changed, 65 insertions(+)
>
> diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
> index 780187d8bf..cd9a05d8c1 100644
> --- a/kernel/linux/kni/kni_misc.c
> +++ b/kernel/linux/kni/kni_misc.c
> @@ -236,12 +236,24 @@ kni_release(struct inode *inode, struct file *file)
>      return 0;
>  }
>
> +static int kni_check_abi_version_magic(uint16_t abi_version_magic) {
> +    if (abi_version_magic != RTE_KNI_ABI_VERSION_MAGIC) {
> +        pr_err("KNI kmod ABI incompatible with librte_kni -- %u\n",
> +               RTE_KNI_ABI_VERSION_FROM_MAGIC(abi_version_magic));
> +        return -1;
> +    }
> +    return 0;
> +}
> +
>  static int
>  kni_check_param(struct kni_dev *kni, struct rte_kni_device_info *dev)
>  {
>      if (!kni || !dev)
>          return -1;
>
> +    if (kni_check_abi_version_magic(dev->abi_version_magic) < 0)
> +        return -1;
> +
>      /* Check if network name has been used */
>      if (!strncmp(kni->name, dev->name, RTE_KNI_NAMESIZE)) {
>          pr_err("KNI name %s duplicated\n", dev->name);
> @@ -301,12 +313,19 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
>      struct rte_kni_device_info dev_info;
>      struct net_device *net_dev = NULL;
>      struct kni_dev *kni, *dev, *n;
> +    uint16_t abi_version_magic;
>
>      pr_info("Creating kni...\n");
>      /* Check the buffer size, to avoid warning */
>      if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
>          return -EINVAL;
>
> +    /* perform abi check ahead of copy, to avoid possible violation */
> +    if (copy_from_user(&abi_version_magic, (void *)ioctl_param,
> sizeof(uint16_t)))
> +        return -EFAULT;
> +    if (kni_check_abi_version_magic(abi_version_magic) < 0)
> +        return -EINVAL;
> +
>      /* Copy kni info from user space */
>      if (copy_from_user(&dev_info, (void *)ioctl_param, sizeof(dev_info)))
>          return -EFAULT;
> @@ -451,10 +470,17 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
>      int ret = -EINVAL;
>      struct kni_dev *dev, *n;
>      struct rte_kni_device_info dev_info;
> +    uint16_t abi_version_magic;
>
>      if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
>          return -EINVAL;
>
> +    /* perform abi check ahead of copy, to avoid possible violation */
> +    if (copy_from_user(&abi_version_magic, (void *)ioctl_param,
> sizeof(uint16_t)))
> +        return -EFAULT;
> +    if (kni_check_abi_version_magic(abi_version_magic) < 0)
> +        return -EINVAL;
> +
>      if (copy_from_user(&dev_info, (void *)ioctl_param, sizeof(dev_info)))
>          return -EFAULT;
>
> @@ -484,6 +510,15 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
>      return ret;
>  }
>
> +static int
> +kni_ioctl_abi_version(struct net *net, uint32_t ioctl_num,
> +        unsigned long ioctl_param)
> +{
> +    uint16_t abi_version = RTE_KNI_ABI_VERSION;
> +    copy_to_user((void *)ioctl_param, &abi_version, sizeof(uint16_t));
> +    return 0;
> +}
> +
>  static long
>  kni_ioctl(struct file *file, unsigned int ioctl_num, unsigned long ioctl_param)
>  {
> @@ -505,6 +540,9 @@ kni_ioctl(struct file *file, unsigned int
> ioctl_num, unsigned long ioctl_param)
>      case _IOC_NR(RTE_KNI_IOCTL_RELEASE):
>          ret = kni_ioctl_release(net, ioctl_num, ioctl_param);
>          break;
> +    case _IOC_NR(RTE_KNI_IOCTL_ABI_VERSION):
> +        ret = kni_ioctl_abi_version(net, ioctl_num, ioctl_param);
> +        break;
>      default:
>          pr_debug("IOCTL default\n");
>          break;
> diff --git a/lib/kni/rte_kni.c b/lib/kni/rte_kni.c
> index 7971c56bb4..d7116e582c 100644
> --- a/lib/kni/rte_kni.c
> +++ b/lib/kni/rte_kni.c
> @@ -113,6 +113,19 @@ rte_kni_init(unsigned int max_kni_ifaces __rte_unused)
>          }
>      }
>
> +    uint16_t abi_version;
> +    int ret = ioctl(kni_fd, RTE_KNI_IOCTL_ABI_VERSION, &abi_version);
> +    if (ret < 0) {
> +        RTE_LOG(ERR, KNI, "Cannot verify rte_kni kmod ABI version:
> ioctl failed\n");
> +        return -1;
> +    }
> +    if (abi_version != RTE_KNI_ABI_VERSION) {
> +        RTE_LOG(ERR, KNI,
> +          "rte_kni kmod ABI version mismatch: "
> +          "need %" PRIu16 " got %" PRIu16 "\n", RTE_KNI_ABI_VERSION,
> abi_version);
> +        return -1;
> +    }
> +
>      return 0;
>  }
>
> @@ -255,6 +268,7 @@ rte_kni_alloc(struct rte_mempool *pktmbuf_pool,
>          kni->ops.port_id = UINT16_MAX;
>
>      memset(&dev_info, 0, sizeof(dev_info));
> +    dev_info.abi_version_magic = RTE_KNI_ABI_VERSION_MAGIC;
>      dev_info.core_id = conf->core_id;
>      dev_info.force_bind = conf->force_bind;
>      dev_info.group_id = conf->group_id;
> @@ -409,6 +423,8 @@ rte_kni_release(struct rte_kni *kni)
>      if (!kni)
>          return -1;
>
> +    dev_info.abi_version_magic = RTE_KNI_ABI_VERSION_MAGIC;
> +
>      kni_list = RTE_TAILQ_CAST(rte_kni_tailq.head, rte_kni_list);
>
>      rte_mcfg_tailq_write_lock();
> diff --git a/lib/kni/rte_kni_common.h b/lib/kni/rte_kni_common.h
> index 8d3ee0fa4f..c353043cb6 100644
> --- a/lib/kni/rte_kni_common.h
> +++ b/lib/kni/rte_kni_common.h
> @@ -26,6 +26,14 @@ extern "C" {
>
>  #define RTE_CACHE_LINE_MIN_SIZE 64
>
> +/*
> + * Ascend this number if ABI is altered between kmod and userland lib
> + */
> +#define RTE_KNI_ABI_VERSION 1

Instead of creating a new magic number, could you reference the
ABI_VERSION instead. 

> +#define RTE_KNI_ABI_VERSION_MAGIC_MASK 0xAAAA
> +#define RTE_KNI_ABI_VERSION_MAGIC (((RTE_KNI_ABI_VERSION) ^
> RTE_KNI_ABI_VERSION_MAGIC_MASK))
> +#define RTE_KNI_ABI_VERSION_FROM_MAGIC(__magic) (((__magic) ^
> RTE_KNI_ABI_VERSION_MAGIC_MASK))
> +
>  /*
>   * Request id.
>   */
> @@ -102,6 +110,8 @@ struct rte_kni_mbuf {
>   */
>
>  struct rte_kni_device_info {
> +    uint16_t abi_version_magic;
> +
>      char name[RTE_KNI_NAMESIZE];  /**< Network device name for KNI */
>
>      phys_addr_t tx_phys;
> @@ -139,6 +149,7 @@ struct rte_kni_device_info {
>  #define RTE_KNI_IOCTL_TEST    _IOWR(0, 1, int)
>  #define RTE_KNI_IOCTL_CREATE  _IOWR(0, 2, struct rte_kni_device_info)
>  #define RTE_KNI_IOCTL_RELEASE _IOWR(0, 3, struct rte_kni_device_info)
> +#define RTE_KNI_IOCTL_ABI_VERSION _IOWR(0, 4, uint16_t)
>
>  #ifdef __cplusplus
>  }


-- 
Regards, Ray K

Re: kni: check abi version between kmod and lib

[Stephen Hemminger]

On Thu, 21 Apr 2022 12:38:26 +0800
Stephen Coleman <omegacoleman@gmail.com> wrote:

> KNI ioctl functions copy data from userspace lib, and this interface
> of kmod is not compatible indeed. If the user use incompatible rte_kni.ko
> bad things happen: sometimes various fields contain garbage value,
> sometimes it cause a kmod soft lockup.
> 
> Some common distros ship their own rte_kni.ko, so this is likely to
> happen.
> 
> This patch add abi version checking between userland lib and kmod so
> that:
> 
> * if kmod ioctl got a wrong abi magic, it refuse to go on
> * if userland lib, probed a wrong abi version via newly added ioctl, it
>   also refuse to go on
> 
> Bugzilla ID: 998


Kernel API's are supposed to be 99% stable.
If this driver was playing by the upstream kernel rules this would not
have happened.

Re: kni: check abi version between kmod and lib

[Ray Kinsella]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Thu, 21 Apr 2022 12:38:26 +0800
> Stephen Coleman <omegacoleman@gmail.com> wrote:
>
>> KNI ioctl functions copy data from userspace lib, and this interface
>> of kmod is not compatible indeed. If the user use incompatible rte_kni.ko
>> bad things happen: sometimes various fields contain garbage value,
>> sometimes it cause a kmod soft lockup.
>> 
>> Some common distros ship their own rte_kni.ko, so this is likely to
>> happen.
>> 
>> This patch add abi version checking between userland lib and kmod so
>> that:
>> 
>> * if kmod ioctl got a wrong abi magic, it refuse to go on
>> * if userland lib, probed a wrong abi version via newly added ioctl, it
>>   also refuse to go on
>> 
>> Bugzilla ID: 998
>
>
> Kernel API's are supposed to be 99% stable.
> If this driver was playing by the upstream kernel rules this would not
> have happened.

Well look, it is out-of-tree and never likely to be in-tree, so those
rules don't apply. Making sure the ABI doesn't change during the ABI
stablity period, should be good enough?

-- 
Regards, Ray K

Re: kni: check abi version between kmod and lib

[Stephen Hemminger]

On Thu, 21 Apr 2022 11:40:00 -0400
Ray Kinsella <mdr@ashroe.eu> wrote:

> Stephen Hemminger <stephen@networkplumber.org> writes:
> 
> > On Thu, 21 Apr 2022 12:38:26 +0800
> > Stephen Coleman <omegacoleman@gmail.com> wrote:
> >  
> >> KNI ioctl functions copy data from userspace lib, and this interface
> >> of kmod is not compatible indeed. If the user use incompatible rte_kni.ko
> >> bad things happen: sometimes various fields contain garbage value,
> >> sometimes it cause a kmod soft lockup.
> >> 
> >> Some common distros ship their own rte_kni.ko, so this is likely to
> >> happen.
> >> 
> >> This patch add abi version checking between userland lib and kmod so
> >> that:
> >> 
> >> * if kmod ioctl got a wrong abi magic, it refuse to go on
> >> * if userland lib, probed a wrong abi version via newly added ioctl, it
> >>   also refuse to go on
> >> 
> >> Bugzilla ID: 998  
> >
> >
> > Kernel API's are supposed to be 99% stable.
> > If this driver was playing by the upstream kernel rules this would not
> > have happened.  
> 
> Well look, it is out-of-tree and never likely to be in-tree, so those
> rules don't apply. Making sure the ABI doesn't change during the ABI
> stablity period, should be good enough?
> 

I think if KNI changes, it should just add more ioctl numbers and
be compatible, it is not that hard.

[PATCH v2] kni: check abi version between kmod and lib

[youcai]

KNI ioctl functions copy data from userspace lib, and this interface
of kmod is not compatible indeed. If the user use incompatible rte_kni.ko
bad things happen: sometimes various fields contain garbage value,
sometimes it cause a kmod soft lockup.

Some common distros ship their own rte_kni.ko, so this is likely to
happen.

This patch add abi version checking between userland lib and kmod so
that:

* if kmod ioctl got a wrong abi magic, it refuse to go on
* if userland lib, probed a wrong abi version via newly added ioctl, it
  also refuse to go on

Bugzilla ID: 998

Signed-off-by: youcai <omegacoleman@gmail.com>

---
V2: use ABI_VERSION instead of a new magic
V2: fix some indent
---
 kernel/linux/kni/kni_misc.c  | 40 ++++++++++++++++++++++++++++++++++++
 kernel/linux/kni/meson.build |  4 ++--
 lib/kni/meson.build          |  1 +
 lib/kni/rte_kni.c            | 17 +++++++++++++++
 lib/kni/rte_kni_abi.h        | 17 +++++++++++++++
 lib/kni/rte_kni_common.h     |  3 +++
 6 files changed, 80 insertions(+), 2 deletions(-)
 create mode 100644 lib/kni/rte_kni_abi.h

diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
index 780187d8bf..9a1ed22fed 100644
--- a/kernel/linux/kni/kni_misc.c
+++ b/kernel/linux/kni/kni_misc.c
@@ -17,6 +17,7 @@
 #include <net/netns/generic.h>
 
 #include <rte_kni_common.h>
+#include <rte_kni_abi.h>
 
 #include "compat.h"
 #include "kni_dev.h"
@@ -236,12 +237,24 @@ kni_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int kni_check_abi_version_magic(uint16_t abi_version_magic) {
+	if (abi_version_magic != RTE_KNI_ABI_VERSION_MAGIC) {
+		pr_err("KNI kmod ABI incompatible with librte_kni -- %u\n",
+				RTE_KNI_ABI_VERSION_FROM_MAGIC(abi_version_magic));
+		return -1;
+	}
+	return 0;
+}
+
 static int
 kni_check_param(struct kni_dev *kni, struct rte_kni_device_info *dev)
 {
 	if (!kni || !dev)
 		return -1;
 
+	if (kni_check_abi_version_magic(dev->abi_version_magic) < 0)
+		return -1;
+
 	/* Check if network name has been used */
 	if (!strncmp(kni->name, dev->name, RTE_KNI_NAMESIZE)) {
 		pr_err("KNI name %s duplicated\n", dev->name);
@@ -301,12 +314,19 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
 	struct rte_kni_device_info dev_info;
 	struct net_device *net_dev = NULL;
 	struct kni_dev *kni, *dev, *n;
+	uint16_t abi_version_magic;
 
 	pr_info("Creating kni...\n");
 	/* Check the buffer size, to avoid warning */
 	if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
 		return -EINVAL;
 
+	/* perform abi check ahead of copy, to avoid possible violation */
+	if (copy_from_user(&abi_version_magic, (void *)ioctl_param, sizeof(uint16_t)))
+		return -EFAULT;
+	if (kni_check_abi_version_magic(abi_version_magic) < 0)
+		return -EINVAL;
+
 	/* Copy kni info from user space */
 	if (copy_from_user(&dev_info, (void *)ioctl_param, sizeof(dev_info)))
 		return -EFAULT;
@@ -451,10 +471,17 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
 	int ret = -EINVAL;
 	struct kni_dev *dev, *n;
 	struct rte_kni_device_info dev_info;
+	uint16_t abi_version_magic;
 
 	if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
 		return -EINVAL;
 
+	/* perform abi check ahead of copy, to avoid possible violation */
+	if (copy_from_user(&abi_version_magic, (void *)ioctl_param, sizeof(uint16_t)))
+		return -EFAULT;
+	if (kni_check_abi_version_magic(abi_version_magic) < 0)
+		return -EINVAL;
+
 	if (copy_from_user(&dev_info, (void *)ioctl_param, sizeof(dev_info)))
 		return -EFAULT;
 
@@ -484,6 +511,16 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
 	return ret;
 }
 
+static int
+kni_ioctl_abi_version(struct net *net, uint32_t ioctl_num,
+		unsigned long ioctl_param)
+{
+	uint16_t abi_version = ABI_VERSION_MAJOR;
+	if (copy_to_user((void *)ioctl_param, &abi_version, sizeof(uint16_t)))
+		return -EFAULT;
+	return 0;
+}
+
 static long
 kni_ioctl(struct file *file, unsigned int ioctl_num, unsigned long ioctl_param)
 {
@@ -505,6 +542,9 @@ kni_ioctl(struct file *file, unsigned int ioctl_num, unsigned long ioctl_param)
 	case _IOC_NR(RTE_KNI_IOCTL_RELEASE):
 		ret = kni_ioctl_release(net, ioctl_num, ioctl_param);
 		break;
+	case _IOC_NR(RTE_KNI_IOCTL_ABI_VERSION):
+		ret = kni_ioctl_abi_version(net, ioctl_num, ioctl_param);
+		break;
 	default:
 		pr_debug("IOCTL default\n");
 		break;
diff --git a/kernel/linux/kni/meson.build b/kernel/linux/kni/meson.build
index 4c90069e99..c8cd23acd9 100644
--- a/kernel/linux/kni/meson.build
+++ b/kernel/linux/kni/meson.build
@@ -3,12 +3,12 @@
 
 # For SUSE build check function arguments of ndo_tx_timeout API
 # Ref: https://jira.devtools.intel.com/browse/DPDK-29263
-kmod_cflags = ''
+kmod_cflags = '-DABI_VERSION_MAJOR=@0@'.format(abi_version.split('.')[0])
 file_path = kernel_source_dir + '/include/linux/netdevice.h'
 run_cmd = run_command('grep', 'ndo_tx_timeout', file_path, check: false)
 
 if run_cmd.stdout().contains('txqueue') == true
-   kmod_cflags = '-DHAVE_ARG_TX_QUEUE'
+   kmod_cflags += ' -DHAVE_ARG_TX_QUEUE'
 endif
 
 
diff --git a/lib/kni/meson.build b/lib/kni/meson.build
index 8a71d8ba6f..f22a27b15b 100644
--- a/lib/kni/meson.build
+++ b/lib/kni/meson.build
@@ -14,3 +14,4 @@ endif
 sources = files('rte_kni.c')
 headers = files('rte_kni.h', 'rte_kni_common.h')
 deps += ['ethdev', 'pci']
+cflags += ['-DABI_VERSION_MAJOR=@0@'.format(abi_version.split('.')[0])]
diff --git a/lib/kni/rte_kni.c b/lib/kni/rte_kni.c
index 7971c56bb4..1c8a610bd4 100644
--- a/lib/kni/rte_kni.c
+++ b/lib/kni/rte_kni.c
@@ -22,6 +22,7 @@
 #include <rte_eal_memconfig.h>
 #include <rte_kni_common.h>
 #include "rte_kni_fifo.h"
+#include "rte_kni_abi.h"
 
 #define MAX_MBUF_BURST_NUM            32
 
@@ -113,6 +114,19 @@ rte_kni_init(unsigned int max_kni_ifaces __rte_unused)
 		}
 	}
 
+	uint16_t abi_version;
+	int ret = ioctl(kni_fd, RTE_KNI_IOCTL_ABI_VERSION, &abi_version);
+	if (ret < 0) {
+		RTE_LOG(ERR, KNI, "Cannot verify rte_kni kmod ABI version: ioctl failed\n");
+		return -1;
+	}
+	if (abi_version != ABI_VERSION_MAJOR) {
+		RTE_LOG(ERR, KNI,
+				"rte_kni kmod ABI version mismatch: "
+				"need %" PRIu16 " got %" PRIu16 "\n", ABI_VERSION_MAJOR, abi_version);
+		return -1;
+	}
+
 	return 0;
 }
 
@@ -255,6 +269,7 @@ rte_kni_alloc(struct rte_mempool *pktmbuf_pool,
 		kni->ops.port_id = UINT16_MAX;
 
 	memset(&dev_info, 0, sizeof(dev_info));
+    dev_info.abi_version_magic = RTE_KNI_ABI_VERSION_MAGIC;
 	dev_info.core_id = conf->core_id;
 	dev_info.force_bind = conf->force_bind;
 	dev_info.group_id = conf->group_id;
@@ -409,6 +424,8 @@ rte_kni_release(struct rte_kni *kni)
 	if (!kni)
 		return -1;
 
+    dev_info.abi_version_magic = RTE_KNI_ABI_VERSION_MAGIC;
+
 	kni_list = RTE_TAILQ_CAST(rte_kni_tailq.head, rte_kni_list);
 
 	rte_mcfg_tailq_write_lock();
diff --git a/lib/kni/rte_kni_abi.h b/lib/kni/rte_kni_abi.h
new file mode 100644
index 0000000000..7dde394c72
--- /dev/null
+++ b/lib/kni/rte_kni_abi.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: (BSD-3-Clause OR LGPL-2.1) */
+/*
+ * Copyright(c) 2007-2014 Intel Corporation.
+ */
+
+#ifndef _RTE_KNI_ABI_H_
+#define _RTE_KNI_ABI_H_
+
+#ifndef ABI_VERSION_MAJOR
+#error Need ABI_VERSION_MAJOR being the major part of dpdk/ABI_VERSION
+#endif
+#define RTE_KNI_ABI_VERSION_MAGIC_MASK 0xAAAA
+#define RTE_KNI_ABI_VERSION_MAGIC (((ABI_VERSION_MAJOR) ^ RTE_KNI_ABI_VERSION_MAGIC_MASK))
+#define RTE_KNI_ABI_VERSION_FROM_MAGIC(__magic) (((__magic) ^ RTE_KNI_ABI_VERSION_MAGIC_MASK))
+
+#endif
+
diff --git a/lib/kni/rte_kni_common.h b/lib/kni/rte_kni_common.h
index 8d3ee0fa4f..f9432b742c 100644
--- a/lib/kni/rte_kni_common.h
+++ b/lib/kni/rte_kni_common.h
@@ -102,6 +102,8 @@ struct rte_kni_mbuf {
  */
 
 struct rte_kni_device_info {
+	uint16_t abi_version_magic;
+
 	char name[RTE_KNI_NAMESIZE];  /**< Network device name for KNI */
 
 	phys_addr_t tx_phys;
@@ -139,6 +141,7 @@ struct rte_kni_device_info {
 #define RTE_KNI_IOCTL_TEST    _IOWR(0, 1, int)
 #define RTE_KNI_IOCTL_CREATE  _IOWR(0, 2, struct rte_kni_device_info)
 #define RTE_KNI_IOCTL_RELEASE _IOWR(0, 3, struct rte_kni_device_info)
+#define RTE_KNI_IOCTL_ABI_VERSION _IOWR(0, 4, uint16_t)
 
 #ifdef __cplusplus
 }
-- 
2.35.1

Re: kni: check abi version between kmod and lib

[Ray Kinsella]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Thu, 21 Apr 2022 11:40:00 -0400
> Ray Kinsella <mdr@ashroe.eu> wrote:
>
>> Stephen Hemminger <stephen@networkplumber.org> writes:
>> 
>> > On Thu, 21 Apr 2022 12:38:26 +0800
>> > Stephen Coleman <omegacoleman@gmail.com> wrote:
>> >  
>> >> KNI ioctl functions copy data from userspace lib, and this interface
>> >> of kmod is not compatible indeed. If the user use incompatible rte_kni.ko
>> >> bad things happen: sometimes various fields contain garbage value,
>> >> sometimes it cause a kmod soft lockup.
>> >> 
>> >> Some common distros ship their own rte_kni.ko, so this is likely to
>> >> happen.
>> >> 
>> >> This patch add abi version checking between userland lib and kmod so
>> >> that:
>> >> 
>> >> * if kmod ioctl got a wrong abi magic, it refuse to go on
>> >> * if userland lib, probed a wrong abi version via newly added ioctl, it
>> >>   also refuse to go on
>> >> 
>> >> Bugzilla ID: 998  
>> >
>> >
>> > Kernel API's are supposed to be 99% stable.
>> > If this driver was playing by the upstream kernel rules this would not
>> > have happened.  
>> 
>> Well look, it is out-of-tree and never likely to be in-tree, so those
>> rules don't apply. Making sure the ABI doesn't change during the ABI
>> stablity period, should be good enough?
>> 
>
> I think if KNI changes, it should just add more ioctl numbers and
> be compatible, it is not that hard.

True, fair point, I am unsure what that buys us though. My thinking was
that we should be doing the minimal amount of work on KNI, and directing
people to use upstream alternatives where possible.

For me minimizing means DPDK ABI alignment. However I see your point,
let KNI maintain it own ABI versioning independent of DPDK, with
stricter kernel-like guarantees is probably not much more work.

-- 
Regards, Ray K

Re: kni: check abi version between kmod and lib

[Stephen Coleman]

thanks for your replies

I'm aware that kernel guidelines propose ascending ioctl numbers to
max out compatibility, but this will not work with dpdk, especially
our case here.

If you look into kni_net.c you'll see the module is actually
internally depending on the memory layout of mbuf and a few other
structs, you will need to change ioctl numbers if those change, and
that's very implicit and requires extra effort. Plus the compatibility
is almost impossible to maintain across dpdk releases, as the module
won't know which version of mbuf layout it is working with.

In short, rte_kni.ko is part of dpdk rather than part of kernel, and
different parts of different dpdk releases do not work together -- so
we reject them early in the first before it make a disaster.

p.s. working on v3 to fix code format issues
p.p.s. forgot to 'reply all' last time, sorry for the duplication


>
>
> Stephen Hemminger <stephen@networkplumber.org> writes:
>
> > On Thu, 21 Apr 2022 11:40:00 -0400
> > Ray Kinsella <mdr@ashroe.eu> wrote:
> >
> >> Stephen Hemminger <stephen@networkplumber.org> writes:
> >>
> >> > On Thu, 21 Apr 2022 12:38:26 +0800
> >> > Stephen Coleman <omegacoleman@gmail.com> wrote:
> >> >
> >> >> KNI ioctl functions copy data from userspace lib, and this interface
> >> >> of kmod is not compatible indeed. If the user use incompatible rte_kni.ko
> >> >> bad things happen: sometimes various fields contain garbage value,
> >> >> sometimes it cause a kmod soft lockup.
> >> >>
> >> >> Some common distros ship their own rte_kni.ko, so this is likely to
> >> >> happen.
> >> >>
> >> >> This patch add abi version checking between userland lib and kmod so
> >> >> that:
> >> >>
> >> >> * if kmod ioctl got a wrong abi magic, it refuse to go on
> >> >> * if userland lib, probed a wrong abi version via newly added ioctl, it
> >> >>   also refuse to go on
> >> >>
> >> >> Bugzilla ID: 998
> >> >
> >> >
> >> > Kernel API's are supposed to be 99% stable.
> >> > If this driver was playing by the upstream kernel rules this would not
> >> > have happened.
> >>
> >> Well look, it is out-of-tree and never likely to be in-tree, so those
> >> rules don't apply. Making sure the ABI doesn't change during the ABI
> >> stablity period, should be good enough?
> >>
> >
> > I think if KNI changes, it should just add more ioctl numbers and
> > be compatible, it is not that hard.
>
> True, fair point, I am unsure what that buys us though. My thinking was
> that we should be doing the minimal amount of work on KNI, and directing
> people to use upstream alternatives where possible.
>
> For me minimizing means DPDK ABI alignment. However I see your point,
> let KNI maintain it own ABI versioning independent of DPDK, with
> stricter kernel-like guarantees is probably not much more work.
>
> --
> Regards, Ray K

[PATCH v3] kni: check abi version between kmod and lib

[youcai]

KNI ioctl functions copy data from userspace lib, and this interface
of kmod is not compatible indeed. If the user use incompatible rte_kni.ko
bad things happen: sometimes various fields contain garbage value,
sometimes it cause a kmod soft lockup.

Some common distros ship their own rte_kni.ko, so this is likely to
happen.

This patch add abi version checking between userland lib and kmod so
that:

* if kmod ioctl got a wrong abi magic, it refuse to go on
* if userland lib, probed a wrong abi version via newly added ioctl, it
  also refuse to go on

Bugzilla ID: 998

Signed-off-by: youcai <omegacoleman@gmail.com>

---
V3: fix code format issues

V2: use ABI_VERSION instead of a new magic
V2: fix some indent
---
 kernel/linux/kni/kni_misc.c  | 42 ++++++++++++++++++++++++++++++++++++
 kernel/linux/kni/meson.build |  4 ++--
 lib/kni/meson.build          |  1 +
 lib/kni/rte_kni.c            | 18 ++++++++++++++++
 lib/kni/rte_kni_abi.h        | 17 +++++++++++++++
 lib/kni/rte_kni_common.h     |  3 +++
 6 files changed, 83 insertions(+), 2 deletions(-)
 create mode 100644 lib/kni/rte_kni_abi.h

diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
index 780187d8bf..d92500414d 100644
--- a/kernel/linux/kni/kni_misc.c
+++ b/kernel/linux/kni/kni_misc.c
@@ -17,6 +17,7 @@
 #include <net/netns/generic.h>
 
 #include <rte_kni_common.h>
+#include <rte_kni_abi.h>
 
 #include "compat.h"
 #include "kni_dev.h"
@@ -236,12 +237,26 @@ kni_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int
+kni_check_abi_version_magic(uint16_t abi_version_magic)
+{
+	if (abi_version_magic != RTE_KNI_ABI_VERSION_MAGIC) {
+		pr_err("KNI kmod ABI incompatible with librte_kni -- %u\n",
+				RTE_KNI_ABI_VERSION_FROM_MAGIC(abi_version_magic));
+		return -1;
+	}
+	return 0;
+}
+
 static int
 kni_check_param(struct kni_dev *kni, struct rte_kni_device_info *dev)
 {
 	if (!kni || !dev)
 		return -1;
 
+	if (kni_check_abi_version_magic(dev->abi_version_magic) < 0)
+		return -1;
+
 	/* Check if network name has been used */
 	if (!strncmp(kni->name, dev->name, RTE_KNI_NAMESIZE)) {
 		pr_err("KNI name %s duplicated\n", dev->name);
@@ -301,12 +316,19 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
 	struct rte_kni_device_info dev_info;
 	struct net_device *net_dev = NULL;
 	struct kni_dev *kni, *dev, *n;
+	uint16_t abi_version_magic;
 
 	pr_info("Creating kni...\n");
 	/* Check the buffer size, to avoid warning */
 	if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
 		return -EINVAL;
 
+	/* perform abi check ahead of copy, to avoid possible violation */
+	if (copy_from_user(&abi_version_magic, (void *)ioctl_param, sizeof(uint16_t)))
+		return -EFAULT;
+	if (kni_check_abi_version_magic(abi_version_magic) < 0)
+		return -EINVAL;
+
 	/* Copy kni info from user space */
 	if (copy_from_user(&dev_info, (void *)ioctl_param, sizeof(dev_info)))
 		return -EFAULT;
@@ -451,10 +473,17 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
 	int ret = -EINVAL;
 	struct kni_dev *dev, *n;
 	struct rte_kni_device_info dev_info;
+	uint16_t abi_version_magic;
 
 	if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
 		return -EINVAL;
 
+	/* perform abi check ahead of copy, to avoid possible violation */
+	if (copy_from_user(&abi_version_magic, (void *)ioctl_param, sizeof(uint16_t)))
+		return -EFAULT;
+	if (kni_check_abi_version_magic(abi_version_magic) < 0)
+		return -EINVAL;
+
 	if (copy_from_user(&dev_info, (void *)ioctl_param, sizeof(dev_info)))
 		return -EFAULT;
 
@@ -484,6 +513,16 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
 	return ret;
 }
 
+static int
+kni_ioctl_abi_version(struct net *net, uint32_t ioctl_num,
+		unsigned long ioctl_param)
+{
+	uint16_t abi_version = ABI_VERSION_MAJOR;
+	if (copy_to_user((void *)ioctl_param, &abi_version, sizeof(uint16_t)))
+		return -EFAULT;
+	return 0;
+}
+
 static long
 kni_ioctl(struct file *file, unsigned int ioctl_num, unsigned long ioctl_param)
 {
@@ -505,6 +544,9 @@ kni_ioctl(struct file *file, unsigned int ioctl_num, unsigned long ioctl_param)
 	case _IOC_NR(RTE_KNI_IOCTL_RELEASE):
 		ret = kni_ioctl_release(net, ioctl_num, ioctl_param);
 		break;
+	case _IOC_NR(RTE_KNI_IOCTL_ABI_VERSION):
+		ret = kni_ioctl_abi_version(net, ioctl_num, ioctl_param);
+		break;
 	default:
 		pr_debug("IOCTL default\n");
 		break;
diff --git a/kernel/linux/kni/meson.build b/kernel/linux/kni/meson.build
index 4c90069e99..c8cd23acd9 100644
--- a/kernel/linux/kni/meson.build
+++ b/kernel/linux/kni/meson.build
@@ -3,12 +3,12 @@
 
 # For SUSE build check function arguments of ndo_tx_timeout API
 # Ref: https://jira.devtools.intel.com/browse/DPDK-29263
-kmod_cflags = ''
+kmod_cflags = '-DABI_VERSION_MAJOR=@0@'.format(abi_version.split('.')[0])
 file_path = kernel_source_dir + '/include/linux/netdevice.h'
 run_cmd = run_command('grep', 'ndo_tx_timeout', file_path, check: false)
 
 if run_cmd.stdout().contains('txqueue') == true
-   kmod_cflags = '-DHAVE_ARG_TX_QUEUE'
+   kmod_cflags += ' -DHAVE_ARG_TX_QUEUE'
 endif
 
 
diff --git a/lib/kni/meson.build b/lib/kni/meson.build
index 8a71d8ba6f..f22a27b15b 100644
--- a/lib/kni/meson.build
+++ b/lib/kni/meson.build
@@ -14,3 +14,4 @@ endif
 sources = files('rte_kni.c')
 headers = files('rte_kni.h', 'rte_kni_common.h')
 deps += ['ethdev', 'pci']
+cflags += ['-DABI_VERSION_MAJOR=@0@'.format(abi_version.split('.')[0])]
diff --git a/lib/kni/rte_kni.c b/lib/kni/rte_kni.c
index 7971c56bb4..9bdeeb3806 100644
--- a/lib/kni/rte_kni.c
+++ b/lib/kni/rte_kni.c
@@ -22,6 +22,7 @@
 #include <rte_eal_memconfig.h>
 #include <rte_kni_common.h>
 #include "rte_kni_fifo.h"
+#include "rte_kni_abi.h"
 
 #define MAX_MBUF_BURST_NUM            32
 
@@ -113,6 +114,20 @@ rte_kni_init(unsigned int max_kni_ifaces __rte_unused)
 		}
 	}
 
+	uint16_t abi_version;
+	int ret = ioctl(kni_fd, RTE_KNI_IOCTL_ABI_VERSION, &abi_version);
+	if (ret < 0) {
+		RTE_LOG(ERR, KNI, "Cannot verify rte_kni kmod ABI version: ioctl failed\n");
+		return -1;
+	}
+	if (abi_version != ABI_VERSION_MAJOR) {
+		RTE_LOG(ERR, KNI,
+				"rte_kni kmod ABI version mismatch: "
+				"need %" PRIu16 " got %" PRIu16 "\n",
+				ABI_VERSION_MAJOR, abi_version);
+		return -1;
+	}
+
 	return 0;
 }
 
@@ -255,6 +270,7 @@ rte_kni_alloc(struct rte_mempool *pktmbuf_pool,
 		kni->ops.port_id = UINT16_MAX;
 
 	memset(&dev_info, 0, sizeof(dev_info));
+	dev_info.abi_version_magic = RTE_KNI_ABI_VERSION_MAGIC;
 	dev_info.core_id = conf->core_id;
 	dev_info.force_bind = conf->force_bind;
 	dev_info.group_id = conf->group_id;
@@ -409,6 +425,8 @@ rte_kni_release(struct rte_kni *kni)
 	if (!kni)
 		return -1;
 
+	dev_info.abi_version_magic = RTE_KNI_ABI_VERSION_MAGIC;
+
 	kni_list = RTE_TAILQ_CAST(rte_kni_tailq.head, rte_kni_list);
 
 	rte_mcfg_tailq_write_lock();
diff --git a/lib/kni/rte_kni_abi.h b/lib/kni/rte_kni_abi.h
new file mode 100644
index 0000000000..7dde394c72
--- /dev/null
+++ b/lib/kni/rte_kni_abi.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: (BSD-3-Clause OR LGPL-2.1) */
+/*
+ * Copyright(c) 2007-2014 Intel Corporation.
+ */
+
+#ifndef _RTE_KNI_ABI_H_
+#define _RTE_KNI_ABI_H_
+
+#ifndef ABI_VERSION_MAJOR
+#error Need ABI_VERSION_MAJOR being the major part of dpdk/ABI_VERSION
+#endif
+#define RTE_KNI_ABI_VERSION_MAGIC_MASK 0xAAAA
+#define RTE_KNI_ABI_VERSION_MAGIC (((ABI_VERSION_MAJOR) ^ RTE_KNI_ABI_VERSION_MAGIC_MASK))
+#define RTE_KNI_ABI_VERSION_FROM_MAGIC(__magic) (((__magic) ^ RTE_KNI_ABI_VERSION_MAGIC_MASK))
+
+#endif
+
diff --git a/lib/kni/rte_kni_common.h b/lib/kni/rte_kni_common.h
index 8d3ee0fa4f..f9432b742c 100644
--- a/lib/kni/rte_kni_common.h
+++ b/lib/kni/rte_kni_common.h
@@ -102,6 +102,8 @@ struct rte_kni_mbuf {
  */
 
 struct rte_kni_device_info {
+	uint16_t abi_version_magic;
+
 	char name[RTE_KNI_NAMESIZE];  /**< Network device name for KNI */
 
 	phys_addr_t tx_phys;
@@ -139,6 +141,7 @@ struct rte_kni_device_info {
 #define RTE_KNI_IOCTL_TEST    _IOWR(0, 1, int)
 #define RTE_KNI_IOCTL_CREATE  _IOWR(0, 2, struct rte_kni_device_info)
 #define RTE_KNI_IOCTL_RELEASE _IOWR(0, 3, struct rte_kni_device_info)
+#define RTE_KNI_IOCTL_ABI_VERSION _IOWR(0, 4, uint16_t)
 
 #ifdef __cplusplus
 }
-- 
2.35.1

Re: kni: check abi version between kmod and lib

[Stephen Coleman]

[-- Attachment #1: Type: text/plain, Size: 9534 bytes --]

execuse me, one of the Windows check is failing but I didn't find where's
the build log, nor to determine whether it is related.

> KNI ioctl functions copy data from userspace lib, and this interface
> of kmod is not compatible indeed. If the user use incompatible rte_kni.ko
> bad things happen: sometimes various fields contain garbage value,
> sometimes it cause a kmod soft lockup.
>
> Some common distros ship their own rte_kni.ko, so this is likely to
> happen.
>
> This patch add abi version checking between userland lib and kmod so
> that:
>
> * if kmod ioctl got a wrong abi magic, it refuse to go on
> * if userland lib, probed a wrong abi version via newly added ioctl, it
>   also refuse to go on
>
> Bugzilla ID: 998
>
> Signed-off-by: youcai <omegacoleman@gmail.com>
>
> ---
> V3: fix code format issues
>
> V2: use ABI_VERSION instead of a new magic
> V2: fix some indent
> ---
>  kernel/linux/kni/kni_misc.c  | 42 ++++++++++++++++++++++++++++++++++++
>  kernel/linux/kni/meson.build |  4 ++--
>  lib/kni/meson.build          |  1 +
>  lib/kni/rte_kni.c            | 18 ++++++++++++++++
>  lib/kni/rte_kni_abi.h        | 17 +++++++++++++++
>  lib/kni/rte_kni_common.h     |  3 +++
>  6 files changed, 83 insertions(+), 2 deletions(-)
>  create mode 100644 lib/kni/rte_kni_abi.h
>
> diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
> index 780187d8bf..d92500414d 100644
> --- a/kernel/linux/kni/kni_misc.c
> +++ b/kernel/linux/kni/kni_misc.c
> @@ -17,6 +17,7 @@
>  #include <net/netns/generic.h>
>
>  #include <rte_kni_common.h>
> +#include <rte_kni_abi.h>
>
>  #include "compat.h"
>  #include "kni_dev.h"
> @@ -236,12 +237,26 @@ kni_release(struct inode *inode, struct file *file)
>         return 0;
>  }
>
> +static int
> +kni_check_abi_version_magic(uint16_t abi_version_magic)
> +{
> +       if (abi_version_magic != RTE_KNI_ABI_VERSION_MAGIC) {
> +               pr_err("KNI kmod ABI incompatible with librte_kni --
%u\n",
> +
 RTE_KNI_ABI_VERSION_FROM_MAGIC(abi_version_magic));
> +               return -1;
> +       }
> +       return 0;
> +}
> +
>  static int
>  kni_check_param(struct kni_dev *kni, struct rte_kni_device_info *dev)
>  {
>         if (!kni || !dev)
>                 return -1;
>
> +       if (kni_check_abi_version_magic(dev->abi_version_magic) < 0)
> +               return -1;
> +
>         /* Check if network name has been used */
>         if (!strncmp(kni->name, dev->name, RTE_KNI_NAMESIZE)) {
>                 pr_err("KNI name %s duplicated\n", dev->name);
> @@ -301,12 +316,19 @@ kni_ioctl_create(struct net *net, uint32_t
ioctl_num,
>         struct rte_kni_device_info dev_info;
>         struct net_device *net_dev = NULL;
>         struct kni_dev *kni, *dev, *n;
> +       uint16_t abi_version_magic;
>
>         pr_info("Creating kni...\n");
>         /* Check the buffer size, to avoid warning */
>         if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
>                 return -EINVAL;
>
> +       /* perform abi check ahead of copy, to avoid possible violation */
> +       if (copy_from_user(&abi_version_magic, (void *)ioctl_param,
sizeof(uint16_t)))
> +               return -EFAULT;
> +       if (kni_check_abi_version_magic(abi_version_magic) < 0)
> +               return -EINVAL;
> +
>         /* Copy kni info from user space */
>         if (copy_from_user(&dev_info, (void *)ioctl_param,
sizeof(dev_info)))
>                 return -EFAULT;
> @@ -451,10 +473,17 @@ kni_ioctl_release(struct net *net, uint32_t
ioctl_num,
>         int ret = -EINVAL;
>         struct kni_dev *dev, *n;
>         struct rte_kni_device_info dev_info;
> +       uint16_t abi_version_magic;
>
>         if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
>                 return -EINVAL;
>
> +       /* perform abi check ahead of copy, to avoid possible violation */
> +       if (copy_from_user(&abi_version_magic, (void *)ioctl_param,
sizeof(uint16_t)))
> +               return -EFAULT;
> +       if (kni_check_abi_version_magic(abi_version_magic) < 0)
> +               return -EINVAL;
> +
>         if (copy_from_user(&dev_info, (void *)ioctl_param,
sizeof(dev_info)))
>                 return -EFAULT;
>
> @@ -484,6 +513,16 @@ kni_ioctl_release(struct net *net, uint32_t
ioctl_num,
>         return ret;
>  }
>
> +static int
> +kni_ioctl_abi_version(struct net *net, uint32_t ioctl_num,
> +               unsigned long ioctl_param)
> +{
> +       uint16_t abi_version = ABI_VERSION_MAJOR;
> +       if (copy_to_user((void *)ioctl_param, &abi_version,
sizeof(uint16_t)))
> +               return -EFAULT;
> +       return 0;
> +}
> +
>  static long
>  kni_ioctl(struct file *file, unsigned int ioctl_num, unsigned long
ioctl_param)
>  {
> @@ -505,6 +544,9 @@ kni_ioctl(struct file *file, unsigned int ioctl_num,
unsigned long ioctl_param)
>         case _IOC_NR(RTE_KNI_IOCTL_RELEASE):
>                 ret = kni_ioctl_release(net, ioctl_num, ioctl_param);
>                 break;
> +       case _IOC_NR(RTE_KNI_IOCTL_ABI_VERSION):
> +               ret = kni_ioctl_abi_version(net, ioctl_num, ioctl_param);
> +               break;
>         default:
>                 pr_debug("IOCTL default\n");
>                 break;
> diff --git a/kernel/linux/kni/meson.build b/kernel/linux/kni/meson.build
> index 4c90069e99..c8cd23acd9 100644
> --- a/kernel/linux/kni/meson.build
> +++ b/kernel/linux/kni/meson.build
> @@ -3,12 +3,12 @@
>
>  # For SUSE build check function arguments of ndo_tx_timeout API
>  # Ref: https://jira.devtools.intel.com/browse/DPDK-29263
> -kmod_cflags = ''
> +kmod_cflags = '-DABI_VERSION_MAJOR=@0@'.format(abi_version.split('.')[0])
>  file_path = kernel_source_dir + '/include/linux/netdevice.h'
>  run_cmd = run_command('grep', 'ndo_tx_timeout', file_path, check: false)
>
>  if run_cmd.stdout().contains('txqueue') == true
> -   kmod_cflags = '-DHAVE_ARG_TX_QUEUE'
> +   kmod_cflags += ' -DHAVE_ARG_TX_QUEUE'
>  endif
>
>
> diff --git a/lib/kni/meson.build b/lib/kni/meson.build
> index 8a71d8ba6f..f22a27b15b 100644
> --- a/lib/kni/meson.build
> +++ b/lib/kni/meson.build
> @@ -14,3 +14,4 @@ endif
>  sources = files('rte_kni.c')
>  headers = files('rte_kni.h', 'rte_kni_common.h')
>  deps += ['ethdev', 'pci']
> +cflags += ['-DABI_VERSION_MAJOR=@0@'.format(abi_version.split('.')[0])]
> diff --git a/lib/kni/rte_kni.c b/lib/kni/rte_kni.c
> index 7971c56bb4..9bdeeb3806 100644
> --- a/lib/kni/rte_kni.c
> +++ b/lib/kni/rte_kni.c
> @@ -22,6 +22,7 @@
>  #include <rte_eal_memconfig.h>
>  #include <rte_kni_common.h>
>  #include "rte_kni_fifo.h"
> +#include "rte_kni_abi.h"
>
>  #define MAX_MBUF_BURST_NUM            32
>
> @@ -113,6 +114,20 @@ rte_kni_init(unsigned int max_kni_ifaces
__rte_unused)
>                 }
>         }
>
> +       uint16_t abi_version;
> +       int ret = ioctl(kni_fd, RTE_KNI_IOCTL_ABI_VERSION, &abi_version);
> +       if (ret < 0) {
> +               RTE_LOG(ERR, KNI, "Cannot verify rte_kni kmod ABI
version: ioctl failed\n");
> +               return -1;
> +       }
> +       if (abi_version != ABI_VERSION_MAJOR) {
> +               RTE_LOG(ERR, KNI,
> +                               "rte_kni kmod ABI version mismatch: "
> +                               "need %" PRIu16 " got %" PRIu16 "\n",
> +                               ABI_VERSION_MAJOR, abi_version);
> +               return -1;
> +       }
> +
>         return 0;
>  }
>
> @@ -255,6 +270,7 @@ rte_kni_alloc(struct rte_mempool *pktmbuf_pool,
>                 kni->ops.port_id = UINT16_MAX;
>
>         memset(&dev_info, 0, sizeof(dev_info));
> +       dev_info.abi_version_magic = RTE_KNI_ABI_VERSION_MAGIC;
>         dev_info.core_id = conf->core_id;
>         dev_info.force_bind = conf->force_bind;
>         dev_info.group_id = conf->group_id;
> @@ -409,6 +425,8 @@ rte_kni_release(struct rte_kni *kni)
>         if (!kni)
>                 return -1;
>
> +       dev_info.abi_version_magic = RTE_KNI_ABI_VERSION_MAGIC;
> +
>         kni_list = RTE_TAILQ_CAST(rte_kni_tailq.head, rte_kni_list);
>
>         rte_mcfg_tailq_write_lock();
> diff --git a/lib/kni/rte_kni_abi.h b/lib/kni/rte_kni_abi.h
> new file mode 100644
> index 0000000000..7dde394c72
> --- /dev/null
> +++ b/lib/kni/rte_kni_abi.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: (BSD-3-Clause OR LGPL-2.1) */
> +/*
> + * Copyright(c) 2007-2014 Intel Corporation.
> + */
> +
> +#ifndef _RTE_KNI_ABI_H_
> +#define _RTE_KNI_ABI_H_
> +
> +#ifndef ABI_VERSION_MAJOR
> +#error Need ABI_VERSION_MAJOR being the major part of dpdk/ABI_VERSION
> +#endif
> +#define RTE_KNI_ABI_VERSION_MAGIC_MASK 0xAAAA
> +#define RTE_KNI_ABI_VERSION_MAGIC (((ABI_VERSION_MAJOR) ^
RTE_KNI_ABI_VERSION_MAGIC_MASK))
> +#define RTE_KNI_ABI_VERSION_FROM_MAGIC(__magic) (((__magic) ^
RTE_KNI_ABI_VERSION_MAGIC_MASK))
> +
> +#endif
> +
> diff --git a/lib/kni/rte_kni_common.h b/lib/kni/rte_kni_common.h
> index 8d3ee0fa4f..f9432b742c 100644
> --- a/lib/kni/rte_kni_common.h
> +++ b/lib/kni/rte_kni_common.h
> @@ -102,6 +102,8 @@ struct rte_kni_mbuf {
>   */
>
>  struct rte_kni_device_info {
> +       uint16_t abi_version_magic;
> +
>         char name[RTE_KNI_NAMESIZE];  /**< Network device name for KNI */
>
>         phys_addr_t tx_phys;
> @@ -139,6 +141,7 @@ struct rte_kni_device_info {
>  #define RTE_KNI_IOCTL_TEST    _IOWR(0, 1, int)
>  #define RTE_KNI_IOCTL_CREATE  _IOWR(0, 2, struct rte_kni_device_info)
>  #define RTE_KNI_IOCTL_RELEASE _IOWR(0, 3, struct rte_kni_device_info)
> +#define RTE_KNI_IOCTL_ABI_VERSION _IOWR(0, 4, uint16_t)
>
>  #ifdef __cplusplus
>  }
> --
> 2.35.1
>
>

[-- Attachment #2: Type: text/html, Size: 12224 bytes --]

Re: kni: check abi version between kmod and lib

[Stephen Hemminger]

On Thu, 21 Apr 2022 12:38:26 +0800
Stephen Coleman <omegacoleman@gmail.com> wrote:

> KNI ioctl functions copy data from userspace lib, and this interface
> of kmod is not compatible indeed. If the user use incompatible rte_kni.ko
> bad things happen: sometimes various fields contain garbage value,
> sometimes it cause a kmod soft lockup.
> 
> Some common distros ship their own rte_kni.ko, so this is likely to
> happen.
> 
> This patch add abi version checking between userland lib and kmod so
> that:
> 
> * if kmod ioctl got a wrong abi magic, it refuse to go on
> * if userland lib, probed a wrong abi version via newly added ioctl, it
>   also refuse to go on
> 
> Bugzilla ID: 998
> 
> Signed-off-by: youcai <omegacoleman@gmail.com>

KNI is deprecated and scheduled for removal.
Even though this fixes a bug, because it changes API/ABI it can't go in.
Dropping the patch.