Re: kni: check abi version between kmod and lib

[Ray Kinsella <mdr@ashroe.eu>]

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