Re: bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel

["Tan, Jianfeng" <jianfeng.tan@intel.com>]

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