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

[Frederico Cadete <Frederico.Cadete-ext@oneaccess-net.com>]

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