* 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.