Re: [PATCH v9] virtio-net: support inner header hash

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Feb 21, 2023 at 09:36:06PM +0000, Parav Pandit wrote:
> > So you are saying either live with the problem (this is best effort yes?) 
> Yes to best effort usage.

For sure something can be done to mitigate? How about randomizing the
key for example? That's in just like 1 minute of thinking. I am guessing
more can be done.


> > 
> > 
> > > > > > +\item  The user can observe the traffic information and enqueue
> > > > > > +information
> > > > > > of other normal
> > > > > > +       tunnels, and conduct targeted DoS attacks.
> > > > > Once hash_report_tunnel_types is removed, this second attack is no
> > > > > longer
> > > > applicable.
> > > > > Hence, please remove this too.
> > > >
> > > >
> > > > ?
> > > > I don't get how removing a field helps DoS.
> > > >
> > > I meant for the "observe and enqueue" part of the tunnel as not applicable.
> > 
> > Sorry still don't get it :( I don't know what is the "observe and enqueue" part of
> > the tunnel and what is not applicable. But maybe Heng Qi does.
> > 
> Tunnel type such as vxlan/gre etc is not placed in the virtio_net_hdr.
> This way the net_hdr doesn't leak such information to upper layer drivers as it cannot observe it.

What is this information driver can't observe? It sees all the packets
after all, we are not stripping tunneling headers.
I also don't really know what are upper layer drivers - for sure
layering of drivers is not covered in the spec for now so I am not sure
what do you mean by that.  The risk I mentioned is leaking the
information *on the network*.




> > > > \begin{lstlisting}  struct virtio_net_rss_config {
> > > > > >      le32 hash_types;
> > > > > > +    le32 hash_tunnel_types;
> > > > > This field is not needed as device config space advertisement for
> > > > > the support
> > > > is enough.
> > > > >
> > > > > If the intent is to enable hashing for the specific tunnel(s), an
> > > > > individual
> > > > command is better.
> > > >
> > > > new command? I am not sure why we want that. why not handle tunnels
> > > > like we do other protocols?
> > >
> > > I didn't follow.
> > > We probably discussed in another thread that to set M bits, it is wise to avoid
> > setting N other bits just to keep the command happy, where N >>> M and these
> > N have a very strong relation in hw resource setup and packet steering.
> > > Any examples of 'other protocols'?
> > 
> > #define VIRTIO_NET_HASH_TYPE_IPv4              (1 << 0)
> > #define VIRTIO_NET_HASH_TYPE_TCPv4             (1 << 1)
> > #define VIRTIO_NET_HASH_TYPE_UDPv4             (1 << 2)
> > 
> > this kind of thing.
> > 
> > I don't see how a tunnel is different fundamentally. Why does it need its own
> > field?
> 
> Driver is in control to enable/disable tunnel based inner hash acceleration only when its needed.
> This way certain data path hw parsers can be enabled/disabled.
> Without this it will be always enabled even if there may not be any user of it.
> Device has scope to optimize this flow.

I feel you misunderstand the question. Or maybe I misunderstand what you
are proposing.  So tunnels need their own bits. But why a separate field
and not just more bits along the existing ones?

-- 
MST