Re: [dpdk-dev] [PATCH] kni: Fix request overwritten

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/17/2021 1:31 PM, Elad Nachman wrote:
> Fix lack of multiple KNI requests handling support by introducing a 
> rotating ring mechanism for the sync buffer.
> 

Thanks Elad for the patch.

I have missed that 'rte_kni_handle_request()' can be called by multiple cores,
at least kni sample app does that.

In that case having multiple requests may end up multiple control command run at
the same time from different cores and this may lead undefined behavior.

Forcing to have single request at a time also simplifies to seriliaze the
control commands to the device.

What about your suggestion to return -EAGAIN, if the request is not processed by
the userspace. And this can be detected by a new field in 'rte_kni_request'
which kernel sets when the request issued and userspace unset when the
processing done. 'kni_net_process_request()' can require that field to be unset
before putting a new request. What do you think?

> Prevent kni_net_change_rx_flags() from calling kni_net_process_request()
> with a malformed request.
> 

This is completely something else so can be dropped from this patch, but OK to
add 'if (req.req_id)' check although current request is harmless since 'req' is
already memset.

> Bugzilla ID: 809
> 
> Signed-off-by: Elad Nachman <eladv6@gmail.com>
> ---
>  kernel/linux/kni/kni_dev.h  |  2 ++
>  kernel/linux/kni/kni_misc.c |  2 ++
>  kernel/linux/kni/kni_net.c  | 25 +++++++++++++++++--------
>  lib/kni/rte_kni.c           | 14 +++++++-------
>  lib/kni/rte_kni_common.h    |  1 +
>  5 files changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/kernel/linux/kni/kni_dev.h b/kernel/linux/kni/kni_dev.h
> index c15da311ba..b9e8b3d10d 100644
> --- a/kernel/linux/kni/kni_dev.h
> +++ b/kernel/linux/kni/kni_dev.h
> @@ -74,6 +74,8 @@ struct kni_dev {
>  
>  	void *sync_kva;
>  	void *sync_va;
> +	unsigned int sync_ring_size;
> +	atomic_t sync_ring_idx;
>  
>  	void *mbuf_kva;
>  	void *mbuf_va;
> diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
> index 2b464c4381..f3cee97818 100644
> --- a/kernel/linux/kni/kni_misc.c
> +++ b/kernel/linux/kni/kni_misc.c
> @@ -345,6 +345,8 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
>  
>  	kni->net_dev = net_dev;
>  	kni->core_id = dev_info.core_id;
> +	kni->sync_ring_size = dev_info.sync_ring_size;
> +	kni->sync_ring_idx.counter = 0;
>  	strncpy(kni->name, dev_info.name, RTE_KNI_NAMESIZE);
>  
>  	/* Translate user space info into kernel space info */
> diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
> index 611719b5ee..dc066cdd98 100644
> --- a/kernel/linux/kni/kni_net.c
> +++ b/kernel/linux/kni/kni_net.c
> @@ -110,6 +110,8 @@ kni_net_process_request(struct net_device *dev, struct rte_kni_request *req)
>  	void *resp_va;
>  	uint32_t num;
>  	int ret_val;
> +	unsigned int idx;
> +	char *k_reqptr, *v_reqptr;
>  
>  	ASSERT_RTNL();
>  
> @@ -122,10 +124,17 @@ kni_net_process_request(struct net_device *dev, struct rte_kni_request *req)
>  	}
>  
>  	mutex_lock(&kni->sync_lock);
> -
> +	idx = atomic_read(&kni->sync_ring_idx);
> +	atomic_cmpxchg(&kni->sync_ring_idx, idx,
> +		(idx + 1) % kni->sync_ring_size);
>  	/* Construct data */
> -	memcpy(kni->sync_kva, req, sizeof(struct rte_kni_request));
> -	num = kni_fifo_put(kni->req_q, &kni->sync_va, 1);
> +	k_reqptr = (char *)((uintptr_t)kni->sync_kva +
> +			sizeof(struct rte_kni_request) * idx);
> +	v_reqptr = (char *)((uintptr_t)kni->sync_va +
> +			sizeof(struct rte_kni_request) * idx);
> +
> +	memcpy(k_reqptr, req, sizeof(struct rte_kni_request));
> +	num = kni_fifo_put(kni->req_q, (void **)&v_reqptr, 1);
>  	if (num < 1) {
>  		pr_err("Cannot send to req_q\n");
>  		ret = -EBUSY;
> @@ -147,14 +156,14 @@ kni_net_process_request(struct net_device *dev, struct rte_kni_request *req)
>  		goto fail;
>  	}
>  	num = kni_fifo_get(kni->resp_q, (void **)&resp_va, 1);
> -	if (num != 1 || resp_va != kni->sync_va) {
> +	if (num != 1) {
>  		/* This should never happen */
>  		pr_err("No data in resp_q\n");
>  		ret = -ENODATA;
>  		goto fail;
>  	}
>  
> -	memcpy(req, kni->sync_kva, sizeof(struct rte_kni_request));
> +	memcpy(req, k_reqptr, sizeof(struct rte_kni_request));
>  async:
>  	ret = 0;
>  
> @@ -681,13 +690,12 @@ kni_net_change_mtu(struct net_device *dev, int new_mtu)
>  static void
>  kni_net_change_rx_flags(struct net_device *netdev, int flags)
>  {
> -	struct rte_kni_request req;
> +	struct rte_kni_request req = { 0 };
>  
>  	memset(&req, 0, sizeof(req));
>  
>  	if (flags & IFF_ALLMULTI) {
>  		req.req_id = RTE_KNI_REQ_CHANGE_ALLMULTI;
> -
>  		if (netdev->flags & IFF_ALLMULTI)
>  			req.allmulti = 1;
>  		else
> @@ -703,7 +711,8 @@ kni_net_change_rx_flags(struct net_device *netdev, int flags)
>  			req.promiscusity = 0;
>  	}
>  
> -	kni_net_process_request(netdev, &req);
> +	if (req.req_id)
> +		kni_net_process_request(netdev, &req);
>  }
>  
>  /*
> diff --git a/lib/kni/rte_kni.c b/lib/kni/rte_kni.c
> index d3e236005e..e921d41ce8 100644
> --- a/lib/kni/rte_kni.c
> +++ b/lib/kni/rte_kni.c
> @@ -31,6 +31,9 @@
>  #define KNI_FIFO_COUNT_MAX     1024
>  #define KNI_FIFO_SIZE          (KNI_FIFO_COUNT_MAX * sizeof(void *) + \
>  					sizeof(struct rte_kni_fifo))
> +#define KNI_REQS_SIZE          (KNI_FIFO_COUNT_MAX * \
> +					sizeof(struct rte_kni_request) + \
> +					sizeof(struct rte_kni_fifo))
>  
>  #define KNI_REQUEST_MBUF_NUM_MAX      32
>  
> @@ -175,8 +178,8 @@ kni_reserve_mz(struct rte_kni *kni)
>  	KNI_MEM_CHECK(kni->m_resp_q == NULL, resp_q_fail);
>  
>  	snprintf(mz_name, RTE_MEMZONE_NAMESIZE, KNI_SYNC_ADDR_MZ_NAME_FMT, kni->name);
> -	kni->m_sync_addr = rte_memzone_reserve(mz_name, KNI_FIFO_SIZE, SOCKET_ID_ANY,
> -			RTE_MEMZONE_IOVA_CONTIG);
> +	kni->m_sync_addr = rte_memzone_reserve(mz_name, KNI_REQS_SIZE,
> +			SOCKET_ID_ANY, RTE_MEMZONE_IOVA_CONTIG);
>  	KNI_MEM_CHECK(kni->m_sync_addr == NULL, sync_addr_fail);
>  
>  	return 0;
> @@ -307,6 +310,7 @@ rte_kni_alloc(struct rte_mempool *pktmbuf_pool,
>  	kni->sync_addr = kni->m_sync_addr->addr;
>  	dev_info.sync_va = kni->m_sync_addr->addr;
>  	dev_info.sync_phys = kni->m_sync_addr->iova;
> +	dev_info.sync_ring_size = KNI_FIFO_COUNT_MAX;
>  
>  	kni->pktmbuf_pool = pktmbuf_pool;
>  	kni->group_id = conf->group_id;
> @@ -544,11 +548,7 @@ rte_kni_handle_request(struct rte_kni *kni)
>  	if (ret != 1)
>  		return 0; /* It is OK of can not getting the request mbuf */
>  
> -	if (req != kni->sync_addr) {
> -		RTE_LOG(ERR, KNI, "Wrong req pointer %p\n", req);
> -		return -1;
> -	}
> -
> +	printf("%s: req %p id %u\n", __func__, req, req->req_id);
>  	/* Analyze the request and call the relevant actions for it */
>  	switch (req->req_id) {
>  	case RTE_KNI_REQ_CHANGE_MTU: /* Change MTU */
> diff --git a/lib/kni/rte_kni_common.h b/lib/kni/rte_kni_common.h
> index b547ea5501..a78b9bafa0 100644
> --- a/lib/kni/rte_kni_common.h
> +++ b/lib/kni/rte_kni_common.h
> @@ -110,6 +110,7 @@ struct rte_kni_device_info {
>  	phys_addr_t resp_phys;
>  	phys_addr_t sync_phys;
>  	void * sync_va;
> +	unsigned int sync_ring_size;
>  
>  	/* mbuf mempool */
>  	void * mbuf_va;
>