[ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.

[Olivier Matz]

On Fri, Nov 22, 2019 at 01:27:57PM +0100, David Marchand wrote:
> On Fri, Nov 22, 2019 at 1:15 PM Ilya Maximets <i.maximets at ovn.org> wrote:
> >
> > On 22.11.2019 12:42, David Marchand wrote:
> > > On Thu, Nov 21, 2019 at 2:59 PM Ilya Maximets <i.maximets at ovn.org> wrote:
> > >>
> > >> 'ol_flags' of DPDK mbuf could contain bits responsible for external
> > >> or indirect buffers which are not actually offload flags in a common
> > >> sense.  Clearing/copying of these flags could lead to memory leaks of
> > >> external memory chunks and crashes due to access to wrong memory.
> > >>
> > >> OVS should not clear these flags while resetting offloads and also
> > >> should not copy them to the newly allocated packets.
> > >>
> > >> This change is required to support DPDK 19.11, as some drivers may
> > >> return mbufs with these flags set.  However, it might be good to do
> > >> the same for DPDK 18.11, because these flags are present and should
> > >> be taken into account.
> > >
> > > Did you consider inverting the logic?
> > >
> > > Rather than exclude a set of flags, I would touch/copy only the flags
> > > that OVS uses/understands.
> > >
> > > When OVS uses a DPDK flag, it has an associated API and meaning wrt
> > > the rte_mbuf and the data.
> > > - when the flags are reset in OVS, that's because something has been
> > > done to the data (and the checksums and the rss hash must be
> > > reevaluated),
> > > - when a buffer is copied, OVS copies the data and the context that
> > > matters to OVS,
> > >
> > > Anything else should be discarded when copying to a new dp-packet.
> >
> > Yes, that was the first version I wanted to implement, i.e. to collect
> > all the flags that OVS really uses and clear/copy only these flags.
> >
> > But then I started to think about 'ring' ports and that we might send
> > mbufs with incorrect flags set after the packet modification to the
> > external application.  This doesn't sound good.  Because if OVS doesn't
> > use them doesn't mean that other applications doesn't.  So, I've end up
> > with the current logic with clearing everything except the memory layout
> > flags.
> >
> > Does it make sense?  What do you think?
> 
> Before sending a packet through a dpdk ring, OVS will touch the data
> and update the relevant flags as far as it is concerned.
> 
> The application can read/touch those mbuf flags as long as it complies
> with the DPDK APIs, this concerns both the flags that OVS is aware of,
> and the rest.
> So when getting this mbuf back, the flags should be valid.
> 
> After this, OVS can do what it wants on the packet, it will only touch
> the flags it understands.
> 
> What am I missing?
> 
> 
> > BTW, it might be good to have a DPDK API for offload flags clear/get/copy.
> 
> There is a rte_pktmbuf_copy that has been introduced recently, think a
> helper copies metadata too... the best is to bother Olivier :-)

There is no API for that. Currently, rte_pktmbuf_copy() does:

  mc->ol_flags = m->ol_flags & ~(IND_ATTACHED_MBUF|EXT_ATTACHED_MBUF);


> > BTW2, I don't really understand why memory layout flags are in ol_flags
> > in the first place.  I guess, there was just no room in mbuf structure?
> > Is it possible that these flags will become regular or dynamic fields?
> 
> I think it is the reason.
> Again, Olivier ?

Historically, the ol_flags field was only used for offloads. But since
the introduction of such features, "flags" would have been a better
name. This is something we could do by adding a union { flags; ol_flags; }
and remove ol_flags at next API breakage.

Olivier