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 <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(+)

  * 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
 }
  

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
>  }
  

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.
  

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?
  

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.
  

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.
  

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
  

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.