* bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel @ 2017-06-22 14:58 Frederico Cadete 2017-06-23 15:36 ` Tan, Jianfeng 0 siblings, 1 reply; 6+ messages in thread From: Frederico Cadete @ 2017-06-22 14:58 UTC (permalink / raw) To: yuanhan.liu, maxime.coquelin; +Cc: John Sucaet, jianfeng.tan, dev Hello, I believe commit 260aae9a [1] has introduced a regression for the case of 32-bit process running on a 64-bit kernel. The commit is effectively casting mbuf->buf_physaddr to uintptr_t before dereferencing it. It truncates the physical address to the width of the process's uint, and in the the aforementioned combination this loses important bits. I can confirm this under GDB. When virtqueue_enqueue_xmit is filling in start_dp, I get this result: (gdb) p /x cookie->buf_physaddr $5 = 0x12f94a000 (gdb) p /x start_dp[idx].addr $6 = 0x2f94a080 On my system, I capture the packet that goes out to the host and I see it has the correct size but the content is all-zeroes. I would like to propose a patch that would work for all supported combinations of user/kernel bitwidth *and* virtio-pci/virtio-user. But I don't really see how that could work, given virtio-user tries to store a physical address in rte_mbuf's "void *buf_addr", and this is not always big enough for a physical address. Any suggestions on if and how this could be fixed? Meanwhile, the bug affects dpdk 17.05, 17.02.1 and master. Users not requiring virtio-user support can avoid it by setting CONFIG_VIRTIO_USER=n during compilation. Best regards, Frederico Cadete ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel 2017-06-22 14:58 bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel Frederico Cadete @ 2017-06-23 15:36 ` Tan, Jianfeng 2017-06-26 8:14 ` Frederico Cadete 0 siblings, 1 reply; 6+ messages in thread From: Tan, Jianfeng @ 2017-06-23 15:36 UTC (permalink / raw) To: Frederico Cadete, yuanhan.liu, maxime.coquelin; +Cc: John Sucaet, dev Hi Cadete, On 6/22/2017 10:58 PM, Frederico Cadete wrote: > Hello, > > I believe commit 260aae9a [1] has introduced a regression for the case > of 32-bit process running on a 64-bit kernel. > > The commit is effectively casting mbuf->buf_physaddr to uintptr_t > before dereferencing it. It truncates the physical address to the width > of the process's uint, and in the the aforementioned combination this > loses important bits. > > I can confirm this under GDB. When virtqueue_enqueue_xmit is filling in > start_dp, I get this result: > > (gdb) p /x cookie->buf_physaddr > $5 = 0x12f94a000 > (gdb) p /x start_dp[idx].addr > $6 = 0x2f94a080 Now you are testing a virtio-pci device and app is compiled into a 32-bit executable on 64-bit VM system? > > On my system, I capture the packet that goes out to the host and I see > it has the correct size but the content is all-zeroes. > > I would like to propose a patch that would work for all supported > combinations of user/kernel bitwidth *and* virtio-pci/virtio-user. But > I don't really see how that could work, given virtio-user tries to > store a physical address in rte_mbuf's "void *buf_addr", and this is > not always big enough for a physical address. virtio-user does not store a physical address in rte_mbuf's "void *buf_addr", instead, it uses this field in rte_mbuf to fill desc's addr which is always 64bit long. > Any suggestions on if and how this could be fixed? > > Meanwhile, the bug affects dpdk 17.05, 17.02.1 and master. Users not > requiring virtio-user support can avoid it by setting > CONFIG_VIRTIO_USER=n during compilation. > > Best regards, > Frederico Cadete ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel 2017-06-23 15:36 ` Tan, Jianfeng @ 2017-06-26 8:14 ` Frederico Cadete 2017-06-26 10:15 ` Tan, Jianfeng 0 siblings, 1 reply; 6+ messages in thread From: Frederico Cadete @ 2017-06-26 8:14 UTC (permalink / raw) To: yuanhan.liu, maxime.coquelin, jianfeng.tan; +Cc: John Sucaet, dev On Fri, 2017-06-23 at 23:36 +0800, Tan, Jianfeng wrote: > Hi Cadete, > > > On 6/22/2017 10:58 PM, Frederico Cadete wrote: > > > > Hello, > > > > I believe commit 260aae9a [1] has introduced a regression for the > > case > > of 32-bit process running on a 64-bit kernel. > > > > The commit is effectively casting mbuf->buf_physaddr to uintptr_t > > before dereferencing it. It truncates the physical address to the > > width > > of the process's uint, and in the the aforementioned combination > > this > > loses important bits. > > > > I can confirm this under GDB. When virtqueue_enqueue_xmit is > > filling in > > start_dp, I get this result: > > > > (gdb) p /x cookie->buf_physaddr > > $5 = 0x12f94a000 > > (gdb) p /x start_dp[idx].addr > > $6 = 0x2f94a080 > Now you are testing a virtio-pci device and app is compiled into a > 32-bit executable on 64-bit VM system? Exactly. Furthermore, this bug is only visible if you give the virtual machine enough memory for the mbuf's physiscal address to be above the 4GB mark. > > > > > > > On my system, I capture the packet that goes out to the host and I > > see > > it has the correct size but the content is all-zeroes. > > > > I would like to propose a patch that would work for all supported > > combinations of user/kernel bitwidth *and* virtio-pci/virtio-user. > > But > > I don't really see how that could work, given virtio-user tries to > > store a physical address in rte_mbuf's "void *buf_addr", and this > > is > > not always big enough for a physical address. > virtio-user does not store a physical address in rte_mbuf's "void > *buf_addr", instead, it uses this field in rte_mbuf to fill desc's > addr > which is always 64bit long. Oh, that's right. Sorry about that. In that case I guess that the issue is that the conversion is assuming that on 32-bit apps only 4 bytes are necessary, even in the case of virtio-pci and 64-bit physaddr. Would you say that this is how vring_desc's addr should be filled? | 32-bit app | 64-bit app | ------------+-----------------------+ -----------------------+ virtio-pci | buf_physaddr, 8 bytes | buf_physaddr, 8 bytes | virtio-user | buf_addr, 4 bytes | buf_addr, 8 bytes | I believe that the issue is that after commit 260aae9a, for virtio-pci and 32-bit app it is taking 4 bytes instead of 8. > > > > > Any suggestions on if and how this could be fixed? > > > > Meanwhile, the bug affects dpdk 17.05, 17.02.1 and master. Users > > not > > requiring virtio-user support can avoid it by setting > > CONFIG_VIRTIO_USER=n during compilation. > > > > Best regards, > > Frederico Cadete ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel 2017-06-26 8:14 ` Frederico Cadete @ 2017-06-26 10:15 ` Tan, Jianfeng 2017-06-27 13:32 ` Frederico Cadete 0 siblings, 1 reply; 6+ messages in thread From: Tan, Jianfeng @ 2017-06-26 10:15 UTC (permalink / raw) To: Frederico Cadete, yuanhan.liu, maxime.coquelin; +Cc: John Sucaet, dev On 6/26/2017 4:14 PM, Frederico Cadete wrote: > On Fri, 2017-06-23 at 23:36 +0800, Tan, Jianfeng wrote: >> Hi Cadete, >> >> >> On 6/22/2017 10:58 PM, Frederico Cadete wrote: >>> Hello, >>> >>> I believe commit 260aae9a [1] has introduced a regression for the >>> case >>> of 32-bit process running on a 64-bit kernel. >>> >>> The commit is effectively casting mbuf->buf_physaddr to uintptr_t >>> before dereferencing it. It truncates the physical address to the >>> width >>> of the process's uint, and in the the aforementioned combination >>> this >>> loses important bits. >>> >>> I can confirm this under GDB. When virtqueue_enqueue_xmit is >>> filling in >>> start_dp, I get this result: >>> >>> (gdb) p /x cookie->buf_physaddr >>> $5 = 0x12f94a000 >>> (gdb) p /x start_dp[idx].addr >>> $6 = 0x2f94a080 >> Now you are testing a virtio-pci device and app is compiled into a >> 32-bit executable on 64-bit VM system? > Exactly. Furthermore, this bug is only visible if you give the virtual > machine enough memory for the mbuf's physiscal address to be above the > 4GB mark. That's an important information. > >>> >>> On my system, I capture the packet that goes out to the host and I >>> see >>> it has the correct size but the content is all-zeroes. >>> >>> I would like to propose a patch that would work for all supported >>> combinations of user/kernel bitwidth *and* virtio-pci/virtio-user. >>> But >>> I don't really see how that could work, given virtio-user tries to >>> store a physical address in rte_mbuf's "void *buf_addr", and this >>> is >>> not always big enough for a physical address. >> virtio-user does not store a physical address in rte_mbuf's "void >> *buf_addr", instead, it uses this field in rte_mbuf to fill desc's >> addr >> which is always 64bit long. > Oh, that's right. Sorry about that. > > In that case I guess that the issue is that the conversion is assuming > that on 32-bit apps only 4 bytes are necessary, even in the case of > virtio-pci and 64-bit physaddr. > > Would you say that this is how vring_desc's addr should be filled? > > | 32-bit app | 64-bit app | > ------------+-----------------------+ -----------------------+ > virtio-pci | buf_physaddr, 8 bytes | buf_physaddr, 8 bytes | > virtio-user | buf_addr, 4 bytes | buf_addr, 8 bytes | > > I believe that the issue is that after commit 260aae9a, for virtio-pci > and 32-bit app it is taking 4 bytes instead of 8. Aha, yes, that's the issue! Great analysis. After Bruce's commit 586ec205bcbbb ("mbuf: fix 64-bit address alignment in 32-bit builds"), we can fix this issue by fetching 8 bytes at all cases. But unfortunately, that commit is not back-ported to v17.02.1. I wonder if we can back-port Bruce's patch with a new patch to fix this problem? Any opinions from others? Thanks, Jianfeng > >>> Any suggestions on if and how this could be fixed? >>> >>> Meanwhile, the bug affects dpdk 17.05, 17.02.1 and master. Users >>> not >>> requiring virtio-user support can avoid it by setting >>> CONFIG_VIRTIO_USER=n during compilation. >>> >>> Best regards, >>> Frederico Cadete ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel 2017-06-26 10:15 ` Tan, Jianfeng @ 2017-06-27 13:32 ` Frederico Cadete 2017-06-28 2:55 ` Tan, Jianfeng 0 siblings, 1 reply; 6+ messages in thread From: Frederico Cadete @ 2017-06-27 13:32 UTC (permalink / raw) To: yuanhan.liu, maxime.coquelin, jianfeng.tan; +Cc: John Sucaet, dev On Mon, 2017-06-26 at 18:15 +0800, Tan, Jianfeng wrote: > On 6/26/2017 4:14 PM, Frederico Cadete wrote: > > > > On Fri, 2017-06-23 at 23:36 +0800, Tan, Jianfeng wrote: > > > > > > Hi Cadete, > > > > > > > > > On 6/22/2017 10:58 PM, Frederico Cadete wrote: > > > > > > > > Hello, > > > > > > > > I believe commit 260aae9a [1] has introduced a regression for > > > > the > > > > case > > > > of 32-bit process running on a 64-bit kernel. > > > > > > > > The commit is effectively casting mbuf->buf_physaddr to > > > > uintptr_t > > > > before dereferencing it. It truncates the physical address to > > > > the > > > > width > > > > of the process's uint, and in the the aforementioned > > > > combination > > > > this > > > > loses important bits. > > > > > > > > I can confirm this under GDB. When virtqueue_enqueue_xmit is > > > > filling in > > > > start_dp, I get this result: > > > > > > > > (gdb) p /x cookie->buf_physaddr > > > > $5 = 0x12f94a000 > > > > (gdb) p /x start_dp[idx].addr > > > > $6 = 0x2f94a080 > > > Now you are testing a virtio-pci device and app is compiled into > > > a > > > 32-bit executable on 64-bit VM system? > > Exactly. Furthermore, this bug is only visible if you give the > > virtual > > machine enough memory for the mbuf's physiscal address to be above > > the > > 4GB mark. > That's an important information. > > > > > > > > > > > > > > > > > > > > On my system, I capture the packet that goes out to the host > > > > and I > > > > see > > > > it has the correct size but the content is all-zeroes. > > > > > > > > I would like to propose a patch that would work for all > > > > supported > > > > combinations of user/kernel bitwidth *and* virtio-pci/virtio- > > > > user. > > > > But > > > > I don't really see how that could work, given virtio-user tries > > > > to > > > > store a physical address in rte_mbuf's "void *buf_addr", and > > > > this > > > > is > > > > not always big enough for a physical address. > > > virtio-user does not store a physical address in rte_mbuf's "void > > > *buf_addr", instead, it uses this field in rte_mbuf to fill > > > desc's > > > addr > > > which is always 64bit long. > > Oh, that's right. Sorry about that. > > > > In that case I guess that the issue is that the conversion is > > assuming > > that on 32-bit apps only 4 bytes are necessary, even in the case of > > virtio-pci and 64-bit physaddr. > > > > Would you say that this is how vring_desc's addr should be filled? > > > > | 32-bit app | 64-bit app | > > ------------+-----------------------+ -----------------------+ > > virtio-pci | buf_physaddr, 8 bytes | buf_physaddr, 8 bytes | > > virtio-user | buf_addr, 4 bytes | buf_addr, 8 bytes | > > > > I believe that the issue is that after commit 260aae9a, for virtio- > > pci > > and 32-bit app it is taking 4 bytes instead of 8. > Aha, yes, that's the issue! Great analysis. After Bruce's commit > 586ec205bcbbb ("mbuf: fix 64-bit address alignment in 32-bit > builds"), > we can fix this issue by fetching 8 bytes at all cases. But > unfortunately, that commit is not back-ported to v17.02.1. I don't see how changing the alignment of buf_physaddr allows fetching 8 bytes in all cases, even in the case of 32-bit virtio-user where what we need are 4 bytes from buf_addr. Am I missing something? Besides, Bruce's patch changes the memory layout of rte_mbuf. A priori that's not the kind I would like to find in an update of a stable branch :) > > I wonder if we can back-port Bruce's patch with a new patch to fix > this > problem? > > Any opinions from others? > > Thanks, > Jianfeng > > > > > > > > > > > > > > > > Any suggestions on if and how this could be fixed? > > > > > > > > Meanwhile, the bug affects dpdk 17.05, 17.02.1 and master. > > > > Users > > > > not > > > > requiring virtio-user support can avoid it by setting > > > > CONFIG_VIRTIO_USER=n during compilation. > > > > > > > > Best regards, > > > > Frederico Cadete ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel 2017-06-27 13:32 ` Frederico Cadete @ 2017-06-28 2:55 ` Tan, Jianfeng 0 siblings, 0 replies; 6+ messages in thread From: Tan, Jianfeng @ 2017-06-28 2:55 UTC (permalink / raw) To: Frederico Cadete, yuanhan.liu, maxime.coquelin; +Cc: John Sucaet, dev > -----Original Message----- > From: Frederico Cadete [mailto:Frederico.Cadete-ext@oneaccess-net.com] > Sent: Tuesday, June 27, 2017 9:32 PM > To: yuanhan.liu@linux.intel.com; maxime.coquelin@redhat.com; Tan, > Jianfeng > Cc: John Sucaet; dev@dpdk.org > Subject: Re: bug: virtio PMD sends malformed packets for 32-bit processes > on 64-bit kernel ... > > > > > > In that case I guess that the issue is that the conversion is > > > assuming > > > that on 32-bit apps only 4 bytes are necessary, even in the case of > > > virtio-pci and 64-bit physaddr. > > > > > > Would you say that this is how vring_desc's addr should be filled? > > > > > > | 32-bit app | 64-bit app | > > > ------------+-----------------------+ -----------------------+ > > > virtio-pci | buf_physaddr, 8 bytes | buf_physaddr, 8 bytes | > > > virtio-user | buf_addr, 4 bytes | buf_addr, 8 bytes | > > > > > > I believe that the issue is that after commit 260aae9a, for virtio- > > > pci > > > and 32-bit app it is taking 4 bytes instead of 8. > > Aha, yes, that's the issue! Great analysis. After Bruce's commit > > 586ec205bcbbb ("mbuf: fix 64-bit address alignment in 32-bit > > builds"), > > we can fix this issue by fetching 8 bytes at all cases. But > > unfortunately, that commit is not back-ported to v17.02.1. > > I don't see how changing the alignment of buf_physaddr allows fetching > 8 bytes in all cases, even in the case of 32-bit virtio-user where what > we need are 4 bytes from buf_addr. Am I missing something? After that, | 4-byte buf_addr | 4-byte padding | 8-byte buf_physaddr| for 32-bit system, right? I would expect 4-byte padding are all-zero. So we can just fetch all first 8-byte outside. > > Besides, Bruce's patch changes the memory layout of rte_mbuf. A priori > that's not the kind I would like to find in an update of a stable > branch :) Yes, you are right. Stable branches won't accept such change. > > > > > I wonder if we can back-port Bruce's patch with a new patch to fix > > this > > problem? > > > > Any opinions from others? > > > > Thanks, > > Jianfeng > > > > > > > > > > > > > > > > > > > > > > Any suggestions on if and how this could be fixed? > > > > > > > > > > Meanwhile, the bug affects dpdk 17.05, 17.02.1 and master. > > > > > Users > > > > > not > > > > > requiring virtio-user support can avoid it by setting > > > > > CONFIG_VIRTIO_USER=n during compilation. > > > > > > > > > > Best regards, > > > > > Frederico Cadete ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-28 2:55 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-22 14:58 bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel Frederico Cadete 2017-06-23 15:36 ` Tan, Jianfeng 2017-06-26 8:14 ` Frederico Cadete 2017-06-26 10:15 ` Tan, Jianfeng 2017-06-27 13:32 ` Frederico Cadete 2017-06-28 2:55 ` Tan, Jianfeng
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.