* [dpdk-dev] [PATCH] kni: Fix request overwritten
@ 2021-09-17 12:31 Elad Nachman
2021-09-21 13:57 ` Ferruh Yigit
0 siblings, 1 reply; 2+ messages in thread
From: Elad Nachman @ 2021-09-17 12:31 UTC (permalink / raw)
To: ferruh.yigit; +Cc: dev, erclists, iryzhov, Elad Nachman
Fix lack of multiple KNI requests handling support by introducing a
rotating ring mechanism for the sync buffer.
Prevent kni_net_change_rx_flags() from calling kni_net_process_request()
with a malformed request.
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;
--
2.17.1
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [dpdk-dev] [PATCH] kni: Fix request overwritten
2021-09-17 12:31 [dpdk-dev] [PATCH] kni: Fix request overwritten Elad Nachman
@ 2021-09-21 13:57 ` Ferruh Yigit
0 siblings, 0 replies; 2+ messages in thread
From: Ferruh Yigit @ 2021-09-21 13:57 UTC (permalink / raw)
To: Elad Nachman; +Cc: dev, erclists, iryzhov
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;
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-09-21 14:15 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17 12:31 [dpdk-dev] [PATCH] kni: Fix request overwritten Elad Nachman
2021-09-21 13:57 ` Ferruh Yigit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).