DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: "Dey, Souvik" <sodey@rbbn.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
	Yangchao Zhou <zhouyates@gmail.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] kni: fix possible kernel crash with va2pa
Date: Fri, 14 Jun 2019 18:41:21 +0000
Message-ID: <BN7PR03MB38928BFDFCF240C09491CBBFCDEE0@BN7PR03MB3892.namprd03.prod.outlook.com> (raw)
In-Reply-To: <9c7dcfd6-d55c-f5f6-b82c-461bc773dee4@intel.com>

Was there any update to this patch , I am also seeing kernel crash in kni_net_rx_normal dueing skb_put which is happening for chained mbufs.

--
Regards,
Souvik


From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
Sent: Wednesday, March 6, 2019 12:31 PM
To: Yangchao Zhou <zhouyates@gmail.com>; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] kni: fix possible kernel crash with va2pa

________________________________
NOTICE: This email was received from an EXTERNAL sender
________________________________

On 2/28/2019 7:30 AM, Yangchao Zhou wrote:
> va2pa depends on the physical address and virtual address offset of
> current mbuf. It may get the wrong physical address of next mbuf which
> allocated in another hugepage segment.

Hi Yangchao,

The problem you described seems valid, when current mbuf and the mbuf pointed bu
next pointer from different (huge)pages, address calculation will be wrong.

Can you able to reproduce the issue, or recognized the problem theoretically?

>
> Signed-off-by: Yangchao Zhou <zhouyates@gmail.com<mailto:zhouyates@gmail.com>>
> ---
> kernel/linux/kni/kni_net.c | 16 ++--------------
> .../eal/include/exec-env/rte_kni_common.h | 4 ++++
> lib/librte_kni/rte_kni.c | 15 ++++++++++++++-
> 3 files changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
> index 7371b6d58..caef8754f 100644
> --- a/kernel/linux/kni/kni_net.c
> +++ b/kernel/linux/kni/kni_net.c
> @@ -61,18 +61,6 @@ kva2data_kva(struct rte_kni_mbuf *m)
> return phys_to_virt(m->buf_physaddr + m->data_off);
> }
>
> -/* virtual address to physical address */
> -static void *
> -va2pa(void *va, struct rte_kni_mbuf *m)
> -{
> - void *pa;
> -
> - pa = (void *)((unsigned long)va -
> - ((unsigned long)m->buf_addr -
> - (unsigned long)m->buf_physaddr));
> - return pa;
> -}
> -
> /*
> * It can be called to process the request.
> */
> @@ -363,7 +351,7 @@ kni_net_rx_normal(struct kni_dev *kni)
> if (!kva->next)
> break;
>
> - kva = pa2kva(va2pa(kva->next, kva));
> + kva = pa2kva(kva->next_pa);
> data_kva = kva2data_kva(kva);
> }
> }
> @@ -545,7 +533,7 @@ kni_net_rx_lo_fifo_skb(struct kni_dev *kni)
> if (!kva->next)
> break;
>
> - kva = pa2kva(va2pa(kva->next, kva));
> + kva = pa2kva(kva->next_pa);
> data_kva = kva2data_kva(kva);
> }
> }
> diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> index 5afa08713..608f5c13f 100644
> --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> @@ -87,6 +87,10 @@ struct rte_kni_mbuf {
> char pad3[8] __attribute__((__aligned__(RTE_CACHE_LINE_MIN_SIZE)));
> void *pool;
> void *next;
> + union {
> + uint64_t tx_offload;
> + void *next_pa; /**< Physical address of next mbuf. */
> + };

This will cause overwrite the 'tx_offload' via 'next_pa', we don't use
tx_offload in KNI but not sure about removing potential use for future.

What do you think about converting 'm->next' to physical address before putting
them into 'rx_q', and in kernel side after data copied to skb convert 'm->next'
back to virtual address before putting it into 'free_q' ?
I think both address conversion can be possible to do, a little tricky because
address conversion should be calculated in next mbuf and previous mbuf->next in
the chain should be updated.

> };
>
> /*
> diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
> index 73aeccccf..1aaebcfa1 100644
> --- a/lib/librte_kni/rte_kni.c
> +++ b/lib/librte_kni/rte_kni.c
> @@ -353,6 +353,17 @@ va2pa(struct rte_mbuf *m)
> (unsigned long)m->buf_iova));
> }
>
> +static void *
> +va2pa_all(struct rte_mbuf *m)
> +{
> + struct rte_kni_mbuf *mbuf = (struct rte_kni_mbuf *)m;
> + while (mbuf->next) {
> + mbuf->next_pa = va2pa(mbuf->next);
> + mbuf = mbuf->next;
> + }
> + return va2pa(m);
> +}
> +
> static void
> obj_free(struct rte_mempool *mp __rte_unused, void *opaque, void *obj,
> unsigned obj_idx __rte_unused)
> @@ -550,7 +561,7 @@ rte_kni_tx_burst(struct rte_kni *kni, struct rte_mbuf **mbufs, unsigned num)
> unsigned int i;
>
> for (i = 0; i < num; i++)
> - phy_mbufs[i] = va2pa(mbufs[i]);
> + phy_mbufs[i] = va2pa_all(mbufs[i]);
>
> ret = kni_fifo_put(kni->rx_q, phy_mbufs, num);
>
> @@ -607,6 +618,8 @@ kni_allocate_mbufs(struct rte_kni *kni)
> offsetof(struct rte_kni_mbuf, pkt_len));
> RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, ol_flags) !=
> offsetof(struct rte_kni_mbuf, ol_flags));
> + RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, tx_offload) !=
> + offsetof(struct rte_kni_mbuf, tx_offload));
>
> /* Check if pktmbuf pool has been configured */
> if (kni->pktmbuf_pool == NULL) {
>


-----------------------------------------------------------------------------------------------------------------------
Notice: This e-mail together with any attachments may contain information of Ribbon Communications Inc. that
is confidential and/or proprietary for the sole use of the intended recipient.  Any review, disclosure, reliance or
distribution by others or forwarding without express permission is strictly prohibited.  If you are not the intended
recipient, please notify the sender immediately and then delete all copies, including any attachments.
-----------------------------------------------------------------------------------------------------------------------

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-28  7:30 Yangchao Zhou
2019-03-06 17:31 ` Ferruh Yigit
2019-06-14 18:41   ` Dey, Souvik [this message]
2019-03-12  9:22 ` [PATCH v2] " Yangchao Zhou
2019-03-19 18:35   ` Ferruh Yigit
2019-03-22 20:49   ` Ferruh Yigit
2019-06-18  4:06     ` [dpdk-dev] " Dey, Souvik
2019-06-18 15:48   ` Stephen Hemminger
2019-06-25 15:04   ` [dpdk-dev] [PATCH v3] " Yangchao Zhou
2019-07-02 20:07     ` [dpdk-dev] [v3] " Junxiao Shi
2019-07-10 20:11       ` Ferruh Yigit
2019-07-10 20:40         ` yoursunny
2019-07-10 21:23           ` Ferruh Yigit
2019-07-10 23:52             ` yoursunny
2019-07-10 20:09     ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
2019-07-11  7:46       ` Ferruh Yigit
2019-07-15 20:50         ` Thomas Monjalon

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BN7PR03MB38928BFDFCF240C09491CBBFCDEE0@BN7PR03MB3892.namprd03.prod.outlook.com \
    --to=sodey@rbbn.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=zhouyates@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox