[PATCH] doc: fix typos 'depreciated' instead of 'deprecated'
>From 1871ee6f7b98ef89b7c4893d90b5ea264660c201 Mon Sep 17 00:00:00 2001
From: youcai
Date: Tue, 19 Apr 2022 14:38:36 +0800
Subject: [PATCH] doc: fix typos 'depreciated' instead of 'deprecated'
Cc: Ray Kinsella
Same issue was fixed in ABI policy in ec5c0f8, but more of this
misuse persist in comments and docs. 'depreciated' means diminish
in value over a period of time. It should not appear here.
Signed-off-by: youcai
---
doc/guides/contributing/abi_policy.rst | 2 +-
doc/guides/contributing/abi_versioning.rst | 2 +-
lib/kni/rte_kni.h | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/doc/guides/contributing/abi_policy.rst
b/doc/guides/contributing/abi_policy.rst
index 64919b6a2b..5fd4052585 100644
--- a/doc/guides/contributing/abi_policy.rst
+++ b/doc/guides/contributing/abi_policy.rst
@@ -167,7 +167,7 @@ The requirements for changing the ABI are:
API becomes non-experimental, then the old one is marked with
``__rte_deprecated``.
-- The depreciated API should follow the notification process to be removed,
+- The deprecated API should follow the notification process to be removed,
see :ref:`deprecation_notices`.
- At the declaration of the next major ABI version, those ABI changes then
diff --git a/doc/guides/contributing/abi_versioning.rst
b/doc/guides/contributing/abi_versioning.rst
index dd96527ee5..7afd1c1886 100644
--- a/doc/guides/contributing/abi_versioning.rst
+++ b/doc/guides/contributing/abi_versioning.rst
@@ -94,7 +94,7 @@ that library.
...
However when a new ABI version is declared, for example DPDK ``22``, old
-depreciated functions may be safely removed at this point and the entire old
+deprecated functions may be safely removed at this point and the entire old
major ABI version removed, see the section :ref:`deprecating_entire_abi` on
how this may be done.
diff --git a/lib/kni/rte_kni.h b/lib/kni/rte_kni.h
index b85d3dd32b..f6ef572a82 100644
--- a/lib/kni/rte_kni.h
+++ b/lib/kni/rte_kni.h
@@ -64,8 +64,8 @@ struct rte_kni_conf {
uint32_t core_id; /* Core ID to bind kernel thread on */
uint16_t group_id; /* Group ID */
unsigned mbuf_size; /* mbuf size */
-struct rte_pci_addr addr; /* depreciated */
-struct rte_pci_id id; /* depreciated */
+struct rte_pci_addr addr; /* deprecated */
+struct rte_pci_id id; /* deprecated */
__extension__
uint8_t force_bind : 1; /* Flag to bind kernel thread */
--
2.17.1
kni: check abi version between kmod and lib
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
---
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_
Re: kni: check abi version between kmod and lib
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 writes: > > > On Thu, 21 Apr 2022 11:40:00 -0400 > > Ray Kinsella wrote: > > > >> Stephen Hemminger writes: > >> > >> > On Thu, 21 Apr 2022 12:38:26 +0800 > >> > Stephen Coleman 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
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
>
> ---
> 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
>
> #include
> +#include
>
> #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
