From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8640BA0093; Thu, 21 Apr 2022 16:16:39 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5CBED40042; Thu, 21 Apr 2022 16:16:39 +0200 (CEST) Received: from mail-108-mta134.mxroute.com (mail-108-mta134.mxroute.com [136.175.108.134]) by mails.dpdk.org (Postfix) with ESMTP id 5FA4440040 for ; Thu, 21 Apr 2022 16:16:38 +0200 (CEST) Received: from filter006.mxroute.com ([140.82.40.27] 140.82.40.27.vultrusercontent.com) (Authenticated sender: mN4UYu2MZsgR) by mail-108-mta134.mxroute.com (ZoneMTA) with ESMTPSA id 1804c7b5b99000fe85.001 for (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES128-GCM-SHA256); Thu, 21 Apr 2022 14:16:34 +0000 X-Zone-Loop: d3341dbcf1f851e6071064662794f34381a04c90c491 X-Originating-IP: [140.82.40.27] DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=ashroe.eu; s=x; h=Content-Type:MIME-Version:Message-ID:Date:In-reply-to:Subject:Cc:To: From:References:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=NLeFJoTJhOo5mGV5eyNhxwkLLVxy6mQyf3otIg71Cnc=; b=UDRVo7C0LOBxNtTpBn4GTAN//0 NylJlf+OujwpE2UmGGNqmp99ApTcmH+pAaIHjizsFwybELc18KteexzZj2CE2XVfTmdi0iVbJDXzS 2eVGijWXVdMfQcbs3ZVUQtRJKVDWYKiZo62hom/OGKtcTGoszPmp0OzSwvn7G/vwLNrz5agKiGayy 6bCo2XdOwdHyS08FFkLL4Cv8d/A1LKrnttQbVO2WEYWiXjBk9e6HBLtKFsevQCB64fGx2fx6TMZbT RUtASsiQawLYR+XTfLoUsQnQLYWHsADuF+qMFrjxKsBGJ1SlJpeqD6jSYwa1MHlFnqRc06CSsfMqW PTKc6POw==; References: User-agent: mu4e 1.4.15; emacs 27.1 From: Ray Kinsella To: Stephen Coleman Cc: dev@dpdk.org, Ferruh Yigit Subject: Re: kni: check abi version between kmod and lib In-reply-to: Date: Thu, 21 Apr 2022 10:16:31 -0400 Message-ID: <87fsm6o728.fsf@mdr78.vserver.site> MIME-Version: 1.0 Content-Type: text/plain X-AuthUser: mdr@ashroe.eu X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Stephen Coleman 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 > --- > 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